Unify action checks #1941
Merged
Unify action checks #1941
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 25, 2021
c4lliope
approved these changes
Apr 11, 2021
There was a problem hiding this comment.
This change looks good; for your checklist, maybe you could rename:
valid_action?->possible_action?show_action?->permissible_action?
...though I'm unsure because I'm barely involved here.
I'm sure you'll choose sensible names.
Much obliged for your changes, @pablobm !
pablobm
force-pushed
the
unify-action-checks
branch
from
17b760b to
da25d11
Compare
pablobm
force-pushed
the
unify-action-checks
branch
from
da25d11 to
5c7ab86
Compare
pablobm
force-pushed
the
unify-action-checks
branch
from
5c7ab86 to
1cbbc57
Compare
pablobm
force-pushed
the
unify-action-checks
branch
6 times, most recently
from
9bda868 to
8756286
Compare
pablobm
force-pushed
the
unify-action-checks
branch
3 times, most recently
from
43e7d85 to
4159903
Compare
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
pablobm
force-pushed
the
unify-action-checks
branch
6 times, most recently
from
4355d12 to
00d1247
Compare
pablobm
force-pushed
the
unify-action-checks
branch
from
00d1247 to
4831e78
Compare
pablobm
force-pushed
the
unify-action-checks
branch
3 times, most recently
from
a97b0a1 to
ae297b3
Compare
|
@c4lliope @nickcharlton - After more than a year, this PR is finally out of draft! |
When referring to a route in the code, we run two checks: * `valid_action?` is `true` if the route is defined, `false` otherwise. * `show_action?` is expected to be overridden by developers in order to implement authorization. For example, it's implemented by `Administrate::Punditize` in order to integrate Administrate with Pundit. It should return `true` if the current user can access a given route, `false` otherwise. These two check should (almost) always happen together. For this reason, our code is peppered with `if` statements where both are checked... and a few others where we forget one or the other. These checks should be unified into a single method call, in order to avoid issues like the one described at thoughtbot#1861. This introduces a new method, called `accessible_action?`. The original methods should still exist, as they do have their uses individually. The new method will delegate to the existing ones. We also rename the two existing methods to something that will make their intent clear: * `valid_action?` becomes `existing_action?` * `show_action?` becomes `authorized_action?` In order to provide a clear upgrade path, the old names still exist and work, but they show a deprecation warning when used. They can be removed properly at a later version of Administrate.
nickcharlton
force-pushed
the
unify-action-checks
branch
from
ae297b3 to
c3b7100
Compare
Merged
jazairi
added a commit
to MITLibraries/thing
that referenced
this pull request
Aug 22, 2022
This commit includes an update to the administrate gem that requires a small change to our custom `show` template. (See thoughtbot/administrate#1941 for more info). Additionally, I noticed that administrate added a `delete` link to the show template earlier this year: thoughtbot/administrate@e510efe. We haven't yet added this to our custom template, and I'm assuming that for now it's still not needed.
9 tasks
jazairi
added a commit
to MITLibraries/thing
that referenced
this pull request
Aug 26, 2022
Why these changes are being introduced: The most recent version of administrate refactors authorization in a way that has downstream effects. See [#1941](thoughtbot/administrate#1941) for more info. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/ENGX-192 How this addresses that need: This replaces our override of the `show_action?` method with `authorized_action?` in the administrate application controller, and it replaces our combined check of `valid_action?` and `show_action?` with `accessible_action?` in the administrate views. This also updates the ability so thesis processors can continue to access the submitter dashboard. This is not currently needed as we do not have any users with the thesis_processor role, but it seemed wise to address it sooner than later. Side effects of this change: * Thesis processors can now see links to controller actions in the submitter admin dashboard, as well as a link to the admin dashboard itself from the main site nav. * The admin submitter dashboard test will move to the admin/ subdirectory with the other individual dashboard tests. This change will come in a subsequent commit to make the git history easier to read.
9 tasks
francois-ferrandis
added a commit
to betagouv/rdv-service-public
that referenced
this pull request
Nov 9, 2022
4 tasks
francois-ferrandis
added a commit
to betagouv/rdv-service-public
that referenced
this pull request
Nov 10, 2022
francois-ferrandis
added a commit
to betagouv/rdv-service-public
that referenced
this pull request
Nov 14, 2022
thoughtbot/administrate#1941 (cherry picked from commit 5d9eb1e)
This was referenced Apr 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #1861
When referring to a route in the code, we run two checks:
valid_action?istrueif the route is defined,falseotherwise.show_action?is expected to be overridden by developers in order to implement authorization. For example, it's implemented byAdministrate::Punditizein order to integrate Administrate with Pundit. It should returntrueif the current user can access a given route,falseotherwise.These two check should (almost) always happen together. For this reason, our code is peppered with
ifstatements where both are checked... and a few others where we forget one or the other.These checks should be unified into a single method call, in order to avoid issues like the one described at #1861. Here I propose such a method, which I call
accessible_action?.The original methods should still exist, as they do have their uses individually. The new method will delegate to the existing ones.
I have also renamed the two existing methods to something that I hope will make their intent clear:
valid_action?becomesexisting_action?show_action?becomesauthorized_action?Open question!
The original methods have a signature
the_old_question?(action, resource). I have flipped the order of parameters to bethe_new_question?(resource, action), as I find that more natural. But if people think the original order is better (or have some other proposal), I'm happy to oblige.Also! The old
valid_action?has a default parameter value (the full signature isvalid_action?(name, resource = resource_class)). The new version doesn't have it, as I can't see that it has a good use case. Incidentally, this makes sense with the parameter order flip just described.