Skip to content

[PM-36965] feat: Add SeederApi scene for migration cohort CSV-export testing#7906

Open
cyprain-okeke wants to merge 7 commits into
mainfrom
billing/pm-36965-seeder-cohort-to-cohort-management-table
Open

[PM-36965] feat: Add SeederApi scene for migration cohort CSV-export testing#7906
cyprain-okeke wants to merge 7 commits into
mainfrom
billing/pm-36965-seeder-cohort-to-cohort-management-table

Conversation

@cyprain-okeke

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Adds MigrationCohortExportScene to the Seeder — the HTTP/API equivalent of the
PM-36965 SQL seed script — so QA and local environments can populate the Admin
Portal migration-cohort management table with a cohort ready to Export CSV,
without hand-running SQL.

Given a cohortName and orgCount, the scene creates (or reuses by name) a
migration cohort, then bulk-creates N inert placeholder organizations
(Enabled = false, no billing, no users) and their cohort assignments. Real
Organization rows are required because OrganizationPlanMigrationCohortAssignment
has a FK to Organization with a UNIQUE constraint on OrganizationId.

Usage (local, SeederApi on :5047):

POST /seed
{ "template": "MigrationCohortExportScene",
  "arguments": { "cohortName": "PM-36965 Export Test", "orgCount": 5000 } }

Optional args: migrationPathId (default 1; null = churn-only cohort),
namePrefix (default pm36965-seed-).

Implementation notes

  • Organizations and assignments are bulk-inserted via LinqToDB BulkCopy (the same
    path BulkCommitter uses) to stay fast up to the [Range(1, 100000)] ceiling.
  • BulkCopy bypasses the repository CreateAsync play-id tracking hook, so the
    scene records PlayItem rows itself. This lets DELETE /seed/{playId} tear the
    seeded organizations down (assignments cascade via FK). The emptied cohort row is
    not play-id tracked (PlayItem has no CohortId) and is removed by name.
  • Local / non-prod only — the SeederApi refuses to run in production.

Testing

  • Integration tests assert database persistence (cohort + orgs + assignments read
    back), cohort reuse-by-name, and one-PlayItem-per-org tracking for cleanup.
  • Full SeederApi.IntegrationTest suite: 180/180 passing.

📸 Screenshots

…testing

Adds MigrationCohortExportScene, the HTTP/API equivalent of the PM-36965
SQL seed script: it creates a migration cohort plus N inert placeholder
organizations and their cohort assignments, so the Admin Portal
cohort-management table has a populated cohort ready to Export CSV.

Organizations and assignments are bulk-inserted via LinqToDB BulkCopy
(the same path BulkCommitter uses) to stay fast at scale. Because BulkCopy
bypasses the repository CreateAsync tracking hook, the scene records
PlayItem rows itself so DELETE /seed/{playId} can tear the seeded
organizations down; the empty cohort row is not play-id tracked and is
removed by name.

Covered by integration tests asserting database persistence, cohort
reuse-by-name, and play-id tracking.
@cyprain-okeke cyprain-okeke added the ai-review Request a Claude code review label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new MigrationCohortExportScene SeederApi scene (single file, local/non-prod tooling). The scene builds a migration cohort plus N inert placeholder organizations and their cohort assignments, bulk-inserts them via LinqToDB BulkCopy inside a single EF transaction, and self-records PlayItem rows so DELETE /seed/{playId} can tear the data down. I verified the transaction/BulkCopy pattern matches UserRepository, the FK ON DELETE CASCADE on OrganizationPlanMigrationCohortAssignment confirms the documented cleanup behavior, and the keyset-export ordering rationale in the remarks is accurate.

Code Review Details

No code-level findings. The implementation follows established Seeder conventions (BulkCopy + await using transaction rollback, mapper-based Core→EF mapping, explicit SetNewId() where BulkCopy bypasses the repository COMB hook), and cohort reuse-by-name is consistent with the IX_OrganizationPlanMigrationCohort_Name UNIQUE constraint.

PR Metadata Assessment

  • QUESTION: The Testing section states integration tests were added and "180/180 passing", but no test file is present in the diff and the branch history shows scene-specific tests were removed per Scenes owner guidance. Consider updating the description so the stated test coverage matches what ships.

Comment thread util/Seeder/Scenes/MigrationCohortExportScene.cs Fixed
@cyprain-okeke cyprain-okeke added the t:feature Change Type - Feature Development label Jul 1, 2026
… add churn-path and range tests

- Wrap cohort + orgs + assignments + PlayItem inserts in one transaction so a
  mid-sequence failure rolls back instead of leaving partly-seeded state.
- Move cohort creation into the transactional BulkCopy path.
- Enforce OrgCount range explicitly in SeedAsync (SceneExecutor does not run
  DataAnnotations on scene requests), returning BadRequest out of range.
- Reframe the CreationDate comment around observed batch behavior (rot-hardening).
- Add tests: null MigrationPathId => churn cohort, OrgCount range validation
  (0/-1/100001 rejected, 1 accepted), and assert persisted MigrationPathId.
Comment thread util/Seeder/Scenes/MigrationCohortExportScene.cs Fixed
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.40%. Comparing base (5e773db) to head (1fc872a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7906   +/-   ##
=======================================
  Coverage   61.40%   61.40%           
=======================================
  Files        2227     2227           
  Lines       98345    98345           
  Branches     8894     8894           
=======================================
  Hits        60386    60386           
  Misses      35822    35822           
  Partials     2137     2137           

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

SeedControllerTests covers scene discovery/naming and delete generically, not
per-scene end-to-end behavior; and cleanup is handled by DeleteOldPlayDataJob
(verified running clean locally against SQL Server with no Command errors), so
the dedicated cleanup tests are redundant. Restores SeedControllerTests to its
original state and removes MigrationCohortExportSceneCleanupTests.
@cyprain-okeke cyprain-okeke marked this pull request as ready for review July 2, 2026 08:48
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 t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants