Skip to content

[PM-35357] Update Trial Length Parameter#7597

Open
sbrown-livefront wants to merge 33 commits into
mainfrom
billing/pm-35357/update-trial-length-parameter
Open

[PM-35357] Update Trial Length Parameter#7597
sbrown-livefront wants to merge 33 commits into
mainfrom
billing/pm-35357/update-trial-length-parameter

Conversation

@sbrown-livefront
Copy link
Copy Markdown
Collaborator

@sbrown-livefront sbrown-livefront commented May 7, 2026

🎟️ Tracking

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

📔 Objective

This pull request implements a custom trial length to be specified.

API and model changes:

  • Added TrialLength (with validation of 0–30 days) properties to organization creation and signup models (OrganizationCreateRequestModel, OrganizationNoPaymentCreateRequest, OrganizationSignup) and ensured these are passed through to business logic and billing.
  • Updated the API for sending trial initiation verification emails to require and validate the new fields.

Business logic and billing integration:

  • The billing service uses the custom trial length if provided and validated; otherwise, it falls back to the default plan trial period.

📸 Screenshots

Screen.Recording.2026-05-08.at.2.03.17.PM.mov

@sbrown-livefront sbrown-livefront self-assigned this May 7, 2026
@sbrown-livefront sbrown-livefront added the ai-review Request a Claude code review label May 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a customizable TrialLength parameter (0–30 days) to organization signup, threading it from the API request models through OrganizationSignupOrganizationSaleSubscriptionSetup to Stripe's TrialPeriodDays. Input is defended at the API model ([Range(0, 30)]) and again in CloudOrganizationSignUpCommand.ValidateTrialLength, with a fallback to plan.TrialPeriodDays when no override is supplied. The trial verification email request model is tightened with [Required], [MinLength(1)], and init-only properties, and the hard-coded 7 is extracted to TrialInitiationConstants.DefaultTrialLengthDays. Test coverage is comprehensive across boundary values, null, and the mapping into OrganizationSignup.

Code Review Details

No new findings. All previously raised concerns are resolved:

  • Cache miss bypass on trial length validation — resolved (cache removed in 288072d).
  • Custom trial length not bound to verified email — confirmed intentional by author; PR description updated.
  • Trial-length tests passing for the wrong reason — resolved (thread is outdated against current code).

Comment thread src/Core/Billing/Cache/TrialInitiationCache.cs Outdated
@sbrown-livefront sbrown-livefront changed the title Billing/pm 35357/update trial length parameter [PM-35357] Update Trial Length Parameter May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.95%. Comparing base (a354aff) to head (411ba18).

Files with missing lines Patch % Lines
.../Billing/Models/Mail/TrialInititaionVerifyEmail.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7597      +/-   ##
==========================================
+ Coverage   59.84%   59.95%   +0.11%     
==========================================
  Files        2120     2120              
  Lines       93168    93179      +11     
  Branches     8264     8264              
==========================================
+ Hits        55753    55866     +113     
+ Misses      35440    35338     -102     
  Partials     1975     1975              

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

@sbrown-livefront sbrown-livefront marked this pull request as ready for review May 8, 2026 18:29
@sbrown-livefront sbrown-livefront requested review from a team as code owners May 8, 2026 18:29
JaredScar
JaredScar previously approved these changes May 8, 2026
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@cyprain-okeke cyprain-okeke left a comment

Choose a reason for hiding this comment

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

Nice Work!

.Select(d => d.Id.ToString());
}

private static void ValidateTrialLength(OrganizationSignup signup)
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.

❓ Now that this is just a range check, the [Range(0, 30)] on both request models already covers it for the public API path. Is the duplicate guard here for callers that bypass the model binder (provider signup, internal callers)? If so, fine — if not, we can drop it and let the model attribute be the single source of truth.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep this is for internal calls that may not run into the model.


await PerformConstantTimeOperationsAsync();

if (trialLength != 0 && trialLength != 7)
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.

Nice cleanup

Comment thread src/Core/Billing/Models/Mail/TrialInititaionVerifyEmail.cs
@bre-deploy bre-deploy Bot deployed to EU-QA Cloud May 15, 2026 14:09 Active
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.

3 participants