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

MMT-3907: Fixed Attempting to Update a Collection Permission ACL With Collection From a Different Provider Silently Ignores the User Request #1303

Merged
merged 22 commits into from
Oct 2, 2024

Conversation

cgokey
Copy link
Collaborator

@cgokey cgokey commented Sep 25, 2024

Overview

What is the feature?

As part of the process of testing saving ACLs with more than 20 selected collections (MMT-3895), there are some cases where trying to add collections to the selected collection list does not work properly.
In these cases, the collection gets added to the selected collection list and even when you click submit, you get a successful response back stating the ACL was saved properly. But, when you look at the ACL permissions page, the added collections are not there.

The issue is the user is allowed to add any collection to the selected collection list, but the only ones that will work is those collections in the same provider as the ACL.

What is the Solution?

If the user is trying to update the ACL, we know the provider, so the solution is to just simply filter the available provider list by the ACL's provider so they can't choose an collection outside of their provider.

If the user is trying to create a new ACL, we do not know the provider until submission, so the solution is to run a check to verify that the selected collections match the selected provider associated with the ACL.

What areas of the application does this impact?

Collection Permissions

Testing

Try editing an existing ACL permission, the available collections list should be filtered by the ACL's provider id.
Try creating a new ACL permission, the available collection list should show all collections (not filtered by any provider id). But when you submit, the UI will ask you for the provider and then a check will occur to make sure all selected collections match that provider id. If they do not, it will produce an error.

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.66%. Comparing base (86b5bf0) to head (fb1b6b4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1303   +/-   ##
=======================================
  Coverage   97.66%   97.66%           
=======================================
  Files         362      362           
  Lines        5601     5608    +7     
  Branches     1166     1179   +13     
=======================================
+ Hits         5470     5477    +7     
  Misses        130      130           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@abbottry abbottry left a comment

Choose a reason for hiding this comment

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

I'm confused by the solution here, I'm seeing some ui schema changes that I'm unsure the purpose of, but rather than preventing the user from doing something, we're just throwing an error expecting that they clean it up. We have a few pages in the app that use a provider dropdown, why not just use the dropdown to scope the collections that show up and then a vast majority of this PR can be undone.

static/src/js/components/PermissionForm/PermissionForm.jsx Outdated Show resolved Hide resolved
static/src/js/components/PermissionForm/PermissionForm.jsx Outdated Show resolved Hide resolved
static/src/js/components/Layout/Layout.jsx Show resolved Hide resolved
@cgokey
Copy link
Collaborator Author

cgokey commented Sep 27, 2024

I'm confused by the solution here, I'm seeing some ui schema changes that I'm unsure the purpose of, but rather than preventing the user from doing something, we're just throwing an error expecting that they clean it up. We have a few pages in the app that use a provider dropdown, why not just use the dropdown to scope the collections that show up and then a vast majority of this PR can be undone.

Done, now basically uses AppContext for setting and getting provider id.

@cgokey cgokey requested a review from abbottry October 1, 2024 15:47
…new ACL (worked on an update just not new)
@cgokey cgokey merged commit 375a3b2 into main Oct 2, 2024
5 of 7 checks passed
@cgokey cgokey deleted the MMT-3907 branch October 2, 2024 13:59
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