[Cases] Lens as persistable state attachment type by cnasikas · Pull Request #159004 · elastic/kibana

cnasikas · 2023-06-05T09:04:24Z

Summary

This PR registers the lens attachment type using the cases attachment framework.

Testing scenarios

Attaching a lens visualization to the markdown editor works as expected
Attaching a lens visualization from the dashboard creates a lens attachment (persistable state)
Attaching a lens visualization from the security solution dashboard (Security solution -> Dashboards -> Create dashboard) creates a lens attachment (persistable state).
Attaching a lens visualization from the security solution overview dashboard (Security solution -> Dashboards -> Overview) creates a lens attachment (persistable state).

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-06-13T06:39:28Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-06-13T06:39:30Z

Pinging @elastic/response-ops-cases (Feature:Cases)

cnasikas · 2023-06-13T06:42:46Z

x-pack/plugins/cases/public/components/markdown_editor/plugins/lens/index.ts

 import { LensParser } from './parser';
-import { LensMarkDownRenderer } from './processor';
 import { VISUALIZATION } from './translations';
+import LensRenderer from '../../../visualizations/lens_renderer';


Same code as before. Moved to the visualization folder.

cnasikas · 2023-06-13T07:10:27Z

x-pack/plugins/cases/public/components/visualizations/lens_renderer.tsx

+export const LensRenderer = React.memo(LensRendererComponent);
+
+// eslint-disable-next-line import/no-default-export
+export { LensRenderer as default };


Same code as before. Different naming.

adcoelho · 2023-06-13T08:14:19Z

x-pack/plugins/cases/public/components/visualizations/attachment.tsx

+import type { LensProps } from './types';
+import { OpenLensButton } from './open_lens_button';
+
+function getOpenLensButton(attachmentId: string, props: LensProps) {


Lazy loading here would decrease the page bundle size iirc

I think it is ok as we do not import any new component from other plugins and the OpenLensButton component is already in the bundle as it is used by the markdown editor.

adcoelho · 2023-06-13T08:16:32Z

x-pack/plugins/cases/public/components/visualizations/attachment.tsx

+    persistableStateAttachmentState as unknown as LensProps;
+
+  return {
+    event: 'added visualization',


Translation missing for the event

adcoelho · 2023-06-13T08:20:54Z

x-pack/plugins/cases/public/components/visualizations/attachment.tsx

+      const { LensRenderer } = await import('./lens_renderer');
+
+      return {
+        // eslint-disable-next-line react/display-name


nit: could you avoid this comment with the below?

Suggested change

// eslint-disable-next-line react/display-name

const my_function = (props: PersistableStateAttachmentViewProps) => {

const { attributes, timeRange } = props.persistableStateAttachmentState as unknown as LensProps;

return <LensRenderer attributes={attributes} timeRange={timeRange} />;

}

return {

default: React.memo(my_function,

...

adcoelho · 2023-06-13T08:44:31Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.test.tsx

+    });
+  });
+
+describe('createCommentUserActionBuilder', () => {


nit: should this be createRegisteredAttachmentUserActionBuilder?

createCommentUserActionBuilder is in x-pack/plugins/cases/public/components/user_actions/comment/comment.test.tsx

Bad copy paste 🙂

adcoelho · 2023-06-13T08:49:07Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.test.tsx

+    });
+  });
+
+  it('builds the action correctly', async () => {


Could you please also add a test for when hideDefaultActions is true?

angorayc · 2023-06-13T16:41:22Z

...ins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.tsx

+        type: CommentType.persistableState as const,
      },
-    ];
+    ] as CaseAttachmentsWithoutOwner;


Could you please add the test below to its test file:

const mockOpenCaseFlyout = jest.fn(); const mockGetUseCasesAddToNewCaseFlyout = jest.fn().mockReturnValue({ open: mockOpenCaseFlyout }); it('should open create case flyout', () => { const mockClick = jest.fn(); const { result } = renderHook(() => useAddToNewCase({ lensAttributes: kpiHostMetricLensAttributes, timeRange, onClick: mockClick, }) ); result.current.onAddToNewCaseClicked(); expect(mockOpenCaseFlyout).toHaveBeenCalledWith({ attachments: [ { persistableStateAttachmentState: { attributes: kpiHostMetricLensAttributes, timeRange }, persistableStateAttachmentTypeId: LENS_ATTACHMENT_TYPE, type: CommentType.persistableState as const, }, ], }); expect(mockClick).toHaveBeenCalled(); });

angorayc · 2023-06-13T16:51:36Z

...ecurity_solution/public/common/components/visualization_actions/use_add_to_existing_case.tsx

+        type: CommentType.persistableState as const,
      },
-    ];
+    ] as CaseAttachmentsWithoutOwner;


