Add tag bulk action context menu #82816

pgayvallet · 2020-11-06T10:49:11Z

Summary

Part of #81980
Required for #81970

Add maxWidth option to OverlayModalConfirmOptions and add missing exports from core/public/index
Add the bulk action menu to the tag management section
Implement the bulk delete action
Implement the clear selection action

Screenshots

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
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

…k-action-menu

pgayvallet

Self review

pgayvallet · 2020-11-09T08:17:49Z

x-pack/plugins/saved_objects_tagging/public/tags/tags_client.ts

+      if (this.changeListener) {
+        tagIds.forEach((tagId) => {
+          this.changeListener!.onDelete(tagId);
+        });
+      }
+    });


I wanted to avoid adding more hooks on the TagCache class. Doing a loop for each delete tag felt good enough.

pgayvallet · 2020-11-09T08:22:16Z

x-pack/plugins/saved_objects_tagging/server/routes/internal/bulk_delete.ts

+      for (const tagId of tagIds) {
+        await client.delete(tagId);
+      }


We don't have a SoClient.bulkDelete API, and I need to use the TagClient instance anyway to perform the references cleanup, which is why there is a loop here.

Note that is there is an error during any tag removal (or associated references cleanup), we will fail fast and do not attempt to delete the next tags in the list. I did that because the most likely cause of failure would be an RBAC related failure, and in that case, it will be thrown for the first tag, and attempting to remove the next tags felt unnecessary.

I did not create a bulkDelete API on the TagClient as I thought the logic was simple enough to be kept in the handler (and to avoid increase API surface, as bulkDelete is supposed to be internal), but I can if we think this is better.

The performance of this is probably going to be fine in 99% of cases, but it is certainly possible for this to partially fail in cases where 1000s of tags are being deleted at once. The reason I think this is acceptable for now is that failing part of the way through (most likely b/c of a timeout) doesn't corrupt the data and the user can easily refresh & try again to delete any remaining tags.

pgayvallet · 2020-11-09T09:00:21Z

x-pack/plugins/saved_objects_tagging/public/management/components/_action_bar.scss

+.tagMgt__actionBar + .euiSpacer {
+  display: none;
+}


elastic/eui#4103 unblocked us by allowing to inject arbitrary content between the InMemoryTable's searchbar and table. However the EuiSpace added when using childrenBetween conflicts with the expected design, so I have to hack it into display: none;

Unfortunately, this whole file goes against our policies for custom styles on top of EUI. There are a couple of issues:

Don't target .eui classes. All our component accept a className prop which you can use to create your own class instance and append to the EUI component class list. Then target this class name so that, in case EUI ever changes it's naming structure (most likely to happen within the next year), your styles won't break.

Don't use hard-coded pixel values. Always use our SASS variables for sizing and font sizes. Most likely a prop exists on the component to help this as well.

If you're having trouble affecting change on an EUI component, please let us know before targeting the .eui classes directly. Most likely we can get a fix in pretty quickly, or if there's a long-term solution being worked on, for instances like these, it's better to be consistent with the EUI components than to override them.

Don't target .eui classes. All our component accept a className

Unfortunately I can't here. I'm targeting the EuiSpacer that is inside the EuiInMemoryTable, between the childrenBetween node and the table. See elastic/eui#4103 / https://github.com/elastic/eui/blob/master/src/components/basic_table/in_memory_table.tsx#L695.

Yep. Mostly my comment for that specifically is to say, please ask us or create a PR on the EUI side so we can support your needs without needing hacks. I see this as a valid request that we shouldn't really be introducing a hard-coded spacer here and leave it up to the consumer to provide one.

Created elastic/eui#4246

Thanks @pgayvallet ! I've already got a PR up for you to fix this elastic/eui#4246

I'll let it slide for now 😉 because at least when the fix merges into Kibana, your hack won't have any ill effect. I'd def then appreciate a follow-up.

Created #83161. Will do it as soon as elastic/eui#4246 get shipped in next Kibana EUI update.

Thanks again for the quick fix btw

x-pack/plugins/saved_objects_tagging/public/management/components/_action_bar.scss

MichaelMarcialis

This is looking great, @pgayvallet! Excellent work all around. I left a handful of minor comments for your review.

And one final nit: I was wondering if it would also be possible to change the delete modals' max width from the current 768px to 560px. Doing so would better match the design and make the line lengths a little easier on the eyes. Thanks!

x-pack/plugins/saved_objects_tagging/public/management/components/_action_bar.scss

MichaelMarcialis · 2020-11-09T16:19:51Z

x-pack/plugins/saved_objects_tagging/public/management/actions/bulk_delete.ts

+          confirmButtonText: i18n.translate(
+            'xpack.savedObjectsTagging.management.actions.bulkDelete.confirm.confirmButtonText',
+            {
+              defaultMessage: 'Delete {count, plural, one {tag} other {tags}}',
+              values: {
+                count: tagIds.length,
+              },
+            }
+          ),
+          buttonColor: 'danger',


In addition to delete button text and color, would it be possible to also specify an icon (trash) to precede the button text, as shown in the design? This would also apply to the non-bulk, individual delete tag modals as well.

Not right now unfortunately. We had the same issue in the MVP for the single 'delete' action. I created #81482 at this time.

x-pack/plugins/saved_objects_tagging/public/management/components/action_bar.tsx

x-pack/plugins/saved_objects_tagging/public/management/tag_management_page.tsx

pgayvallet · 2020-11-09T19:35:26Z

src/core/public/index.ts

+export {
+  OverlayStart,
+  OverlayBannersStart,
+  OverlayRef,
+  OverlayFlyoutStart,
+  OverlayFlyoutOpenOptions,
+  OverlayModalOpenOptions,
+  OverlayModalConfirmOptions,
+  OverlayModalStart,
+} from './overlays';


There was a lot of missing exports from the overlays package (and a lot of missing associated generated doc files) I saw that when adding the maxWidth option did not update any file from the generated doc, so I fixed it in this PR.

elasticmachine · 2020-11-12T08:00:53Z

Pinging @elastic/apm-ui (Team:apm)

elasticmachine · 2020-11-12T08:00:53Z

Pinging @elastic/ingest-management (Team:Ingest Management)

pgayvallet · 2020-11-12T08:02:59Z

Sorry for the noise, teams. Messed up my rebase.

…k-action-menu

joshdover

A few small issues for robustness, but otherwise LGTM

x-pack/plugins/saved_objects_tagging/public/management/tag_management_page.tsx

joshdover · 2020-11-16T23:38:07Z

x-pack/plugins/saved_objects_tagging/server/routes/internal/bulk_delete.ts

+      for (const tagId of tagIds) {
+        await client.delete(tagId);
+      }


The performance of this is probably going to be fine in 99% of cases, but it is certainly possible for this to partially fail in cases where 1000s of tags are being deleted at once. The reason I think this is acceptable for now is that failing part of the way through (most likely b/c of a timeout) doesn't corrupt the data and the user can easily refresh & try again to delete any remaining tags.

joshdover · 2020-11-16T23:39:01Z

x-pack/plugins/saved_objects_tagging/public/management/tag_management_page.tsx

+        if (action.refreshAfterExecute) {
+          await fetchTags();
+        }


Given that bulk delete could fail part of the way through the list of tags, I think it would make sense to also refresh the tags after a failure, not just in the success case.

…k-action-menu

kibanamachine · 2020-11-18T09:24:11Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`savedObjectsTagging`	51	66	+15

Async chunks

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

id	before	after	diff
`savedObjectsTagging`	59.3KB	81.7KB	+22.4KB

Distributable file count

id	before	after	diff
`default`	42849	42851	+2

Page load bundle

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

id	before	after	diff
`core`	547.4KB	547.9KB	+493.0B
`savedObjectsTagging`	44.1KB	44.7KB	+525.0B
total			+1018.0B

History

💚 Build #88273 succeeded 8e1ce3d
💚 Build #87901 succeeded 3529160
💚 Build #87550 succeeded 58d68fa
💚 Build #87356 succeeded 7c5c9d5
💚 Build #87150 succeeded d68c4a3

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

* add the delete tag bulk action * add unit tests for bulk delete * fix duplicate i18n key * add RBAC test on bulk delete * add functional tests * self review * design nits * add maxWidth option for confirm modal and add missing doc * change bulk delete confirm modal max width * add more missing doc * only show loading state when performing the bulk delete * use spacer instead of custom margin on horizontal rule * use link instead of button to remove custom styles * remove spacers, just use styles * add divider when action menu is displayed * set max-width for single delete confirm * a11y fixes * address nits * add aria-label to delete action Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>

* add the delete tag bulk action * add unit tests for bulk delete * fix duplicate i18n key * add RBAC test on bulk delete * add functional tests * self review * design nits * add maxWidth option for confirm modal and add missing doc * change bulk delete confirm modal max width * add more missing doc * only show loading state when performing the bulk delete * use spacer instead of custom margin on horizontal rule * use link instead of button to remove custom styles * remove spacers, just use styles * add divider when action menu is displayed * set max-width for single delete confirm * a11y fixes * address nits * add aria-label to delete action Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co> Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>

