Closed jmufugi closed 8 years ago
Looking good other than my comments :smile:
Agreed - using Enum.concat is a great idea. Will amend. rgds
Awesome! Just need some tests now.
+1, can you merge this? @cpjk Very nice! @jmufugi
@romariolopez We just need some regression tests to ensure that future changes don't break the changes here, and we should be good to go! :smiley_cat:
@jmufugi ^ :smile:
Noted. Will endeavor to add a couple of tests tomorrow.
rgds
On Sat, Nov 5, 2016 at 9:29 AM, Chris Kelly notifications@github.com wrote:
@jmufugi https://github.com/jmufugi ^ 😄
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cpjk/canary/pull/45#issuecomment-258596354, or mute the thread https://github.com/notifications/unsubscribe-auth/AMASRKYovo5_D1o0Gb8RKcha0dWUoO92ks5q7DBvgaJpZM4JzqX1 .
@cpjk, style is subjective, pls you adjust to yr taste on master after merging. Also prefer consistency, suggest you refactor code to use Plug.Conn.assign (agree is cleaner) on all tests.
thx
On Sun, Nov 6, 2016 at 11:47 PM, Chris Kelly notifications@github.com wrote:
@cpjk commented on this pull request.
In test/plug_test.exs https://github.com/cpjk/canary/pull/45:
+
- test "it authorizes the resource correctly when non_id_actions is a list" do
when opts[:non_id_actions] is set as a list
- opts = [model: Post, non_id_actions: [:other_action]] +
- params = %{"id" => 1}
- conn = conn(
- %Plug.Conn{
- private: %{phoenix_action: :other_action},
- assigns: %{current_user: %User{id: 1}, authorized: true}
- },
- :get,
- "/posts/other-action",
- params
- )
- expected = %{conn | assigns: Map.put(conn.assigns, :authorized, true)}
@jmufugi https://github.com/jmufugi ^
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cpjk/canary/pull/45, or mute the thread https://github.com/notifications/unsubscribe-auth/AMASRMguBx30EX9eLP7xS-NLpkPvnQ2yks5q7krjgaJpZM4JzqX1 .
PR to extend non-id actions ie those that require model name as opposed to struct; list is currently hard-coded to [:index, :new, :create]. Actions are specified using 'non_id' option on authorize_resource plug. e.g, non_id: [:action1, :custom_action2, ...]. Note that this also addresses issue #38 on authorizing singleton resources.