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.
lib/ex_admin/table.ex
Outdated
| 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