Add new facility setting fields to support picture password login. by AllanOXDi · Pull Request #14400 · learningequality/kolibri

AllanOXDi · 2026-03-18T07:22:50Z

Summary

This PR adds picture_password_settings JSONField on FacilityDataset and update Facility Dataset Serializers and Viewsets to include the new field. Whether picture login is enabled for a facility is determined by checking whether picture_password_settings is null, a non-null value means the setting is active.

Add picture_password_settings JSONField to FacilityDataset (default None); a non-null value indicates picture login is enabled for the facility
Update FacilityDatasetSerializer to expose the field with structural validation (icon_style must be "standard" or "colorful", show_icon_text must be a boolean)
Update PublicFacilitySerializer to expose the field so the login screen can determine which login UI to show
Update FacilityDatasetViewSet and FacilityViewSet to include picture_password_settings in their responses

closes #14351

References

#14351

Reviewer guidance

Run the relevant tests including API smoke tests and ensure they do pass. For API smoke tests enable picture login ,confirm incompatibility is rejected.

AI usage

Gemini help a lot in planing and writing some test.

rtibblesbot

New facility settings for picture-password login: model fields, migration, serializer validation, login enforcement, and incompatibility guards.

CI still in progress — multiple Python unit test and Morango integration jobs are failing or pending. PR description is empty (placeholder … in all sections) and no issues are linked.

blocking: additionalProperties: False on syncable JSON field risks breaking morango sync (see inline)
blocking: No tests for the login enforcement logic — the security-critical auth changes (see inline)
suggestion: PR targets release-v0.19.x, not develop — is this intentional for a new feature?
suggestion: Double-negation in learner role check is hard to follow (see inline)
praise: Serializer validate correctly handles PATCH by falling back to instance values
praise: Login blocking uses a generic INVALID_CREDENTIALS error to prevent username enumeration

@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/models.py

kolibri/core/auth/test/test_api.py

kolibri/core/auth/api.py

kolibri/core/auth/test/test_datasets.py

kolibri/core/auth/serializers.py

kolibri/core/auth/api.py

rtibblesbot

Delta review — only the [pre-commit.ci lite] apply automatic fixes commit landed since the prior review. All prior findings remain unaddressed.

CI failing: makemigrations --check detects the migration is out of sync (missing validator on picture_password_settings). Morango integration tests are still running.

Prior-review status

#	Prior finding	Status
1	blocking: `additionalProperties: False` on syncable JSON field	UNADDRESSED — see inline
2	blocking: No tests for login enforcement logic	UNADDRESSED — see inline
3	suggestion: PR targets `release-v0.19.x`, not `develop`	UNADDRESSED
4	suggestion: Double-negation in learner role check	UNADDRESSED — see inline

New findings

blocking: Migration 0033 is missing the JSON_Schema_Validator on picture_password_settings, causing CI to fail (see inline)
praise: FacilityUsernameViewSet queryset correctly uses Q OR to include picture-password learners

@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/migrations/0033_facilitydataset_picture_password.py

kolibri/core/auth/models.py

kolibri/core/auth/api.py

github-actions · 2026-03-18T08:03:56Z

Build Artifacts

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

Smoke test screenshot

Download screenshot

rtibblesbot

Delta review — all four prior blocking findings are now resolved. CI still pending.

Prior-review status

#	Prior finding	Status
1	blocking: `additionalProperties: False` on syncable JSON field	RESOLVED — removed from schema
2	blocking: No tests for login enforcement logic	RESOLVED — `PicturePasswordLoginTestCase` added with 5 tests
3	suggestion: PR targets `release-v0.19.x`, not `develop`	UNADDRESSED
4	suggestion: Double-negation in learner role check	RESOLVED — rewritten as `if user is None or user.is_superuser or user.roles.exists()`
5	blocking: Migration 0033 missing `JSON_Schema_Validator`	RESOLVED — validator now present in migration

New findings

suggestion: Missing dataset-level test for no_password + picture_password incompatibility (see inline)
suggestion: Facility presets may conflict with ensure_compatibility (see inline)
suggestion: PR description still empty (see inline)
praise: Login enforcement tests are thorough, including edge cases like disabled flag and admin-via-token rejection

