-
Notifications
You must be signed in to change notification settings - Fork 699
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: os_user unnecessary password field #11814
fix: os_user unnecessary password field #11814
Conversation
@marcellamaki any updates on it? |
Hi @ThEditor, thanks for the contribution! We have a queue of pull requests. Generally, a review may take up to two weeks. |
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ThEditor - I apologize for the delay in following up on this PR. The code changes look good and I've done manual QA to confirm. Thank you for your contribution here!
Its fine, no worries. Thanks for approving the PR. :D |
I'm just rerunning the Python checks, and if it sorts itself out, this will be good to merge, and I'll do that later today :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change the code here, but flagging that the issue this is fixing is targeted for Planned Patch 1 - this means that this PR should be targeted at the release-v0.16.x branch, not develop!
For details on how to do a rebase from one to another, please see here: https://kolibri-dev.readthedocs.io/en/develop/howtos/rebasing_a_pull_request.html
Note, I have retarged the pull request already, so you just need to do the rebase and force push to update the PR branch.
I've rebased the PR, does it look correct? |
From the manual QA point of view we're struggling to find the real-world scenario this PR is supposed to fix... Is it only replicable in the dev environment, or there is some angle we're missing when using built assets? |
Thanks @ThEditor - I confirm that the issue described in #11777 is now fixed. cc @marcellamaki |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual QA passed, good to go! 💯 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for retargeting this PR @ThEditor and for your patience!
Requested change - rebasing the PR - has been addressed
7a953e2
into
learningequality:release-v0.16.x
Summary
Resets
usernameSubmittedWithoutPassword
back tofalse
onclearUser
(executed when user goes back using "Change User")References
Closes #11777
Reviewer guidance
Run Kolibri locally using (https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#running-in-app-mode). (let's call this "os_user")
Select "pw_user". You should be directed to the next step, where you enter your password. Do not sign in. Go back via "Change User"
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)