Skip to content

Move user_auth onto the new resource methods - #15207

Open
rtibblesbot wants to merge 2 commits into
learningequality:developfrom
rtibblesbot:issue-15067-ee474c
Open

Move user_auth onto the new resource methods#15207
rtibblesbot wants to merge 2 commits into
learningequality:developfrom
rtibblesbot:issue-15067-ee474c

Conversation

@rtibblesbot

@rtibblesbot rtibblesbot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Both facilityusername reads in SignInPage move to list.
  • Sign-up POSTs through create, so the payload is no longer nested under data.
  • The deprovision queue clear moves to TaskResource.clearAll_v2.

References

Fixes #15067. Recipe: #15057. New methods: #15056.

Reviewer guidance

Code review

  • Repeated identical reads now hit the network; the legacy Collection cache was module-level.
  • create resolves the response body, not the body merged over the payload. The sign-up .then reads picture_password, which is response-only.
  • SignInPage.spec.js mocks kolibri/client rather than the resource: only the boundary proves facility still leaves as a query param, not a getParams blob.

QA steps

No UI changes — sign-in and sign-up should behave as on develop. Rig: a facility with password-free sign-in and account creation enabled (Facility > Settings), 16 users or fewer, three sharing a prefix (alice, alina, alvaro); a second facility with more; the APK for step 2.

  1. Suggestions. Type ali on the small facility's sign-in page: the three usernames drop down. Select by mouse, then by arrow keys — both sign in. Backspacing to al empties it, Escape hides it.
  2. App context. In the Android app the small facility lists its users instead of a username box, the large one shows the box. Change facility between them shows each facility's own users.
  3. Sign up. Create an account with gender and birth year unset: the learner lands on Learn, signed in. An existing username is refused on step one.

Unverified: the deprovision clear tasks path renders only on a device with no usable superuser.

State Screenshot
Suggestions, search=ali suggestions
App-context user list, max_results=16 users list

AI usage

Claude Code applied the recipe and wrote the tests. Verified with the user_auth Jest suite, prek, and the QA above.


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

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Rebased onto the target branch and resolved any conflicts
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks

Deviations from the issue spec

Criterion Why not
Reactive reads use useRetrieve / useList where possible SignInPage mixes setup() with Options API; per review, useList there splits state across both halves. Both reads use list() instead.
@rtibblesbot

🟡 Waiting for feedback

Last updated: 2026-08-24 16:09 UTC

@github-actions github-actions Bot added APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: medium labels Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

@rtibblesbot
rtibblesbot force-pushed the issue-15067-ee474c branch 3 times, most recently from b684450 to e19868c Compare August 17, 2026 20:12
@rtibblesbot
rtibblesbot marked this pull request as ready for review August 17, 2026 21:52
@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Three red checks, one cause. Build WHL failed in make distMakefile:189: staticdeps-cext on a truncated wheel download:

ERROR: Could not install packages due to an OSError: ('Connection broken: IncompleteRead(220864 bytes read, 10598 more expected)')
Installation failed for package cffi-1.15.1-cp310-musllinux_1_1_x86_64.

Both WHL smoke tests failures are the needs gate on that job (exit 1, 2s) — not independent.

Transient PyPI/piwheels truncation rather than anything on this branch:

  • The diff is five .vue/.spec.js files under kolibri/plugins/user_auth/frontend/ — no Python, requirements, or build inputs.
  • The two failed runs truncate different platform wheels (musllinux_1_1_x86_64, manylinux_2_17_s390x, musllinux_1_1_i686) at identical byte counts — a CDN chunk boundary, not a poisoned .cext_cache, which would fail every run.
  • An unrelated branch's WHL build went green at 20:41 UTC, after both failures here.

No code change to push, and re-running the job needs admin rights this account lacks.


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

@AlexVelezLl AlexVelezLl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little request, please also rebase against develop again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that code is split into setup code and options API code, the useList usage has make this a bit harder to follow. Could you please revert the usage of useList here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted — created() and setSuggestions() are back in the Options API, with fetchCollection({ getParams }) swapped for list(params). Net diff on this file is now those two calls.

Swept the branch for the same pattern: git grep -n "useList\|useFetch" kolibri/plugins/user_auth/ — those two were the only composable-in-setup() conversions; the other three call sites (SignUpPage, DeviceUnusableMessage) were plain method renames.

Kept the spec on the kolibri/client mock rather than the resource module: it is what proves facility still leaves as a query param instead of a getParams blob.

Rebased onto develop (479b7962c5).

@AlexVelezLl

Copy link
Copy Markdown
Member

Also, @rtibblesbot, please add QA steps for things that QA testing should check for this PR. Similar to other API Resource migration PRs.

@rtibblesbot
rtibblesbot force-pushed the issue-15067-ee474c branch 3 times, most recently from 16fbf82 to b4f99f2 Compare August 24, 2026 14:50
@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Added ### QA steps to the body, split out from ### Code review: one step per migrated user-facing call site — the suggestions dropdown, the app-context user list, sign-up — plus the facility rig each needs.

Swept the branch's five call sites for flows the section misses: SignInPage's two reads are steps 1–2, SignUpPage is step 3, and DeviceUnusableMessage's clearAll_v2 is called out as unverifiable — it renders only on a device with no usable superuser. Nothing user-facing on this branch is left without a step.

Also force-pushed a history fix: the last self-review cycle left proceedToSignupSubmit hoisted out of its describe in the working tree but not in any commit, so the pushed tip called a helper it never defined. Folded back into the commit that moved it. pnpm test-jest --testPathPatterns user_auth — 14 suites, 140 tests green. Branch is on develop @ 479b7962c5.

rtibblesbot and others added 2 commits August 24, 2026 07:58
`create` resolves the server's response body where `saveModel` resolved it
merged over the POST payload; the sign-up `.then` reads `picture_password`,
which is read-only on `FacilityUserSerializer` and so only ever in the response.

`clearAll_v2` resolves `response.data` rather than the whole axios response;
the deprovision call site awaits and discards it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The spec moves its mock from the resource module to `kolibri/client`: the
boundary is what proves `facility` still goes out as a query param rather than
a `getParams` blob, which `list` would send as an unfiltered request.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@AlexVelezLl AlexVelezLl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rtibblesbot! Code changes look good, manual QA steps are clear and correct, ready for qa review :)

@AlexVelezLl AlexVelezLl self-assigned this Aug 24, 2026
@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Holding off on changes while this PR is in 🧪 Needs QA — the branch will not move while it is being tested. Questions still get answered.

Change the board status when QA is done and I'll pick up any outstanding review feedback, CI failures and conflicts on the next cycle.


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

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Rebased onto the target branch and resolved any conflicts
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate user_auth to the new Resource-layer methods

2 participants