Fixes to roles_controller update & delete action. Changes need to be reflected in SiteSetting DefaultRole setting. #5951
Conversation
…h role. A.> I have tried to modify return condition for error status :method_not_allowed by adding additional OR condition which checks if any User is associated with the role because if we don't manually do it here we get a Internal Server Error from postgres stating foreign key constraint violation which is correct at its place. B.> But at UI we get "The action cant be completed" in the toast message. This don't align with our requirements. So i have manually added the OR condition check. C.> This way I eliminate Internal Server Error stuff which is a good practice. Also the toast shows the proper message as already stated in useDeleteRole hook. The same message gets displayed when we try to delete the pre defined roles. D.> If we look carefully at UI we dont have option to delete pre defined roles. So the return condition for error status :method_not_allowed will never be true. On adding the OR condition to this we ensure proper invoking in this case.
…further error handler to useDeleteRole.jsx A.> In update action we chain the name update to SiteSetting DefaultRole. This ensures consistency.action we chain the name update to SiteSetting DefaultRole. This ensures consistency. B.> We avoid the delete action for the role that is assigned as DefaultRole in SiteSetting. If we don't do this we don't have any side effects as this case is already handled by setting fallback to User role, in case the DefaultRole is not present. But I think this should be handled properly by giving a correct error message. We return status: :forbidden in that case and the same is handled in the useDeleteRole hook. C.> What I observe is that if the update and delete action is not aligned with SiteSetting Default Role then the dropdown appearing for the setting in the UI is left empty without any valid selection. This isn't consistent.
|
|
I think some test cases defined are not expected to work this way. I dont have good knowledge to change the test cases. I leave this on your team. For update actions its easy to chain the changes upto site setting default role. Hence no issue here. For delete action, what i did is prevented the role from being deleted. But in your code you have already set a fallback to USER role. If this is the case let me know what design you want to follow. If you want a successful deletion and assign USER role to site setting default role we can do but I dont think this a suitable move. What your test case expect that its able to delete a role not associated to any user but it dont checks the point if thats a default role or not. Also in case if a role is associated with any user it expects deletion should lead to internal server error but in that case at UI toast comes as problem in completing actions thats why i returned method_not_allowed status so that a proper toast message is displayed for which our hook is already designed and seeded a proper message. My main proposal is to chain out the changes till site setting default role dropdown so that its not empty or inconsistent. Let me know what design your team wants to follow. |
|
@farhatahmad please review |
First Commit - Extended return condition to render error if Users are associated with role.
I have tried to modify return condition for error status :method_not_allowed by adding additional OR condition which checks if any User is associated with the role because if we don't manually do it here we get a Internal Server Error from Postgres stating foreign key constraint violation which is correct at its place.
But at UI we get "The action cant be completed" in the toast message. This don't align with our requirements. So i have manually added the OR condition check.
This way I eliminate Internal Server Error stuff which is a good practice. Also the toast shows the proper message as already stated in useDeleteRole hook. The same message gets displayed when we try to delete the pre defined roles.
PFA Screenshot of the issue :
Suppose I have a user with Role as : test-role
Now when i try to delete test-role i get toast as :
and in my terminal i get :
After Fixing the issue :
Toast I get :
And on terminal i get :
Second Commit - Modified update and destroy action route for roles_controller. Added further error handler to useDeleteRole.jsx
In update action we chain the name update to SiteSetting DefaultRole. This ensures consistency.
We avoid the delete action for the role that is assigned as DefaultRole in SiteSetting. If we don't do this we don't have any side effects as this case is already handled by setting fallback to User role, in case the DefaultRole is not present. But I think this should be handled properly by giving a correct error message. We return status: :forbidden in that case and the same is handled in the useDeleteRole hook.
What I observe is that if the update and delete action is not aligned with SiteSetting Default Role then the drop-down appearing for the setting in the UI is left empty without any valid selection. This isn't good practice.
PFA Screenshot of the issue :
Suppose I have a Role as : test-role. This i set as default role in site setting
Now when i update the test-role to test-role-update, it gets successfully updated while in the UI the dropdown selection gets empty.
Same situation happens when we try to delete this test-role. Deletion happens successfully.
Note : We can delete only if no users are associated to that role. Here I am considering no users are associated and then when we delete
After Fixing the issue :
On Update :
On Delete the toast i get :
On terminal output shows as :