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] Fix legacy sort saved search stored in dashboard saved objects #137488

Merged
merged 23 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
65517f6
Refactor sort functionality
kertal Jul 28, 2022
10ed706
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 28, 2022
326009f
Improve code and add tests
kertal Jul 29, 2022
cc0c094
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 29, 2022
de3c271
Merge branch 'main' into discover-refactor-sort-functions
kertal Jul 29, 2022
32c584d
Merge branch 'main' into discover-refactor-sort-functions
kibanamachine Aug 1, 2022
135b9d5
Improve sorting types
kertal Aug 2, 2022
2dd7bef
Improve types
kertal Aug 2, 2022
c2ffbe6
Prevent duplicate of fetching
kertal Aug 2, 2022
2a51392
migrate functions to utils/sorting
kertal Aug 2, 2022
95d1c49
Rename SortOrder to SortPair
kertal Aug 2, 2022
ffe9fd9
Refactor to use just a single SortPair
kertal Aug 2, 2022
036074f
Merge remote-tracking branch 'upstream/main' into discover-refactor-s…
kertal Aug 3, 2022
615447c
add debug log
kertal Aug 3, 2022
c5f674e
Fix functional test
kertal Aug 4, 2022
289062c
Merge branch 'main' into discover-refactor-sort-functions
kibanamachine Aug 8, 2022
54a7a91
Merge main, fix conflicts
kertal Aug 10, 2022
f60ba59
Merge branch 'main' into discover-refactor-sort-functions
kibanamachine Aug 11, 2022
86746a0
Merge main, fix conflicts
kertal Aug 22, 2022
6d14142
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Aug 22, 2022
130e99b
Merge branch 'main' into discover-refactor-sort-functions
kibanamachine Aug 22, 2022
4e8cfb6
Migrate type SortPairArr to SortOrder of the saved_search plugin
kertal Aug 23, 2022
3604da6
Merge remote-tracking branch 'upstream/main' into discover-refactor-s…
kertal Aug 23, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ import { AppState } from './services/context_state';
import { SurrDocType } from './services/context';
import { MAX_CONTEXT_SIZE, MIN_CONTEXT_SIZE } from './services/constants';
import { DocTableContext } from '../../components/doc_table/doc_table_context';
import type { SortPairArr } from '../../components/doc_table/utils/get_sort';
import { useDiscoverServices } from '../../hooks/use_discover_services';
import type { DataTableRecord } from '../../types';
import type { DataTableRecord, SortPairArr } from '../../types';

