cpjk / canary

:hatching_chick: Elixir authorization and resource-loading library for Plug applications.
MIT License
474 stars 51 forks source link

Add handle_not_found on load_resource with the same behaviour as handle_unauthorized #34

Closed simonprev closed 8 years ago

simonprev commented 8 years ago

Thanks for this package, it’s very useful. I wanted this behavior to handle a "resource not found" globally on my app.

This adds the same handler logic for the load_resource function. It adds the assign :not_found with "true" or "false" each time load_resource is called.

The added assign required a modification of every test cases. I don’t know if this is a behavior you want because it might break some apps. But it acts the same way as the unauthorized assign.

cpjk commented 8 years ago

Hey Simon, thanks for the PR!

I like the addition of a not found handler, but I am wondering what your reasoning is behind adding the :not_found key to conn.assigns. It looks like it is just used to pass information to handle_not_found/2. I feel like a new key isn't really needed, since you can already already check for resource presence using something like Map.get(conn.assigns, resource_name(conn, opts)) in [nil, []].

simonprev commented 8 years ago

Yeah you’re right, the key doesn’t add any value. I’ll remove it :smile:

simonprev commented 8 years ago

@cpjk I’ve made the changes you requested, anything else?

cpjk commented 8 years ago

Great, thanks! Looks good. Would you mind also adding tests for when the :not_found_handler is configured, and for when it is both configured and specified as an opt? Sorry, I must have missed it the first time. You can look at https://github.com/cpjk/canary/blob/master/test/plug_test.exs#L843-L875 for how it was done for the :unauthorized_handler.

simonprev commented 8 years ago

Oh of course, I didn’t see those tests either when implementing the feature.

Now everything is tested like unauthorized_handler :smile:

cpjk commented 8 years ago

:+1: Great, looks good to me. Thanks again for the PR!