Skip to content

Fixes Bulk User Creation Failed prop console error #13830

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fixes Bulk User Creation Failed prop console error #13830

wants to merge 6 commits into from

Conversation

@AllanOXDi
Copy link
Member

Summary

This PR fixes the console errors when creating a new user
closes #13696

References

#13696

###Before
Screenshot 2025-10-14 at 20 45 38

###After
Screenshot 2025-10-14 at 20 45 04

Reviewer guidance

  1. Navigate to facility > Users > New User
  2. Observe the console

@AllanOXDi AllanOXDi requested a review from AlexVelezLl October 14, 2025 18:44
@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: very small labels Oct 14, 2025
@AllanOXDi AllanOXDi changed the title fixes console errors at creating new user Fixes Bulk User Creation Failed prop console error Oct 14, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2025 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.19.0b1.dev0_git.34.g5b013a55.pex
Windows Installer (EXE) kolibri-0.19.0b1.dev0+git.34.g5b013a55-windows-setup-unsigned.exe
Debian Package kolibri_0.19.0b1.dev0+git.34.g5b013a55-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.19.0b1.dev0+git.34.g5b013a55.dmg
Android Package (APK) kolibri-0.19.0b1.dev0+git.34.g5b013a55-0.1.6-debug.apk
Raspberry Pi Image kolibri-pi-image-0.19.0b1.dev0+git.34.g5b013a55.zip
TAR file kolibri-0.19.0b1.dev0+git.34.g5b013a55.tar.gz
WHL file kolibri-0.19.0b1.dev0+git.34.g5b013a55-py2.py3-none-any.whl

@pcenov
Copy link
Member

pcenov commented Oct 16, 2025

Thanks @AllanOXDi - LGTM, the only remaining visible console errors are:

2025-10-16_12-09-17

If these are not in scope for this PR, then it should be good to go!

nucleogenesis
nucleogenesis requested changes Oct 17, 2025
View reviewed changes
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I'm seeing the textColor error - why was it reverted?

Otherwise the changes look okay - just want to clarify the scope before we merge.

kolibri/plugins/facility/assets/src/views/users/common/CloseConfirmationGuard.vue Outdated
isConfirmationModalOpen.value = true;
closeConfirmationToRoute.value = to;
next(false);
return;
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker. What console errors did this fix?

Copy link
Member

Choose a reason for hiding this comment

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

The intention of hook is to prevent the route navigation, and the correct way to do this is by calling next(false) https://v3.router.vuejs.org/guide/advanced/navigation-guards.html#in-component-guards. So this change should be reverted.

@AllanOXDi
Copy link
Member Author

AllanOXDi commented Oct 20, 2025 •
edited
Loading

I'm seeing the textColor error - why was it reverted?

Otherwise the changes look okay - just want to clarify the scope before we merge.

Here was the slack conversation
https://learningequality.slack.com/archives/CB37UM23A/p1760540636649449

AlexVelezLl
AlexVelezLl requested changes Oct 20, 2025
View reviewed changes
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.

Just found a couple of regressions. I feel the error in the KSelect value should be solved by updating the handler of the selectValue computed property.

kolibri/plugins/facility/assets/src/views/users/common/CloseConfirmationGuard.vue Outdated
isConfirmationModalOpen.value = true;
closeConfirmationToRoute.value = to;
next(false);
return;
Copy link
Member

Choose a reason for hiding this comment

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

The intention of hook is to prevent the route navigation, and the correct way to do this is by calling next(false) https://v3.router.vuejs.org/guide/advanced/navigation-guards.html#in-component-guards. So this change should be reverted.

kolibri/plugins/facility/assets/src/views/users/sidePanels/UserCreate/ClassesSelect.vue
Copy link
Member

Choose a reason for hiding this comment

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

The selectedOption here is not doing anything, its just a ref property that is initially set to {} but never changes. The previous selectValue property is the correct value that should be passed here, as this handles the select all behaviour https://github.com/AllanOXDi/kolibri/blob/45135e4a7b987684c34223770d01244e2138c314/kolibri/plugins/facility/assets/src/views/users/sidePanels/UserCreate/ClassesSelect.vue#L63.

This update is causing regressions when on the select all checkbox:

Before this PR:

Grabacion.de.pantalla.2025-10-20.a.la.s.10.08.38.a.m.mov

After this PR:

Grabacion.de.pantalla.2025-10-20.a.la.s.10.06.34.a.m.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlexVelezLl I think this could be addressed from the KSelect by allowing the value prop accept either Array or Object. I have just made a fix to it on this PR.
learningequality/kolibri-design-system#1150

Copy link
Member

Choose a reason for hiding this comment

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

Hey @AllanOXDi. Yes! It seems that having KSelect support an array value should mute the error logs here; Jus commented on the PR a minor nitpick before merging the PR. However, the selectedOption change should be reverted here as well!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could just close this PR after - once we fixed from KSelect itself

Copy link
Member

Choose a reason for hiding this comment

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

I mean, we still have the addition of the missing return of the showErrorWarning ref 😄.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved

@nucleogenesis nucleogenesis mentioned this pull request Oct 23, 2025
Merged
@AllanOXDi AllanOXDi requested a review from AlexVelezLl October 30, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nucleogenesis nucleogenesis nucleogenesis requested changes

@AlexVelezLl AlexVelezLl Awaiting requested review from AlexVelezLl

Requested changes must be addressed to merge this pull request.

Assignees

@AlexVelezLl AlexVelezLl

Labels

APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small SIZE: very small

Projects

None yet

Milestone

Kolibri 0.19: Planned Patch 1

Development

Successfully merging this pull request may close these issues.

[Bulk User Creation]: Failed prop console error

4 participants

@AllanOXDi @pcenov @nucleogenesis @AlexVelezLl