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

Refactor ApiUsersController #2622

Merged
merged 12 commits into from
Jan 4, 2024
Merged

Refactor ApiUsersController #2622

merged 12 commits into from
Jan 4, 2024

Conversation

chrisroos
Copy link
Contributor

@chrisroos chrisroos commented Jan 4, 2024

These changes should make it a bit easier to move the ApiUsersController#manage_permissions functionality to the design system by making the existing behaviour easier to reason about.

@chrisroos chrisroos force-pushed the refactor-api-users-controller branch 2 times, most recently from ecaafe2 to 3ca9b72 Compare January 4, 2024 14:00
Now that it is only used in this template.
These are a hangover from when this logic was in a partial that was
shared between users and api_users templates.
These are a hangover from when this logic was in a partial that was used
for both persisted and un-persisted objects.
This currently duplicates the `applications_and_permissions` method but
I'll remove that duplication separately.
The `applications_and_permissions` method is now only used for web
users, and the `visible_applications` and
`api_user_applications_and_permissions` methods are only used for api
users.
Now that it's only used by this controller.
Now that it's only used by this controller.
We use the ApiUserPolicy to prevent non-superadmin users from accessing
any of the actions in this controller.

The visible_applications method is only used in this controller: in
`index` via `ApiUsersHelper#application_list` and in
`manage_permissions` via `api_user_applications_and_permissions`, so we
can rely on our Pundit policy to ensure the current user is a
superadmin.
The `visible_applications` method already
`includes(:supported_permissions)` so we don't have to repeat that here.
This isn't being used anywhere.
@chrisroos chrisroos force-pushed the refactor-api-users-controller branch from 3ca9b72 to 9d4b8aa Compare January 4, 2024 16:18
@chrisroos chrisroos marked this pull request as ready for review January 4, 2024 16:28
@chrisroos chrisroos changed the title WIP: Refactor ApiUsersController Refactor ApiUsersController Jan 4, 2024
@floehopper floehopper self-assigned this Jan 4, 2024
@floehopper floehopper self-requested a review January 4, 2024 16:40
Copy link
Contributor

@floehopper floehopper left a comment

Choose a reason for hiding this comment

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

This all looks great to me - good work! 🎉

@chrisroos chrisroos merged commit 0fd9216 into main Jan 4, 2024
16 checks passed
@chrisroos chrisroos deleted the refactor-api-users-controller branch January 4, 2024 16:46
@chrisroos
Copy link
Contributor Author

Thanks @floehopper!

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.

2 participants