Update user edit page and user table for picture login by bjester · Pull Request #14573 · learningequality/kolibri

bjester · 2026-04-09T22:50:17Z

Summary

Adds new UserPicturePassword component for rendering a picture password with semantic structure
Modifies the user edit page:
- To show the picture password of a learner if they have one and feature is enabled
- Shows alert for when picture passwords are exhausted and disallows learner creation
- Minor structural changes
- Uses accessible friendly structure for showing picture password
Updates user tables to hide reset password option for learners when picture password feature is enabled
Adds frontend tests for the new changes

Learner	Coach

References

closes #14426

Reviewer guidance

modify LEARNER_PICTURE_PASSWORD_LIMIT in kolibri/core/auth/constants/picture_passwords.py to temporarily lower the learner limit
is the learner option disabled for coaches when over the limit?
does a learner's picture password show?
is there a reset password menu options for learners in the user table?

AI usage

I used AI to implement most of the changes at first. Then I revised much of the changes to the user edit page to simplify and improve it. Then used AI to help write the tests, which it got stuck, then I got stuck, then figured it out.

rtibblesbot

Clean, well-scoped implementation. CI is still running. Screenshots provided and verified against acceptance criteria.

suggestion: See inline comment on iconName — could be silent rendering failure if icon_style is ever unset
nitpick: aria-readonly on <ol> (see inline)
praise: Defensive check in disableLearnerRoleOption and submitForm guard (see inline)

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
Architecture: duplicated concerns, minimal interfaces, composition over inheritance
Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
Checked CI status and linked issue acceptance criteria
For UI changes: inspected screenshots for layout, visual completeness, and consistency

github-actions · 2026-04-09T23:31:21Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.20.0.dev0_git.20260413200403.pex
Windows Installer (EXE)	kolibri-0.20.0.dev0+git.20260413200403-windows-setup-unsigned.exe
Debian Package	kolibri_0.20.0.dev0+git.20260413200403-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.20.0.dev0+git.20260413200403.dmg
Raspberry Pi Image	kolibri-pi-image-0.20.0.dev0+git.20260413200403.zip
TAR file	kolibri-0.20.0.dev0+git.20260413200403.tar.gz
WHL file	kolibri-0.20.0.dev0+git.20260413200403-py2.py3-none-any.whl

Smoke test screenshot

Download screenshot

bjester · 2026-04-10T20:52:29Z

I'm still working on a test for the user-edit page changes. It doesn't have any existing tests, so I'm keeping it scoped to the new changes (bare minimum). Battling it a bit though 😅

bjester · 2026-04-10T22:41:05Z

+        if (this.disableLearnerRoleOption && this.newUserKind === UserKinds.LEARNER) {
+          this.isLearnerLimitModalOpen = true;
+          return;
+        }


AI added this. I don't think hitting this is possible, but is it worth keeping?

Missed this earlier, sorry. It doesn't seem like we'll ever this point, but it is only defensive so should be fine.

rtibblesbot

Prior findings resolved; no new blocking issues found.

Prior findings

Resolved:

aria-readonly on <ol> had no a11y effect (nitpick) — removed from UserPicturePassword.vue

Acknowledged (not re-raised):

icon.iconName undefined for unrecognized iconStyle (suggestion) — author confirmed icon_style is always set when picture_password_settings is non-null

2/2 prior findings accounted for. 0 re-raised.

CI passing (APK build failure appears pre-existing and unrelated to these changes). Screenshots verified: learner view correctly shows picture password section with 3 colorful icons and captions; coach view correctly hides the section and shows the learner limit warning with "Learn more" link — both match the acceptance criteria.

nitpick: See inline comment on UserEditPage.spec.js:188
praise: See inline comment on UserPicturePassword.vue:3

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-04-12T05:59:05Z

@@ -0,0 +1,93 @@
+<template>
+
+  <ol class="picture-password-icons">


praise: Extracting the picture password display into its own component is the right call — it keeps UserEditPage readable and gives the sub-component a clear single responsibility. The semantic structure (<ol>/<li>/<figure>/<figcaption>) is well-chosen: <ol> correctly conveys ordered sequence, and <figcaption> ties each label to its icon in a way screen readers understand. The prop validator catching malformed strings at the boundary is good defensive practice.

rtibblesbot

Tests added, prior finding resolved — good to go.

CI: frontend tests pass; APK build failure is pre-existing. Screenshots verified: learner view shows picture password section with icons and captions, coach view shows learner limit warning with "Learn more" link.

Prior findings

All prior findings remain resolved or acknowledged — no changes since last review.

Newly resolved:

