Skip to content

[PM-36963] Bulk cohort assignment#7792

Open
kdenney wants to merge 39 commits into
mainfrom
billing/PM-36963/bulk-cohort-assignment
Open

[PM-36963] Bulk cohort assignment#7792
kdenney wants to merge 39 commits into
mainfrom
billing/PM-36963/bulk-cohort-assignment

Conversation

@kdenney

@kdenney kdenney commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-36963

📔 Objective

Adds the operator-facing bulk cohort assignment (CSV upload) workflow under the Tools menu of the Bitwarden Portal (src/Admin), gated behind the same business-plan price-migration feature flag as the Cohorts CRUD UI (#7732).

What this adds:

  • Bulk-assign page (Index): upload a 2-column CSV (OrganizationId,CohortName) with a header row. An empty cohort cell un-assigns the organization, organizations not listed are left unchanged, and cohort names match case-insensitively. File uploads are capped at 25 MB via a new MaxFileSizeAttribute + FileSize25mb constant.
  • CSV parsing and validation in Core (CohortBulkAssignmentCsvParser): malformed rows, unknown organizations, and unknown cohort names are collected with line numbers and surfaced back on the upload page as a line-by-line error list. Nothing is written to the database when the file has errors.
  • Atomic apply through a single set-based OPENJSON MERGE stored procedure (OrganizationPlanMigrationCohortAssignment_SyncMany) that inserts, updates, and un-assigns in one statement and returns Inserted / Updated / Unassigned / Skipped counts.
  • Lean bulk reads to resolve the file: Organization_ReadPlanTypesByIds (backed by a new OrganizationPlanTypeView) and OrganizationPlanMigrationCohort_ReadManyByNames. Dapper + EF Core parity for the reads; the bulk sync is MSSQL/Dapper-only and the EF implementation intentionally throws NotSupportedException.
  • Locked-assignment protection (see product note below): organizations whose assignment is already scheduled to migrate, has migrated, or has had a churn discount applied are treated as locked and skipped by the bulk apply. The result page reports how many rows were skipped and why.
  • Plan-mismatch detection (informational, never blocks): counts migration-cohort rows whose organization plan doesn't match the cohort's migration path.
  • Result page: a stat-block breakdown (Inserted / Updated / Unassigned / Plan mismatch / Skipped) with conditional notes that explain the plan-mismatch and skipped counts only when they are non-zero.
  • BulkSyncCohortAssignmentsCommand orchestrating parse → validate → resolve → sync, DI registration, the controller (RequirePermission(Tools_ManagePlanMigrationCohorts), feature-flag gated), and a nav entry under Tools.
  • New dependency to Core (already present in Commercial.Core): CsvHelper.
  • New stored procedures: OrganizationPlanMigrationCohortAssignment_SyncMany, Organization_ReadPlanTypesByIds, OrganizationPlanMigrationCohort_ReadManyByNames; plus an update to the existing OrganizationPlanMigrationCohortAssignment_ReadNonPendingCountByCohortId to align its lock predicate with the locked-assignment behavior (adds a new dated migration since that proc is already deployed).

Product decision: locking already scheduled / migrated assignments

After discussing with product, we decided that assignments whose organization is already scheduled to migrate or has already been migrated (and, for Churn-only cohorts, has had a churn discount applied) should be treated as locked and skipped during a bulk upload, rather than reassigned or un-assigned. This mirrors the single-organization edit path, which already protects these organizations, and prevents a bulk CSV from disturbing in-flight or completed migrations. The bulk sync skips these rows, reports the count as Skipped on the result page with a short explanation, and the same lock definition is now shared across both SQL lock gates and the C# IsLocked() entity method.

Where this deviates from the mockups and why

  • The two-step dry-run preview → "Confirm and apply" flow from the mockup was removed from scope (see Jira ticket). The page uploads and applies in a single step and then routes to a results breakdown.
  • Added a Skipped count and explanatory note that isn't in the mockup, to surface the locked organizations that were intentionally left unchanged (per the product decision above).
  • The mockup's "Currently selected: (N rows)" helper text under the file input isn't shown — the form doesn't introspect the file before submit.
  • Result counts render as stat blocks with thousands separators, and the plan-mismatch / skipped explanations appear only when their count is greater than zero, rather than as always-on descriptive text.

📸 Screenshots

image image image image

kdenney added 30 commits June 9, 2026 10:29
The migration was named 2026-06-03_00 but base already occupies
2026-06-03_00 and _01. Bump to _02 so it sorts chronologically after
the base migrations while staying before the 2026-06-08 update.
@kdenney kdenney added the ai-review Request a Claude code review label Jun 9, 2026
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the bulk cohort assignment (CSV upload) workflow: the Admin controller/views, the BulkSyncCohortAssignmentsCommand, the CohortBulkAssignmentCsvParser, the new Dapper reads and the MSSQL-only OPENJSON MERGE sync proc, the EF parity changes, and the two dated migrations. The change is well-structured with strong unit and integration test coverage, documented dual-ORM behavior (EF intentionally throws NotSupportedException for the bulk sync), and a UNIQUE(OrganizationId) backstop behind the MERGE. No blocking issues found.

Code Review Details

No new findings. Notes from verification:

  • Lock-predicate parity verified. The MigratedDate arm is now present in all three gates — the EF GetCohortNonPendingAssignmentsCountAsync query, the _ReadNonPendingCountByCohortId proc (plus its dated migration), and the new _UpdateManySync #Locked predicate — and is consistent with the entity's IsLocked(). Covered by the MigratedDate-only integration case.
  • Dependency: CsvHelper 33.1.0. Already an approved direct dependency on main (Commercial.Core); this PR promotes it to Core at the identical version. Not a net-new dependency, so no AppSec approval finding applies.
  • MERGE concurrency. The sync MERGE has no HOLDLOCK hint, but the operator-only, feature-flag-gated, low-concurrency workflow plus the UNIQUE constraint on OrganizationId make a true insert race both unlikely and self-correcting (surfaces as Unhandled). Not flagged.
  • All existing review threads correspond to feedback already addressed in later commits (proc rename to _UpdateManySync, temp tables, @RevisionDate parameterization, MemoryStream disposal).

@kdenney kdenney marked this pull request as ready for review June 9, 2026 21:23
@kdenney kdenney requested review from a team as code owners June 9, 2026 21:23

@amorask-bitwarden amorask-bitwarden left a comment

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.

🔥 🔥 🔥

BTreston
BTreston previously approved these changes Jun 11, 2026

@BTreston BTreston left a comment

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.

Approved for AC owned files

@mkincaid-bw mkincaid-bw left a comment

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.

Naming issue and performance-related question.

@kdenney kdenney dismissed stale reviews from BTreston and amorask-bitwarden via e6e5fce June 15, 2026 16:57

@mkincaid-bw mkincaid-bw left a comment

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.

The temp table changes look good but I missed the usage of GETUTCDATE().

@sonarqubecloud

Copy link
Copy Markdown

@kdenney kdenney requested a review from mkincaid-bw June 16, 2026 00:15
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.

4 participants