@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/test/test_datasets.py

kolibri/core/auth/models.py

kolibri/core/auth/test/test_api.py

kolibri/core/auth/api.py

LianaHarris360

Thank you for your work on this @AllanOXDi! There's a few changes that can be completely removed, and RTibblesbot caught some things that I didn't. I resolved the comments that aren't necessary to fix due to changes that will be removed.

kolibri/core/auth/models.py

kolibri/core/auth/api.py

rtibblesbot

Delta review — author addressed some prior feedback (removed learner_can_login_with_picture_password boolean field and PicturePasswordLoginTestCase), but maintainer-requested changes remain partially unaddressed.

CI: all unit tests passing, Morango integration tests and some builds still pending.

Prior-review status

#	Prior finding	Status
1	blocking: `additionalProperties: False` on syncable JSON field	RESOLVED
2	blocking: No tests for login enforcement logic	CONTESTED — maintainer says remove the login enforcement code entirely
3	suggestion: PR targets `release-v0.19.x`, not `develop`	UNADDRESSED
4	suggestion: Double-negation in learner role check	RESOLVED — rewritten as positive check
5	blocking: Migration 0033 missing `JSON_Schema_Validator`	RESOLVED
6	suggestion: Missing dataset-level test for `no_password + picture_password`	CONTESTED — maintainer says this incompatibility isn't needed
7	suggestion: Facility presets need `picture_password_settings`	Maintainer confirmed — see below
8	suggestion: PR description still empty	UNADDRESSED

New findings

blocking: CreateSessionSerializer login enforcement changes still present — maintainer requested removal (see inline)
blocking: PicturePasswordLoginTestCase tests removed but no tests remain for the token-path gating that's still in the code (see inline)
suggestion: facility_configuration_presets.json still missing picture_password_settings — maintainer confirmed this is needed
praise: Good work removing the separate boolean field and deriving the flag from picture_password_settings is not None

@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

rtibblesbot

Delta review — prior blocking findings are all resolved. Login enforcement code correctly removed per maintainer request. Two items remain.

CI: black formatting failure in serializers.py — pre-commit.ci lite is running and should auto-fix. Most test/build jobs still pending.

Prior-review status

Resolved:

additionalProperties: False on syncable JSON field — removed
No tests for login enforcement — login enforcement code removed (moved to separate task)
Double-negation in learner role check — code removed with login enforcement
Migration missing JSON_Schema_Validator — validator now present
CreateSessionSerializer login enforcement still present — removed in 1ea226dc7f
No tests for token-path gating — token-path code removed

Contested (maintainer decision):

Missing dataset-level test for no_password + picture_password incompatibility — @LianaHarris360 confirmed this incompatibility isn't needed

Unaddressed:

facility_configuration_presets.json missing picture_password_settings — @LianaHarris360 confirmed this is needed (see inline)
PR description still empty

New findings

blocking: Presets file still missing picture_password_settings (see inline)
suggestion: PR description is still placeholder text
praise: Clean removal of CreateSessionSerializer changes to keep scope focused

Comments on lines not in diff:

kolibri/core/auth/constants/facility_configuration_presets.json:11 — blocking (from prior review — UNADDRESSED): @LianaHarris360 confirmed that picture_password_settings should be added to the presets. Without it, reset_to_default_settings() won't clear picture-password when resetting to a preset where learner_can_edit_password=True (like nonformal or informal), causing ensure_compatibility to raise IncompatibleDeviceSettingError.

Add "picture_password_settings": null to each preset:

"formal": {
  "mappings": {
    ...
    "picture_password_settings": null
  }
}

@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

rtibblesbot · 2026-03-18T19:23:58Z

kolibri/core/auth/serializers.py

