-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-12490] Extract OrganizationService.EnableAsync into commands #5321
base: main
Are you sure you want to change the base?
Conversation
…Command for organization enabling
# Conflicts: # src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs
d3b751a
to
4c3c230
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5321 +/- ##
==========================================
+ Coverage 44.37% 44.39% +0.02%
==========================================
Files 1487 1489 +2
Lines 68595 68602 +7
Branches 6179 6179
==========================================
+ Hits 30438 30459 +21
+ Misses 36838 36824 -14
Partials 1319 1319 ☔ View full report in Codecov by Sentry. |
Great job, no security vulnerabilities found in this Pull Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from Billing
# Conflicts: # src/Core/AdminConsole/Services/IOrganizationService.cs # src/Core/AdminConsole/Services/Implementations/OrganizationService.cs # src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs
56d8794
I had to fix a few merge conflicts |
.../Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationEnableCommandTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, just one minor request with the test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
public async Task EnableAsync(Guid organizationId) | ||
{ | ||
var organization = await _organizationRepository.GetByIdAsync(organizationId); | ||
if (organization is { Enabled: false }) | ||
{ | ||
organization.Enabled = true; | ||
|
||
await _organizationRepository.ReplaceAsync(organization); | ||
await _applicationCacheService.UpsertOrganizationAbilityAsync(organization); | ||
} | ||
} | ||
|
||
public async Task EnableAsync(Guid organizationId, DateTime? expirationDate) | ||
{ | ||
var organization = await _organizationRepository.GetByIdAsync(organizationId); | ||
if (organization is { Enabled: false, Gateway: not null }) | ||
{ | ||
organization.Enabled = true; | ||
organization.ExpirationDate = expirationDate; | ||
organization.RevisionDate = DateTime.UtcNow; | ||
|
||
await _organizationRepository.ReplaceAsync(organization); | ||
await _applicationCacheService.UpsertOrganizationAbilityAsync(organization); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving a non-blocking drive-by-nit ⛏️ before approving! Looking at these two methods, it feels like they can be combined into a single method. I don't have all the context around this, so there's probably a reason they're not! Something like this is what I was imagining:
public async Task EnableAsync(Guid organizationId, DateTime? expirationDate = null)
{
var organization = await _organizationRepository.GetByIdAsync(organizationId);
if (organization is null || organization.Enabled || expirationDate is not null && organization.Gateway is null)
{
return;
}
organization.Enabled = true;
if (expirationDate is not null && organization.Gateway is not null)
{
organization.ExpirationDate = expirationDate;
organization.RevisionDate = DateTime.UtcNow;
}
await _organizationRepository.ReplaceAsync(organization);
await _applicationCacheService.UpsertOrganizationAbilityAsync(organization);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking! I've pushed a commit with your suggestion. I also added an extra unit test just to make sure we're not breaking anything.
…d optional expiration
bf7871c
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12490
📔 Objective
Extract
OrganizationService.EnableAsync
toOrganizationEnableCommand
.Add unit tests for OrganizationEnableCommand.
⏰ 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