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

Add error if filter index pattern is missing #66979

Merged
merged 36 commits into from
Jun 3, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented May 19, 2020

Summary

Note: Commented out maps test - fix in #64861

See #67177 for full explanation.

Release Notes

This PR adds error and warning statuses to FilterBar filters, according to the following scenarios:

  • If a filter's index pattern is missing the filter will show in an error state and disable itself.
  • If a filter's index pattern field is missing the filter will show in an error state and disable itself.
  • If a filter's uses an unrelated index pattern and the field in that filter does not exist in any of the index patterns used by the app - the filter will show in a warning state. i.e. Trying to apply a filter for the logs data set file extension field to the ecommerce data set dashboard. The filter will not be automatically disabled.
  • If a filter's uses an unrelated index pattern but the field in that filter exist in at least one of the index patterns used by the app - the filter will show in a normal state. i.e. Trying to apply a filter for the logs data set "name" field to the ecommerce data set dashboard (that also has a name field). If you try to edit this filter, you will still have to re-select the index pattern and re-configure the filter.

Error state:

image

Warning state:

image

To reproduce

Error filter

  1. Create an index pattern
  2. Go to discover and create a filter for that index pattern
  3. Save it as a saved query
  4. Delete the index pattern you created
  5. Go to another dashboard and load that saved query

Warning filter

  1. Load two data sets (ecommerce and logs for example).
  2. Go to one of the dashboards and create a filter
  3. Save a Saved Query
  4. Go to the other dashboard and try loading the Saved Query.

The filter should appear in an error state

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom requested a review from a team as a code owner May 19, 2020 11:20
@lizozom lizozom self-assigned this May 19, 2020
@lizozom lizozom added v7.6.3 v7.7.1 v7.8.0 v8.0.0 bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes labels May 19, 2020
}),
});
valueLabel = this.props.intl.formatMessage({
errorMessage = valueLabel = this.props.intl.formatMessage({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notification was removed as it renders multiple times - showing the error message on the filter is clear enough.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Tested using steps from description. Dashboard is displayed and see error in filter 👍

One note: I was trying to fix broken filter from UI, it allowed me to take new existing index pattern, which is great. But is looks it didn't save changes, please see:

May-19-2020 14-31-15

I think, if this is a quick fix, it worth fixing?

Also, I'd consider adding a test for this bug.
Maybe even a functional one,
I'd setup it the following way:

  1. Prepare a saved query with broken filter upfront (using API or esarchiver). (just do not click UI for this)
  2. Open any dashboard and apply the saved query
  3. Assert that dashboard is displayed

@Dosant
Copy link
Contributor

Dosant commented May 19, 2020

FYI, when tested noticed regression in master: #67003

@Dosant
Copy link
Contributor

Dosant commented May 19, 2020

I tried to reproduce "Empty/broken dashboard" in master using the steps from the description, but I still can see the dashboard. Just filter isn't working.

Is there a way to get into state with empty dashboard? or it is not possible in Master?

@lizozom
Copy link
Contributor Author

lizozom commented May 20, 2020

@Dosant I suspect that the reason you didn't see it update is because of a problem with the build system at the moment (it seems that plugins are built, but they are actually still building for a few mins after kibana seems to start).

captured

Also working on adding the test you requested and fixing the broken test as well.

@lizozom lizozom requested a review from a team as a code owner May 21, 2020 15:59
@lizozom lizozom requested a review from Dosant May 21, 2020 16:00
@@ -93,6 +93,12 @@ export class IndexPatternsService {
).savedObjects;
}

getAll = async (refresh: boolean = false) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime what do you think of this?

* Filters may be applied from an index pattern other than the active index patterns list provided.
* Retrieve the actual index pattern from the filter for validation.
*/
getIndexPatterns()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get index patterns once in filter bar and pass them to each FilterItem

this.props.filter.meta.disabled ? 'disabled' : 'enabled'
}`;
const dataTestSubjPinned = `filter-${isFilterPinned(filter) ? 'pinned' : 'unpinned'}`;
private isValidLabel(labelConfig: LabelOptions) {
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 broke down the logic in this component to helper functions for easier debugging and reviewing

@lizozom
Copy link
Contributor Author

lizozom commented Jun 2, 2020

@Dosant validated that 7.8 works with the new data.json.gz file

@lizozom
Copy link
Contributor Author

lizozom commented Jun 2, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@lizozom lizozom merged commit fbcb74c into elastic:master Jun 3, 2020
lizozom added a commit to lizozom/kibana that referenced this pull request Jun 3, 2020
* add error if filter index pattern is gone

* docs change - why?

* Fix i18n

* Added a functional test for broken filters (field + index pattern)

* Clarify readme

* Moved readme

* New warning status

* Remove getAll

* git pull upstream master

* Fix translation files

* Fix merge

* added filterbar texts

* disabled correction

* Disable check in maps test until elastic#64861 is resolved

* Fix tests, warning state is not disabled.

* Adjust warning filter - ignore filters from "foreign" index pattern, that are still applicable

* Add an additional unrelaeted filter test

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Fixed test data

* Revert mapping

* Update data with missing test

* Update test to match data

* Code review

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lizozom added a commit that referenced this pull request Jun 3, 2020
* add error if filter index pattern is gone

* docs change - why?

* Fix i18n

* Added a functional test for broken filters (field + index pattern)

* Clarify readme

* Moved readme

* New warning status

* Remove getAll

* git pull upstream master

* Fix translation files

* Fix merge

* added filterbar texts

* disabled correction

* Disable check in maps test until #64861 is resolved

* Fix tests, warning state is not disabled.

* Adjust warning filter - ignore filters from "foreign" index pattern, that are still applicable

* Add an additional unrelaeted filter test

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Fixed test data

* Revert mapping

* Update data with missing test

* Update test to match data

* Code review

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lizozom added a commit to lizozom/kibana that referenced this pull request Jun 5, 2020
* add error if filter index pattern is gone

* docs change - why?

* Fix i18n

* Added a functional test for broken filters (field + index pattern)

* Clarify readme

* Moved readme

* New warning status

* Remove getAll

* git pull upstream master

* Fix translation files

* Fix merge

* added filterbar texts

* disabled correction

* Disable check in maps test until elastic#64861 is resolved

* Fix tests, warning state is not disabled.

* Adjust warning filter - ignore filters from "foreign" index pattern, that are still applicable

* Add an additional unrelaeted filter test

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Fixed test data

* Revert mapping

* Update data with missing test

* Update test to match data

* Code review

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	test/functional/fixtures/es_archiver/dashboard/current/kibana/data.json.gz
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Jun 5, 2020
* add error if filter index pattern is gone

* docs change - why?

* Fix i18n

* Added a functional test for broken filters (field + index pattern)

* Clarify readme

* Moved readme

* New warning status

* Remove getAll

* git pull upstream master

* Fix translation files

* Fix merge

* added filterbar texts

* disabled correction

* Disable check in maps test until elastic#64861 is resolved

* Fix tests, warning state is not disabled.

* Adjust warning filter - ignore filters from "foreign" index pattern, that are still applicable

* Add an additional unrelaeted filter test

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Update src/plugins/data/public/ui/filter_bar/_global_filter_item.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* Fixed test data

* Revert mapping

* Update data with missing test

* Update test to match data

* Code review

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 9, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@lizozom lizozom added v7.8.1 and removed v7.8.0 labels Jun 10, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 11, 2020
@bhavyarm
Copy link
Contributor

This fix didn't go into 7.8.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:fix v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants