search

venuu / jsonapi-authorization

Authorization for JSONAPI::Resource
http://www.rubydoc.info/github/venuu/jsonapi-authorization
MIT License
132 stars 59 forks source link

Support for Namespacing #110

Closed g13ydson closed 5 years ago

g13ydson commented 5 years ago

Adds the ability to inform the namespaces of policies based on https://github.com/varvet/pundit#policy-namespacing.

In the context it is possible to inform the namespace. This solves problems related to applications that use APIs versioning.

def context
   {user: current_user, namespace: [:api, :v1] }
end
g13ydson commented 5 years ago

Some tests are failing because of the versions with pundit < 2.0

valscion commented 5 years ago

Hi! Thanks for the PR.

There's quite a lot of diff to go through — more than 2 thousand lines. Do you think there's any way to get that to an amount that is reasonable to review?

g13ydson commented 5 years ago

Hi! Most of the added lines are from the RSpec tests. I have had to repeat in some cases to cover all test cases with the namespace.

valscion commented 5 years ago

Is there a way to construct the existing specs in a way that wouldn't require repeating the code so much? I'm afraid that when we do changes in the future, we will forget to update one of the spec cases.

Also, we will need to keep supporting Pundit v1 somehow. If there's a way to skip the new tests when running without Pundit >= v2 then that's the way we should go

valscion commented 5 years ago

I'm fine with the duplication in setup code, but the duplication of *_spec.rb files is something we will need to avoid.

Maybe there's some sort of solution to that?

g13ydson commented 5 years ago

Hi @valscion,I refactored much of the code to decrease duplication. Although there is a lot of modification, duplication of code has been reduced. For this, I had to create some shared_examples. Thanks for the feedback.

valscion commented 5 years ago

Nice, this is starting to look reviewable now that I can locally checkout this PR and run:

git diff master --find-copies --color-moved -w

to see the actual changes.

I'll release v1.0.0 of this gem now, and then we can target this feature to v1.1.0. I'd like to avoid shipping this change as part of the actual v1.0.0 release.

I'll still need to test drive this a bit later and maybe play around with the specs, but this is looking pretty nice. I see you've allowed edits from maintainers for these files so I'll update this PR if I see some changes I'd like to be done.

This feature support has been asked many times and I'm very happy that you've contributed this :relaxed: Thank you!

valscion commented 5 years ago

Feel free to point the applications which need this PR to your branch:

# Gemfile
gem 'jsonapi-authorization', github: 'g13ydson/jsonapi-authorization', branch: 'feature/namespace'

If that's not sufficient, maybe you can release a fork of this gem with another name until I get to reviewing this further?

This might take a long time, so it'd be a shame if this were to be a blocker for your development. I have limited time to look into this.

valscion commented 5 years ago

I'm sorry, but the big amount of spec changes needed to support this feature is too big for me to review this feature and know that it's worth the effort.

I'm not sure what's a good solution to overcome this. If this feature requires a big PR like this, it will not be possible to do it. This work will need to be done in smaller pieces, yet I don't know what those smaller pieces could look like.