refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role - #1812
refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role#1812jacalata wants to merge 4 commits into
Conversation
Correctness: silent behavior change for several
|
| Role | Old CSV output (license, admin, publish) | New CSV output |
|---|---|---|
Interactor |
("Interactor", "", 1) |
("Explorer", "None", "0") |
Publisher |
("Publisher", "", 1) |
("Explorer", "None", "1") |
ReadOnly |
("ReadOnly", "", 0) |
("Viewer", "None", "0") |
UnlicensedWithPublish |
("UnlicensedWithPublish", "", 1) |
("Unlicensed", "None", "0") — falls into the .get() default, silently drops publish |
ViewerWithPublish |
("ViewerWithPublish", "", 1) |
("Unlicensed", "None", "0") — same silent drop |
Guest |
("Guest", "", 0) |
("Unlicensed", "None", "0") |
SupportUser |
("SupportUser", "", 0) |
("Unlicensed", "None", "0") |
Some of these (Interactor, Publisher, ReadOnly) look like reasonable improvements — their old outputs already emitted invalid license strings, so mapping them to real license values is a fix. But UnlicensedWithPublish and ViewerWithPublish look like a real regression: the old code at least preserved publish=1 even though license was garbage. The new _role_map.get(..., default) silently coerces both to Unlicensed/publish=0. Any caller using site_role = UserItem.Roles.ViewerWithPublish will now get a user provisioned as fully unlicensed instead of a licensed publisher, with no error or warning.
Suggestion: add explicit entries for Guest, SupportUser, UnlicensedWithPublish, and ViewerWithPublish to _role_map (matching whatever the CSV import spec actually expects for these), or if they're genuinely unsupported by CSV import, raise/log rather than silently defaulting to Unlicensed. At minimum, worth calling out in the PR description that these four roles' behavior is changing — two of them silently and detrimentally.
|
@jacalata take a look at the consistency check that claude called out - does that need to be fixed/improved? |
d3376c9 to
948e382
Compare
…ion check and debug prints
- create_users_csv was producing 7-column CSV, silently dropping
auth_setting; bulk_add roundtrip would lose auth type
- create_from_file extension check used filepath.find("csv") which
evaluates as falsy only when "csv" is at index 0, letting all other
paths through; fixed to "csv" not in filepath
- remove two debug print() calls left in create_from_file
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…th _evaluate_site_role
Moves the ad-hoc site role → (license, admin_level, publish) logic from
create_users_csv into UserItem.CSVImport._decompose_site_role, making it
the explicit inverse of _evaluate_site_role.
Also fixes pre-existing bugs in the decomposition:
- ExplorerCanPublish was emitted as license="ExplorerCanPublish" (not a
valid CSV license value); now correctly "Explorer" with publish=1
- SiteAdministrator (legacy role) was emitting license="" via
str.replace; now maps to ("Explorer", "Site", "1")
- Non-admin roles now emit admin_level="None" (explicit CSV spec value)
rather than "" (empty string); both are accepted by the server but
"None" is consistent with the spec and _evaluate_site_role input
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6402e1d to
7d4522d
Compare
…ensed"
Legacy values in UserItem.Roles (UnlicensedWithPublish, ViewerWithPublish,
Guest, SupportUser) have never been accepted by the server-side CSV license
parser (workgroup: CsvLicenseRoleTypeConverter). The initial _decompose_site_role
default of ("Unlicensed", "None", "0") silently coerced these to a valid but
semantically wrong Unlicensed user, replacing an old server-side per-row
rejection with silent success.
Default to license="Invalid" instead so the server continues to reject rows
with USER_CSV_INVALID_LICENSE, preserving the pre-refactor observable
behavior for callers who inspect job results for per-row failures. Batch
resilience is unaffected: bad rows fail, good rows succeed, no client-side
throw takes down the whole bulk_add call.
Follow-up: deprecate UnlicensedWithPublish/ViewerWithPublish from
UserItem.Roles (never worked on any code path); see forthcoming issue.
|
Good question. Several of the old values were actually broken already, and I was under the impression that the server was returning a new Unlicensed user. However it turns out it was throwing an error and not creating an account at all, so Claude was correct that we were changing from an error to a silent not-quite-as-expected success. so, the old behavior: The server's parser ( Fix: unmapped roles now emit The legacy Roles values ( |
The role map handles legacy UserItem.Roles values in two ways depending on whether the server has a modern equivalent. SiteAdministrator, Publisher, Interactor, and ReadOnly map to their current-model equivalents; UnlicensedWithPublish, ViewerWithPublish, Guest, and SupportUser fall to license="Invalid" so the server rejects the row. Reviewers otherwise read the code and see "why does Publisher get mapped but Guest doesn't?" - the answer is server behavior, not arbitrary choice. Docstring now says so. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Stacks on #1811.
Moves the ad-hoc site role → CSV columns logic from
create_users_csvintoUserItem.CSVImport._decompose_site_role, making it the explicit inverse of_evaluate_site_role. Both encode/decode functions now live together inCSVImport.What changed
New method:
UserItem.CSVImport._decompose_site_role(site_role) -> (license, admin_level, publish)Replaces the if/elif block in
create_users_csvwith a lookup table.create_users_csvbecomes:Pre-existing bugs fixed in the decomposition:
ExplorerCanPublishwas emitted aslicense="ExplorerCanPublish"— not a valid CSV license value; now("Explorer", "None", "1")SiteAdministrator(legacy role) was emittinglicense=""viastr.replace("SiteAdministrator", ""); now maps to("Explorer", "Site", "1")admin_level="None"(explicit CSV spec value) rather than""— both accepted by the server but"None"is consistent with the spec and what_evaluate_site_roleexpects as inputTest updated:
test_create_users_csvnow asserts"None"for non-admin roles (previously asserted"") and verifies the auth column.Test plan
pytest test/test_user.py test/test_user_model.py— 46 passed🤖 Generated with Claude Code