Fix for AZDO #3763

[jeffmaki]

https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3763

Updated the add_user_role_table Alembic migration to manage the PostgreSQL workspace_role enum explicitly.

• Creates the enum type idempotently before creating user_workspace_roles
• Adds the auth_uid_unique constraint on users
• Uses the shared enum definition for the role column
• Drops the table and enum cleanly during downgrade with checkfirst=True

This removes the inline enum/raw SQL drop approach and makes the migration more robust.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a8019503-f67f-4dac-962d-957740f79613

📥 Commits

Reviewing files that changed from the base of the PR and between e8c0902 and 0f056c6.

📒 Files selected for processing (1)

• alembic_osm/versions/9221408912dd_add_user_role_table.py

---

📝 Walkthrough

Walkthrough

A single Alembic migration file is updated to manage the workspace_role PostgreSQL enum type explicitly. A module-level postgresql.ENUM variable with create_type=False replaces the previous inline sa.Enum, and upgrade()/downgrade() now call .create(..., checkfirst=True) and .drop(..., checkfirst=True) instead of raw SQL execution.

Changes

workspace_role Enum Lifecycle Refactor

Layer / File(s)	Summary
Explicit enum definition and upgrade/downgrade lifecycle alembic_osm/versions/9221408912dd_add_user_role_table.py	Adds a module-level workspace_role = postgresql.ENUM(..., create_type=False) variable. upgrade() calls workspace_role.create(op.get_bind(), checkfirst=True) before table creation and references the shared enum for the role column. downgrade() calls workspace_role.drop(op.get_bind(), checkfirst=True) instead of op.execute(text("DROP TYPE workspace_role")).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• TaskarCenterAtUW/workspaces-backend#13: Addresses the same 9221408912dd_add_user_role_table.py migration and the double CREATE TYPE workspace_role problem that this PR's create_type=False/checkfirst=True approach directly resolves.

Poem

🐇 Hop hop, no more raw SQL to fear,
The enum now manages itself, how dear!
checkfirst=True keeps idempotence in sight,
No double-type errors to ruin the night.
The migration is tidy, the rabbit is pleased! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title references the tracked work item but does not describe the actual migration changes.	Rename it to summarize the main change, e.g. explicit workspace_role enum handling in the user role migration.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cyrossignol]

Functionally, this will produce the same effect as the trunk version. However, it introduces a couple of anti-patterns:

1. Use of engine-specific DDL - there is no need to describe the workspace_role type with postgresql.ENUM since sa.Enum maps this type cleanly.
2. The use of checkfirst=True violates a core tenant of these kinds of migration systems: the DB should reflect the exact state described by the migrations or fail loudly. The failure is a feature and helps us to control state drift. Besides, this check is name-based and doesn't actually verify the definition.

I'm not sure why the AI flagged this. I recommend closing the PR. The trunk version is correct and idiomatic.

[jeffmaki]

Sure, I'll take your word for it, thanks