[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #84904

banderror · 2020-12-03T13:30:42Z

Addresses: #76587

Summary

In this PR I'm doing basically 2 things:

Making readonly callouts we have in Detections primary instead of warning and thus persistable in local storage (if a callout is dismissed, we remember it there)
Creating a reusable implementation for that.

TODO:

Adjust all callouts used in Detections to be of type primary
Implement the local storage logic (dumb)
Implement the local storage logic (reusable)
Add a new user role: "Reader" (read-only user)
Add Cypress tests

Out of scope:

Add unit tests. I'll probably address this in a follow-up PR. Do you think it's worth it or better to wait until the rework (see the next point)?
Rework callouts to standardise them across Detections, Cases and other parts of the Security app? See my comment below in this PR.

Screen recordings

Quick demo of how this implementation works:

primary callouts
warning, danger (callout types here were manually adjusted)

Additional notes

Cypress tests are based on the work done in #81866.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Found issues:

Keyboard navigation. There's a bug. If focus is on "+ Add filter" and I press Tab, it first goes to "+" and "Untitled timeline" at the bottom, and then goes back to "Dismiss" button of the callout. After that it works as expected. See https://puu.sh/GXMcF/93f194e7b1.png
Keyboard navigation in Safari is not working as expected on the page in general.

For maintainers

This was checked for breaking API changes and was labeled appropriately

banderror · 2020-12-09T21:13:00Z

Further rework towards cases-like implementation?

In Cases, they have a slightly more sophisticated implementation of callouts. It's 5% bound to specifics of Cases and 95% generic from the code standpoint.

Maybe there's an opportunity to create a 100% generic implementation for use across the whole security_solution plugin that would give:

the users a standardised UI and us developers a common way to show and implement callouts: both single and multiple simultaneously;
grouping callouts of the same type so the user could click "Dismiss" only once (that grouping could be optional);
reusable components and logic;
easiness to drop a bunch of CallOutMessage instances to smth like reusable CallOutGroup to be able to show any number of callouts on a page.

Here's a screenshot. The first two callouts are our, from detections (and this is how it looks like in a real app when the user has read-only permissions). The next few callouts are rendered by the CaseCallOut component (added it for demo purposes).

Code-wise, here the page renders:

<NoWriteAlertsCallOut /> // 'You cannot change alert states'
<ReadOnlyCallOut /> // 'Rule permissions required'
<DemoCaseCallOut /> // 'Callout group title'

where DemoCaseCallOut is:

const message1 = {
  errorType: 'primary' as const,
  id: 'msg1',
  title: 'You cannot change alert states',
  description: <>{'You only have permissions to view alerts. If you need to update alert states (open or close alerts), contact your Kibana administrator.'}</>,
};

const message2 = {
  errorType: 'primary' as const,
  id: 'msg2',
  title: 'Rule permissions required',
  description: <>{'You are currently missing the required permissions to create/edit detection engine rule. Please contact your administrator for further assistance.'}</>,
};

const message3 = {
  errorType: 'warning' as const,
  id: 'msg3',
  title: 'You cannot do nothing',
  description: <>{'You have no permissions...'}</>,
};

export const DemoCaseCallOut = () => {
  return <CaseCallOut title="Callout group title" messages={[message1, message2, message3]} />;
};

So do you think it might make sense to (eventually) rework/standardise callouts this way? Or we might never need it?

x-pack/plugins/security_solution/public/detections/components/callouts/index.ts

banderror · 2020-12-09T21:19:48Z

x-pack/plugins/security_solution/public/detections/components/callouts/use_callout_storage.ts

+  dismiss: (message: CallOutMessage) => void;
+}
+
+export const useCallOutStorage = (


Is this a right place for this kind of code?
We have some containers folders in the plugin, and they contain hooks (mostly, I think).
On the other hand, this guy useCallOutStorage is tightly coupled to the components (dumb and smart) represented in this folder.

More broad question: how do we structure components and business logic they use?

banderror · 2020-12-09T21:26:20Z

x-pack/plugins/security_solution/public/detections/components/callouts/use_callout_storage.ts

+  const [visibilityState, setVisibilityState] = useMap(visibilityStateInitial);
+
+  const { getMessages, addMessage } = useMessagesStorage();
+
+  const dismissedMessagesKey = getDismissedMessagesStorageKey(namespace);
+
+  const getVisibleMessageIds = useCallback(() => {
+    return Object.entries(visibilityState)
+      .filter(([messageId, isVisible]) => isVisible)
+      .map(([messageId, isVisible]) => messageId);
+  }, [visibilityState]);


Here I implemented it with normal state and callback hooks, because at this point this is the approach I saw in the codebase.

Question: do we use useReducer, redux or maybe even rx or sagas to build app logic? I think I saw some redux usage somewhere within security_colution...
Do we have some conventions/agreements on how to build it?

...ugins/security_solution/public/detections/components/no_write_alerts_callout/translations.ts

banderror · 2020-12-09T21:32:38Z

x-pack/plugins/security_solution/public/detections/components/callouts/callout_switcher.tsx

+  return shouldRender ? <CallOut message={message} onDismiss={dismiss} /> : null;
+};
+
+export const CallOutSwitcher = memo(CallOutSwitcherComponent);


