feat: support status field on user create, document invite caveat - #1665
feat: support status field on user create, document invite caveat#1665ruvenzx wants to merge 3 commits into
Conversation
Adds an optional `status` parameter to `mgmt.user.create()` (sync and async), matching the status field already supported by the create-user API and by `patch()`. Also documents in the README that `invite` only controls whether an invitation message is sent, not the resulting user status.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment strategy prioritizes comprehensive coverage across all changed file categories: (1) Core implementation reviewers (dorsha, LioriE, asafshen) with complementary strengths in the user management module and base classes; (2) Testing verification (omercnet) to ensure the status parameter is properly tested; and (3) Documentation review (chris4490) for the README updates. This balanced team ensures the feature is correctly implemented across sync/async paths, thoroughly tested, and well-documented.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds an optional status to mgmt.user.create() (sync + async), threads it through _compose_create_body, and documents the invite vs. status distinction in the README.
Good bones! status is a trailing keyword-default param so the 4 other positional _compose_create_body call sites are unaffected, sync/async are identical, and the parametrized client_factory fixture covers both modes.
Sniffed out 2 issues:
- 2 🟢 LOW:
invite()still can't set status (the exact case the new README note describes); status allowlist now duplicated 6x
See inline comments for details. Woof!
Declared coverage: FULL — 5/5 changed files reviewed.
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)descope/management/user.py
descope/management/user_async.py
descope/management/_user_base.py
|
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
The only change since the last review is de4bf679 (ruff-format collapsing a line in tests/management/test_user.py) — no logic changed.
Both prior LOW findings (duplicated status allowlist, invite() missing status) are unchanged and left open in their original threads. No new issues. Good bones, just a formatting pass!
⚠️ Review follow-up issue: Unresolved prior review threads: PRRT_kwDOHT_eGc6ZxU3a, PRRT_kwDOHT_eGc6ZxU3f.
Declared coverage: PARTIAL — 1/5 changed files reviewed.
create(), patch(), and patch_batch() (sync + async) each repeated the same status allowlist and error message. Moved it into a single UserBase._validate_status() so adding a new status only means editing one place.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
This delta (ada8d40d) refactors the status-validation logic into a shared UserBase._validate_status() + VALID_USER_STATUSES constant, replacing 6 copies of the same inline check across create/patch/patch_batch (sync + async).
Both prior LOW threads are resolved: the duplication was fixed exactly as suggested, and error-message substrings still match existing test assertions. No new issues found. Good bones, tail's wagging!
Declared coverage: PARTIAL — 3/5 changed files reviewed.
2 participants
Related Issues
Fixes https://github.com/descope/etc/issues/17782
Description
Adds an optional
statusparameter tomgmt.user.create()(sync and async), matching the status field already supported by the create-user API and bypatch(). Also documents in the README thatinviteonly controls whether an invitation message is sent, not the resulting user status.Must