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

[Dashboard] [Controls] Add "Exists" functionality to options list #143762

Merged

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Oct 20, 2022

Closes #140700
Closes #139932

@elastic/kibana-docs Refer to #143762 (comment)
@elastic/kibana-design Refer to discussion starting at #143762 (comment)

Summary

This PR adds the ability for the options list control to be used as an exists filter for a given field.

Screen.Recording.2022-10-25.at.2.58.41.PM.mov

The exists option list entry will show up at all times, even during search, unless one of the following conditions is met:

  1. The exists option is not selected and "Show only selected" is selected in the dropdown
  2. The "Allow exists query" toggle is set to false in the create/edit control flyout
  3. The options list has no suggestions due to chaining, search string returning no results, etc.
Screen.Recording.2022-10-26.at.9.49.09.AM.mov

As part of this, I performed a refactor of the OptionsListPopover component because, with the addition of both the exclude and exists functionality, the logic was starting to get extremely difficult to follow. This included making it so that the search bar auto focuses when the popover is open.

How to Test

Testing general exists / does not exist queries:

  1. Download the following test data I generated: testExistsData.csv
  2. Upload this data to Kibana
  3. This data has 100 rows and two columns - one, that always has a value (always exists) and two, which has a value 57 times (sometimes exists). Verify that, when creating a control for these fields, you get the expected results for the exists and does not exist queries.

Warning
After discussion with @andreadelrio, we decided that the validation should not apply to the exists selection because you can get unexpected results - for example, if a field always exists, a "Does not exist" selection would automatically jump to "Ignored selections" when validation was on.

In this case, a "no data" state actually makes it clearer to the user that the field simply always exists, whereas the selection automatically being ignored causes confusion on why their selection does not apply. So, since the exists query is special in this way, it should never be ignored - i.e.exists or does not exist selections should always apply regardless of if the selection results in a no data state.

Therefore, the following discussion is outdated / no longer relevant:

#### Testing validation of `exists` and `does not exist` queries:
1.  Download the smaller test data if you want, since it's easier to test the validation with this: [testSmallExistsData.csv](https://github.com/elastic/kibana/files/9922840/testSmallExistsData.csv). Note that this dataset is simply the first 10 rows of the original dataset, so feel free to modify that dataset if you'd prefer.
2. Add a control for `always exists` followed by a control for `sometimes exists`
3. One way to get an invalid `does not exist`  selection...
    1.  In the `always exists` options list, exclude the selections and select `Does not exist (!)` 
    2. This selection should automatically be ignored<br>
     ![Nov-02-2022 14-54-09](https://user-images.githubusercontent.com/8698078/199600579-09d8f77d-ad3c-4a1e-a10c-a2714273f2f9.gif)

4. Another way to get an invalid `does not exist`  selection...
    1. In the `sometimes exists` options list, exclude the selections and select `Does not exist (!)` 
    4. In the `always exists` options list, select `"Katherine Marshall"` and `"Suzanne Howard"`
    5. Notice that the `Does not exist (!)` selection in the `sometimes exists` options list is invalid<br>
     ![image](https://user-images.githubusercontent.com/8698078/199601370-fb09606e-15df-4de5-a722-9e114f938698.png)

7. To get an invalid `exists` selection...
    1. In the `sometimes exists` options list, include the selections and select `Exists (*)` 
    2. Keep the selections from step 4(ii) above, but toggle `exclude` to `true`
    4. Notice that the `Exists (*)` selection in the `sometimes exists` options list is invalid<br>
      ![image](https://user-images.githubusercontent.com/8698078/199601533-79b74a85-32b0-4c24-902d-17a584c979df.png)

Flaky Test Runner

  • control_group_chaining.ts

  • options_list.ts

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Heenawter Heenawter added Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. release_note:feature Makes this part of the condensed release notes Project:Controls v8.6.0 labels Oct 20, 2022
@Heenawter Heenawter self-assigned this Oct 20, 2022
@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch 6 times, most recently from 3814148 to cd9c222 Compare October 25, 2022 17:33
@Heenawter Heenawter added loe:large Large Level of Effort ui-copy Review of UI copy with docs team is recommended and removed loe:medium Medium Level of Effort labels Oct 25, 2022
@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch 12 times, most recently from 6c39445 to a323772 Compare October 28, 2022 22:04
@Heenawter Heenawter marked this pull request as ready for review November 3, 2022 17:09
@Heenawter Heenawter requested review from a team as code owners November 3, 2022 17:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

{existsSelected
? OptionsListStrings.control.getExcludeExists()
: OptionsListStrings.control.getNegate()}
</span>{' '}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to add this space using CSS :after, but then this messes with the screen reader because the title of the button becomes "DOES NOTExist"- so, need to add the space explicitly here instead.

@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch from 409adbf to 5927369 Compare November 3, 2022 17:17
@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch from 3eb07d4 to b1bbfc8 Compare November 3, 2022 17:40
@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch from c8ca893 to 20a1dd2 Compare November 3, 2022 18:14
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design LGTM, great job! Exciting to see controls getting closer to filter pills.

@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch from a0b14fd to 8f255b4 Compare November 3, 2022 18:44
@Heenawter Heenawter force-pushed the add-exists-to-options-list_2022-10-19 branch from 8f255b4 to 0318b0a Compare November 3, 2022 18:59
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested this locally and all of the test cases I could think of worked properly, including any weird settings on the chain, and turning on and off the exists capabilities in various selection states. ✅

Code wise, the new organization of the Options List Popover into 3 components looks great! It's much more organized, and is a good foundation for adding even more features & complexity to the Options List. ✅

Also great looking unit test and functional test coverage! ✅

'common',
'header',
]);

const DASHBOARD_NAME = 'Test Options List Control';

describe('Dashboard options list integration', () => {
const newDocuments: Array<{ index: string; id: string }> = [];

const addDocument = async (index: string, document: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's too bad there isn't a helper for this function. Do other functional tests that you've seen use the console to add docs?

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 actually wasn't able to find any tests that add new documents except those that are literally testing the console 😅 So I just came up with my own way of doing it

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a first time for everything I guess! If we find ourselves having to do this more and more, we should create our own helper then maybe.

@@ -429,7 +496,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
});

it('Does not mark selections invalid with Query', async () => {
await queryBar.setQuery('isDog : false ');
await queryBar.setQuery('NOT animal.keyword : "dog" ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, no reason to also test scripted fields here as well.

@Heenawter Heenawter enabled auto-merge (squash) November 4, 2022 20:55
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 152 156 +4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
controls 224 228 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 471.6KB 473.6KB +2.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 20.6KB 21.9KB +1.3KB
Unknown metric groups

API count

id before after diff
controls 233 237 +4

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 106 111 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 107 113 +6
securitySolution 517 523 +6
total +20

History

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

cc @Heenawter

@Heenawter Heenawter merged commit a30e6c2 into elastic:main Nov 4, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 4, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 28, 2022
kibanamachine added a commit that referenced this pull request Nov 28, 2022
# Backport

This will backport the following commits from `main` to `8.6`:
- [[DOCS] Adds the 8.6 Presentation docs
(#145474)](#145474)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kaarina
Tungseth","email":"kaarina.tungseth@elastic.co"},"sourceCommit":{"committedDate":"2022-11-28T16:20:38Z","message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","Team:Presentation","release_note:skip","v8.6.0","v8.7.0"],"number":145474,"url":"https://github.com/elastic/kibana/pull/145474","mergeCommit":{"message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145474","number":145474,"mergeCommit":{"message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09"}}]}]
BACKPORT-->

Co-authored-by: Kaarina Tungseth <kaarina.tungseth@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Controls release_note:feature Makes this part of the condensed release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas ui-copy Review of UI copy with docs team is recommended v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Controls] Add "Exists" functionality to Options List [Controls] Options List Should Auto Focus on Search
8 participants