CBV mixin for checking permissions automatically based on view type

[bob1de]

As mentioned in #99 already, here is a possible implementation of the CBV mixin. It's rather short and simply extends PermissionRequiredMixin.

While I already use a similar thing in production, I rewrote this one completely to be more generic, so don't expect it to be 100% complete. Docs are still missing as well.

[bob1de]

Ah, and it depends on #99 being merged first.

[bob1de]

I've got an equivalent mixin for viewsets in django-rest-framework with support for custom actions as well, BTW.

[highpost]

Even better. That's exactly my use case. Thanks so much for this work.

[bob1de]

The view mixin has already been tested and used in action, but not the rest part. Would be great if you could share your experience with it.

[highpost]

I'll do that since I'm working on a DRF view using Rules for permission handling. It may take a few days to get to the point of testing. But before that I'll look at your code. Thanks again.

[bob1de]

@highpost Great, thanks.

If you need a branch to test against which has both the model and viewset additions, you can use this line in requirements.txt for testing:

git+git://github.com/efficiosoft/django-rules@mix

I'll keep the branch there for the next couple of weeks.

[dfunckt]

Hey @efficiosoft, this is all much appreciated. From a quick look, the code looks solid, will probably only have to comment on a couple of typos when I sit down to properly review.

My only concern is that I will have no idea if/how this works unless I invest significant effort into (re-)learning DRF -- I haven't used it for years -- and about how I'm going to maintain this code going forward. Any ideas for adding tests for this code? I'd probably feel even better if testing it didn't have to bring in DRF as a dependency. Can the semantics of DRF be mocked into a small class perhaps so we can focus on the code's expected behaviour?

Edit: I'm obviously talking about the ViewSet mixin, the CBV looks great as it is, though that should part probably have some tests as well.

[bob1de]

Hi @dfunckt
Hm, I could of course construct a mock object trying to simulate the DRF viewset, but that would add another component which we'd need to make sure behaves like the real DRF, so I think I'm against this approach. DRF is relatively small and has no dependencies apart from Django itself, hence I'm for simply adding it to the testing dependencies.

I'll see if I can get some tests done next week, but have no idea how to cleanly simulate the full view invocation with a request yet. If I get this done for the CBV mixin, it should be doable for the DRF mixin as well.

What do you think?

[dfunckt]

That sounds reasonable @efficiosoft.

I suggested mocking DRF to avoid introducing the dependency for one, but also not having to deal with mocking a full request, thinking you could construct something that just meets the mixin’s requirements. Can the django test client be used to test both CBV and DRF mixins? Doesn’t DRF have something equivalent?

[bob1de]

DRF viewsets are just a bunch of regular Django views for which URLs are generated automatically by a router. So I expect to be able to use whatever Django provides for testing views for DRF, too.

[bob1de]

I just checked and yes, there is a convenient-to-use request factory for generating test requests for DRF. I'm going to write the tests within the next days.

[bob1de]

Hi @dfunckt

Tests for the CBV mixin are ready. Are you ok with them? If so, I'd create similar ones for the DRF mixin.

[bob1de]

The tests require #99 being merged to pass. I added tests there as well.

[bob1de]

Ok, just did it for the DRF mixin as well. The tests might seem a bit clumsy because I had to put guards in place to not run them under Python 2 everywhere, but this will go away :)

[bob1de]

@highpost Please use the auto-view-mixin branch for testing, it now also contains the changes from #99 and I've dropped the mix branch.

[bob1de]

@dfunckt Docs and tests complete, commits squashed

[dfunckt]

That’s amazing @efficiosoft ! Left a few comments, nothing significant.

[bob1de]

Hi @dfunckt

Thanks a lot for the thorough review!

I applied some of your suggestions and commented on others which I think should not be changed.

Can't await seeing this used in production :)

[bob1de]

Ok @dfunckt

Then all review comments are resolved now and we can finally merge :)

[bob1de]

Thanks @dfunckt for merging. Have you an idea when this will land in a release?

[bob1de]

Oh, already there, great! ;)