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

[Security Solution][Exceptions] - Fixes up some bugs in the all exception items view #141682

Merged
merged 7 commits into from
Sep 29, 2022

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Sep 23, 2022

Summary

Addresses #140709, #141056, #141421, #141042

  • Removed the option of selecting a single row - not sure there's much use there.

Screen Shot 2022-09-23 at 8 39 44 AM

  • Updated text under exception tabs to indicate to user where each exception type is evaluated

Screen Shot 2022-09-28 at 8 30 09 PM

Screen Shot 2022-09-28 at 8 30 43 PM

  • Updated the exception lists management page search placeholder

Screen Shot 2022-09-28 at 8 45 04 PM

  • Fixed some jank occurring on changing rows per page in the exception tabs
rowChange.mov

Checklist

@yctercero yctercero added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team v8.5.0 ci:cloud-redeploy Always create a new Cloud deployment labels Sep 23, 2022
@yctercero yctercero self-assigned this Sep 23, 2022
@yctercero yctercero requested review from a team as code owners September 23, 2022 17:16
@yctercero yctercero requested a review from xcrzx September 23, 2022 17:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@yctercero yctercero changed the title fixing up some bugs in the all exception items view [Security Solution][Exceptions] - Fixes up some bugs in the all exception items view Sep 23, 2022
@yctercero yctercero requested a review from a team September 23, 2022 17:17
@yctercero yctercero removed the ci:cloud-redeploy Always create a new Cloud deployment label Sep 23, 2022
@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

Looks great thanks for the fixes!

@xcrzx
Copy link
Contributor

xcrzx commented Sep 26, 2022

It looks like there's a bug somewhere in the exception pagination. I just opened a random rule and saw "Showing 1-0 of 0":

Screenshot 2022-09-26 at 13 13 13

@nastasha-solomon
Copy link
Contributor

@yctercero thanks for tagging the docs team for the UI copy review! Please find my comments and questions below.

Recommend capitalizing "id" in the placeholder text.
exception-list-placeholder

Couple notes/questions about this image:

  • If the sentence at the top always displays (i.e., it shows regardless of whether endpoint/rule exceptions exist) you can probably remove "Endpoint exceptions are applied to the endpoint and the rule detection" from the center text.
  • Can we change the sentence at the top to align it with the language we use in our docs? For example, "Endpoint exceptions are added to both the detection rule and the Elastic Endpoint agent on your hosts." I feel like this explanation is pretty clear (shoutout to @joepeeples for the lang)
  • Where is the second sentence linking to? In its current state, I'm not quite sure what it's recommending.

exception-tab-1

A few suggestions for the text in this image:

  • Swap "e.g." with "for example". We try to avoid using abbreviations like i.e. and e.g. to accomodate ESL folks.
  • Is the query in the placeholder text KQL? If it is, what are your thoughts on the following revision:
    Filter exceptions using Kibana Query Language (KQL), for example, name:"my list"

exception-tab-2

@nastasha-solomon nastasha-solomon added the ui-copy Review of UI copy with docs team is recommended label Sep 26, 2022
@yctercero
Copy link
Contributor Author

@yctercero thanks for tagging the docs team for the UI copy review! Please find my comments and questions below.

Recommend capitalizing "id" in the placeholder text. exception-list-placeholder

I updated it to specify that it was meant to be list_id, not the list ID. Does that make sense?

Couple notes/questions about this image:

  • If the sentence at the top always displays (i.e., it shows regardless of whether endpoint/rule exceptions exist) you can probably remove "Endpoint exceptions are applied to the endpoint and the rule detection" from the center text.

Done!

  • Can we change the sentence at the top to align it with the language we use in our docs? For example, "Endpoint exceptions are added to both the detection rule and the Elastic Endpoint agent on your hosts." I feel like this explanation is pretty clear (shoutout to @joepeeples for the lang)
  • Where is the second sentence linking to? In its current state, I'm not quite sure what it's recommending.
exception-tab-1

Yea, I like that. I updated the text and removed that link - it was confusing. I was trying to give guidance as to how to link the endpoint list to the rule, but it's super not clear.

A few suggestions for the text in this image:

  • Swap "e.g." with "for example". We try to avoid using abbreviations like i.e. and e.g. to accomodate ESL folks.
  • Is the query in the placeholder text KQL? If it is, what are your thoughts on the following revision:
    Filter exceptions using Kibana Query Language (KQL), for example, name:"my list"
exception-tab-2

Updated the text! It's not KQL, it's simple query syntax - I updated it to try to reflect that - let me know what you think.

@yctercero
Copy link
Contributor Author

It looks like there's a bug somewhere in the exception pagination. I just opened a random rule and saw "Showing 1-0 of 0":

Screenshot 2022-09-26 at 13 13 13

Thanks for finding this! It's a result of an empty exception list being linked to a rule. Pushed up a fix.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 6.6MB 6.6MB +1.6KB

History

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

cc @yctercero

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes, LGTM 👍

@yctercero
Copy link
Contributor Author

@nastasha-solomon I'm going to merge, but please still comment if changes are needed to copy and I will follow up with them.

@yctercero yctercero merged commit 49d0858 into elastic:main Sep 29, 2022
@yctercero yctercero deleted the exceptions_postff_bugs branch September 29, 2022 15:20
Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

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

Just left one comment. Thanks!

{
defaultMessage: 'e.g. Example List Name',
defaultMessage: 'Search by name or list_id',
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delayed response. Feel free to revert this to list id or list ID so that it matches the way you're representing the name field in this helper text. That's all - everything else looks good. :) Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good! I'll update and post follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here - #142258

@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 29, 2022
…tion items view (#141682) (#142237)

### Summary
Addresses #140709, #141056, #141421, #141042

(cherry picked from commit 49d0858)

Co-authored-by: Yara Tercero <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 2, 2022
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:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. ui-copy Review of UI copy with docs team is recommended v8.5.0 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants