[PM-37954] Add Command for Provisioning Staged OrganizationUsers#7859
[PM-37954] Add Command for Provisioning Staged OrganizationUsers#7859sven-bitwarden wants to merge 7 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7859 +/- ##
==========================================
- Coverage 65.83% 65.76% -0.07%
==========================================
Files 2214 2221 +7
Lines 98091 98102 +11
Branches 8856 8849 -7
==========================================
- Hits 64574 64513 -61
- Misses 31300 31375 +75
+ Partials 2217 2214 -3 ☔ 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?)
There was a problem hiding this comment.
They're already there (on the enum), but I did remove the extra details from this guy
| 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.
There was a problem hiding this comment.
Can do 👍 not fussed one way or the other
| Key = null, | ||
| Type = OrganizationUserType.User, | ||
| Status = OrganizationUserStatusType.Staged, | ||
| // StatusNew is intentionally left null - it is only populated by the revoke flow when |
There was a problem hiding this comment.
cliffhanger deleted: it just be populated by the revoke flow 😆
| // 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.
There was a problem hiding this comment.
I can totally add the API tests later and remove these, but it does feel like we're missing a capability to test commands. Repositories are generally the agnostic building blocks, but the parameters and order-of-operations validity (the broad business logic) comes from the commands
| // 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).