Feature/hidden problems#106
Conversation
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
This looks well-designed and well-implemented to me. My one reservation is that when someone tries to access a problem they are not supposed to see, you return a different problem instead. That should be a 404 or 403, as far as I'm concerned.
I do think the UI is well designed and I like the thorough testing. 👍
| response = client.get(f"/api/problem/{hidden_problem.id}/") | ||
| assert response.status_code == status.HTTP_200_OK | ||
| assert response.data['problem']['id'] == sample_problem.id, "Unauthenticated users requesting a hidden problem should receive the first non-hidden problem instead." |
There was a problem hiding this comment.
I find this strange. Asking for a problem with a particular ID, which you're not allowed to see, and then getting another problem with a different ID instead. That will look like a bug to legitimate non-masters. Why not just return a 404?
There was a problem hiding this comment.
The reasoning behind this is that this view is also used by users who change their filters in such a way that the problem they are currently viewing is excluded. It feels a bit heavy-handed to hit them with a 404, but I'm happy to be convinced otherwise.
Maybe we can return a 404 if there are no filters applied (since the user will then be looking for a specific problem, e.g. by manipulating the URL) anyway, and return the first available problem (as it is) otherwise?
A separate suggestion would be to show a small toast/pop-up message when a different problem is returned, so it is more obvious to the user that they have been 'redirected' to the first available problem, not the problem they requested.
There was a problem hiding this comment.
The justification you're giving sounds like a list endpoint, where the query might return zero or more problems. In that case, if the list was filtered by a particular ID, I would expect it to return an empty list if the user cannot see the problem with that ID. Only if there was no ID-based filter criterion, would I expect that the user might get other results than the one they had in mind initially.
The test code I'm reading, on the other hand, seems to suggest that this is an endpoint that is supposed to return exactly one problem. If that is the case, the problem either exists or it doesn't (from the point of view of the user). In this case, where the user is not allowed to see the problem in question, there are two ways in which you can present this: "it exists but you're not allowed to see it" (403) or (I'm going to pretend) "it doesn't exist" (404).
Letting a user select one particular ID, like in this test, and then showing a problem with a different ID is never correct, as far as I'm concerned.
Perhaps the frontend is using this endpoint in a way that I'm not aware of? We can discuss this tomorrow or the day after if necessary.
There was a problem hiding this comment.
As far as I can tell, you only added two lines, but for some reason, GitHub thinks that you replaced the entire file. Can you tell what is causing this?
There was a problem hiding this comment.
My apologies, this is my editor "illegally" changing the line endings from LF to CRLF. I'll push a commit that fixes this.
2 participants
Closes #56
This PR introduces a new field 'hidden' on Problems, and allows users belonging to the Master Annotator user group (and superusers) to hide certain problems, so only they can see them. 'Hidden' status is clearly indicated with an info banner at the top of the screen that also contains some explanation on what this status entails.
A filter on the left side of the screen (only visible to Master Annotators and superusers) allows you to filter problems according to their hidden status.