Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
412 changes: 412 additions & 0 deletions frontend/src/__tests__/purchase-execution-toast.test.ts

Large diffs are not rendered by default.

49 changes: 49 additions & 0 deletions frontend/src/__tests__/recommendations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ describe('Recommendations Module', () => {
</div>
<div id="purchase-modal" class="hidden">
<div id="purchase-details"></div>
<div class="modal-buttons">
<button type="button" id="close-purchase-modal-btn">Cancel</button>
<button type="button" id="execute-purchase-btn" class="primary">Send for Approval</button>
</div>
</div>
`;

Expand Down Expand Up @@ -851,6 +855,51 @@ describe('Recommendations Module', () => {

await expect(openPurchaseModal([])).resolves.not.toThrow();
});

// Issue #288: the primary action does NOT execute the purchase — it
// sends an approval-request email. Pin the post-fix label + body
// wording so a regression that reverts to the misleading "Execute
// Purchase" framing fails this suite.
describe('approval-required messaging (issue #288)', () => {
const baseRec = {
id: 'rec-288',
provider: 'aws' as const,
service: 'ec2',
resource_type: 't3.medium',
region: 'us-east-1',
count: 5,
term: 1,
savings: 100,
upfront_cost: 500,
};

test('primary button reads "Send for Approval", not "Execute Purchase"', async () => {
await openPurchaseModal([baseRec]);

const btn = document.getElementById('execute-purchase-btn');
expect(btn?.textContent).toBe('Send for Approval');
// Belt-and-braces: ensure no element on the rendered modal still
// carries the pre-#288 text — guards against a future template
// re-introducing the misleading wording somewhere new.
const modal = document.getElementById('purchase-modal');
expect(modal?.textContent).not.toContain('Execute Purchase');
});

test('modal body carries the approval-required explanation', async () => {
await openPurchaseModal([baseRec]);

const details = document.getElementById('purchase-details');
expect(details?.textContent).toContain('will email an approval request');
});

test('approval-required note renders with its dedicated class', async () => {
await openPurchaseModal([baseRec]);

const note = document.querySelector('#purchase-details .approval-required-note');
expect(note).not.toBeNull();
expect(note?.textContent).toMatch(/approval request/i);
});
});
});

describe('refreshRecommendations', () => {
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/api/purchases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ export interface RetryPurchaseResult {
retry_attempt_n: number;
email_sent?: boolean;
email_reason?: string;
// Resolved To address that received the approval email; surfaced so
// the post-submit toast can name the approver per CR pass on PR #294.
// Absent when recipient resolution itself failed (no approvers configured).
approval_recipient?: string;
}

export async function retryPurchase(
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ export interface PurchaseResult {
// History instead of waiting for an inbox.
email_sent?: boolean;
email_reason?: string;
// Resolved To address that received the approval email. Surfaced so the
// post-submit toast can name the approver ("Approval request sent to
// alice@acme.com") per the CR pass on PR #294 / issue #288. Absent when
// recipient resolution itself failed (no approvers configured).
approval_recipient?: string;
results?: Array<{
recommendation_id: string;
status: string;
Expand Down
94 changes: 75 additions & 19 deletions frontend/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,11 +275,17 @@ async function handleExecutePurchase(): Promise<void> {
return;
}

// Default approval-required path: clicking sends an approval request to
// the configured approver(s) — it does NOT spend money. The actual
// upfront charge fires only after an approver clicks the email link.
// Issue #289 will introduce a session-permission branch where holders
// of `execute-any:purchases` can opt into direct execution; until that
// lands, every user is on this approval path.
const ok = await confirmDialog({
title: `Execute ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'}?`,
body: 'This will spend real money on cloud commitments. Make sure the selection + terms + payment options are what you intend.',
confirmLabel: 'Execute purchases',
destructive: true,
title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`,
body: 'This will email an approval request to the configured approver. Cloud commitments are charged only after the approver clicks the link in that email.',
confirmLabel: 'Send for approval',
destructive: false,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
if (!ok) return;

Expand Down Expand Up @@ -319,7 +325,7 @@ async function handleExecutePurchase(): Promise<void> {
const executeBtn = document.getElementById('execute-purchase-btn') as HTMLButtonElement | null;
if (executeBtn) {
executeBtn.disabled = true;
executeBtn.textContent = 'Executing...';
executeBtn.textContent = 'Sending...';
}

try {
Expand All @@ -339,16 +345,28 @@ async function handleExecutePurchase(): Promise<void> {
timeout: null,
});
} else {
showToast({ message: 'Purchase submitted — check your email to approve.', kind: 'success', timeout: 10_000 });
// Name the approver in the success toast so the user can confirm WHO
// received the request (CR pass on PR #294 / issue #288). The backend
// surfaces `approval_recipient` from `resolveApprovalRecipients`; the
// field may be absent on older deploys or when only the global notify
// mailbox is configured — fall back to the generic line in that case.
const recipient = result.approval_recipient;
showToast({
message: recipient
? `Approval request sent to ${recipient}.`
: 'Purchase submitted — check your email to approve.',
kind: 'success',
timeout: 10_000,
});
}
await loadDashboard();
} catch (error) {
const err = error as Error;
showToast({ message: `Failed to execute purchase: ${err.message}`, kind: 'error' });
showToast({ message: `Failed to send purchase for approval: ${err.message}`, kind: 'error' });
} finally {
if (executeBtn) {
executeBtn.disabled = false;
executeBtn.textContent = 'Execute Purchase';
executeBtn.textContent = 'Send for Approval';
}
}
}
Expand All @@ -363,18 +381,21 @@ async function handleExecutePurchase(): Promise<void> {
* confirmDialog.
*/
async function handleFanOutExecute(buckets: FanOutBucket[]): Promise<void> {
// Same approval-required default as the single-purchase path: each
// bucket POSTs a request that triggers an approval email; the actual
// charges fire when each approver clicks the link in their email.
const ok = await confirmDialog({
title: `Execute ${buckets.length} bulk purchase${buckets.length === 1 ? '' : 's'}?`,
body: `This will submit ${buckets.length} separate purchase execution${buckets.length === 1 ? '' : 's'} and send ${buckets.length} approval email${buckets.length === 1 ? '' : 's'}. Each must be approved individually.`,
confirmLabel: 'Execute all',
destructive: true,
title: `Send ${buckets.length} bulk purchase${buckets.length === 1 ? '' : 's'} for approval?`,
body: `This will submit ${buckets.length} separate purchase request${buckets.length === 1 ? '' : 's'} and email ${buckets.length} approval request${buckets.length === 1 ? '' : 's'}. Each must be approved individually before its commitments are charged.`,
confirmLabel: 'Send all for approval',
destructive: false,
});
if (!ok) return;

const executeBtn = document.getElementById('execute-purchase-btn') as HTMLButtonElement | null;
if (executeBtn) {
executeBtn.disabled = true;
executeBtn.textContent = `Executing 0/${buckets.length}…`;
executeBtn.textContent = `Sending 0/${buckets.length}…`;
}

// Fire all POSTs in parallel via allSettled so one failure doesn't
Expand Down Expand Up @@ -402,22 +423,57 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise<void> {
);
const results = await Promise.allSettled(promises);

const succeeded = results.filter((r) => r.status === 'fulfilled').length;
// Reclassify business-level email failures: a fulfilled POST that returns
// email_sent === false or status === 'failed' is not a true success —
// the approval email never went out (CR pass on PR #294 Finding 2).
const fulfilled = results.filter(
(r): r is PromiseFulfilledResult<api.PurchaseResult> => r.status === 'fulfilled',
);
const submissionFailures = fulfilled.filter(
(r) => r.value.email_sent === false || r.value.status === 'failed',
);
const succeeded = fulfilled.length - submissionFailures.length;
const failed = results.length - succeeded;
closePurchaseModal();
clearFanOutBuckets();
clearPurchaseModalRecommendations();

if (failed === 0) {
// Collect the unique approval-recipient set from truly-succeeded responses
// only (email_sent !== false and status !== 'failed') so the toast doesn't
// name a recipient whose email never arrived. Multi-bucket purchases can
// route to different approvers; dedupe so the toast is compact.
const recipients = new Set<string>();
for (const r of fulfilled) {
if (
r.value.email_sent !== false &&
r.value.status !== 'failed' &&
r.value.approval_recipient
) {
recipients.add(r.value.approval_recipient);
}
}
const noun = succeeded === 1 ? 'purchase' : 'purchases';
let message: string;
if (recipients.size === 0) {
message = `${succeeded} ${noun} submitted — check your email to approve each.`;
} else if (recipients.size === 1) {
message = `${succeeded} ${noun} sent for approval to ${[...recipients][0]}.`;
} else {
message = `${succeeded} ${noun} sent for approval to ${recipients.size} recipients (${[...recipients].sort().join(', ')}).`;
}
showToast({
message: `${succeeded} purchase${succeeded === 1 ? '' : 's'} submitted — check your email to approve each.`,
message,
kind: 'success',
timeout: 15_000,
});
} else {
const failureMsgs = results
.filter((r): r is PromiseRejectedResult => r.status === 'rejected')
.map((r) => (r.reason instanceof Error ? r.reason.message : String(r.reason)))
const failureMsgs = [
...results
.filter((r): r is PromiseRejectedResult => r.status === 'rejected')
.map((r) => (r.reason instanceof Error ? r.reason.message : String(r.reason))),
...submissionFailures.map((r) => r.value.email_reason || 'approval email did not send'),
]
.slice(0, 3)
.join('; ');
showToast({
Expand All @@ -430,7 +486,7 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise<void> {

if (executeBtn) {
executeBtn.disabled = false;
executeBtn.textContent = 'Execute Purchase';
executeBtn.textContent = 'Send for Approval';
}
}

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ <h2 id="purchase-modal-title">Configure Purchase</h2>
<div id="purchase-details"></div>
<div class="modal-buttons">
<button type="button" id="close-purchase-modal-btn">Cancel</button>
<button type="button" id="execute-purchase-btn" class="primary">Execute Purchase</button>
<button type="button" id="execute-purchase-btn" class="primary">Send for Approval</button>
</div>
</div>
</div>
Expand Down
18 changes: 16 additions & 2 deletions frontend/src/recommendations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2081,7 +2081,7 @@ function handleBulkPurchaseClick(recommendations: LocalRecommendation[]): void {
}

// Single-bucket happy path: open the preview modal + submit via the
// existing execute-purchase flow. The modal's "Execute Purchase" button
// existing approval-request flow. The modal's "Send for Approval" button
// (wired in app.ts) picks up the recs via getPurchaseModalRecommendations.
// openPurchaseModal is async (issue #111 (iii): per-rec override
// prefetch); fire-and-forget — the modal is the user's surface.
Expand Down Expand Up @@ -2125,7 +2125,7 @@ export interface FanOutBucket {
paymentSource: 'override' | 'toolbar';
}

// Fan-out modal state. app.ts's Execute Purchase click reads these
// Fan-out modal state. app.ts's Send-for-Approval click reads these
// via getFanOutBuckets() to fire one POST per bucket. Cleared when
// the modal closes.
let currentFanOutBuckets: FanOutBucket[] | null = null;
Expand Down Expand Up @@ -2742,6 +2742,20 @@ export async function openPurchaseModal(recommendations: LocalRecommendation[]):
upfrontLine.appendChild(upfrontStrong);
summary.appendChild(upfrontLine);

// Approval-required note: clicking the modal's primary button does NOT
// execute the purchase — it sends an approval-request email to the
// configured approver(s). The actual upfront charges fire only when an
// approver clicks the link in that email. Issue #288 closed the
// earlier "Execute Purchase" button-label gap that implied immediate
// execution; #289 will introduce a session-permission branch where
// holders of `execute-any:purchases` can opt into direct execution and
// this note will become conditional on the resolved auth path.
const approvalNote = document.createElement('p');
approvalNote.className = 'approval-required-note';
approvalNote.textContent =
'Submitting will email an approval request to the configured approver — commitments are charged only after the approver clicks the link in that email.';
summary.appendChild(approvalNote);

container.appendChild(summary);

// Commitments table with per-row Term and Payment selects.
Expand Down
38 changes: 25 additions & 13 deletions internal/api/handler_purchases.go
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU
// `failed` so the user sees the reason in History; the linkage on
// the original row is unaffected (it points at the failed-again
// successor, which is exactly the audit trail we want).
emailSent, emailReason := h.sendPurchaseApprovalEmail(ctx, req, newExecution, failedExec.Recommendations, totalUpfront, totalSavings)
emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, newExecution, failedExec.Recommendations, totalUpfront, totalSavings)
status := h.finalizePurchaseStatus(ctx, newExecution, emailSent, emailReason)

resp := map[string]any{
Expand All @@ -580,6 +580,9 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU
if emailReason != "" {
resp["email_reason"] = emailReason
}
if recipient != "" {
resp["approval_recipient"] = recipient
}
return resp, nil
}

Expand Down Expand Up @@ -1092,7 +1095,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio
// best-effort and never blocks the response body; the returned
// email_sent / email_reason fields let the UI tell the user whether they
// should wait for an inbox or cancel/retry manually.
emailSent, emailReason := h.sendPurchaseApprovalEmail(ctx, req, execution, execReq.Recommendations, totalUpfront, totalSavings)
emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, execution, execReq.Recommendations, totalUpfront, totalSavings)
status := h.finalizePurchaseStatus(ctx, execution, emailSent, emailReason)

message := "Purchase execution created and pending approval"
Expand All @@ -1111,24 +1114,33 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio
if emailReason != "" {
resp["email_reason"] = emailReason
}
if recipient != "" {
resp["approval_recipient"] = recipient
}
return resp, nil
}

// sendPurchaseApprovalEmail sends an approval-request email for a newly created
// execution and returns a structured outcome:
// - (true, "") on successful send
// - (false, "<reason>") on any preflight gate or send error
// - (true, "", recipient) on successful send
// - (false, "<reason>", "") on any preflight gate or send error
// - (false, "<reason>", 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.
//
// 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.
func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, recs []config.RecommendationRecord, totalUpfront, totalSavings float64) (bool, string) {
func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, recs []config.RecommendationRecord, totalUpfront, totalSavings float64) (bool, string, string) {
if h.emailNotifier == nil {
return false, "email notifier not configured for this deployment"
return false, "email notifier not configured for this deployment", ""
}
globalCfg, err := h.config.GetGlobalConfig(ctx)
if err != nil {
logging.Errorf("Failed to load global config for approval email: %v", err)
return false, fmt.Sprintf("failed to load settings: %v", err)
return false, fmt.Sprintf("failed to load settings: %v", err), ""
}
globalNotify := ""
if globalCfg.NotificationEmail != nil {
Expand All @@ -1137,10 +1149,10 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam
to, cc, approvers, err := h.resolveApprovalRecipients(ctx, recs, globalNotify)
if err != nil {
logging.Errorf("Failed to resolve approval recipients: %v", err)
return false, fmt.Sprintf("failed to resolve recipients: %v", err)
return false, fmt.Sprintf("failed to resolve recipients: %v", err), ""
}
if to == "" {
return false, "no notification email set in Settings → General and no account contact emails configured"
return false, "no notification email set in Settings → General and no account contact emails configured", ""
}
summaries := make([]email.RecommendationSummary, 0, len(recs))
for _, rec := range recs {
Expand Down Expand Up @@ -1168,14 +1180,14 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam
logging.Errorf("Failed to send purchase approval email: %v", err)
switch {
case errors.Is(err, email.ErrNoRecipient):
return false, "no notification email set in Settings → General"
return false, "no notification email set in Settings → General", ""
case errors.Is(err, email.ErrNoFromEmail):
return false, "FROM_EMAIL not configured for this deployment"
return false, "FROM_EMAIL not configured for this deployment", to
default:
return false, fmt.Sprintf("send failed: %v", err)
return false, fmt.Sprintf("send failed: %v", err), to
}
}
return true, ""
return true, "", to
}

// resolveDashboardURL returns the absolute base URL to embed in email
Expand Down