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

Consolidate list remote users api #12321

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

Conversation

AlexVelezLl
Copy link
Member

Summary

Consolidates the logic of listing remote users from the user_profile and setup_wizard plugins, and migrates them to core auth so that they are accessible by multiple plugins.

There were some differences between both endpoints, and the following decisions were made to merge them:

  • The setup wizard endpoint returned an object { admin, students }, while the user_profile endpoint returned a list of users. It was decided to keep the response the same as that of user_profile as it was more flexible, and in the setup wizard to make the admin, students filter on the frontend instead of the backend.
  • The setup wizard endpoint returns a 403 error if there are authentication errors, but the user_profile endpoint returns a 200 code, but with an error object. The 403 error was maintained, and where it is called to the user_profile endpoint in the frontend the error handling was updated.
  • The setup_wizard endpoint gets a "facility_id" param, while the user_profile endpoint gets a "facility" param. The naming of "facility_id" was preferred.
  • The user_profile endpoint validated the input data, and the setup_wizard endpoint did not. It was preferred to validate it as the user_profile.

Reviewer guidance

  • Check the flow of importing users as administrator of the setup wizard.
  • Check the flow of changing facilities for an "on my own" user, and merging users using administrator credentials.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@AlexVelezLl AlexVelezLl added the TODO: needs review Waiting for review label Jun 19, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: frontend labels Jun 19, 2024
@rtibbles rtibbles self-assigned this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: backend Python, databases, networking, filesystem... DEV: frontend SIZE: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants