search

FJNR-inc / dry-rest-permissions

Rules based permissions for the Django Rest Framework
ISC License
75 stars 11 forks source link

Add DRYPermissions._get_permission_targets #15

Open arianesc opened 3 years ago

arianesc commented 3 years ago

update tests and add kwargs obj=None. closes #12

rodrigondec commented 3 years ago

Disclaimer

this PR changes the interface for the has_{action}_object_permission methods!

from:

class SomeModel():
    def has_update_object_permission(self, request):
        pass

to:

class SomeModel():
    def has_update_object_permission(self, request, obj):
        pass

or

class SomeModel():
    def has_update_object_permission(self, request, **kwargs):
        pass

To update for this new version/changes, the users must declare the obj arg or **kwagrs in their models. Otherwise it will happen a TypeError: fun() takes 1 positional argument but 2 were given

rodrigondec commented 3 years ago

@RignonNoel news?

RignonNoel commented 3 years ago

@rodrigondec Hi! Sorry for the delay, we got some emergency with customers and i don't wanted to rush this integration since it's using a non-conventional DRF way to work and that it asked to learn a bit on this architecture to review it.

Did you have the time to test this PR on a project where you work ? Is it possible to take a look on it ?

I will make sure to integrate this PR as soon as possible, just need to run it in local this week-end. In the meantime if you have an example project to check the integration it could be nice.

rodrigondec commented 3 years ago

Did you have the time to test this PR on a project where you work ? Is it possible to take a look on it ?

Yes, we tested on our private project (work related). We will create a public sandbox repo with the implementation to facilitate your testing

rodrigondec commented 3 years ago

@RignonNoel, I made a example for this PR with the minimal code sample possible.

https://github.com/imobanco/dry-rest-permission-ddd-example

If you need something else just ask :wink:

arianesc commented 3 years ago

@RignonNoel, I fixed the modification requests you made.

I think it's ready for another review.

Just say if there's something else that needs to be done.

RignonNoel commented 3 years ago

@arianesc @rodrigondec I wait for a last approval but the PR will be merged this weekend. Thanks for the last change @arianesc and for the testing repository @rodrigondec

I think it would be nice to keep your test repo alive after the merge to help people know more about the way you work with it. We could add a point in the readme for that.

rodrigondec commented 3 years ago

@RignonNoel any news?

rodrigondec commented 2 years ago

@RignonNoel news?