[EC-144] Fix stripe revert logic (#2014)

* Revert scaling by previous value

* Throw is Stripe subscription revert fails

* Remove unused property

* Add null check to accommodate for not existing storage-gb-xxx subscription item

* Use long? instead of Nullable<long>

* Remove redundant try/catch

* Ensure collectionMethod is changed back, even when revertSub fails

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

src/Core/Models/Business/SubscriptionUpdate.cs

@@ -1,4 +1,5 @@
-using System.Collections.Generic;
+using System;
+using System.Collections.Generic;
 using System.Linq;
 using Bit.Core.Entities;
 using Stripe;
@@ -34,16 +35,17 @@ namespace Bit.Core.Models.Business
 
     public class SeatSubscriptionUpdate : SubscriptionUpdate
     {
-        private readonly Organization _organization;
+        private readonly int _previousSeats;
         private readonly StaticStore.Plan _plan;
         private readonly long? _additionalSeats;
         protected override List<string> PlanIds => new() { _plan.StripeSeatPlanId };
 
+
         public SeatSubscriptionUpdate(Organization organization, StaticStore.Plan plan, long? additionalSeats)
         {
-            _organization = organization;
             _plan = plan;
             _additionalSeats = additionalSeats;
+            _previousSeats = organization.Seats ?? 0;
         }
 
         public override List<SubscriptionItemOptions> UpgradeItemsOptions(Subscription subscription)
@@ -63,6 +65,7 @@ namespace Bit.Core.Models.Business
 
         public override List<SubscriptionItemOptions> RevertItemsOptions(Subscription subscription)
         {
+
             var item = SubscriptionItem(subscription, PlanIds.Single());
             return new()
             {
@@ -70,8 +73,8 @@ namespace Bit.Core.Models.Business
                 {
                     Id = item?.Id,
                     Plan = PlanIds.Single(),
-                    Quantity = _organization.Seats,
+                    Quantity = _previousSeats,
-                    Deleted = item?.Id != null ? true : (bool?)null,
+                    Deleted = _previousSeats == 0 ? true : (bool?)null,
                 }
             };
         }
@@ -79,6 +82,7 @@ namespace Bit.Core.Models.Business
 
     public class StorageSubscriptionUpdate : SubscriptionUpdate
     {
+        private long? _prevStorage;
         private readonly string _plan;
         private readonly long? _additionalStorage;
         protected override List<string> PlanIds => new() { _plan };
@@ -92,6 +96,7 @@ namespace Bit.Core.Models.Business
         public override List<SubscriptionItemOptions> UpgradeItemsOptions(Subscription subscription)
         {
             var item = SubscriptionItem(subscription, PlanIds.Single());
+            _prevStorage = item?.Quantity ?? 0;
             return new()
             {
                 new SubscriptionItemOptions
@@ -106,6 +111,11 @@ namespace Bit.Core.Models.Business
 
         public override List<SubscriptionItemOptions> RevertItemsOptions(Subscription subscription)
         {
+            if (!_prevStorage.HasValue)
+            {
+                throw new Exception("Unknown previous value, must first call UpgradeItemsOptions");
+            }
+
             var item = SubscriptionItem(subscription, PlanIds.Single());
             return new()
             {
@@ -113,8 +123,8 @@ namespace Bit.Core.Models.Business
                 {
                     Id = item?.Id,
                     Plan = _plan,
-                    Quantity = item?.Quantity ?? 0,
+                    Quantity = _prevStorage.Value,
-                    Deleted = item?.Id != null ? true : (bool?)null,
+                    Deleted = _prevStorage.Value == 0 ? true : (bool?)null,
                 }
             };
         }

src/Core/Services/Implementations/StripePaymentService.cs

@@ -710,6 +710,8 @@ namespace Bit.Core.Services
         private async Task<string> FinalizeSubscriptionChangeAsync(IStorableSubscriber storableSubscriber,
             SubscriptionUpdate subscriptionUpdate, DateTime? prorationDate)
         {
+            // remember, when in doubt, throw
+
             var sub = await _stripeAdapter.SubscriptionGetAsync(storableSubscriber.GatewaySubscriptionId);
             if (sub == null)
             {
@@ -759,65 +761,71 @@ namespace Bit.Core.Services
                 }
             }
 
-            var subResponse = await _stripeAdapter.SubscriptionUpdateAsync(sub.Id, subUpdateOptions);
-
-            var invoice = await _stripeAdapter.InvoiceGetAsync(subResponse?.LatestInvoiceId, new Stripe.InvoiceGetOptions());
-            if (invoice == null)
-            {
-                throw new BadRequestException("Unable to locate draft invoice for subscription update.");
-            }
-
             string paymentIntentClientSecret = null;
-            if (invoice.AmountDue > 0 && updatedItemOptions.Any(i => i.Quantity > 0))
+            try
             {
-                try
+                var subResponse = await _stripeAdapter.SubscriptionUpdateAsync(sub.Id, subUpdateOptions);
+
+                var invoice = await _stripeAdapter.InvoiceGetAsync(subResponse?.LatestInvoiceId, new Stripe.InvoiceGetOptions());
+                if (invoice == null)
                 {
-                    if (chargeNow)
+                    throw new BadRequestException("Unable to locate draft invoice for subscription update.");
+                }
+
+                if (invoice.AmountDue > 0 && updatedItemOptions.Any(i => i.Quantity > 0))
+                {
+                    try
                     {
-                        paymentIntentClientSecret = await PayInvoiceAfterSubscriptionChangeAsync(
+                        if (chargeNow)
-                            storableSubscriber, invoice);
-                    }
-                    else
-                    {
-                        invoice = await _stripeAdapter.InvoiceFinalizeInvoiceAsync(subResponse.LatestInvoiceId, new Stripe.InvoiceFinalizeOptions
                         {
-                            AutoAdvance = false,
+                            paymentIntentClientSecret = await PayInvoiceAfterSubscriptionChangeAsync(
+                                storableSubscriber, invoice);
+                        }
+                        else
+                        {
+                            invoice = await _stripeAdapter.InvoiceFinalizeInvoiceAsync(subResponse.LatestInvoiceId, new Stripe.InvoiceFinalizeOptions
+                            {
+                                AutoAdvance = false,
+                            });
+                            await _stripeAdapter.InvoiceSendInvoiceAsync(invoice.Id, new Stripe.InvoiceSendOptions());
+                            paymentIntentClientSecret = null;
+                        }
+                    }
+                    catch
+                    {
+                        // Need to revert the subscription
+                        await _stripeAdapter.SubscriptionUpdateAsync(sub.Id, new Stripe.SubscriptionUpdateOptions
+                        {
+                            Items = subscriptionUpdate.RevertItemsOptions(sub),
+                            // This proration behavior prevents a false "credit" from
+                            //  being applied forward to the next month's invoice
+                            ProrationBehavior = "none",
+                            CollectionMethod = collectionMethod,
+                            DaysUntilDue = daysUntilDue,
                         });
-                        await _stripeAdapter.InvoiceSendInvoiceAsync(invoice.Id, new Stripe.InvoiceSendOptions());
+                        throw;
-                        paymentIntentClientSecret = null;
                     }
                 }
-                catch
+                else if (!invoice.Paid)
+                {
+                    // Pay invoice with no charge to customer this completes the invoice immediately without waiting the scheduled 1h
+                    invoice = await _stripeAdapter.InvoicePayAsync(subResponse.LatestInvoiceId);
+                    paymentIntentClientSecret = null;
+                }
+
+            }
+            finally
+            {
+                // Change back the subscription collection method and/or days until due
+                if (collectionMethod != "send_invoice" || daysUntilDue == null)
                 {
-                    // Need to revert the subscription
                     await _stripeAdapter.SubscriptionUpdateAsync(sub.Id, new Stripe.SubscriptionUpdateOptions
                     {
-                        Items = subscriptionUpdate.RevertItemsOptions(sub),
-                        // This proration behavior prevents a false "credit" from
-                        //  being applied forward to the next month's invoice
-                        ProrationBehavior = "none",
                         CollectionMethod = collectionMethod,
                         DaysUntilDue = daysUntilDue,
                     });
-                    throw;
                 }
             }
-            else if (!invoice.Paid)
-            {
-                // Pay invoice with no charge to customer this completes the invoice immediately without waiting the scheduled 1h
-                invoice = await _stripeAdapter.InvoicePayAsync(subResponse.LatestInvoiceId);
-                paymentIntentClientSecret = null;
-            }
-
-            // Change back the subscription collection method and/or days until due
-            if (collectionMethod != "send_invoice" || daysUntilDue == null)
-            {
-                await _stripeAdapter.SubscriptionUpdateAsync(sub.Id, new Stripe.SubscriptionUpdateOptions
-                {
-                    CollectionMethod = collectionMethod,
-                    DaysUntilDue = daysUntilDue,
-                });
-            }
 
             return paymentIntentClientSecret;
         }