-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add chart with average basket price #450
Add chart with average basket price #450
Conversation
tapir/statistics/views.py
Outdated
tapir_user = get_object_or_404(TapirUser, pk=(self.kwargs["pk"])) | ||
logged_user = self.request.user | ||
if tapir_user.pk != logged_user.pk and not logged_user.has_perm( | ||
PERMISSION_COOP_MANAGE |
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'm actually not sure about this line - maybe it should be another permission?
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 PERMISSION_ACCOUNTS_VIEW is more appropriate.
Also, you can use the PermissionRequiredMixin to check for permissions, you can then reuse the same logic as the the TapirUserDetailView :
Lines 49 to 52 in 5ef7e14
def get_permission_required(self): | |
if self.request.user.pk == self.kwargs["pk"]: | |
return [] | |
return [PERMISSION_ACCOUNTS_VIEW] |
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.
Thanks a lot for doing this 🙇
There was a confusion about what number we want to show, see the comments.
Otherwise, appart from a few small improvements, the structure is good 💯
tapir/statistics/views.py
Outdated
tapir_user = get_object_or_404(TapirUser, pk=(self.kwargs["pk"])) | ||
logged_user = self.request.user | ||
if tapir_user.pk != logged_user.pk and not logged_user.has_perm( | ||
PERMISSION_COOP_MANAGE |
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 PERMISSION_ACCOUNTS_VIEW is more appropriate.
Also, you can use the PermissionRequiredMixin to check for permissions, you can then reuse the same logic as the the TapirUserDetailView :
Lines 49 to 52 in 5ef7e14
def get_permission_required(self): | |
if self.request.user.pk == self.kwargs["pk"]: | |
return [] | |
return [PERMISSION_ACCOUNTS_VIEW] |
About adding test data, usually we do it through the models |
Thanks again for contributing, it's great 🎉 |
I applied all the suggestions. Thanks for the review :) |
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.
Great!
This adds a new graph:
Testing it requires some data:
And then you can go to: http://localhost:8000/accounts/user/1066/