Skip to content

Commit 84521a6

Browse files
[PM-30908]Correct Premium subscription status handling (#6877)
* Implement the correct changes * failing test has been removed * Add unit testing and logs * Resolve the pr comment on missed requirements * fix the lint error * resolve the build lint * Fix the failing test * Fix the failing test * Add the IncompleteExpired status * resolve the lint error * Fix the build lint error * Implement the IncompleteExpired flow
1 parent ea2b9b7 commit 84521a6

File tree

9 files changed

+538
-26
lines changed

9 files changed

+538
-26
lines changed

src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public async Task<IResult> GetSubscriptionAsync(
102102
[BindNever] User user)
103103
{
104104
var subscription = await getBitwardenSubscriptionQuery.Run(user);
105-
return TypedResults.Ok(subscription);
105+
return subscription == null ? TypedResults.NotFound() : TypedResults.Ok(subscription);
106106
}
107107

108108
[HttpPost("subscription/reinstate")]

src/Billing/Services/Implementations/PaymentMethodAttachedHandler.cs

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -104,35 +104,56 @@ await _stripeFacade.UpdateSubscription(invoicedProviderSubscription.Id,
104104
var unpaidSubscriptions = subscriptions?.Data.Where(subscription =>
105105
subscription.Status == StripeConstants.SubscriptionStatus.Unpaid).ToList();
106106

107-
if (unpaidSubscriptions == null || unpaidSubscriptions.Count == 0)
107+
var incompleteSubscriptions = subscriptions?.Data.Where(subscription =>
108+
subscription.Status == StripeConstants.SubscriptionStatus.Incomplete).ToList();
109+
110+
// Process unpaid subscriptions
111+
if (unpaidSubscriptions != null && unpaidSubscriptions.Count > 0)
112+
{
113+
foreach (var subscription in unpaidSubscriptions)
114+
{
115+
await AttemptToPayOpenSubscriptionAsync(subscription);
116+
}
117+
}
118+
119+
// Process incomplete subscriptions - only if there's exactly one to avoid overcharging
120+
if (incompleteSubscriptions == null || incompleteSubscriptions.Count == 0)
108121
{
109122
return;
110123
}
111124

112-
foreach (var unpaidSubscription in unpaidSubscriptions)
125+
if (incompleteSubscriptions.Count > 1)
113126
{
114-
await AttemptToPayOpenSubscriptionAsync(unpaidSubscription);
127+
_logger.LogWarning(
128+
"Customer {CustomerId} has {Count} incomplete subscriptions. Skipping automatic payment retry to avoid overcharging. Subscription IDs: {SubscriptionIds}",
129+
customer.Id,
130+
incompleteSubscriptions.Count,
131+
string.Join(", ", incompleteSubscriptions.Select(s => s.Id)));
132+
return;
115133
}
134+
135+
// Exactly one incomplete subscription - safe to retry
136+
await AttemptToPayOpenSubscriptionAsync(incompleteSubscriptions.First());
116137
}
117138

118-
private async Task AttemptToPayOpenSubscriptionAsync(Subscription unpaidSubscription)
139+
private async Task AttemptToPayOpenSubscriptionAsync(Subscription subscription)
119140
{
120-
var latestInvoice = unpaidSubscription.LatestInvoice;
141+
var latestInvoice = subscription.LatestInvoice;
121142

122-
if (unpaidSubscription.LatestInvoice is null)
143+
if (subscription.LatestInvoice is null)
123144
{
124145
_logger.LogWarning(
125-
"Attempted to pay unpaid subscription {SubscriptionId} but latest invoice didn't exist",
126-
unpaidSubscription.Id);
146+
"Attempted to pay subscription {SubscriptionId} with status {Status} but latest invoice didn't exist",
147+
subscription.Id, subscription.Status);
127148

128149
return;
129150
}
130151

131152
if (latestInvoice.Status != StripeInvoiceStatus.Open)
132153
{
133154
_logger.LogWarning(
134-
"Attempted to pay unpaid subscription {SubscriptionId} but latest invoice wasn't \"open\"",
135-
unpaidSubscription.Id);
155+
"Attempted to pay subscription {SubscriptionId} with status {Status} but latest invoice wasn't \"open\"",
156+
subscription.Id, subscription.Status);
136157

137158
return;
138159
}
@@ -144,8 +165,8 @@ private async Task AttemptToPayOpenSubscriptionAsync(Subscription unpaidSubscrip
144165
catch (Exception e)
145166
{
146167
_logger.LogError(e,
147-
"Attempted to pay open invoice {InvoiceId} on unpaid subscription {SubscriptionId} but encountered an error",
148-
latestInvoice.Id, unpaidSubscription.Id);
168+
"Attempted to pay open invoice {InvoiceId} on subscription {SubscriptionId} with status {Status} but encountered an error",
169+
latestInvoice.Id, subscription.Id, subscription.Status);
149170
throw;
150171
}
151172
}

