[PM-31140] Consolidate unpaid subscription handling
#6918
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6918 +/- ##
==========================================
- Coverage 56.06% 56.06% -0.01%
==========================================
Files 1969 1969
Lines 87078 87014 -64
Branches 7757 7735 -22
==========================================
- Hits 48821 48784 -37
+ Misses 36448 36425 -23
+ Partials 1809 1805 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| var premiumPriceIds = premiumPlans.SelectMany(p => new[] { p.Seat.StripePriceId, p.Storage.StripePriceId }).ToHashSet(); | ||
| return subscription.Items.Any(i => premiumPriceIds.Contains(i.Price.Id)); | ||
| } | ||
| var now = subscription.TestClock?.FrozenTime ?? DateTime.UtcNow; |
There was a problem hiding this comment.
Oh man, I'm filing this away to remember in the future. I am now wondering if I should have used this instead of using the invoice period end in my current PR haha
kdenney
left a comment
kdenney
left a comment
There was a problem hiding this comment.
Nice job!! 🎉 This is way cleaner. I have just a couple of non-blocking questions.
| LatestInvoice.BillingReason: BillingReasons.SubscriptionCreate or BillingReasons.SubscriptionCycle | ||
| }; | ||
|
|
||
| private Task DisableSubscriberAsync(SubscriberId subscriberId, DateTime? currentPeriodEnd) => |
There was a problem hiding this comment.
❓ What do you think about moving these methods that operate on a SubscriberId into SubscriberService? It would help clean up this file even more, they will be public so unit tests can target the methods individually which is always a bonus, and I could even see a possibility for these to be used elsewhere in the application at some point so having them in a service makes that easy.
| { | ||
| var scheduler = await _schedulerFactory.GetScheduler(); | ||
|
|
||
| var job = JobBuilder.Create<SubscriptionCancellationJob>() |
There was a problem hiding this comment.
❓ This job is now unused. I assume that removing SubscriptionCancellationJob from the codebase would cause any scheduled jobs in the queue at the time of release to throw exceptions? I also assume that's why you didn't delete it in this PR? 😆 Is there a follow-up ticket for deleting it?
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31140
📔 Objective
Consolidates the handling of subscriptions that transition to
unpaidstatus across all entity types (Premium users, Organizations, and Providers).Previously, each entity type was handled differently:
cancel_atwas set on the subscription to 7 days in the futureThis PR standardizes on the Provider approach for all entity types:
cancel_atto 7 days in the futureThis gives customers a consistent 7-day grace period to resolve payment issues before their subscription is canceled, regardless of subscription type.
Additional improvements:
ProrationBehavior.Noneto prevent unintended proration charges during cancellation/reactivationCancellationDetails.Commentfor better audit trail when setting cancellationTesting: Unit tests in
SubscriptionUpdatedHandlerTests.cswere updated to reflect the consolidated behavior and verify the new subscription update options.📸 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