[PM-36949] feat: Add OrganizationPlanMigrationCohort and Assignment tables with bare repositories#7644
Conversation
… types Add the foundation for cohort-based plan migrations: - Two tables: OrganizationPlanMigrationCohort and OrganizationPlanMigrationCohortAssignment - Two views and nine stored procedures (four CRUD on cohort, four CRUD plus ReadByOrganizationId on assignment) - Single Migrator script for MSSQL deployment - Core entities, MigrationPath value object and its registry, and bare repository interfaces under Bit.Core.Billing.Organizations.PlanMigration The cohort table holds the human-managed metadata (name, discount coupons, MigrationPathId byte) and the assignment table records each organization's position in the migration lifecycle (scheduled, migrated, churn-mitigated). Both Update SPs follow the accept-but-don't-assign pattern: immutable columns (OrganizationId, CohortId, CreatedAt) are parameters but not SET clauses.
OrganizationPlanMigrationCohortRepository inherits the base Repository<T, TId> CRUD methods unchanged. OrganizationPlanMigrationCohortAssignmentRepository also relies on the base for CRUD and adds GetByOrganizationIdAsync which returns at most one row (the UNIQUE constraint on OrganizationId at the database layer guarantees this).
…rations for plan migration cohort tables - EF models wrap the Core entities; the assignment model exposes nav properties for Organization and Cohort so the FK + cascade-delete is inferred by EF. - EntityTypeConfiguration classes pin ID generation to application code (ValueGeneratedNever) and declare the UNIQUE indexes plus the composite (CohortId, ScheduledAt, MigratedAt) index. - Repositories follow the OrganizationInstallationRepository template; the assignment repo adds GetByOrganizationIdAsync to mirror the SP exposed on the MSSQL side. - DatabaseContext gets two DbSet properties; auto-discovery picks up the configuration classes. - Generated migrations for MySQL, Postgres, and SQLite create matching schemas; EF truncates FK and index names on providers with 64-char identifier limits, which is consistent with the rest of the codebase.
…ories Register both Dapper and EF Core repositories in their respective service collection extensions, following the existing AddSingleton convention in these files. Add tests: - MigrationPathIdsSnapshotTests guards the immortal byte IDs that downstream code pins on. The class- and method-level comments document why these values can never be renumbered. - MigrationPathTests covers the FromId round-trip and the null-on-unknown behavior the registry promises to callers. - OrganizationPlanMigrationCohortRepositoryTests exercises CRUD, the UNIQUE Name constraint, and verifies that ReplaceAsync ignores CreatedAt mutations (per the accept-but-don't-assign Update SP). - OrganizationPlanMigrationCohortAssignmentRepositoryTests exercises CRUD, GetByOrganizationIdAsync, the UNIQUE OrganizationId constraint, cascade-delete from both Organization and Cohort, and verifies that ReplaceAsync ignores OrganizationId, CohortId, and CreatedAt mutations.
Replace the byte Id with a byte-backed MigrationPathId enum and replace the string FromPlan/ToPlan fields with PlanType. Persistence is unchanged -- EF normalises enum-backed properties to their underlying type in the model snapshot, and Dapper handles enum-to-byte parameter mapping automatically.
…mmutability parity The Dapper _Update SPs accept-but-don't-assign certain columns (CreatedAt on cohort; OrganizationId, CohortId, and CreatedAt on assignment), but the base EF Repository<T,TEntity,Guid>.ReplaceAsync uses SetValues which writes every scalar. Override on both repos and mark the immutable properties as IsModified = false so MySQL/Postgres/Sqlite match MSSQL behavior. Mirrors the existing DeviceRepository.ReplaceAsync pattern.
Add [MaxLength] attributes to the three cohort string properties so the EF providers (MySQL/Postgres/Sqlite) enforce the same limits as MSSQL, where the columns were already NVARCHAR-capped. Widen Name from 64 to 255 chars across MSSQL DDL, both _Create/_Update SP signatures, the Migrator script, the entity, and all three regenerated EF migrations. Coupon codes stay at 64 (Stripe IDs are short).
The snapshot test class doc says "byte N means a specific FromPlan -> ToPlan transition forever", but only the byte value was being asserted. A silent refactor of the registry's PlanType references would not have been caught. Add per-path FromPlan/ToPlan assertions to close the gap.
Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the foundational schema and repository layer for the business plan price migration program (PM-36949). The PR adds two tables ( The implementation follows established codebase patterns: The snapshot tests appropriately lock the byte values of No findings. |
Postgres timestamp and MySQL datetime(6) store microsecond precision (6 fractional digits), but .NET DateTime is 100ns ticks (7 digits). Exact Assert.Equal fails by a single tick on round-trip. Switch the three DateTime comparisons in ReplaceAsync_UpdatesMutableColumns_AndIgnoresImmutableOnes to LaxDateTimeComparer.Default -- the same 2ms-tolerance comparer used by SendRepositoryTests and InstallationRepositoryTests for the same precision issue.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7644 +/- ##
==========================================
+ Coverage 59.84% 64.31% +4.47%
==========================================
Files 2121 2133 +12
Lines 93460 93620 +160
Branches 8291 8296 +5
==========================================
+ Hits 55931 60215 +4284
+ Misses 35548 31335 -4213
- Partials 1981 2070 +89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Just some minor naming issues.
Also, total nitpick but there should be no space between the type name and its size (e.g., DATETIME2(7) not DATETIME2 (7)
https://contributing.bitwarden.com/contributing/code-style/sql/#column-definitions
| [ProactiveDiscountCouponCode] NVARCHAR (64) NULL, | ||
| [ChurnDiscountCouponCode] NVARCHAR (64) NULL, | ||
| [IsActive] BIT NOT NULL CONSTRAINT [DF_OrganizationPlanMigrationCohort_IsActive] DEFAULT (0), | ||
| [CreatedAt] DATETIME2 (7) NOT NULL, |
There was a problem hiding this comment.
Should be named CreationDate
| [ScheduledAt] DATETIME2 (7) NULL, | ||
| [MigratedAt] DATETIME2 (7) NULL, | ||
| [ChurnDiscountAppliedAt] DATETIME2 (7) NULL, | ||
| [CreatedAt] DATETIME2 (7) NOT NULL, |
There was a problem hiding this comment.
Datetime column names should end with Date, not At
| @ProactiveDiscountCouponCode NVARCHAR(64), | ||
| @ChurnDiscountCouponCode NVARCHAR(64), | ||
| @IsActive BIT, | ||
| @CreatedAt DATETIME2 (7), |
There was a problem hiding this comment.
Same with the column names, Datetime parameter names should end with Date, not At
| @ProactiveDiscountCouponCode NVARCHAR(64), | ||
| @ChurnDiscountCouponCode NVARCHAR(64), | ||
| @IsActive BIT, | ||
| @CreatedAt DATETIME2 (7), |
There was a problem hiding this comment.
Same issue (Date vs At)
| @ScheduledAt DATETIME2 (7), | ||
| @MigratedAt DATETIME2 (7), | ||
| @ChurnDiscountAppliedAt DATETIME2 (7), | ||
| @CreatedAt DATETIME2 (7), |
There was a problem hiding this comment.
Same issue (Date vs At)
| @ScheduledAt DATETIME2 (7), | ||
| @MigratedAt DATETIME2 (7), | ||
| @ChurnDiscountAppliedAt DATETIME2 (7), | ||
| @CreatedAt DATETIME2 (7), |
There was a problem hiding this comment.
Same issue (Date vs At)
sbrown-livefront
left a comment
sbrown-livefront
left a comment
There was a problem hiding this comment.
✨ Clean. Just a question.
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-36949
📔 Objective
Foundational schema and repository layer for the business plan price migration program — the first implementation ticket under epic PM-35215. Lands the database tables, entities, value object, and bare CRUD repositories that ~12 sibling tickets consume. Scope is deliberately narrow; bulk CSV import, list-with-counts aggregates, the scheduler refactor, the churn-mitigation runtime, and all Admin Portal endpoints are owned by downstream tickets.
What's in
OrganizationPlanMigrationCohortandOrganizationPlanMigrationCohortAssignment. Includes composite index(CohortId, ScheduledAt, MigratedAt)on the assignment table to support the downstream Cohort Management aggregate without a follow-upALTER.MigrationPathvalue object +MigrationPathsregistry, andMigrationPathIdbyte-backed enum inBit.Core.Billing.Organizations.PlanMigration.Repository<T, TId>; assignment repo additionally exposesGetByOrganizationIdAsync.IEntityTypeConfiguration<T>+ repository implementations; provider migrations registered for MySQL, Postgres, and Sqlite._Updatestored procedures use the "accept-but-don't-assign" pattern for immutable columns (cohort:CreatedAt; assignment:OrganizationId,CohortId,CreatedAt). EF repositories mirror this viaReplaceAsyncoverrides (IsModified = falseon the immutable properties), matching theDeviceRepository.ReplaceAsyncidiom.MigrationPathIdbyte values and each registered path'sFromPlan/ToPlan— these are immortal once shipped.Nameand assignmentOrganizationId, and cascade-delete from bothOrganizationandCohort.What's intentionally out of scope
Tracked by sibling tickets under PM-35215. Not in this PR:
PriceIncreaseSchedulerrefactor — PM-37064UpcomingInvoiceHandlerbusiness-plan branch — PM-37068