Mrc-5324 Update return values of Create/update/delete + add role/user-update endpoint #68
Conversation
…pendencies" This reverts commit 344d7e5.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 93.98% 94.29% +0.30%
==========================================
Files 67 71 +4
Lines 532 578 +46
Branches 131 146 +15
==========================================
+ Hits 500 545 +45
- Misses 30 31 +1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
absternator
changed the title
| @@ -45,6 +48,17 @@ class RoleController(private val roleService: RoleService) | |||
| return ResponseEntity.noContent().build() | |||
There was a problem hiding this comment.
Since we're now returning the updated role on PUT of users, for consistency should do the same when PUT permissions. Similarly for any other PUTs.
| { | ||
| @PostMapping("/basic") | ||
| fun createBasicUser( | ||
| @RequestBody @Validated createBasicUser: CreateBasicUser | ||
| ): ResponseEntity<Map<String, String?>> | ||
| ): ResponseEntity<UserDto?> |
There was a problem hiding this comment.
Response type here shouldn't need to be nullable I don't think? It isn't for create role.
| @@ -34,7 +37,7 @@ class RoleController(private val roleService: RoleService) | |||
| return ResponseEntity.noContent().build() | |||
| } | |||
|
|
|||
| @PutMapping("/update-permissions/{roleName}") | |||
| @PutMapping("/{roleName}/permissions") | |||
There was a problem hiding this comment.
The Delete function above should have return type ResponseEntity<Unit> too - similarly any others which now return noContent
| fun updateUsersToRole( | ||
| @RequestBody @Validated usersToUpdate: UpdateRoleUsers, | ||
| @PathVariable roleName: String | ||
| ): ResponseEntity<RoleDto?> |
There was a problem hiding this comment.
Again, don't think this should need to be nullable?
| val user = userService.getByUsername(username) | ||
| ?: throw PackitException("userNotFound", HttpStatus.NOT_FOUND) | ||
|
|
||
| val rolesToUpdate = getRolesForUpdate(updateUserRoles.roleNamesToAdd + updateUserRoles.roleNamesToRemove) |
There was a problem hiding this comment.
It feels a bit odd to concatenate the role names like this to fetch the roles, and then have to filter the returned role list for the add and remove. Feels like a bit of a false economy, when you could just make two calls to getRolesForUpdate and keep them separate all along.
There was a problem hiding this comment.
the main reason for this is to save a db hit...if I called both separate that would hit db twice
| user.roles.add(role) | ||
| role.users.add(user) |
There was a problem hiding this comment.
Do you need to do both of these? Shouldn't just one add the row to the link table? It's the user that you're saving in the calling method, so should just need to update the user entity?
There was a problem hiding this comment.
you are correct updating the user entity is enough to persist data.... the updating of role is mainly so we can return the role back with correct users
| return userService.saveUser(user) | ||
| } | ||
|
|
||
| override fun updateRoleUsers(roleName: String, usersToUpdate: UpdateRoleUsers): Role |
There was a problem hiding this comment.
We might also want to consider adding a check that a user isn't attempting to remove themselves from the ADMIN role..
There was a problem hiding this comment.
i feel like if they want to they should be able to 😄 ... eg. you give some ADMIN rights but want to retract that later
There was a problem hiding this comment.
In that case it wouldn't be them removing their own role...probably! Or maybe should make sure there's always at least one admin? Well, can add that later if we need to.
Mrc 5359-Update Password
absternator
changed the title
…b.com/mrc-ide/packit into mrc-5324-update-endpoints-return-data
As per previious PR comments, we are no longer returning the
x has been created. for creation and update we are returning to. For deletion returning nocontentAlso I have added another endpoint role/update-users... this is the opposite of the user/update-roles endpoint... found would be good to have both for the FE when doing mockups...These are both extracted into
UserRoleService... this endpoint has added in this PR and not part of role.update PR as had more DTos and structure