-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/#554 filter destruction lists #572
base: main
Are you sure you want to change the base?
Conversation
50661f6
to
eec7128
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #572 +/- ##
==========================================
+ Coverage 84.18% 84.30% +0.12%
==========================================
Files 208 212 +4
Lines 5639 5716 +77
Branches 570 576 +6
==========================================
+ Hits 4747 4819 +72
- Misses 892 897 +5 ☔ View full report in Codecov by Sentry. |
f7e024e
to
e54e2b7
Compare
class UsersView(ListAPIView): | ||
serializer_class = UserSerializer | ||
|
||
def get_queryset(self) -> QuerySet[User]: | ||
return User.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.
Is there a reason for not doing:
class UsersView(ListAPIView): | |
serializer_class = UserSerializer | |
def get_queryset(self) -> QuerySet[User]: | |
return User.objects.all() | |
class UsersView(ListAPIView): | |
serializer_class = UserSerializer | |
queryset = User.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.
- For endpoints using the manager method that filter on permissions, this may cause issues when initializing as the queryset is initialized straight away.
- For the this particular case its uses the get_queryset method for consistency.
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.
These endpoints are getting a bit out of hand, maybe we should refactor to use filters?
So then we would have GET /api/v1/users?role=reviewer
?
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.
return self._users_with_permission(permission) | ||
|
||
def _users_with_permission(self, permission: Permission) -> "UserQuerySet": | ||
return self.filter( # noqa |
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.
why the added noqa?
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.
My IDE resolves the type to be QuerySet but AFAIK if executed in this context it will return a UserQuerySet.
e54e2b7
to
06c06e7
Compare
… name, status, author, reviewer and assignee
…n name, status, author, reviewer and assignee
06c06e7
to
103afe9
Compare
closes #554