What's our approach to memoizing components?

I can see it's used pretty extensively throughout the code. The docs say "This method only exists as a performance optimization. Do not rely on it to “prevent” a render, as this can lead to bugs."

I believe we typically memoize any exported component whether it be inline or on a new line like you've done here except in certain cases where they might be memoized elsewhere already (like some of the components in forms where the components they're passed to are memoized. Someone with a more in depth knowledge of the entire codebase might be able to give you more edge case examples)

Thank you, Davis 👍

x-pack/plugins/security_solution/public/detections/components/callouts/callout_types.ts

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/index.test.tsx

spong · 2020-12-10T23:13:28Z

Further rework towards cases-like implementation?

In Cases, they have a slightly more sophisticated implementation of callouts. It's 5% bound to specifics of Cases and 95% generic from the code standpoint.

Maybe there's an opportunity to create a 100% generic implementation for use across the whole security_solution plugin that would give:

the users a standardised UI and us developers a common way to show and implement callouts: both single and multiple simultaneously;

grouping callouts of the same type so the user could click "Dismiss" only once (that grouping could be optional);

reusable components and logic;

easiness to drop a bunch of CallOutMessage instances to smth like reusable CallOutGroup to be able to show any number of callouts on a page.

Here's a screenshot. The first two callouts are our, from detections (and this is how it looks like in a real app when the user has read-only permissions). The next few callouts are rendered by the CaseCallOut component (added it for demo purposes).

So do you think it might make sense to (eventually) rework/standardise callouts this way? Or we might never need it?

I'm in agreement with all the above points @banderror. From a UX perspective seeing stacked callouts (of like type's) makes it a bit hard to decipher the message itself, and eats further into the vertical real estate. And from a developer perspective, having an easy to re-use component that supports local persistence and grouping would streamline implementations, and keep things consistent throughout the app. It's easy to miss if another component is adding a callout for the current page, so something where you could post your error to be added to the global list for the page sounds enticing (similar to how toasts are managed). I don't want to get too far into implementation land, but definitely support a future effort to better standardize callouts across the app and improve the dev experience as well (on top of what you've already added 🙂). This might even be something that @elastic/eui-design would be interested in driving, as I'm sure it'd be helpful to have across all solutions.

rylnd

Code and behavior looks great so far! I still see a lot of questions, skipped tests, and TODOs so I don't think this is ready to approve/merge, but once that's cleaned up I think this'll be a quick LGTM 👍

MadameSheema · 2020-12-15T12:32:41Z

x-pack/plugins/security_solution/cypress/integration/alerts_detection_callouts_readonly.spec.ts

+  });
+});
+
+const loadPageAsReadOnlyUser = (url: string) => {


Can we move all these methods to the tasks folder?

Hmm 🤔 , to be honest I'm not sure!

I moved functions that seem to me truly reusable, like some actions on callouts.

This code, on the other hand, is quite coupled to the test suite:

const loadPageAsReadOnlyUser = (url: string) => { waitForPageWithoutDateRange(url, ROLES.reader); waitForPageTitleToBeShown(); }; const reloadPage = () => { cy.reload(); waitForPageTitleToBeShown(); }; const waitForPageTitleToBeShown = () => { cy.get(PAGE_TITLE).should('be.visible'); };

Here, for example, it waits for the page title. This looks natural for this particular suite only because callouts are rendered on top of the page title. Other suites might have different needs in terms of what to wait for when we reloadPage() - they might need to wait less or more to pass and also be resilient.

Please let me know if you have any strong opinion on that!

I completely understand your point :)

