Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Show unmapped fields by default and persist setting #91783

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions src/plugins/discover/public/application/angular/discover.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,13 +412,15 @@ function discoverController($route, $scope, Promise) {
index: $scope.indexPattern.id,
interval: 'auto',
filters: _.cloneDeep(persistentSearchSource.getOwnField('filter')),
hideUnmapped: false,
};
if (savedSearch.grid) {
defaultState.grid = savedSearch.grid;
}
if (savedSearch.hideChart) {
defaultState.hideChart = savedSearch.hideChart;
}
defaultState.hideUnmapped = savedSearch.hideUnmapped;
Comment on lines +415 to +423
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think it makes sense not to set hideUnmapped in default state unless it is explicitly set in savedSearch? This way it would also not become part of the URL unless the user changes the switch. Think the currently functional failure would be fixes without editing any expect, and we would not add a variable to URL without user action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's reasonable.


return defaultState;
}
Expand Down Expand Up @@ -675,21 +677,12 @@ function discoverController($route, $scope, Promise) {
history.push('/');
};

const showUnmappedFieldsDefaultValue = $scope.useNewFieldsApi && !!$scope.opts.savedSearch.pre712;
let showUnmappedFields = showUnmappedFieldsDefaultValue;

const onChangeUnmappedFields = (value) => {
showUnmappedFields = value;
$scope.unmappedFieldsConfig.showUnmappedFields = value;
$scope.onChangeUnmappedFields = (value) => {
$scope.state.hideUnmapped = value;
$scope.opts.setAppState({ ...$scope.state, hideUnmapped: value });
$scope.fetch();
};

$scope.unmappedFieldsConfig = {
showUnmappedFieldsDefaultValue,
showUnmappedFields,
onChangeUnmappedFields,
};

$scope.updateDataSource = () => {
const { indexPattern, useNewFieldsApi } = $scope;
const { columns, sort } = $scope.state;
Expand All @@ -701,7 +694,7 @@ function discoverController($route, $scope, Promise) {
sort,
columns,
useNewFieldsApi,
showUnmappedFields,
hideUnmappedFields: $scope.state.hideUnmapped,
});
return Promise.resolve();
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
time-range="timeRange"
top-nav-menu="topNavMenu"
use-new-fields-api="useNewFieldsApi"
unmapped-fields-config="unmappedFieldsConfig"
on-change-unmapped-fields="onChangeUnmappedFields"
>
</discover>
</discover-app>
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ export interface AppState {
* id of the used saved query
*/
savedQuery?: string;
/**
* Used with new fields API. Affects the field list and doc views. The default
* hidden state will likely change in a future release
*/
hideUnmapped?: boolean;
}

interface GetStateParams {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ export function createTableRowDirective($compile: ng.ICompileService) {
);
};

// create a tr element that lists the value for each *column*
// create a tr element that lists the value for each *column*. this summary is not updated
// as part of the normal angular update process, it is only updated when the ng-repeat setup
// in doc_table.html causes it to update by reference.
function createSummaryRow(row: any) {
const indexPattern = $scope.indexPattern;
$scope.flattenedRow = indexPattern.flattenHit(row);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface DocTableLegacyProps {
sampleSize: number;
sort?: string[][];
useNewFieldsApi?: boolean;
hideUnmapped?: boolean;
}
export interface AngularDirective {
template: string;
Expand Down Expand Up @@ -81,7 +82,8 @@ function getRenderFn(domNode: Element, props: any) {
on-remove-column="renderProps.onRemoveColumn"
render-complete
use-new-fields-api="renderProps.useNewFieldsApi"
sorting="renderProps.sort"></doc_table>`,
sorting="renderProps.sort"
hide-unmapped="renderProps.hideUnmapped"></doc_table>`,
};

return async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
on-remove-column="onRemoveColumn"
></thead>
<tbody>
<tr ng-repeat="row in pageOfItems|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing"
<tr ng-repeat="row in pageOfItems|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing+hideUnmapped"
kbn-table-row="row"
columns="columns"
sorting="sorting"
Expand Down Expand Up @@ -87,7 +87,7 @@
on-remove-column="onRemoveColumn"
></thead>
<tbody>
<tr ng-repeat="row in hits|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing"
<tr ng-repeat="row in hits|limitTo:limit track by row._index+row._type+row._id+row._score+row._version+row._routing+hideUnmapped"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a functional test for this because it was super weird to me. The cell contents don't update unless one of these properties is updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this seems to be the concatenated variable Angular used to track updates, great that you've fixed it and even greater you've added a functional test for it 👍

kbn-table-row="row"
columns="columns"
sorting="sorting"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export function createDocTableDirective(pagerFactory: any, $filter: any) {
onRemoveColumn: '=?',
inspectorAdapters: '=?',
useNewFieldsApi: '<',
hideUnmapped: '<',
},
link: ($scope: LazyScope, $el: JQuery) => {
$scope.persist = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ export function createDiscoverDirective(reactDirective: any) {
['topNavMenu', { watchDepth: 'reference' }],
['updateQuery', { watchDepth: 'reference' }],
['updateSavedQueryId', { watchDepth: 'reference' }],
['unmappedFieldsConfig', { watchDepth: 'value' }],
['onChangeUnmappedFields', { watchDepth: 'reference' }],
]);
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ function getProps(indexPattern: IndexPattern): DiscoverProps {
rows: esHits,
searchSource: searchSourceMock,
state: { columns: [] },
onChangeUnmappedFields: jest.fn(),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export function Discover({
searchSource,
state,
timeRange,
unmappedFieldsConfig,
onChangeUnmappedFields,
}: DiscoverProps) {
const [expandedDoc, setExpandedDoc] = useState<ElasticSearchHit | undefined>(undefined);
const scrollableDesktop = useRef<HTMLDivElement>(null);
Expand Down Expand Up @@ -230,7 +230,7 @@ export function Discover({
state={state}
isClosed={isSidebarClosed}
trackUiMetric={trackUiMetric}
unmappedFieldsConfig={unmappedFieldsConfig}
onChangeUnmappedFields={onChangeUnmappedFields}
useNewFieldsApi={useNewFieldsApi}
/>
</EuiFlexItem>
Expand Down Expand Up @@ -384,6 +384,7 @@ export function Discover({
onSort={onSort}
sampleSize={opts.sampleSize}
useNewFieldsApi={useNewFieldsApi}
hideUnmapped={state.hideUnmapped}
/>
)}
{!isLegacy && rows && rows.length && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,26 @@ describe('DiscoverFieldSearch', () => {
const onChangeUnmappedFields = jest.fn();
const componentProps = {
...defaultProps,
showUnmappedFields: true,
useNewFieldsApi: false,
useNewFieldsApi: true,
hideUnmappedFields: false,
onChangeUnmappedFields,
};
const component = mountComponent(componentProps);
const btn = findTestSubject(component, 'toggleFieldFilterButton');
btn.simulate('click');
const unmappedFieldsSwitch = findTestSubject(component, 'unmappedFieldsSwitch');
act(() => {
unmappedFieldsSwitch.simulate('click');
});
expect(onChangeUnmappedFields).toHaveBeenCalledWith(true);
});

test('enabling unmapped fields', () => {
const onChangeUnmappedFields = jest.fn();
const componentProps = {
...defaultProps,
useNewFieldsApi: true,
hideUnmappedFields: true,
onChangeUnmappedFields,
};
const component = mountComponent(componentProps);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export interface State {
aggregatable: string;
type: string;
missing: boolean;
unmappedFields: boolean;
[index: string]: string | boolean;
}

Expand Down Expand Up @@ -69,9 +68,9 @@ export interface Props {
onChangeUnmappedFields?: (value: boolean) => void;

/**
* should unmapped fields switch be rendered
* state of the unmapped flag, externally controlled
*/
showUnmappedFields?: boolean;
hideUnmappedFields?: boolean;
}

/**
Expand All @@ -83,7 +82,7 @@ export function DiscoverFieldSearch({
value,
types,
useNewFieldsApi,
showUnmappedFields,
hideUnmappedFields,
onChangeUnmappedFields,
}: Props) {
const searchPlaceholder = i18n.translate('discover.fieldChooser.searchPlaceHolder', {
Expand Down Expand Up @@ -111,14 +110,8 @@ export function DiscoverFieldSearch({
aggregatable: 'any',
type: 'any',
missing: true,
unmappedFields: !!showUnmappedFields,
});

if (typeof value !== 'string') {
// at initial rendering value is undefined (angular related), this catches the warning
// should be removed once all is react
return null;
}
const [hideUnmapped, setUnmapped] = useState(Boolean(hideUnmappedFields));

const filterBtnAriaLabel = isPopoverOpen
? i18n.translate('discover.fieldChooser.toggleFieldFilterButtonHideAriaLabel', {
Expand Down Expand Up @@ -182,8 +175,9 @@ export function DiscoverFieldSearch({
};

const handleUnmappedFieldsChange = (e: EuiSwitchEvent) => {
const unmappedFieldsValue = e.target.checked;
handleValueChange('unmappedFields', unmappedFieldsValue);
// The checkbox is the opposite of the stored value
const unmappedFieldsValue = !e.target.checked;
setUnmapped(unmappedFieldsValue);
if (onChangeUnmappedFields) {
onChangeUnmappedFields(unmappedFieldsValue);
}
Expand Down Expand Up @@ -262,46 +256,44 @@ export function DiscoverFieldSearch({
};

const footer = () => {
if (!showUnmappedFields && useNewFieldsApi) {
return null;
}
return (
<EuiPopoverFooter>
{showUnmappedFields ? (
<EuiFlexGroup>
<EuiFlexItem component="span">
<EuiSwitch
label={i18n.translate('discover.fieldChooser.filter.showUnmappedFields', {
defaultMessage: 'Show unmapped fields',
})}
checked={values.unmappedFields}
onChange={handleUnmappedFieldsChange}
data-test-subj="unmappedFieldsSwitch"
/>
</EuiFlexItem>
<EuiFlexItem component="span" grow={false}>
<EuiToolTip
position="right"
content={i18n.translate('discover.fieldChooser.filter.unmappedFieldsWarning', {
defaultMessage:
'Unmapped fields will be deprecated and removed in a future release.',
})}
>
<EuiIcon type="alert" />
</EuiToolTip>
</EuiFlexItem>
</EuiFlexGroup>
<EuiSwitch
label={i18n.translate('discover.fieldChooser.filter.hideMissingFieldsLabel', {
defaultMessage: 'Hide missing fields',
})}
checked={values.missing}
onChange={handleMissingChange}
data-test-subj="missingSwitch"
/>
{useNewFieldsApi ? (
<>
<EuiSpacer size="s" />
<EuiFlexGroup>
<EuiFlexItem component="span">
<EuiSwitch
label={i18n.translate('discover.fieldChooser.filter.showUnmappedFields', {
defaultMessage: 'Show unmapped fields',
})}
checked={!hideUnmapped}
onChange={handleUnmappedFieldsChange}
data-test-subj="unmappedFieldsSwitch"
/>
</EuiFlexItem>
<EuiFlexItem component="span" grow={false}>
<EuiToolTip
position="right"
content={i18n.translate('discover.fieldChooser.filter.unmappedFieldsWarning', {
defaultMessage:
'Unmapped fields will be deprecated and removed in a future release.',
})}
>
<EuiIcon type="alert" />
</EuiToolTip>
</EuiFlexItem>
</EuiFlexGroup>
</>
) : null}
{useNewFieldsApi ? null : (
<EuiSwitch
label={i18n.translate('discover.fieldChooser.filter.hideMissingFieldsLabel', {
defaultMessage: 'Hide missing fields',
})}
checked={values.missing}
onChange={handleMissingChange}
data-test-subj="missingSwitch"
/>
)}
</EuiPopoverFooter>
);
};
Expand All @@ -322,6 +314,12 @@ export function DiscoverFieldSearch({
</div>
);

if (typeof value !== 'string') {
// at initial rendering value is undefined (angular related), this catches the warning
// should be removed once all is react
return null;
}

return (
<React.Fragment>
<EuiFlexGroup responsive={false} gutterSize={'s'}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ function getCompProps(): DiscoverSidebarProps {
fieldFilter: getDefaultFieldFilter(),
setFieldFilter: jest.fn(),
setAppState: jest.fn(),
onChangeUnmappedFields: jest.fn(),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export function DiscoverSidebar({
trackUiMetric,
useNewFieldsApi = false,
useFlyout = false,
unmappedFieldsConfig,
onChangeUnmappedFields,
}: DiscoverSidebarProps) {
const [fields, setFields] = useState<IndexPatternField[] | null>(null);

Expand Down Expand Up @@ -99,17 +99,9 @@ export function DiscoverSidebar({
fieldCounts,
fieldFilter,
useNewFieldsApi,
!!unmappedFieldsConfig?.showUnmappedFields
state.hideUnmapped
),
[
fields,
columns,
popularLimit,
fieldCounts,
fieldFilter,
useNewFieldsApi,
unmappedFieldsConfig?.showUnmappedFields,
]
[fields, columns, popularLimit, fieldCounts, fieldFilter, useNewFieldsApi, state.hideUnmapped]
);

const fieldTypes = useMemo(() => {
Expand Down Expand Up @@ -205,8 +197,8 @@ export function DiscoverSidebar({
value={fieldFilter.name}
types={fieldTypes}
useNewFieldsApi={useNewFieldsApi}
onChangeUnmappedFields={unmappedFieldsConfig?.onChangeUnmappedFields}
showUnmappedFields={unmappedFieldsConfig?.showUnmappedFieldsDefaultValue}
hideUnmappedFields={state.hideUnmapped}
onChangeUnmappedFields={onChangeUnmappedFields}
/>
</form>
</EuiFlexItem>
Expand Down
Loading