Please add the test to its test file:

const mockOpenCaseModal = jest.fn(); const mockGetUseCasesAddToExistingCaseModal = jest .fn() .mockReturnValue({ open: mockOpenCaseModal }); it('should open add to existing case modal', () => { const mockClick = jest.fn(); const { result } = renderHook(() => useAddToExistingCase({ lensAttributes: kpiHostMetricLensAttributes, timeRange, onAddToCaseClicked: mockClick, }) ); result.current.onAddToExistingCaseClicked(); const attachments = mockOpenCaseModal.mock.calls[0][0].getAttachments(); expect(attachments).toEqual([ { persistableStateAttachmentState: { attributes: kpiHostMetricLensAttributes, timeRange, }, persistableStateAttachmentTypeId: LENS_ATTACHMENT_TYPE, type: CommentType.persistableState as const, }, ]); expect(mockClick).toHaveBeenCalled(); });

jonathan-buttner

Looking good, left a few comments. Is there a way we can test the deferred migrations for user actions and comments? Or do our tests already cover it?

jonathan-buttner · 2023-06-13T16:02:07Z

...es/public/components/user_actions/comment/__snapshots__/registered_attachments.test.tsx.snap

+    "children": <React.Suspense
+      fallback={<EuiLoadingSpinner />}
+    >
+      <UNDEFINED


Is this supposed to be undefined 🤔

Probably because the registered component does not have a displayName. I find it weird tbh.

