Catalog: stop offering a refused Save for service users (5217 stack 6/9)

[nl0]

Description

In Admin → Users, the roles dialog and the Role column offered an edit that
cannot happen. Both gated on isRoleAssignmentDisabled alone, so a service
user with that flag unset got a dialog headed "Assign roles to …", an assignable
RoleSelect, and a live Save — a Save the registry refuses. The Role column's
tooltip invited the click on the same basis.

Both now derive one readOnly from the pair. The registry couples these flags,
but nothing here depends on that holding: refusing a service user is correct on
its own terms.

Why this is a real fix and not a claim about an unreachable state. It is worth
stating explicitly, because it was challenged and survived: before this change,
the dialog title, RoleSelect.nonAssignable and the Save action all gated on
isRoleAssignmentDisabled alone. A Save therefore was offered for any user
with isService && !isRoleAssignmentDisabled. That combination is what the fix
closes, and the failure it produces is the worst kind — the Save submits unchanged
values and reports success, so the admin is told a change landed that did not.

Review findings addressed

• f49 — the Role column's half of the change was covered by a test; the
  dialog's was not, and the dialog is where the refused Save lived. EditRoles
  was unexported and never rendered by a test. It is exported for testing now, and
  both flags are driven through it.

Recorded:

• f43 — checked and refuted. The disclosure names two sites and the Role
  column did change too; both are in this PR.
• f53 — checked and refuted, and the refutation's reasoning is the paragraph
  above.

Verification

cd catalog && npx vitest run app/containers/Admin/UsersAndRoles

18 tests. The three new ones render EditRoles directly; they were checked
against a version gated on isRoleAssignmentDisabled alone, which fails the
service-user case.

Position in the stack

PR 6 of 9, based on
stack/5217-5-admin-disabled-switch-reason.

Part of the split of #5217 asked for in
f27. PR 5
edits the same file; the two sets of hunks are disjoint and share no symbol, so
either order is correct and neither depends on the other's behaviour. They are
stacked rather than combined so that the accessibility fix and the permission fix
can be reviewed, and reverted, separately.

TODO

• Unit tests
• Security: Confirm that this change meets security best practices and does not violate the security model
• Open: Confirm that this change doesn't break the Open variant
• Changelog entry

🤖 Generated with Claude Code

Greptile Summary

This PR makes service-user role management consistently read-only across the Admin Users role column and dialog.

• Uses one read-only predicate for dialog copy, role selection, actions, and the Role-column tooltip.
• Adds direct coverage for service, SSO-managed, and ordinary users.
• Documents the corrected behavior in the Catalog changelog.

Confidence Score: 5/5

The PR appears safe to merge with no actionable regressions identified.

The role column and dialog now share the same read-only condition, and the affected service-user, SSO-managed-user, and ordinary-user branches have focused render coverage.

Important Files Changed

Filename	Overview
catalog/app/containers/Admin/UsersAndRoles/Users.tsx	Consistently applies the combined service-user and role-assignment-disabled predicate to every affected role-editing affordance.
catalog/app/containers/Admin/UsersAndRoles/Users.spec.tsx	Adds focused coverage proving both read-only cases suppress Save while ordinary users retain role editing.
catalog/CHANGELOG.md	Accurately records the service-user role-management UI fix.

_{Reviews (1): Last reviewed commit: "docs(changelog): entry for the service-u..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 35.99%. Comparing base (a633a1c) to head (dac681d).

Additional details and impacted files

@@                              Coverage Diff                              @@
##           stack/5217-5-admin-disabled-switch-reason    #5264      +/-   ##
=============================================================================
+ Coverage                                      35.70%   35.99%   +0.29%     
=============================================================================
  Files                                            741      741              
  Lines                                          23823    23829       +6     
  Branches                                        6433     6434       +1     
=============================================================================
+ Hits                                            8505     8577      +72     
+ Misses                                         13577    13520      -57     
+ Partials                                        1741     1732       -9     

Flag	Coverage Δ
catalog	35.99% <100.00%> (+0.29%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nl0]

Review pass on this layer

Local code review at high effort against this PR's own base (stack/5217-5-admin-disabled-switch-reason), not master, so only this layer's changes were in scope.

Greptile has reviewed this content — its pass is the summary block in the description (confidence 5/5, no actionable regressions), with no review threads or inline comments, so there was nothing to reply to. Its last-reviewed commit predates the rebase, so I diffed the two: the reviewed commit's layer payload is line-for-line identical to head, i.e. it did review exactly what is here.

No correctness bug was found in the layer. readOnly = isRoleAssignmentDisabled || isService is the right predicate at all three sites it feeds, and the reason string stays correct in all four flag combinations. Five quality findings; four addressed, one declined.

Accepted

1 + 2 — the read-only predicate was duplicated, and the reason was derived separately (caf854d)

The expression was written out twice, in EditRoles and in the RoleDisplay that opens it, each carrying a comment pointing at the other. Worse, nonAssignableReason was still derived from isService alone, so a third cause joining readOnly would have disabled the select under the SSO copy — "Roles are assigned via role mapping", which would simply be false. Both now come from one whyRoleReadOnly(user) returning the cause, with readOnly being whether there is one. This mirrors whyEnabledDisabled/whyAdminDisabled directly above, which exist for exactly this reason. No behaviour change: the resolver agrees with the old code in all four combinations.

5 — the module stubs in the spec were dishonest (1e1ee82)

vi.mock('utils/GraphQL', () => ({ useMutation: ... })) replaced the whole module with one export for every test in the file. Users.tsx also reaches for GQL.useQueryS, so the next test here to render anything touching it would have failed with undefined is not a function and no hint the mock caused it. Both stubs now spread the actual module, as the specs stubbing react-router-dom already do.

4 (in part) — the changelog over-claimed (fabdaf1)

The entry said the change reads read-only "end to end". The Role column's signal is the title MUI seeds on a Clickable, which is mouse-only, so "end to end" was too strong. The entry now states what the change does.

Also added

The service-user helper copy ("Roles for this service user are managed by the stack.") only became reachable in this layer — before it, the select was not disabled for that flag combination — and nothing pinned it. Both read-only cases now assert their cause, so the copy cannot drift from the predicate that selects it.

Declined

3 — add a readOnly guard inside onSubmit. Suggested as defence in depth. Declined: there is no path to it (with nonAssignable set, every control in RoleSelect is disabled and no submit button renders, so there is nothing to Enter-submit), and an early return from a final-form onSubmit resolves the submission as success, so the guard would introduce a subtly wrong branch that no test can exercise. The invariant is better served by the single resolver above.

4 (in full) — make the Role column's read-only state non-mouse-only. Real, but pre-existing and wider than this layer: Clickable is a bare span with onClick and no role/tabIndex, so it is unreachable by keyboard for editable users too. Fixing it changes the Role column for everyone, which does not belong in a permission fix. Worth a follow-up — EmailDisplay twelve lines up handles the same "not editable for service users" state by dropping the affordance entirely, which is probably the shape the fix should take.

Verification

npx vitest run app/containers/Admin/UsersAndRoles — 18 passed; tsc --noEmit clean; oxfmt --check and oxlint clean.

Both new assertions were checked to be load-bearing rather than decorative: reverting the resolver to the single flag fails exactly the two service-user cases, and mapping a service user to the SSO reason fails the new copy assertion.

Layers 7–9 were rebased onto this branch in order and force-pushed with lease; each of #5265, #5266 and #5267 still shows only its own layer (5f/+200−20, 3f/+76−9, 15f/+524−45, unchanged).