-
Notifications
You must be signed in to change notification settings - Fork 760
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 "change learning facility" workflow bugs #13122
base: develop
Are you sure you want to change the base?
Fix "change learning facility" workflow bugs #13122
Conversation
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.
One thing that seems to be required by the vuejs documentation.
}, | ||
role() { | ||
return this.state.value.role; |
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.
Also perhaps a sign that we should do more targeted provide/inject than a monolithic state
object.
@@ -99,6 +100,7 @@ | |||
|
|||
const isFormSubmitted = ref(false); | |||
const isPasswordInvalid = ref(false); | |||
const passwordTextbox = ref(null); |
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.
Dear god, I hadn't come across the Vue composition API syntax for template refs before, this is truly horrendous.
Noting that you have skipped one piece of guidance from the documentation:
https://vuejs.org/guide/essentials/template-refs
Prior to 3.5 you should return the ref from the setup function also.
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.
Ah, thanks! I'll update
…r and ensure password input errors always display, even if user is not clicking into the form field
51b882f
to
7c57e71
Compare
Build Artifacts
|
Still not able to change facility using the assets from this PR... 😕 On LoD, after I set up the password required on the class facility, the screen returns to the previous step (after selecting the facility), and it seems to be in the loop there. Also note the missing (empty quotes) facility and learner names, and the scrolling needed to get to the 'Continue' button. change-facility.mp4 |
Same experience if LoD is set up on a Windows device, a 400 error in there... Home folder from the LoD: .kolibri.zip change-facility-windows.mp4 |
Summary
There were a few bugs in the change facility workflow, I think due to some lingering side effects of composition API updates and other tech debt 0.18 projects. This PR is mainly around syntax adjustment to account for that (the underlying logic is unchanged), and also makes use of an a prop in
KTextbox
to more clearly surface password errors to the user.References
Fixes #13097
Reviewer guidance
You should be able to successfully create or merge accounts
change-facility.mov