Add `bulk assign` action to tag management #84177

pgayvallet · 2020-11-24T07:24:00Z

Summary

Part of #81970

This PR adds the assign (per tag) and bulk_assign (multiple tags) action to the tag management section.

add the new assign and bulk_assign actions
extract the per-tag actions from the page/table component, for better isolation and consistency with the bulk_actions

Screenshots

Bulk action

Column action

Flyout

Filtering

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
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

…sign-tags

ryankeairns · 2020-11-30T22:13:59Z

@ryankeairns, am I correct in my assumptions above? If so, would it be worth it for me to write up an EUI issue to ask that we not show any isPrimary action in the popover menu unless the user is viewing from a smaller viewport size where those primary buttons are hidden?

I just posted and deleted a similar message :) A separate EUI dicuss item seems like a good approach as this is recommended behavior, per the docs, to include all links in the popover.

pgayvallet · 2020-12-01T06:52:13Z

Do we need both an edit icon and a context menu item to edit a tag?

I was surprised of this behavior to be honest, but I don't have any control over it. As @ryankeairns explained:

The EUI docs suggest that any number of actions greater than two will result in this 'more actions' button. Per the docs, all links would then appear in the popover menu

This would need to be discussed upstream

…sign-tags

…es in its name

pgayvallet · 2020-12-01T07:05:24Z

@alexfrancoeur the bug when navigating to SO management by selecting a tag with whitespaces is fixed by 385bab9

ryankeairns · 2020-12-01T15:16:02Z

Do we need both an edit icon and a context menu item to edit a tag?

I was surprised of this behavior to be honest, but I don't have any control over it. As @ryankeairns explained:

The EUI docs suggest that any number of actions greater than two will result in this 'more actions' button. Per the docs, all links would then appear in the popover menu

This would need to be discussed upstream

Created an EUI discuss item to follow up on that overflow menu behavior; to be tracked aside from this PR.
elastic/eui#4326

joshdover · 2020-12-02T23:42:44Z

ack: will do thorough in-depth code review tomorrow

…sign-tags

joshdover

The implementation here is super solid. Mostly small nits, nothing major to block this. Thanks Pierre! 🎉

joshdover · 2020-12-03T18:33:21Z

x-pack/plugins/saved_objects_tagging/common/constants.ts

+/**
+ * The list of saved object types that are currently supporting tagging.
+ */
+export const taggableTypes = ['dashboard', 'visualization', 'map', 'lens'];


This is definitely a little out of the norm, but I'm willing to entertain the idea. The main con I see to this option is a poorer Dev XP. What would the consequences be of forgetting to add a type name to this array? Or what if a type changes its name? Another con is that external plugins wouldn't be able to integrate with this, but I don't think that's particularly important for this feature at this time.

Will this create any issues if the plugins that provide these SO types are disabled? For instance if xpack.maps.enabled: false is set.

x-pack/plugins/saved_objects_tagging/common/references.ts

joshdover · 2020-12-03T21:55:56Z

x-pack/plugins/saved_objects_tagging/public/components/assign_flyout/assign_flyout.tsx