jonathan-buttner · 2023-06-13T16:08:36Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.tsx


    const props = {
      ...getAttachmentViewProps(),
+      attachmentId: userAction.id,


Should this be attachmentId: comment.id?

hmm, good point. I will change it.

jonathan-buttner · 2023-06-13T16:10:33Z

x-pack/plugins/cases/public/components/visualizations/actions/add_to_existing_case.test.tsx

            {
-              comment: `!{lens${JSON.stringify({
-                timeRange: mockTimeRange,
+              persistableStateAttachmentState: {


nit: I know this isn't your change but I wonder if we should do a more exact check instead of expect.objectContaining. Or are there other fields that aren't represented here?

I removed it and it seems that only these fields are the expected ones. Not sure why it was there in the first place.

jonathan-buttner · 2023-06-13T18:48:29Z

x-pack/plugins/cases/server/saved_object_types/migrations/user_actions/index.ts

+          context,
+          error,
+          docType: 'user action persistable lens attachment',
+          docKey: 'comment',


Should this be user-action or something similar?

jonathan-buttner · 2023-06-13T18:50:10Z

x-pack/plugins/cases/server/saved_object_types/migrations/user_actions/index.ts

+  return mergeSavedObjectMigrationMaps(userActionsMigrations, embeddableMigrationsToMerge);
+};
+
+export const lensMigratorFactory = (


I think it's fine if we want to migrate the user actions. But just to make sure I'm understanding correctly, we still aren't migrating the user actions where a lens embeddable is within the comment via a json blob (aka not a persistable attachment)?

As we discussed offline, I removed the lens migrations in user actions.

js-jankisalvi · 2023-06-14T12:11:15Z

Testing scenarios

Attaching a lens visualization to the markdown editor works as expected ✅

Attaching a lens visualization from the dashboard creates a lens attachment (persistable state) ✅

Attaching a lens visualization from the security solution dashboard (Security solution -> Dashboards -> Create dashboard) creates a lens attachment (persistable state). ✅

Attaching a lens visualization from the security solution overview dashboard (Security solution -> Dashboards -> Overview) creates a lens attachment (persistable state). ✅

Verified all 4 scenarios, works as expected 👍

shahzad31

Uptime changes LGTM !!

cnasikas · 2023-06-15T11:07:30Z

Is there a way we can test the deferred migrations for user actions and comments? Or do our tests already cover it?

I don't think so as there are no 8.9+ migrations registered by the lens team yet. The unit tests should cover all scenarios. Wdyt?

jonathan-buttner · 2023-06-15T12:38:32Z

Is there a way we can test the deferred migrations for user actions and comments? Or do our tests already cover it?

I don't think so as there are no 8.9+ migrations registered by the lens team yet. The unit tests should cover all scenarios. Wdyt?

Sounds good.

jonathan-buttner

Looks good!

cnasikas · 2023-06-16T06:02:18Z

@elasticmachine merge upstream

kibana-ci · 2023-06-16T07:52:25Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 9a9af51

Failed CI Steps

Security Solution Tests #7

Test Failures

[job] [logs] Security Solution Tests #7 / Prebuilt rules Actions with prebuilt rules Rules table "before each" hook for "Allows to delete all rules at once"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	639	643	+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
`cases`	64	65	+1

Async chunks

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

id	before	after	diff
`cases`	416.8KB	415.4KB	-1.4KB
`exploratoryView`	199.6KB	199.6KB	+80.0B
`securitySolution`	10.8MB	10.8MB	+480.0B
total			-903.0B

Page load bundle

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

id	before	after	diff
`cases`	135.7KB	139.1KB	+3.4KB
`exploratoryView`	44.8KB	44.9KB	+135.0B
total			+3.6KB

Unknown metric groups

API count

id	before	after	diff
`cases`	79	80	+1

ESLint disabled line counts

id	before	after	diff
`cases`	57	58	+1
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+7

Total ESLint disabled count

id	before	after	diff
`cases`	74	75	+1
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+7

History

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

cc @cnasikas

cnasikas added Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Cases Cases feature labels Jun 5, 2023

cnasikas self-assigned this Jun 5, 2023

cnasikas added 7 commits June 8, 2023 11:41

Mark lens migrations as deferred

5143120

Register lens as persistable attachment

58fe11f

Use the new lens attachment type in solutions

7033968

Add open in visualization button

0b02ba7

Migrate persistable lens attachments

7bdaeb6

Migrate persistable lens user actions

6b8f607

Merge branch 'main' into lens_as_attachment

3b4bad4

cnasikas force-pushed the lens_as_attachment branch from 59525ef to 3b4bad4 Compare June 8, 2023 20:23

cnasikas added the v8.9.0 label Jun 8, 2023

cnasikas added 5 commits June 11, 2023 14:57

Merge branch 'main' into lens_as_attachment

2b749e6

Add tests

c3c0d44

Fix types

c69a599

Fix i18n

bd56973

Fix import in integration tests

5cd900b

cnasikas marked this pull request as ready for review June 13, 2023 06:39

cnasikas requested review from a team as code owners June 13, 2023 06:39

cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label Jun 13, 2023

cnasikas commented Jun 13, 2023

View reviewed changes

Merge branch 'main' into lens_as_attachment

7f9bed7

cnasikas commented Jun 13, 2023

View reviewed changes

adcoelho reviewed Jun 13, 2023

View reviewed changes

cnasikas added 2 commits June 13, 2023 15:43

Fix user actions migrations

da7ebcd

Fix o11y

600c393

angorayc reviewed Jun 13, 2023

View reviewed changes

angorayc approved these changes Jun 13, 2023

View reviewed changes

jonathan-buttner reviewed Jun 13, 2023

View reviewed changes

Merge branch 'main' into lens_as_attachment

5ceb84e

shahzad31 approved these changes Jun 15, 2023

View reviewed changes

PR feedback

8aac440

cnasikas requested review from adcoelho, jonathan-buttner and shahzad31 June 15, 2023 11:17

shahzad31 approved these changes Jun 15, 2023

View reviewed changes

Fix import

74b2fe6

Remove lens migrations in user actions

398abe4

jonathan-buttner approved these changes Jun 15, 2023

View reviewed changes

cnasikas added the ci:skip-cypress-osquery Skips osquery cypress checks label Jun 15, 2023

Merge branch 'main' into lens_as_attachment

9a9af51

cnasikas merged commit 5df612e into elastic:main Jun 16, 2023

cnasikas deleted the lens_as_attachment branch June 16, 2023 09:46

kibanamachine added the backport:skip This PR does not require backporting label Jun 16, 2023

cnasikas mentioned this pull request Jun 16, 2023

[Cases] Add Lens embeddables as case attachments #149018

Closed

cnasikas mentioned this pull request Jul 11, 2023

[DOCS] Add case highlights for 8.9 #161591

Merged

-        // eslint-disable-next-line react/display-name
+const my_function = (props: PersistableStateAttachmentViewProps) => {
+    const { attributes, timeRange } = props.persistableStateAttachmentState as unknown as LensProps;
+    return <LensRenderer attributes={attributes} timeRange={timeRange} />;
+}
+return {
+    default: React.memo(my_function,
+    ...

Conversation

cnasikas commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing scenarios

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 13, 2023

Uh oh!

elasticmachine commented Jun 13, 2023

Uh oh!

cnasikas Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcoelho Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js-jankisalvi commented Jun 14, 2023

Testing scenarios

Uh oh!

shahzad31 left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Jun 15, 2023

Uh oh!

jonathan-buttner commented Jun 15, 2023

Uh oh!

jonathan-buttner left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Jun 16, 2023

Uh oh!

kibana-ci commented Jun 16, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

cnasikas commented Jun 5, 2023 •

edited

Loading

cnasikas Jun 13, 2023 •

edited

Loading

adcoelho Jun 13, 2023 •

edited

Loading

cnasikas Jun 15, 2023 •

edited

Loading