diff --git a/frontend/src/__tests__/commitmentOptions.test.ts b/frontend/src/__tests__/commitmentOptions.test.ts index b9501eede..7b5d9d008 100644 --- a/frontend/src/__tests__/commitmentOptions.test.ts +++ b/frontend/src/__tests__/commitmentOptions.test.ts @@ -11,10 +11,12 @@ import { populatePaymentSelect, getPaymentLabel, normalizePaymentValue, + formatPaymentAdjustmentNotice, CommitmentConfig, PaymentOption, TermOption } from '../commitmentOptions'; +import type { PaymentAdjustment } from '../api/types'; describe('commitmentOptions', () => { describe('getCommitmentConfig', () => { @@ -633,8 +635,14 @@ describe('commitmentOptions', () => { expect(normalizePaymentValue('all-upfront', 'azure')).toBe('upfront'); }); - it('should convert partial-upfront to upfront for Azure', () => { - expect(normalizePaymentValue('partial-upfront', 'azure')).toBe('upfront'); + // Regression test for #1503: partial-upfront has no Azure equivalent + // and must land on 'monthly', never 'upfront'. Azure's billing plan is + // immutable after purchase and both plans cost the same total, so + // pre-selecting 'upfront' would hand the user an irreversible full + // upfront charge they never chose. Mirrors the Go-side assertion in + // internal/config/validation_test.go:TestNormalizePaymentOption. + it('should convert partial-upfront to monthly (not upfront) for Azure', () => { + expect(normalizePaymentValue('partial-upfront', 'azure')).toBe('monthly'); }); it('should convert no-upfront to monthly for Azure', () => { @@ -815,4 +823,62 @@ describe('commitmentOptions', () => { expect(isValidCombination('aws', 'rds', 1, 'partial-upfront')).toBe(true); }); }); + + // #1503: the backend coerces a payment option the target provider cannot + // express (Azure has only Upfront/Monthly) instead of rejecting it. That is + // only acceptable if the change is disclosed, so this formatter is the copy + // every purchase-submit path renders. A null return means "nothing to say". + describe('formatPaymentAdjustmentNotice', () => { + const adj = (over: Partial = {}): PaymentAdjustment => ({ + rec_index: 0, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'no azure equivalent', + ...over, + }); + + it('returns null when there is nothing to disclose', () => { + expect(formatPaymentAdjustmentNotice(undefined)).toBeNull(); + expect(formatPaymentAdjustmentNotice([])).toBeNull(); + }); + + it('names both the requested and the applied schedule for a single rec', () => { + const msg = formatPaymentAdjustmentNotice([adj()]); + // Human labels, not raw API tokens -- the user picked "Partial Upfront" + // in the UI, so that is the wording they will recognise. + expect(msg).toContain('Partial Upfront'); + expect(msg).toContain('Pay Monthly'); + expect(msg).toContain('AZURE'); + expect(msg).toContain('vm'); + // The raw tokens must not leak into user-facing copy. + expect(msg).not.toContain('partial-upfront'); + }); + + it('collapses a batch to the distinct requested -> applied pairs', () => { + const msg = formatPaymentAdjustmentNotice([ + adj({ rec_index: 0 }), + adj({ rec_index: 1 }), + adj({ rec_index: 2 }), + ]); + // Count reflects every affected rec... + expect(msg).toContain('3 recommendations'); + // ...but the identical mapping is stated once, not three times. + expect(msg?.match(/Partial Upfront/g)).toHaveLength(1); + }); + + it('lists every distinct mapping when a batch was coerced differently', () => { + const msg = formatPaymentAdjustmentNotice([ + adj({ requested_payment_option: 'partial-upfront', applied_payment_option: 'monthly' }), + adj({ requested_payment_option: 'all-upfront', applied_payment_option: 'upfront' }), + ]); + expect(msg).toContain('2 recommendations'); + expect(msg).toContain('Partial Upfront'); + expect(msg).toContain('All Upfront'); + expect(msg).toContain('Pay Monthly'); + expect(msg).toContain('Pay Upfront'); + }); + }); + }); diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 356cc39de..f35f6dd5d 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -173,6 +173,15 @@ function lastToastMessage(): string | null { return last?.querySelector('.toast-message')?.textContent ?? last?.textContent ?? null; } +/** Return the text of every rendered toast, oldest first. */ +function allToastMessages(): string[] { + const container = document.getElementById('toast-container'); + if (!container) return []; + return Array.from(container.querySelectorAll('.toast')).map( + (t) => t.querySelector('.toast-message')?.textContent ?? t.textContent ?? '', + ); +} + /** Return the kind class of the most recently rendered toast (success/error/warning). */ function lastToastKind(): string | null { const container = document.getElementById('toast-container'); @@ -842,3 +851,142 @@ describe('handleExecutePurchase — double-submit guard (#644)', () => { expect(btn.textContent).toBe('Send for Approval'); }); }); + +// ── #1503: payment-option coercion must be disclosed to the user ───────────── +// +// The backend does not reject a payment option the target provider cannot +// express (Azure has exactly two billing plans, so an inherited AWS-style +// 'partial-upfront' token has nowhere to land). It coerces to the closest +// supported schedule and reports it in `payment_adjustments`. That coercion is +// only defensible if the user is actually TOLD -- otherwise their billing +// schedule changes behind their back, which is the failure #1503 reported. +// +// These are the end-to-end guards: a green unit test on the formatter alone +// could not prove the notice survives to a toast the user actually sees. +describe('#1503 — payment-option coercion is disclosed in the purchase toast', () => { + function buildAzureBucket(id: string) { + return { + key: `key-${id}`, + label: `Bucket ${id}`, + provider: 'azure', + service: `svc-${id}`, + recs: [buildMinimalRec()], + payment: 'partial-upfront', + capacityPercent: 100, + }; + } + + 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('single path — partial-upfront coerced to monthly raises a warning toast', async () => { + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-adj-1', + status: 'queued', + email_sent: true, + approval_recipient: 'approver@example.com', + payment_adjustments: [ + { + rec_index: 0, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'payment option "partial-upfront" is not in azure\'s supported set', + }, + ], + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const messages = allToastMessages(); + // The success toast must still be shown -- disclosure is additive. + expect(messages.some((m) => m.includes('Approval request sent to'))).toBe(true); + // ...and the coercion must be surfaced, naming both schedules so the user + // can tell what they asked for from what they are actually getting. + const notice = messages.find((m) => m.includes('Billing schedule adjusted')); + expect(notice).toBeDefined(); + expect(notice).toContain('Partial Upfront'); + expect(notice).toContain('Pay Monthly'); + expect(notice).toContain('AZURE'); + // Warning, and non-expiring: an irreversible billing-schedule change must + // not scroll away before the user reads it. + expect(lastToastKind()).toBe('warning'); + }); + + test('single path — no adjustments means no extra toast (no noise on the common case)', async () => { + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-adj-2', + status: 'queued', + email_sent: true, + approval_recipient: 'approver@example.com', + // payment_adjustments intentionally absent: everything was canonical. + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const messages = allToastMessages(); + expect(messages.some((m) => m.includes('Billing schedule adjusted'))).toBe(false); + expect(lastToastKind()).toBe('success'); + }); + + test('fan-out path — adjustments from every bucket are disclosed, incl. email-failed buckets', async () => { + (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([]); + (recs.getFanOutBuckets as jest.Mock).mockReturnValue([ + buildAzureBucket('a'), + buildAzureBucket('b'), + ]); + + const adjustment = (recIndex: number) => ({ + rec_index: recIndex, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'no azure equivalent', + }); + + (api.executePurchase as jest.Mock) + .mockResolvedValueOnce({ + execution_id: 'exec-a', + status: 'queued', + email_sent: true, + approval_recipient: 'alice@example.com', + payment_adjustments: [adjustment(0)], + }) + // Bucket b's approval email failed to send, but the pending execution + // still exists carrying the coerced schedule, so its adjustment must + // be disclosed too rather than dropped with the "failed" bucket. + .mockResolvedValueOnce({ + execution_id: 'exec-b', + status: 'queued', + email_sent: false, + email_reason: 'SMTP timeout', + payment_adjustments: [adjustment(0)], + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const notice = allToastMessages().find((m) => m.includes('Billing schedule adjusted')); + expect(notice).toBeDefined(); + // Both buckets contributed, so the count must be 2 -- proving the + // email-failed bucket's coercion was not silently dropped. + expect(notice).toContain('2 recommendations'); + expect(notice).toContain('Partial Upfront'); + expect(notice).toContain('Pay Monthly'); + }); +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index ec7dd3786..ca89e1411 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -357,6 +357,26 @@ export interface DeploymentInfo { } // Purchase types + +/** + * PaymentAdjustment is one payment-option coercion notice returned by the + * purchase-execute endpoint (#1503 follow-up). Mirrors the backend + * PaymentAdjustment struct (internal/api/validation.go) field for field. + * + * Named (rather than inlined into PurchaseResult) so the presentation helper + * that turns these into user-facing copy + * (commitmentOptions.ts:formatPaymentAdjustmentNotice) can be typed and unit + * tested against the same shape the API actually returns. + */ +export interface PaymentAdjustment { + rec_index: number; + provider: string; + service: string; + requested_payment_option: string; + applied_payment_option: string; + reason: string; +} + export interface PurchaseResult { execution_id: string; status: string; @@ -378,6 +398,17 @@ export interface PurchaseResult { // True when the request was handled via the direct-execute path (issue // #289). Absent (undefined) on the standard approval-required flow. direct_execute?: boolean; + // Per-rec payment-option coercion notices (#1503 follow-up). Present only + // when the backend normalized a requested payment option onto a token that + // bills on a DIFFERENT schedule (e.g. Azure "partial-upfront" -> "monthly"): + // each entry names what was requested, what was actually applied, and why, + // so the UI can tell the user instead of silently changing the billing + // schedule. Absent (never an empty array) when every option was already + // canonical, and also when the backend only respelled a token into the + // provider's own vocabulary for the same schedule (Azure "all-upfront" -> + // "upfront") — that changes nothing the user pays, so warning about it + // would just teach them to dismiss the notice that matters. + payment_adjustments?: PaymentAdjustment[]; results?: Array<{ recommendation_id: string; status: string; diff --git a/frontend/src/app.ts b/frontend/src/app.ts index caf9a372b..27b449543 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -16,6 +16,7 @@ import { loadHistory, setupHistoryHandlers } from './history'; import { initSavingsHistory } from './modules/savings-history'; import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; +import { formatPaymentAdjustmentNotice } from './commitmentOptions'; import { confirmDialog } from './confirmDialog'; import { handlePurchaseDeeplink } from './purchases-deeplink'; import { handleArcheraDeeplink, openArcheraOfferModal } from './archera'; @@ -438,6 +439,16 @@ async function handleExecutePurchase(): Promise { timeout: 10_000, }); } + // Disclose any payment-option coercion the backend applied (#1503). This + // is a SEPARATE, non-expiring warning toast rather than an addition to the + // success copy above: the billing schedule changing is the one thing the + // user did not ask for, and on Azure it cannot be changed after purchase, + // so it must not scroll away inside a success message. + const paymentNotice = formatPaymentAdjustmentNotice(result.payment_adjustments); + if (paymentNotice) { + showToast({ message: paymentNotice, kind: 'warning', timeout: null }); + } + // Offer Archera Insurance immediately after the user approves the // pre-purchase confirmation and the approval-submission call succeeds // (issue #499 follow-up). Firing here, rather than after the async @@ -550,6 +561,18 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise { clearFanOutBuckets(); clearPurchaseModalRecommendations(); + // Disclose payment-option coercions across every bucket that reached the + // backend (#1503). Collected from all fulfilled responses, not just the + // truly-succeeded ones: a bucket whose approval email failed to send still + // created a pending execution carrying the coerced billing schedule, so the + // user needs to know before they approve it from History. + const fanOutNotice = formatPaymentAdjustmentNotice( + fulfilled.flatMap((r) => r.value.payment_adjustments ?? []), + ); + if (fanOutNotice) { + showToast({ message: fanOutNotice, kind: 'warning', timeout: null }); + } + 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 diff --git a/frontend/src/commitmentOptions.ts b/frontend/src/commitmentOptions.ts index abdc8c87d..a37898de3 100644 --- a/frontend/src/commitmentOptions.ts +++ b/frontend/src/commitmentOptions.ts @@ -5,6 +5,8 @@ * for each cloud provider and service combination. */ +import type { PaymentAdjustment } from './api/types'; + export interface PaymentOption { value: string; label: string; @@ -292,15 +294,32 @@ export async function fetchAndPopulateCommitmentOptions(fetchFn?: FetchLike): Pr } /** - * Map legacy AWS payment values to display labels + * Map legacy AWS payment values onto the provider-canonical token. + * + * MUST stay in lockstep with the Go-side mapping in + * internal/config/validation.go:crossProviderPaymentAlias — this function + * decides which option the plan/purchase dropdowns pre-select, so a + * disagreement means the UI shows one billing schedule while the backend + * canonicalizes to another. See #1503. + * + * Azure reservations offer exactly two billing plans, Upfront and Monthly, + * and the total cost is identical either way ("The total cost of up-front and + * monthly reservations is the same and you don't pay any extra fees when you + * choose to pay monthly" — + * https://learn.microsoft.com/en-us/azure/cost-management-billing/reservations/prepare-buy-reservation). + * There is no partial-upfront equivalent, so that token has to land on one of + * the two. It lands on 'monthly': the billing plan cannot be changed after + * purchase (same doc set, "Manage Azure Reservations"), so pre-selecting + * 'upfront' would put a full, irreversible upfront charge in front of a user + * who never asked for one. */ export function normalizePaymentValue(value: string, provider: string): string { // Handle legacy values or cross-provider values if (provider === 'azure') { - if (value === 'all-upfront' || value === 'partial-upfront') { + if (value === 'all-upfront') { return 'upfront'; } - if (value === 'no-upfront') { + if (value === 'no-upfront' || value === 'partial-upfront') { return 'monthly'; } } else if (provider === 'gcp') { @@ -309,3 +328,52 @@ export function normalizePaymentValue(value: string, provider: string): string { } return value; } + +/** + * Turn the backend's payment-option coercion notices into one line of + * user-facing copy, or null when there is nothing to disclose. + * + * Why this exists (#1503): the backend does not reject a payment option that + * the target provider cannot express (e.g. Azure has exactly two billing + * plans, Upfront and Monthly, so an inherited AWS-style 'partial-upfront' + * token has nowhere to land). It coerces to the closest supported schedule + * and reports the change in `payment_adjustments`. Coercing is only + * defensible if the user is actually told, so every purchase-submit path + * MUST render this notice -- otherwise the user's billing schedule changes + * behind their back, which is exactly the failure #1503 reported. + * + * The applied value, not the requested one, is what the purchase carries; + * the copy leads with that because it is the fact the user has to act on. + * Azure's billing plan cannot be changed after purchase. + */ +export function formatPaymentAdjustmentNotice( + adjustments: PaymentAdjustment[] | undefined, +): string | null { + if (!adjustments || adjustments.length === 0) return null; + + const only = adjustments.length === 1 ? adjustments[0] : undefined; + if (only) { + return ( + `Billing schedule adjusted: ${only.provider.toUpperCase()} ${only.service} does not offer ` + + `"${getPaymentLabel(only.requested_payment_option)}", so this purchase was applied as ` + + `"${getPaymentLabel(only.applied_payment_option)}".` + ); + } + + // Multiple recs: collapse to the distinct requested -> applied pairs so the + // toast stays short when a whole batch inherited the same legacy token, + // while still naming every distinct change that was made. + const pairs = [ + ...new Set( + adjustments.map( + a => + `"${getPaymentLabel(a.requested_payment_option)}" applied as ` + + `"${getPaymentLabel(a.applied_payment_option)}"`, + ), + ), + ]; + return ( + `Billing schedule adjusted on ${adjustments.length} recommendations ` + + `(unsupported payment option for the target provider): ${pairs.join('; ')}.` + ); +} diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 136ffaac9..2d59b2e44 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -295,7 +295,7 @@ func TestExecutedNotification_DirectExecutePath(t *testing.T) { } session := &Session{Email: adminEmail, UserID: "admin-uid"} - result, err := handler.directExecutePurchase(ctx, req, exec, session) + result, err := handler.directExecutePurchase(ctx, req, exec, session, nil) require.NoError(t, err) resultMap := result.(map[string]any) assert.Equal(t, "completed", resultMap["status"]) @@ -336,7 +336,7 @@ func TestExecutedNotification_DirectExecute_NilNotifierNoPanic(t *testing.T) { req := &events.LambdaFunctionURLRequest{} session := &Session{Email: adminEmail, UserID: "admin-uid"} - result, err := handler.directExecutePurchase(ctx, req, exec, session) + result, err := handler.directExecutePurchase(ctx, req, exec, session, nil) require.NoError(t, err) assert.Equal(t, "completed", result.(map[string]any)["status"]) mockPurchase.AssertExpectations(t) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4b9e4efca..2ea40a328 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2025,25 +2025,28 @@ type ExecutePurchaseRequest struct { // validateExecutePurchaseRequest handles the permission check, body parse, // and recommendation-list bounds + scope checks. Extracted so executePurchase -// itself stays linear and under the gocyclo threshold. -func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (ExecutePurchaseRequest, *Session, error) { +// itself stays linear and under the gocyclo threshold. The returned +// PaymentAdjustment slice carries any payment-option coercions performed by +// the per-rec validation, for the response to surface to the caller (#1503 +// follow-up); it is empty/nil when every rec was already canonical. +func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (ExecutePurchaseRequest, *Session, []PaymentAdjustment, error) { session, err := h.requirePermission(ctx, req, "execute", "purchases") if err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } var execReq ExecutePurchaseRequest if err := json.Unmarshal([]byte(req.Body), &execReq); err != nil { - return ExecutePurchaseRequest{}, nil, NewClientError(400, "invalid request body") + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, "invalid request body") } const maxRecommendations = 1000 if len(execReq.Recommendations) == 0 { - return ExecutePurchaseRequest{}, nil, NewClientError(400, "no recommendations provided") + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, "no recommendations provided") } if len(execReq.Recommendations) > maxRecommendations { - return ExecutePurchaseRequest{}, nil, NewClientError(400, fmt.Sprintf("too many recommendations: %d (max %d)", len(execReq.Recommendations), maxRecommendations)) + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, fmt.Sprintf("too many recommendations: %d (max %d)", len(execReq.Recommendations), maxRecommendations)) } if err := normalizeCapacityPercent(&execReq); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Scope: reject the whole request if any recommendation targets an // account outside the session's allowed_accounts. Safer than silently @@ -2052,7 +2055,7 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // Runs before per-rec content validation so an out-of-scope request is // rejected as 403 regardless of the rec's Term/Payment/Count contents. if err := h.validatePurchaseRecommendationScope(ctx, session, execReq.Recommendations); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Per-rec Provider/Service/Term/Payment/Count validation at the API // boundary so a malformed client-supplied rec (e.g. Term:7, Payment:"foo", @@ -2060,15 +2063,16 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // execute time (#643). This is scoped to the web execute path only — the // retry path replays recs from an already-validated execution and must // not be re-gated by the same rules. - if err := validateExecutePurchaseRecommendations(execReq.Recommendations); err != nil { - return ExecutePurchaseRequest{}, nil, err + adjustments, recErr := validateExecutePurchaseRecommendations(execReq.Recommendations) + if recErr != nil { + return ExecutePurchaseRequest{}, nil, nil, recErr } // Cross-check the audit-only capacity_percent against the scaled rec // counts so the persisted execution can't claim a capacity that // disagrees with what was actually purchased (#647). Skipped per-rec // when the rec carries no recommended_count. if err := validateCapacityConsistency(execReq.Recommendations, execReq.CapacityPercent); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Enforce the per-permission Constraints (MaxPurchaseAmount, Providers, // Services, Regions, AccountIDs) configured on the granting @@ -2080,9 +2084,9 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // splitting a large purchase across recs or by a no-upfront commitment // whose real cost is entirely recurring. if err := h.enforcePurchaseConstraints(ctx, session, execReq.Recommendations); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } - return execReq, session, nil + return execReq, session, adjustments, nil } // enforcePurchaseConstraints builds the per-recommendation @@ -2199,15 +2203,22 @@ func normalizeCapacityPercent(execReq *ExecutePurchaseRequest) error { // validateExecutePurchaseRecommendations runs the per-rec #643 boundary // validation over every rec in a web execute request, returning the first -// failure. Extracted so validateExecutePurchaseRequest stays under the -// gocyclo threshold. -func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) error { +// failure. On success it also returns the payment-option coercions that +// occurred (nil-free, in rec order) so the response can surface them to the +// caller (#1503 follow-up). Extracted so validateExecutePurchaseRequest stays +// under the gocyclo threshold. +func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) ([]PaymentAdjustment, error) { + var adjustments []PaymentAdjustment for i := range recs { - if err := validatePurchaseRecommendation(&recs[i], i); err != nil { - return err + adjustment, err := validatePurchaseRecommendation(&recs[i], i) + if err != nil { + return nil, err + } + if adjustment != nil { + adjustments = append(adjustments, *adjustment) } } - return nil + return adjustments, nil } // finalizePurchaseStatus flips an execution's stored status to "failed" if @@ -2482,7 +2493,7 @@ func newPendingExecution(req *ExecutePurchaseRequest, totalUpfront, totalSavings } func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - execReq, session, err := h.validateExecutePurchaseRequest(ctx, req) + execReq, session, paymentAdjustments, err := h.validateExecutePurchaseRequest(ctx, req) if err != nil { return nil, err } @@ -2531,7 +2542,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio } if dupExec != nil { logging.Infof("concurrent duplicate purchase submit collapsed to existing execution %s", dupExec.ExecutionID) - return buildDuplicatePurchaseResponse(dupExec), nil + return withPaymentAdjustments(buildDuplicatePurchaseResponse(dupExec), paymentAdjustments), nil } // Direct-execute path (issue #289): a session with execute-any or @@ -2546,7 +2557,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio if err := h.authorizeSessionExecuteDirect(ctx, session, creatorID); err != nil { return nil, err } - return h.directExecutePurchase(ctx, req, execution, session) + return h.directExecutePurchase(ctx, req, execution, session, paymentAdjustments) } // Send approval email synchronously so the response can surface the @@ -2557,7 +2568,18 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, execution, execReq.Recommendations, totalUpfront, totalSavings) status := h.finalizePurchaseStatus(ctx, execution, emailSent, emailReason) - return buildApprovalPendingResponse(executionID, status, len(execReq.Recommendations), totalUpfront, totalSavings, emailSent, emailReason, recipient), nil + return withPaymentAdjustments(buildApprovalPendingResponse(executionID, status, len(execReq.Recommendations), totalUpfront, totalSavings, emailSent, emailReason, recipient), paymentAdjustments), nil +} + +// withPaymentAdjustments attaches the payment-option coercion notices to an +// executePurchase response body under "payment_adjustments" (#1503 follow-up). +// The key is omitted entirely when no coercion happened, so existing clients +// see an unchanged response on the common canonical-input case. +func withPaymentAdjustments(resp map[string]any, adjustments []PaymentAdjustment) map[string]any { + if len(adjustments) > 0 { + resp["payment_adjustments"] = adjustments + } + return resp } // buildApprovalPendingResponse assembles the JSON-serialisable response body @@ -2612,7 +2634,9 @@ func buildApprovalPendingResponse( // This is the only email the direct-execute flow emits -- no approval // email precedes it -- so it is the path where the executed-notification // matters most. -// 4. Return a "completed" status to the caller. +// 4. Return a "completed" status to the caller, carrying any payment-option +// coercion notices (paymentAdjustments, from the request validation) so +// the direct-execute response surfaces them like the approval path does. // // The audit fields are best-effort if ApproveAndExecute's SavePurchaseExecution // races with our pre-call stamp -- but in practice ApproveAndExecute calls @@ -2621,7 +2645,7 @@ func buildApprovalPendingResponse( // TransitionExecutionStatus. The critical audit invariant is that a non-nil // executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, // and both are set atomically in the same SavePurchaseExecution call here. -func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session) (any, error) { +func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session, paymentAdjustments []PaymentAdjustment) (any, error) { t0 := time.Now() executionID := execution.ExecutionID logging.Infof("purchase[%s]: directExecutePurchase entry (auth=session)", executionID) @@ -2661,7 +2685,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF // guard live inside sendPurchaseExecutedEmail. session.Email is the actor // who direct-executed, matching the actor passed to ApproveAndExecute above. h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) - return map[string]any{ + return withPaymentAdjustments(map[string]any{ "execution_id": executionID, "status": "completed", "recommendation_count": len(execution.Recommendations), @@ -2669,7 +2693,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF "estimated_savings": execution.EstimatedSavings, "direct_execute": true, "message": "Purchase executed immediately (direct-execute permission).", - }, nil + }, paymentAdjustments), nil } // archeraEducationURL returns dashboardBase + "/archera-insurance", or "" when diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 400af8ce5..c15185d7a 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -6,7 +6,9 @@ import ( "testing" "time" + "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/CUDly/internal/config" @@ -42,83 +44,116 @@ func TestValidatePurchaseRecommendation(t *testing.T) { name string rec config.RecommendationRecord wantError bool + // wantPayment is set ONLY on rows whose input token gets rewritten: it + // is asserted against rec.Payment after a successful call. "" means the + // token was already canonical for the provider (case-only differences + // included). + wantPayment string + // wantAdjustment marks the rows whose rewrite changes the BILLING + // SCHEDULE rather than just the spelling, and so must surface a + // caller-visible PaymentAdjustment (#1503 follow-up). A rewrite between + // two spellings of the same schedule (Azure "all-upfront" -> "upfront") + // costs the customer nothing and must NOT be disclosed. Field-level + // assertions live in + // TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment and + // TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename. + wantAdjustment bool }{ // --- AWS canonical set --- - {"valid aws all-upfront 3y", validRec(), false}, - {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false}, - {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false}, - {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true}, - {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true}, + {"valid aws all-upfront 3y", validRec(), false, "", false}, + {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false, "", false}, + {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false, "", false}, + {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true, "", false}, + {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true, "", false}, // --- Azure canonical set --- - {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false}, - {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false}, - // Legacy AWS-style tokens on Azure are normalized to Azure-canonical before validation. - {"azure accepts legacy all-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false, "", false}, + {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false, "", false}, + // Legacy AWS-style tokens on Azure are normalized to Azure-canonical + // before validation. Both of these are pure respellings -- Azure's + // "upfront" IS all-upfront and its "monthly" IS no-upfront -- so the + // customer's cash flow is untouched and no adjustment is surfaced. + {"azure accepts legacy all-upfront (respelled upfront, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "all-upfront" - }), false}, - {"azure accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + }), false, "upfront", false}, + {"azure accepts legacy no-upfront (respelled monthly, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "no-upfront" - }), false}, - {"azure accepts legacy partial-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + }), false, "monthly", false}, + // partial-upfront has no Azure equivalent; it coerces to monthly (the + // no-upfront default), never to upfront, so the caller never gets + // silently billed an all-upfront schedule it did not choose (#1503). + // This one genuinely changes the schedule, so it must be disclosed. + {"azure accepts legacy partial-upfront (coerced to monthly, not upfront)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "partial-upfront" - }), false}, + }), false, "monthly", true}, {"azure rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "foo" - }), true}, + }), true, "", false}, // --- GCP canonical set (monthly-only) --- - {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false}, - // Legacy tokens on GCP are all normalized to monthly. + {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false, "", false}, + // Legacy tokens on GCP are all normalized to monthly. GCP has no + // upfront tier at all, so every upfront-shaped token really does move + // the customer onto a different schedule; only no-upfront is a + // respelling of the schedule they already asked for. {"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "upfront" - }), false}, + }), false, "monthly", true}, {"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "all-upfront" - }), false}, - {"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + }), false, "monthly", true}, + {"gcp accepts legacy no-upfront (respelled monthly, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "no-upfront" - }), false}, + }), false, "monthly", false}, {"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "foo" - }), true}, + }), true, "", false}, // --- General --- - {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false}, - {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true}, - {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true}, - {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true}, - {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true}, + {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false, "", false}, + {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true, "", false}, + {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true, "", false}, + {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true, "", false}, + {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true, "", false}, {"negative monthly cost rejected", mutate(func(r *config.RecommendationRecord) { m := -1.0 r.MonthlyCost = &m - }), true}, - {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false}, + }), true, "", false}, + {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false, "", false}, {"zero monthly cost accepted", mutate(func(r *config.RecommendationRecord) { m := 0.0 r.MonthlyCost = &m - }), false}, - {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true}, - {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true}, - {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true}, - {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true}, - {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true}, + }), false, "", false}, + {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true, "", false}, + {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true, "", false}, + {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true, "", false}, + {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true, "", false}, + {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true, "", false}, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() rec := tt.rec - err := validatePurchaseRecommendation(&rec, 0) + adjustment, err := validatePurchaseRecommendation(&rec, 0) if tt.wantError { require.Error(t, err) + return + } + require.NoError(t, err) + if tt.wantPayment != "" { + assert.Equal(t, tt.wantPayment, rec.Payment) + } + if tt.wantAdjustment { + require.NotNil(t, adjustment, "a billing-schedule change must surface a PaymentAdjustment") + assert.Equal(t, tt.wantPayment, adjustment.AppliedPaymentOption) } else { - require.NoError(t, err) + assert.Nil(t, adjustment, "the billing schedule did not change, so nothing should be disclosed") } }) } @@ -133,13 +168,266 @@ func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) { rec := validRec() rec.Provider = "azure" rec.Payment = "foo" // not in azure canonical set and has no normalization alias - err := validatePurchaseRecommendation(&rec, 0) + _, err := validatePurchaseRecommendation(&rec, 0) require.Error(t, err) assert.Contains(t, err.Error(), "azure") assert.Contains(t, err.Error(), "upfront") assert.Contains(t, err.Error(), "monthly") } +// TestValidatePurchaseRecommendation_NormalizationWarning is a regression test +// for #1504: the doc comment on config.NormalizePaymentOption +// (internal/config/validation.go) states that the caller is expected to WARN +// when a raw payment-option token differs from its normalized canonical form, +// so an operator can audit a money-affecting coercion. validatePurchaseRecommendation +// is that caller; it must emit a WARN log when normalization actually changes +// the value (raw != canonical). This test captures the default logger's +// output via captureDefaultLog (defined in handler_accounts_test.go, package +// api) and asserts the log line is emitted with the provider, service, raw +// and canonical values, and rec index. +// +// Not run with t.Parallel(): captureDefaultLog mutates the shared default +// logger's output, which would race against other SetOutput-using tests if +// parallelized (see TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak). +func TestValidatePurchaseRecommendation_NormalizationWarning(t *testing.T) { + logBuf := captureDefaultLog(t) + + // Azure partial-upfront has no direct Azure equivalent; NormalizePaymentOption + // coerces it to "monthly" (#1503). This is a real raw != canonical + // transition and must be logged. + rec := validRec() + rec.Provider = "azure" + rec.Payment = "partial-upfront" + _, err := validatePurchaseRecommendation(&rec, 7) + require.NoError(t, err) + // The money-affecting coercion itself must be unchanged by adding the log. + assert.Equal(t, "monthly", rec.Payment, "coercion behavior must stay partial-upfront -> monthly, not upfront") + + logged := logBuf.String() + assert.Contains(t, logged, "[WARN]") + assert.Contains(t, logged, "rec 7 (azure/ec2)", "warning must identify the rec index, provider and service for audit") + assert.Contains(t, logged, `raw="partial-upfront"`) + assert.Contains(t, logged, `canonical="monthly"`) +} + +// TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical guards that +// an already-canonical payment option (no real normalization) does not emit a +// WARN log: only an actual raw->canonical transition should be observable, so +// operators aren't flooded with noise on the common case. +// +// Not run with t.Parallel(); see TestValidatePurchaseRecommendation_NormalizationWarning. +func TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical(t *testing.T) { + logBuf := captureDefaultLog(t) + + rec := validRec() + rec.Provider = "azure" + rec.Payment = "monthly" // already canonical for azure; no normalization occurs + adjustment, err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + assert.Nil(t, adjustment, "already-canonical payment option must not surface an adjustment") + assert.Equal(t, "monthly", rec.Payment) + + assert.Empty(t, logBuf.String(), "no normalization occurred, so nothing should be logged") +} + +// TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment pins the +// caller-facing coercion notice (#1503 follow-up): when the web execute path +// normalizes a payment option onto a DIFFERENT provider-canonical token, the +// returned PaymentAdjustment must identify the rec and carry the requested +// token, the applied token, and a reason naming both, so the API response can +// tell the caller what actually happened to this money-affecting field instead +// of only WARN-logging it for operators. +func TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment(t *testing.T) { + t.Parallel() + rec := validRec() + rec.Provider = "azure" + rec.Payment = "partial-upfront" + adjustment, err := validatePurchaseRecommendation(&rec, 3) + require.NoError(t, err) + require.NotNil(t, adjustment, "azure partial-upfront is coerced and must surface an adjustment") + assert.Equal(t, 3, adjustment.RecIndex) + assert.Equal(t, "azure", adjustment.Provider) + assert.Equal(t, rec.Service, adjustment.Service) + assert.Equal(t, "partial-upfront", adjustment.RequestedPaymentOption) + assert.Equal(t, "monthly", adjustment.AppliedPaymentOption) + // The reason must be self-explanatory to the caller: it names the rejected + // token, the applied token, and the provider whose billing model forced it. + assert.Contains(t, adjustment.Reason, `"partial-upfront"`) + assert.Contains(t, adjustment.Reason, `"monthly"`) + assert.Contains(t, adjustment.Reason, "azure") + // The adjustment must agree with the actual mutation applied to the rec: + // applied means applied, not merely advertised. + assert.Equal(t, adjustment.AppliedPaymentOption, rec.Payment) +} + +// TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename +// guards the other half of the #1503 disclosure contract: a coercion that only +// respells a token in the target provider's vocabulary must NOT be reported as +// an adjustment, because nothing about the customer's cash flow changed. +// +// This is not hypothetical noise-avoidance. The fan-out purchase modal's +// per-bucket Payment dropdown is populated from paymentOptionsFor +// (frontend/src/lib/purchase-compatibility.ts), whose candidate list has no +// "upfront" entry — so an Azure bucket the user pays upfront ALWAYS submits +// the AWS-style "all-upfront". Reporting that rename would put a sticky +// "Billing schedule adjusted" warning on every ordinary Azure upfront +// purchase, claiming a change that did not happen and drowning out the +// partial-upfront case that did. +// +// The operator WARN is deliberately still emitted for these: a non-canonical +// token on the wire is an upstream input bug worth auditing even when it costs +// the customer nothing. +// +// Not run with t.Parallel(): captureDefaultLog mutates the shared default +// logger's output (see TestValidatePurchaseRecommendation_NormalizationWarning). +func TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename(t *testing.T) { + tests := []struct { + name string + provider string + payment string + wantPayment string + }{ + // Azure spells AWS's all-upfront "upfront": same single charge at + // purchase, different word. + {"azure all-upfront is upfront respelled", "azure", "all-upfront", "upfront"}, + // Azure spells AWS's no-upfront "monthly": same per-period billing. + {"azure no-upfront is monthly respelled", "azure", "no-upfront", "monthly"}, + // GCP likewise has only the recurring schedule spelled "monthly". + {"gcp no-upfront is monthly respelled", "gcp", "no-upfront", "monthly"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + logBuf := captureDefaultLog(t) + + rec := validRec() + rec.Provider = tc.provider + rec.Payment = tc.payment + adjustment, err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + assert.Nil(t, adjustment, + "%s bills identically to %s; a rename must not be disclosed as a billing-schedule change", + tc.payment, tc.wantPayment) + // The canonicalization itself must still happen. + assert.Equal(t, tc.wantPayment, rec.Payment) + // ...and the operator-facing audit log must still fire. + assert.Contains(t, logBuf.String(), "[WARN]", + "a non-canonical token on the wire is still an upstream input bug worth logging") + }) + } +} + +// TestValidatePurchaseRecommendation_AdjustmentWhenScheduleChanges covers the +// non-Azure half of the same contract: GCP commitments are monthly-only, so an +// upfront-shaped token really does move the customer onto a different billing +// schedule and MUST be disclosed. +func TestValidatePurchaseRecommendation_AdjustmentWhenScheduleChanges(t *testing.T) { + t.Parallel() + for _, payment := range []string{"all-upfront", "upfront", "partial-upfront"} { + t.Run(payment, func(t *testing.T) { + t.Parallel() + rec := validRec() + rec.Provider = "gcp" + rec.Payment = payment + adjustment, err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + require.NotNil(t, adjustment, + "gcp has no upfront billing tier, so %q lands on a different schedule and must be disclosed", payment) + assert.Equal(t, payment, adjustment.RequestedPaymentOption) + assert.Equal(t, "monthly", adjustment.AppliedPaymentOption) + assert.Equal(t, "monthly", rec.Payment) + }) + } +} + +// TestHandler_executePurchase_SurfacesPaymentAdjustments is the response-level +// regression test for the #1503 follow-up: an Azure partial-upfront purchase +// submitted through the real executePurchase handler must return a +// payment_adjustments entry telling the caller the request was applied as +// monthly; a green validator-level test alone could not prove the notice +// survives to the response body the client actually sees. +func TestHandler_executePurchase_SurfacesPaymentAdjustments(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + // rec 0 is already canonical (azure/monthly); rec 1 carries the #1503 + // partial-upfront token and must be the only rec surfaced. + Body: `{"recommendations": [` + + `{"id": "rec-1", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "monthly", "upfront_cost": 100.0, "savings": 50.0},` + + `{"id": "rec-2", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "partial-upfront", "upfront_cost": 200.0, "savings": 25.0}]}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + + resultMap := result.(map[string]any) + adjustments, ok := resultMap["payment_adjustments"].([]PaymentAdjustment) + require.True(t, ok, "response must carry payment_adjustments when a coercion occurred") + require.Len(t, adjustments, 1, "only the coerced rec must be surfaced") + adj := adjustments[0] + assert.Equal(t, 1, adj.RecIndex, "adjustment must point at the coerced rec's position") + assert.Equal(t, "azure", adj.Provider) + assert.Equal(t, "vm", adj.Service) + assert.Equal(t, "partial-upfront", adj.RequestedPaymentOption) + assert.Equal(t, "monthly", adj.AppliedPaymentOption) + assert.NotEmpty(t, adj.Reason) +} + +// TestHandler_executePurchase_NoAdjustmentsWhenCanonical guards the +// backward-compat contract of the #1503 follow-up: when every payment option +// is already provider-canonical, the payment_adjustments key must be ABSENT +// (not an empty array), so existing clients see a byte-identical response on +// the common case. +func TestHandler_executePurchase_NoAdjustmentsWhenCanonical(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations": [{"id": "rec-1", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "monthly", "upfront_cost": 100.0, "savings": 50.0}]}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + + resultMap := result.(map[string]any) + assert.NotContains(t, resultMap, "payment_adjustments", + "canonical-only request must not carry the payment_adjustments key at all") +} + // The per-rec #643 validation is wired into the web execute boundary // (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations // which the retry path also calls with replayed recs. This test pins that diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 56da6b542..5878c99c2 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4110,7 +4110,7 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfE realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockStore}) handler := &Handler{config: mockStore, purchase: realManager} - _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, execution, ownerSession) + _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, execution, ownerSession, nil) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a clientError") diff --git a/internal/api/validation.go b/internal/api/validation.go index 9b5fdf34b..e9d080a64 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-lambda-go/events" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/logging" ) // Security constants. @@ -521,6 +522,34 @@ var purchaseTermWhitelist = map[string]map[int]bool{ "gcp": {1: true, 3: true}, } +// PaymentAdjustment surfaces a payment-option coercion to the API caller +// (follow-up to #1503): when the web execute path normalizes a caller-supplied +// payment option onto a provider-canonical token that bills on a DIFFERENT +// schedule than what was requested (e.g. Azure "partial-upfront" -> "monthly"), +// the response carries one of these per adjusted rec so the caller sees what +// they requested, what was actually applied, and why, not just the operator +// WARN log. Purely additive visibility: the coercion policy itself lives in +// config.NormalizePaymentOption and is unchanged. +// +// Cross-provider renames of the SAME schedule (Azure "all-upfront" -> +// "upfront", "no-upfront" -> "monthly") are not adjustments: nothing about +// the customer's cash flow changed, so there is nothing to disclose. See +// config.PaymentCoercionChangesSchedule. +type PaymentAdjustment struct { + // RecIndex is the rec's position in the request's recommendations slice. + RecIndex int `json:"rec_index"` + Provider string `json:"provider"` + Service string `json:"service"` + // RequestedPaymentOption is the caller's token after trim/lowercase only + // (case-only differences are not surfaced as adjustments, matching the + // operator WARN-log semantics). + RequestedPaymentOption string `json:"requested_payment_option"` + // AppliedPaymentOption is the provider-canonical token the purchase will + // actually carry. + AppliedPaymentOption string `json:"applied_payment_option"` + Reason string `json:"reason"` +} + // validatePurchaseRecommendation validates a single client-supplied // recommendation before it reaches the cloud purchase SDK. Unlike the // query-time validateProvider (which permits ""/"all"), the execute path @@ -536,22 +565,27 @@ var purchaseTermWhitelist = map[string]map[int]bool{ // 2. The provider-canonical set (from config.ValidPaymentOptionsByProvider) // rejects anything that has no canonical mapping, with an error that // names the provider and lists the accepted tokens. -func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) error { +// +// When step 1 changes the BILLING SCHEDULE (not merely the spelling), the +// returned *PaymentAdjustment describes the coercion so the response can +// surface it to the caller; nil means the payment option was already canonical +// or was respelled onto an equivalent schedule. +func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) (*PaymentAdjustment, error) { provider := strings.ToLower(strings.TrimSpace(rec.Provider)) payments := purchasePaymentSet(provider) if payments == nil { - return NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider)) } rec.Provider = provider rec.Service = strings.TrimSpace(rec.Service) if rec.Service == "" { - return NewClientError(400, fmt.Sprintf("recommendation %d is missing a service", idx)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d is missing a service", idx)) } if rec.Count <= 0 { - return NewClientError(400, fmt.Sprintf("recommendation %d has non-positive count: %d", idx, rec.Count)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has non-positive count: %d", idx, rec.Count)) } if !purchaseTermWhitelist[provider][rec.Term] { - return NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) } // A negative MonthlyCost would let recTotalCommitment's recurring leg // subtract from the batch's total commitment, offsetting or masking a @@ -559,24 +593,69 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e // review follow-up to #1210). Nil is fine (no recurring charge); only a // present-and-negative value is rejected. if rec.MonthlyCost != nil && *rec.MonthlyCost < 0 { - return NewClientError(400, fmt.Sprintf("recommendation %d has negative monthly cost: %.2f", idx, *rec.MonthlyCost)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has negative monthly cost: %.2f", idx, *rec.MonthlyCost)) } payment := strings.ToLower(strings.TrimSpace(rec.Payment)) // Coerce any legacy/cross-provider alias before the whitelist check so // that callers using old AWS-style tokens are transparently redirected to - // the canonical token for the target provider. + // the canonical token for the target provider. WARN when a real + // normalization occurs (raw != canonical) so an operator can audit the + // coercion of this money-affecting field, matching the WARN contract + // documented on config.NormalizePaymentOption and mirroring the same + // coerced/uncoerced logging convertRecommendations does at the + // scheduler's emission boundary (internal/scheduler/scheduler.go). + // + // The caller-facing PaymentAdjustment is deliberately NARROWER than the + // WARN: it is returned only when the rewrite changes the billing schedule + // itself, not when it merely respells the same schedule in the target + // provider's vocabulary (Azure "all-upfront" -> "upfront", "no-upfront" -> + // "monthly"). Every WARN is worth an operator's attention because it means + // an upstream caller sent a non-canonical token; only a schedule change is + // worth interrupting the user, whose money is what actually moved. The + // fan-out purchase modal submits "all-upfront" for Azure buckets by + // construction (frontend/src/lib/purchase-compatibility.ts), so surfacing + // renames too would warn on the ordinary Azure upfront purchase and teach + // users to dismiss the notice that matters. + var adjustment *PaymentAdjustment if normalized, ok := config.NormalizePaymentOption(provider, payment); ok { + if normalized != payment { + logging.Warnf("validatePurchaseRecommendation: rec %d (%s/%s) payment option normalized: raw=%q canonical=%q", + idx, provider, rec.Service, payment, normalized) + } + adjustment = paymentAdjustmentFor(idx, provider, rec.Service, payment, normalized) payment = normalized } if !payments[payment] { - return NewClientError(400, fmt.Sprintf( + return nil, NewClientError(400, fmt.Sprintf( "invalid payment option for %s service: %q (valid for %s: %s)", provider, rec.Payment, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), )) } rec.Payment = payment - return nil + return adjustment, nil +} + +// paymentAdjustmentFor builds the caller-facing coercion notice for a single +// rec, or returns nil when there is nothing to disclose: either the token was +// left alone, or it was only respelled into the target provider's vocabulary +// for the same billing schedule (see config.PaymentCoercionChangesSchedule). +// +// requested is the caller's token after trim/lowercase; applied is the +// provider-canonical token the purchase will actually carry. +func paymentAdjustmentFor(idx int, provider, service, requested, applied string) *PaymentAdjustment { + if !config.PaymentCoercionChangesSchedule(requested, applied) { + return nil + } + return &PaymentAdjustment{ + RecIndex: idx, + Provider: provider, + Service: service, + RequestedPaymentOption: requested, + AppliedPaymentOption: applied, + Reason: fmt.Sprintf("payment option %q is not in %s's supported set (%s); the closest supported option %q was applied", + requested, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), applied), + } } // validateCapacityConsistency cross-checks the client-supplied capacity_percent diff --git a/internal/config/validation.go b/internal/config/validation.go index 2f5d80677..409902d76 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -93,8 +93,18 @@ func validPaymentOptionsFor(provider string) []string { // // - AWS : passthrough (AWS already speaks the three-tier set). // - Azure: all-upfront → upfront, no-upfront → monthly, -// partial-upfront → upfront (no semantic equivalent — coerce to the -// all-upfront tier rather than drop the rec; caller may log). +// partial-upfront → monthly (no semantic equivalent — coerce to the +// no-upfront tier, CUDly's default billing schedule for Azure, rather +// than drop the rec; caller may log). Coercing to upfront here would +// silently bill an all-upfront schedule the caller never chose: the +// canonical token is persisted onto the execution and copied into +// common.Recommendation.PaymentOption (internal/purchase/execution.go), +// and the reservation-purchase billingPlan wiring being added in +// #1495/#1502 maps only upfront/monthly tokens and hard-errors on +// partial-upfront, so landing on monthly keeps the rec on the +// no-upfront schedule instead of an irreversible upfront charge — +// Azure does not allow changing a reservation's billing frequency +// after purchase. // - GCP : all-upfront → monthly, no-upfront → monthly, // partial-upfront → monthly, upfront → monthly (GCP CUDs are // inherently monthly-billed — every non-monthly token collapses to the @@ -106,9 +116,26 @@ func validPaymentOptionsFor(provider string) []string { // // The partial-upfront coercion is deliberate on both Azure and GCP: dropping // the rec would be a silent data loss for the user, while coercing to the -// closest billing model the provider offers preserves the rec. The caller is -// expected to log a warning when raw != canonical so an operator notices the -// upstream input bug. +// closest billing model the provider offers preserves the rec without +// changing the total the caller committed to. Microsoft documents that +// choice as cost-neutral: "The total cost of up-front and monthly +// reservations is the same and you don't pay any extra fees when you choose +// to pay monthly" +// (https://learn.microsoft.com/en-us/azure/cost-management-billing/reservations/prepare-buy-reservation). +// Caveat from the same page: monthly payments are NOT offered for SUSE Linux +// reservations, Red Hat plans, Azure Red Hat OpenShift licenses, or +// pre-purchase plans, so for those products the coerced token can make Azure +// reject the purchase. That is the intended failure mode — a loud purchase- +// time rejection is preferable to silently committing the caller to an +// upfront charge they never chose and cannot undo. +// +// The caller is expected to log a warning when raw != canonical so an +// operator notices the upstream input bug. +// +// The frontend mirrors this mapping in +// frontend/src/commitmentOptions.ts:normalizePaymentValue (it decides which +// option the plan/purchase dropdowns pre-select); the two must stay in +// lockstep. // // Empty raw passes through as ("", true) — callers that distinguish "unset" // from "invalid" can check the returned bool only when raw is non-empty. @@ -146,12 +173,20 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { case "azure": // Azure reservations model both billing plans; coerce AWS-style tokens // to the Azure-canonical spelling. partial-upfront has no Azure - // equivalent — coerce to the all-upfront tier so the rec survives - // validation rather than dropping silently (caller WARN-logs). + // equivalent — coerce to the no-upfront (monthly) tier, CUDly's + // default billing schedule, so the rec survives validation rather + // than dropping silently (caller WARN-logs). Coercing to "upfront" + // would silently bill an upfront schedule the caller never chose + // and cannot undo (Azure forbids changing a reservation's billing + // frequency after purchase), whereas monthly costs the same total. + // The reservation-purchase billingPlan wiring being added in + // #1495/#1502 hard-errors on partial-upfront, so only the + // canonical upfront/monthly tokens survive to the purchase body. + // Rationale and doc citations: see NormalizePaymentOption above. switch raw { - case "all-upfront", "partial-upfront": + case "all-upfront": return "upfront", true - case "no-upfront": + case "no-upfront", "partial-upfront": return "monthly", true } case "gcp": @@ -166,6 +201,70 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { return "", false } +// PaymentSchedule is the cash-flow shape a payment-option token implies, +// stripped of the provider-specific spelling of that token. Two tokens that +// map to the same PaymentSchedule bill the customer identically; only the +// word differs (AWS spells all-upfront what Azure spells upfront, and AWS +// spells no-upfront what Azure and GCP spell monthly). +// +// It exists so callers can tell a rename apart from a real change when +// NormalizePaymentOption rewrites a token: a rename is bookkeeping, a real +// change moves the customer's money and has to be disclosed (#1503). +type PaymentSchedule string + +const ( + // PaymentScheduleUpfront: the whole commitment is charged once, at purchase. + PaymentScheduleUpfront PaymentSchedule = "upfront" + // PaymentSchedulePartialUpfront: part is charged at purchase, the rest recurs. + PaymentSchedulePartialUpfront PaymentSchedule = "partial-upfront" + // PaymentScheduleRecurring: nothing is charged at purchase; the commitment + // is billed per period across the term. + PaymentScheduleRecurring PaymentSchedule = "recurring" + // PaymentScheduleUnknown: the token is not one of the modeled schedules. + // Two unrecognized tokens compare equal under this classification, so + // callers that must distinguish them have to compare the raw tokens too. + // In practice unmapped tokens never reach a coercion comparison: + // NormalizePaymentOption returns ok=false for them and the validator + // rejects them at the next boundary. + PaymentScheduleUnknown PaymentSchedule = "unknown" +) + +// PaymentScheduleFor classifies a payment-option token by the billing +// schedule it implies. The token must already be lowercased and trimmed +// (validatePurchaseRecommendation and NormalizePaymentOption both work on +// such tokens). +func PaymentScheduleFor(token string) PaymentSchedule { + switch token { + case "all-upfront", "upfront": + return PaymentScheduleUpfront + case "partial-upfront": + return PaymentSchedulePartialUpfront + case "no-upfront", "monthly": + return PaymentScheduleRecurring + default: + return PaymentScheduleUnknown + } +} + +// PaymentCoercionChangesSchedule reports whether rewriting raw to canonical +// actually changes what the customer pays and when, as opposed to merely +// renaming the same schedule into the target provider's vocabulary. +// +// Azure "all-upfront" -> "upfront" and Azure/GCP "no-upfront" -> "monthly" +// are renames: identical cash flow, different spelling. Azure +// "partial-upfront" -> "monthly" and GCP "upfront" -> "monthly" are real +// changes: the customer is billed on a schedule they did not ask for. +// +// Only real changes are worth putting in front of a user (#1503). The +// fan-out purchase modal submits the AWS-style "all-upfront" for Azure +// buckets by construction (frontend/src/lib/purchase-compatibility.ts: +// paymentOptionsFor), so treating every rewrite as a change would fire a +// "billing schedule adjusted" warning on the ordinary Azure upfront +// purchase and train users to dismiss the one notice that matters. +func PaymentCoercionChangesSchedule(raw, canonical string) bool { + return PaymentScheduleFor(raw) != PaymentScheduleFor(canonical) +} + // ValidOfferingClasses lists the accepted EC2 RI offering class values for // GlobalConfig. The empty string is also accepted (maps to "convertible" at // purchase time to preserve pre-694 behavior). diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 94aa7ad02..118d5c710 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -986,7 +986,7 @@ func TestNormalizePaymentOption(t *testing.T) { // Azure: AWS-style aliases coerced to canonical. {"azure all-upfront → upfront", "azure", "all-upfront", "upfront", true}, {"azure no-upfront → monthly", "azure", "no-upfront", "monthly", true}, - {"azure partial-upfront → upfront (nearest)", "azure", "partial-upfront", "upfront", true}, + {"azure partial-upfront → monthly (default, not upfront)", "azure", "partial-upfront", "monthly", true}, // GCP: canonical passthrough (monthly-only — every non-monthly token // collapses to monthly because GCP CUDs only model one billing plan). @@ -1021,6 +1021,68 @@ func TestNormalizePaymentOption(t *testing.T) { } } +// TestPaymentScheduleFor pins the token -> cash-flow classification that lets +// callers tell a cross-provider RENAME apart from a real billing-schedule +// change (#1503). Getting this wrong in either direction is a user-visible +// defect: over-classifying warns on ordinary purchases until users learn to +// dismiss the warning, under-classifying hides a schedule change the customer +// never chose. +func TestPaymentScheduleFor(t *testing.T) { + tests := []struct { + token string + want PaymentSchedule + }{ + // One charge at purchase, spelled two ways. + {"all-upfront", PaymentScheduleUpfront}, + {"upfront", PaymentScheduleUpfront}, + // AWS's middle tier; no Azure or GCP equivalent. + {"partial-upfront", PaymentSchedulePartialUpfront}, + // Nothing at purchase, billed per period; spelled two ways. + {"no-upfront", PaymentScheduleRecurring}, + {"monthly", PaymentScheduleRecurring}, + // Not a modeled schedule. + {"", PaymentScheduleUnknown}, + {"ohai", PaymentScheduleUnknown}, + } + for _, tt := range tests { + t.Run(tt.token, func(t *testing.T) { + assert.Equal(t, tt.want, PaymentScheduleFor(tt.token)) + }) + } +} + +// TestPaymentCoercionChangesSchedule walks every rewrite crossProviderPaymentAlias +// can actually perform and pins whether it is a disclosable change. The pairs +// are the ones NormalizePaymentOption produces, so this test breaks if a future +// mapping change silently flips a rename into a schedule change or vice versa. +func TestPaymentCoercionChangesSchedule(t *testing.T) { + tests := []struct { + name string + raw string + canonical string + want bool + }{ + // Azure vocabulary: same cash flow, different word. + {"azure all-upfront respelled upfront", "all-upfront", "upfront", false}, + {"azure no-upfront respelled monthly", "no-upfront", "monthly", false}, + // Azure has no partial tier, so this really moves the money. + {"azure partial-upfront becomes monthly", "partial-upfront", "monthly", true}, + // GCP is monthly-only: upfront-shaped tokens all change the schedule. + {"gcp upfront becomes monthly", "upfront", "monthly", true}, + {"gcp all-upfront becomes monthly", "all-upfront", "monthly", true}, + {"gcp partial-upfront becomes monthly", "partial-upfront", "monthly", true}, + {"gcp no-upfront respelled monthly", "no-upfront", "monthly", false}, + // No rewrite at all. + {"identity", "monthly", "monthly", false}, + {"empty identity", "", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, PaymentCoercionChangesSchedule(tt.raw, tt.canonical)) + }) + } +} + func TestIsValidRampScheduleType(t *testing.T) { assert.True(t, isValidRampScheduleType("immediate")) assert.True(t, isValidRampScheduleType("weekly"))