Skip to content

[PM-37470] Add UseRiskInsights data migration for existing Enterprise organizations#7691

Open
AlexRubik wants to merge 6 commits into
mainfrom
dirt/pm-37470/use-risk-insights-data-migration
Open

[PM-37470] Add UseRiskInsights data migration for existing Enterprise organizations#7691
AlexRubik wants to merge 6 commits into
mainfrom
dirt/pm-37470/use-risk-insights-data-migration

Conversation

@AlexRubik
Copy link
Copy Markdown
Contributor

@AlexRubik AlexRubik commented May 21, 2026

🎟️ Tracking

PM-37470
Parent epic: PM-37469

📔 Objective

Add the data migration step from the Organization Ability README for UseRiskInsights. Sets UseRiskInsights = 1 on every existing Organization row with an Enterprise plan type (4, 5, 10, 11, 14, 15, 19, 20). Covers MSSQL (cloud) and the three EF providers (MySQL, Postgres, SQLite) used by self-host.

The column has existed since 2024-11-25_00_AddUseRiskInsightsToOrganization.sql but the README's step 4 (data migration for existing orgs) was never performed. A stopgap one-off datafix (DBOPS-59) handled cloud in Dec 2025, but orgs created since are missing the flag and self-host instances never received it. This PR delivers the proper repeatable migration.

Re-runs are no-ops: MSSQL uses a WHERE UseRiskInsights = 0 guard so the batched loop exits immediately when there is nothing to update; EF variants set 1 over 1 which is idempotent by construction.

Other subtasks under the parent epic handle plan-definition mapping (PM-37471), admin portal toggle (PM-37473), self-host license claims (PM-37474), re-enabling the endpoint guard (PM-37475), and client-side divergence (PM-37476).

