Skip to content

Commit

Permalink
[Discover] Prevent overwriting managed content from editor (elastic#1…
Browse files Browse the repository at this point in the history
…75256)

## Summary

Close elastic#172382

I marked this a breaking change since it is preventing users from doing
something they have been able to do before. They can no longer save
changes to managed saved searches. Instead, they have to save changes to
a new saved search.

This is how the UI should look.

<img width="970" alt="Screenshot 2024-01-22 at 11 33 31 AM"
src="https://github.com/elastic/kibana/assets/315764/8cba2ca6-b13f-4c4a-a67a-cd235ca61691">

<img width="333" alt="Screenshot 2024-01-22 at 11 34 26 AM"
src="https://github.com/elastic/kibana/assets/315764/c03091bc-5130-443b-b879-e708e7a23063">


### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials — going to
happen in elastic#175150
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed —
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4939
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
drewdaemon and kibanamachine authored Jan 26, 2024
1 parent 3213541 commit e7e3498
Show file tree
Hide file tree
Showing 23 changed files with 147 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
import { getTopNavBadges } from './get_top_nav_badges';
import { discoverServiceMock } from '../../../../__mocks__/services';
import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock';
import { savedSearchMock } from '../../../../__mocks__/saved_search';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

const stateContainer = getDiscoverStateMock({ isTimeBased: true });

