fixed offer permissions #343
Conversation
minghansun1
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #343 +/- ##
==========================================
+ Coverage 88.89% 89.00% +0.10%
==========================================
Files 73 73
Lines 3171 3183 +12
==========================================
+ Hits 2819 2833 +14
+ Misses 352 350 -2 ☔ View full report in Codecov by Sentry. |
| @@ -299,5 +308,6 @@ def destroy(self, request, *args, **kwargs): | |||
| def list(self, request, *args, **kwargs): | |||
| if not Item.objects.filter(pk=int(self.kwargs["item_id"])).exists(): | |||
| raise exceptions.NotFound("No Item matches the given query") | |||
| self.check_object_permissions(request, Item.objects.get(pk=int(self.kwargs["item_id"]))) | |||
| for offer in self.get_queryset(): | |||
| self.check_object_permissions(request, offer) | |||
There was a problem hiding this comment.
i dont think you need to check manually... won't setting the permission class already check?
| @@ -299,5 +308,6 @@ def destroy(self, request, *args, **kwargs): | |||
| def list(self, request, *args, **kwargs): | |||
| if not Item.objects.filter(pk=int(self.kwargs["item_id"])).exists(): | |||
| raise exceptions.NotFound("No Item matches the given query") | |||
| self.check_object_permissions(request, Item.objects.get(pk=int(self.kwargs["item_id"]))) | |||
| for offer in self.get_queryset(): | |||
There was a problem hiding this comment.
On another note, do you have to manually raise NotFound exception? If you do, shouldn't you be raising in the queryset?
| @@ -299,5 +308,6 @@ def destroy(self, request, *args, **kwargs): | |||
| def list(self, request, *args, **kwargs): | |||
| if not Item.objects.filter(pk=int(self.kwargs["item_id"])).exists(): | |||
| raise exceptions.NotFound("No Item matches the given query") | |||
| self.check_object_permissions(request, Item.objects.get(pk=int(self.kwargs["item_id"]))) | |||
| for offer in self.get_queryset(): | |||
There was a problem hiding this comment.
I feel like we don't need to manually override list if we do the queryset and object permissions right
| @@ -299,5 +308,6 @@ def destroy(self, request, *args, **kwargs): | |||
| def list(self, request, *args, **kwargs): | |||
There was a problem hiding this comment.
Looking at the destroy code on line 303, it seems like you are doing some manual checks to see if the object can be destroyed. Again, I think if we get the permissions right (which I think they are) and the queryset right (which I think you have correct as well), Django's built in destroy should be able to handle all of it for us?
No description provided.