src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ public async Task HandleAsync(Event parsedEvent)
6969

7070
var currentPeriodEnd = subscription.GetCurrentPeriodEnd();
7171

72-
if (SubscriptionWentUnpaid(parsedEvent, subscription))
72+
if (SubscriptionWentUnpaid(parsedEvent, subscription) ||
73+
SubscriptionWentIncompleteExpired(parsedEvent, subscription))
7374
{
7475
await DisableSubscriberAsync(subscriberId, currentPeriodEnd);
7576
await SetSubscriptionToCancelAsync(subscription);
@@ -111,6 +112,18 @@ SubscriptionStatus.Active or
111112
LatestInvoice.BillingReason: BillingReasons.SubscriptionCreate or BillingReasons.SubscriptionCycle
112113
};
113114

115+
private static bool SubscriptionWentIncompleteExpired(
116+
Event parsedEvent,
117+
Subscription currentSubscription) =>
118+
parsedEvent.Data.PreviousAttributes.ToObject<Subscription>() is Subscription
119+
{
120+
Status: SubscriptionStatus.Incomplete
121+
} && currentSubscription is
122+
{
123+
Status: SubscriptionStatus.IncompleteExpired,
124+
LatestInvoice.BillingReason: BillingReasons.SubscriptionCreate or BillingReasons.SubscriptionCycle
125+
};
126+
114127
private static bool SubscriptionBecameActive(
115128
Event parsedEvent,
116129
Subscription currentSubscription) =>

src/Core/Billing/Payment/Commands/UpdatePaymentMethodCommand.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Bit.Core.Billing.Constants;
44
using Bit.Core.Billing.Payment.Models;
55
using Bit.Core.Billing.Services;
6+
using Bit.Core.Billing.Subscriptions.Models;
67
using Bit.Core.Entities;
78
using Bit.Core.Services;
89
using Bit.Core.Settings;
@@ -143,6 +144,24 @@ private async Task<BillingCommandResult<MaskedPaymentMethod>> AddPayPalAsync(
143144
await stripeAdapter.UpdateCustomerAsync(customer.Id, new CustomerUpdateOptions { Metadata = metadata });
144145
}
145146

147+
// If the subscriber has an incomplete subscription, pay the invoice with the new PayPal payment method
148+
if (!string.IsNullOrEmpty(subscriber.GatewaySubscriptionId))
149+
{
150+
var subscription = await stripeAdapter.GetSubscriptionAsync(subscriber.GatewaySubscriptionId);
151+
152+
if (subscription.Status == StripeConstants.SubscriptionStatus.Incomplete)
153+
{
154+
var invoice = await stripeAdapter.UpdateInvoiceAsync(subscription.LatestInvoiceId,
155+
new InvoiceUpdateOptions
156+
{
157+
AutoAdvance = false,
158+
Expand = ["customer"]
159+
});
160+
161+
await braintreeService.PayInvoice(new UserId(subscriber.Id), invoice);
162+
}
163+
}
164+
146165
var payPalAccount = braintreeCustomer.DefaultPaymentMethod as PayPalAccount;
147166

148167
return MaskedPaymentMethod.From(payPalAccount!);

