Stripe Auto-refund cancellation within Grace Period #894
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #894 +/- ##
=======================================
Coverage 96.23% 96.23%
=======================================
Files 823 823
Lines 18972 19002 +30
=======================================
+ Hits 18257 18287 +30
Misses 715 715
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
calvin-codecov
force-pushed
the
cy/cancellation_grace
branch
from
5aad806 to
813eed3
Compare
calvin-codecov
changed the title
services/billing.py
Outdated
| cancel_at_period_end=True, | ||
| proration_behavior="none", | ||
| # we give an auto-refund grace period of 24 hours for a monthly subscription or 72 hours for a yearly subscription | ||
| # current_subscription_timestamp = subscription["current_period_start"] |
There was a problem hiding this comment.
did you mean to leave this?
There was a problem hiding this comment.
yeah, for some descriptuon
There was a problem hiding this comment.
not the second line. oops
services/billing.py
Outdated
| ) | ||
| differenceFromNow = datetime.now() - current_subscription_datetime |
There was a problem hiding this comment.
oops, haha habit
services/billing.py
Outdated
| ) | ||
| within_refund_grace_period = ( | ||
| subscription_plan_interval == "month" and differenceFromNow.days < 1 | ||
| ) or (subscription_plan_interval == "year" and differenceFromNow.days < 3) |
There was a problem hiding this comment.
does .days operate the way you expect? Did you check for weird rounding errors in how it defines a day?
There was a problem hiding this comment.
.days is defined as full days so it will be an integer
ajay-sentry
reviewed
Oct 18, 2024
services/billing.py
Outdated
| ) | ||
| differenceFromNow = datetime.now() - current_subscription_datetime | ||
|
|
||
| subscription_plan_interval = ( |
There was a problem hiding this comment.
This is equivalent to subscription_plan_interval = getattr(subscription.plan, "interval", None) I believe
ajay-sentry
reviewed
Oct 18, 2024
services/billing.py
Outdated
| ): | ||
| stripe.Refund.create(invoice["charge"]) | ||
| created_refund = True | ||
| if created_refund == True: |
There was a problem hiding this comment.
can get rid of this secondary if (and remove created_refund) and have this clause be in the top level statement with what you have in the else as an else for the conditional starting on 198, right?
stripe.Customer.modify( owner.stripe_customer_id, balance=0, )
calvin-codecov
force-pushed
the
cy/cancellation_grace
branch
2 times, most recently
from
0a029bb to
8432340
Compare
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2647 tests with View the full list of failed testspytest
|
services/billing.py
Outdated
Comment on lines
181
to
185
| customer = stripe.Customer.retrieve(owner.stripe_customer_id) | ||
| # we are giving customers 2 autorefund instances | ||
| autorefunds_remaining = int( | ||
| customer["metadata"].get("autorefunds_remaining", "2") | ||
| ) |
There was a problem hiding this comment.
I want this logic to be inside the if within_refund_grace_period block so it's only called when they are within the cancellation window
There was a problem hiding this comment.
That makes sense. Any ideas about how to prevent another level of if statements or in your opinion, does that seem fine? Do we have a general rule of thumb for how many levels we think is acceptable?
ajay-sentry
reviewed
Oct 24, 2024
| cancel_at_period_end=True, | ||
| proration_behavior="none", | ||
| # we give an auto-refund grace period of 24 hours for a monthly subscription or 72 hours for a yearly subscription | ||
| current_subscription_datetime = datetime.fromtimestamp( |
There was a problem hiding this comment.
Just to confirm, are the timezones for both datetime.now() and datetime.fromtimestamp() the same?
There was a problem hiding this comment.
That's a great question. I just checked and both are in machine local so since we only care about the difference, we're good.
There was a problem hiding this comment.
great! Thanks for checking
There was a problem hiding this comment.
I'm converting these to utc now actually because I realized depending on what the local time of the machine is, it could change the date. Like if february 29th was converted to march 1st due to timezone difference, subtracting 1 month would give february 1st instead of january 29th
ajay-sentry
reviewed
Oct 24, 2024
services/billing.py
Outdated
| if within_refund_grace_period and autorefunds_remaining > 0: | ||
| stripe.Subscription.cancel(owner.stripe_subscription_id) | ||
|
|
||
| invoices_list = stripe.Invoice.list( |
There was a problem hiding this comment.
it looks like you can add a created parameter to this call too and maybe shorten the amount of results to filter through as well. Not sure how much of an optimization... maybe to the point of not needing this stuff:
and invoice_created_datetime < current_subscription_datetime
and invoice_created_datetime >= start_of_last_period
There was a problem hiding this comment.
There was a problem hiding this comment.
That would allow you to this variable above and condense this for loop as well
start_of_last_period = (
current_subscription_datetime - relativedelta(months=1)
if subscription_plan_interval == "month"
else current_subscription_datetime - relativedelta(years=1)
)
ajay-sentry
reviewed
Oct 24, 2024
services/billing.py
Outdated
| balance=0, | ||
| metadata={"autorefunds_remaining": str(autorefunds_remaining - 1)}, | ||
| ) | ||
| log.info( |
There was a problem hiding this comment.
any other log info that might be useful here? Maybe customer id and sub id off the top of my head, we can add them with the extra=dict() param
calvin-codecov
force-pushed
the
cy/cancellation_grace
branch
from
8432340 to
3ecef75
Compare
services/billing.py
Outdated
Comment on lines
151
to
158
| # cancels a Stripe customer subscription immediately and attempts to refund their payments for the current period | ||
| def cancel_and_refund( | ||
| self, | ||
| owner, | ||
| current_subscription_datetime, | ||
| subscription_plan_interval, | ||
| autorefunds_remaining, | ||
| ): |
There was a problem hiding this comment.
Suggested change
| # cancels a Stripe customer subscription immediately and attempts to refund their payments for the current period | |
| def cancel_and_refund( | |
| self, | |
| owner, | |
| current_subscription_datetime, | |
| subscription_plan_interval, | |
| autorefunds_remaining, | |
| ): | |
| def cancel_and_refund( | |
| self, | |
| owner, | |
| current_subscription_datetime, | |
| subscription_plan_interval, | |
| autorefunds_remaining, | |
| ): | |
| # cancels a Stripe customer subscription immediately and attempts to refund their payments for the current period |
nora-codecov
approved these changes
Oct 28, 2024
|
|
||
| if created_refund: | ||
| # update the customer's balance back to 0 in accordance to | ||
| # https://support.stripe.com/questions/refunding-credit-balance-to-customer-after-subscription-downgrade-or-cancellation |
There was a problem hiding this comment.
ty for the documentation!!
| autorefunds_remaining=autorefunds_remaining, | ||
| ), | ||
| ) | ||
|
|
calvin-codecov
force-pushed
the
cy/cancellation_grace
branch
from
ab9bfe0 to
0d29795
Compare
calvin-codecov
force-pushed
the
cy/cancellation_grace
branch
from
e9eff0c to
5c9eec8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose/Motivation
What is the feature? Why is this being done?
Adding auto-refund logic for when a user cancels their subscription within 24 hours (monthly charge) or 72 hours (yearly charge) of the period starting and being charged.
Open to any and all feedback including styling and syntax recommendations as this is one of my first python/codecov-api PRs. Should I wrap Stripe API calls in a try/catch?
Links to relevant tickets
Closes codecov/engineering-team#2508
What does this PR do?
Our current logic is that when someone cancels, since they have already paid for their current period (either a month or a year), we just "modify" there subscription to cancel at period end. We will still keep this logic for when they cancel out of the grace period but within the grace period, we will cancel immediately and refund them their money.
Doing now:
Notable change
{"autorefunds_remaining: int"}as opposed to making a new column in the DBOwnerstable