feat: implement delete role action#423
Conversation
|
Sorry, the PR may not be broken up into small focused commits. If you want, I can rebase later. |
|
Yes, thank you so much for you time. |
Ptrskay3
left a comment
Ptrskay3
left a comment
There was a problem hiding this comment.
Marked some ambiguous parts and future improvements
| let users_affected = self | ||
| .user_role_service | ||
| .delete_role_relations_by_id(params.role_id) | ||
| .await | ||
| .map_err(|_| RoleError::InternalServerError)?; |
There was a problem hiding this comment.
Deleting a role should somehow interact with existing users. Basically there are two options:
- Don't allow deleting a role if any users have it. It's probably correct to do that, but in my opinion that is a horrible user experience, since you may have to care about tons of users and unassign/replace the existing role.
- Wipe all user relations tied to the role. This is what I ended up doing.
I'm not sure which path is better tho, I'm happy to hear your opinion.
There was a problem hiding this comment.
I think that for the sake of convenience, if a role is deleted, the relationships --> should be automatically deleted.
I just looked at the migrations and we have an ON DELETE CASCADE.
There was a problem hiding this comment.
Sure, I missed the ON DELETE CASCADE part. Deleted this whole logic , along with the users_affected return value in 4f519eb
core/src/domain/user/ports.rs
Outdated
| fn delete_role_relations_by_id( | ||
| &self, | ||
| role_id: Uuid, | ||
| ) -> impl Future<Output = Result<u64, UserError>> + Send; |
There was a problem hiding this comment.
Not sure this is the right place or the right name for this fn, but it does the thing.
There was a problem hiding this comment.
Maybe it is not necessary, because by default the relation is deleted when the user or the role is deleted.
| self.role_service | ||
| .delete_by_id(params.role_id) | ||
| .await | ||
| .map_err(|_| RoleError::InternalServerError)?; |
There was a problem hiding this comment.
Currently there is no protection to delete all roles, even system-level ones, that may break the whole Ferriskey instance.
There was a problem hiding this comment.
Yes is similar to keycloak, in keycloak you can update the realm master name, and its finish xD
Maybe later , add the defensive system for this use-case.
| // FIXME: there is no bulk delete endpoint, and this one may be inefficient, and the | ||
| // stacked toast messages will look bad. |
There was a problem hiding this comment.
Arguably there shouldn't be a bulk delete endpoint for this, but the issue in the comment is somewhat real. I'm no React magician by any means, but happy to hear suggestions.
| pub async fn get_role( | ||
| Path(realm_name): Path<String>, | ||
| Path(role_id): Path<Uuid>, | ||
| Path((realm_name, role_id)): Path<(String, Uuid)>, |
There was a problem hiding this comment.
I'm using 0.8.4 axum, and these changes were necessary for me to avoid a panic, namely
Note that multiple parameters must be extracted with a tuple `Path<(_, _)>` or a struct `Path<YourParams>
There was a problem hiding this comment.
Yes, we had to use non-typed routes, and during migration we put a lot of endpoints in this case. I fix them when I come across them.
|
It's ready for review ? |
|
Yes. A cargo fmt is missing, will do it soon. |
NathaelB
left a comment
NathaelB
left a comment
There was a problem hiding this comment.
Thank you for your time, for all the fixes you suggested, and for the role deletion feature.
2 participants
Closes #378.
Still WIP.