Skip to content

Conversation

@DanBloxham-sw
Copy link
Contributor

@DanBloxham-sw DanBloxham-sw commented Jun 8, 2022

… at centre if it exists

JIRA link

https://softwiretech.atlassian.net/browse/HEEDLS-958

Description

Promote to Admin now reactivates the inactive admin account for that delegate at that centre (if it exists). It will also update the record with personal details (name, etc.) from the delegate record to make sure it is up to date, before setting the admin roles as the user specified on the form page.

Screenshots

N/A, all backend changes.


Developer checks

(Leave tasks unticked if they haven't been appropriate for your ticket.)

I have:

  • Run the formatter and made sure there are no IDE errors.
  • Written tests for the changes (accessibility tests, unit tests for controller, data services, services, view models, etc)
  • Manually tested my work with and without JavaScript. Full manual testing guidelines can be found here: https://softwiretech.atlassian.net/wiki/spaces/HEE/pages/6703648740/Testing
  • Updated/added documentation in Swiki and/or Readme. Links (if any) are below:
  • Updated my Jira ticket with information about other parts of the system that were touched as part of the MR and have to be sanity tested to ensure nothing’s broken.
  • Scanned over my own MR to ensure everything is as expected.

adminUser.Id
);
passwordDataService.SetPasswordByAdminId(adminUser.Id, delegateUser.Password);
userDataService.UpdateAdminUserPermissions(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that this does not force the IsCentreManager permission to false like we do when registering a new admin. So if a user was a centre manager, and was then deactivated, then reactivated, they would still be a centre manager.

I don't think this will cause any issues, but it is a scenario where we have the potential for an admin to "create" an admin with higher permissions than they themselves have. @SteveJacksonSoft shall I leave this as it is, or ensure the IsCentreManager permission is set to false here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch. I think in nearly all use cases we would want them not to be a centre manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the ReactivateAdmin data service method revoke permissions higher than CentreAdmin (CentreManager and UserAdmin but UserAdmin is unlikely to be true in any case).

Though I now realise that the promote to admin button and controller require the user to be a centre manager anyway. So centre managers are probably safe reactivating other centre managers. Given that centre manager is a substantial permission, we might still want to revoke it (and UserAdmin) by default. Not sure if there are limits on the number of centre managers, but I don't think centre managers are something admins can easily promote others to. @SteveJacksonSoft thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally there's expected to be a single centre manager per centre. I think the reason we don't let managers make people centre manager or user admin when they're promoting them is that we don't want it to be possible.
So I think it's safe to say we should revoke any existing centre manager or user admin permissions.

Copy link
Contributor

@AlexJacksonDS AlexJacksonDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing else to add, other than the stuff already noted

@DanBloxham-sw DanBloxham-sw merged commit aa58e32 into master Jun 10, 2022
@DanBloxham-sw DanBloxham-sw deleted the HEEDLS-958-PromoteAdminWithInactiveAccountFix branch June 10, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants