Register task to assign picture password#14565
Register task to assign picture password#14565ozer550 wants to merge 10 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
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
| createSnackbar(saveSuccess$()); | ||
| } | ||
| } catch (error) { | ||
| createSnackbar(saveFailure$()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Understood — a composable-level single save function is the cleaner fix. Makes sense to defer it.
| if job: | ||
| job.update_progress(0, total) | ||
|
|
||
| for i, learner in enumerate(learners.iterator(), start=1): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Feel free to generalize the learner count utility so you can access the queryset for use here too.
There was a problem hiding this comment.
Noted — exposing the queryset from the learner count utility would let this code use it directly rather than doing a separate lookup.
ozer550
force-pushed
the
add-api-support-for-picture-login
branch
from
bc776c9 to
b2f369f
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
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 onsave_facility_login_settings(suggestion)useFacilityEditor.js:229—response.data.id && response.data.statusas task discriminant (suggestion)index.vue:492—copySettings()race when bothPromise.alllegs 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
| class="nested-settings picture-password-assignment-status" | ||
| data-testid="picture_password_assignment_status" | ||
| > | ||
| <KCircularLoader :size="24" /> |
There was a problem hiding this comment.
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
left a comment
bjester
left a comment
There was a problem hiding this comment.
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
| @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"} |
There was a problem hiding this comment.
nitpick: is this overriding the default return_value=set() in the decorator above?
| if job: | ||
| job.update_progress(0, total) | ||
|
|
||
| for i, learner in enumerate(learners.iterator(), start=1): |
There was a problem hiding this comment.
Feel free to generalize the learner count utility so you can access the queryset for use here too.
| createSnackbar(saveSuccess$()); | ||
| } | ||
| } catch (error) { | ||
| createSnackbar(saveFailure$()); |
There was a problem hiding this comment.
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
left a comment
LianaHarris360
left a comment
There was a problem hiding this comment.
The task works as expected, including when the settings are updated. There's a few areas on the frontend that could be improved.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Most prior suggestions resolved; two remain unaddressed.
CI passing. UI not verified — no screenshots in the PR and no dev server running locally. Please add screenshots before final review.
Prior findings
2 prior findings unchanged (1 resolved, 1 acknowledged).
Newly resolved:
- HTTP 202 status check in
useFacilityEditor.js(suggestion) ← was UNADDRESSED —response.data.id && response.data.statusreplaced withresponse.status === 202 && response.data.task?.id, and backend now nests the dataset and task under their own keys
Still unaddressed (re-raised below):
api.py:295—permission_classes=[IsAuthenticated]override (suggestion) — no change to this line despite agreement in threadindex.vue:159(prior) →index.vue:204/260—KCircularLoaderlacks accessible label (suggestion) — loader moved butaria-*attributes still absent
2/4 prior findings resolved or acknowledged. 2 re-raised below.
@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
| methods=["post"], | ||
| detail=True, | ||
| url_path="save-facility-login-settings", | ||
| permission_classes=[IsAuthenticated], |
There was a problem hiding this comment.
suggestion (re-raised): permission_classes=[IsAuthenticated] still overrides the viewset-level KolibriAuthPermissions. The thread confirmed the fix: drop this override so KolibriAuthPermissions handles authentication and can_update serves as defense-in-depth, matching the pattern used by resetsettings and update_pin. The manual can_update check at line 300 can stay.
There was a problem hiding this comment.
I tried removing permission_classes=[IsAuthenticated] and relying only on the viewset-level KolibriAuthPermissions, but that caused valid admin requests to save_facility_login_settings to start returning 403. The reason is that KolibriAuthPermissions.has_object_permission does not treat POST on an existing object as an update path, POST falls through to False.
There was a problem hiding this comment.
So maybe that fact that it's a POST contributes to this? If we're only doing an update, then PATCH seems appropriate.
| :disabled="!settingsHaveChanged || pictureLoginTaskLoading" | ||
| @click="saveConfig()" | ||
| /> | ||
| <KCircularLoader |
There was a problem hiding this comment.
suggestion (re-raised): KCircularLoader was moved next to the save button, which is an improvement in placement, but it still has no accessible label. While the button is disabled during loading, screen-reader users won't know why or that work is in progress. Consider adding an aria-label to the loader (e.g., aria-label="Assigning picture passwords…") or an aria-live region that announces the loading state when it starts. Same applies to the identical loader in BottomAppBar below (line 260).
| const pictureLoginTaskId = ref(null); | ||
|
|
||
| async function saveFacilityLoginSettings() { | ||
| const data = pick(settings.value, LOGIN_SETTINGS_FIELDS); |
There was a problem hiding this comment.
praise: Using pick(settings.value, LOGIN_SETTINGS_FIELDS) here with the constant already used in saveFacilityConfig to exclude the same fields is clean — the field list is defined once and both halves of the split save stay in sync automatically.
ozer550
changed the title
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good progress — the permission override and description gaps are now closed.
CI passing. Screenshot added; spinner placement next to the save button looks correct.
Prior findings
1 prior finding unchanged (1 acknowledged — index.vue copySettings race).
Newly resolved:
api.py:295—permission_classes=[IsAuthenticated]override (suggestion) ← was UNADDRESSED — override dropped in commit 6bb0079, action changed to PATCH,self.get_object()now delegates auth to the viewset-levelKolibriAuthPermissions- PR description and screenshots (blocking) ← resolved — summary, references, reviewer guidance, and AI usage are all present; screenshot added
Still unaddressed (re-raised below):
index.vue:204/260—KCircularLoaderlacks accessible label (suggestion) — no aria attributes added, no reply to thread
2/3 prior unresolved findings resolved. 1 re-raised below.
@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
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Code is looking a lot better and I think it's pretty solid. Much of the previous review feedback is resolved. I've tested it and things are working as it should.
Although, I think the styling needs some help. I think it can be simplified a bit, with some styling deduplicated and positioning enhancements. I think rtibblesbot's comment about an announcement is valid, and worth raising if we need a new string or if there's an applicable core string we could use.
| self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) | ||
|
|
||
| def _setup_task_mocks(self, mock_storage, mock_task): | ||
| from kolibri.core.tasks.job import Job |
There was a problem hiding this comment.
Likely unnecessary inline import?
| <KCircularLoader | ||
| v-if="pictureLoginTaskLoading" | ||
| :size="24" | ||
| data-testid="picture_password_assignment_status" | ||
| /> |
There was a problem hiding this comment.
Since the button is right-aligned, putting the loader to the right of the button causes the button / layout to shift. This is not pleasing to the eye. I suggest moving it to the other side
|
Also, you have a merge conflict. Tests won't run and we can't be ready to merge this until those conflicts are resolved. |
ozer550
force-pushed
the
add-api-support-for-picture-login
branch
from
6bb0079 to
6e43f6b
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
All prior findings remain resolved or acknowledged — no changes since last review.
CI still in progress (build assets) at review time; no failures detected. The PR screenshot was taken before the consolidate styling commit — the BottomAppBar now renders the spinner before the button (inverted from what the screenshot shows). Updated screenshots would help confirm the visual state.
- suggestion: Spinner/button order inconsistency — see inline comment
- praise:
Jobimport moved to file top; CSS deduplication removes theposition: absolutehack and replaces it with normal flow layout
@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
| /> | ||
| class="bottom-bar-save-group" | ||
| > | ||
| <KCircularLoader |
There was a problem hiding this comment.
suggestion: The consolidate styling commit moved the spinner before the button here, so the BottomAppBar now renders <spinner> <button> (spinner on the left). The isAppContext save area (line 193–208) retains <button> <spinner> (button on the left). These two save UIs are now visually inconsistent — one is button-first, the other spinner-first. The PR screenshot (pre-commit) showed button-then-spinner in both. Consider keeping the same order in both: button first, spinner after, to match the isAppContext layout and the existing screenshot.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior finding re-raised; new test-only commit looks good.
CI passing.
Screenshot note: the PR screenshot predates the consolidate styling commit. The BottomAppBar currently renders the spinner before the button — opposite to what the screenshot shows. Updated screenshots would confirm the intended visual.
Prior findings
1 prior finding unchanged.
Still unaddressed (re-raised below):
index.vue:250— spinner/button DOM order inconsistency in BottomAppBar (suggestion) — no reply, no code change
0/1 prior findings resolved. 1 re-raised below.
@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
| /> | ||
| class="bottom-bar-save-group" | ||
| > | ||
| <KCircularLoader |
There was a problem hiding this comment.
suggestion (re-raised): In the BottomAppBar the DOM order is <KCircularLoader> then <KButton>, but in the inline isAppContext section (line 194) it's <KButton> then <KCircularLoader>. The flex container renders them in DOM order, so the two contexts show the spinner on opposite sides of the button. Picking one order consistently (button-then-spinner matches the inline section and the existing PR screenshot) would remove the visual discrepancy.
There was a problem hiding this comment.
Putting the loader on the same side of each button will cause one of the buttons to shift when the loader is displayed, which is not visually pleasing. Disregard this suggestion.
| })); | ||
| jest.mock('kolibri/composables/useSnackbar'); | ||
| jest.mock('../../composables/useFacilityEditor'); | ||
| jest.mock('kolibri-common/composables/useTaskPolling', () => { |
There was a problem hiding this comment.
praise: The useTaskPolling mock shape ({ tasks: ref([]) }) exactly matches the destructuring in setup(), and adding pictureLoginTaskId and saveFacilityLoginSettings to createMockFacilityConfig gives the component everything it needs without over-specifying behavior — clean and minimal.
…e_passwords_exhausted
ozer550
force-pushed
the
add-api-support-for-picture-login
branch
from
6541ce0 to
877dd2d
Compare
Reviewer Guidance (scenarios to test):
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
1 prior finding unchanged; no new commits.
CI build assets still in progress; no failures detected. Visual: screenshot predates consolidate styling — see inline comment.
Prior findings
1 prior finding unchanged.
Still unaddressed (re-raised below):
index.vue:250— spinner/button DOM order inconsistency in BottomAppBar (suggestion) — no reply, no code change
0/1 prior findings resolved. 1 re-raised below.
@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
|
Hi @ozer550 and @LianaHarris360 I confirm that the points in the reviewer guidance are implemented as specified, however while regression testing I noticed that none of the options under 'Users' are working: users.settings.mp4 |
LianaHarris360
dismissed
their stale review
Requested changes have been addressed
6 participants
Summary
References
closes #14354
Reviewer guidance
kolibri/core/auth/tasks.pyinsideassign_picture_passwords_to_facilityso the task runs long enough to visibly show spinners.pytest kolibri/core/auth/test/test_api.pykolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.jsAI usage
AI tools were used to explore and validate approaches for the implementation logic. They also assisted in generating initial versions of the test cases, which were then reviewed and refined to align with the codebase and requirements.