[PM-40744] Update Server Error Messages for VFO1 - #8087
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8087 +/- ##
==========================================
+ Coverage 62.67% 62.72% +0.05%
==========================================
Files 2296 2301 +5
Lines 100044 100217 +173
Branches 9003 9020 +17
==========================================
+ Hits 62701 62863 +162
- Misses 35161 35169 +8
- Partials 2182 2185 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed the VFO1 error-copy pass across AdminConsole, Billing, and Commercial provider code, focusing on the changes pushed since the last review. The Code Review Details
Also noted, not worth its own comment: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| sutProvider.GetDependency<IAutomaticUserConfirmationPolicyEnforcementHandler>() | ||
| .GetAutoConfirmPolicyViolation(Arg.Any<AutomaticUserConfirmationPolicyRequirement>(), Arg.Any<Guid>(), Arg.Any<bool>(), Arg.Any<int>()) | ||
| .Returns(new UserCannotBelongToAnotherOrganization()); | ||
| .Returns(new UserCannotBelongToAnotherOrganization(string.Empty)); |
There was a problem hiding this comment.
♻️ DEBT: The new bulk email lookup is never exercised — these tests pass identically with the fix reverted.
Details and fix
SetupPassingBulkDataStubs (line 45) does not stub IUserRepository.GetManyAsync, so NSubstitute returns an empty sequence, emailByUserId is empty, TryGetValue fails, and WithEmail falls through to _ => error — leaving the string.Empty the stub supplied. Combined with the type-only assertions (Assert.IsType<UserCannotBelongToAnotherOrganization>), nothing here would fail if IUserRepository, usersTask, and WithEmail were removed from the validator, which is the blank-email bug that was just fixed.
Stubbing the repository in the helper and asserting on the message closes the gap:
sutProvider.GetDependency<IUserRepository>()
.GetManyAsync(Arg.Any<IEnumerable<Guid>>())
.Returns(orgUsers.Select(ou => new User { Id = ou.UserId!.Value, Email = $"{ou.UserId}@example.com" }));
// then, in the violation tests:
Assert.Contains($"{userId}@example.com", results[0].AsError.Message);
|
|
||
| public const string UserNotFoundErrorMessage = "User not found."; | ||
| public const string UsersInvalidErrorMessage = "Users invalid."; | ||
| public static readonly string UsersInvalidErrorMessage = new UsersInvalid().Message; |
There was a problem hiding this comment.
♻️ DEBT: The remove flow now imports the restore feature's namespace to reuse an identical "Users invalid." string.
Details
UsersInvalid is declared in RestoreUser/v1/Errors.cs, so this file now carries using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RestoreUser.v1; for an error that has nothing to do with restoring, and the rendered copy is unchanged either way. Two side effects:
UsersInvalidErrorMessagehad to dropconstforstatic readonlywhile its five neighbours stayconst, so it can no longer be used in constant contexts (caselabels, attributes).- Line 207 throws
new UsersInvalid().Messageinstead of the constant declared right here, leaving the public field consumed only byRemoveOrganizationUserCommandTests.
Declaring UsersInvalid in the OrganizationUsers-level Errors.cs (or leaving this constant as-is, since the text did not change) keeps the feature folders independent. The same shape shows up at OrganizationUsersController.cs:784, where the bulk enable-secrets-manager endpoint reaches into UpdateUser.v2's errors.
r-tome
left a comment
r-tome
left a comment
There was a problem hiding this comment.
Use organization.DisplayName() everywhere, otherwise looks good
| if (!InviteLinkDomainValidator.IsEmailDomainAllowed(user.Email, link.GetAllowedDomains())) | ||
| { | ||
| return new EmailDomainNotAllowed(); | ||
| return new EmailDomainNotAllowed(organization.Name); |
There was a problem hiding this comment.
We should always use DisplayName()
| return new EmailDomainNotAllowed(organization.Name); | |
| return new EmailDomainNotAllowed(organization.DisplayName()); |
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40744
📔 Objective
Updates copy of user facing server error messages for VFO1. For reviewers, the copy changes are organized by commit, with the latter commits being pure refactors to consolidate inline error strings into typed errors.
📸 Screenshots