Test setup used { ...mockUser, kind: UserKinds.ADMIN } instead of createUser(UserKinds.ADMIN) (nitpick) ← was UNADDRESSED — fixed at UserEditPage.spec.js:188

1/1 prior finding resolved.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-04-13T15:23:03Z

+  member_of: [],
+};
+
+function createUser(kind = UserKinds.LEARNER, overrides = {}) {


praise: createUser() helper is now used consistently throughout all test cases — roles is correctly populated so UserType() will read it properly, and the overrides param keeps individual tests concise. Good clean-up.

pcenov · 2026-04-16T10:48:56Z

Hi @bjester - I confirm that everything is implemented as specified. I just have the following clarifying questions:

When I go to Facility > Settings and I modify the How learners sign in settings then when I go to Facility > Users I have to manually refresh the page in order to see the previously applied setting taking effect. Is fixing this in scope of this PR?

settings.mp4

When editing a Coach or an Admin I can see the string "Learner creation is currently disabled due to reaching limit of 1300 learners. Learn more" but at the same time if I go to manually add a new learner that's still possible so I'm not sure how to interpret that. Perhaps it should be clarified that it's not possible to create learners with picture password?

limit.reached.mp4

pcenov · 2026-04-16T13:24:30Z

Ah, I see that point 2 is being implemented in #14600, please ignore it :)

LianaHarris360 · 2026-04-16T14:07:03Z

For point 1, there is currently an issue written & assigned to address it.

LianaHarris360

The userEditPage + userTable have been updated as specced and no issues found through QA testing, thanks Blaine!

bjester · 2026-04-16T14:35:02Z

Thanks @pcenov! Point (1) will be fixed in this issue #14545

…ity#14573) * Update utility to use constants * Add ambiguous key for iconName * Update user edit page to show picture password and disable learner role * Disable reset password when piclo enabled * Create basic test for UserEditPage changes

github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: medium labels Apr 9, 2026

bjester requested a review from rtibblesbot April 9, 2026 22:51

rtibblesbot reviewed Apr 9, 2026

View reviewed changes

Comment thread kolibri/plugins/facility/frontend/views/UserEditPage.vue Outdated

Comment thread kolibri/plugins/facility/frontend/views/UserEditPage.vue Outdated

Comment thread kolibri/plugins/facility/frontend/views/UserEditPage.vue

LianaHarris360 reviewed Apr 10, 2026

View reviewed changes

Comment thread packages/kolibri-common/utils/picturePassword.js

bjester force-pushed the piclo-user-edit branch from a92611e to 9c23138 Compare April 10, 2026 22:36

github-actions bot added SIZE: large and removed SIZE: medium labels Apr 10, 2026

bjester commented Apr 10, 2026

View reviewed changes

bjester mentioned this pull request Apr 10, 2026

Use iconName in return value of getPicturePasswordIcons #14585

Open

4 tasks

rtibblesbot reviewed Apr 12, 2026

View reviewed changes

bjester force-pushed the piclo-user-edit branch from 9c23138 to 1ae6b58 Compare April 13, 2026 15:02

rtibblesbot approved these changes Apr 13, 2026

View reviewed changes

LianaHarris360 requested a review from AlexVelezLl April 13, 2026 15:46

LianaHarris360 reviewed Apr 13, 2026

View reviewed changes

Comment thread packages/kolibri-common/components/UserPicturePassword.vue

bjester added 5 commits April 13, 2026 13:03

Update utility to use constants

6f34113

Add ambiguous key for iconName

2e98a66

Update user edit page to show picture password and disable learner role

2aa49ee

Disable reset password when piclo enabled

dab3d4a

Create basic test for UserEditPage changes

f2ca100

bjester force-pushed the piclo-user-edit branch from 1ae6b58 to f2ca100 Compare April 13, 2026 20:04

LianaHarris360 approved these changes Apr 16, 2026

View reviewed changes

LianaHarris360 merged commit 6457a41 into learningequality:develop Apr 16, 2026
62 of 63 checks passed

		@@ -0,0 +1,93 @@
		<template>

		<ol class="picture-password-icons">

Uh oh!

Conversation

bjester commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

AI usage

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Artifacts

Uh oh!

Uh oh!

bjester commented Apr 10, 2026

Uh oh!

bjester Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

LianaHarris360 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Uh oh!

rtibblesbot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

rtibblesbot Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pcenov commented Apr 16, 2026

Uh oh!

pcenov commented Apr 16, 2026

Uh oh!

LianaHarris360 commented Apr 16, 2026

Uh oh!

LianaHarris360 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjester commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bjester commented Apr 9, 2026 •

edited

Loading

github-actions bot commented Apr 9, 2026 •

edited

Loading