-
Notifications
You must be signed in to change notification settings - Fork 2
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/16 save extra report notes #23
Conversation
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.
Appreciate your contribution. Thank you :)
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.
Found one issue and left a note
@@ -45,6 +45,7 @@ class Meta: | |||
"date", | |||
"table_name", | |||
"table_extra_notes", | |||
"report_notes", |
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.
This does not fully cover requirement on who can see the report.
Anyone with access to this endpoint can pull the data and read the notes. It should be returned as a null or empty string, if the requesting user is not a DM. So we need a backend to check access to the field. This can be easily covered with MethodField
.
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 for the info. I added check in the serializer.
@@ -64,6 +65,11 @@ def get_time_start(self, game): | |||
def get_time_end(self, game): | |||
return game.time_end.strftime("%H:%M") if game.time_end else "" | |||
|
|||
def get_report_notes(self, game): | |||
if self.context["request"].user.profile != game.dm: |
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.
Actually it should be visible to ANY DM in the system, so the role of the profile should be checked. AFAIR there's a method/property for that.
Added saving report extra notes via api ( #16 )
Not sure if this is the refactoring you had in mind. Let me know if I should add some tests.
To be merged after #22