[PM-19332] Create InitPendingOrganizationCommand #5584
Conversation
| private async Task<bool> CanUpdateAsync(Guid organizationId) | ||
| { | ||
| var organization = _currentContext.GetOrganization(organizationId); | ||
| if (organization != null) |
There was a problem hiding this comment.
Not fully sure which checks here would really be best
|
Fixed Issues (4)Great job! The following issues were fixed in this Pull Request
|
| @@ -173,6 +174,7 @@ private static void AddOrganizationUserCommandsQueries(this IServiceCollection s | |||
|
|
|||
| services.AddScoped<IAuthorizationHandler, OrganizationUserUserMiniDetailsAuthorizationHandler>(); | |||
| services.AddScoped<IAuthorizationHandler, OrganizationUserUserDetailsAuthorizationHandler>(); | |||
| services.AddScoped<IAuthorizationHandler, OrganizationAuthorizationHandler>(); | |||
There was a problem hiding this comment.
I don't think this goes here... Which method should contain this, AddOrganizationAuthCommands?
|
@jrmccannon @eliykat Adding you both to this draft for some initial thoughts on this. Not super familiar with dotnet authorization so please feel free to correct me on anything that's just plain wrong. Ignore the current lack of tests and abundance of failures |
|
I would say lets hold off on adding any |
|
I agree with Jared. Let's do a simple cut & paste first just to physically move the existing code and update callers. Then once that's merged into main we can revisit the rewrite here. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5584 +/- ##
==========================================
+ Coverage 44.90% 45.04% +0.14%
==========================================
Files 1559 1563 +4
Lines 71128 71513 +385
Branches 6353 6403 +50
==========================================
+ Hits 31942 32216 +274
- Misses 37835 37931 +96
- Partials 1351 1366 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| await _collectionRepository.CreateAsync(defaultCollection, null, defaultOwnerAccess); | ||
| } | ||
|
|
||
| return new CommandResult(); |
There was a problem hiding this comment.
If you're going to return a CommandResult. Then I would suggest you return a Success<InitializePendingOrganizationResponse> and create a new empty record model InitializePendingOrganizationResponse.
This way if we want to return something later, we can simply add a property to the model and populate it.
src/Core/AdminConsole/OrganizationFeatures/Organizations/InitPendingOrganizationCommand.cs
Show resolved
Hide resolved
|
| Task ReplaceAndUpdateCacheAsync(Organization org, EventType? orgEvent = null); | ||
|
|
||
| void ValidatePasswordManagerPlan(Models.StaticStore.Plan plan, OrganizationUpgrade upgrade); | ||
| void ValidateSecretsManagerPlan(Models.StaticStore.Plan plan, OrganizationUpgrade upgrade); | ||
| Task ValidateOrganizationUserUpdatePermissions(Guid organizationId, OrganizationUserType newType, | ||
| OrganizationUserType? oldType, Permissions permissions); | ||
| Task ValidateOrganizationCustomPermissionsEnabledAsync(Guid organizationId, OrganizationUserType newType); | ||
| Task ValidateSignUpPoliciesAsync(Guid ownerId); |
There was a problem hiding this comment.
This method is just a single policy check - I don't think it needs to be reused. I suggest copying it into the new command rather than exposing it as public here. One less tie back to OrganizationService. (It's also what we did in the cloud org signup command)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19332
📔 Objective
Simple lift and shift of the InitPendingOrg method into a command.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes