Skip to content

[PM-16095] validation message updates server - #8295

Open
JaredScar wants to merge 2 commits into
mainfrom
ac/pm-16095-validation-message-updates-server
Open

[PM-16095] validation message updates server#8295
JaredScar wants to merge 2 commits into
mainfrom
ac/pm-16095-validation-message-updates-server

Conversation

@JaredScar

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Changes up validation messages to messages that design signed off on

@JaredScar
JaredScar requested a review from a team as a code owner August 31, 2026 15:13
@JaredScar JaredScar added ai-review Request a Claude code review t:bugfix Change Type - Bugfix labels Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the invite-user validation error message changes across the Password Manager, Provider, and self-host environment validators, plus the supporting InviteOrganizationProvider constructor change and the updated unit test. Traced each string.Format call site and confirmed the nullable Seats value cannot be null on any reachable path — InviteUsersPasswordManagerValidator.ValidateAsync returns early when Seats is null because SeatsRequiredToAdd is 0, so no message can render an empty seat count. The InviteOrganizationProvider constructor signature change has a single call site and no test usages, so nothing else breaks. The pattern matches the pre-existing PasswordManagerPlanOnlyAllowsMaxAdditionalSeatsError, and no sensitive data is introduced into user-facing messages.

Code Review Details
  • ♻️ : Seats on a provider model holds the organization's seat count, which reads as provider seats
    • src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/Validation/Provider/InviteOrganizationProvider.cs:11
  • ❓ : PasswordManagerSeatLimitHasBeenReachedError still uses the old copy while its three siblings were updated
    • src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/InviteUsers/Validation/PasswordManager/Errors.cs:17

public ProviderType Type { get; init; }
public ProviderStatusType Status { get; init; }
public bool Enabled { get; init; }
public int? Seats { get; init; }

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.

♻️ DEBT: Seats on a provider model holds the organization's seat count, which reads as provider seats.

Details and fix

The only call site passes the organization's seat total:

new InviteOrganizationProvider(provider, request.Seats) // request is PasswordManagerSubscriptionUpdate

Providers have their own seat concepts (ProviderPlan.SeatMinimum, PurchasedSeats, AllocatedSeats), so a bare Seats on InviteOrganizationProvider is ambiguous. In ProviderBillableSeatLimitError/ProviderResellerSeatLimitError the message renders "Contact your provider to purchase additional seats" next to invalidRequest.Seats, which reinforces the wrong reading.

Renaming to OrganizationSeats (and the constructor parameter to organizationSeats) removes the ambiguity with no behavior change.

private static string GetErrorMessage(PasswordManagerSubscriptionUpdate invalidRequest) =>
string.Format(Code, invalidRequest.Seats);

public const string Code = "Seat limit of {0} has been reached. Contact Customer Support to upgrade your plan.";

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.

QUESTION: Was PasswordManagerSeatLimitHasBeenReachedError intentionally left out of this update?

Details

Three errors in this PR moved to "Seat limit of {0} has been reached. …", but PasswordManagerSeatLimitHasBeenReachedError (line 8) still reads "Seat limit has been reached." That one fires from InviteUsersPasswordManagerValidator.ValidatePasswordManager when MaxSeatsExceeded is true, which is the most common seat-limit path for cloud organizations, so users hitting the autoscale ceiling will see the old copy while the plan/provider/self-host paths show the new copy.

If design did include it, the natural substitution would be MaxAutoScaleSeats (the limit that was exceeded) rather than Seats. If it was deliberately scoped out, no change needed.

@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 16.66667% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.66%. Comparing base (17a1912) to head (0c8e3d5).

Files with missing lines Patch % Lines
...ionUsers/InviteUsers/Validation/Provider/Errors.cs 0.00% 4 Missing ⚠️
.../Validation/Provider/InviteOrganizationProvider.cs 0.00% 3 Missing ⚠️
...n/GlobalSettings/CannotAutoScaleOnSelfHostError.cs 0.00% 2 Missing ⚠️
...wordManager/InviteUsersPasswordManagerValidator.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8295      +/-   ##
==========================================
- Coverage   63.67%   63.66%   -0.01%     
==========================================
  Files        2467     2467              
  Lines      105693   105699       +6     
  Branches     9551     9551              
==========================================
  Hits        67295    67295              
- Misses      36082    36088       +6     
  Partials     2316     2316              

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

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:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant