-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store phonenumbers somewhere #102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
==========================================
+ Coverage 73.08% 73.96% +0.88%
==========================================
Files 32 34 +2
Lines 1211 1260 +49
==========================================
+ Hits 885 932 +47
- Misses 326 328 +2
Continue to review full report at Codecov.
|
I've squashed some things and force pushed. New stuff: checking for ownership in the viewset. |
src/argus/auth/views.py
Outdated
|
||
class PhoneNumberViewSet(ModelViewSet): | ||
permission_classes = [IsAuthenticated, IsOwner] | ||
queryset = PhoneNumber.objects.all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better in our case if we defined this method instead of queryset
:
def get_queryset(self):
return self.request.user.phone_numbers.all()
In that case, we might as well remove the IsOwner
permission, but on the other hand, it could be nice to keep it to be explicit. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to manually test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've force-pushed a fix for this, after testing. I've also rebased on the drf-permissions merge-point.
Part of #97.