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

Limit importers and exporters by user #201

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Conversation

laritakr
Copy link
Contributor

Story

Refs

Prior work limited the ability to view the importer and exporter pages by user role, but did not limit what importers and exporters could be seen.

With this work, only admin users can see all importers and exporters, while other users can only see importers and exporters they have created.

Expected Behavior Before Changes

Users with advanced depositor role can see all importers and exporters.

Expected Behavior After Changes

Only admin can see all importers and exporters. Advanced depositors can only see what they have created.

Screenshots / Video

Screenshot 2023-11-28 at 5 34 02 PM

Screenshot 2023-11-28 at 5 24 52 PM

Screenshot 2023-11-28 at 5 26 12 PM

Screenshot 2023-11-28 at 5 27 32 PM

Notes

Refs
- #123

Prior work limited the ability to view the importer and exporter pages
by user role, but did not limit what importers and exporters could be
seen.

With this work, only admin users can see all importers and exporters,
while other users can only see importers and exporters they have created.
Copy link
Contributor

@bkiahstroud bkiahstroud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR introduces complex permissions surround importers and exporters, I think writing CanCanCan rules is probably the way we want to go (as opposed to the current #check_permissions overrides). Especially if this feature gets pushed back to Hyku.

In my head, this would look like:

  1. Creating a #bulkrax_permissions method somewhere (either in Bulkrax itself, app/models/ability.rb, or an ability concern like app/models/concerns/hyrax/ability/collection_ability.rb in Hyku)
  2. Define can rules pertaining to importers and exporters1
  3. Add :bulkrax_permissions to Ability.ability_logic

1 E.g.

can :read, Bulkrax::Importer, user_id: current_user.id

@jeremyf
Copy link
Contributor

jeremyf commented Nov 29, 2023

Since this PR introduces complex permissions surround importers and exporters, I think writing CanCanCan rules is probably the way we want to go (as opposed to the current #check_permissions overrides). Especially if this feature gets pushed back to Hyku.

In my head, this would look like:

  1. Creating a #bulkrax_permissions method somewhere (either in Bulkrax itself, app/models/ability.rb, or an ability concern like app/models/concerns/hyrax/ability/collection_ability.rb in Hyku)
  2. Define can rules pertaining to importers and exporters1
  3. Add :bulkrax_permissions to Ability.ability_logic

In principle I really do agree with this. And this was our initial assumed approach. But once we articulated that approach, it felt like a lot of work, so we looked at how we could shorten that time. However, there are a few complicating facts:

  1. The already shipped code breaks the CanCan pattern by asking a question (e.g. current_ability#can_export_works?)
  2. To incorporate the other pattern is a more costly undertaking (in that we'd need to coordinate effort with Bulkrax and Atla, create a version upgrade which would impact our current work across Adventist, Atla, UTK, and Pals)
  3. This implementation cleaves closest to the existing Bulkrax implementation.

I believe the correct approach is to file an issue surrounding this behavior. Further complicating the entire fact is the index action's filter of Exporters and Importers is something that should be bulkrax configurable (e.g. a custom lambda) because CanCan does not understand filtering rules for query sets.

Also fix incorrect copy/paste error in exporters decorator.
@laritakr laritakr dismissed bkiahstroud’s stale review November 29, 2023 18:51

Fixed items, made bulkrax ticket for cancan work.

@laritakr laritakr merged commit a07bb7c into main Nov 29, 2023
4 of 7 checks passed
@laritakr laritakr deleted the limit-importer-exporter-by-user branch November 29, 2023 19:00
@ShanaLMoore
Copy link
Contributor

rake hyku:roles:create_default_roles_and_groups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants