Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
๏ปฟusing Bit.Billing.Constants;
using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Billing.Enums;
using Bit.Core.Context;
Expand All @@ -17,7 +18,6 @@
{
private readonly ILogger<PaymentSucceededHandler> _logger;
private readonly IStripeEventService _stripeEventService;
private readonly IOrganizationService _organizationService;
private readonly IUserService _userService;
private readonly IStripeFacade _stripeFacade;
private readonly IProviderRepository _providerRepository;
Expand All @@ -27,6 +27,7 @@
private readonly IUserRepository _userRepository;
private readonly IStripeEventUtilityService _stripeEventUtilityService;
private readonly IPushNotificationService _pushNotificationService;
private readonly IOrganizationEnableCommand _organizationEnableCommand;

public PaymentSucceededHandler(
ILogger<PaymentSucceededHandler> logger,
Expand All @@ -39,8 +40,8 @@
IUserRepository userRepository,
IStripeEventUtilityService stripeEventUtilityService,
IUserService userService,
IOrganizationService organizationService,
IPushNotificationService pushNotificationService)
IPushNotificationService pushNotificationService,
IOrganizationEnableCommand organizationEnableCommand)

Check warning on line 44 in src/Billing/Services/Implementations/PaymentSucceededHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/PaymentSucceededHandler.cs#L43-L44

Added lines #L43 - L44 were not covered by tests
{
_logger = logger;
_stripeEventService = stripeEventService;
Expand All @@ -52,8 +53,8 @@
_userRepository = userRepository;
_stripeEventUtilityService = stripeEventUtilityService;
_userService = userService;
_organizationService = organizationService;
_pushNotificationService = pushNotificationService;
_organizationEnableCommand = organizationEnableCommand;

Check warning on line 57 in src/Billing/Services/Implementations/PaymentSucceededHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/PaymentSucceededHandler.cs#L57

Added line #L57 was not covered by tests
}

/// <summary>
Expand Down Expand Up @@ -142,7 +143,7 @@
return;
}

await _organizationService.EnableAsync(organizationId.Value, subscription.CurrentPeriodEnd);
await _organizationEnableCommand.EnableAsync(organizationId.Value, subscription.CurrentPeriodEnd);

Check warning on line 146 in src/Billing/Services/Implementations/PaymentSucceededHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/PaymentSucceededHandler.cs#L146

Added line #L146 was not covered by tests
var organization = await _organizationRepository.GetByIdAsync(organizationId.Value);
await _pushNotificationService.PushSyncOrganizationStatusAsync(organization);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
๏ปฟusing Bit.Billing.Constants;
using Bit.Billing.Jobs;
using Bit.Core;
using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces;
using Bit.Core.OrganizationFeatures.OrganizationSponsorships.FamiliesForEnterprise.Interfaces;
using Bit.Core.Platform.Push;
using Bit.Core.Repositories;
Expand All @@ -24,6 +25,7 @@
private readonly IOrganizationRepository _organizationRepository;
private readonly ISchedulerFactory _schedulerFactory;
private readonly IFeatureService _featureService;
private readonly IOrganizationEnableCommand _organizationEnableCommand;

public SubscriptionUpdatedHandler(
IStripeEventService stripeEventService,
Expand All @@ -35,7 +37,8 @@
IPushNotificationService pushNotificationService,
IOrganizationRepository organizationRepository,
ISchedulerFactory schedulerFactory,
IFeatureService featureService)
IFeatureService featureService,
IOrganizationEnableCommand organizationEnableCommand)

Check warning on line 41 in src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs#L40-L41

Added lines #L40 - L41 were not covered by tests
{
_stripeEventService = stripeEventService;
_stripeEventUtilityService = stripeEventUtilityService;
Expand All @@ -47,6 +50,7 @@
_organizationRepository = organizationRepository;
_schedulerFactory = schedulerFactory;
_featureService = featureService;
_organizationEnableCommand = organizationEnableCommand;

Check warning on line 53 in src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs#L53

Added line #L53 was not covered by tests
}