Expand Down Expand Up @@ -40,6 +43,41 @@ describe('getTopNavBadges()', function () {
`);
});

describe('managed saved search', () => {
const stateContainerWithManagedSavedSearch = getDiscoverStateMock({
savedSearch: { ...savedSearchMock, managed: true },
});

test('should return the managed badge when managed saved search', () => {
const topNavBadges = getTopNavBadges({
hasUnsavedChanges: false,
services: discoverServiceMock,
stateContainer: stateContainerWithManagedSavedSearch,
topNavCustomization: undefined,
});

expect(topNavBadges).toHaveLength(1);
expect(topNavBadges[0].badgeText).toEqual('Managed');
});

test('should not show save in unsaved changed badge', () => {
const topNavBadges = getTopNavBadges({
hasUnsavedChanges: true,
services: discoverServiceMock,
stateContainer: stateContainerWithManagedSavedSearch,
topNavCustomization: undefined,
});

expect(topNavBadges).toHaveLength(2);
const unsavedChangesBadge = topNavBadges[0];
expect(unsavedChangesBadge.badgeText).toEqual('Unsaved changes');

render(unsavedChangesBadge.renderCustomBadge!({ badgeText: 'Unsaved changes' }));
userEvent.click(screen.getByRole('button')); // open menu
expect(screen.queryByText('Save')).toBeNull();
});
});

test('should not return the unsaved changes badge when disabled in customization', () => {
const topNavBadges = getTopNavBadges({
hasUnsavedChanges: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import type { TopNavMenuBadgeProps } from '@kbn/navigation-plugin/public';
import { getTopNavUnsavedChangesBadge } from '@kbn/unsaved-changes-badge';
import { getManagedContentBadge } from '@kbn/managed-content-badge';
import { i18n } from '@kbn/i18n';
import { DiscoverStateContainer } from '../../services/discover_state';
import type { TopNavCustomization } from '../../../../customizations';
import { onSaveSearch } from './on_save_search';
Expand Down Expand Up @@ -38,13 +40,17 @@ export const getTopNavBadges = ({
const defaultBadges = topNavCustomization?.defaultBadges;
const entries = [...(topNavCustomization?.getBadges?.() ?? [])];

const isManaged = stateContainer.savedSearchState.getState().managed;

if (hasUnsavedChanges && !defaultBadges?.unsavedChangesBadge?.disabled) {
entries.push({
data: getTopNavUnsavedChangesBadge({
onRevert: stateContainer.actions.undoSavedSearchChanges,
onSave: async () => {
await saveSearch();
},
onSave: !isManaged
? async () => {
await saveSearch();
}
: undefined,
onSaveAs: async () => {
await saveSearch(true);
},
Expand All @@ -53,5 +59,17 @@ export const getTopNavBadges = ({
});
}

if (isManaged) {
entries.push({
data: getManagedContentBadge(
i18n.translate('discover.topNav.managedContentLabel', {
defaultMessage:
'This saved search is managed by Elastic. Changes here must be saved to a new saved search.',
})
),
order: 101,
});
}

return entries.sort((a, b) => a.order - b.order).map((entry) => entry.data);
};
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ export async function onSaveSearch({
description={savedSearch.description}
timeRestore={savedSearch.timeRestore}
tags={savedSearch.tags ?? []}
managed={savedSearch.managed}
onSave={onSave}
onClose={onClose ?? (() => {})}
/>
Expand All @@ -197,6 +198,7 @@ const SaveSearchObjectModal: React.FC<{
tags: string[];
onSave: (props: OnSaveProps & { newTimeRestore: boolean; newTags: string[] }) => void;
onClose: () => void;
managed: boolean;
}> = ({
isTimeBased,
services,
Expand All @@ -208,6 +210,7 @@ const SaveSearchObjectModal: React.FC<{
timeRestore: savedTimeRestore,
onSave,
onClose,
managed,
}) => {
const { savedObjectsTagging } = services;
const [timeRestore, setTimeRestore] = useState<boolean>(
Expand Down Expand Up @@ -277,6 +280,14 @@ const SaveSearchObjectModal: React.FC<{
options={options}
onSave={onModalSave}
onClose={onClose}
mustCopyOnSaveMessage={
managed
? i18n.translate('discover.localMenu.mustCopyOnSave', {
defaultMessage:
'This saved search is managed by Elastic. Changes here must be saved to a new saved search.',
})
: undefined
}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ describe('Test discover app state container', () => {
hideChart: true,
rowsPerPage: 250,
hideAggregatedPreview: true,
managed: false,
} as SavedSearch;

test('should return correct output', () => {
Expand Down Expand Up @@ -135,6 +136,7 @@ describe('Test discover app state container', () => {
filter: [customFilter],
query: undefined,
}),
managed: false,
};
const appState = state.getAppStateFromSavedSearch(newSavedSearchMock);
expect(appState).toMatchObject(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ describe('saved search embeddable', () => {
sort: [['message', 'asc']] as Array<[string, string]>,
searchSource,
viewMode: viewModeMockValue,
managed: false,
};
executeTriggerActions = jest.fn();
jest
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/discover/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
"@kbn/core-plugins-server",
"@kbn/shared-ux-button-toolbar",
"@kbn/serverless",
"@kbn/deeplinks-observability"
"@kbn/deeplinks-observability",
"@kbn/managed-content-badge"
],
"exclude": ["target/**/*"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ describe('kibanaContextFn', () => {
filter: [],
}),
} as unknown as SavedSearch['searchSource'],
{} as SavedSearch['sharingSavedObjectProps']
{} as SavedSearch['sharingSavedObjectProps'],
false
)
);
const args = {
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/saved_search/common/saved_searches_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export const fromSavedSearchAttributes = (
id: string | undefined,
attributes: SavedSearchAttributes,
tags: string[] | undefined,
searchSource: SavedSearch['searchSource']
searchSource: SavedSearch['searchSource'],
managed: boolean
): SavedSearch => ({
id,
searchSource,
Expand All @@ -34,4 +35,5 @@ export const fromSavedSearchAttributes = (
rowsPerPage: attributes.rowsPerPage,
sampleSize: attributes.sampleSize,
breakdownField: attributes.breakdownField,
managed,
});
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ describe('getSavedSearch', () => {
"hideChart": false,
"id": "ccf1af80-2297-11ec-86e0-1155ffb9c7a7",
"isTextBasedQuery": undefined,
"managed": false,
"references": Array [
Object {
"id": "ff959d40-b880-11e8-a6d9-e546fe2bba5f",
Expand Down Expand Up @@ -200,6 +201,7 @@ describe('getSavedSearch', () => {
"hideChart": true,
"id": "ccf1af80-2297-11ec-86e0-1155ffb9c7a7",
"isTextBasedQuery": true,
"managed": false,
"references": Array [
Object {
"id": "ff959d40-b880-11e8-a6d9-e546fe2bba5f",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,13 @@ export const convertToSavedSearch = async (
attributes,
references,
sharingSavedObjectProps,
managed,
}: {
savedSearchId: string | undefined;
attributes: SavedSearchAttributes;
references: Reference[];
sharingSavedObjectProps: SavedSearch['sharingSavedObjectProps'];
managed: boolean | undefined;
},
{ searchSourceCreate, savedObjectsTagging }: GetSavedSearchDependencies
) => {
Expand All @@ -92,7 +94,8 @@ export const convertToSavedSearch = async (
tags,
references,
await searchSourceCreate(searchSourceValues),
sharingSavedObjectProps
sharingSavedObjectProps,
Boolean(managed)
);

return returnVal;
Expand All @@ -106,6 +109,7 @@ export const getSavedSearch = async (savedSearchId: string, deps: GetSavedSearch
attributes: so.item.attributes,
references: so.item.references,
sharingSavedObjectProps: so.meta,
managed: so.item.managed,
},
deps
);
Expand All @@ -124,4 +128,5 @@ export const getNewSavedSearch = ({
searchSource: ISearchStartSearchSource;
}): SavedSearch => ({
searchSource: searchSource.createEmpty(),
managed: false,
});
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ describe('saved_searches_utils', () => {
['tags-1', 'tags-2'],
[],
createSearchSourceMock(),
{}
{},
false
)
).toMatchInlineSnapshot(`
Object {
Expand All @@ -52,6 +53,7 @@ describe('saved_searches_utils', () => {
"hideChart": true,
"id": "id",
"isTextBasedQuery": false,
"managed": false,
"references": Array [],
"refreshInterval": undefined,
"rowHeight": undefined,
Expand Down Expand Up @@ -106,6 +108,7 @@ describe('saved_searches_utils', () => {
hideChart: true,
isTextBasedQuery: true,
usesAdHocDataView: false,
managed: false,
};

expect(toSavedSearchAttributes(savedSearch, '{}')).toMatchInlineSnapshot(`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ export const fromSavedSearchAttributes = (
tags: string[] | undefined,
references: SavedObjectReference[] | undefined,
searchSource: SavedSearch['searchSource'],
sharingSavedObjectProps: SavedSearch['sharingSavedObjectProps']
sharingSavedObjectProps: SavedSearch['sharingSavedObjectProps'],
managed: boolean
): SavedSearch => ({
...fromSavedSearchAttributesCommon(id, attributes, tags, searchSource),
...fromSavedSearchAttributesCommon(id, attributes, tags, searchSource, managed),
sharingSavedObjectProps,
references,
});
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/saved_search/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export interface SavedSearch {
rowsPerPage?: number;
sampleSize?: number;
breakdownField?: string;
// Whether or not this saved search is managed by the system
managed: boolean;
references?: SavedObjectReference[];
sharingSavedObjectProps?: {
outcome?: SavedObjectsResolveResponse['outcome'];
Expand Down
1 change: 1 addition & 0 deletions src/plugins/saved_search/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const savedSearchStartMock = (): SavedSearchPublicPluginStart => ({
id,
title: result.attributes.title,
searchSource: createEmptySearchSource(),
managed: false,
})
),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ describe('saveSavedSearch', () => {
sharingSavedObjectProps: {
outcome: 'aliasMatch',
},
managed: false,
} as SavedSearch;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ describe('getSavedSearchAttributeService', () => {
"hideChart": false,
"id": "saved-object-id",
"isTextBasedQuery": false,
"managed": false,
"references": Array [
Object {
"id": "1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { createGetSavedSearchDeps } from './create_get_saved_search_deps';

export interface SavedSearchUnwrapMetaInfo {
sharingSavedObjectProps: SavedSearch['sharingSavedObjectProps'];
managed: boolean | undefined;
}

export interface SavedSearchUnwrapResult {
Expand Down Expand Up @@ -72,6 +73,7 @@ export function getSavedSearchAttributeService(
},
metaInfo: {
sharingSavedObjectProps: so.meta,
managed: so.item.managed,
},
};
},
Expand All @@ -91,13 +93,14 @@ export const toSavedSearch = async (
result: SavedSearchUnwrapResult,
services: SavedSearchesServiceDeps
) => {
const { sharingSavedObjectProps } = result.metaInfo ?? {};
const { sharingSavedObjectProps, managed } = result.metaInfo ?? {};

return await convertToSavedSearch(
{
...splitReferences(result.attributes),
savedSearchId: id,
sharingSavedObjectProps,
managed,
},
createGetSavedSearchDeps(services)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const getSavedSearch = async (savedSearchId: string, deps: GetSavedSearch
savedSearchId,
savedSearch.attributes,
undefined,
await deps.searchSourceStart.create(searchSourceValues)
await deps.searchSourceStart.create(searchSourceValues),
Boolean(savedSearch.managed)
);
};
4 changes: 4 additions & 0 deletions test/functional/fixtures/kbn_archiver/managed_content.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@
{"attributes":{"description":"","state":{"adHocDataViews":{},"datasourceStates":{"formBased":{"layers":{"e633b1af-3ab4-4bf5-8faa-fefde06c4a4a":{"columnOrder":["f2555a1a-6f93-43fd-bc63-acdfadd47729","d229daf9-9658-4579-99af-01d8adb2f25f"],"columns":{"d229daf9-9658-4579-99af-01d8adb2f25f":{"dataType":"number","isBucketed":false,"label":"Median of bytes","operationType":"median","params":{"emptyAsNull":true},"scale":"ratio","sourceField":"bytes"},"f2555a1a-6f93-43fd-bc63-acdfadd47729":{"dataType":"date","isBucketed":true,"label":"@timestamp","operationType":"date_histogram","params":{"dropPartials":false,"includeEmptyRows":true,"interval":"auto"},"scale":"interval","sourceField":"@timestamp"}},"incompleteColumns":{},"sampling":1}}},"indexpattern":{"layers":{}},"textBased":{"layers":{}}},"filters":[],"internalReferences":[],"query":{"language":"kuery","query":""},"visualization":{"axisTitlesVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"fittingFunction":"None","gridlinesVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"labelsOrientation":{"x":0,"yLeft":0,"yRight":0},"layers":[{"accessors":["d229daf9-9658-4579-99af-01d8adb2f25f"],"layerId":"e633b1af-3ab4-4bf5-8faa-fefde06c4a4a","layerType":"data","position":"top","seriesType":"bar_stacked","showGridlines":false,"xAccessor":"f2555a1a-6f93-43fd-bc63-acdfadd47729"}],"legend":{"isVisible":true,"position":"right"},"preferredSeriesType":"bar_stacked","tickLabelsVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"valueLabels":"hide"}},"title":"Lens vis (managed)","visualizationType":"lnsXY"},"coreMigrationVersion":"8.8.0","created_at":"2024-01-18T17:42:12.920Z","id":"managed-36db-4a3b-a4ba-7a64ab8f130b","managed":true,"references":[{"id":"5f863f70-4728-4e8d-b441-db08f8c33b28","name":"indexpattern-datasource-layer-e633b1af-3ab4-4bf5-8faa-fefde06c4a4a","type":"index-pattern"}],"type":"lens","typeMigrationVersion":"8.9.0","updated_at":"2024-01-18T17:42:12.920Z","version":"WzQ1LDFd"}

{"attributes":{"description":"","state":{"adHocDataViews":{},"datasourceStates":{"formBased":{"layers":{"e633b1af-3ab4-4bf5-8faa-fefde06c4a4a":{"columnOrder":["f2555a1a-6f93-43fd-bc63-acdfadd47729","d229daf9-9658-4579-99af-01d8adb2f25f"],"columns":{"d229daf9-9658-4579-99af-01d8adb2f25f":{"dataType":"number","isBucketed":false,"label":"Median of bytes","operationType":"median","params":{"emptyAsNull":true},"scale":"ratio","sourceField":"bytes"},"f2555a1a-6f93-43fd-bc63-acdfadd47729":{"dataType":"date","isBucketed":true,"label":"@timestamp","operationType":"date_histogram","params":{"dropPartials":false,"includeEmptyRows":true,"interval":"auto"},"scale":"interval","sourceField":"@timestamp"}},"incompleteColumns":{},"sampling":1}}},"indexpattern":{"layers":{}},"textBased":{"layers":{}}},"filters":[],"internalReferences":[],"query":{"language":"kuery","query":""},"visualization":{"axisTitlesVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"fittingFunction":"None","gridlinesVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"labelsOrientation":{"x":0,"yLeft":0,"yRight":0},"layers":[{"accessors":["d229daf9-9658-4579-99af-01d8adb2f25f"],"layerId":"e633b1af-3ab4-4bf5-8faa-fefde06c4a4a","layerType":"data","position":"top","seriesType":"bar_stacked","showGridlines":false,"xAccessor":"f2555a1a-6f93-43fd-bc63-acdfadd47729"}],"legend":{"isVisible":true,"position":"right"},"preferredSeriesType":"bar_stacked","tickLabelsVisibilitySettings":{"x":true,"yLeft":true,"yRight":true},"valueLabels":"hide"}},"title":"Lens vis (unmanaged)","visualizationType":"lnsXY"},"coreMigrationVersion":"8.8.0","created_at":"2024-01-18T17:42:12.920Z","id":"unmanaged-36db-4a3b-a4ba-7a64ab8f130b","managed":false,"references":[{"id":"5f863f70-4728-4e8d-b441-db08f8c33b28","name":"indexpattern-datasource-layer-e633b1af-3ab4-4bf5-8faa-fefde06c4a4a","type":"index-pattern"}],"type":"lens","typeMigrationVersion":"8.9.0","updated_at":"2024-01-18T17:42:12.920Z","version":"WzQ1LDFd"}

{"attributes":{"columns":["@tags","clientip"],"description":"","grid":{},"hideChart":false,"isTextBasedQuery":false,"kibanaSavedObjectMeta":{"searchSourceJSON":"{\"query\":{\"query\":\"agent.raw:\\\"Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)\\\" \",\"language\":\"kuery\"},\"filter\":[],\"indexRefName\":\"kibanaSavedObjectMeta.searchSourceJSON.index\"}"},"sort":[["@timestamp","desc"]],"timeRestore":false,"title":"Saved search","usesAdHocDataView":false},"coreMigrationVersion":"8.8.0","created_at":"2024-01-22T18:11:05.016Z","id":"managed-3d62-4113-ac7c-de2e20a68fbc","managed":true,"references":[{"id":"5f863f70-4728-4e8d-b441-db08f8c33b28","name":"kibanaSavedObjectMeta.searchSourceJSON.index","type":"index-pattern"}],"type":"search","typeMigrationVersion":"8.0.0","updated_at":"2024-01-22T18:11:05.016Z","version":"WzIzLDFd"}

{"attributes":{"columns":["@tags","clientip"],"description":"","grid":{},"hideChart":false,"isTextBasedQuery":false,"kibanaSavedObjectMeta":{"searchSourceJSON":"{\"query\":{\"query\":\"agent.raw:\\\"Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)\\\" \",\"language\":\"kuery\"},\"filter\":[],\"indexRefName\":\"kibanaSavedObjectMeta.searchSourceJSON.index\"}"},"sort":[["@timestamp","desc"]],"timeRestore":false,"title":"Saved search","usesAdHocDataView":false},"coreMigrationVersion":"8.8.0","created_at":"2024-01-22T18:11:05.016Z","id":"unmanaged-3d62-4113-ac7c-de2e20a68fbc","managed":false,"references":[{"id":"5f863f70-4728-4e8d-b441-db08f8c33b28","name":"kibanaSavedObjectMeta.searchSourceJSON.index","type":"index-pattern"}],"type":"search","typeMigrationVersion":"8.0.0","updated_at":"2024-01-22T18:11:05.016Z","version":"WzIzLDFd"}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ describe('createSearchItems', () => {
[],
{
getField: getFieldMock(searchSource),
} as unknown as ISearchSource
} as unknown as ISearchSource,
false
);

test('should match data view', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('savedSearchComparator', () => {
index: dataViewMock,
query: customQuery,
}),
managed: false,
};

it('should result true when saved search is same', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ describe('copyTimeline', () => {
index: dataViewMock,
query: customQuery,
}),
managed: false,
};

beforeAll(() => {
Expand Down
Loading

0 comments on commit e7e3498

Please sign in to comment.