Custom actions authorization#337
Conversation
smpallen99
left a comment
smpallen99
left a comment
There was a problem hiding this comment.
Overall, a great first pass. Thanks!! Couple things...
The authorization protocol requires the conn object, so that part needs to be reworked. Also, support should also be added for the ActiveAdmin theme.
| do: th(".th-#{parameterize field_name} #{humanize field_name}") | ||
| def build_th(field_name, opts, %{fields: fields} = table_opts) do | ||
| if String.to_atom(field_name) in fields and opts in [%{}, %{link: true}] do | ||
| if opts[:link] == true do |
There was a problem hiding this comment.
Why do you think its safe to replace the original implementation? I think this may have regression issues, but I'm not sure without regressing it. Why the change?
There was a problem hiding this comment.
I added a couple of changes to this PR which are not related to the authorization logic. This part is one of them. Will do some cleanups
| @tag as_resource: %TestExAdmin.ExAdmin.Simple{} | ||
| test "action_button", %{defn: defn, conn: conn} do | ||
| result = ExAdmin.action_button(conn, defn, "Simple", :show, :edit, defn.actions, "17") | ||
| result = ExAdmin.ResourceTitleActions.action_button(conn, defn, "Simple", :show, :edit, defn.actions, "17") |
There was a problem hiding this comment.
Aliasing ExAdmin.ResourceTitleActions would reduce a little noise here.
| end | ||
|
|
||
| @doc false | ||
| def action_button(conn, defn, name, _page, action, actions, id \\ nil) do |
There was a problem hiding this comment.
I know this is the old code, just moved. But I'd appreciate if you could remove the nesting. Something like this (not tested):
def action_button(conn, defn, name, _page, action, actions, id \\ nil) do
with true <- action in actions,
true <- Utils.authorized_action?(conn, action, defn) do
action_name = defn.action_labels[action] || Utils.humanize(action)
[action_link(conn, "#{action_name} #{name}", action, id)]
else
_ -> []
end
end
sineed
force-pushed
the
custom-action-authorization
branch
from
810f5df to
50afd90
Compare
2 participants
Related to #334
Things in which I'm not confident enough:
%Plug.Conn{}struct. As a compromise I think about putting:ex_admin_metadatakey insideconn.assignsmap.ExAdmin. I started to go further but realised that this is not as simple as I thought. I don't think that 7 arguments is good for functions but currently I have bad knowledge ofex_admindomain.I understand that you may not accept this PR without proper module design