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

Add bulk assign to saved object tagging #81970

Closed
3 tasks
alexfrancoeur opened this issue Oct 29, 2020 · 33 comments
Closed
3 tasks

Add bulk assign to saved object tagging #81970

alexfrancoeur opened this issue Oct 29, 2020 · 33 comments
Labels
Feature:Saved Object Tagging Saved Objects Tagging feature REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@alexfrancoeur
Copy link

alexfrancoeur commented Oct 29, 2020

Building off of #74571 and #79096, we'd like to add bulk actions for adding tags to existing saved objects. Members of our community can have hundreds if not thousands of visualizations and dashboards. Having a quick easy way to apply a tag to all saved objects will make this transition significantly easier.

There are some early flows from @MichaelMarcialis here:
https://www.figma.com/proto/kh7Gm1iM0zJPj504BeDQC4/Mockups-Version-3-Awaiting-Dev?node-id=445%3A0&viewport=13425%2C-7073%2C0.5&scaling=min-zoom

I'm thinking we may want this experience in the tag manager, but would like to open up the discussion. Here's the flow I'm envisioning:

  • Click into a tag action "Assign"
  • Open fly out with a similar experience as the add panel view in a dashboard that allows you to filter on supported types (in this case SO's that support tags)
  • Filter by search / type, select appropriate tags via checkboxes
  • Assign tag
  • Repeat for another tag

This flow would of course adhere to RBAC permissions.

What do you think @MichaelMarcialis @ryankeairns @pgayvallet ? This would be really nice to augment the MVP PR for our initial release.

Tasks

@alexfrancoeur alexfrancoeur added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc REASSIGN from Team:Core UI Deprecated label for old Core UI team labels Oct 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@pgayvallet pgayvallet added the Feature:Saved Object Tagging Saved Objects Tagging feature label Oct 29, 2020
@pgayvallet pgayvallet changed the title Add bulk actions to saved object tagging Add bulk assign to saved object tagging Oct 29, 2020
@pgayvallet
Copy link
Contributor

This is definitely something we want.

Still waiting on elastic/eui#4095 to be integrated to Kibana to be able to implement the design-compliant bulk action menu. @chandlerprall, can you tell us when the next EUI update is planned for Kibana?

Just a technical remark:

Open fly out with a similar experience as the add panel view in a dashboard that allows you to filter on supported types (in this case SO's that support tags)

The dashboard panel selector (that used the SavedObjectFinder component) is a single-selection widget. We probably gonna need a brand new component to allow multi-selection from this flyout.

Also,

The second, (complementary) option to this proposal would be to have a new bulk assign tag action available from the SO management section.

  • select objects
  • assign tags
  • opens a modal/flyout
  • select tags
  • validate
  • tags are assigned to the selected objects

@MichaelMarcialis I think we do, but could you confirm we got mockups for both worflows (and share them here to continue the discussion?)

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis I think we do, but could you confirm we got mockups for both worflows (and share them here to continue the discussion?)

@pgayvallet: We do have mockups and prototypes for assigning one or more tags to multiple saved objects on the saved objects page, as shown here: https://www.figma.com/proto/kh7Gm1iM0zJPj504BeDQC4/Mockups-Version-3-Awaiting-Dev?node-id=445%3A0&viewport=13425%2C-7073%2C0.5&scaling=min-zoom

I would like to expand this further by also providing a row-level action on the saved objects page table that allows users to assign one or more tags to a single saved object.

After that is implemented, I feel the next highest priority should be to incorporate those flows for assigning one or more tags to saved objects and apply the same interface to the Kibana applications' main pages. I think asking users to manage the meta details of their saved objects from the saved objects page makes sense in the short term, but longer term I believe users will wish to make such bulk tag assignments directly from the Kibana applications they are working within.

@alexfrancoeur, regarding your thoughts on having users go to the tag management page instead (to pick a tag they would like to assign to one or more saved objects), I agree that this could be a viable use case. However, I do worry that may end up being more of an edge case that comes secondary to users wishing to make such tag changes directly from the applications they're using. Thoughts?

@alexfrancoeur
Copy link
Author

The second, (complementary) option to this proposal would be to have a new bulk assign tag action available from the SO management section.

++ and that's what was originally proposed in the mocks I believe. This is probably more of an edge case, but I could see potentially having write permissions for tag management and none for saved objects. That is why I was leaning to having one option for bulk tag assignment to be done through the tag management UI.

agree that this could be a viable use case. However, I do worry that may end up being more of an edge case that comes secondary to users wishing to make such tag changes directly from the applications they're using. Thoughts?

I agree that most users would prefer to assign from the application themselves. That was where my head was at with this comment #79096 (comment). I also think we probably want a more administrative way to do this as well. Both use cases seem valid to me. At this point, I'd lean towards whatever can ship with the first release if it's possible. My concern is that adoption (#81847) may not be as impactful with clusters that need this functionality most. I'm happy to iterate, but whatever can get us this type of functionality in MVP would be my preferred approach 😉 If that's from the SO management UI, that works for me. In which case, we simply lean into the app first route.

@MichaelMarcialis
Copy link
Contributor

I'm happy to iterate, but whatever can get us this type of functionality in MVP would be my preferred approach

Yeah, good point. Once we know which direction we'd be able to implement first, let me know and I'll be happy to mock it up.

@pgayvallet
Copy link
Contributor

This is probably more of an edge case, but I could see potentially having write permissions for tag management and none for saved objects. That is why I was leaning to having one option for bulk tag assignment to be done through the tag management UI.

You're right, this is probably an edge case, but I guess if we were to only have one initial way to assign tags, it probably makes more sense to do so from the tags management instead of SO management, so it is probably better to start with that. (even if we should be able to implement both for 7.11)

@MichaelMarcialis could you start working on mockups for this workflow then?

Bonus question to @alexfrancoeur: Do we want a bulk unassign tag action (in which case we gonna need the mockups for that too)

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis could you start working on mockups for this workflow then?

Yeah, I'll plan to work up some mockups for this.

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 4, 2020

(extracted from #81980 (comment))

Regarding the bulk assign action:

A user can have write or read permission to SO tagging (therefor being allowed to assign tags), while still only having read permission to other SOs. That can make the bulk assign object-searching logic difficult, as we gonna want to only display objects the user has write permission to (else it will cause a permission error when trying to update the object to add the tag reference). I think that's kinda the first time we have this need to only return from a read operation objects that the active user has write permission to. @legrego @kobelb are you aware of any precedence for that? How should we achieve that ? We may be retrieve the list of SO type the user have write permission to and restrict the find operation to them? I don't think the SO permissions are directly accessible from core's Capabilities or the features plugin though, so how can I retrieve that? Another easy workaround would be to only display the bulk assign action if the user got SO management write permission, even if ideally, we would avoid such restriction.

(depending on previous point) the 'assign tag' permission is available for users having read permission to the SO tagging feature. These users should still be able to have the bulk action menu and should be able to bulk assign tags to objects they have write permission to right?

Note that this permission issue is only present for the bulk assign from the tag management section. From the SO section, we know that if the user got write permission to the SO management section, he got write permission to all types of SOs.

@legrego
Copy link
Member

legrego commented Nov 4, 2020

I think that's kinda the first time we have this need to only return from a read operation objects that the active user has write permission to. @legrego @kobelb are you aware of any precedence for that?

We don't have precedent for this for "normal" saved objects. The SpacesClient offers a getAll function which takes an optional purpose. Using this, we can ask the Spaces plugin to retrieve all spaces that the user is authorized to perform a specific action in. For example, spacesClient.getAll('shareSavedObjectsIntoSpace') returns the spaces the user is authorized to share saved objects into, and exclude all other spaces, even if they are authorized to access them for other purposes. This is very much a one-off implementation though, and not something that's designed to scale.

How should we achieve that ? We may be retrieve the list of SO type the user have write permission to and restrict the find operation to them?

Something like this might work. The downside is that it would require two API calls to fulfill the request: a pre-flight check to get the authorized types, and another to actually retrieve the saved objects.

It would be great if X-Pack plugins could augment the SO APIs, but we don't have the infrastructure in place to support that. With this, we could add a similar purpose option to the SO find operation, to avoid the extra round-trip.

I don't think the SO permissions are directly accessible from core's Capabilities or the features plugin though, so how can I retrieve that?

When we first designed UI Capabilities, we intentionally omitted such implementation details like: "Can I create a saved object of type search?" and instead opted for more user-centric operations like: "Should the save button in the Discover be enabled?"

Another easy workaround would be to only display the bulk assign action if the user got SO management write permission, even if ideally, we would avoid such restriction.

I agree. This is arguably the easiest approach, but the SO Management privileges are rather powerful, so it would prevent a good number of users from taking advantage of this feature.

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 5, 2020

I think that's kinda the first time we have this need to only return from a read operation objects that the active user has write permission to

We don't have precedent for this for "normal" saved objects.

Following our discussion from yesterday, we will soon have similar needs for OLS. For OLS, we will need to have the 'read/write' permission info on each individual object returned from the SO api. Note that this would work for tagging too: If the find API was returning, for each object, if the user has write permission for it, we could just filter the results. However as mentioned yesteday, this would cause issues with pagination, so it's probably not good enough, and we would really need a way to only return the objects with read permission from ES in the first place.

Something like this might work. The downside is that it would require two API calls to fulfill the request: a pre-flight check to get the authorized types, and another to actually retrieve the saved objects.

Do we really need to perform an ES call to get the write authorized types for the user? I thought security would have knowledge of that internally from the registered capabilities and/or features?

we could add a similar purpose option to the SO find operation

That would leaks (again) licensed features into oss, but at this point, I don't really mind anymore until the big SO v2 discussion arises. Having security being able to perform this check 'implicitly' just by passing an additional parameter to SOC.find sounds the easier / most pragmastic solution for consumers.

However, I wonder if we shouldn't wait for more similar needs to arise before even thinking about doing that. The 'tagging' references and their technical implications here are quite special, so maybe we shouldn't do this just for tagging's specific requirements. If manually accessing the list of types the user has write permission to is an option, it would be good enough for us to manually implement this logic.

This is arguably the easiest approach, but the SO Management privileges are rather powerful, so it would prevent a good number of users from taking advantage of this feature.

Yea, this would be our last resort if we can't have anything else working for 7.11 (cc @alexfrancoeur)

@alexfrancoeur
Copy link
Author

Great discussion! Responding to a few direct pings

Yea, this would be our last resort if we can't have anything else working for 7.11

++ If we can't do anything else for 7.11, I think this would be the last resort so that the capability itself is available.

Bonus question to @alexfrancoeur: Do we want a bulk unassign tag action (in which case we gonna need the mockups for that too)

I think this request will naturally come up. If it's fairly trivial to add to the MVP along with bulk assign, let's do it. If not, we can push to a subsequent release.

More generally, if it impacts the the amount of time needed to build the better user experience for the next release, I would aim to support this type of functionality for tags first and defer decisions to support OLS or other SO's that may need this requirement in the future if we can. That being said, I'm not sure how feasible that is or what amount of tech debt we'd incur, but OLS feels further out

@legrego
Copy link
Member

legrego commented Nov 5, 2020

Do we really need to perform an ES call to get the write authorized types for the user? I thought security would have knowledge of that internally from the registered capabilities and/or features?

Kibana knows about all of the registered capabilities, but only Elasticsearch knows which of those the user is authorized for based on their assigned roles. The security plugin has the knowledge to ask the question, but it needs Elasticsearch to provide the answer.

That would leaks (again) licensed features into oss, but at this point, I don't really mind anymore until the big SO v2 discussion arises. Having security being able to perform this check 'implicitly' just by passing an additional parameter to SOC.find sounds the easier / most pragmastic solution for consumers.

++ agree with all of this.


Another short-term alternative is to have the tagging plugin expose its own API to retrieve these objects. If the tagging plugin added an optional dependency on the security plugin, then tagging could perform its unique authorization check up front to get the list of "updatable" types, and then pass that list of types through to the normal SO find operation.

@pgayvallet
Copy link
Contributor

Another short-term alternative is to have the tagging plugin expose its own API to retrieve these objects.

That's what I was planning to do anyway to be honest. The tag type has its custom client, so adding additional methods for tag specific operations such as this one makes sense.

If the tagging plugin added an optional dependency on the security plugin, then tagging could perform its unique authorization check up front to get the list of "updatable" types

That would be fine to me, and is probably the most pragmatic approach for the short term.

Would you mind giving me some pointers on how to use the security plugin to perform such check / retrieve the list of 'updatable'/'writable' types using its API?

@legrego
Copy link
Member

legrego commented Nov 6, 2020

Would you mind giving me some pointers on how to use the security plugin to perform such check / retrieve the list of 'updatable'/'writable' types using its API?

Here's a code sample that I briefly tested. I used all visible types here, but feel free to adjust that as needed:

  public async getUpdatableSavedObjectTypes(
    request: KibanaRequest,
    typeRegistry: Pick<SavedObjectTypeRegistry, 'getVisibleTypes'>,
    authorization?: SecurityPluginSetup['authz']
  ) {
    const visibleTypes = typeRegistry.getVisibleTypes();

    // Don't bother authorizing if the security plugin is disabled, or if security is disabled in ES
    const shouldAuthorize = authorization?.mode.useRbacForRequest(request) ?? false;
    if (!shouldAuthorize) {
      return visibleTypes;
    }

    // Each Saved Object type has a distinct privilege/action that we need to check
    const typeActionMap = visibleTypes.reduce((acc, type) => {
      return {
        ...acc,
        [type.name]: authorization!.actions.savedObject.get(type.name, 'update'),
      };
    }, {} as Record<string, string>);

    // Perform the privilege check
    const checkPrivileges = authorization!.checkPrivilegesDynamicallyWithRequest(request);
    const { privileges } = await checkPrivileges({ kibana: Object.values(typeActionMap) });

    // Filter results to only include the types that passed the authorization check above.
    return visibleTypes.filter((type) => {
      const requiredPrivilege = typeActionMap[type.name];

      const hasRequiredPrivilege = privileges.kibana.some(
        (kibanaPrivilege) =>
          kibanaPrivilege.privilege === requiredPrivilege && kibanaPrivilege.authorized === true
      );

      return hasRequiredPrivilege;
    });
  }

@MichaelMarcialis
Copy link
Contributor

Hey, gang. Per our previous conversations, I've updated the saved object tagging design mockups and prototype to account for bulk tag assignment to saved objects via the tag management page. I've put together a quick design update video that runs through these changes. We can discuss further in today's meeting, but feel free to let me know if you have any feedback in the mean time. Thanks.

One Saved Object

CCing @alexfrancoeur, @pgayvallet, @ryankeairns.

@pgayvallet
Copy link
Contributor

After our sync discussion from yesterday, I think we got all we need to start working on this issue once #82816 is merged

@ryankeairns
Copy link
Contributor

ryankeairns commented Nov 20, 2020

See related discussion in #83590 where we seem to be arriving at keeping tags in SO management (update: in addition to the Tags management UI).

cc:/ @kobelb

@alexfrancoeur
Copy link
Author

alexfrancoeur commented Nov 20, 2020 via email

@ryankeairns
Copy link
Contributor

ryankeairns commented Nov 20, 2020

Ah yes, I can see how the wording of my comment was unclear. What I meant was, tags are already in SO management, so we should keep them there along with the bulk editing (in addition to having this feature in the Tags UI).

@alexfrancoeur
Copy link
Author

++ thanks for clarifying Ryan. I think we can continue with this work as is then @pgayvallet

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 23, 2020

@MichaelMarcialis @ryankeairns @alexfrancoeur I'm currently working on the UI implementation of the flyout, and I found something that we may want to change regarding the user experience. It is about the X full & Y partially selected saved objects

Screenshot 2020-11-23 at 14 25 15

It actually doesn't reflect the actions that will be performed when clicking the CTA (save) of the flyout.

A quick example:

  • User open the flyout by selecting some arbitrary tags on the table and then clicking on Manage assignments from the action menu. (Some of these tags are already fully and/or partially assigned to some objects)
  • The flyout opens, and the label displays 3 full & 2 partially selected saved objects
  • User deselects the 3 full and 2 partially selected saved objects by clicking on them
  • The label displays 0 selected saved object
  • As the user, I would expect that clicking on save would be a NO-OP, as 'nothing is selected'
  • When clicking on save, I'm actually unassigning the tags from the objects that I clicked on.

I think a wording stating the real actions that will be performed would be really better, something like will assign tags to X objects and removed assignement from Y objects. At least this info should appear somewhere before executing the action. It could either be here, or in a confirmation modal when the user click saved (the current prototype doesn't seem to have such confirmation modal), but it has to be somewhere.

Also, overall, I think in term of UX, it would be great if the user would have a way to see which results are going to be updated / are modified directly from the search results.

For instance, in the following screenshot, I selected and unselected some tag assignments. But I have no way to see which line/result I performed that on / which ones I will update when clicking on Save. All I see if the 'final' state.

Screenshot 2020-11-23 at 14 36 06

WDYT?

@alexfrancoeur
Copy link
Author

I think a wording stating the real actions that will be performed would be really better, something like will assign tags to X objects and removed assignement from Y objects.

I agree that more explicit text would be helpful here

But I have no way to see which line/result I performed that on / which ones I will update when clicking on Save. All I see if the 'final' state.

++ I agree this would be helpful as well. @MichaelMarcialis @ryankeairns is there UX we can "borrow" from the saved object management copy to space conflict resolution experience? (wow, that was a mouthful 😄 )

@ryankeairns
Copy link
Contributor

Michael is out, so I'll take a look a the conflict resolution stuff. There may also be a way to separate the two actions (assign vs unassign) within the flyout and avoid this matrix of possible combinations.

@pgayvallet
Copy link
Contributor

There may also be a way to separate the two actions (assign vs unassign) within the flyout and avoid this matrix of possible combinations.

As long as this is compatible with EuiSelectable (or any other component we might use instead), I'm fine with it. Note that both the client-side and server-side code may be heavily impacted by such changes. Should I stop working on the implementation until we know what the alternative are?

@ryankeairns
Copy link
Contributor

ryankeairns commented Nov 23, 2020

Perhaps, although what I'm thinking would use the same components. Here is a rough sketch...
I'm about to jump on a meeting but will explain furhter. Basically wondering if it simplifies the UX by separating the two actions to avoid the partial state.

Screen Shot 2020-11-23 at 11 58 27 AM

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 23, 2020

Basically wondering if it simplifies the UX by separating the two actions to avoid the partial state.

So, very quick thoughts:

  • It resolves the X partially and Y fully selected issue, as we would now be only displaying X selected (for assign / unassign)

For the 'new' bulk assign:

  • I think we would still have 3 'initial' states: unselected, selected, partially selected? However when checking then unchecking a 'partially selected' object, it would go back to 'partially selected' (where, in current mockups, it would go to 'unselected' and remove the partial tag assignments on apply). This seems better.
  • Main question is regarding results that are already 'selected' (already assigned to all the tags). What would then happen when the user click on it? Basically nothing, as we no longer allow to unassign from this 'mode'. In that case, should we exclude these results? Note that technically, we can't ask the SO find API to 'find all objects that are NOT assigned to XXX tag(s), so this filtering would have to be performed manually.

For the 'new' bulk unassign:

  • the main upside I see is that we would be able to filter and only display results that are currently assigned to any of the tags we want to unassign.
  • regarding the tags displayed on the right: Due to the limited space, and the fact that the EuiSelectable requires fixed-height results, we would need the 'display only the first X tags' component we talked about for the navigational search. this one is not developed yet (current TagList component just display them all)

@ryankeairns
Copy link
Contributor

Thanks for the quick feedback. I'm out of meetings and will continue iterating as quickly as possible.

@ryankeairns
Copy link
Contributor

ryankeairns commented Nov 23, 2020

Ok, I'm backtracking here a bit, but I think this gets us to a better spot and does not deviate materially from the original design.

Proposed changes (as seen below)

  • For a stronger visual reference, show the tags in the header (nice-to-have; strong visual cue; plenty of space)
  • Bulk edit controls: simplify text to show either 'X currently assigned' or 'X pending changes' (no mixing of full/partial states)
  • Add a reset link to get back to the original state (nice-to-have; otherwise we just rely on the 'Cancel' button in footer)
  • Show the change status in each row to clarify what you are proposing... also, it's quick confirmation that these add up to the 'X pending changes' in the bulk edit controls
  • Sort the list to show the 'currently assigned' objects first (nice-to-have, but makes it easier to recall the starting state)

Screen Shot 2020-11-23 at 4 26 36 PM

Let me know what questions you have. I'm feeling much better about this set of changes.

One shortcoming is that you would have to potentially click many 'currently assigned' objects in order to unassign a large set, but that feels like a less common use case. Alternatively, you could just delete the tag (and create/bulk assign a new tag) if you have that many to change. Scratch that - you could simply 'Deselect all' and then apply changes.

@pgayvallet
Copy link
Contributor

@ryankeairns I really like it. It addresses the changes/status visibility while staying closer to the original design. These seem all minor changes, will try adding them asap.

@alexfrancoeur
Copy link
Author

I think we can close this out now, ya?

@pgayvallet
Copy link
Contributor

I think we can close this out now, ya?

Would be happy to. Just want to be sure we are alright closing it, even if all subtasks listed on the initial description are not completed:

(done) Add bulk assign from tag management - #84177
(not done) Add bulk assign from SO management
(not done) Add APIs to allow bulk assign from application listing pages

I guess we can close this one and reopen another issue if we want to continue later, wdty @alexfrancoeur?

@alexfrancoeur
Copy link
Author

👍 let's close this out. I've opened #88204 for application specific bulk assignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Object Tagging Saved Objects Tagging feature REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants