Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to roles_controller update & delete action. Changes need to be reflected in SiteSetting DefaultRole setting. #5951

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes to roles_controller update & delete action. Changes need to be reflected in SiteSetting DefaultRole setting. #5951

wants to merge 3 commits into from

Commits on Sep 29, 2024

  1. Extended return condition to render error if Users are associated wit… 

    …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.
    @nirajkumar999
    nirajkumar999 committed Sep 29, 2024
    Configuration menu
    Copy the full SHA
    dee6663 View commit details
    Browse the repository at this point in the history

  2. Modified update and destroy action route for roles_controller. Added … 

    …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.
    @nirajkumar999
    nirajkumar999 committed Sep 29, 2024
    Configuration menu
    Copy the full SHA
    f340750 View commit details
    Browse the repository at this point in the history

  3. passed rubocop + eslint test

    @nirajkumar999
    nirajkumar999 committed Sep 29, 2024
    Configuration menu
    Copy the full SHA
    2880ed1 View commit details
    Browse the repository at this point in the history