…o-node-details * 'master' of github.com:elastic/kibana: (65 commits) update chromedriver dependency to 87 (elastic#83624) [TSVB] use new Search API for rollup search (elastic#83275) [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438) [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302) Add tag bulk action context menu (elastic#82816) [code coverage] adding plugin to flush coverage data (elastic#83447) [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413) Added eventBus to trigger and listen plotHandler event (elastic#83435) [Runtime fields] Editor phase 1 (elastic#81472) [Maps] Fix threshold alert issue resolving nested fields (elastic#83577) chore(NA): remove usage of unverified es snapshots (elastic#83589) [DOCS] Adds Elastic Contributor Program link (elastic#83561) Upgrade EUI to v30.2.0 (elastic#82730) Don't show loading screen during auto-reload (elastic#83376) Functional tests - fix esArchive mappings with runtime fields (elastic#83530) [deb/rpm] Create keystore after installation (elastic#76465) [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144) [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) ...

add the delete tag bulk action

3106167

pgayvallet added Feature:Saved Object Tagging Saved Objects Tagging feature release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 6, 2020

pgayvallet changed the title ~~add the delete tag bulk action~~ Add tag bulk action context menu Nov 6, 2020

pgayvallet added 6 commits November 6, 2020 17:27

add unit tests for bulk delete

c35af85

fix duplicate i18n key

d93a19a

add RBAC test on bulk delete

e6e50f3

add functional tests

068c769

Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…

74944f8

…k-action-menu

self review

ad7b509

pgayvallet commented Nov 9, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review November 9, 2020 10:49

pgayvallet requested review from a team as code owners November 9, 2020 10:49

pgayvallet requested review from MichaelMarcialis, alexfrancoeur and a team and removed request for MichaelMarcialis November 9, 2020 10:49

MichaelMarcialis reviewed Nov 9, 2020

View reviewed changes

pgayvallet added 4 commits November 9, 2020 19:41

design nits

bdb270d

add maxWidth option for confirm modal and add missing doc

a9989df

change bulk delete confirm modal max width

d05f01c

add more missing doc

32391bb

pgayvallet commented Nov 9, 2020

View reviewed changes

only show loading state when performing the bulk delete

17ff9d1

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 12, 2020

pgayvallet force-pushed the kbn-81980-tag-bulk-action-menu branch from 8f8f1b8 to 71236d5 Compare November 12, 2020 08:02

pgayvallet removed request for a team November 12, 2020 08:02

Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…

58d68fa

…k-action-menu

myasonik mentioned this pull request Nov 12, 2020

Add more review info to bulk delete of tags #83292

Open

pgayvallet mentioned this pull request Nov 13, 2020

Add bulk assign to saved object tagging #81970

Closed

3 tasks

Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…

3529160

…k-action-menu

joshdover approved these changes Nov 16, 2020

View reviewed changes

pgayvallet added 4 commits November 17, 2020 12:27

Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…

02ab502

…k-action-menu

address nits

8e1ce3d

Merge remote-tracking branch 'upstream/master' into kbn-81980-tag-bul…

f03823d

…k-action-menu

add aria-label to delete action

acc1cfd

pgayvallet merged commit a7e5f07 into elastic:master Nov 18, 2020

pgayvallet mentioned this pull request Nov 18, 2020

[7.x] Add tag bulk action context menu (#82816) #83618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tag bulk action context menu #82816

Add tag bulk action context menu #82816

pgayvallet commented Nov 6, 2020 •

edited

Loading

pgayvallet left a comment

pgayvallet Nov 9, 2020

pgayvallet Nov 9, 2020

joshdover Nov 16, 2020

pgayvallet Nov 9, 2020

cchaos Nov 10, 2020

pgayvallet Nov 10, 2020 •

edited

Loading

cchaos Nov 10, 2020

pgayvallet Nov 10, 2020

cchaos Nov 10, 2020

pgayvallet Nov 11, 2020

MichaelMarcialis left a comment

MichaelMarcialis Nov 9, 2020

pgayvallet Nov 9, 2020

pgayvallet Nov 9, 2020

elasticmachine commented Nov 12, 2020

elasticmachine commented Nov 12, 2020

pgayvallet commented Nov 12, 2020

joshdover left a comment

joshdover Nov 16, 2020

joshdover Nov 16, 2020

kibanamachine commented Nov 18, 2020

Add tag bulk action context menu #82816

Add tag bulk action context menu #82816

Conversation

pgayvallet commented Nov 6, 2020 • edited Loading

Summary

Screenshots

Checklist

pgayvallet 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

pgayvallet Nov 10, 2020 • edited Loading

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

MichaelMarcialis 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

elasticmachine commented Nov 12, 2020

elasticmachine commented Nov 12, 2020

pgayvallet commented Nov 12, 2020

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Nov 18, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

History

pgayvallet commented Nov 6, 2020 •

edited

Loading

pgayvallet Nov 10, 2020 •

edited

Loading