+  return assignedCount === 0 ? 'none' : assignedCount === assignedTags.length ? 'full' : 'partial';
+};
+
+export const AssignFlyout: FC<AssignFlyoutProps> = ({


Since this component has a decent amount of logic, should we add unit tests or is this covered elsewhere?

TBH, snapshot testing is imho very inefficient, and I basically assumed that the FTR coverage was enough. We would probably need to setup storybook for the plugin if we want to have proper UT coverage on the components?

x-pack/plugins/saved_objects_tagging/public/components/assign_flyout/open_assign_flyout.tsx

joshdover · 2020-12-03T22:05:14Z

x-pack/plugins/saved_objects_tagging/public/components/assign_flyout/lib/parse_query.ts

+  selectedTypes?: string[];
+}
+
+export function parseQuery(query: Query): ParsedQuery {


nit: the naming here is a bit misleading since the parsing is actually done prior to calling this function. Maybe selectedTypesFromQuery would be more appropriate?

x-pack/plugins/saved_objects_tagging/public/management/bulk_actions/index.test.ts

joshdover · 2020-12-03T22:21:53Z

x-pack/plugins/saved_objects_tagging/server/services/assignments/assignment_service.ts

+    // if we failed to fetch any object, just halt and throw an error
+    const firstObjWithError = objects.find((obj) => !!obj.error);
+    if (firstObjWithError) {
+      const firstError = firstObjWithError.error!;
+      throw new AssignmentError(firstError.message, firstError.statusCode);
+    }


Sounds like a smart decision 😄

joshdover · 2020-12-03T22:22:47Z

x-pack/plugins/saved_objects_tagging/server/services/assignments/assignment_service.ts

+export class AssignmentService {
+  private readonly soClient: SavedObjectsClientContract;
+  private readonly typeRegistry: ISavedObjectTypeRegistry;
+  private readonly authorization?: SecurityPluginSetup['authz'];
+  private readonly request: KibanaRequest;
+
+  constructor({ client, typeRegistry, authorization, request }: AssignmentServiceOptions) {
+    this.soClient = client;
+    this.typeRegistry = typeRegistry;
+    this.authorization = authorization;
+    this.request = request;
+  }


I think that unless we anticipate having a non-scoped version, AssignmentService is fine for brevity's sake.

joshdover · 2020-12-03T22:33:19Z

x-pack/plugins/saved_objects_tagging/server/services/assignments/assignment_service.ts

+
+    const searchFields = uniq(
+      assignableTypes.map(
+        (name) => this.typeRegistry.getType(name)?.management!.defaultSearchField!


Do we validate elsewhere that these types all have this defined? If not, this could be a easy-to-create bug that wouldn't easily be caught.

uniq implementation removes all undefined values from the array, so worse case, missing search fields will be ignored.

x-pack/plugins/saved_objects_tagging/server/routes/assignments/update_tags_assignments.ts

…sign-tags

kibanamachine · 2020-12-07T10:08:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5fb4119

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`savedObjectsTagging`	66	99	+33

Async chunks

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

id	before	after	diff
`savedObjectsTagging`	81.7KB	115.6KB	+33.9KB

Distributable file count

id	before	after	diff
`default`	46897	47675	+778

Page load bundle

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

id	before	after	diff
`savedObjectsTagging`	44.7KB	48.8KB	+4.1KB

Unknown metric groups

async chunk count

id	before	after	diff
`savedObjectsTagging`	3	4	+1

History

💚 Build #91811 succeeded 3356ebd
💚 Build #90945 succeeded 385bab9
💚 Build #90630 succeeded 9e99947
💚 Build #90407 succeeded 62a758a
💚 Build #90351 succeeded 8be87b9

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

* initial draft * move components to their own files * create services folder and move tags package * add assignment service * fix some types * prepare assign tag route * move server-side tag client under the `services` folder * add security check, move a lot of stuff. * design improvements * display tags in flyout * improve button and add notification on save * add action on tag rows * fix types * fix mock import paths * add lens to the list of assignable types * update generated doc * add base functional tests * move api to internal * add api/security test suites * add / use get_assignable_types API * fix feature control tests * fix assignable types propagation * rename actions folder to bulk_actions * extract actions to their own module * add common / server unit tests * add client-side assign tests * add some tests and tsdoc * typo * add getActions test * revert width change * fix typo in API * various minor improvements * typo * tsdoc on flyout page object * close flyout when leaving the page * fix bug when redirecting to SO management with a tag having whitespaces in its name * check for dupes in toAdd and toRemove * add lazy load to assign modal opener * add lazy load to edit/create modals * check if at least one assign or unassign tag id is specified * grammar * add explicit type existence check

* Add `bulk assign` action to tag management (#84177) * initial draft * move components to their own files * create services folder and move tags package * add assignment service * fix some types * prepare assign tag route * move server-side tag client under the `services` folder * add security check, move a lot of stuff. * design improvements * display tags in flyout * improve button and add notification on save * add action on tag rows * fix types * fix mock import paths * add lens to the list of assignable types * update generated doc * add base functional tests * move api to internal * add api/security test suites * add / use get_assignable_types API * fix feature control tests * fix assignable types propagation * rename actions folder to bulk_actions * extract actions to their own module * add common / server unit tests * add client-side assign tests * add some tests and tsdoc * typo * add getActions test * revert width change * fix typo in API * various minor improvements * typo * tsdoc on flyout page object * close flyout when leaving the page * fix bug when redirecting to SO management with a tag having whitespaces in its name * check for dupes in toAdd and toRemove * add lazy load to assign modal opener * add lazy load to edit/create modals * check if at least one assign or unassign tag id is specified * grammar * add explicit type existence check * fix mappings for 7.x

pgayvallet added 25 commits November 23, 2020 16:32

initial draft

3180729

move components to their own files

11d3380

create services folder and move tags package

cae00e0

add assignment service

08a2eed

Merge remote-tracking branch 'upstream/master' into kbn-81970-bulk-as…

51751f8

…sign-tags

fix some types

3c73596

prepare assign tag route

403a473

move server-side tag client under the services folder

110a272

add security check, move a lot of stuff.

e4c5d77

design improvements

cb5fd88

display tags in flyout

b038536

improve button and add notification on save

5ef0734

add action on tag rows

862b0cc

fix types

d10160c

fix mock import paths

9dfb1c9

Merge remote-tracking branch 'upstream/master' into kbn-81970-bulk-as…

f6ed56c

…sign-tags

add lens to the list of assignable types

3526e37

update generated doc

82148bc

add base functional tests

5080202

move api to internal

059c699

add api/security test suites

e6ae5c8

add / use get_assignable_types API

160aa49

fix feature control tests

361eb3f

fix assignable types propagation

6103922

rename actions folder to bulk_actions

60ed0b5

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 25, 2020

pgayvallet added 2 commits December 1, 2020 07:54

Merge remote-tracking branch 'upstream/master' into kbn-81970-bulk-as…

e32e0fd

…sign-tags

fix bug when redirecting to SO management with a tag having whitespac…

385bab9

…es in its name

pgayvallet added Feature:Saved Object Tagging Saved Objects Tagging feature and removed Feature:Embedding Embedding content via iFrame labels Dec 1, 2020

pgayvallet mentioned this pull request Dec 1, 2020

Add bulk assign to saved object tagging #81970

Closed

3 tasks

Merge remote-tracking branch 'upstream/master' into kbn-81970-bulk-as…

3356ebd

…sign-tags

botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 3, 2020

pgayvallet removed the Feature:Embedding Embedding content via iFrame label Dec 3, 2020

joshdover approved these changes Dec 3, 2020

View reviewed changes

botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 3, 2020

pgayvallet added 7 commits December 7, 2020 08:06

Merge remote-tracking branch 'upstream/master' into kbn-81970-bulk-as…

2a6c220

…sign-tags

check for dupes in toAdd and toRemove

f8516f6

add lazy load to assign modal opener

9fba665

add lazy load to edit/create modals

6ff5268

check if at least one assign or unassign tag id is specified

b0d11a0

grammar

a58269d

add explicit type existence check

5fb4119

pgayvallet merged commit 446390d into elastic:master Dec 7, 2020

pgayvallet mentioned this pull request Dec 7, 2020

[7.x] Add bulk assign action to tag management (#84177) #85119

Merged

This was referenced Dec 9, 2020

Add saved object tags to integrations with Kibana saved objects elastic/integrations#327

Closed

[Discuss] Documentation for Tags and Navigational Search #85630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `bulk assign` action to tag management #84177

Add `bulk assign` action to tag management #84177

pgayvallet commented Nov 24, 2020 •

edited

Loading

ryankeairns commented Nov 30, 2020

pgayvallet commented Dec 1, 2020

pgayvallet commented Dec 1, 2020

ryankeairns commented Dec 1, 2020

joshdover commented Dec 2, 2020

joshdover left a comment

joshdover Dec 3, 2020

joshdover Dec 3, 2020

pgayvallet Dec 4, 2020

joshdover Dec 3, 2020

joshdover Dec 3, 2020

joshdover Dec 3, 2020

joshdover Dec 3, 2020

pgayvallet Dec 7, 2020

kibanamachine commented Dec 7, 2020

async chunk count

Add bulk assign action to tag management #84177

Add bulk assign action to tag management #84177

Conversation

pgayvallet commented Nov 24, 2020 • edited Loading

Summary

Screenshots

Checklist

ryankeairns commented Nov 30, 2020

pgayvallet commented Dec 1, 2020

pgayvallet commented Dec 1, 2020

ryankeairns commented Dec 1, 2020

joshdover commented Dec 2, 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

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

kibanamachine commented Dec 7, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

async chunk count

History

Add `bulk assign` action to tag management #84177

Add `bulk assign` action to tag management #84177

pgayvallet commented Nov 24, 2020 •

edited

Loading