We are used to work in the other way, so, if for whatever reason another colleague wants to extend the tests, will probably create a new spec in order to don't overload the current one. And most likely is going to check the tasks folder for the methods he can use rather than check the spec for them.

If you think that the scenario I mentioned is not going to happen and you want to keep it on the spec file I would suggest you to move it at the top of the file after the imports to give a bit of visibility ;)

Sure thing, easy! Did it. Thank you @MadameSheema !

x-pack/plugins/security_solution/cypress/tasks/detections/callouts.ts

…tion

…onents/callouts

rylnd

This looks great! Truly appreciate the thought toward reusability here, and the comprehensive review materials are incredibly helpful!

It'll be nice to add some unit tests to these new pieces since the old ones weren't testing much, but in the meantime I think that the cypress tests cover us at the high level 👍

rylnd · 2020-12-15T18:24:23Z

x-pack/plugins/security_solution/cypress/tasks/login.ts

+
+export const waitForPageWithoutDateRange = (url: string, role?: RolesType) => {
+  cy.visit(role ? getUrlWithRoute(role, url) : url);
+  cy.get('[data-test-subj="headerGlobalNav"]', { timeout: 120000 });


Nit: We should capture this selector in a constant, I think.

Yes +1, there's a room for cleanup in this file, I just didn't want to touch too many things in this PR.

dplumlee · 2020-12-15T19:43:14Z

x-pack/plugins/security_solution/cypress/tasks/login.ts

+
+export const waitForPageWithoutDateRange = (url: string, role?: RolesType) => {
+  cy.visit(role ? getUrlWithRoute(role, url) : url);
+  cy.get('[data-test-subj="headerGlobalNav"]', { timeout: 120000 });


Any reason this timeout is set so high?

Not sure. What I did is took an existing function loginAndWaitForPageWithoutDateRange (it's above this one) and removed the login part from it to be able to implement the tests.

loginAndWaitForPageWithoutDateRange with this timeout was introduced by @XavierM along with the "defaultCommandTimeout": 120000 in cypress.json. See here:

1216b0f

I guess that's some legacy, because the current "defaultCommandTimeout": 60000

dplumlee · 2020-12-15T19:44:17Z

x-pack/plugins/security_solution/public/common/components/callouts/callout.tsx

+import { CallOutDescription } from './callout_description';
+import { CallOutDismissButton } from './callout_dismiss_button';
+
+export interface CallOutProps {


Probably don't need to export all these prop interfaces

dplumlee · 2020-12-15T19:44:48Z

x-pack/plugins/security_solution/public/common/components/callouts/callout_dismiss_button.tsx

+import { CallOutMessage } from './callout_types';
+import * as i18n from './translations';
+
+export interface CallOutDismissButtonProps {


same export comment as above

x-pack/plugins/translations/translations/zh-CN.json

x-pack/plugins/translations/translations/ja-JP.json

dplumlee

Mostly nits and questions. Great job!

kibanamachine · 2020-12-15T22:01:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c99615b

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2135	2145	+10

Async chunks

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

id	before	after	diff
`securitySolution`	8.3MB	8.3MB	+9.0KB

Distributable file count

id	before	after	diff
`default`	47237	48007	+770

History

💚 Build #94445 succeeded b336286
💚 Build #94187 succeeded 2092641
💔 Build #94158 failed fc5faf7
💚 Build #93505 succeeded 8e06ca4
💔 Build #93252 failed a9bd663

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

…g to info so they persist when dismissed (elastic#84904) **Addresses:** elastic#76587 ## Summary In this PR I'm doing basically 2 things: 1. Making readonly callouts we have in Detections `primary` instead of `warning` and thus persistable in local storage (if a callout is dismissed, we remember it there) 2. Creating a reusable implementation for that. TODO: - [x] Adjust all callouts used in Detections to be of type `primary` - [x] Implement the local storage logic (dumb) - [x] Implement the local storage logic (reusable) - [x] Add a new user role: "Reader" (read-only user) - [x] Add Cypress tests Out of scope: - Add unit tests. I'll probably address this in a follow-up PR. Do you think it's worth it or better to wait until the rework (see the next point)? - Rework callouts to standardise them across Detections, Cases and other parts of the Security app? See my comment below in this PR. ## Screen recordings Quick demo of how this implementation works: - [primary callouts](https://drive.google.com/file/d/1SYQd_ihKPvzlVUxELI8qNEqLBOkL18Gd/view?usp=sharing) - [warning, danger](https://drive.google.com/file/d/1lrAFPyXNjOYSiEsUXxY_fjXsvmyDcdWY/view?usp=sharing) (callout types here were manually adjusted) ## Additional notes Cypress tests are based on the work done in elastic#81866. ![](https://puu.sh/GXwOd/1c855cb03f.png)

…g to info so they persist when dismissed (#84904) (#86047) **Addresses:** #76587 ## Summary In this PR I'm doing basically 2 things: 1. Making readonly callouts we have in Detections `primary` instead of `warning` and thus persistable in local storage (if a callout is dismissed, we remember it there) 2. Creating a reusable implementation for that. TODO: - [x] Adjust all callouts used in Detections to be of type `primary` - [x] Implement the local storage logic (dumb) - [x] Implement the local storage logic (reusable) - [x] Add a new user role: "Reader" (read-only user) - [x] Add Cypress tests Out of scope: - Add unit tests. I'll probably address this in a follow-up PR. Do you think it's worth it or better to wait until the rework (see the next point)? - Rework callouts to standardise them across Detections, Cases and other parts of the Security app? See my comment below in this PR. ## Screen recordings Quick demo of how this implementation works: - [primary callouts](https://drive.google.com/file/d/1SYQd_ihKPvzlVUxELI8qNEqLBOkL18Gd/view?usp=sharing) - [warning, danger](https://drive.google.com/file/d/1lrAFPyXNjOYSiEsUXxY_fjXsvmyDcdWY/view?usp=sharing) (callout types here were manually adjusted) ## Additional notes Cypress tests are based on the work done in #81866. ![](https://puu.sh/GXwOd/1c855cb03f.png)

banderror self-assigned this Dec 3, 2020

banderror force-pushed the make-readonly-callouts-of-type-info branch 4 times, most recently from 850e429 to a9bd663 Compare December 9, 2020 19:59

banderror commented Dec 9, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/callouts/index.ts Outdated Show resolved Hide resolved

banderror commented Dec 9, 2020

View reviewed changes

...ugins/security_solution/public/detections/components/no_write_alerts_callout/translations.ts Outdated Show resolved Hide resolved

banderror commented Dec 9, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/callouts/callout_types.ts Outdated Show resolved Hide resolved

banderror commented Dec 9, 2020

View reviewed changes

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx Outdated Show resolved Hide resolved

banderror force-pushed the make-readonly-callouts-of-type-info branch from a9bd663 to 8e06ca4 Compare December 10, 2020 14:51

banderror commented Dec 10, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/index.test.tsx Outdated Show resolved Hide resolved

banderror force-pushed the make-readonly-callouts-of-type-info branch 2 times, most recently from fc5faf7 to 2092641 Compare December 14, 2020 20:21

banderror marked this pull request as ready for review December 14, 2020 20:22

banderror requested review from a team as code owners December 14, 2020 20:22

rylnd reviewed Dec 15, 2020

View reviewed changes

MadameSheema reviewed Dec 15, 2020

View reviewed changes

x-pack/plugins/security_solution/cypress/tasks/detections/callouts.ts Outdated Show resolved Hide resolved

banderror added 10 commits December 15, 2020 14:35

Store dismissal in local storage: the most straightforward implementa…

988d0b8

…tion

Store dismissal in local storage: refactored, reusable implementation

ae36124

Fix and skip (temporarily) unit tests

9ca264f

Add a new user role: Reader (read-only user)

2408222

Add Cypress tests

56183bf

Clean up

c295640

Clean up: move reusable callouts to public/common/components

ebd3aa7

Clean up: remove skipped tests

9094120

Clean up: rename and move concrete callouts to public/detections/comp…

d161755

…onents/callouts

Clean up: adjust translations

732211e

banderror force-pushed the make-readonly-callouts-of-type-info branch from 2092641 to b336286 Compare December 15, 2020 13:36

banderror requested review from rylnd, MadameSheema, dplumlee and a team December 15, 2020 14:23

rylnd approved these changes Dec 15, 2020

View reviewed changes

Clean up tests

c99615b

banderror force-pushed the make-readonly-callouts-of-type-info branch from b336286 to c99615b Compare December 15, 2020 19:48

dplumlee reviewed Dec 15, 2020

View reviewed changes

dplumlee approved these changes Dec 15, 2020

View reviewed changes

banderror merged commit 06907b8 into elastic:master Dec 15, 2020

banderror mentioned this pull request Dec 15, 2020

[7.x] [Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed (#84904) #86047

Merged

banderror deleted the make-readonly-callouts-of-type-info branch December 15, 2020 22:46

banderror mentioned this pull request Dec 15, 2020

[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #76587

Closed

This was referenced Jan 4, 2021

[SecuritySolution][Detections] Fix "Closing a signal silently fails with reduced privileges" #86908

Merged

[Security Solution][Detections] Create a standard UX and common implementation of callouts for Elastic Security app #87681

Open

spong mentioned this pull request Mar 6, 2021

[RAC][Alert Triage] Permission Callout Components #93875

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #84904

[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #84904

banderror commented Dec 3, 2020 •

edited

Loading

banderror commented Dec 9, 2020 •

edited

Loading

banderror Dec 9, 2020

banderror Dec 9, 2020

banderror Dec 9, 2020

dplumlee Dec 9, 2020

banderror Dec 15, 2020

spong commented Dec 10, 2020

Further rework towards cases-like implementation?

rylnd left a comment

MadameSheema Dec 15, 2020

banderror Dec 15, 2020

MadameSheema Dec 15, 2020

banderror Dec 15, 2020

rylnd left a comment

rylnd Dec 15, 2020

banderror Dec 15, 2020

dplumlee Dec 15, 2020

banderror Dec 15, 2020

dplumlee Dec 15, 2020

dplumlee Dec 15, 2020

dplumlee left a comment •

edited

Loading

kibanamachine commented Dec 15, 2020

[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #84904

[Security Solution][Detections] Update read-only callouts from warning to info so they persist when dismissed #84904

Conversation

banderror commented Dec 3, 2020 • edited Loading

Summary

Screen recordings

Additional notes

Checklist

For maintainers

banderror commented Dec 9, 2020 • edited Loading

Further rework towards cases-like implementation?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spong commented Dec 10, 2020

Further rework towards cases-like implementation?

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplumlee left a comment • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Dec 15, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

History

banderror commented Dec 3, 2020 •

edited

Loading

banderror commented Dec 9, 2020 •

edited

Loading

dplumlee left a comment •

edited

Loading