From 74178c1c9c5fafddf7e7b402fb33c98a2065c0e6 Mon Sep 17 00:00:00 2001 From: Cy Okeke Date: Wed, 21 Jan 2026 17:06:00 +0100 Subject: [PATCH] Resolve null checks --- .../SubscriptionUpdatedHandler.cs | 36 +------- .../Implementations/SubscriberService.cs | 87 +++++++++---------- 2 files changed, 45 insertions(+), 78 deletions(-) diff --git a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs index 483d242d6d..8645ba91be 100644 --- a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs +++ b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs @@ -208,23 +208,12 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler private async Task IsPremiumSubscriptionAsync(Subscription subscription) { - if (subscription.Items == null || !subscription.Items.Any()) - { - return false; - } - var premiumPlans = await _pricingClient.ListPremiumPlans(); - if (premiumPlans == null || !premiumPlans.Any()) - { - return false; - } - var premiumPriceIds = premiumPlans - .Where(p => p.Seat != null && p.Storage != null) .SelectMany(p => new[] { p.Seat.StripePriceId, p.Storage.StripePriceId }) .ToHashSet(); - return subscription.Items.Any(i => i.Price != null && premiumPriceIds.Contains(i.Price.Id)); + return subscription.Items.Any(i => premiumPriceIds.Contains(i.Price.Id)); } /// @@ -294,13 +283,7 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler // Get all plan IDs that include Secrets Manager support to check if the organization has secret manager in the // previous and/or current subscriptions. - var plans = await _pricingClient.ListPlans(); - if (plans == null) - { - return; - } - - var planIdsOfPlansWithSecretManager = plans + var planIdsOfPlansWithSecretManager = (await _pricingClient.ListPlans()) .Where(orgPlan => orgPlan.SupportsSecretsManager && orgPlan.SecretsManager.StripeSeatPlanId != null) .Select(orgPlan => orgPlan.SecretsManager.StripeSeatPlanId) .ToHashSet(); @@ -314,7 +297,6 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler previousSubscriptionItem => planIdsOfPlansWithSecretManager.Contains(previousSubscriptionItem.Plan.Id)); var currentSubscriptionHasSecretsManager = - subscription.Items is not null && subscription.Items.Any( currentSubscriptionItem => planIdsOfPlansWithSecretManager.Contains(currentSubscriptionItem.Plan.Id)); @@ -456,22 +438,8 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler return; } - // STEP 5: Check if subscription still has OrganizationId (race condition check) - // If reversion already happened, OrganizationId would be removed and we should skip cleanup - if (!subscription.Metadata.ContainsKey(StripeConstants.MetadataKeys.OrganizationId)) - { - _logger.LogInformation( - "Skipping cleanup for subscription {SubscriptionId} - appears to have been reverted already", - subscription.Id); - return; - } - try { - _logger.LogInformation( - "Cleaning up premium upgrade metadata for subscription {SubscriptionId} after trial ended", - subscription.Id); - // Remove all premium upgrade metadata keys while preserving other metadata var updatedMetadata = new Dictionary(subscription.Metadata); updatedMetadata.Remove(StripeConstants.MetadataKeys.PreviousPremiumPriceId); diff --git a/src/Core/Billing/Services/Implementations/SubscriberService.cs b/src/Core/Billing/Services/Implementations/SubscriberService.cs index 73c143cca4..564cb4bebe 100644 --- a/src/Core/Billing/Services/Implementations/SubscriberService.cs +++ b/src/Core/Billing/Services/Implementations/SubscriberService.cs @@ -62,9 +62,10 @@ public class SubscriberService( // Check if this is a Premium-to-Organization upgrade that can be reverted if (subscriber is Organization organization) { - var canRevert = await TryRevertPremiumUpgradeAsync(subscription, organization); + var canRevert = await CanRevertPremiumUpgradeAsync(subscription, organization); if (canRevert) { + await RevertPremiumUpgradeAsync(subscription, organization); return; } } @@ -87,7 +88,7 @@ public class SubscriberService( if (cancelImmediately) { - if (subscription.Metadata.ContainsKey("organizationId")) + if (subscription.Metadata.ContainsKey(StripeConstants.MetadataKeys.OrganizationId)) { await stripeAdapter.UpdateSubscriptionAsync(subscription.Id, new SubscriptionUpdateOptions { @@ -975,43 +976,31 @@ public class SubscriberService( } /// - /// Attempts to revert a Premium-to-Organization upgrade by checking for premium upgrade metadata - /// and reverting the subscription to the original Premium plan if found. + /// Checks if a Premium-to-Organization upgrade can be reverted by validating the subscription metadata + /// and organization match. /// /// The Stripe subscription to check /// The organization attempting to cancel - /// True if the reversion was successful, false if no reversion metadata was found - private async Task TryRevertPremiumUpgradeAsync(Subscription subscription, Organization organization) + /// True if the reversion is possible, false otherwise + private Task CanRevertPremiumUpgradeAsync(Subscription subscription, Organization organization) { // Extract all metadata once var metadata = subscription.Metadata; - // VALIDATION PHASE: Quick checks to determine if reversion is applicable - // These checks return false (no error) because they indicate "reversion not needed" - // rather than "reversion failed" - the cancellation should proceed normally. - - // Check if metadata exists - if (metadata == null) - { - logger.LogDebug("Subscription {SubscriptionId} has no metadata", subscription.Id); - return false; - } - // Check if subscription has the premium upgrade metadata - if (!metadata.TryGetValue(MetadataKeys.PreviousPremiumPriceId, out var previousPremiumPriceId) || + if (!metadata.TryGetValue(MetadataKeys.PreviousPremiumPriceId, out _) || !metadata.TryGetValue(MetadataKeys.UpgradedOrganizationId, out var upgradedOrganizationIdStr) || - !metadata.TryGetValue(MetadataKeys.PreviousPremiumUserId, out var previousPremiumUserIdStr)) + !metadata.TryGetValue(MetadataKeys.PreviousPremiumUserId, out _)) { logger.LogDebug("Subscription {SubscriptionId} does not have premium upgrade metadata", subscription.Id); - return false; + return Task.FromResult(false); } // Parse IDs once - if (!Guid.TryParse(upgradedOrganizationIdStr, out var upgradedOrganizationId) || - !Guid.TryParse(previousPremiumUserIdStr, out var previousPremiumUserId)) + if (!Guid.TryParse(upgradedOrganizationIdStr, out var upgradedOrganizationId)) { logger.LogWarning("Invalid GUID format in premium upgrade metadata for subscription {SubscriptionId}", subscription.Id); - return false; + return Task.FromResult(false); } // Verify that this is the organization that was upgraded @@ -1020,7 +1009,7 @@ public class SubscriberService( logger.LogWarning( "Organization {OrganizationId} attempted to revert subscription {SubscriptionId} that belongs to organization {ActualOrganizationId}", organization.Id, subscription.Id, upgradedOrganizationId); - return false; + return Task.FromResult(false); } // Verify subscription is in trial - reversion only allowed during trial period @@ -1029,13 +1018,33 @@ public class SubscriberService( logger.LogWarning( "Cannot revert subscription {SubscriptionId} - not in trial period (current status: {Status})", subscription.Id, subscription.Status); - return false; + return Task.FromResult(false); + } + + return Task.FromResult(true); + } + + /// + /// Reverts a Premium-to-Organization upgrade by restoring the subscription to the original Premium plan. + /// This method should only be called after CanRevertPremiumUpgradeAsync returns true. + /// + /// The Stripe subscription to revert + /// The organization whose subscription is being reverted + /// Thrown if the reversion fails + private async Task RevertPremiumUpgradeAsync(Subscription subscription, Organization organization) + { + var metadata = subscription.Metadata; + + // Extract required metadata values (we know they exist from CanRevertPremiumUpgradeAsync) + var previousPremiumPriceId = metadata[MetadataKeys.PreviousPremiumPriceId]; + var previousPremiumUserIdStr = metadata[MetadataKeys.PreviousPremiumUserId]; + + if (!Guid.TryParse(previousPremiumUserIdStr, out var previousPremiumUserId)) + { + logger.LogError("Invalid GUID format for previous premium user ID in subscription {SubscriptionId}", subscription.Id); + throw new BillingException("Invalid premium upgrade metadata"); } - // REVERSION PHASE: At this point we've determined reversion IS applicable and should happen. - // Any failures from here on throw exceptions (not return false) because they indicate - // a serious error that should fail the cancellation - we don't want to cancel an org - // subscription if we can't properly revert it back to the user. try { logger.LogInformation("Reverting Premium-to-Organization upgrade for organization {OrganizationId}", organization.Id); @@ -1124,9 +1133,6 @@ public class SubscriberService( // Ensure user ID is in metadata updatedMetadata[MetadataKeys.UserId] = previousPremiumUserId.ToString(); - // Remove organization ID from metadata since this is now a user subscription - updatedMetadata.Remove(MetadataKeys.OrganizationId); - // STEP 6: Update Stripe subscription var updateOptions = new SubscriptionUpdateOptions { @@ -1147,27 +1153,20 @@ public class SubscriberService( await userRepository.ReplaceAsync(user); - // STEP 8: Clear organization's subscription reference + // STEP 8: Clear organization's gateway references and disable it // NOTE: The organization entity itself is NOT deleted. It remains in the database with: + // - GatewayCustomerId set to null (no gateway customer) // - GatewaySubscriptionId set to null (no active subscription) + // - Enabled set to false (organization is disabled) // - All existing data (vaults, members, etc.) preserved - // - Effectively becomes a "Free" organization (no billing) - // - // TODO: Discuss with @micahblut about whether we should: - // 1. Explicitly transition the organization to a Free plan, or - // 2. Delete the organization entirely, or - // 3. Leave it as-is (current behavior) - // - // Current behavior: Organization remains but has no active subscription, making it - // effectively unusable for new billing operations but data is preserved. + organization.GatewayCustomerId = null; organization.GatewaySubscriptionId = null; + organization.Enabled = false; await organizationRepository.ReplaceAsync(organization); logger.LogInformation( "Successfully reverted Premium-to-Organization upgrade for organization {OrganizationId}. Subscription {SubscriptionId} restored to user {UserId}", organization.Id, subscription.Id, previousPremiumUserId); - - return true; } catch (Exception ex) {