From 0813f7d1192805d0cc132d3f37a4a8a20b6dc6db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 17:43:18 +0200 Subject: [PATCH 1/2] fix(api/approvals): use notification_email in approval_recipient toast 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. --- .../purchase-execution-toast.test.ts | 55 +++++++++++ internal/api/coverage_gaps_test.go | 99 +++++++++++++++++++ internal/api/handler_purchases.go | 35 +++++-- 3 files changed, 183 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 6c2317f7e..6f190e301 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -388,6 +388,61 @@ describe('handleExecutePurchase — single-record path', () => { }); }); +// Issue #735: toast must show notification_email (approval_recipient from API), +// not a hardcoded or per-account contact_email. The backend now sets +// approval_recipient = notification_email when configured; the toast must use +// whatever the API returns without a hardcoded fallback. +describe('issue #735 — toast uses API approval_recipient, not hardcoded email', () => { + beforeEach(() => { + jest.clearAllMocks(); + (recs.getFanOutBuckets as jest.Mock).mockReturnValue([]); + (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([buildMinimalRec()]); + (plans.closePurchaseModal as jest.Mock).mockImplementation(() => undefined); + }); + + afterEach(() => { + document.body.textContent = ''; + }); + + test('toast shows the notification_email returned by the API as approval_recipient', async () => { + const notificationEmail = 'admin@company.example'; + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-735', + status: 'queued', + email_sent: true, + approval_recipient: notificationEmail, + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(lastToastMessage()).toContain(notificationEmail); + expect(lastToastMessage()).toContain('Approval request sent to'); + expect(lastToastKind()).toBe('success'); + }); + + test('toast does not contain a different address when API returns notification_email', async () => { + const notificationEmail = 'admin@company.example'; + const contactEmail = 'contact@acct.example'; + // Simulate backend now returning notification_email (not contact_email). + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-735b', + status: 'queued', + email_sent: true, + approval_recipient: notificationEmail, + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(lastToastMessage()).toContain(notificationEmail); + expect(lastToastMessage()).not.toContain(contactEmail); + expect(lastToastKind()).toBe('success'); + }); +}); + describe('handleFanOutExecute — fan-out path', () => { function buildBucket(id: string, capacityPercent = 100) { return { diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index 4f5ad3256..999de28dc 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -768,6 +768,105 @@ func TestHandler_sendPurchaseApprovalEmail_NoNotificationEmail(t *testing.T) { ) } +// --------------------------------------------------------------------------- +// sendPurchaseApprovalEmail — approval_recipient uses notification_email (issue #735) +// --------------------------------------------------------------------------- + +// recordingEmailNotifier captures the NotificationData passed to +// SendPurchaseApprovalRequest so tests can assert on recipient fields. +type recordingEmailNotifier struct { + stubEmailNotifier + captured email.NotificationData +} + +func (r *recordingEmailNotifier) SendPurchaseApprovalRequest(_ context.Context, data email.NotificationData) error { + r.captured = data + return nil +} + +// TestHandler_sendPurchaseApprovalEmail_ResponseRecipientUsesNotificationEmail +// is the regression test for issue #735: when both a per-account contact_email +// and a global notification_email are configured, the approval_recipient field +// returned by sendPurchaseApprovalEmail must equal notification_email (the +// value the History UI shows via resolvePendingApproverEmail), not contact_email. +// The actual email To address remains the contact_email — the fix only affects +// the approval_recipient used in the post-submit toast. +func TestHandler_sendPurchaseApprovalEmail_ResponseRecipientUsesNotificationEmail(t *testing.T) { + ctx := context.Background() + notificationEmail := "admin@example.com" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: ¬ificationEmail, + }, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + notifier := &recordingEmailNotifier{} + h := &Handler{ + config: mockStore, + emailNotifier: notifier, + } + + exec := &config.PurchaseExecution{ + ExecutionID: "11111111-1111-1111-1111-111111111111", + ApprovalToken: "tok", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + emailSent, _, responseRecipient := h.sendPurchaseApprovalEmail(ctx, nil, exec, exec.Recommendations, 0, 0) + + require.True(t, emailSent, "email send must succeed") + // The response recipient surfaced in the toast must be the notification_email + // (consistent with History), not the per-account contact_email. + assert.Equal(t, notificationEmail, responseRecipient, + "approval_recipient must equal notification_email so the toast matches History (issue #735)") + // The actual email To must still be the contact_email (security model unchanged). + assert.Equal(t, contactEmail, notifier.captured.RecipientEmail, + "actual email To address must be the per-account contact_email") +} + +// TestHandler_sendPurchaseApprovalEmail_ResponseRecipientFallsBackToContactEmail +// covers the case where no notification_email is set: responseRecipient falls +// back to the per-account contact_email (the actual To address). +func TestHandler_sendPurchaseApprovalEmail_ResponseRecipientFallsBackToContactEmail(t *testing.T) { + ctx := context.Background() + contactEmail := "contact@acct.example.com" + accountID := "acct-2" + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + // NotificationEmail intentionally not set. + }, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + notifier := &recordingEmailNotifier{} + h := &Handler{ + config: mockStore, + emailNotifier: notifier, + } + + exec := &config.PurchaseExecution{ + ExecutionID: "22222222-2222-2222-2222-222222222222", + ApprovalToken: "tok2", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + emailSent, _, responseRecipient := h.sendPurchaseApprovalEmail(ctx, nil, exec, exec.Recommendations, 0, 0) + + require.True(t, emailSent, "email send must succeed") + // Without a notification_email, the response recipient falls back to contact_email. + assert.Equal(t, contactEmail, responseRecipient, + "when notification_email is absent, approval_recipient falls back to contact_email") +} + // --------------------------------------------------------------------------- // Helper types for tests above // --------------------------------------------------------------------------- diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index c00644a6f..ba0f89930 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1501,6 +1501,19 @@ func archeraEducationURL(dashboardBase string) string { return dashboardBase + "/archera-insurance" } +// approvalResponseRecipient returns the email address to surface in the +// approval_recipient API response field (and therefore in the post-submit toast). +// It returns globalNotify when set, matching the address the History handler shows +// for pending rows (resolvePendingApproverEmail also returns globalNotify first). +// Falls back to to (the per-account contact_email) when globalNotify is empty. +// Extracted to keep sendPurchaseApprovalEmail under the cyclomatic-complexity ceiling. +func approvalResponseRecipient(globalNotify, to string) string { + if globalNotify != "" { + return globalNotify + } + return to +} + // sendPurchaseApprovalEmail sends an approval-request email for a newly created // execution and returns a structured outcome: // - (true, "", recipient) on successful send @@ -1508,9 +1521,13 @@ func archeraEducationURL(dashboardBase string) string { // - (false, "", recipient) when send failed AFTER recipient resolution // (so the response can still surface who would have been notified) // -// `recipient` is the resolved To address per `resolveApprovalRecipients` — -// surfaced in the response so the post-submit toast can name the approver -// per CR pass on PR #294 / issue #288. +// `recipient` is the address surfaced in the post-submit toast. It is the +// Admin notification email (Settings -> General) when configured, matching the +// value the History UI shows for pending rows via resolvePendingApproverEmail. +// When no notification email is set, it falls back to the per-account +// contact_email (the actual To address). This fixes issue #735 where the toast +// named the per-account contact_email instead of the Admin notification email, +// creating a discrepancy with the History "awaiting approval from X" display. // // Errors are also logged at Errorf level so they show up in CloudWatch, but // the reason string is what the API response surfaces to the UI. @@ -1535,6 +1552,12 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam if to == "" { return false, "no notification email set in Settings → General and no account contact emails configured", "" } + // responseRecipient is the email address surfaced in the UI toast (approval_recipient + // API field). It matches what the History handler shows for pending rows via + // resolvePendingApproverEmail, which always returns globalNotify when set. Using + // globalNotify here keeps both displays consistent. When globalNotify is empty, + // fall back to to (the per-account contact_email). See issue #735. + responseRecipient := approvalResponseRecipient(globalNotify, to) summaries := make([]email.RecommendationSummary, 0, len(recs)) for _, rec := range recs { summaries = append(summaries, email.RecommendationSummary{ @@ -1565,12 +1588,12 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam case errors.Is(err, email.ErrNoRecipient): return false, "no notification email set in Settings → General", "" case errors.Is(err, email.ErrNoFromEmail): - return false, "FROM_EMAIL not configured for this deployment", to + return false, "FROM_EMAIL not configured for this deployment", responseRecipient default: - return false, fmt.Sprintf("send failed: %v", err), to + return false, fmt.Sprintf("send failed: %v", err), responseRecipient } } - return true, "", to + return true, "", responseRecipient } // resolveDashboardURL returns the absolute base URL to embed in email From 01440cc7fd147747a90b163801a9ebde4fd0fb9c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 18:28:13 +0200 Subject: [PATCH 2/2] fix(approvals): trim whitespace before notification_email emptiness check (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. --- internal/api/handler_purchases.go | 11 ++++++----- internal/api/handler_purchases_test.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ba0f89930..943662a21 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1503,13 +1503,14 @@ func archeraEducationURL(dashboardBase string) string { // approvalResponseRecipient returns the email address to surface in the // approval_recipient API response field (and therefore in the post-submit toast). -// It returns globalNotify when set, matching the address the History handler shows -// for pending rows (resolvePendingApproverEmail also returns globalNotify first). -// Falls back to to (the per-account contact_email) when globalNotify is empty. +// It returns globalNotify when set (after trimming whitespace), matching the +// address the History handler shows for pending rows (resolvePendingApproverEmail +// also returns globalNotify first). Falls back to to (the per-account +// contact_email) when globalNotify is empty or whitespace-only. // Extracted to keep sendPurchaseApprovalEmail under the cyclomatic-complexity ceiling. func approvalResponseRecipient(globalNotify, to string) string { - if globalNotify != "" { - return globalNotify + if trimmed := strings.TrimSpace(globalNotify); trimmed != "" { + return trimmed } return to } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 111c483b4..c6c976d95 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2572,3 +2572,21 @@ func TestResolveOpsHint(t *testing.T) { }) } } + +// TestHandler_approvalResponseRecipient_TrimsWhitespace verifies that a +// whitespace-only notification_email does not count as set: the contact_email +// fallback must be used instead, and no whitespace must appear in the response. +func TestHandler_approvalResponseRecipient_TrimsWhitespace(t *testing.T) { + result := approvalResponseRecipient(" \t\n ", "contact@example.com") + assert.Equal(t, "contact@example.com", result, + "whitespace-only globalNotify must fall back to contact email") +} + +// TestHandler_approvalResponseRecipient_TrimsNonEmptyValue verifies that when +// notification_email has surrounding whitespace the returned value is trimmed, +// so no stray spaces appear in the toast or email headers. +func TestHandler_approvalResponseRecipient_TrimsNonEmptyValue(t *testing.T) { + result := approvalResponseRecipient(" cristi@example.com ", "contact@example.com") + assert.Equal(t, "cristi@example.com", result, + "globalNotify with surrounding whitespace must be returned trimmed") +}