+        if settings is not None:
+            if settings.get("icon_style") not in ("standard", "colorful"):
+                raise serializers.ValidationError(
+                    {


suggestion: This validate method only checks picture_password_settings when it's explicitly in attrs. For a PATCH that sends only picture_password_settings (without learner_can_edit_password), the cross-field incompatibility is still caught by ensure_compatibility in save() — which is correct. But consider whether the serializer should also validate against the instance's existing learner_can_edit_password to give a cleaner error message at the serializer level rather than letting it bubble up from the model's IncompatibleDeviceSettingError. Not blocking since the model-level check prevents invalid state.

We don't need to bubble up the model so I think we can handle this better in the UI

kolibri/core/auth/test/test_api.py

rtibblesbot

Delta review — all prior blocking findings are now resolved, including the presets file which now has picture_password_settings: null in all three presets.

CI: linting passed, builds still pending.

Prior findings — resolved:

additionalProperties: False on syncable JSON field
No tests for login enforcement logic (code removed per maintainer)
Double-negation in learner role check (code removed)
Migration missing JSON_Schema_Validator
CreateSessionSerializer login enforcement still present (removed)
Presets file missing picture_password_settings (now added)

Prior findings — contested (maintainer decision):

Missing dataset-level test for no_password + picture_password incompatibility

Prior findings — unaddressed:

PR targets release-v0.19.x, not develop — is this intentional for a new feature?
PR description is still placeholder text

New findings:

blocking: Serializer validate crashes with AttributeError on non-dict input (see inline)
suggestion: PR description still empty
praise: Presets correctly include picture_password_settings: null in all three configurations
praise: Good test coverage for incompatibility, validation, and round-trip scenarios

@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-03-18T19:51:03Z

kolibri/core/auth/serializers.py


+    def validate(self, attrs):
+        settings = attrs.get("picture_password_settings")
+        if settings is not None:


blocking: If picture_password_settings is sent as a non-dict JSON value (e.g., "hello", 42, [1,2]), DRF's JSONField will accept it, but settings.get("icon_style") on line 199 will raise AttributeError — resulting in a 500 error instead of a 400 validation error.

Add a type check before accessing dict methods:

def validate(self, attrs): settings = attrs.get("picture_password_settings") if settings is not None: if not isinstance(settings, dict): raise serializers.ValidationError( {"picture_password_settings": "Must be an object or null"} ) if settings.get("icon_style") not in ("standard", "colorful"): ...

kolibri/core/auth/constants/facility_configuration_presets.json

LianaHarris360 · 2026-03-19T14:26:01Z

kolibri/core/auth/serializers.py


 class PublicFacilitySerializer(serializers.ModelSerializer):
    learner_can_login_with_no_password = serializers.SerializerMethodField()
+    learner_can_login_with_picture_password = serializers.SerializerMethodField()


Just flagging that learner_can_login_with_picture_password and get_learner_can_login_with_picture_password should be removed from this file as well.

rtibblesbot

Delta review — all prior blocking findings are resolved. CI passing.

Prior findings — resolved:

additionalProperties: False on syncable JSON field — removed
No tests for login enforcement — code removed per maintainer
Double-negation in learner role check — code removed
Migration missing JSON_Schema_Validator — validator present
CreateSessionSerializer login enforcement — removed
Presets missing picture_password_settings — added to all three presets
Serializer validate on non-dict input — isinstance(settings, dict) check handles this
PR description empty — now filled in
Maintainer flag about learner_can_login_with_picture_password / get_learner_can_login_with_picture_password — already removed (not present in codebase)

Prior findings — contested (maintainer decision):

Missing dataset-level test for no_password + picture_password incompatibility

Prior findings — unaddressed:

PR targets release-v0.19.x but issue #14351 says "Target branch: develop" and the repo default branch is develop — see inline

All acceptance criteria from #14351 are met.

@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-03-19T15:33:58Z

kolibri/core/auth/serializers.py

+                raise serializers.ValidationError(
+                    {"picture_password_settings": "show_icon_text must be a boolean"}
+                )
+        return attrs


praise: Good structural validation — checks type (isinstance) before accessing dict methods, handles both null and non-dict inputs cleanly.

rtibblesbot · 2026-03-19T15:33:58Z

kolibri/core/auth/api.py

        return FacilityUser.objects.filter(
-            dataset__learner_can_login_with_no_password=True, roles=None
+            Q(dataset__learner_can_login_with_no_password=True)
+            | Q(dataset__picture_password_settings__isnull=False),


praise: Clean use of Q OR to include picture-password learners in the username list endpoint, correctly paralleling the existing learner_can_login_with_no_password check.

rtibblesbot · 2026-03-19T15:33:58Z

kolibri/core/auth/models.py

 # '"optional":True' is obsolete but needed while we keep using an
 # old json_schema_validator version compatible with python 2.7.
 # "additionalProperties": False must be avoided for backwards compatibility
+picture_password_settings_schema = {


suggestion (from prior review — UNADDRESSED): Issue #14351 states "Target branch: develop" but this PR targets release-v0.19.x. New features (new model field + migration) typically land on the default branch. Is this intentional?

We are targeting release-v0.19.x

rtibblesbot

Delta review — all prior blocking findings remain resolved. CI passing. PR now correctly targets develop and description is filled in.

Prior findings — all resolved:

additionalProperties: False on syncable JSON field
No tests for login enforcement (code removed per maintainer)
Double-negation in learner role check (code removed)
Migration missing JSON_Schema_Validator
CreateSessionSerializer login enforcement (removed)
Presets missing picture_password_settings (added)
Serializer validate on non-dict input (isinstance check added)
PR description empty (now filled in)
PR targets release-v0.19.x (now targets develop)

Prior findings — contested (maintainer decision):

Missing dataset-level test for no_password + picture_password incompatibility

New findings:

blocking: 33 out-of-scope files from unrelated features (see inline)
praise: Clean serializer validation and comprehensive test coverage for picture-password settings

@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-03-19T19:22:50Z

kolibri/core/attendance/api.py

@@ -1,46 +1,37 @@
-import datetime
-
+from django.db import transaction


blocking: This PR includes 33 files from unrelated features — attendance tracking, content import pagination, SparklineBar, coach frontend views, i18n strings, etc. Issue #14351 explicitly scopes this to backend-only picture-password settings and states "Out of Scope: Frontend changes of any kind."

The branch has 63 commits, but only ~15 are picture-password related. The rest appear to be work from other features that were committed to this branch.

Please rebase this branch onto develop and drop the unrelated commits, or cherry-pick only the picture-password commits onto a clean branch. Out-of-scope changes bypass issue tracking and make the PR much harder to review (reviewers need to assess 44 files instead of 11).

rtibblesbot · 2026-03-19T19:22:50Z

kolibri/core/auth/serializers.py

            "preset",
        )

+    def validate(self, attrs):


praise: Solid structural validation — checks isinstance before accessing dict methods, handles both null and non-dict inputs cleanly, and the save() wrapper translates IncompatibleDeviceSettingError into a DRF ValidationError for a proper 400 response.

rtibblesbot · 2026-03-19T19:22:50Z

kolibri/core/auth/test/test_api.py

+        )
+        self.assertEqual(response.status_code, 400)
+
+    def test_learner_can_edit_password_incompatible_with_existing_picture_password(


praise: Good coverage of the reverse-direction incompatibility (picture password already enabled, then PATCH learner_can_edit_password=True). This catches regressions that a forward-only test would miss.

…re passwords instead of text passwords

…migration

…cture_password combo in ensure_compatibility

…lling back to instance values

…n login path

…ties

…s already enabled

…_settings null check

…arate task

rtibblesbot

Delta review — the 33 out-of-scope files from the prior review have been removed, but one out-of-scope model field remains.

CI pending. All acceptance criteria from #14351 are met.

Prior findings

8 prior findings unchanged (7 resolved, 1 contested by maintainer).

Partially resolved:

33 out-of-scope files (blocking) — files removed, but enable_mark_attendance field remains in models.py (see inline)

8/9 prior findings resolved. 1 re-raised below.

New findings

blocking: Out-of-scope enable_mark_attendance field (see inline)
praise: Comprehensive test coverage for picture-password settings (see inline)

@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-03-19T20:02:26Z

kolibri/core/auth/models.py

    learner_can_delete_account = models.BooleanField(default=True)
    learner_can_login_with_no_password = models.BooleanField(default=False)
    show_download_button_in_learn = models.BooleanField(default=True)
+    enable_mark_attendance = models.BooleanField(default=False)


blocking: enable_mark_attendance is not on develop, is not mentioned in issue #14351, has no migration (will fail makemigrations --check), and is not exposed in any serializer or viewset. This appears to be a leftover from the batch of out-of-scope changes flagged in the prior review. Remove this line — if this field is needed, it should be added in a separate PR with its own migration.

This happened after rebase.

rtibblesbot · 2026-03-19T20:02:26Z

kolibri/core/auth/test/test_api.py

+        }
+        self.facility.dataset.save()
+        self.client.login(username=self.admin.username, password=DUMMY_PASSWORD)
+        response = self.client.patch(


praise: Good coverage of the reverse-direction incompatibility (picture password already enabled, then PATCH learner_can_edit_password=True). This catches a real edge case that a simpler test suite would miss.

rtibblesbot

Delta review — all prior findings resolved. CI: unit tests and linting passing; builds and Morango integration tests still pending.

Prior findings

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

additionalProperties: False on syncable JSON field — RESOLVED
No tests for login enforcement — RESOLVED (code removed per maintainer)
PR targets release-v0.19.x — RESOLVED (now targets develop)
Double-negation in learner role check — RESOLVED (code removed)
Migration missing JSON_Schema_Validator — RESOLVED
Missing dataset-level test for no_password + picture_password — CONTESTED (maintainer decision)
Presets missing picture_password_settings — RESOLVED
CreateSessionSerializer login enforcement still present — RESOLVED
No tests for token-path gating — RESOLVED
Serializer validate on non-dict input — RESOLVED
PR description empty — RESOLVED
33 out-of-scope files — RESOLVED
Out-of-scope enable_mark_attendance field — RESOLVED

All acceptance criteria from #14351 are met:

picture_password_settings JSONField present with default=None
Non-null value is the source of truth for picture login enabled
Migration 0034 created with proper JSON_Schema_Validator
Field serialized and returned by FacilityDatasetViewSet, FacilityViewSet, and PublicFacilitySerializer
Tests cover: default value, incompatibility, API response round-trip, invalid icon_style, non-boolean show_icon_text

No new issues found. Clean, focused PR.

@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-03-19T20:31:25Z

kolibri/core/auth/serializers.py

+                raise serializers.ValidationError(
+                    {"picture_password_settings": "show_icon_text must be a boolean"}
+                )
+        return attrs


praise: Clean serializer validation — the isinstance(settings, dict) guard, enum check, and boolean type check cover all the structural validation needed without over-engineering.

rtibblesbot · 2026-03-19T20:31:25Z

kolibri/core/auth/test/test_api.py

+            {"learner_can_edit_password": True},
+            format="json",
+        )
+        self.assertEqual(response.status_code, 400)


praise: Good test coverage for the reverse-direction incompatibility (picture password already enabled, then PATCH learner_can_edit_password=True). This catches a real edge case that the forward-direction test alone wouldn't cover.

LianaHarris360

Logic is correct and match the intent; this is good to go, thank you Allan! I'll wait to approve & merge it, after the off-cycle patch release has been tagged.

github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: medium labels Mar 18, 2026

AllanOXDi requested review from AlexVelezLl, LianaHarris360 and rtibblesbot March 18, 2026 07:23

rtibblesbot reviewed Mar 18, 2026

View reviewed changes

rtibblesbot suggested changes Mar 18, 2026

View reviewed changes

AllanOXDi force-pushed the picture-login branch 2 times, most recently from 2d0dee5 to 55dc794 Compare March 18, 2026 08:13

rtibblesbot reviewed Mar 18, 2026

View reviewed changes

kolibri/core/auth/test/test_datasets.py Show resolved Hide resolved

kolibri/core/auth/models.py Show resolved Hide resolved

kolibri/core/auth/test/test_api.py Show resolved Hide resolved

kolibri/core/auth/api.py Outdated Show resolved Hide resolved

LianaHarris360 requested changes Mar 18, 2026

View reviewed changes

kolibri/core/auth/models.py Show resolved Hide resolved

kolibri/core/auth/models.py Show resolved Hide resolved

kolibri/core/auth/models.py Outdated Show resolved Hide resolved

kolibri/core/auth/api.py Show resolved Hide resolved

AllanOXDi force-pushed the picture-login branch from eb8545a to ecafdd1 Compare March 18, 2026 18:30

rtibblesbot suggested changes Mar 18, 2026

View reviewed changes

kolibri/core/auth/api.py Outdated Show resolved Hide resolved

kolibri/core/auth/api.py Show resolved Hide resolved

rtibblesbot suggested changes Mar 18, 2026

View reviewed changes

AllanOXDi force-pushed the picture-login branch from 66da11b to 4e6626a Compare March 18, 2026 19:33

rtibblesbot suggested changes Mar 18, 2026

View reviewed changes

LianaHarris360 reviewed Mar 19, 2026

View reviewed changes

AllanOXDi marked this pull request as ready for review March 19, 2026 15:16

rtibblesbot reviewed Mar 19, 2026

View reviewed changes

AllanOXDi changed the base branch from release-v0.19.x to develop March 19, 2026 16:41

AllanOXDi changed the base branch from develop to release-v0.19.x March 19, 2026 16:54

AllanOXDi force-pushed the picture-login branch from 0c87a95 to 28db704 Compare March 19, 2026 19:04

github-actions bot added the SIZE: very large label Mar 19, 2026

AllanOXDi changed the base branch from release-v0.19.x to develop March 19, 2026 19:04

rtibblesbot suggested changes Mar 19, 2026

View reviewed changes

AllanOXDi force-pushed the picture-login branch from 28db704 to 8b3e068 Compare March 19, 2026 19:43

github-actions bot added the SIZE: large label Mar 19, 2026

AllanOXDi and others added 17 commits March 19, 2026 22:55

Add new authentication feature allowing learners to login using pictu…

14eae2a

…re passwords instead of text passwords

Remove unused django.db.models.deletion import from picture-password …

8547c69

…migration

Add picture_password_settings_schema and attach JSON validator to field

d7b116a

Reject learner_can_login_with_no_password + learner_can_login_with_pi…

a4c90ab

…cture_password combo in ensure_compatibility

Fix FacilityDatasetSerializer.validate to handle PATCH requests by fa…

fb8892f

…lling back to instance values

Include picture-password learners in FacilityUsernameViewSet queryset

a0323e7

Enforce facility flag and learner role check on the user_id/auth_toke…

04cf9cb

…n login path

Block username+password login for learners in picture-password facili…

2fb266f

…ties

Add regression test for PATCH incompatibility when picture-password i…

edcfe42

…s already enabled

migrations

df7513e

add PicturePasswordLoginTestCase tests

dc13824

Replace learner_can_login_with_picture_password with picture_password…

accc8f2

…_settings null check

Remove CreateSessionSerializer picture-password changes, moved to sep…

e39571a

…arate task

validate picture passwordsettings

d91b1d7

add picture_password_settings to facility configuration presets

21ce64e

[pre-commit.ci lite] apply automatic fixes

b266f04

remove learner_can_login_with_picture_password field

5313f45

AllanOXDi force-pushed the picture-login branch from 8b3e068 to 5313f45 Compare March 19, 2026 19:56

AllanOXDi requested a review from LianaHarris360 March 19, 2026 19:57

rerun migrations

4766937

AllanOXDi force-pushed the picture-login branch from 753ef81 to 4766937 Compare March 19, 2026 20:00

rtibblesbot suggested changes Mar 19, 2026

View reviewed changes

Remove enable_mark_attendance from FacilityDataset model

4003c51

rtibblesbot approved these changes Mar 19, 2026

View reviewed changes

LianaHarris360 reviewed Mar 19, 2026

View reviewed changes

		@@ -1,46 +1,37 @@
		import datetime

		from django.db import transaction

Conversation

AllanOXDi commented Mar 18, 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!

Uh oh!

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior-review status

New findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior-review status

New findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LianaHarris360 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior-review status

New findings

Uh oh!

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior-review status

New findings

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

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

AllanOXDi commented Mar 18, 2026 •

edited

Loading

github-actions bot commented Mar 18, 2026 •

edited

Loading