Skip to content

fix(api/approvals): toast and history use Admin notification_email, not per-account contact (closes #735) - #740

Merged
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/735-notification-email-toast
May 27, 2026
Merged

fix(api/approvals): toast and history use Admin notification_email, not per-account contact (closes #735)#740
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/735-notification-email-toast

Conversation

@cristim

@cristim cristim commented May 26, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #735 (P1). Diverging "approver" identity surfaced in two UIs for the same execution: toast said Approval request sent to jcorbett@archera.ai, Purchase History said awaiting approval from cristi@leanercloud.com.

Root cause: sendPurchaseApprovalEmail's API response set approval_recipient to the to field returned by resolveApprovalRecipients, which uses the per-account contact_email (e.g. the Archera contact) as primary to with notification_email only on Cc. Meanwhile, Purchase History's resolvePendingApproverEmail reads globalCfg.NotificationEmail directly. The two paths diverged whenever a per-account contact_email was set.

Fix

Added approvalResponseRecipient helper; changed responseRecipient to prefer globalNotify (the Admin-set notification_email) over the per-account to. Toast and Purchase History now read the same identity.

Test plan

  • TestHandler_sendPurchaseApprovalEmail_ResponseRecipientUsesNotificationEmail
  • TestHandler_sendPurchaseApprovalEmail_ResponseRecipientFallsBackToContactEmail
  • Frontend purchase-execution-toast.test.ts 2 new #735 tests
  • Backend 1233 pass, frontend 1933 pass
  • gocyclo -over 10: zero violations
  • Manual: set Admin notification_email; trigger Azure approval; toast and Purchase History should name the same email

Summary by CodeRabbit

  • New Features

    • Approval success notifications now correctly display the configured notification recipient email address, with fallback to account-specific email when not configured.
  • Tests

    • Added comprehensive test coverage for purchase approval email recipient handling and fallback behavior.

Review Change Stack

…t field

The success toast after submitting an Azure purchase for approval was naming
the per-account contact_email (e.g. jcorbett@archera.ai) as the addressee,
while the Purchase History row correctly showed the Admin-set notification_email
(e.g. cristi@leanercloud.com). The two UIs used different data sources for the
same "who received the approval email" field.

Root cause: sendPurchaseApprovalEmail returned the resolveApprovalRecipients `to`
address (contact_email when present) as approval_recipient. But the History
handler's resolvePendingApproverEmail always reads globalCfg.NotificationEmail.

Fix: compute responseRecipient = globalNotify when set, else fall back to `to`.
This matches the History handler's resolution logic exactly so both UIs show the
same address. The actual email delivery (To = contact_email, Cc = notification_email)
and the authorised-approver security model are unchanged.

Tests:
- Backend: TestHandler_sendPurchaseApprovalEmail_ResponseRecipientUsesNotificationEmail
  asserts responseRecipient = notificationEmail when both are configured, and
  that the actual email To remains contact_email (security model intact).
- Backend: TestHandler_sendPurchaseApprovalEmail_ResponseRecipientFallsBackToContactEmail
  asserts the contact_email fallback when no notification_email is set.
- Frontend: two tests in issue #735 describe block assert the toast shows
  whatever approval_recipient the API returns.

Fixes #735.
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/many Affects most users labels May 26, 2026
@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 46 minutes and 58 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2713438d-1781-4612-a487-548300a727c2

📥 Commits

Reviewing files that changed from the base of the PR and between 0813f7d and 01440cc.

📒 Files selected for processing (2)
  • internal/api/handler_purchases.go
  • internal/api/handler_purchases_test.go
📝 Walkthrough

Walkthrough

The PR fixes bug #735 by ensuring the approval-success toast and Purchase History consistently display the Admin-configured notification email instead of a hardcoded per-account contact email. The backend computes and returns the correct recipient, and the frontend validates the toast shows the API-provided data.

Changes

Approval recipient correction

Layer / File(s) Summary
API approval recipient selection
internal/api/handler_purchases.go
Added approvalResponseRecipient(globalNotify, to) helper to prefer global notification email; updated sendPurchaseApprovalEmail to compute and return this recipient in all response paths (success and error cases).
Backend regression tests and test infrastructure
internal/api/coverage_gaps_test.go
Introduced recordingEmailNotifier test stub to capture email data, plus two regression tests asserting that responseRecipient uses the notification email when set and falls back to contact email when absent.
Frontend toast assertion tests
frontend/src/__tests__/purchase-execution-toast.test.ts
Added test suite for issue #735 verifying the approval toast contains the API-returned approval recipient and does not surface a different hardcoded email.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • LeanerCloud/CUDly#294: Modifies approval-email recipient selection in sendPurchaseApprovalEmail and corresponding frontend toast tests.

Suggested labels

effort/m, type/bug


🐰 A toast that whispers the truth,
No hardcoded secrets of yore,
The Admin's email shines through,
Consistent across each door,
Bug #735 is no more! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: aligning the approval toast and history to use the Admin-set notification_email instead of per-account contact email, directly addressing the issue.
Linked Issues check ✅ Passed The PR successfully addresses all coding objectives from issue #735: added backend tests verifying responseRecipient uses notification_email with fallback logic, frontend toast tests, and modified sendPurchaseApprovalEmail to return the correct approval_recipient value.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the notification_email divergence: test additions, the approvalResponseRecipient helper, and sendPurchaseApprovalEmail updates are all necessary to resolve the issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/735-notification-email-toast

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 1510-1514: The function approvalResponseRecipient should trim
globalNotify before testing emptiness so whitespace-only notification_email
doesn't count as set; update approvalResponseRecipient to call
strings.TrimSpace(globalNotify) and check that for != "" before returning it,
and apply the same trim-and-check fix where resolveApprovalRecipients selects
approval_recipient (the same recipient-selection logic around lines 1555-1560)
so contact email fallback works when notification_email is only whitespace.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2fadcc29-4188-410e-9662-79c9c8277ff6

📥 Commits

Reviewing files that changed from the base of the PR and between 6fcf6a6 and 0813f7d.

📒 Files selected for processing (3)
  • frontend/src/__tests__/purchase-execution-toast.test.ts
  • internal/api/coverage_gaps_test.go
  • internal/api/handler_purchases.go

Comment thread internal/api/handler_purchases.go
…heck (CR #740)

approvalResponseRecipient now calls strings.TrimSpace(globalNotify) before
testing for non-empty, so a whitespace-only notification_email value does not
qualify as set. The trimmed value is returned to the caller, preventing stray
spaces from appearing in the toast or email headers.

resolveApprovalRecipients already performs the same trim at line 1653; this
brings approvalResponseRecipient into parity.

Regression tests added:
- TestHandler_approvalResponseRecipient_TrimsWhitespace: whitespace-only falls
  back to contact_email.
- TestHandler_approvalResponseRecipient_TrimsNonEmptyValue: padded address is
  returned trimmed.

Fixes: CodeRabbit finding on PR #740.
@cristim

cristim commented May 26, 2026

Copy link
Copy Markdown
Member Author

Addressed the CodeRabbit finding: approvalResponseRecipient now calls strings.TrimSpace(globalNotify) before testing emptiness, so a whitespace-only notification_email falls back to the contact email rather than being returned as-is. The trimmed value is used in the return path so no stray spaces appear in the toast or email headers.

resolveApprovalRecipients already had the same trim at line 1653; this brings approvalResponseRecipient into parity.

Two regression tests added:

  • TestHandler_approvalResponseRecipient_TrimsWhitespace -- whitespace-only globalNotify falls back to contact@example.com
  • TestHandler_approvalResponseRecipient_TrimsNonEmptyValue -- padded address is returned trimmed

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

@cristim Thanks for the thorough follow-up! The strings.TrimSpace parity with resolveApprovalRecipients is the right call, and the two regression tests covering the whitespace-fallback and trim-non-empty paths look like solid coverage for the edge case. Triggering a review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 67cd495 into feat/multicloud-web-frontend May 27, 2026
5 checks passed
@cristim
cristim deleted the fix/735-notification-email-toast branch June 3, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact/many Affects most users priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant