Skip to content

[PM-38388] Support sales-assisted trials#7881

Open
enmande wants to merge 23 commits into
mainfrom
auth/pm-38388/support-sales-assisted-trials
Open

[PM-38388] Support sales-assisted trials#7881
enmande wants to merge 23 commits into
mainfrom
auth/pm-38388/support-sales-assisted-trials

Conversation

@enmande

@enmande enmande commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-38388

📔 Objective

What changed

Registration path (Identity) - Added `SalesAssistedRegistrationTokenable` — a signed, expiring token carrying the prospect's email and optional name. Lifetime is configured via `globalSettings.salesAssistedRegistrationTokenLifetimeDays`. - Added `ISalesAssistedRegistrationTokenableFactory` — the sole mint path; guards the internal lifetime-setting constructor per the tokenable factory convention. - Added `RegisterUserViaSalesAssistedToken` to `RegisterUserCommand`. Unlike the standard path, this method intentionally omits `ValidateOpenRegistrationAllowed()` — the token itself is the authorization.
Admin portal (Admin) - New `SalesAssistedTrialController` (Auth area) with a form to configure and send invitations. Placed in Auth rather than Billing because the feature is fundamentally about bypassing the registration gate, not about billing or trial setup. - Nav link added to the Tools dropdown, gated behind the `pm-35092-auth-sales-assisted-trials` feature flag and the `Org_InitiateSalesAssistedTrial` permission. The flag keeps the entry hidden in Admin until the corresponding client work ships. - Form validation moved server-side: removed `type="email"` and `min`/`max` from inputs; added `[EmailAddress]` to the model so all fields validate consistently via ASP.NET model binding.
Invitation email (Core) - New MJML-based HTML email using the `mj-bw-ac-hero` component, product-aware feature bullets, and a "We're here for you!" support footer. - `SalesAssistedTrialInvitationEmailView` exposes computed properties (`HeroTitle`, `ProductName`, `SpotImageUrl`, `Features`) so the template stays logic-free. - `ExpiryDays` sourced from `globalSettings.SalesAssistedRegistrationTokenLifetimeDays`, distinct from the trial length.
SendSalesAssistedTrialInvitationCommand (Core.Billing) - Validates product tier, trial length range, and that the email is not already registered before minting the token. - Delegates mail send to `IMailer` (not the deprecated `IMailService`).

Tests

  • Unit tests for SendSalesAssistedTrialInvitationCommand — covers happy path, existing user rejection, trial length range validation, ExpiryDays sourced from settings (not trial length), and sender email forwarding.
  • Unit tests for RegisterUserCommand — covers token validation, expiry, and email binding.
  • Integration tests for the registration endpoint — three new cases in AccountsControllerTests: valid token with disableUserRegistration=true succeeds and creates the user; undecodable token returns 400; token bound to a different email returns 400.
  • IdentityApplicationFactory.RegisterNewIdentityFactoryUserAsync now explicitly nulls SalesAssistedToken before the finish call — AutoFixture previously populated it with a random value that shadowed the email verification token and caused unrelated integration tests to fail.

Notes for reviewers

  • The controller lives in the Auth area even though SendSalesAssistedTrialInvitationCommand is in Core.Billing. Auth area = registration bypass concern; Billing = email and trial lifecycle concern. A comment in the controller explains this.
  • The disableUserRegistration bypass is intentional and load-bearing. The token is the authorization mechanism.
  • The feature flag (pm-35092-auth-sales-assisted-trials) must be enabled before the Admin nav link is visible

📸 Screenshots

Happy Path

Admin fills out the supporting form and submits to onboard a new user and start their trial.
pm-38388__happy-path.mov

User Exists

The intended user already exists in the environment; Admin is denied use of this feature with a descriptive error message. From here, they can use existing pathways tailor-made for trial assistance. _Why?_ This flow is primarily concerned with onboarding users to environments where open registration is disallowed/disabled. This flow should be prioritized to solve that problem; initiating a trial for an existing user would constitute an off-label use of this feature.
pm-38388__user-exists.mov

Form Validation

Exercising various paths to submit the form.
pm-38388__form-validation.mov

Comment thread src/Admin/Auth/Controllers/SalesAssistedTrialController.cs Dismissed
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.44534% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.38%. Comparing base (2b9945a) to head (6318721).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
...il/Mailer/SalesAssistedTrialInvitationEmailView.cs 21.31% 48 Missing ⚠️
...c/Admin/Auth/Views/SalesAssistedTrial/Index.cshtml 0.00% 7 Missing ⚠️
src/Admin/Utilities/RolePermissionMapping.cs 0.00% 6 Missing ⚠️
src/Admin/Views/Shared/_Layout.cshtml 0.00% 6 Missing ⚠️
...Api/Request/Accounts/RegisterFinishRequestModel.cs 45.45% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7881      +/-   ##
==========================================
+ Coverage   61.21%   61.38%   +0.17%     
==========================================
  Files        2219     2243      +24     
  Lines       98059    98788     +729     
  Branches     8848     8937      +89     
==========================================
+ Hits        60025    60641     +616     
- Misses      35917    36010      +93     
- Partials     2117     2137      +20     

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

@enmande enmande added t:feature Change Type - Feature Development ai-review Request a Claude code review labels Jun 29, 2026
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the latest commit (6318721, "factory-own SalesAssistedRegistrationTokenable expiration") and changes not covered by the prior approving review. The commit correctly moves ExpirationDate computation into the factory via an injected TimeProvider (matching the OrgUserInviteTokenableFactory exemplar and the auth-tokenables rule), tightens both constructors to internal, and replaces the flaky time-tolerance test assertions with frozen-clock exact assertions. DI registration of TimeProvider.System uses the idempotent TryAddSingleton pattern, consistent with five existing usages. The feature-flag gating and generic-catch discussions from the prior review remain resolved as intentional.

Code Review Details
  • ❓ : The governing rules file .claude/rules/auth-tokenables.md was not updated to match this commit. Lines 19–20 still instruct authors to keep the [JsonConstructor] parameterless ctor public, but the exemplar now makes it internal (and its XML doc explains System.Text.Json can still deserialize via a non-public [JsonConstructor] ctor). A future tokenable author following the rule would contradict the pattern this PR establishes. The local working tree contains edits to this file (a new binding-properties requirement and a "Known limitation" section) but those are uncommitted and absent from the PR diff, so no inline comment could be posted. Consider committing the doc updates and reconciling lines 19–20 with the internal-ctor pattern.
    • .claude/rules/auth-tokenables.md:19 (not in PR diff)

Comment thread src/Admin/Auth/Controllers/SalesAssistedTrialController.cs
@sonarqubecloud

Copy link
Copy Markdown

@enmande enmande marked this pull request as ready for review June 29, 2026 22:31
@enmande enmande requested review from a team as code owners June 29, 2026 22:31
SalesAssistedRegistrationTokenableFactory computed ExpirationDate by
passing lifetimeDays into the tokenable's own constructor, rather than
setting it directly — violating .claude/rules/auth-tokenables.md's
"factory MUST set ExpirationDate from a GlobalSettings value" and
diverging from the OrgUserInviteTokenableFactory exemplar it was
modeled on.

That structure also made the mint-time computation untestable without
a tolerance window: SalesAssistedRegistrationTokenableTests carried a
10-second _timeTolerance to paper over two independent, unsynchronized
DateTime.UtcNow reads (one in the test, one in the constructor).

- Factory now sets ExpirationDate directly via an injected TimeProvider,
  matching OrgUserInviteTokenableFactory's shape; tests freeze time and
  assert exact values instead of tolerating a window.
- Both SalesAssistedRegistrationTokenable constructors are now internal,
  including the [JsonConstructor] one — previously public, so nothing
  but a comment stopped other assemblies from bypassing the factory.
  Verified System.Text.Json can still deserialize via a non-public
  [JsonConstructor] ctor (round-trip test still passes).
- Deleted a test covering a constructor responsibility that no longer
  exists; fixed two tests whose Valid/IsExpired assertions implicitly
  relied on the constructor's removed default ExpirationDate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@connerbw connerbw 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 billing with a nit

Comment thread src/Core/Constants.cs
public const string DebugDisableSelfHostPremiumCheck = "debug-disable-self-host-premium-check";
public const string PM35215_BusinessPlanPriceMigration = "pm-35215-business-plan-price-migration";
public const string PM37597_AlwaysEnableStripeAutomaticTax = "pm-37597-always-enable-stripe-automatic-tax";
public const string PM35092AuthSalesAssistedTrials = "pm-35092-auth-sales-assisted-trials";

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.

⛏️ This one is landing in the billing FF section. Could you move this up to auth's on line 166?

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