[PM-37954] Add Command for Provisioning Staged OrganizationUsers#7859
[PM-37954] Add Command for Provisioning Staged OrganizationUsers#7859sven-bitwarden wants to merge 2 commits into
Conversation
sven-bitwarden
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review Details
No inline comments posted; the above is a minor, non-blocking note. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7859 +/- ##
==========================================
- Coverage 65.83% 65.77% -0.06%
==========================================
Files 2214 2220 +6
Lines 98091 98092 +1
Branches 8856 8849 -7
==========================================
- Hits 64574 64520 -54
- Misses 31300 31360 +60
+ Partials 2217 2212 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| /// Staged users are placeholders created by automated directory sync (SCIM / Directory Connector): they do not | ||
| /// yet consume a seat, are not subject to organization policies, and have not been sent an invitation. |
There was a problem hiding this comment.
I think these 2 lines belong on the enum definition rather than here - it is defining what a Staged user is. (While you're at it - can you add documentation to the other enum values as well?)
| Task<ICollection<OrganizationUser>> ProvisionStagedOrganizationUsersAsync( | ||
| Organization organization, | ||
| IEnumerable<(string Email, string ExternalId)> users, | ||
| EventSystemUser eventSystemUser); |
There was a problem hiding this comment.
Command conventions (undocumented, sorry! working to fix this):
- method should be called RunAsync
- should use a dedicated request object
- should return a v2 CommandResult
| /// Staged users are placeholders created by automated directory sync (SCIM / Directory Connector): they do not | ||
| /// yet consume a seat, are not subject to organization policies, and have not been sent an invitation. | ||
| /// </summary> | ||
| public interface IProvisionStagedOrganizationUsersCommand |
There was a problem hiding this comment.
(non-blocking, not strongly held): sticking to CRUD verbs might be clearer than Provision. e.g. CreateStagedOrganizationUserCommand.
| Key = null, | ||
| Type = OrganizationUserType.User, | ||
| Status = OrganizationUserStatusType.Staged, | ||
| // StatusNew is intentionally left null - it is only populated by the revoke flow when |
| // The command is a thin orchestration over the repository; event emission is covered by unit tests, | ||
| // so a no-op event service keeps this integration test focused on real database persistence. | ||
| var sut = new ProvisionStagedOrganizationUsersCommand(organizationUserRepository, new NoopEventService()); |
There was a problem hiding this comment.
This project is for integration testing against the database, so it should call repositories directly, not Core methods. Here you're not adding a new repository method, you're re-using CreateManyAsync, so no provision-specific db tests are required. That said, you could make sure that CreateManyAsync is tested.
When you add the api endpoint, you can also add api integration tests which will hit the whole stack.
| // Each row is actually persisted as a Staged member with the expected shape. | ||
| foreach (var created in result) | ||
| { | ||
| var persisted = await organizationUserRepository.GetByIdAsync(created.Id); |
There was a problem hiding this comment.
There is a GetManyAsync method.
2 participants
🎟️ Tracking
PM-37954
📔 Objective
Staged users need merely a fraction of the validation present for invited users - they're lightweight placeholders. Their validation will take place when the are promoted from Staged -> Invited (far future), or Staged -> Accepted (via invite link).