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

Capture more errors from the backend and show error messages for them #11807

Merged

Conversation

jredrejo
Copy link
Member

Summary

In the wizard setup, when importing users from a remote facility

  • Force validation in some cases that were not covered
  • Better capture of the error messages provided by the backend
  • Clean previous errors when the modal to use admin credentials is shown

References

Closes #11711

In reference to #11711 , the last point of the description:

when we have both the Username and Password fields displayed and I enter the username of a learner who was originally created without a password then it's not accepted as valid credentials and I have to import that learner signed in as an admin

has not being changed as it seems a correct behaviour of the application. When facility settings are changed, previous user not requiring a password to login can't login until they have a password. Importing them remotely should not be different. In any case, if @jtamiace has a better solution, perfect.

Reviewer guidance

Follow the steps @pcenov described in #11711

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

@jredrejo jredrejo added the TODO: needs review Waiting for review label Jan 26, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: frontend SIZE: small labels Jan 26, 2024
@rtibbles
Copy link
Member

when we have both the Username and Password fields displayed and I enter the username of a learner who was originally created without a password then it's not accepted as valid credentials and I have to import that learner signed in as an admin

I think this is correct, and only allowing to do this on the Kolibri device the user is on and not remotely seems more secure.

@pcenov
Copy link
Member

pcenov commented Jan 29, 2024

Hi @jredrejo while regression testing the import of individual user accounts I noticed that now if I select the 'Use an admin account' option, when I click the 'Back' arrow then I am brought back to the 'Import individual user accounts' page and I can't proceed because now the 'Username' and 'Password' fields are required.
This workflow was confusing previously as well but the admin was still able to click the 'Continue' button and proceed with the import. So it should be further discussed how to handle that case.

Here's a video of what I am doing:

2024-01-29_18-28-18.mp4

@jredrejo
Copy link
Member Author

Hi @jredrejo while regression testing the import of individual user accounts I noticed that now if I select the 'Use an admin account' option, when I click the 'Back' arrow then I am brought back to the 'Import individual user accounts' page and I can't proceed because now the 'Username' and 'Password' fields are required. This workflow was confusing previously as well but the admin was still able to click the 'Continue' button and proceed with the import. So it should be further discussed how to handle that case.

Here's a video of what I am doing:

2024-01-29_18-28-18.mp4

@pcenov fixed!

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thank you @jredrejo - manual QA passes, good to go!

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.

All changes make sense, manual QA checks out. @marcellamaki I suspect we should merge this for 0.16.0.

@marcellamaki
Copy link
Member

Yes - that sounds good to me @rtibbles. This seems worth including, and as @pcenov has done manual QA, I feel confident about us merging it in :)

@marcellamaki marcellamaki merged commit 6008a25 into learningequality:release-v0.16.x Jan 31, 2024
34 checks passed
@jredrejo jredrejo deleted the small_validation_errors branch February 1, 2024 13:24
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.) DEV: backend Python, databases, networking, filesystem... DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants