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.

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

feat: help users with user role assignment [DHIS2-18422] [DHIS2-18446] #1506

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

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Dec 18, 2024

Background

This PR covers two tickets:

These are conceptually linked because both tickets were created to help users understand and troubleshoot why they were not able to assign user roles from the users app.

Changes

user roles - legacy authorities

You will now see a list of "legacy" authorities (or orphan authorities) when looking at a user role (if applicable).

"Facility Tracker" role on Sierra Leone v42

image

Antenatal care program (no legacy authorities)

image

user role edit page - assignability warning

(Note, none of these warnings appear for superusers (e.g. system user))

-If the role has an authority you do not have, you will see a message explaining that you cannot assign it (by default the list of authorities is collapsed)
image

-If you set Allow users to grant own roles to false in Settings app, you will get a warning about roles that you are assigned to:
image

user role view page - assignability warning

  • the view details page has the same warnings as above. The route logic has been updated to allow all users to have access to user-roles/view/{id} (users are still only able to see details for user roles they have access to)
  • by default, the list of authorities that you do not have will be expanded on this page

user edit/create page - warnings about unassignable roles

  • if you create/edit a user, you will now have a warning about roles that are not assignable by you. Each role has a link to the details page where the warning explains why you cannot assign the user.
image

Testing

  • I've manually tested with a variety of users to try to cover the scenarios. Note that superusers will have none of the warnings detailed above; the only change they will see is the addition of.the UI to remove legacy authorities

Automated tests

  • as a general note, I wanted to use for more of these tests, but it appears not to work well in this app 😕, e.g. the useDataQuery requests always remain in the loading state regardless of the data I pass to CustomDataProvider. It does seem to be called, it's just not resolving for some reason. Anyway, that made me structure the tests slightly different than I would have liked.
  • I've added some integration-level tests for the RoleForm that cover the UI for removing legacy authorities and the warning messages relating to whether user can assign a user role (this covers the main changes of this PR)
  • I've added a couple more unit-level tests relating to the warnings in the users page (I wanted to do this more as an integration-level test from UserForm, but setting up the mocking was going to take more time than I thought it was worth)
  • ideally I would have also added a test to check that all users can get to the user-roles/view/{id} page, but I thought this was going to be a pain to set up for limited value (so just going with manual testing on that point)

General code notes

  • I've refactored slightly, e.g. because I needed to reuse a system/authorities call on the user role details "/view" page, so I've put this request into a provider.
  • I didn't like that the CurrentUserProvider was not in the directory "providers", so I've moved it to try to be more consistent with where the context/providers setups are stored.

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Dec 18, 2024

🚀 Deployed on https://pr-1506--dhis2-user.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify December 18, 2024 08:58 Inactive
})
})

it.skip('does not show a warning if user cannot assign role because role is assigned to user and keyCanGrantOwnUserAuthorityGroups:true ', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm skipping this test because the mocking of useSystemInformation is not working (and I don't know why 😢). The original mocked value (with keyCanGrantOwnUserAuthorityGroups:false is being used in the but in other files the values that I mock within the test get used 🤔)

@tomzemp tomzemp requested a review from a team December 18, 2024 09:08
@tomzemp
Copy link
Member Author

tomzemp commented Dec 19, 2024

@flaminic and I talked through this high-level and we noticed that when removing legacy authorities on debug/dev with admin user the request succeeds but then the user was being logged out. Need to ask the backend team about why that might be happening 🤔

(also a general note: e2e is failing because fixtures are not updated, but we are waiting on a backend fix to DHIS2-18646)

@dhis2-bot dhis2-bot temporarily deployed to netlify December 20, 2024 15:22 Inactive
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.

2 participants