Mirrors the pattern from PM-35253 (#7489) UseInviteLinksDataMigration. SQL formatted per the Bitwarden SQL style guide (each top-level keyword on its own line).

🤖 Testing

The IMigrationTesterService infrastructure was disabled in PM-37482 (#7633, May 14 2026) due to flakiness, so no automated migration integration test ships with this PR. Verification is manual:

  • Seed local MSSQL with one org per Enterprise plan type (4, 5, 10, 11, 14, 15, 19, 20) and a non-Enterprise control, all with UseRiskInsights = 0.
  • Apply migration. Confirm all Enterprise rows flip to 1 and the non-Enterprise row stays at 0.
  • Re-run the script. Confirm the batched loop exits on first iteration with @@ROWCOUNT = 0.
  • On at least one EF provider (MySQL, Postgres, or SQLite), seed equivalent rows and apply the migration via dotnet ef database update. Confirm same outcome.

If this PR sits in review across a calendar shift, rename the MSSQL script's date prefix to the actual merge date.

AlexRubik added 2 commits May 21, 2026 12:45
Set UseRiskInsights = 1 on existing Organization rows with Enterprise
plan types (4, 5, 10, 11, 14, 15, 19, 20). Batched and idempotent.

[PM-37470]
Adds equivalent UPDATE migration for MySQL, Postgres, and SQLite
providers used by self-hosted instances. Mirrors the MSSQL script.

[PM-37470]
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.15%. Comparing base (0569807) to head (5240a59).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7691   +/-   ##
=======================================
  Coverage   65.15%   65.15%           
=======================================
  Files        2167     2167           
  Lines       96061    96061           
  Branches     8649     8649           
=======================================
  Hits        62593    62593           
  Misses      31299    31299           
  Partials     2169     2169           

☔ View full report in Codecov by Sentry.
📢 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.

@AlexRubik AlexRubik added the ai-review Request a Claude code review label May 21, 2026
Keep UPDATE TOP (N) [target] (and bare UPDATE [target] in the EF
variants) as a single header line. SET and WHERE arguments still
indent onto their own line.

No functional change.

[PM-37470]
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the UseRiskInsights data migration covering MSSQL (cloud) and the three EF providers (MySQL, Postgres, SQLite) used by self-host. The migration mirrors the established UseInviteLinksDataMigration pattern (#7489) and strictly improves on it by adding AND UseRiskInsights = 0 guards across all four Up() implementations, making re-runs idempotent. Enterprise plan-type values (4, 5, 10, 11, 14, 15, 19, 20) match PlanType.cs exactly, and the previously raised inline comment requesting a trailing GO in the MSSQL script has been addressed.

Code Review Details

No findings.

The MSSQL script uses the batched WHILE @RowsAffected > 0 loop to avoid table locks, the EF migrations are kept consistent with that intent through the WHERE … = 0/false guard, and the PR description clearly justifies the manual test plan in light of IMigrationTesterService being disabled in PM-37482.

Match the MSSQL DbScript, which only updates rows where
UseRiskInsights is currently 0. Reduces write churn on
already-correct rows and keeps cloud (MSSQL) and self-host
(EF) UPDATE shape identical.

[PM-37470]
@AlexRubik
Copy link
Copy Markdown
Contributor Author

@claude review this again pleaseee

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 28, 2026

Claude finished @AlexRubik's task in 1m 43s —— View job


Claude Code Review (re-review)

  • Read MSSQL data migration script
  • Read EF Up()/Down() migration files (MySQL, Postgres, SQLite)
  • Verify Enterprise plan-type IDs against PlanType.cs
  • Verify column type and prior schema migration
  • Compare against UseInviteLinksDataMigration reference pattern
  • Post review findings

Overall Assessment: APPROVE — no new findings since the prior review.

Verification notes

  • Plan-type IDs are complete and correct. (4, 5, 10, 11, 14, 15, 19, 20) cover every Enterprise variant in src/Core/Billing/Enums/PlanType.cs:16-48 (2019, 2020, 2023, current). No Enterprise plan is missed; no non-Enterprise plan is included.
  • Underlying column is safe to update unconditionally. 2024-11-25_00_AddUseRiskInsightsToOrganization.sql:2 defines [UseRiskInsights] BIT NOT NULL DEFAULT (0), so the EF UPDATE ... SET UseRiskInsights = 1 cannot violate NOT NULL or read NULL.
  • MSSQL batched loop terminates correctly. WHILE @RowsAffected > 0 + the AND [UseRiskInsights] = 0 guard at util/Migrator/DbScripts/2026-05-21_00_UseRiskInsightsDataMigration.sql:17 guarantees the loop exits on first pass when there's nothing to do. Idempotent re-runs are no-ops.
  • EF Up() guard is an improvement over the reference. The AND UseRiskInsights = 0/false clause in all three EF Up() blocks (vs. unconditional UPDATE in 20260416151346_UseInviteLinksDataMigration.cs:16) avoids touching already-set rows. Nice tightening per the commit d4cc007.
  • SQL formatting matches the Bitwarden SQL style guide. Top-level keywords (UPDATE, SET, WHERE, AND) each on their own line. Consistent with the fc1342b formatting fix.
  • EF migration timestamp ordering is clean. 20260521164709 (Postgres) / 20260521164730 (MySQL) / 20260521164638 (SQLite) all sort after 20260518143024_AddOrganizationPlanMigrationCohort — no apply-order surprises.
  • Provider boolean encoding is correct per dialect. MySQL/SQLite use 1/0; Postgres uses true/false. Matches the column types defined in each provider's snapshot.
  • Down() behavior matches established precedent. The EF Down() resets every Enterprise org's UseRiskInsights to 0, which would clobber any value manually toggled in the Bitwarden Portal. This mirrors UseInviteLinksDataMigration's Down() exactly, and rolling back a data migration is inherently best-effort — flagging it for awareness, not as a blocker.

Observation (not a blocker)

The PR description already calls this out: if the PR sits in review past calendar shifts, the MSSQL filename 2026-05-21_00_UseRiskInsightsDataMigration.sql should be renamed to the actual merge date prefix so it slots into the correct chronological position in util/Migrator/DbScripts/. Today is 2026-05-28, so still fine.
| Branch: dirt/pm-37470/use-risk-insights-data-migration

@AlexRubik AlexRubik marked this pull request as ready for review May 28, 2026 21:34
@AlexRubik AlexRubik requested a review from a team as a code owner May 28, 2026 21:34
AND [UseRiskInsights] = 0;

SET @RowsAffected = @@ROWCOUNT;
END
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add trailing:
GO

Addresses @rkac-bw's review comment on PR #7691. Matches the
convention used by 9 of the 10 most recent DbScripts.

[PM-37470]
@rkac-bw rkac-bw dismissed their stale review June 2, 2026 20:16

fixed

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 2, 2026

@rkac-bw
Copy link
Copy Markdown
Contributor

rkac-bw commented Jun 2, 2026

@AlexRubik need to bump the SQL Server migration's date prefix to 2026-06-02_00 (or later) so it sorts after main's latest migration and no longer causes pr validation workflow to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants