[tabcmd] refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role - #1812
[tabcmd] refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role#1812jacalata wants to merge 10 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? |
…servation; add find_by_name Fixes for UserItem.CSVImport (issue #1809): - MAX=8 (was 7=AUTH index): 8-column lines with auth type no longer rejected as "too many columns" - create_user_from_line no longer lowercases the whole line before splitting — username case is preserved - _validate_import_line_or_throw normalizes license/admin/publisher to lowercase and auth to canonical form before comparison, so 'Viewer', 'Creator', 'SAML', 'tableauidwithmfa' etc. are all accepted - Add TableauIDWithMFA to valid auth values in validation (was missing) - 5 new tests covering each fix Add QuerysetEndpoint.find_by_name(name) (issue #1810): - Thin wrapper over .filter(name=name) returning a list - Available on all content-item endpoints (workbooks, datasources, views, users, projects, groups) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates user CSV generation/import helpers to support an additional “auth” column and centralizes site-role-to-CSV column derivation into a shared helper, with corresponding test updates.
Changes:
- Add
authcolumn tocreate_users_csv()output and update tests accordingly. - Refactor site-role decomposition logic into
UserItem.CSVImport._decompose_site_role(). - Fix
create_from_file()CSV file validation and remove debug prints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/test_user.py | Updates CSV expectations and adds coverage for unsupported legacy roles producing license="Invalid". |
| tableauserverclient/server/endpoint/users_endpoint.py | Updates CSV validation logic and extends generated CSV rows with auth setting. |
| tableauserverclient/models/user_item.py | Introduces _decompose_site_role() mapping for consistent CSV export semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jacalata
changed the title
…test Two related changes to make `_evaluate_site_role` and `_decompose_site_role` a real inverse pair: - `_evaluate_site_role` now accepts "1", "true", and "yes" for the publisher column (and, symmetrically, treats anything else as "no"). Previously only "yes" was accepted, which meant `_decompose_site_role` emitting "1" for Creator/ExplorerCanPublish would round-trip as Explorer. The set of publisher values mirrors what `_valid_attributes[publisher]` already documents as legal (["yes", "true", "1", "no", "false", "0"]), so this brings the two code paths in agreement. - Added a parametrized `test_decompose_then_evaluate_round_trips` covering every entry in `_role_map` plus the two documented label asymmetries (ServerAdministrator -> SiteAdministrator on the way back; legacy roles Publisher/Interactor/ReadOnly/SiteAdministrator folded into their modern equivalents). If either function drifts, a specific input names the broken case rather than the whole loop dying on the first mismatch. - `_decompose_site_role` docstring updated with a Round-trip note naming the two intentional asymmetries so readers do not have to reason about them from the mapping table. Full suite still passes: 884 passed, 1 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Strip five non-ASCII characters (arrow + em-dashes) from docstrings and comments; project convention is ASCII-only. - create_user_from_line now passes an unknown auth value through unchanged (previously silently .get'd to None). _validate_import_ line_or_throw on the same input raises, so the two entry points no longer disagree on what counts as valid. Not addressed here (per fresh-eyes review, non-blocking): - Deprecated create_from_file's "csv" substring check remains loose (matches report.csv.bak, rejects USERS.CSV); pre-existing on a @deprecated method, out of scope for this refactor. - PR body test-plan checkbox that's actually complete: tick before un-drafting. 59 user tests pass.
…ole-decompose # Conflicts: # tableauserverclient/models/user_item.py # test/test_user_model.py
Post-merge follow-up on #1811 -- the strict behavior it introduced (create_user_from_line raises ValueError on any auth value not in _AUTH_CANONICAL) would block CSV imports against newer servers as soon as Tableau ships an auth type TSC's hardcoded list doesn't yet know about. This is the same category of stale-list problem the _set_values enum-guard bypass exists to avoid on the server-parse path. Both entry points now warn and pass the value through: - create_user_from_line: unknown values raise a UserWarning naming the value and known set, then get assigned to auth_setting as-is. If the value really is a typo, the server rejects the row when the request posts -- a slightly-later error, but the import stays possible against forward-compatible servers. - _validate_import_line_or_throw: same shape. Skips the allowlist check for the AUTH column when the value isn't in _AUTH_CANONICAL, so validate_file_for_import doesn't return the row as invalid. Server-version-aware validation would be the cleaner long-term fix here (and for the enum-guard bypass in _set_values); noted for planning, not filing an issue. Tests updated: two former "raises ValueError" cases now assert pytest.warns(UserWarning) and confirm the raw value round-trips onto UserItem.auth_setting.
Inadvertently pulled the password-column mask (safe_value = "***" if column == PASS else value) into this PR while restructuring the validation loop for the warn-on-unknown-auth change. The mask is #1862's core content and shouldn't sneak in through the site-role decompose PR -- reviewers on either PR would see mysterious overlap. #1862's full feature (log mask + INFO->DEBUG downgrade + _redact_password_column helper for invalid_lines sanitization) stays where it belongs, on jac/csv-import-privacy. The `column = ColumnType(i)` local rename stays because it's used by the log line's `{column.name}` format and by the AUTH branch's comparison; that's plain cleanup and doesn't overlap with #1862.
Motivation
create_users_csvhad an ad-hoc if/elif block that mapped site rolesback to CSV columns. It's the inverse of the existing
UserItem.CSVImport._evaluate_site_role, but sat in a different modulewhere nobody looking at the encoder would find the decoder. Moving it
into
CSVImportputs both directions in one place.Also picked up three pre-existing bugs in the decomposition path along
the way (see Behavior change).
Behavior change
create_users_csvnow emits three fields correctly that it previouslyemitted wrongly:
ExplorerCanPublishlicense="ExplorerCanPublish"(not a valid CSV license value)("Explorer", "None", "1")SiteAdministrator(legacy)license=""viastr.replace(...)("Explorer", "Site", "1")admin_level=""admin_level="None"(explicit CSV spec value)The
""->"None"change is cosmetic on the server side (bothaccepted) but is what
_evaluate_site_roleexpects on the input side,so a round-trip through the CSV now composes cleanly.
Unknown-auth handling (post-#1811 rebase adjustment):
create_user_from_lineand_validate_import_line_or_thrownowwarnings.warn(UserWarning)on an auth value that isn't in_AUTH_CANONICALand pass the raw value through, rather than raisingValueError. TSC's hardcoded canonical set lags server-side auth-typeadditions; refusing would block CSV imports against newer servers as
soon as Tableau ships a new auth type. If the value really is a typo,
the server rejects the row when the request posts. Same category of
stale-list problem the
_set_valuesenum-guard bypass exists toavoid on the server-parse path.
Test plan
pytest test/test_user.py test/test_user_model.py— 63 passedtest_create_users_csvupdated for the"None"and auth-columnassertions
_evaluate_site_role(*_decompose_site_role(role)) == roleacross every current-model role -- added as
test_decompose_then_evaluate_round_tripsparametrized over thefull role set (with the two documented asymmetries called out
explicitly:
ServerAdministrator->SiteAdministratorlabel,legacy-role fold to modern equivalents)
test_create_user_with_unknown_auth_passes_through_with_warningand
test_validate_import_line_warns_on_unknown_auth🤖 Generated with Claude Code