search

cpjk / canary

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

Add :id_field option to "load_resource" #36

Closed juljimm closed 8 years ago

juljimm commented 8 years ago

Hi Chris,

Not sure if Ecto version dependency must be raised because of Repo.get_by/2 mocked in tests.

Thank you!

juljimm commented 8 years ago

Not sure if raising elixir dependency to v1.2.0 could be too disruptive. Maybe this can be implemented in any other way to suppot old elixir releases.

cpjk commented 8 years ago

I don't think bumping Elixir to 1.2 will be disruptive: users can just use the previous version of Canary if they don't want to update for some reason.

I also don't think bumping Ecto should be a big deal. It looks like Repo.get_by was introduced in Ecto 1.0.0. We can change the Ecto dependency to ~>1.1.

Other than that, it looks good.

Would you mind also adding tests for authorize_resource/2 and load_and_authorize_resource/2?

juljimm commented 8 years ago

Done, I think this could be enough.

Also, please check my english in README :)

cpjk commented 8 years ago

Just a few small comments :smile:

juljimm commented 8 years ago

Done. Sorry for my bad english :cry:

cpjk commented 8 years ago

LGTM :shipit:

Thanks for the PR! :smile: