Fixes: https://github.com/thoughtbot/administrate/issues/1861
When referring to a route in the code, we run two checks:
-
valid_action?
istrue
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 byAdministrate::Punditize
in order to integrate Administrate with Pundit. It should returntrue
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 https://github.com/thoughtbot/administrate/issues/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?
- 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.
Open question!
The original methods have a signature the_old_question?(action, resource)
. I have flipped the order of parameters to be the_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 is valid_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.