src/Core/Billing/Premium/Commands/CreatePremiumCloudHostedSubscriptionCommand.cs

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,13 @@ public Task<BillingCommandResult<None>> Run(
7272
BillingAddress billingAddress,
7373
short additionalStorageGb) => HandleAsync<None>(async () =>
7474
{
75-
if (user.Premium)
75+
// A "terminal" subscription is one that has ended and cannot be renewed/reactivated.
76+
// These are: 'canceled' (user canceled) and 'incomplete_expired' (payment failed and time expired).
77+
// We allow users with terminal subscriptions to create a new subscription even if user.Premium is still true,
78+
// enabling the resubscribe workflow without requiring Premium status to be cleared first.
79+
var hasTerminalSubscription = await HasTerminalSubscriptionAsync(user);
80+
81+
if (user.Premium && !hasTerminalSubscription)
7682
{
7783
return new BadRequest("Already a premium user.");
7884
}
@@ -98,8 +104,11 @@ public Task<BillingCommandResult<None>> Run(
98104
* purchased account credit but chose to use a tokenizable payment method to pay for the subscription. In this case,
99105
* we need to add the payment method to their customer first. If the incoming payment method is account credit,
100106
* we can just go straight to fetching the customer since there's no payment method to apply.
107+
*
108+
* Additionally, if this is a resubscribe scenario with a tokenized payment method, we should update the payment method
109+
* to ensure the new payment method is used instead of the old one.
101110
*/
102-
else if (paymentMethod.IsTokenized && !await hasPaymentMethodQuery.Run(user))
111+
else if (paymentMethod.IsTokenized && (!await hasPaymentMethodQuery.Run(user) || hasTerminalSubscription))
103112
{
104113
await updatePaymentMethodCommand.Run(user, paymentMethod.AsTokenized, billingAddress);
105114
customer = await subscriberService.GetCustomerOrThrow(user, new CustomerGetOptions { Expand = _expand });
@@ -122,7 +131,7 @@ public Task<BillingCommandResult<None>> Run(
122131
case { Type: TokenizablePaymentMethodType.PayPal }
123132
when subscription.Status == SubscriptionStatus.Incomplete:
124133
case { Type: not TokenizablePaymentMethodType.PayPal }
125-
when subscription.Status == SubscriptionStatus.Active:
134+
when subscription.Status is SubscriptionStatus.Active or SubscriptionStatus.Incomplete:
126135
{
127136
user.Premium = true;
128137
user.PremiumExpirationDate = subscription.GetCurrentPeriodEnd();
@@ -369,4 +378,28 @@ private async Task<Subscription> CreateSubscriptionAsync(
369378

370379
return subscription;
371380
}
381+
382+
private async Task<bool> HasTerminalSubscriptionAsync(User user)
383+
{
384+
if (string.IsNullOrEmpty(user.GatewaySubscriptionId))
385+
{
386+
return false;
387+
}
388+
389+
try
390+
{
391+
var existingSubscription = await stripeAdapter.GetSubscriptionAsync(user.GatewaySubscriptionId);
392+
return existingSubscription.Status is
393+
SubscriptionStatus.Canceled or
394+
SubscriptionStatus.IncompleteExpired;
395+
}
396+
catch (Exception ex)
397+
{
398+
// Subscription doesn't exist in Stripe or can't be fetched (e.g., network issues, invalid ID)
399+
// Log the issue but proceed with subscription creation to avoid blocking legitimate resubscribe attempts
400+
_logger.LogWarning(ex, "Unable to fetch existing subscription {SubscriptionId} for user {UserId}. Proceeding with subscription creation",
401+
user.GatewaySubscriptionId, user.Id);
402+
return false;
403+
}
404+
}
372405
}

src/Core/Billing/Subscriptions/Queries/GetBitwardenSubscriptionQuery.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,21 @@ public interface IGetBitwardenSubscriptionQuery
3131
/// Currently only supports <see cref="User"/> subscribers. Future versions will support all
3232
/// <see cref="ISubscriber"/> types (User and Organization).
3333
/// </remarks>
34-
Task<BitwardenSubscription> Run(User user);
34+
Task<BitwardenSubscription?> Run(User user);
3535
}
3636

3737
public class GetBitwardenSubscriptionQuery(
3838
ILogger<GetBitwardenSubscriptionQuery> logger,
3939
IPricingClient pricingClient,
4040
IStripeAdapter stripeAdapter) : IGetBitwardenSubscriptionQuery
4141
{
42-
public async Task<BitwardenSubscription> Run(User user)
42+
public async Task<BitwardenSubscription?> Run(User user)
4343
{
44+
if (string.IsNullOrEmpty(user.GatewaySubscriptionId))
45+
{
46+
return null;
47+
}
48+
4449
var subscription = await stripeAdapter.GetSubscriptionAsync(user.GatewaySubscriptionId, new SubscriptionGetOptions
4550
{
4651
Expand =

0 commit comments

Comments
 (0)