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

fix: validation #11672

Closed

Conversation

nick2432
Copy link
Contributor

@nick2432 nick2432 commented Dec 25, 2023

Summary

I updated the response in error so that the correct error can be catched on the frontend.
Screenshot from 2023-12-25 14-21-53

Screenshot from 2023-12-25 14-21-49

References

Fixes #11540

Reviewer guidance


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... SIZE: very small labels Dec 25, 2023
@akolson akolson requested review from AlexVelezLl, pcenov and radinamatic and removed request for pcenov December 29, 2023 17:20
@nick2432
Copy link
Contributor Author

nick2432 commented Jan 2, 2024

@AlexVelezLl @radinamatic Why are these tests failing?

@radinamatic
Copy link
Member

@nick2432 Did you try running the recommended commands to fix the linting errors?

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Tried to replicate the issue but couldn't be able to get the 500 error that @pcenov reported. Will wait for @pcenov to review the PR and confirm that it solves the problem.

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 3, 2024

Tried to replicate the issue but couldn't be able to get the 500 error that @pcenov reported. Will wait for @pcenov to review the PR and confirm that it solves the problem.

ok

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 3, 2024

@nick2432 Did you try running the recommended commands to fix the linting errors?

No, I haven't tried the recommended commands yet. I'll give them a shot. Thanks for the suggestion!

@radinamatic
Copy link
Member

@pcenov Artifacts should be ready for testing, thank you!

@pcenov
Copy link
Member

pcenov commented Jan 5, 2024

Hi @nick2432 could you double check what is going here as when testing with the latest .deb asset I am not seeing any changes to the issues described in #11540
Specifically if I try to import a learner by inputting the username with incorrect letter case (the correct username is L1 while I am typing l1) then when I click the Finish button I still get an error screen.

2024-01-05_14-52-54.mp4

Also there's still the 500 Internal Server error issue if on the server I have enabled the Require password for learners setting at Facility >Settings:

2024-01-05_14-56-21.mp4

On a side note @radinamatic while taking a closer look at the required fields validation behavior at both Import individual user accounts and Use an admin account it's completely missing at the moment. I think this will be covered by work here #11605 but posting a recording of what I am observing to keep things consolidated:

2024-01-05_15-22-33.mp4

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 6, 2024

@pcenov yes, this one is not working, but when I made changes to the code and tested it locally, it worked fine. I believe the 500 internal error is resolved by @bjester . Link: (#11541). I only resolved the validation error part.

2024-01-06.09-36-35.mp4

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

As @pcenov noted, this introduces a regression that was fixed in #11541 and therefore causes a 500 error under specific circumstances:

Also there's still the 500 Internal Server error issue if on the server I have enabled the Require password for learners setting at Facility >Settings

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 8, 2024

When I used this change, I did not encounter a 500 error, as shown in the video above, @bjester .

@pcenov yes, this one is not working, but when I made changes to the code and tested it locally, it worked fine. I believe the 500 internal error is resolved by @bjester . Link: (#11541). I only resolved the validation error part.

2024-01-06.09-36-35.mp4

@bjester
Copy link
Member

bjester commented Jan 8, 2024

@nick2432 There are two code paths that you changed, which result in two different AuthenticationFailed scenarios. The video you shared seems to produce one of them, but what about the other? If we assume the 500 error is a regression and caused by the same issue, and you take a look at the fix for the 500 error, it occurred when the server was preparing a 403 response:

  File "/usr/lib/python3/dist-packages/kolibri/core/utils/exception_handler.py", line 21, in custom_exception_handler
    response.data = _handle_403_format(response, context)
  File "/usr/lib/python3/dist-packages/kolibri/core/utils/exception_handler.py", line 60, in _handle_403_format
    "id": response.data["detail"].code.upper(),
TypeError: list indices must be integers or slices, not str

Your video shows a 401 response being returned.

@rtibbles
Copy link
Member

rtibbles commented Jan 8, 2024

Before continuing to work on this here, it is worth noting that some intersecting work on a related issue may be useful to work on top of. @jredrejo's PR #11704 when merged will be good to work on top of (and may be perhaps worth starting fresh on top of to solve just the uppercase validation issue).

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 9, 2024

@rtibbles , should I close this PR and wait until #11704 is merged?

@rtibbles
Copy link
Member

#11704 has now been merged. I'd suggest starting from current release-v0.16.x, focusing specifically on the casing issue.

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 11, 2024

#11704 has now been merged. I'd suggest starting from current release-v0.16.x, focusing specifically on the casing issue.

@rtibbles @pcenov @bjester
If the letter case is incorrect, what should I do? Should it return an error stating 'username not valid'?

@nick2432 nick2432 closed this Jan 17, 2024
@nick2432 nick2432 deleted the validation-fix branch January 23, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup Wizard > LOD > Import user - Missing validation for uppercase in the usernames and passwords
7 participants