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.

Sign up for GitHub

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

Jump to bottom

[7.6.1] [Lens] Fix bugs in Lens filters (#56441) #56648

Merged
merged 14 commits into from
Feb 14, 2020
Merged

[7.6.1] [Lens] Fix bugs in Lens filters (#56441) #56648

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b8c7cc4
[Lens] Fix bugs in Lens filters (#56441)
Feb 3, 2020
7af76da
Merge branch '7.6' into backport/7.6/pr-56441
elasticmachine Feb 3, 2020
1659294
Merge branch '7.6' into backport/7.6/pr-56441
elasticmachine Feb 3, 2020
c2f0725
Merge remote-tracking branch 'origin/7.6' into lens/7.6-filters
wylieconlon Feb 4, 2020
2d7eef3
Fix bugs
wylieconlon Feb 4, 2020
7011360
Merge branch 'backport/7.6/pr-56441' of github.com:wylieconlon/kibana…
wylieconlon Feb 4, 2020
4fff8e8
Update
wylieconlon Feb 6, 2020
4880d6b
Merge remote-tracking branch 'origin/7.6' into lens/7.6-filters
wylieconlon Feb 6, 2020
71144c6
Fix infinite loop
wylieconlon Feb 6, 2020
b6f5d4e
Actually fix infinite loop due to referential equality
wylieconlon Feb 6, 2020
3772ea1
Merge remote-tracking branch 'origin/7.6' into lens/7.6-filters
wylieconlon Feb 12, 2020
19bd60a
Update snapshot
wylieconlon Feb 12, 2020
7870e5b
Build filters explicitly
wylieconlon Feb 14, 2020
424a8b6
Merge remote-tracking branch 'origin/7.6' into lens/7.6-filters
wylieconlon Feb 14, 2020
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
90 changes: 81 additions & 9 deletions x-pack/legacy/plugins/lens/public/app_plugin/app.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,18 @@ function createMockFilterManager() {
const unsubscribe = jest.fn();

let subscriber: () => void;
let filters: unknown = [];
let filters: esFilters.Filter[] = [
{
meta: { alias: null, negate: false, disabled: false },
query: { match_phrase: { global: 'yes' } },
$state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
},
{
meta: { alias: null, negate: false, disabled: false },
query: { match_phrase: { test: 'yes' } },
$state: { store: esFilters.FilterStateStore.APP_STATE },
},
];

return {
getUpdates$: () => ({
Expand All @@ -55,11 +66,15 @@ function createMockFilterManager() {
return unsubscribe;
},
}),
setFilters: (newFilters: unknown[]) => {
setFilters: jest.fn((newFilters: esFilters.Filter[]) => {
filters = newFilters;
subscriber();
},
if (subscriber) subscriber();
}),
getFilters: () => filters,
getAppFilters: () =>
filters.filter(filter => filter.$state?.store === esFilters.FilterStateStore.APP_STATE),
getGlobalFilters: () =>
filters.filter(filter => filter.$state?.store === esFilters.FilterStateStore.GLOBAL_STATE),
removeAll: () => {
filters = [];
subscriber();
Expand Down Expand Up @@ -166,7 +181,38 @@ describe('Lens App', () => {
"toDate": "now",
},
"doc": undefined,
"filters": Array [],
"filters": Array [
Object {
"$state": Object {
"store": "globalState",
},
"meta": Object {
"alias": null,
"disabled": false,
"negate": false,
},
"query": Object {
"match_phrase": Object {
"global": "yes",
},
},
},
Object {
"$state": Object {
"store": "appState",
},
"meta": Object {
"alias": null,
"disabled": false,
"negate": false,
},
"query": Object {
"match_phrase": Object {
"test": "yes",
},
},
},
],
"onChange": [Function],
"onError": [Function],
"query": Object {
Expand All @@ -180,6 +226,15 @@ describe('Lens App', () => {
`);
});

it('clears app filters on load', () => {
const defaultArgs = makeDefaultArgs();
mount(<App {...defaultArgs} />);

expect(defaultArgs.data.query.filterManager.setFilters).toHaveBeenCalledWith(
defaultArgs.data.query.filterManager.getGlobalFilters()
);
});

it('sets breadcrumbs when the document title changes', async () => {
const defaultArgs = makeDefaultArgs();
const instance = mount(<App {...defaultArgs} />);
Expand Down Expand Up @@ -217,14 +272,22 @@ describe('Lens App', () => {
expect(args.docStorage.load).not.toHaveBeenCalled();
});

it('loads a document and uses query if there is a document id', async () => {
it('loads a document and uses query and filters if there is a document id', async () => {
const args = makeDefaultArgs();
args.editorFrame = frame;

const savedFilter = {
query: { match_phrase: { src: 'test' } },
$state: { store: esFilters.FilterStateStore.APP_STATE },
meta: { disabled: false, negate: false, alias: null },
};

(args.docStorage.load as jest.Mock).mockResolvedValue({
id: '1234',
expression: 'valid expression',
state: {
query: 'fake query',
filters: [savedFilter],
datasourceMetaData: { filterableIndexPatterns: [{ id: '1', title: 'saved' }] },
},
});
Expand All @@ -236,6 +299,9 @@ describe('Lens App', () => {

expect(args.docStorage.load).toHaveBeenCalledWith('1234');
expect(args.data.indexPatterns.get).toHaveBeenCalledWith('1');
expect(args.data.query.filterManager.setFilters).toHaveBeenCalledWith(
args.data.query.filterManager.getGlobalFilters().concat([savedFilter])
);
expect(TopNavMenu).toHaveBeenCalledWith(
expect.objectContaining({
query: 'fake query',
Expand All @@ -251,6 +317,7 @@ describe('Lens App', () => {
expression: 'valid expression',
state: {
query: 'fake query',
filters: [savedFilter],
datasourceMetaData: { filterableIndexPatterns: [{ id: '1', title: 'saved' }] },
},
},
Expand Down Expand Up @@ -695,22 +762,27 @@ describe('Lens App', () => {
);
});

it('updates the filters when the user changes them', () => {
it('updates the filters when the user changes them', async () => {
const args = defaultArgs;
args.editorFrame = frame;

const instance = mount(<App {...args} />);
const indexPattern = ({ id: 'index1' } as unknown) as IIndexPattern;
const field = ({ name: 'myfield' } as unknown) as IFieldType;

args.data.query.filterManager.setFilters([esFilters.buildExistsFilter(field, indexPattern)]);
await waitForPromises();
const filter = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Not sure if it's valuable (treat this comment as learning opportunity for me :D ) but in the query tests below we construct filters using setting Filter Store:

      const unpinned = esFilters.buildExistsFilter(field, indexPattern);
      FilterManager.setFiltersStore([unpinned], esFilters.FilterStateStore.APP_STATE);

while here you construct the filter object. Maybe it's worth to unify it?

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 looked into this a bit more and specifically the exists filter has a different type definition than the other types. This is definitely worth highlighting to so I'm building the filters manually!

...esFilters.buildExistsFilter(field, indexPattern),
$state: { store: esFilters.FilterStateStore.APP_STATE },
};
args.data.query.filterManager.setFilters([filter]);

instance.update();

expect(frame.mount).toHaveBeenCalledWith(
expect.any(Element),
expect.objectContaining({
filters: [esFilters.buildExistsFilter(field, indexPattern)],
filters: [filter],
})
);
});
Expand Down
28 changes: 21 additions & 7 deletions x-pack/legacy/plugins/lens/public/app_plugin/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ interface State {
toDate: string;
};
query: Query;
filters: esFilters.Filter[];
savedQuery?: SavedQuery;
}

Expand Down Expand Up @@ -76,16 +75,22 @@ export function App({
fromDate: currentRange.from,
toDate: currentRange.to,
},
filters: [],
};
});

const { lastKnownDoc } = state;

useEffect(() => {
// Clear app-specific filters when navigating to Lens. Necessary because Lens
// can be loaded without a full page refresh, using the same singleton
if (data.query.filterManager.getAppFilters().length) {
data.query.filterManager.setFilters(data.query.filterManager.getGlobalFilters());
}

const filterSubscription = data.query.filterManager.getUpdates$().subscribe({
next: () => {
setState(s => ({ ...s, filters: data.query.filterManager.getFilters() }));
// Trigger a re-render, since filters are no longer tracked on state
setState(s => ({ ...s }));
trackUiEvent('app_filters_updated');
},
});
Expand Down Expand Up @@ -123,13 +128,23 @@ export function App({
core.notifications
)
.then(indexPatterns => {
// Keep any pinned filters and set app filters from doc
data.query.filterManager.setFilters(
data.query.filterManager
.getGlobalFilters()
.concat(
(doc.state.filters || []).filter(
filter => filter.$state?.store === esFilters.FilterStateStore.APP_STATE
)
)
);

setState(s => ({
...s,
isLoading: false,
persistedDoc: doc,
lastKnownDoc: doc,
query: doc.state.query,
filters: doc.state.filters,
indexPatternsForTopNav: indexPatterns,
}));
})
Expand Down Expand Up @@ -239,7 +254,7 @@ export function App({
setState(s => ({ ...s, savedQuery }));
}}
onSavedQueryUpdated={savedQuery => {
data.query.filterManager.setFilters(savedQuery.attributes.filters || state.filters);
data.query.filterManager.setFilters(savedQuery.attributes.filters || []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great as a fix for 7.6.1, but consider using #56160 for 7.7 to avoid similar issues 🙏

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 fix using that code is already in 7.7, but I couldn't backport it to 7.6

setState(s => ({
...s,
savedQuery: { ...savedQuery }, // Shallow query for reference issues
Expand All @@ -256,7 +271,6 @@ export function App({
setState(s => ({
...s,
savedQuery: undefined,
filters: [],
query: {
query: '',
language:
Expand All @@ -278,7 +292,7 @@ export function App({
nativeProps={{
dateRange: state.dateRange,
query: state.query,
filters: state.filters,
filters: data.query.filterManager.getFilters(),
savedQuery: state.savedQuery,
doc: state.persistedDoc,
onError,
Expand Down