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

Manage users in LOD #12228

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

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented May 30, 2024

Summary

  • Allow admins to remove LOD users from facility, and import them outside of the setup wizard.
  • Created a new SAP to manage users for LOD admins.
  • Created a new importLodMachine to reuse the common user import steps in LOD.
  • Refactored the wizard machine and Setup Wizard to match and use the new state machine.
  • Added a new lodOnly prop on registerNavItem to register nav items just for LOD devices.
  • Created new core.auth.deleteimporteduser endponit to remove a user in LOD devices along with its certificates and morango metadata.

Screencasts

  • Import user as admin / with credentials
Compartir.pantalla.-.2024-06-26.18_35_32.mp4
  • Re-import removed user
Compartir.pantalla.-.2024-06-26.18_38_21.mp4
  • Remove last admin
Compartir.pantalla.-.2024-06-26.18_33_36.mp4

References

Closes #12226

Reviewer guidance

Follow the specs.


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

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend SIZE: medium labels May 30, 2024
@rtibbles rtibbles self-assigned this Jun 4, 2024
@github-actions github-actions bot added APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) SIZE: large SIZE: very large labels Jun 13, 2024
@github-actions github-actions bot added the APP: User Re: User app (sign-in, sign-up, user profile, etc.) label Jun 26, 2024
@AlexVelezLl AlexVelezLl added the TODO: needs review Waiting for review label Jun 26, 2024
@AlexVelezLl AlexVelezLl marked this pull request as ready for review June 26, 2024 18:20
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Some quick feedback - I've raised my main concern (which is more with the original spec, than the work you have done here) on Slack for further discussion, but ultimately I think having this as a separate SPA is not the right direction - but we'll see what the discussion leads us to.

@@ -210,6 +211,9 @@ export default {
},
resources,
themeConfig,
machines: {
importLodUsersMachine,
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather put this in kolibri-common rather than add it to the core API spec.

@@ -4,7 +4,7 @@ export default function UserType(userObject) {
if (userObject.is_superuser) {
return UserKinds.SUPERUSER;
}
if (!userObject.roles.length) {
if (!userObject.roles || !userObject.roles.length) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to change but could also do:

if (!userObject?.roles.length) {

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

(I feel like you used this in the useBaseSearch refactor work)

@rtibbles rtibbles added the DEV: Core JS API Changes related to, or to the Core JS API label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) 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: Core JS API Changes related to, or to the Core JS API DEV: frontend SIZE: large SIZE: medium SIZE: very large TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow admins to remove LOD users from facility, and import them outside of the setup wizard
3 participants