register task to assign picture password by ozer550 · Pull Request #14565 · learningequality/kolibri

ozer550 · 2026-04-09T17:43:50Z

Summary

WIP
…

References

…

Reviewer guidance

…

AI usage

github-actions · 2026-04-09T18:29:20Z

Build Artifacts

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

Smoke test screenshot

Download screenshot

ozer550 · 2026-04-13T11:38:18Z

kolibri/core/auth/api.py

+                ),
+            )
+            .annotate(
+                picture_passwords_exhausted=Case(


How do we incorporate this in the viewset action as the annotated field will only be available through api? Do we want to add a separate field in the model itself?

rtibblesbot

CI passing. No screenshots in the PR body and the dev server wasn't running locally, so UI was not visually verified — please add screenshots before requesting final review. PR description (summary, references, reviewer guidance, AI usage) is still template placeholder text; AI usage is marked REQUIRED in the template.

suggestion × 4: see inline comments
praise × 1: see inline comment

@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

kolibri/core/auth/api.py

rtibblesbot · 2026-04-13T11:56:51Z

kolibri/plugins/facility/frontend/composables/useFacilityEditor.js

+      method: 'POST',
+      data,
+    });
+    if (response.data.id && response.data.status) {


suggestion: Distinguishing a task response from a dataset response by checking response.data.id && response.data.status works today because FacilityDatasetSerializer has no status field, but it's implicitly relying on that absence. The endpoint returns HTTP 202 for the task case and 200 otherwise, so response.status === 202 is a more explicit and stable check.

rtibblesbot · 2026-04-13T11:56:51Z

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

+            createSnackbar(saveSuccess$());
+          }
        } catch (error) {
          createSnackbar(saveFailure$());


suggestion: saveFacilityConfig and saveFacilityLoginSettings each call copySettings() internally. When run in Promise.all, if saveFacilityLoginSettings completes first (calling copySettings() with the pending settings as the new baseline), and saveFacilityConfig then fails, the undoSettingsChange() call here restores settings to the just-copied pending state — effectively a no-op. The user sees the failure snackbar but the UI keeps the unsaved values.

One fix: remove the copySettings() calls from inside both functions and call it once in saveConfig() only after Promise.all resolves successfully.

This might be motivation for moving this task wrangling into the composable, so there's a single save function, but I don't think this is high priority

Understood — a composable-level single save function is the cleaner fix. Makes sense to defer it.

rtibblesbot · 2026-04-13T11:56:51Z

requirements/dev.txt

@@ -1,4 +1,4 @@
-setuptools
+setuptools<=81.0.0


suggestion: Pinning setuptools<=81.0.0 is unrelated to picture login. Please submit this in a separate PR with a note explaining what breakage it prevents.

rtibblesbot · 2026-04-13T11:56:51Z

kolibri/core/auth/tasks.py

+    if job:
+        job.update_progress(0, total)
+
+    for i, learner in enumerate(learners.iterator(), start=1):


praise: Using .iterator() here is the right call for large facilities — avoids loading all learner rows into memory at once. The per-user progress updates and the metadata annotation before re-raising NoAvailableSequences make failure diagnosis straightforward.

Feel free to generalize the learner count utility so you can access the queryset for use here too.

Noted — exposing the queryset from the learner count utility would let this code use it directly rather than doing a separate lookup.

…ing workflow

…e_passwords_exhausted

rtibblesbot

Prior findings

Resolved:

requirements/dev.txt:1 — unrelated setuptools pinning (suggestion) — file removed from PR diff

Unaddressed (re-raised below):

api.py:295 — permission_classes=[IsAuthenticated] override on save_facility_login_settings (suggestion)
useFacilityEditor.js:229 — response.data.id && response.data.status as task discriminant (suggestion)
index.vue:492 — copySettings() race when both Promise.all legs call it (suggestion)
PR description and screenshots still placeholder/absent (blocking)

1/5 prior findings resolved. 4 re-raised below.

The rebase commit cleanly delegates exhaustion-checking to are_picture_passwords_exhausted and drops the picture_passwords_exhausted FacilityViewSet annotation — both positive simplifications (inline praise on api.py:318). Still no screenshots and the PR description remains template placeholder text; the prior review's blocking request stands.

blocking:

PR description still contains template placeholder text ("WIP…"). Summary, references, reviewer guidance, and AI usage sections are all required before final review.
No screenshots added for the Vue/UI changes — visual inspection is not possible without them.

suggestions (re-raised): see inline comments on api.py:295, useFacilityEditor.js:229, index.vue:492

new: see inline comments on api.py:318 (praise), index.vue:159 (suggestion)

@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

kolibri/core/auth/api.py

kolibri/plugins/facility/frontend/composables/useFacilityEditor.js

rtibblesbot · 2026-04-13T18:07:02Z

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

-          await saveFacilityConfig();
-          createSnackbar(saveSuccess$());
+          pictureLoginTaskLoading.value = true;
+          await Promise.all([saveFacilityConfig(), saveFacilityLoginSettings()]);


suggestion (re-raised): Both saveFacilityConfig() (line 192 of useFacilityEditor.js) and saveFacilityLoginSettings() (line 232) call copySettings() internally. When run in Promise.all, whichever resolves first calls copySettings() — updating the baseline to the current pending state. If the other then fails, the undoSettingsChange() in the catch block restores to that just-copied pending state, making the undo a no-op. One fix: remove the copySettings() calls from inside both functions and call it once in saveConfig() only after Promise.all resolves successfully.

kolibri/core/auth/api.py

rtibblesbot · 2026-04-13T18:07:02Z

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

+                class="nested-settings picture-password-assignment-status"
+                data-testid="picture_password_assignment_status"
+              >
+                <KCircularLoader :size="24" />


suggestion: The KCircularLoader is shown without any visible or accessible label while picture passwords are being assigned. Screen-reader users won't get feedback about why the save button is disabled. Consider adding a visually-hidden string (e.g., via $computedClass with a screen-reader-only utility) or an aria-live region announcing the progress state.

bjester

I'd like to see a test on the permissions handling, and if the permissions handling could be cleaned up.

Out of scope: the existing code that was used as an example for some of this

bjester · 2026-04-13T17:14:13Z

kolibri/core/auth/test/test_api.py

+    @patch("kolibri.core.auth.api.get_assigned_sequences", return_value=set())
+    @patch("kolibri.core.auth.api.PICTURE_PASSWORD_SEQUENCE_COUNT", new=1)
+    def test_enable_rejected_when_exhausted(self, mock_assigned, mock_task):
+        mock_assigned.return_value = {"1.2.3"}


nitpick: is this overriding the default return_value=set() in the decorator above?

kolibri/plugins/facility/frontend/composables/useFacilityEditor.js

kolibri/core/auth/api.py

bjester · 2026-04-13T18:07:02Z

kolibri/core/auth/api.py

+        try:
+            dataset = FacilityDataset.objects.get(pk=pk)
+        except FacilityDataset.DoesNotExist:
+            raise Http404("Facility not found")
+
+        if not request.user.can_update(dataset):
+            raise PermissionDenied("You do not have permission to update this facility")
+
+        facility = Facility.objects.get(dataset_id=pk)


I realize the other existing actions on this viewset do this, but the reason for the permission checks might be a side effect of this.

If this code uses self.get_object, that might allow the permissions check to function properly and would likely clean this up, and the other actions, a lot. I think you could focus on trying it on your new code for now, and if it works, we can write a followup issue to address it on the other actions. A unit test to verify the permissions would provide certainty!

You might have to use a query param of facility_id instead of the PK, for it to correctly find the dataset. They should always be 1:1

kolibri/core/auth/api.py

kolibri/core/auth/tasks.py

bjester · 2026-04-13T18:15:30Z

kolibri/core/auth/tasks.py

+    if job:
+        job.update_progress(0, total)
+
+    for i, learner in enumerate(learners.iterator(), start=1):


Feel free to generalize the learner count utility so you can access the queryset for use here too.

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

bjester · 2026-04-13T18:24:05Z

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

+            createSnackbar(saveSuccess$());
+          }
        } catch (error) {
          createSnackbar(saveFailure$());


This might be motivation for moving this task wrangling into the composable, so there's a single save function, but I don't think this is high priority

LianaHarris360

The task works as expected, including when the settings are updated. There's a few areas on the frontend that could be improved.

kolibri/plugins/facility/frontend/composables/useFacilityEditor.js

LianaHarris360 · 2026-04-13T20:10:12Z

kolibri/plugins/facility/frontend/views/FacilityConfigPage/index.vue

+                class="nested-settings picture-password-assignment-status"
+                data-testid="picture_password_assignment_status"
+              >
+                <KCircularLoader :size="24" />


The KCircularLoader should be moved next to the save button instead of being in a separate div inside the KRadioButtonGroup. This way, it's clearer to the user that the spinner is related to saving the settings.

Keep in mind that since there are two save buttons based on isAppContext, there should be two loading spinners as well.

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

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

ozer550 commented Apr 13, 2026

View reviewed changes

ozer550 requested a review from rtibblesbot April 13, 2026 11:48

rtibblesbot reviewed Apr 13, 2026

View reviewed changes

LianaHarris360 requested review from LianaHarris360 and bjester April 13, 2026 15:46

ozer550 added 6 commits April 13, 2026 22:44

register task to assign picture password

e18b562

add enable_picture_login action in the FacilityDatasetViewset

386f7db

add picture login api and task tests

4ec83a8

create task polling in frontend to get updates and integrate api call…

056c99e

…ing workflow

convert picture login to a broader facility login functionality

17e6854

rebase and remove extra added changes for already existing are_pictur…

b2f369f

…e_passwords_exhausted

ozer550 force-pushed the add-api-support-for-picture-login branch from bc776c9 to b2f369f Compare April 13, 2026 17:44

rtibblesbot suggested changes Apr 13, 2026

View reviewed changes

bjester requested changes Apr 13, 2026

View reviewed changes

LianaHarris360 requested changes Apr 13, 2026

View reviewed changes

		@@ -1,4 +1,4 @@
		setuptools
		setuptools<=81.0.0

Uh oh!

Conversation

ozer550 commented Apr 9, 2026

Summary

References

Reviewer guidance

AI usage

Uh oh!

github-actions bot commented Apr 9, 2026

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LianaHarris360 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ozer550 Apr 13, 2026 •

edited

Loading