/// <summary>
Expand Down Expand Up @@ -90,7 +94,7 @@
}
case StripeSubscriptionStatus.Active when organizationId.HasValue:
{
await _organizationService.EnableAsync(organizationId.Value);
await _organizationEnableCommand.EnableAsync(organizationId.Value);

Check warning on line 97 in src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs#L97

Added line #L97 was not covered by tests
var organization = await _organizationRepository.GetByIdAsync(organizationId.Value);
await _pushNotificationService.PushSyncOrganizationStatusAsync(organization);
break;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
๏ปฟnamespace Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces;

public interface IOrganizationEnableCommand
{
/// <summary>
/// Enables an organization if it is currently disabled.
/// </summary>
/// <param name="organizationId">The unique identifier of the organization to enable.</param>
Task EnableAsync(Guid organizationId);

/// <summary>
/// Enables an organization with a specified expiration date if it is currently disabled and has a gateway configured.
/// </summary>
/// <param name="organizationId">The unique identifier of the organization to enable.</param>
/// <param name="expirationDate">The optional expiration date when the organization's subscription will expire.</param>
Task EnableAsync(Guid organizationId, DateTime? expirationDate);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
๏ปฟusing Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces;
using Bit.Core.Repositories;
using Bit.Core.Services;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Organizations;

public class OrganizationEnableCommand : IOrganizationEnableCommand
{
private readonly IApplicationCacheService _applicationCacheService;
private readonly IOrganizationRepository _organizationRepository;

public OrganizationEnableCommand(
IApplicationCacheService applicationCacheService,
IOrganizationRepository organizationRepository)
{
_applicationCacheService = applicationCacheService;
_organizationRepository = organizationRepository;
}

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);
}
}
Copy link
Contributor

@cturnbull-bitwarden cturnbull-bitwarden Feb 3, 2025

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);
}

Copy link
Contributor Author

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.

}
2 changes: 0 additions & 2 deletions src/Core/AdminConsole/Services/IOrganizationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@ Task ReplacePaymentMethodAsync(Guid organizationId, string paymentToken, Payment
string ownerKey, string collectionName, string publicKey, string privateKey);
Task InitiateDeleteAsync(Organization organization, string orgAdminEmail);
Task DeleteAsync(Organization organization);
Task EnableAsync(Guid organizationId, DateTime? expirationDate);
Task DisableAsync(Guid organizationId, DateTime? expirationDate);
Task UpdateExpirationDateAsync(Guid organizationId, DateTime? expirationDate);
Task EnableAsync(Guid organizationId);
Task UpdateAsync(Organization organization, bool updateBilling = false, EventType eventType = EventType.Organization_Updated);
Task UpdateTwoFactorProviderAsync(Organization organization, TwoFactorProviderType type);
Task DisableTwoFactorProviderAsync(Organization organization, TwoFactorProviderType type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -728,18 +728,6 @@ await _referenceEventService.RaiseEventAsync(
await _applicationCacheService.DeleteOrganizationAbilityAsync(organization.Id);
}

public async Task EnableAsync(Guid organizationId, DateTime? expirationDate)
{
var org = await GetOrgById(organizationId);
if (org != null && !org.Enabled && org.Gateway.HasValue)
{
org.Enabled = true;
org.ExpirationDate = expirationDate;
org.RevisionDate = DateTime.UtcNow;
await ReplaceAndUpdateCacheAsync(org);
}
}

public async Task DisableAsync(Guid organizationId, DateTime? expirationDate)
{
var org = await GetOrgById(organizationId);
Expand All @@ -765,16 +753,6 @@ public async Task UpdateExpirationDateAsync(Guid organizationId, DateTime? expir
}
}

public async Task EnableAsync(Guid organizationId)
{
var org = await GetOrgById(organizationId);
if (org != null && !org.Enabled)
{
org.Enabled = true;
await ReplaceAndUpdateCacheAsync(org);
}
}

public async Task UpdateAsync(Organization organization, bool updateBilling = false, EventType eventType = EventType.Organization_Updated)
{
if (organization.Id == default(Guid))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Organizations;
using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
Expand Down Expand Up @@ -52,6 +53,7 @@ public static void AddOrganizationServices(this IServiceCollection services, IGl
services.AddOrganizationLicenseCommandsQueries();
services.AddOrganizationDomainCommandsQueries();
services.AddOrganizationSignUpCommands();
services.AddOrganizationEnableCommands();
services.AddOrganizationAuthCommands();
services.AddOrganizationUserCommands();
services.AddOrganizationUserCommandsQueries();
Expand All @@ -61,6 +63,9 @@ public static void AddOrganizationServices(this IServiceCollection services, IGl
private static IServiceCollection AddOrganizationSignUpCommands(this IServiceCollection services) =>
services.AddScoped<ICloudOrganizationSignUpCommand, CloudOrganizationSignUpCommand>();

private static void AddOrganizationEnableCommands(this IServiceCollection services) =>
services.AddScoped<IOrganizationEnableCommand, OrganizationEnableCommand>();

private static void AddOrganizationConnectionCommands(this IServiceCollection services)
{
services.AddScoped<ICreateOrganizationConnectionCommand, CreateOrganizationConnectionCommand>();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
๏ปฟusing Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.OrganizationFeatures.Organizations;
using Bit.Core.Enums;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using NSubstitute;
using Xunit;

namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Organizations;

[SutProviderCustomize]
public class OrganizationEnableCommandTests
{
[Theory, BitAutoData]
public async Task EnableAsync_WhenOrganizationDoesNotExist_DoesNothing(
Guid organizationId,
SutProvider<OrganizationEnableCommand> sutProvider)
{
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organizationId)
.Returns((Organization)null);

await sutProvider.Sut.EnableAsync(organizationId);

await sutProvider.GetDependency<IOrganizationRepository>()
.DidNotReceive()
.ReplaceAsync(Arg.Any<Organization>());
await sutProvider.GetDependency<IApplicationCacheService>()
.DidNotReceive()
.UpsertOrganizationAbilityAsync(Arg.Any<Organization>());
}

[Theory, BitAutoData]
public async Task EnableAsync_WhenOrganizationAlreadyEnabled_DoesNothing(
Organization organization,
SutProvider<OrganizationEnableCommand> sutProvider)
{
organization.Enabled = true;
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);

await sutProvider.Sut.EnableAsync(organization.Id);

await sutProvider.GetDependency<IOrganizationRepository>()
.DidNotReceive()
.ReplaceAsync(Arg.Any<Organization>());
await sutProvider.GetDependency<IApplicationCacheService>()
.DidNotReceive()
.UpsertOrganizationAbilityAsync(Arg.Any<Organization>());
}

[Theory, BitAutoData]
public async Task EnableAsync_WhenOrganizationDisabled_EnablesAndSaves(
Organization organization,
SutProvider<OrganizationEnableCommand> sutProvider)
{
organization.Enabled = false;
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);

await sutProvider.Sut.EnableAsync(organization.Id);

Assert.True(organization.Enabled);
await sutProvider.GetDependency<IOrganizationRepository>()
.Received(1)
.ReplaceAsync(organization);
await sutProvider.GetDependency<IApplicationCacheService>()
.Received(1)
.UpsertOrganizationAbilityAsync(organization);
}

[Theory, BitAutoData]
public async Task EnableAsync_WithExpiration_WhenOrganizationHasNoGateway_DoesNothing(
Organization organization,
DateTime expirationDate,
SutProvider<OrganizationEnableCommand> sutProvider)
{
organization.Enabled = false;
organization.Gateway = null;
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);

await sutProvider.Sut.EnableAsync(organization.Id, expirationDate);

await sutProvider.GetDependency<IOrganizationRepository>()
.DidNotReceive()
.ReplaceAsync(Arg.Any<Organization>());
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
await sutProvider.GetDependency<IApplicationCacheService>()
.DidNotReceive()
.UpsertOrganizationAbilityAsync(Arg.Any<Organization>());
}

[Theory, BitAutoData]
public async Task EnableAsync_WithExpiration_WhenValid_EnablesAndSetsExpiration(
Organization organization,
DateTime expirationDate,
SutProvider<OrganizationEnableCommand> sutProvider)
{
organization.Enabled = false;
organization.Gateway = GatewayType.Stripe;
organization.RevisionDate = DateTime.UtcNow.AddDays(-1);
var originalRevisionDate = organization.RevisionDate;
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);

await sutProvider.Sut.EnableAsync(organization.Id, expirationDate);

Assert.True(organization.Enabled);
Assert.Equal(expirationDate, organization.ExpirationDate);
Assert.True(organization.RevisionDate > originalRevisionDate);
await sutProvider.GetDependency<IOrganizationRepository>()
.Received(1)
.ReplaceAsync(organization);
await sutProvider.GetDependency<IApplicationCacheService>()
.Received(1)
.UpsertOrganizationAbilityAsync(organization);
}
}
Loading