Fix (Billing): API usage notification evaluates against actual plan limit instead of cache#8003
Conversation
|
@srijantrpth is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAPI usage notification eligibility now compares usage against Estimated code review effort: 2 (Simple) | ~10 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes premature API usage warning emails for paid organisations by ensuring the usage-threshold evaluation uses the authoritative subscription plan limit rather than potentially stale cached subscription information.
Changes:
- Updates
handle_api_usage_notificationsto filter organisations usingsubscription.max_api_calls(plan limit) instead of the subscription information cache. - Updates
handle_api_usage_notification_for_organisationto compute the allowance usingorganisation.subscription.max_api_callsrather than the cachedallowed_30d_api_calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
api/organisations/tasks.py |
Updates the notification task’s pre-filter to compare 30-day usage against the actual subscription plan API call limit. |
api/organisations/task_helpers.py |
Ensures the percentage calculation uses the subscription’s max_api_calls directly instead of the cached allowance value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/tests/unit/organisations/test_unit_organisations_tasks.py (1)
501-584: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical: new test absorbs the previous test's tail, breaking both tests.
The new test's assertion block ends at Line 553, but Lines 554-584 (which reference
api_usage_notification, and re-run the notification check) are unchanged code left over fromtest_handle_api_usage_notifications__usage_below_100_percent__sends_90_percent_notification(Lines 424-499). That test's actual closing assertions were never reattached after the earlier lines were split off, so:
test_handle_api_usage_notifications__stale_cache_lower_than_subscription_limit__sends_no_emailnow inherits Lines 554-584 into its own body and will raiseNameError: name 'email' is not definedat runtime, since this test never assignslen(mailoutbox) == 0).- The 90%-notification test lost its email/idempotency assertions entirely.
This will break collection/execution of this test module. Reattach the dangling tail to the first test and cleanly close the new test at Line 553.
🐛 Proposed fix to restore correct test boundaries
assert email.alternatives[0][0] == render_to_string( "organisations/api_usage_notification.html", context={ "organisation": organisation, "matched_threshold": 90, "usage_url": f"{get_current_site_url()}/organisation/{organisation.id}/usage", }, ) + assert email.from_email == "noreply@flagsmith.com" + # Only admin because threshold is under 100. + assert email.to == ["admin@example.com"] + + assert ( + OrganisationAPIUsageNotification.objects.filter( + organisation=organisation, + ).count() + == 1 + ) + api_usage_notification = OrganisationAPIUsageNotification.objects.filter( + organisation=organisation, + ).first() + + assert api_usage_notification.percent_usage == 90 # type: ignore[union-attr] + + # Now re-run the usage to make sure the notification isn't resent. + handle_api_usage_notifications() + + assert ( + OrganisationAPIUsageNotification.objects.filter( + organisation=organisation, + ).count() + == 1 + ) + assert ( + OrganisationAPIUsageNotification.objects.filter( + organisation=organisation + ).first() + == api_usage_notification + ) `@pytest.mark.freeze_time`("2023-01-19T09:09:47.325132+00:00") def test_handle_api_usage_notifications__stale_cache_lower_than_subscription_limit__sends_no_email( ... ): ... assert ( OrganisationAPIUsageNotification.objects.filter( organisation=organisation, ).count() == 0 ) - assert email.from_email == "noreply@flagsmith.com" - # Only admin because threshold is under 100. - assert email.to == ["admin@example.com"] - - assert ( - OrganisationAPIUsageNotification.objects.filter( - organisation=organisation, - ).count() - == 1 - ) - api_usage_notification = OrganisationAPIUsageNotification.objects.filter( - organisation=organisation, - ).first() - - assert api_usage_notification.percent_usage == 90 # type: ignore[union-attr] - - # Now re-run the usage to make sure the notification isn't resent. - handle_api_usage_notifications() - - assert ( - OrganisationAPIUsageNotification.objects.filter( - organisation=organisation, - ).count() - == 1 - ) - assert ( - OrganisationAPIUsageNotification.objects.filter( - organisation=organisation - ).first() - == api_usage_notification - )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e9dd86e-2a7c-4e5e-874f-545d5e2d6d98
📒 Files selected for processing (1)
api/tests/unit/organisations/test_unit_organisations_tasks.py
3a5e584 to
99fc1aa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/tests/unit/organisations/test_unit_organisations_tasks.py (1)
535-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the org is excluded at the queryset stage, not just that no email was sent.
This test correctly reproduces the
#7976bug pattern (stale cache close to threshold vs. a much higher truemax_api_calls), but it only checks the downstream symptom (no email/notification). Since the fix operates by excluding the organisation via thesubscription__max_api_calls-based queryset filter inhandle_api_usage_notifications, the org should never reachget_current_api_usageat all. Addmock_api_usage.assert_not_called()to pin down that the mechanism (queryset exclusion), not just its effect, works as intended — this also clarifies thatmock_api_usage.return_value = usageis otherwise unused here.✅ Suggested strengthening
# Then + mock_api_usage.assert_not_called() + # Because usage (95) is evaluated against the true allowance (50,000), # it is < 1%, so no notification should be sent. assert len(mailoutbox) == 0
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4812c45b-9bcc-4ec5-b8dc-b01df14161f4
📒 Files selected for processing (1)
api/tests/unit/organisations/test_unit_organisations_tasks.py
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis judgement: 🟠 Fix before mergeThe production code change is well-motivated: switching the notification threshold evaluation from the potentially stale
🟠 Majors
🧹 Nits
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: the existing test suite covers this once the missed fixture is fixed. Product take: This is a solid improvement for paid-plan users who were receiving premature API usage warning emails due to stale cache data. Prevents unnecessary alarm and support burden from false-positive notifications. Big picture: The notification path now uses 🧭 Assumptions & unverified claims
The cache giveth and the cache taketh away — but at least now it's only the subscription's word that matters. · reviewed at b475015 1 finding(s) anchored outside the diff (not posted inline)
Test will fail: Observed: this test sets Fix: add |
Co-authored-by: themis-blindfold[bot] <302911107+themis-blindfold[bot]@users.noreply.github.com>
….com/srijantrpth/flagsmith into fix/issue-7976-api-notification-math
|
Hi @Zaimwa9 , I've pushed the updates to fix the failing test and removed the transient comment flagged by the bot. Let me know if anything else is needed!. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8003 +/- ##
==========================================
- Coverage 98.63% 98.49% -0.14%
==========================================
Files 1496 1506 +10
Lines 59131 59592 +461
==========================================
+ Hits 58325 58697 +372
- Misses 806 895 +89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7976
Organisations on paid plans were receiving API usage warning emails prematurely. The
handle_api_usage_notificationstask was evaluating usage against thesubscription_information_cacherather than the true plan limit. Because the cache can become stale, the percentage evaluated artificially high.handle_api_usage_notificationsto usesubscription__max_api_calls.elseblock ofhandle_api_usage_notification_for_organisationto bypass the cache and useorganisation.subscription.max_api_callsdirectly.Review effort: 1/5
How did you test this code?
make formatto ensure code style compliance.tests/unit/organisations/test suite, as my local Docker test-database permissions are currently restricted.