Skip to content

[PM-36419] [BEEEP] Add collection management settings to seeder#7576

Merged
theMickster merged 7 commits intomainfrom
ac/pm-36419/beeep-add-collection-management-settings-to-seeder
May 7, 2026
Merged

[PM-36419] [BEEEP] Add collection management settings to seeder#7576
theMickster merged 7 commits intomainfrom
ac/pm-36419/beeep-add-collection-management-settings-to-seeder

Conversation

@eliykat
Copy link
Copy Markdown
Member

@eliykat eliykat commented May 2, 2026

🎟️ Tracking

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

📔 Objective

Allow control of collection management settings via the seeder. These alter organization/collection behavior significantly and go hand-in-hand with different collection/group assignments to set up a customer's preferred access controls.

I wanted to avoid appending lots of nullable parameters to various methods, so I've created an OrganizationOverrides bucket to contain them. This should allow for easier modification in in the future as well.

📸 Screenshots

@eliykat eliykat requested a review from theMickster May 2, 2026 03:38
Comment thread util/Seeder/Factories/PlanFeatures.cs
@eliykat eliykat added the ai-review Request a Claude code review label May 2, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 2, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed PR #7576: adds collection management settings (AllowAdminAccessToAllCollectionItems, LimitCollectionCreation, LimitCollectionDeletion, LimitItemDeletion, plus UseAutomaticUserConfirmation) to the Seeder utility via a new OrganizationOverrides bucket. Changes are propagated cleanly through the CLI args, the JSON preset schema, the SeedPresetOrganization model, and the PresetLoader -> RecipeBuilder -> CreateOrganizationStep pipeline. The previously hard-coded true defaults in PlanFeatures.Apply are removed so seeded values now match the production CLR defaults of false, which is the intended behavior.

Code Review Details

No actionable findings. Notable verifications:

  • The behavior change (removing true defaults in PlanFeatures.Apply) was discussed and acknowledged in an existing review thread; no preset under util/Seeder/Seeds/fixtures/presets/ references these properties, so nothing currently depends on the old defaults.
  • PlanFeatures.ApplyOrganizationOverrides correctly handles a null overrides argument and applies ?? org.X per property, so a non-null record with all-null fields is a safe no-op (the path the CLI always takes).
  • <see cref="...OrganizationSeeder.Create"/> references in OrganizationOverrides.cs and OrganizationVaultOptions.cs were corrected in commit c39ee19.
  • Adding optional parameters to RecipeBuilderExtensions.UseOrganization / CreateOrganization is source-compatible; the methods are only consumed inside this util.
  • Per-feature unit-test absence matches the established convention for this util (e.g., PlanFeatures.Apply and CreateOrganizationStep are likewise covered only via integration tests).

Comment thread util/Seeder/Seeds/schemas/preset.schema.json
@codecov
Copy link
Copy Markdown

codecov Bot commented May 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.76%. Comparing base (5b06505) to head (c39ee19).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7576      +/-   ##
==========================================
- Coverage   64.14%   59.76%   -4.38%     
==========================================
  Files        2103     2103              
  Lines       92738    92738              
  Branches     8262     8262              
==========================================
- Hits        59486    55425    -4061     
- Misses      31207    35355    +4148     
+ Partials     2045     1958      -87     

☔ 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.

@theMickster
Copy link
Copy Markdown
Contributor

theMickster commented May 6, 2026

Working through a multi-agent-review with Claude, brought up an interesting, minor change to wording in the XML comments that worth considering.

♻️ Doc comments say "keep the plan default" but these settings are not plan-gated

util/Seeder/Options/OrganizationOverrides.cs:4
Caught by: Code quality agent

Three doc comments in this diff describe a null override as "keeping the plan default":

  1. OrganizationOverrides.cs class summary: "Null properties mean 'keep the plan default'."
  2. PlanFeatures.ApplyOrganizationOverrides XML summary: "null means 'keep the plan default'"
  3. OrganizationVaultOptions.Overrides summary: "Null means 'use plan defaults for everything'."

None of the five settings (UseAutomaticUserConfirmation, AllowAdminAccessToAllCollectionItems, LimitItemDeletion, LimitCollectionCreation, LimitCollectionDeletion) are set anywhere in the PlanType switch. The deleted comment in PlanFeatures.Apply was accurate: // Org-level admin settings — not plan-gated, safe defaults for seeding. The real fallback when no override is supplied is whatever OrganizationSeeder.Create initializes the entity to (the C# bool default of false), not a plan-derived value.

What "plan default" implies: That the value is determined by which PlanType the org uses — Enterprise gets one default, Teams gets another, Free gets another, etc.

What actually happens: None of these five settings appear anywhere in the PlanType switch in PlanFeatures.Apply. There is no per-plan logic for them — never was. They're org-level admin settings orthogonal to billing plan. If no override is supplied, the value falls back to whatever the Organization entity initializes to: the C# bool default of false.

It's a ♻️ (not⚠️) because the code behaves correctly. No one is getting incorrect data; a future maintainer just gets a misleading mental model.

Why it matters: A developer reading "plan default" in the future might:

  • Look in the PlanType switch for where these defaults come from and find nothing
  • Assume Enterprise orgs get different defaults than Free orgs and write a test around that assumption
  • Add per-plan initialization thinking they're filling in a gap

The accurate wording would be something like "keep the current value" or "leave unchanged from OrganizationSeeder.Create" — meaning null is purely a no-op passthrough, not a delegation to plan logic.

Suggested fix — replace "plan default" with "entity default" or a reference to OrganizationSeeder.Create in all three locations. Example:

/// <summary>
/// Optional overrides applied on top of the organization's initial values.
/// Null properties mean "keep the value set by <see cref="OrganizationSeeder.Create"/>".
/// </summary>

Copy link
Copy Markdown
Contributor

@theMickster theMickster left a comment

Choose a reason for hiding this comment

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

Two minor changes to documentation, but otherwise support this 100%

@eliykat eliykat requested a review from theMickster May 7, 2026 04:29
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

@theMickster theMickster merged commit 55e06e3 into main May 7, 2026
44 checks passed
@theMickster theMickster deleted the ac/pm-36419/beeep-add-collection-management-settings-to-seeder branch May 7, 2026 06: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.

2 participants