Skip to content

fix: remove the unassignable Spaces administrator role - #1391

Merged
igboyes merged 2 commits into
mainfrom
igboyes/vir-2977-drop-the-spaces-administrator-role
Aug 14, 2026
Merged

fix: remove the unassignable Spaces administrator role#1391
igboyes merged 2 commits into
mainfrom
igboyes/vir-2977-drop-the-spaces-administrator-role

Conversation

@igboyes

@igboyes igboyes commented Aug 14, 2026

Copy link
Copy Markdown
Member

Summary

  • The role picker offered spaces, but assigning it 500s: Python's migration 997cf9a66f10 remapped those rows to base and added an administrator_role_valid CHECK permitting only full, settings, users and base. Dropped it from AdministratorRoleName, the level map, ADMINISTRATOR_ROLES and the request validator.
  • That same migration dropped the administratorrole type, so the Drizzle mirror was describing a type Postgres no longer has. users.administrator_role is now text().$type<AdministratorRoleName>(), matching jobs.state.
  • Fixed two comments citing deleted Python code, and dropped the Free Spaces line from the base role description to match Python's current docstring.

Python dropped the role in migration 997cf9a66f10, which remapped existing
rows to base, converted administrator_role to text and dropped the
administratorrole type. The role picker still offered it and assigning it
failed against the administrator_role_valid CHECK constraint.

The Drizzle mirror declared a pgEnum for the dropped type; it is now text
plus $type, matching jobs.state.
@linear-code

linear-code Bot commented Aug 14, 2026

Copy link
Copy Markdown

VIR-2977

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The allowed administrator role values are duplicated across the contracts type, Zod schema, ADMINISTRATOR_ROLES, and test fakes; consider centralizing them from a single source (e.g., the AdministratorRoleName union) to reduce the risk of future divergence.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The allowed administrator role values are duplicated across the contracts type, Zod schema, ADMINISTRATOR_ROLES, and test fakes; consider centralizing them from a single source (e.g., the AdministratorRoleName union) to reduce the risk of future divergence.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Matches PERMISSION_NAMES in permissions.ts, so the request validator's enum
cannot drift from the union.
@igboyes

igboyes commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

Applied in 8b7e957: AdministratorRoleName is now derived from an ADMINISTRATOR_ROLE_NAMES tuple, which the request validator's z.enum consumes directly — same shape as PERMISSION_NAMES.

ADMINISTRATOR_ROLES stays a literal: it carries each role's display name and description, mirrored from the docstrings on Python's AdministratorRole enum, so there is nothing to derive them from. Its id is already typed AdministratorRoleName and data.test.ts pins the list. The test fake is a fixture and is meant to be an independent copy.

@igboyes
igboyes merged commit c771682 into main Aug 14, 2026
23 checks passed
@igboyes
igboyes deleted the igboyes/vir-2977-drop-the-spaces-administrator-role branch August 14, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant