[PM-28423] Add latest_invoice expansion / logging to SubscriptionCancellationJob
#6603
Conversation
amorask-bitwarden
changed the title
latest_invoice expansion / logging to SubscriptionCancellationJoblatest_invoice expansion / logging to SubscriptionCancellationJob
|
Claude finished @amorask-bitwarden's task —— View job Code Review CompleteSummaryThis PR successfully fixes the bug where Status of Previous Review CommentsAll four previous review comments have been implicitly resolved by the current implementation:
Critical FindingsNone identified. Detailed FindingsFinding 1: Pattern matching uses Stripe SDK enum while rest of codebase uses string constants inconsistently SubscriptionCancellationJob.cs:37 uses 💭 The codebase has inconsistent patterns:
While the SDK enum provides type safety for pattern matching, it creates inconsistency. Most handlers in Example from existing code// SubscriptionUpdatedHandler.cs:91 uses string constants
case StripeSubscriptionStatus.Unpaid or StripeSubscriptionStatus.IncompleteExpired
// PaymentMethodAttachedHandler.cs:51 uses StripeConstants
subscription.Status != StripeConstants.SubscriptionStatus.CanceledConsider standardizing to Finding 2: Magic strings for billing reasons SubscriptionCancellationJob.cs:38 uses hardcoded strings ♻️ These billing reason strings appear in multiple places but aren't centralized in Suggested addition to StripeConstants.cspublic static class BillingReason
{
public const string SubscriptionCycle = "subscription_cycle";
public const string SubscriptionCreate = "subscription_create";
public const string Manual = "manual";
// Add others as needed
}This is low priority since the strings are standard Stripe values, but centralization would improve maintainability. Finding 3: Missing null safety warning suppression SubscriptionCancellationJob.cs:38 uses the pattern 💭 While the pattern correctly ensures Alternative (more explicit)if (subscription is not
{
Status: SubscriptionStatus.Unpaid,
LatestInvoice: { } invoice
} || invoice.BillingReason is not ("subscription_cycle" or "subscription_create"))
{
logger.LogWarning(...);
return;
}The current pattern is correct and idiomatic for C# 10+, but consider team preferences. Finding 4: Test coverage is excellent but uses different type pattern test/Billing.Test/Jobs/SubscriptionCancellationJobTests.cs uses 🎨 This is actually correct - tests should set up Subscription objects with string values since that's what the Stripe SDK returns. The pattern match in production code then compares against the SDK enum, which works because Stripe.SubscriptionStatus enum values match the string constants. However, for consistency and to match the test setup, production code could use string constants in the pattern match. This is a minor style preference. Good Practices Observed
Recommendations
PR Title and Description✅ Title: Clear and descriptive |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (subscription is not | ||
| { | ||
| Status: SubscriptionStatus.Unpaid, | ||
| LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create" |
There was a problem hiding this comment.
❌ 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:
| 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.
| if (subscription is not | ||
| { | ||
| Status: SubscriptionStatus.Unpaid, | ||
| LatestInvoice.BillingReason: "subscription_cycle" or "subscription_create" |
There was a problem hiding this comment.
"subscription_cycle" and "subscription_create" to constants in StripeConstants.BillingReason for better maintainability and consistency with the rest of the codebase.
|
|
||
| if (subscription is not | ||
| { | ||
| Status: SubscriptionStatus.Unpaid, |
There was a problem hiding this comment.
💭 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.
| var subscription = new Subscription | ||
| { | ||
| Id = subscriptionId, | ||
| Status = StripeSubscriptionStatus.Active, |
There was a problem hiding this comment.
💭 Type inconsistency: Tests use StripeSubscriptionStatus.Active (string constant) while production code at line 37 uses SubscriptionStatus.Unpaid (Stripe SDK enum). Consider aligning these for consistency.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6603 +/- ##
==========================================
+ Coverage 52.98% 53.04% +0.05%
==========================================
Files 1909 1909
Lines 84828 84840 +12
Branches 7634 7634
==========================================
+ Hits 44948 45003 +55
+ Misses 38127 38082 -45
- Partials 1753 1755 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28423
📔 Objective
The
SubscriptionCancellationJobwas never running because it was checking for abilling_reasonon the incoming subscription'slatest_invoicewhile never expandinglatest_invoicein the request to retrieve the subscription.This PR resolves that as well as adds logging and unit tests.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes