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

[Lens] Integrate searchSessionId into Lens app #86297

Merged
merged 11 commits into from
Dec 29, 2020
136 changes: 134 additions & 2 deletions x-pack/plugins/lens/public/app_plugin/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ function createMockFrame(): jest.Mocked<EditorFrameInstance> {
};
}

function createMockSearchService() {
let sessionIdCounter = 1;
return {
session: {
start: jest.fn(() => `sessionId-${sessionIdCounter++}`),
clear: jest.fn(),
},
};
}

function createMockFilterManager() {
const unsubscribe = jest.fn();

Expand Down Expand Up @@ -118,11 +128,19 @@ function createMockQueryString() {
function createMockTimefilter() {
const unsubscribe = jest.fn();

let timeFilter = { from: 'now-7d', to: 'now' };
let subscriber: () => void;
return {
getTime: jest.fn(() => ({ from: 'now-7d', to: 'now' })),
setTime: jest.fn(),
getTime: jest.fn(() => timeFilter),
setTime: jest.fn((newTimeFilter) => {
timeFilter = newTimeFilter;
if (subscriber) {
subscriber();
}
}),
getTimeUpdate$: () => ({
subscribe: ({ next }: { next: () => void }) => {
subscriber = next;
return unsubscribe;
},
}),
Expand Down Expand Up @@ -209,6 +227,7 @@ describe('Lens App', () => {
return new Promise((resolve) => resolve({ id }));
}),
},
search: createMockSearchService(),
} as unknown) as DataPublicPluginStart,
storage: {
get: jest.fn(),
Expand Down Expand Up @@ -295,6 +314,7 @@ describe('Lens App', () => {
"query": "",
},
"savedQuery": undefined,
"searchSessionId": "sessionId-1",
"showNoDataPopover": [Function],
},
],
Expand Down Expand Up @@ -1072,6 +1092,53 @@ describe('Lens App', () => {
})
);
});

it('updates the searchSessionId when the user changes query or time in the search bar', () => {
const { component, frame, services } = mountWith({});
act(() =>
component.find(TopNavMenu).prop('onQuerySubmit')!({
dateRange: { from: 'now-14d', to: 'now-7d' },
query: { query: '', language: 'lucene' },
})
);
component.update();
expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
searchSessionId: `sessionId-1`,
})
);

// trigger again, this time changing just the query
act(() =>
component.find(TopNavMenu).prop('onQuerySubmit')!({
dateRange: { from: 'now-14d', to: 'now-7d' },
query: { query: 'new', language: 'lucene' },
})
);
component.update();
expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
searchSessionId: `sessionId-2`,
})
);

const indexPattern = ({ id: 'index1' } as unknown) as IIndexPattern;
const field = ({ name: 'myfield' } as unknown) as IFieldType;
act(() =>
services.data.query.filterManager.setFilters([
esFilters.buildExistsFilter(field, indexPattern),
])
);
component.update();
expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
searchSessionId: `sessionId-3`,
})
);
});
});

describe('saved query handling', () => {
Expand Down Expand Up @@ -1165,6 +1232,37 @@ describe('Lens App', () => {
);
});

it('updates the searchSessionId when the query is updated', () => {
const { component, frame } = mountWith({});
act(() => {
component.find(TopNavMenu).prop('onSaved')!({
id: '1',
attributes: {
title: '',
description: '',
query: { query: '', language: 'lucene' },
},
});
});
act(() => {
component.find(TopNavMenu).prop('onSavedQueryUpdated')!({
id: '2',
attributes: {
title: 'new title',
description: '',
query: { query: '', language: 'lucene' },
},
});
});
component.update();
expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
searchSessionId: `sessionId-1`,
})
);
});

it('clears all existing unpinned filters when the active saved query is cleared', () => {
const { component, frame, services } = mountWith({});
act(() =>
Expand All @@ -1190,6 +1288,32 @@ describe('Lens App', () => {
})
);
});

it('updates the searchSessionId when the active saved query is cleared', () => {
const { component, frame, services } = mountWith({});
act(() =>
component.find(TopNavMenu).prop('onQuerySubmit')!({
dateRange: { from: 'now-14d', to: 'now-7d' },
query: { query: 'new', language: 'lucene' },
})
);
const indexPattern = ({ id: 'index1' } as unknown) as IIndexPattern;
const field = ({ name: 'myfield' } as unknown) as IFieldType;
const pinnedField = ({ name: 'pinnedField' } as unknown) as IFieldType;
const unpinned = esFilters.buildExistsFilter(field, indexPattern);
const pinned = esFilters.buildExistsFilter(pinnedField, indexPattern);
FilterManager.setFiltersStore([pinned], esFilters.FilterStateStore.GLOBAL_STATE);
act(() => services.data.query.filterManager.setFilters([pinned, unpinned]));
component.update();
act(() => component.find(TopNavMenu).prop('onClearSavedQuery')!());
component.update();
expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
searchSessionId: `sessionId-2`,
})
);
});
});

describe('showing a confirm message when leaving', () => {
Expand Down Expand Up @@ -1316,5 +1440,13 @@ describe('Lens App', () => {
expect(confirmLeave).toHaveBeenCalled();
expect(defaultLeave).not.toHaveBeenCalled();
});

it('should clear the session when leaving the app', () => {
const { props, services } = mountWith({});
const { clear } = services.data.search.session;
const lastCall = props.onAppLeave.mock.calls[props.onAppLeave.mock.calls.length - 1][0];
lastCall({ default: defaultLeave, confirm: confirmLeave });
expect(clear).toHaveBeenCalled();
});
});
});
26 changes: 21 additions & 5 deletions x-pack/plugins/lens/public/app_plugin/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export function App({
isSaveModalVisible: false,
indicateNoData: false,
isSaveable: false,
searchSessionId: data.search.session.start(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove dateRange from the LensAppState now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the dateRange out of state in the last commit. An object reference needs to be still hold for the frame component, but there's a single source of truth now for it.

};
});

Expand Down Expand Up @@ -160,7 +161,11 @@ export function App({

const filterSubscription = data.query.filterManager.getUpdates$().subscribe({
next: () => {
setState((s) => ({ ...s, filters: data.query.filterManager.getFilters() }));
setState((s) => ({
...s,
filters: data.query.filterManager.getFilters(),
searchSessionId: data.search.session.start(),
}));
trackUiEvent('app_filters_updated');
},
});
Expand All @@ -174,6 +179,7 @@ export function App({
fromDate: currentRange.from,
toDate: currentRange.to,
},
searchSessionId: data.search.session.start(),
}));
},
});
Expand All @@ -196,6 +202,7 @@ export function App({
}, [
data.query.filterManager,
data.query.timefilter.timefilter,
data.search.session,
notifications.toasts,
uiSettings,
data.query,
Expand All @@ -205,6 +212,8 @@ export function App({

useEffect(() => {
onAppLeave((actions) => {
// Clear the session when leaving Lens
data.search.session.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely should not be adding side effects to the onAppLeave handler because of how it's called. The onAppLeave function is executed immediately, and is not executed when the user tries to leave. Basically it sets up a state that says "if the user were to leave in the future, prevent them".

What this means is that I don't think we have a reliable way to make a side effect happen as the user is leaving, and I don't think we should be adding any side effects to the onAppLeave handlers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I guess we can put it in an unmount handler (e.g. useUnmount() )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some investigation on this area:

  • during the saving process, no problem with that, as the clear can be explicitly set
  • otherwise if the user clicks on another plugin/app there's no other way to intercept it within Lens and clear the session. The session check happens before the component get unmounted leading to an error page. Note that this error page happens only on the dev environment, while in production the session gets cleared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with @flash1293 and found a simpler solution to that

// Confirm when the user has made any changes to an existing doc
// or when the user has configured something without saving
if (
Expand All @@ -231,6 +240,7 @@ export function App({
state.persistedDoc,
getLastKnownDocWithoutPinnedFilters,
application.capabilities.visualize.save,
data.search.session,
]);

// Sync Kibana breadcrumbs any time the saved document's title changes
Expand Down Expand Up @@ -428,13 +438,15 @@ export function App({
if (saveProps.returnToOrigin && redirectToOrigin) {
// disabling the validation on app leave because the document has been saved.
onAppLeave((actions) => {
data.search.session.clear();
return actions.default();
});
redirectToOrigin({ input: newInput, isCopied: saveProps.newCopyOnSave });
return;
} else if (saveProps.dashboardId && redirectToDashboard) {
// disabling the validation on app leave because the document has been saved.
onAppLeave((actions) => {
data.search.session.clear();
return actions.default();
});
redirectToDashboard(newInput, saveProps.dashboardId);
Expand Down Expand Up @@ -541,6 +553,7 @@ export function App({
if (savingPermitted && lastKnownDoc) {
// disabling the validation on app leave because the document has been saved.
onAppLeave((actions) => {
data.search.session.clear();
return actions.default();
});
runSave(
Expand Down Expand Up @@ -601,14 +614,16 @@ export function App({
data.query.timefilter.timefilter.setTime(dateRange);
trackUiEvent('app_date_change');
} else {
// Query has changed, renew the session id.
// Time change will be picked up by the time subscription
setState((s) => ({
...s,
searchSessionId: data.search.session.start(),
}));
trackUiEvent('app_query_change');
}
setState((s) => ({
...s,
dateRange: {
fromDate: dateRange.from,
toDate: dateRange.to,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

So we're using the timeFilter manager instead now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was already using the timeFilter, but this code here was covering the scenario where the data didn't change but we want the state to trigger a new render ("Refresh" click).
Previously when the dateRange values changed the state was update twice with the same content, triggering a double rendering. Now this scenario is covered by the timeFilter subscription.
In case of no dateRange change ("Refresh" button), the session id is regenerated triggering a re-render now.

We still need the dateRange state, but we can reduce the number of time we update it.

query: query || s.query,
}));
}}
Expand Down Expand Up @@ -650,6 +665,7 @@ export function App({
className="lnsApp__frame"
render={editorFrame.mount}
nativeProps={{
searchSessionId: state.searchSessionId,
dateRange: state.dateRange,
query: state.query,
filters: state.filters,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/lens/public/app_plugin/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export interface LensAppState {
savedQuery?: SavedQuery;
isSaveable: boolean;
activeData?: TableInspectorAdapter;
searchSessionId: string;
}

export interface RedirectToOriginProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ function getDefaultProps() {
},
palettes: chartPluginMock.createPaletteRegistry(),
showNoDataPopover: jest.fn(),
searchSessionId: 'sessionId',
};
}

Expand Down Expand Up @@ -264,6 +265,7 @@ describe('editor_frame', () => {
filters: [],
dateRange: { fromDate: 'now-7d', toDate: 'now' },
availablePalettes: defaultProps.palettes,
searchSessionId: 'sessionId',
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface EditorFrameProps {
query: Query;
filters: Filter[];
savedQuery?: SavedQuery;
searchSessionId: string;
onChange: (arg: {
filterableIndexPatterns: string[];
doc: Document;
Expand Down Expand Up @@ -105,7 +106,7 @@ export function EditorFrame(props: EditorFrameProps) {
dateRange: props.dateRange,
query: props.query,
filters: props.filters,

searchSessionId: props.searchSessionId,
availablePalettes: props.palettes,

addNewLayer() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('editor_frame state management', () => {
query: { query: '', language: 'lucene' },
filters: [],
showNoDataPopover: jest.fn(),
searchSessionId: 'sessionId',
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ export const InnerVisualizationWrapper = ({
padding="m"
expression={expression!}
searchContext={context}
searchSessionId={framePublicAPI.searchSessionId}
reload$={autoRefreshFetch$}
onEvent={onEvent}
onData$={onData$}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/lens/public/editor_frame_service/mocks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export function createMockFramePublicAPI(): FrameMock {
get: () => palette,
getAll: () => [palette],
},
searchSessionId: 'sessionId',
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ describe('editor_frame service', () => {
indexPatternId: '1',
fieldName: 'test',
},
searchSessionId: 'sessionId',
});
instance.unmount();
})()
Expand All @@ -78,6 +79,7 @@ describe('editor_frame service', () => {
query: { query: '', language: 'lucene' },
filters: [],
showNoDataPopover: jest.fn(),
searchSessionId: 'sessionId',
});
instance.unmount();

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/lens/public/editor_frame_service/service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export class EditorFrameService {
onChange,
showNoDataPopover,
initialContext,
searchSessionId,
}
) => {
domElement = element;
Expand Down Expand Up @@ -172,6 +173,7 @@ export class EditorFrameService {
onChange={onChange}
showNoDataPopover={showNoDataPopover}
initialContext={initialContext}
searchSessionId={searchSessionId}
/>
</I18nProvider>,
domElement
Expand Down
Loading