Skip to content

fix: handle empty notification recipients#7589

Open
SahilJat wants to merge 5 commits into
Flagsmith:mainfrom
SahilJat:fix/api-usage-email-400-error
Open

fix: handle empty notification recipients#7589
SahilJat wants to merge 5 commits into
Flagsmith:mainfrom
SahilJat:fix/api-usage-email-400-error

Conversation

@SahilJat
Copy link
Copy Markdown
Contributor

Thanks for submitting a PR! Please check the boxex below:

  • I have read the Contributing
    Guide
    .

  • I have added information to docs/ if required
    so 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_list is 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_notification to prevent the same crash there.

  • Logs a warning (notification.no_recipients)
    instead of attempting to send the email.

How did you test this code?

  • Added a new unit test: test_handle_api_usage_notifications__no_admin_users_ _skips_notification to explicitly verify that the
    notification is safely skipped and no 400 error is thrown when an organisation has no users.
  • Ran existing unit tests for organisations_tasks
    to ensure no regressions.

@SahilJat SahilJat requested a review from a team as a code owner May 25, 2026 05:50
@SahilJat SahilJat requested review from khvn26 and removed request for a team May 25, 2026 05:50
@vercel
Copy link
Copy Markdown

vercel Bot commented May 25, 2026

@SahilJat is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions Bot added the api Issue related to the REST API label May 25, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +88 to +94
if not recipient_emails:
logger.warning(
"notification.no_recipients",
organisation__id=organisation.id,
matched_threshold=matched_threshold,
)
return
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SahilJat requested a review from a team as a code owner May 25, 2026 06:03
@github-actions github-actions Bot added the docs Documentation updates label May 25, 2026
@Zaimwa9
Copy link
Copy Markdown
Contributor

Zaimwa9 commented May 25, 2026

@SahilJat thanks for your contribution. You'll need to run make lint or generate-docs to regenerate the events catalogue as you created 2 new ones

@SahilJat SahilJat requested a review from a team as a code owner May 25, 2026 11:42
@SahilJat SahilJat force-pushed the fix/api-usage-email-400-error branch from 2da8be0 to 07d4f78 Compare May 25, 2026 11:56
@SahilJat SahilJat closed this May 25, 2026
@SahilJat SahilJat reopened this May 25, 2026
@SahilJat
Copy link
Copy Markdown
Contributor Author

Hi @Zaimwa9 i did run make lint and it shows around 22k lines edited , is this fine?

@Zaimwa9
Copy link
Copy Markdown
Contributor

Zaimwa9 commented May 25, 2026

Sorry, I should have been more specific as the command should be run from api/, it edited all the docs it seems

@SahilJat SahilJat force-pushed the fix/api-usage-email-400-error branch 3 times, most recently from 001cf1e to f44aa4e Compare May 26, 2026 05:03
@SahilJat
Copy link
Copy Markdown
Contributor Author

hi @Zaimwa9 thanks for the hint , i did the changes required now, please have a look and let me know.

@SahilJat SahilJat force-pushed the fix/api-usage-email-400-error branch from f44aa4e to 1add2f6 Compare May 26, 2026 05:08
Copy link
Copy Markdown
Contributor

@Zaimwa9 Zaimwa9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests need an update 👍

Comment thread api/tests/unit/organisations/test_unit_organisations_tasks.py
@SahilJat SahilJat force-pushed the fix/api-usage-email-400-error branch from 21991e4 to 1fcd6f8 Compare May 26, 2026 08:40
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.35%. Comparing base (2e16ede) to head (5a1373e).
⚠️ Report is 9 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SahilJat
Copy link
Copy Markdown
Contributor Author

Hi @Zaimwa9 all the test cases are passing and test coverage is covered too. Thanks

Copy link
Copy Markdown
Contributor

@Zaimwa9 Zaimwa9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the back and forth. Gemini comment is worth addressing

@SahilJat SahilJat force-pushed the fix/api-usage-email-400-error branch from 5a1373e to 7c75218 Compare May 26, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Issue related to the REST API docs Documentation updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sendgrid API calls raising HTTP 400 Bad Requests

2 participants