-
Notifications
You must be signed in to change notification settings - Fork 36
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
base: main
Are you sure you want to change the base?
Conversation
… Collection From a Different Provider Silently Ignores the User Request
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
=======================================
Coverage 97.65% 97.66%
=======================================
Files 362 362
Lines 5584 5601 +17
Branches 1172 1174 +2
=======================================
+ Hits 5453 5470 +17
Misses 130 130
Partials 1 1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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.
if (providerIdCheckFails(providerId, conceptIds)) { | ||
errorLogger('Error multiple providers in collections', 'PermissionForm: providerIdCheck') | ||
addNotification({ | ||
message: 'Error multiple providers in collections', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message isn't actionable, if we're going to display a message to the user, they need to be able to take an action to make the error go away. This message doesn't tell them what they need to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions? This is the use case for creating a new ACL, where we don't have the provider id, until submission. The old code was should just fail silently and showing a passing message even thought there were collections not in the same provider as the ACL. We can make the message longer, if you have a better suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed your comment, I'll add the provider id dropdown directly into the form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, adding the provider id combo box doesn't really prevent this, as they potentially still select multiple collections with multiple provider ids (i.e., by selecting a few collections under 1 provider, then choosing a few more under another provider). So I'm going to keep the check, I don't think it hurts, but if you have some suggestions as to how to word the error, that would be great.
useEffect(() => { | ||
if (providerId) { | ||
const newUiSchema = cloneDeep(uiSchema) | ||
newUiSchema.collectionSelection.selectedCollections['ui:providerId'] = providerId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of modifying the UI schema, I'm not clear on the intentions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to pass the providerId to the collection selector field, it needs to go through the uischema. This is so we can filter that left hand side (available collections) by the providerId.
@@ -236,7 +236,7 @@ const Layout = ({ className, displayNav }) => { | |||
naked | |||
Icon={FaUserAlt} | |||
> | |||
{`${user.name} `} | |||
{`${user?.name} `} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we seeing errors related to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URS had some issues yesterday and it would not return the user name (MMT crashed in sit, uat, and prod) because of this.
…when providerId changes.
You want to add the provider id dropdown directly into the form? It really only necessary for the new ACL use case. We know the provider id already for the update use case. It is just the "new ACL" that we don't know it until submission. Most of the PR is actually just filtering the available collections by the provider id, the only code that will disappear is the provider id check for the new use case, but I agree, I think it makes sense to just add the combo box directly into the form rather than this check. |
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