fix: handle empty notification recipients#7589
Conversation
|
@SahilJat is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Code Review
This pull request introduces checks to handle cases where no recipient emails are found for API usage and blocked notifications, including a new unit test for this scenario. Feedback suggests that returning early without creating a notification record will lead to redundant processing and log noise in subsequent task runs, as the organization will be re-evaluated every 12 hours.
| if not recipient_emails: | ||
| logger.warning( | ||
| "notification.no_recipients", | ||
| organisation__id=organisation.id, | ||
| matched_threshold=matched_threshold, | ||
| ) | ||
| return |
There was a problem hiding this comment.
Returning early here without creating an OrganisationAPIUsageNotification record will cause this organization to be re-processed and a warning to be logged every time the handle_api_usage_notifications task runs (currently every 12 hours). This leads to persistent redundant processing and log noise for organizations with no valid recipients.
Consider creating the notification record even if no emails are sent, to ensure the 'notify once per threshold' logic is respected for the current billing period. While this might mean a newly added admin misses a notification for a threshold already crossed, they would still see the status in the dashboard and receive future notifications.
There was a problem hiding this comment.
Sorry for the back and forth, I thought this has been addressed. It's actually a comment on-point as otherwise the organisations will be re-evaluated on every task run.
Can we move the early return to after the OrganisationAPIUsageNotification.objects.create call please?
|
@SahilJat thanks for your contribution. You'll need to run |
2da8be0 to
07d4f78
Compare
|
Hi @Zaimwa9 i did run make lint and it shows around 22k lines edited , is this fine? |
|
Sorry, I should have been more specific as the command should be run from |
001cf1e to
f44aa4e
Compare
|
hi @Zaimwa9 thanks for the hint , i did the changes required now, please have a look and let me know. |
for more information, see https://pre-commit.ci
f44aa4e to
1add2f6
Compare
21991e4 to
1fcd6f8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7589 +/- ##
==========================================
- Coverage 98.50% 98.35% -0.16%
==========================================
Files 1430 1436 +6
Lines 54254 54274 +20
==========================================
- Hits 53444 53382 -62
- Misses 810 892 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @Zaimwa9 all the test cases are passing and test coverage is covered too. Thanks |
Zaimwa9
left a comment
There was a problem hiding this comment.
Sorry for the back and forth. Gemini comment is worth addressing
5a1373e to
7c75218
Compare
Thanks for submitting a PR! Please check the boxex below:
I have read the Contributing
Guide.
I have added information to
docs/if requiredso people know about the feature.
I have filled in the "Changes" section below.
I have filled in the "How did you test this
code" section below.
Changes
Closes Sendgrid API calls raising HTTP 400 Bad Requests #7353
This PR resolves an issue where the background tasks for sending API usage and flag blocked notifications crash with a
BadRequestsError: HTTP Error 400: Bad Request.This occurs when using external API-based email
backends (like SendGrid or AWS SES) and the
organisation has no valid recipients (e.g., zero
total users, or no admin users when the usage
threshold is <100%). Unlike traditional SMTP
backends, API backends strictly validate the payload and return a 400 error if the
recipient_listis empty.Changes made:
Added an early return guard checking
if not recipient_emails:in_send_api_usage_notification.Added the same early return guard in
send_api_flags_blocked_notificationto prevent the same crash there.Logs a warning (
notification.no_recipients)instead of attempting to send the email.
How did you test this code?
test_handle_api_usage_notifications__no_admin_users_ _skips_notificationto explicitly verify that thenotification is safely skipped and no 400 error is thrown when an organisation has no users.
organisations_tasksto ensure no regressions.