Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
30 changes: 22 additions & 8 deletions src/Billing/Jobs/SubscriptionCancellationJob.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
๏ปฟ// FIXME: Update this file to be null safe and then delete the line below
#nullable disable

using Bit.Billing.Services;
๏ปฟusing Bit.Billing.Services;
using Bit.Core.Billing.Constants;
using Bit.Core.Repositories;
using Quartz;
using Stripe;

namespace Bit.Billing.Jobs;

using static StripeConstants;

public class SubscriptionCancellationJob(
IStripeFacade stripeFacade,
IOrganizationRepository organizationRepository)
IOrganizationRepository organizationRepository,
ILogger<SubscriptionCancellationJob> logger)
: IJob
{
public async Task Execute(IJobExecutionContext context)
Expand All @@ -21,20 +22,31 @@ public async Task Execute(IJobExecutionContext context)
var organization = await organizationRepository.GetByIdAsync(organizationId);
if (organization == null || organization.Enabled)
{
logger.LogWarning("{Job} skipped for subscription ({SubscriptionID}) because organization is either null or enabled", nameof(SubscriptionCancellationJob), subscriptionId);
// Organization was deleted or re-enabled by CS, skip cancellation
return;
}

var subscription = await stripeFacade.GetSubscription(subscriptionId);
if (subscription?.Status != "unpaid" ||
subscription.LatestInvoice?.BillingReason is not ("subscription_cycle" or "subscription_create"))
var subscription = await stripeFacade.GetSubscription(subscriptionId, new SubscriptionGetOptions
{
Expand = ["latest_invoice"]
});

if (subscription is not
{
Status: SubscriptionStatus.Unpaid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Consistency question: This uses SubscriptionStatus.Unpaid (Stripe SDK enum), but other services in the codebase use StripeSubscriptionStatus.Unpaid or StripeConstants.SubscriptionStatus.Unpaid (string constants). Should this align with the existing pattern?

For reference, see SubscriptionUpdatedHandler.cs:91 which uses StripeSubscriptionStatus.Unpaid.

LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โŒ Potential NullReferenceException: The pattern LatestInvoice.BillingReason will throw if LatestInvoice is null. While the expansion should populate it, defensive programming suggests checking for null explicitly:

Suggested change
LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create"
{
Status: SubscriptionStatus.Unpaid,
LatestInvoice: { BillingReason: "subscription_cycle" or "subscription_create" }
})

This nested pattern ensures LatestInvoice is not null before accessing BillingReason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Magic strings: Consider extracting "subscription_cycle" and "subscription_create" to constants in StripeConstants.BillingReason for better maintainability and consistency with the rest of the codebase.

})
{
logger.LogWarning("{Job} skipped for subscription ({SubscriptionID}) because subscription is not unpaid or does not have a cancellable billing reason", nameof(SubscriptionCancellationJob), subscriptionId);
return;
}

// Cancel the subscription
await stripeFacade.CancelSubscription(subscriptionId, new SubscriptionCancelOptions());

logger.LogInformation("{Job} cancelled subscription ({SubscriptionID})", nameof(SubscriptionCancellationJob), subscriptionId);

// Void any open invoices
var options = new InvoiceListOptions
{
Expand All @@ -46,6 +58,7 @@ public async Task Execute(IJobExecutionContext context)
foreach (var invoice in invoices)
{
await stripeFacade.VoidInvoice(invoice.Id);
logger.LogInformation("{Job} voided invoice ({InvoiceID}) for subscription ({SubscriptionID})", nameof(SubscriptionCancellationJob), invoice.Id, subscriptionId);
}

while (invoices.HasMore)
Expand All @@ -55,6 +68,7 @@ public async Task Execute(IJobExecutionContext context)
foreach (var invoice in invoices)
{
await stripeFacade.VoidInvoice(invoice.Id);
logger.LogInformation("{Job} voided invoice ({InvoiceID}) for subscription ({SubscriptionID})", nameof(SubscriptionCancellationJob), invoice.Id, subscriptionId);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ private async Task AlignOrganizationSubscriptionConcernsAsync(
new SubscriptionItemOptions
{
Id = passwordManagerItem.Id,
Price = families.PasswordManager.StripePlanId
Price = families.PasswordManager.StripePlanId
}
],
ProrationBehavior = ProrationBehavior.None
Expand Down
Loading
Loading