fix(notifications): send organization low-balance alerts to owners and admins - #1863
fergusfinn wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying control-layer with
|
| Latest commit: |
93497ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6e34035a.control-layer.pages.dev |
| Branch Preview URL: | https://fix-org-low-balance-recipien.control-layer.pages.dev |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93497ac743
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| { | ||
| crate::background_error!( | ||
| NOTIFICATIONS, "email_send", Warning, | ||
| NOTIFICATIONS, "low_balance_recipients", Warning, | ||
| user_id = %user.id, | ||
| email = %user.email, | ||
| error = %e, | ||
| "Failed to send low-balance notification email" | ||
| "Failed to list organization admins for a low-balance notification" | ||
| ); | ||
| continue; | ||
| Vec::new() |
There was a problem hiding this comment.
When list_admin_emails fails transiently, converting the error to an empty recipient list still sends to the contact address; that successful delivery then causes low_balance_notification_sent to be set below. Since subsequent ticks exclude accounts with that flag, none of the owners or administrators are retried until the balance first recovers above the threshold, defeating the new notification behavior. Skip this account on lookup failure so the complete recipient lookup can be retried.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Recipient lookup failures can suppress future notifications, and membership lookups add avoidable per-candidate database work.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Expands organization low-balance alerts to reach active owners and admins while avoiding duplicate emails.
Changes:
- Added owner/admin recipient lookup and case-insensitive deduplication.
- Added delivery tracking and notification tests.
- Preserved existing personal-account behavior.
| File | Summary |
|---|---|
dwctl/src/notifications.rs |
Implements recipient fan-out and tests; two moderate issues remain around lookup failure handling and per-candidate database queries. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "Failed to list organization admins for a low-balance notification" | ||
| ); | ||
| continue; | ||
| Vec::new() |
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dwctl/src/notifications.rs">
<violation number="1" location="dwctl/src/notifications.rs:1211">
P2: This performs one database round trip per low-balance account, including personal accounts, so a large alert batch adds serial N+1 queries before mailing. Batch the lookup for all due organization IDs.</violation>
<violation number="2" location="dwctl/src/notifications.rs:1211">
P2: When `list_admin_emails` fails, the fallback to an empty recipient list silently removes all owners and admins from the alert, and if the contact email is then delivered the org is permanently marked as notified — the admins never got the email and never get retried. This differs from the intentional partial email-send trade-off documented above: on a listing failure nothing has been sent to the admins yet, so skipping this account (`continue`) and retrying next tick would re-mail nobody. Return the error out of the listing instead of degrading to contact-only, or the low-balance alert can miss every credit-adding member for good after a single transient query failure.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let name = user.display_name.as_deref().unwrap_or(&user.username); | ||
|
|
||
| if let Err(e) = email_service.send_low_balance_email(&user.email, Some(name), &balance).await { | ||
| let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| { |
There was a problem hiding this comment.
P2: This performs one database round trip per low-balance account, including personal accounts, so a large alert batch adds serial N+1 queries before mailing. Batch the lookup for all due organization IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dwctl/src/notifications.rs, line 1211:
<comment>This performs one database round trip per low-balance account, including personal accounts, so a large alert batch adds serial N+1 queries before mailing. Batch the lookup for all due organization IDs.</comment>
<file context>
@@ -1202,19 +1208,35 @@ async fn send_low_balance_notifications(
let name = user.display_name.as_deref().unwrap_or(&user.username);
- if let Err(e) = email_service.send_low_balance_email(&user.email, Some(name), &balance).await {
+ let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| {
crate::background_error!(
- NOTIFICATIONS, "email_send", Warning,
</file context>
| let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| { | ||
| crate::background_error!( | ||
| NOTIFICATIONS, "email_send", Warning, | ||
| NOTIFICATIONS, "low_balance_recipients", Warning, | ||
| user_id = %user.id, | ||
| email = %user.email, | ||
| error = %e, | ||
| "Failed to send low-balance notification email" | ||
| "Failed to list organization admins for a low-balance notification" | ||
| ); | ||
| continue; | ||
| Vec::new() | ||
| }); |
There was a problem hiding this comment.
P2: When list_admin_emails fails, the fallback to an empty recipient list silently removes all owners and admins from the alert, and if the contact email is then delivered the org is permanently marked as notified — the admins never got the email and never get retried. This differs from the intentional partial email-send trade-off documented above: on a listing failure nothing has been sent to the admins yet, so skipping this account (continue) and retrying next tick would re-mail nobody. Return the error out of the listing instead of degrading to contact-only, or the low-balance alert can miss every credit-adding member for good after a single transient query failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dwctl/src/notifications.rs, line 1211:
<comment>When `list_admin_emails` fails, the fallback to an empty recipient list silently removes all owners and admins from the alert, and if the contact email is then delivered the org is permanently marked as notified — the admins never got the email and never get retried. This differs from the intentional partial email-send trade-off documented above: on a listing failure nothing has been sent to the admins yet, so skipping this account (`continue`) and retrying next tick would re-mail nobody. Return the error out of the listing instead of degrading to contact-only, or the low-balance alert can miss every credit-adding member for good after a single transient query failure.</comment>
<file context>
@@ -1202,19 +1208,35 @@ async fn send_low_balance_notifications(
let name = user.display_name.as_deref().unwrap_or(&user.username);
- if let Err(e) = email_service.send_low_balance_email(&user.email, Some(name), &balance).await {
+ let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| {
crate::background_error!(
- NOTIFICATIONS, "email_send", Warning,
</file context>
| let admin_emails = Organizations::new(&mut *conn).list_admin_emails(user.id).await.unwrap_or_else(|e| { | |
| crate::background_error!( | |
| NOTIFICATIONS, "email_send", Warning, | |
| NOTIFICATIONS, "low_balance_recipients", Warning, | |
| user_id = %user.id, | |
| email = %user.email, | |
| error = %e, | |
| "Failed to send low-balance notification email" | |
| "Failed to list organization admins for a low-balance notification" | |
| ); | |
| continue; | |
| Vec::new() | |
| }); | |
| let admin_emails = match Organizations::new(&mut *conn).list_admin_emails(user.id).await { | |
| Ok(emails) => emails, | |
| Err(e) => { | |
| crate::background_error!( | |
| NOTIFICATIONS, "low_balance_recipients", Warning, | |
| user_id = %user.id, | |
| error = %e, | |
| "Failed to list organization admins for a low-balance notification" | |
| ); | |
| continue; | |
| } | |
| }; |
Summary
Low-balance notifications for an organization were sent only to the organization's contact email, which is typically the address of whoever created it. Other owners and admins, who can also add credits, never heard about a low balance.
The notification now goes to the organization's contact email plus every active owner and admin. Plain members are not mailed, soft-deleted accounts are skipped (via the existing
list_admin_emailsquery), and each address is mailed once, case-insensitively. Personal accounts have no members, so their behaviour is unchanged.The notification-sent flag is set once at least one recipient was reached. Leaving it unset on a partial failure would re-mail the recipients who already got it on every tick.
Tests
test_low_balance_recipients_lists_each_address_once: de-duplication of the recipient list.test_low_balance_notification_reaches_org_owners_and_admins: builds an org with an owner, an admin and a member, sends through the file email transport, and asserts the contact address, owner and admin each get exactly one email, the member gets none, and the org is marked as notified.cargo test --lib notifications::passes (20 tests).