From 71be3865ea8360257129ff2d55e503311af96826 Mon Sep 17 00:00:00 2001 From: Alex Morask <144709477+amorask-bitwarden@users.noreply.github.com> Date: Tue, 2 Dec 2025 10:16:37 -0600 Subject: [PATCH] [PM-24558] Remove FF: `pm-21821-provider-portal-takeover` (#6613) * Remove FF: pm-21821-provider-portal-takeover * Run dotnet format --- .../SubscriptionUpdatedHandler.cs | 78 +------- src/Core/Constants.cs | 1 - .../SubscriptionUpdatedHandlerTests.cs | 169 +----------------- 3 files changed, 11 insertions(+), 237 deletions(-) diff --git a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs index 81aeb460c2..07ffef064f 100644 --- a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs +++ b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs @@ -1,7 +1,5 @@ -using System.Globalization; -using Bit.Billing.Constants; +using Bit.Billing.Constants; using Bit.Billing.Jobs; -using Bit.Core; using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; @@ -134,11 +132,6 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler } case StripeSubscriptionStatus.Active when providerId.HasValue: { - var providerPortalTakeover = _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); - if (!providerPortalTakeover) - { - break; - } var provider = await _providerRepository.GetByIdAsync(providerId.Value); if (provider != null) { @@ -321,13 +314,6 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler Event parsedEvent, Subscription currentSubscription) { - var providerPortalTakeover = _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); - - if (!providerPortalTakeover) - { - return; - } - var provider = await _providerRepository.GetByIdAsync(providerId); if (provider == null) { @@ -343,22 +329,17 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler { var previousSubscription = parsedEvent.Data.PreviousAttributes.ToObject() as Subscription; - var updateIsSubscriptionGoingUnpaid = previousSubscription is - { - Status: + if (previousSubscription is + { + Status: StripeSubscriptionStatus.Trialing or StripeSubscriptionStatus.Active or StripeSubscriptionStatus.PastDue - } && currentSubscription is - { - Status: StripeSubscriptionStatus.Unpaid, - LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create" - }; - - var updateIsManualSuspensionViaMetadata = CheckForManualSuspensionViaMetadata( - previousSubscription, currentSubscription); - - if (updateIsSubscriptionGoingUnpaid || updateIsManualSuspensionViaMetadata) + } && currentSubscription is + { + Status: StripeSubscriptionStatus.Unpaid, + LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create" + }) { if (currentSubscription.TestClock != null) { @@ -369,14 +350,6 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler var subscriptionUpdateOptions = new SubscriptionUpdateOptions { CancelAt = now.AddDays(7) }; - if (updateIsManualSuspensionViaMetadata) - { - subscriptionUpdateOptions.Metadata = new Dictionary - { - ["suspended_provider_via_webhook_at"] = DateTime.UtcNow.ToString(CultureInfo.InvariantCulture) - }; - } - await _stripeFacade.UpdateSubscription(currentSubscription.Id, subscriptionUpdateOptions); } } @@ -399,37 +372,4 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler } } } - - private static bool CheckForManualSuspensionViaMetadata( - Subscription? previousSubscription, - Subscription currentSubscription) - { - /* - * When metadata on a subscription is updated, we'll receive an event that has: - * Previous Metadata: { newlyAddedKey: null } - * Current Metadata: { newlyAddedKey: newlyAddedValue } - * - * As such, our check for a manual suspension must ensure that the 'previous_attributes' does contain the - * 'metadata' property, but also that the "suspend_provider" key in that metadata is set to null. - * - * If we don't do this and instead do a null coalescing check on 'previous_attributes?.metadata?.TryGetValue', - * we'll end up marking an event where 'previous_attributes.metadata' = null (which could be any subscription update - * that does not update the metadata) the same as a manual suspension. - */ - const string key = "suspend_provider"; - - if (previousSubscription is not { Metadata: not null } || - !previousSubscription.Metadata.TryGetValue(key, out var previousValue)) - { - return false; - } - - if (previousValue == null) - { - return !string.IsNullOrEmpty( - currentSubscription.Metadata.TryGetValue(key, out var currentValue) ? currentValue : null); - } - - return false; - } } diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index af5b738cd0..e63d087863 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -188,7 +188,6 @@ public static class FeatureFlagKeys /* Billing Team */ public const string TrialPayment = "PM-8163-trial-payment"; - public const string PM21821_ProviderPortalTakeover = "pm-21821-provider-portal-takeover"; public const string PM22415_TaxIDWarnings = "pm-22415-tax-id-warnings"; public const string PM25379_UseNewOrganizationMetadataStructure = "pm-25379-use-new-organization-metadata-structure"; public const string PM24996ImplementUpgradeFromFreeDialog = "pm-24996-implement-upgrade-from-free-dialog"; diff --git a/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs b/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs index 83ebd4aaa7..4a480f8c30 100644 --- a/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs +++ b/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs @@ -1,7 +1,6 @@ using Bit.Billing.Constants; using Bit.Billing.Services; using Bit.Billing.Services.Implementations; -using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Entities.Provider; using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces; @@ -126,79 +125,6 @@ public class SubscriptionUpdatedHandlerTests Arg.Is(t => t.Key.Name == $"cancel-trigger-{subscriptionId}")); } - [Fact] - public async Task - HandleAsync_UnpaidProviderSubscription_WithManualSuspensionViaMetadata_DisablesProviderAndSchedulesCancellation() - { - // Arrange - var providerId = Guid.NewGuid(); - var subscriptionId = "sub_test123"; - - var previousSubscription = new Subscription - { - Id = subscriptionId, - Status = StripeSubscriptionStatus.Active, - Metadata = new Dictionary - { - ["suspend_provider"] = null // This is the key part - metadata exists, but value is null - } - }; - - var currentSubscription = new Subscription - { - Id = subscriptionId, - Status = StripeSubscriptionStatus.Unpaid, - Items = new StripeList - { - Data = - [ - new SubscriptionItem { CurrentPeriodEnd = DateTime.UtcNow.AddDays(30) } - ] - }, - Metadata = new Dictionary - { - ["providerId"] = providerId.ToString(), - ["suspend_provider"] = "true" // Now has a value, indicating manual suspension - }, - TestClock = null - }; - - var parsedEvent = new Event - { - Id = "evt_test123", - Type = HandledStripeWebhook.SubscriptionUpdated, - Data = new EventData - { - Object = currentSubscription, - PreviousAttributes = JObject.FromObject(previousSubscription) - } - }; - - var provider = new Provider { Id = providerId, Enabled = true }; - - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover).Returns(true); - _stripeEventService.GetSubscription(parsedEvent, true, Arg.Any>()).Returns(currentSubscription); - _stripeEventUtilityService.GetIdsFromMetadata(currentSubscription.Metadata) - .Returns(Tuple.Create(null, null, providerId)); - _providerRepository.GetByIdAsync(providerId).Returns(provider); - - // Act - await _sut.HandleAsync(parsedEvent); - - // Assert - Assert.False(provider.Enabled); - await _providerService.Received(1).UpdateAsync(provider); - - // Verify that UpdateSubscription was called with both CancelAt and the new metadata - await _stripeFacade.Received(1).UpdateSubscription( - subscriptionId, - Arg.Is(options => - options.CancelAt.HasValue && - options.CancelAt.Value <= DateTime.UtcNow.AddDays(7).AddMinutes(1) && - options.Metadata != null && - options.Metadata.ContainsKey("suspended_provider_via_webhook_at"))); - } - [Fact] public async Task HandleAsync_UnpaidProviderSubscription_WithValidTransition_DisablesProviderAndSchedulesCancellation() @@ -243,7 +169,6 @@ public class SubscriptionUpdatedHandlerTests var provider = new Provider { Id = providerId, Enabled = true }; - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover).Returns(true); _stripeEventService.GetSubscription(parsedEvent, true, Arg.Any>()).Returns(currentSubscription); _stripeEventUtilityService.GetIdsFromMetadata(currentSubscription.Metadata) .Returns(Tuple.Create(null, null, providerId)); @@ -256,13 +181,12 @@ public class SubscriptionUpdatedHandlerTests Assert.False(provider.Enabled); await _providerService.Received(1).UpdateAsync(provider); - // Verify that UpdateSubscription was called with CancelAt but WITHOUT suspension metadata + // Verify that UpdateSubscription was called with CancelAt await _stripeFacade.Received(1).UpdateSubscription( subscriptionId, Arg.Is(options => options.CancelAt.HasValue && - options.CancelAt.Value <= DateTime.UtcNow.AddDays(7).AddMinutes(1) && - (options.Metadata == null || !options.Metadata.ContainsKey("suspended_provider_via_webhook_at")))); + options.CancelAt.Value <= DateTime.UtcNow.AddDays(7).AddMinutes(1))); } [Fact] @@ -306,9 +230,6 @@ public class SubscriptionUpdatedHandlerTests _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) .Returns(Tuple.Create(null, null, providerId)); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); - _providerRepository.GetByIdAsync(providerId) .Returns(provider); @@ -353,9 +274,6 @@ public class SubscriptionUpdatedHandlerTests _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) .Returns(Tuple.Create(null, null, providerId)); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); - _providerRepository.GetByIdAsync(providerId) .Returns(provider); @@ -401,9 +319,6 @@ public class SubscriptionUpdatedHandlerTests _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) .Returns(Tuple.Create(null, null, providerId)); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); - _providerRepository.GetByIdAsync(providerId) .Returns(provider); @@ -416,48 +331,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade.DidNotReceive().UpdateSubscription(Arg.Any(), Arg.Any()); } - [Fact] - public async Task HandleAsync_UnpaidProviderSubscription_WhenFeatureFlagDisabled_DoesNothing() - { - // Arrange - var providerId = Guid.NewGuid(); - var subscriptionId = "sub_123"; - var currentPeriodEnd = DateTime.UtcNow.AddDays(30); - - var subscription = new Subscription - { - Id = subscriptionId, - Status = StripeSubscriptionStatus.Unpaid, - Items = new StripeList - { - Data = - [ - new SubscriptionItem { CurrentPeriodEnd = currentPeriodEnd } - ] - }, - Metadata = new Dictionary { { "providerId", providerId.ToString() } }, - LatestInvoice = new Invoice { BillingReason = "subscription_cycle" } - }; - - var parsedEvent = new Event { Data = new EventData() }; - - _stripeEventService.GetSubscription(Arg.Any(), Arg.Any(), Arg.Any>()) - .Returns(subscription); - - _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) - .Returns(Tuple.Create(null, null, providerId)); - - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(false); - - // Act - await _sut.HandleAsync(parsedEvent); - - // Assert - await _providerRepository.DidNotReceive().GetByIdAsync(Arg.Any()); - await _providerService.DidNotReceive().UpdateAsync(Arg.Any()); - } - [Fact] public async Task HandleAsync_UnpaidProviderSubscription_WhenProviderNotFound_DoesNothing() { @@ -489,9 +362,6 @@ public class SubscriptionUpdatedHandlerTests _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) .Returns(Tuple.Create(null, null, providerId)); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); - _providerRepository.GetByIdAsync(providerId) .Returns((Provider)null); @@ -777,8 +647,6 @@ public class SubscriptionUpdatedHandlerTests _stripeFacade .UpdateSubscription(Arg.Any(), Arg.Any()) .Returns(newSubscription); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -800,9 +668,6 @@ public class SubscriptionUpdatedHandlerTests .Received(1) .UpdateSubscription(newSubscription.Id, Arg.Is(options => options.CancelAtPeriodEnd == false)); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -823,8 +688,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .Returns(provider); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -843,9 +706,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceiveWithAnyArgs() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -866,8 +726,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .Returns(provider); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -886,9 +744,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceiveWithAnyArgs() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -909,8 +764,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .Returns(provider); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -929,9 +782,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceiveWithAnyArgs() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -953,8 +803,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .Returns(provider); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -975,9 +823,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceiveWithAnyArgs() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -997,8 +842,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .ReturnsNull(); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -1019,9 +862,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceive() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } [Fact] @@ -1040,8 +880,6 @@ public class SubscriptionUpdatedHandlerTests _providerRepository .GetByIdAsync(Arg.Any()) .Returns(provider); - _featureService.IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover) - .Returns(true); // Act await _sut.HandleAsync(parsedEvent); @@ -1062,9 +900,6 @@ public class SubscriptionUpdatedHandlerTests await _stripeFacade .DidNotReceive() .UpdateSubscription(Arg.Any()); - _featureService - .Received(1) - .IsEnabled(FeatureFlagKeys.PM21821_ProviderPortalTakeover); } private static (Guid providerId, Subscription newSubscription, Provider provider, Event parsedEvent)