-
Notifications
You must be signed in to change notification settings - Fork 403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add API usage billing #3729
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3729 +/- ##
==========================================
+ Coverage 96.18% 96.21% +0.02%
==========================================
Files 1141 1142 +1
Lines 36494 36699 +205
==========================================
+ Hits 35101 35309 +208
+ Misses 1393 1390 -3 ☔ View full report in Codecov by Sentry. |
Ok the code coverage is now at 100%. @matthewelwell did you want to take a look at the introduced tests before merging or are we good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a couple of minor comments, but I've approved it nonetheless as I don't think they're critical.
register_task_mock.call_args_list == [ | ||
call(run_every=timedelta(seconds=43200)), | ||
call(run_every=timedelta(seconds=1800)), | ||
call(run_every=timedelta(seconds=43200)), | ||
call(run_every=timedelta(seconds=43200)), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling you're going to hate me for this, because I know you only really implemented this test for the coverage... but this test feels fairly pointless without actually asserting the functions that we're wrapping. How tricky is it to extend this test to check the task functions?
I think we'd just need to do something like:
assert register_task_mock.return_value.call_args_list = [
call(task_function_1),
...,
call(task_function_n),
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good call. I considered doing this before but I (incorrectly) thought the mocking would be more complicated.
Added.
Co-authored-by: Matthew Elwell <matthew.elwell@flagsmith.com>
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Pretty complicated PR. This introduces charges for API usage overages for monthly billed customers. Once a customer is billed for overages, a record is created and stops them from being billed twice in the same month. Only monthly billed customers are permitted to be billed, as the yearly paying customers need to be handled by sales. In order to ensure that customers are notified before they're billed, there is a pre-step that checks to see if the API usage notification has been sent out before including the customer in the billing step.
How did you test this code?
Manually tested against ChargeBee's backend and verified the results and also added four or five tests to ensure the workflow is proceeding according to plan.