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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Ephemeral Environment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3729 +/- ##
==========================================
+ Coverage 95.98% 96.01% +0.03%
==========================================
Files 1135 1136 +1
Lines 36159 36363 +204
==========================================
+ Hits 34708 34915 +207
+ Misses 1451 1448 -3 ☔ View full report in Codecov by Sentry. |
If the merge conflicts wasn't enough now I'm hitting the codecov :( Will update the tests for this when I can. |
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.
@@ -649,3 +968,20 @@ def test_unrestrict_after_api_limit_grace_period_is_stale( | |||
assert organisation4.stop_serving_flags is True | |||
assert organisation4.block_access_to_admin is True | |||
assert getattr(organisation4, "api_limit_access_block", None) is None | |||
|
|||
|
|||
def test_recurring_tasks(mocker: MockerFixture, settings: SettingsWrapper) -> None: |
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.
minor:
def test_recurring_tasks(mocker: MockerFixture, settings: SettingsWrapper) -> None: | |
def test_register_recurring_tasks(mocker: MockerFixture, settings: SettingsWrapper) -> None: |
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),
]
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.