-
Notifications
You must be signed in to change notification settings - Fork 42
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
UIU-3105: hide permissionSets from settings/users navigation pane if roles interface is present #2689
Conversation
…nterface is present
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 works, but in Eureka, we don't have mod-permissions, so the permissions
interface won't exist at all, right? If that's correct, lets implement like we did for fees and fines on line 133, leveraging the filter on SettingsPage to implement this in a less obtrusive/more generic way.
Unfortunately, backend relies on permissions interface for migrations, and currently we might see this situation(picture below). As a result we will have interface "permissions" in eureka for now. Probably, later when backend will not use |
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 hesitate to approve this because getSettingSections
is just so out of place, and so specific to the perms
route. True, it gets the job done, but without any context since there are no comments here and the route in question is not defined here.
I think a better approach may be to handle this directly in sections.js
where we could use the same conditionals to evaluate whether to unshift this route onto the settingsGeneral
list.
I also don't understand how the new tests evaluate the condition we are adding here. All the tests do is check whether the settings page renders, not whether the content it renders changes when the interface list changes.
I'll try to handle it differently. |
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.
Ooooh, this is very nice! What if we rename dependsOnNoneInterface
to unlessInterface
?
|
…roles interface is present (#2689) Refs UIU-3105.
Purpose
UIU-3105 - suppress Settings > Users > Permission sets when
roles
interface is presentApproach
TODOS and Open Questions
Learning
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.