BF1 - Groups #18
BF1 - Groups #18
Conversation
|
|
||
| @Transactional | ||
| @Modifying(clearAutomatically = true) | ||
| @Query("update Member set groupId = NULL where id = :memberId") |
There was a problem hiding this comment.
This will remove a member from all its groups. I believe that what we want here is to remove the member from a given group (the one from the current year for example).
|
|
||
| @Transactional | ||
| @Modifying(clearAutomatically = true) | ||
| @Query("update Member set groupId = :groupId where id = :memberId") |
There was a problem hiding this comment.
with this, you consider that a given person can only be part of the a single group
|
|
||
| @Transactional | ||
| @Modifying | ||
| @Query("update Member set groupId = null where groupId = :groupId") |
There was a problem hiding this comment.
Instead of an update here, I would consider using a delete: delete from Member where groupId = :groupId
|
|
||
| @Transactional | ||
| @Modifying | ||
| @Query("update Member set groupId = NULL where groupId = :groupId") |
There was a problem hiding this comment.
This is the same as updateMemberGroupDeleted. I'm guessing one of those should remove a single user from the group. Is it correct?
| @@ -40,4 +47,22 @@ public Member byId(@PathVariable long id) { | |||
| public List<Member> byTestId(@PathVariable long id) { | |||
| return memberRepository.findByTestId(id); | |||
| } | |||
|
|
|||
| @PutMapping("/member/{memberId}/group-update/{groupId}") | |||
There was a problem hiding this comment.
I'm not saying the API is 100% consistent. Still, we should always design an API following the simple rules suggested by HTTP.
HTTP suggests:
- a resource is always accessible to the same url
- when interacting with a resource, we must always interact with this URL and use the appropriate HTTP verb. If necessary, we may create a new verb (rarely followed, people usually tend to use a custom header, which is largely accepted)
Here, if we already have /groups/:id/members, we should probably use that. Hence, our URI for the membership of a person in a given group would be /groups/:id/members/:user_id
To add a person to the group, we would use a verb. HTTP verbs are usually used as follows: - POST for creation
- PUT for update
- DELETE for deletion
- OPTIONS to determine the verbs allowed on a given resource
(There is debate in the literacy regarding when to use POST and when to use PUT. My recommendation is the above).
So to create a membership, we shallPOST /groups/:id/member/:user_id. And to remove it, justDELETE /groups/:id/member/:user_id
Feel free to reach out if you want to discuss this.
There was a problem hiding this comment.
As for the return value, an api should prefer sending back the updated resource when doing a PUT, for that's the only real way for the client to be sure what the server has understood.
But maybe this is more of a personal preference, not necessarily a must do of creating an API ;)
I'll maintain the recommendation, but won't enforce it.
|
|
||
| @PutMapping("/member/{memberId}/delete-group") | ||
| @ResponseBody | ||
| public int deleteGroup(@PathVariable long memberId) { |
There was a problem hiding this comment.
deleteGroup, then removeGroup?
There was a problem hiding this comment.
What is the "int" you reply? If it's the number of affected rows, then you should test this number to ensure your result is correct. And if the number is not what you expect, you should consider this a server error (HTTP Error 500).
| @@ -82,6 +82,7 @@ | |||
| response.put("token", new JwtAuthenticationResponse(jwt)); | |||
| response.put("user", (UserPrincipal) authentication.getPrincipal()); // get signed in user | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
not a real problem to have this, but it's symptomatic that you don't review what you are committing. And that is a bah habit.
A commit should be like when you're giving back an exam: you take all the pages you have written down, you go over it quickly to ensure you are not giving back your notes nor your draft, and that everything is readable.
That would avoid uselessly committing files
|
les opérations de "gestion" (ajout / suppression) sont accessibles aux élèves, et pas seulement aux professeurs. L'API devrait théoriquement contrôler les droits d'accès avant d'accepter une demande. Et répondre une erreur 403 Forbidden si ce n'est pas le cas |
Add features :