Feature/user models#61
Conversation
XanderVertegaal
changed the base branch from
develop
to
feature/user-problems
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
#12 has a table of permissions that each role should or should not have. Why not just define those as custom permissions (where needed) and then implement the roles as regular user groups? That way you don't have to hardcode the roles and Django will assist more in enforcing the permissions.
|
Update! At your suggestion, the role field has been replaced by Django user groups and permissions. I have added a data migration that creates them and assigns the required permissions to them. For now there are only two groups: 'annotators' and 'master annotators'. Technically there is a third group (now called 'visitors' in the frontend) for users who are logged in but have not been assigned to either user group. However, these do not (yet?) have any special permissions that non-authenticated users lack, so I did not create a separate group for them now. |
|
Would you like me to take another look? |
|
If you have time, yes please! |
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
I like the comprehensive tests!
I have some comments, but it's mostly questions or nitpicks.
| assert response.status_code == status.HTTP_200_OK | ||
|
|
||
|
|
||
| class TestUserRoleProperties: |
There was a problem hiding this comment.
Terrible nitpick, I know, but Python calls them "attributes".
There was a problem hiding this comment.
On the Python level they may be attributes, but in Django they are marked as 'properties' (cf. the @property decorator used in the definition of these fields) so if you do not mind I will be sticking to TestUserRoleProperties in this case.
In general, by the way, I appreciate nitpicks like this one; please keep them coming!
| private newProblem$(baseParam: number | null): Observable<ProblemResponse> { | ||
| const sharedProblemResponse: Omit<ProblemResponse, "problem"> = { | ||
| index: null, | ||
| first: null, | ||
| last: null, | ||
| next: null, | ||
| previous: null, | ||
| total: 0, | ||
| error: null, | ||
| }; | ||
|
|
||
| if (baseParam !== null) { | ||
| return this.existingProblem$(baseParam.toString()).pipe(map(response => { | ||
| const problem = response?.problem; | ||
| return { | ||
| ...sharedProblemResponse, | ||
| problem: { | ||
| id: null, | ||
| base: baseParam, | ||
| hypothesis: problem?.hypothesis ?? "", | ||
| dataset: Dataset.USER, | ||
| premises: problem?.premises ?? [], | ||
| entailmentLabel: EntailmentLabel.UNKNOWN, | ||
| extraData: null, | ||
| // KB items are not shared across problems. | ||
| kbItems: problem?.kbItems.map(kbItem => ({ | ||
| ...kbItem, | ||
| id: null, | ||
| })) ?? [] | ||
| } | ||
| }; | ||
| })); | ||
| } | ||
| return of<ProblemResponse>({ | ||
| ...sharedProblemResponse, | ||
| problem: { | ||
| id: null, | ||
| base: baseParam, |
There was a problem hiding this comment.
I was going to comment that you should refactor this, but I vaguely recall commenting on very similar code before. Did you already change this on another branch?
There was a problem hiding this comment.
If I recall correctly, it was in a comment that said something along the lines of: "I don't recommend you change this, but if we used 'underscore', you could do this: ...". Did you change your mind about refactoring?
There was a problem hiding this comment.
hmm maybe refactor without underscore? Just to get rid of the deep nesting?
There was a problem hiding this comment.
I noticed that every time I write that you can outfactor something "at module scope", you implement this in a new utility module. As far as I'm concerned, you could also just keep it in the module where the logic originated. Maybe you're already aware of this and you just prefer a separate module, but I thought I'd mention it.
2 participants
Addresses part of #12
Adds a
rolefield to the User model. I opted to add a fieldcanEditOrAddProblemto the serializer so we don't have to implement a method that checks whether a user with role X can do Y in two places.