Assign picture password when user's last role is deleted#14566
Assign picture password when user's last role is deleted#14566LianaHarris360 merged 2 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
rtibblesbot
force-pushed
the
issue-14556-65c860
branch
4 times, most recently
from
384d2f9 to
a543c38
Compare
LianaHarris360
left a comment
LianaHarris360
left a comment
There was a problem hiding this comment.
The code changes align with the issue specced, but an unrelated change to AllPasswordsPage.spec.js should be removed, and a regression test for when a user's last role is deleted in a facility where picture login isn't enable should be added for more thorough testing.
| import { picturePasswordStrings } from 'kolibri-common/strings/picturePasswords'; | ||
| import AllPasswordsPage from '../AllPasswordsPage.vue'; | ||
|
|
||
| jest.mock('kolibri/composables/useNav'); |
There was a problem hiding this comment.
This test file does not need to be updated with this mock for this change, this should be removed.
There was a problem hiding this comment.
Done — removed the line and dropped the entire AllPasswordsPage spec commit. The file is no longer in the diff.
|
|
||
| user.refresh_from_db() | ||
| self.assertIsNone(user.picture_password) | ||
|
|
There was a problem hiding this comment.
It would be worth it to include a test case for when a user's last role is deleted in a facility where picture_password_settings is None to confirm that they are not assigned a picture_password.
There was a problem hiding this comment.
Added — new test test_last_role_deleted_no_assignment_when_picture_password_settings_null covers this case. It temporarily disables picture password settings, deletes the user's last role, and asserts picture_password remains None.
rtibblesbot
force-pushed
the
issue-14556-65c860
branch
from
a543c38 to
7ddcb42
Compare
Move the no-picture-password-settings test out of RoleDeletePicturePasswordTestCase into a separate class with its own fixture data, avoiding the need to temporarily mutate shared state and restore it in a finally block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
After a user's last role is deleted (reverting them to learner status), if picture login is enabled for their facility and they don't yet have a
picture_password, one is now automatically assigned. This mirrors the assignment that happens at new-learner creation and the clearing that happens at role creation (#14549).The change adds a post-delete check to
Role.delete().NoAvailableSequencesis caught silently (capacity-exceeded case), following the same pattern asFacilityUserSerializer.create(). Inline imports are used inside the method body to avoid a circular import (picture_passwords.pyimportsFacility/FacilityUserfrommodels.py).References
Closes #14556
Related: #14549 (clears picture password on role creation — this PR handles the inverse path)
Reviewer guidance
Five new unit tests in
RoleDeletePicturePasswordTestCase(kolibri/core/auth/test/test_api.py) cover all acceptance criteria: last role deleted assigns password, remaining role prevents assignment, existing password not overwritten, at learner limit no assignment/no exception,NoAvailableSequencescaught silently.Area worth checking: the inline imports inside
Role.delete()— this is an intentional workaround for the circular import described above.AI usage
Implemented with Claude Code following a pre-written implementation plan using TDD. Tests were written and confirmed failing before the implementation was added. I reviewed the code, ran the full auth test suite (
pytest kolibri/core/auth/test/), and verified all tests pass with no regressions.@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?