export interface ContextAppContentProps {
columns: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,10 @@ import { DataDocuments$, DataDocumentsMsg, RecordRawType } from '../../hooks/use
import { AppState, GetStateReturn } from '../../services/discover_state';
import { useDataState } from '../../hooks/use_data_state';
import { DocTableInfinite } from '../../../../components/doc_table/doc_table_infinite';
import { SortPairArr } from '../../../../components/doc_table/utils/get_sort';
import { DocumentExplorerCallout } from '../document_explorer_callout';
import { DocumentExplorerUpdateCallout } from '../document_explorer_callout/document_explorer_update_callout';
import { DiscoverTourProvider } from '../../../../components/discover_tour';
import { DataTableRecord } from '../../../../types';
import { DataTableRecord, SortPairArr } from '../../../../types';
import { getRawRecordType } from '../../utils/get_raw_record_type';

const DocTableInfiniteMemoized = React.memo(DocTableInfinite);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ import { useSearchSession } from './use_search_session';
import { useDataState } from './use_data_state';
import { FetchStatus } from '../../types';
import { getDataViewAppState } from '../utils/get_switch_data_view_app_state';
import { SortPairArr } from '../../../components/doc_table/utils/get_sort';
import { DataTableRecord } from '../../../types';
import { DataTableRecord, SortPairArr } from '../../../types';

const MAX_NUM_OF_COLUMNS = 50;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { cloneDeep, isEqual } from 'lodash';
import { IUiSettingsClient } from '@kbn/core/public';
import { DataPublicPluginStart } from '@kbn/data-plugin/public';
import { Storage } from '@kbn/kibana-utils-plugin/public';
import { getDefaultSort, getSortArray } from '../../../utils/sorting';
import {
DEFAULT_COLUMNS_SETTING,
DOC_HIDE_TIME_COLUMN_SETTING,
Expand All @@ -19,7 +20,6 @@ import {
import { SavedSearch } from '../../../services/saved_searches';

import { AppState } from '../services/discover_state';
import { getDefaultSort, getSortArray } from '../../../components/doc_table';
import { CHART_HIDDEN_KEY } from '../components/chart/discover_chart';

function getDefaultColumns(savedSearch: SavedSearch, config: IUiSettingsClient) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
*/
import { isOfAggregateQueryType, Query, AggregateQuery } from '@kbn/es-query';
import type { DataView } from '@kbn/data-views-plugin/public';
import { getSortArray, SortPairArr } from '../../../components/doc_table/utils/get_sort';
import type { SortPairArr } from '../../../types';
import { getSortArray } from '../../../utils/sorting';

/**
* Helper function to remove or adapt the currently selected columns/sort to be valid with the next
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { DataViewType, DataView } from '@kbn/data-views-plugin/public';
import { SORT_DEFAULT_ORDER_SETTING } from '../../../../common';
import type { SortOrder } from '../../../services/saved_searches';
import { DiscoverServices } from '../../../build_services';
import { getSortForSearchSource } from '../../../components/doc_table';
import { getSortForSearchSource } from '../../../utils/sorting';

/**
* Helper function to update the given searchSource before fetching/sharing/persisting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,8 @@ import {
SHOW_MULTIFIELDS,
} from '../../../common';
import { DiscoverGridDocumentToolbarBtn } from './discover_grid_document_selection';
import { SortPairArr } from '../doc_table/utils/get_sort';
import { getFieldsToShow } from '../../utils/get_fields_to_show';
import type { DataTableRecord, ValueToStringConverter } from '../../types';
import type { DataTableRecord, SortPairArr, ValueToStringConverter } from '../../types';
import { useRowHeightsOptions } from '../../hooks/use_row_heights_options';
import { useDiscoverServices } from '../../hooks/use_discover_services';
import { convertValueToString } from '../../utils/convert_value_to_string';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import { i18n } from '@kbn/i18n';
import type { DataView } from '@kbn/data-views-plugin/public';

export type SortOrder = [string, string];
export interface ColumnProps {
name: string;
displayName: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import { mountWithIntl } from '@kbn/test-jest-helpers';
import type { DataView, DataViewField } from '@kbn/data-views-plugin/public';
import { TableHeader } from './table_header';
import { findTestSubject } from '@elastic/eui/lib/test';
import { SortOrder } from './helpers';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { DOC_HIDE_TIME_COLUMN_SETTING } from '../../../../../common';
import { FORMATS_UI_SETTINGS } from '@kbn/field-formats-plugin/common';
import { SortPairArr } from '../../../../types';

const defaultUiSettings = {
get: (key: string) => {
Expand Down Expand Up @@ -63,7 +63,7 @@ function getMockProps(props = {}) {
hideTimeColumn: false,
columns: ['first', 'middle', 'last'],
defaultSortOrder: 'desc',
sortOrder: [['time', 'asc']] as SortOrder[],
sortOrder: [['time', 'asc']] as SortPairArr[],
isShortDots: true,
onRemoveColumn: jest.fn(),
onChangeSortOrder: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,20 @@
import React, { useMemo } from 'react';
import type { DataView } from '@kbn/data-views-plugin/public';
import { FORMATS_UI_SETTINGS } from '@kbn/field-formats-plugin/common';
import { SortPairArr } from '../../../../types';
import { TableHeaderColumn } from './table_header_column';
import { SortOrder, getDisplayedColumns } from './helpers';
import { getDefaultSort } from '../../utils/get_default_sort';
import { getDisplayedColumns } from './helpers';
import { getDefaultSort } from '../../../../utils/sorting';
import { useDiscoverServices } from '../../../../hooks/use_discover_services';
import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../../../../common';

interface Props {
columns: string[];
dataView: DataView;
onChangeSortOrder?: (sortOrder: SortOrder[]) => void;
onChangeSortOrder?: (sortOrder: SortPairArr[]) => void;
onMoveColumn?: (name: string, index: number) => void;
onRemoveColumn?: (name: string) => void;
sortOrder: SortOrder[];
sortOrder: SortPairArr[];
}

export function TableHeader({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import React from 'react';
import { i18n } from '@kbn/i18n';
import { EuiButtonIcon, EuiToolTip, EuiIconTip } from '@elastic/eui';
import { SortOrder } from './helpers';
import { SortPairArr } from '../../../../types';
import { DocViewTableScoreSortWarning } from './score_sort_warning';

interface Props {
Expand All @@ -21,10 +21,10 @@ interface Props {
isTimeColumn: boolean;
customLabel?: string;
name: string;
onChangeSortOrder?: (sortOrder: SortOrder[]) => void;
onChangeSortOrder?: (sortOrder: SortPairArr[]) => void;
onMoveColumn?: (name: string, idx: number) => void;
onRemoveColumn?: (name: string) => void;
sortOrder: SortOrder[];
sortOrder: SortPairArr[];
}

interface IconProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ import type { DataView, DataViewField } from '@kbn/data-views-plugin/public';
import { FormattedMessage } from '@kbn/i18n-react';
import { TableHeader } from './components/table_header/table_header';
import { SHOW_MULTIFIELDS } from '../../../common';
import { SortOrder } from './components/table_header/helpers';
import { TableRow } from './components/table_row';
import { DocViewFilterFn } from '../../services/doc_views/doc_views_types';
import { getFieldsToShow } from '../../utils/get_fields_to_show';
import { useDiscoverServices } from '../../hooks/use_discover_services';
import type { DataTableRecord } from '../../types';
import type { DataTableRecord, SortPairArr } from '../../types';

export interface DocTableProps {
/**
Expand Down Expand Up @@ -149,7 +148,7 @@ export const DocTableWrapper = forwardRef(
onChangeSortOrder={onSort}
onMoveColumn={onMoveColumn}
onRemoveColumn={onRemoveColumn}
sortOrder={sort as SortOrder[]}
sortOrder={sort as SortPairArr[]}
/>
),
[columns, dataView, onMoveColumn, onRemoveColumn, onSort, sort]
Expand Down
42 changes: 19 additions & 23 deletions src/plugins/discover/public/embeddable/saved_search_embeddable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ import { ISearchSource } from '@kbn/data-plugin/public';
import { DataView, DataViewField } from '@kbn/data-views-plugin/public';
import { UiActionsStart } from '@kbn/ui-actions-plugin/public';
import { KibanaContextProvider, KibanaThemeProvider } from '@kbn/kibana-react-plugin/public';
import { getSortForEmbeddable, SortPair } from '../utils/sorting';
import { RecordRawType } from '../application/main/hooks/use_saved_search';
import { buildDataTableRecord } from '../utils/build_data_record';
import { DataTableRecord, EsHitRecord } from '../types';
import { DataTableRecord, SortPairArr, EsHitRecord } from '../types';
import { ISearchEmbeddable, SearchInput, SearchOutput } from './types';
import { SavedSearch } from '../services/saved_searches';
import { SEARCH_EMBEDDABLE_TYPE } from './constants';
Expand All @@ -53,8 +54,6 @@ import { handleSourceColumnState } from '../utils/state_helpers';
import { DiscoverGridProps } from '../components/discover_grid/discover_grid';
import { DiscoverGridSettings } from '../components/discover_grid/types';
import { DocTableProps } from '../components/doc_table/doc_table_wrapper';
import { getDefaultSort } from '../components/doc_table';
import { SortOrder } from '../components/doc_table/components/table_header/helpers';
import { VIEW_MODE } from '../components/view_mode_toggle';
import { updateSearchSource } from './utils/update_search_source';
import { FieldStatisticsTable } from '../application/main/components/field_stats_table';
Expand Down Expand Up @@ -103,6 +102,7 @@ export class SavedSearchEmbeddable
private prevTimeRange?: TimeRange;
private prevFilters?: Filter[];
private prevQuery?: Query;
private prevSort?: SortPairArr[];
private prevSearchSessionId?: string;
private searchProps?: SearchProps;

Expand Down Expand Up @@ -285,10 +285,8 @@ export class SavedSearchEmbeddable
}
};

private getDefaultSort(dataView?: DataView) {
const defaultSortOrder = this.services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING, 'desc');
const hidingTimeColumn = this.services.uiSettings.get(DOC_HIDE_TIME_COLUMN_SETTING, false);
return getDefaultSort(dataView, defaultSortOrder, hidingTimeColumn);
private getSort(sort: SortPair[] | undefined, dataView?: DataView) {
return getSortForEmbeddable(sort, dataView, this.services.uiSettings);
}

private initializeSearchEmbeddableProps() {
Expand All @@ -299,16 +297,13 @@ export class SavedSearchEmbeddable
if (!dataView) {
return;
}

if (!this.savedSearch.sort || !this.savedSearch.sort.length) {
this.savedSearch.sort = this.getDefaultSort(dataView);
}
const sort = this.getSort(this.savedSearch.sort, dataView);

const props: SearchProps = {
columns: this.savedSearch.columns,
dataView,
isLoading: false,
sort: this.getDefaultSort(dataView),
sort,
rows: [],
searchDescription: this.savedSearch.description,
description: this.savedSearch.description,
Expand Down Expand Up @@ -339,10 +334,10 @@ export class SavedSearchEmbeddable
onSetColumns: (columns: string[]) => {
this.updateInput({ columns });
},
onSort: (sort: string[][]) => {
const sortOrderArr: SortOrder[] = [];
sort.forEach((arr) => {
sortOrderArr.push(arr as SortOrder);
onSort: (nextSort: string[][]) => {
const sortOrderArr: SortPairArr[] = [];
nextSort.forEach((arr) => {
sortOrderArr.push(arr as SortPairArr);
});
this.updateInput({ sort: sortOrderArr });
},
Expand Down Expand Up @@ -400,14 +395,15 @@ export class SavedSearchEmbeddable
}

private isFetchRequired(searchProps?: SearchProps) {
if (!searchProps) {
if (!searchProps || !searchProps.dataView) {
return false;
}

return (
!onlyDisabledFiltersChanged(this.input.filters, this.prevFilters) ||
!isEqual(this.prevQuery, this.input.query) ||
!isEqual(this.prevTimeRange, this.input.timeRange) ||
!isEqual(searchProps.sort, this.input.sort || this.savedSearch.sort) ||
!isEqual(this.prevSort, this.input.sort) ||
this.prevSearchSessionId !== this.input.searchSessionId
);
}
Expand All @@ -431,12 +427,11 @@ export class SavedSearchEmbeddable
{ columns: this.input.columns || this.savedSearch.columns },
this.services.core.uiSettings
).columns;
searchProps.sort = this.getSort(
this.input.sort || this.savedSearch.sort,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this case!

I noticed one weird thing when a saved search configuration on Dashboard has legacy format for a column "sort":["extension","desc"]. It replaces the current sorting setting when trying to add another sort criteria on Dashboard (on Discover pages it works as expected)

Aug-08-2022 10-39-45
Aug-08-2022 10-41-54

Is it something we can improve?

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely! another good catch 🎣 ! thx!

Copy link
Member Author

Choose a reason for hiding this comment

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

this is interesting, because it worked for me locally ... maybe you could export the dashboard saved object and send it to me so I could reproduce? many thx!

Copy link
Member Author

Choose a reason for hiding this comment

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

thx for the saved object! So the problem here is that extension is a field that's not sortable, extension.keyword is sortable. when changing the sort, this field is removed from sorting (sorted out 😃). Since the sorting state of the UI can only be changed with manually tweaking the saved search, I think we're safe here. However there might be a potential follow up taking care of the behavior in Discover, which seems to work differently in this case.

searchProps?.dataView
);

const savedSearchSort =
this.savedSearch.sort && this.savedSearch.sort.length
? this.savedSearch.sort
: this.getDefaultSort(this.searchProps?.dataView);
searchProps.sort = this.input.sort || savedSearchSort;
searchProps.sharedItemTitle = this.panelTitle;
searchProps.rowHeightState = this.input.rowHeight || this.savedSearch.rowHeight;
searchProps.rowsPerPageState = this.input.rowsPerPage || this.savedSearch.rowsPerPage;
Expand All @@ -453,6 +448,7 @@ export class SavedSearchEmbeddable
this.prevQuery = this.input.query;
this.prevTimeRange = this.input.timeRange;
this.prevSearchSessionId = this.input.searchSessionId;
this.prevSort = this.input.sort;
this.searchProps = searchProps;
await this.fetch();
} else if (this.searchProps && this.node) {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/discover/public/embeddable/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ import {
} from '@kbn/embeddable-plugin/public';
import type { Filter, TimeRange, Query } from '@kbn/es-query';
import { DataView } from '@kbn/data-views-plugin/public';
import { SortPairArr } from '../types';
import { SavedSearch } from '../services/saved_searches';
import { SortOrder } from '../components/doc_table/components/table_header/helpers';

export interface SearchInput extends EmbeddableInput {
timeRange: TimeRange;
query?: Query;
filters?: Filter[];
hidePanelTitles?: boolean;
columns?: string[];
sort?: SortOrder[];
sort?: SortPairArr[];
rowHeight?: number;
rowsPerPage?: number;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* Side Public License, v 1.
*/
import type { DataView } from '@kbn/data-views-plugin/public';
import { ISearchSource } from '@kbn/data-plugin/public';
import { getSortForSearchSource } from '../../components/doc_table';
import { SortPairArr } from '../../components/doc_table/utils/get_sort';
import type { ISearchSource } from '@kbn/data-plugin/public';
import type { SortPairArr } from '../../types';
import { getSortForSearchSource } from '../../utils/sorting';

export const updateSearchSource = (
searchSource: ISearchSource,
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/discover/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,5 @@ export interface DataTableRecord {
*/
isAnchor?: boolean;
}

export type SortPairArr = [string, string];
2 changes: 1 addition & 1 deletion src/plugins/discover/public/utils/get_sharing_data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import type {
SerializedSearchSourceFields,
} from '@kbn/data-plugin/public';
import type { Filter } from '@kbn/es-query';
import { getSortForSearchSource } from './sorting';
import {
DOC_HIDE_TIME_COLUMN_SETTING,
SEARCH_FIELDS_FROM_SOURCE,
SORT_DEFAULT_ORDER_SETTING,
} from '../../common';
import type { SavedSearch, SortOrder } from '../services/saved_searches';
import { getSortForSearchSource } from '../components/doc_table';
import { AppState, isEqualFilters } from '../application/main/services/discover_state';

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
*/

import type { DataView } from '@kbn/data-views-plugin/public';
import { SortPairArr } from '../../types';
import { isSortable } from './get_sort';
import { SortOrder } from '../components/table_header/helpers';

/**
* use in case the user didn't manually sort.
Expand All @@ -18,7 +18,7 @@ export function getDefaultSort(
dataView: DataView | undefined,
defaultSortOrder: string = 'desc',
hidingTimeColumn: boolean = false
): SortOrder[] {
): SortPairArr[] {
if (
dataView?.timeFieldName &&
isSortable(dataView.timeFieldName, dataView) &&
Expand Down
Loading