From 99e7c226307b4443cca5eb2fa18eb52948a70080 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 22 Jul 2026 23:46:14 +0200 Subject: [PATCH] fix(marketplace): fail loud instead of listing no-upfront RIs at $0 resolveMarketplacePriceSchedule's default-schedule branch silently clamped a negative computed list price to 0 instead of rejecting it. A no-upfront RI (upfrontCost <= 0) or one with an unknown/elapsed term (originalTerm <= 0) makes marketplaceResidualPerUnit return 0, so the RI would be listed on AWS Marketplace for free instead of erroring out like the supplied-schedule branch already does for a non-positive price. The backend now returns an explicit error asking the caller to supply a price_schedule. The sell-consent-modal preview in frontend/src/history.ts had a parallel bug: it computed the preview price with a different formula than the backend (including recurring monthly cost, which the backend deliberately excludes, and not dividing by instance count), so it could show a nonzero preview price for exactly the case the backend now rejects, and diverge from the real listing price for multi-count RIs. The preview now mirrors marketplaceResidualPerUnit and resolveMarketplacePriceSchedule's default branch exactly, and shows "Default list price: unavailable (no upfront cost or unknown term)" instead of a fabricated or zero price when no default can be computed. Adds TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected (backend) and two regression tests in history-marketplace-sell-button.test.ts (frontend): one pinning the per-unit formula against the old recurring-inclusive row-total formula for a multi-count RI, one asserting the no-upfront case shows "unavailable" and never "$0". Follow-up to #808, found during an adversarial review sweep. --- .../history-marketplace-sell-button.test.ts | 90 +++++++++++++++++++ frontend/src/history.ts | 47 +++++++--- internal/api/handler_marketplace.go | 9 +- internal/api/handler_marketplace_test.go | 23 +++++ 4 files changed, 153 insertions(+), 16 deletions(-) diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index 50c8e4503..fc468bc99 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -303,4 +303,94 @@ describe('Marketplace consent modal residual proration (issue #808 follow-up)', // regression back to the $0 / ~1/3 value. expect(text).not.toContain('$0'); }); + + // Regression for the consent-modal math bug (PR follow-up to #808): the + // modal used to compute totalValue = upfrontRemaining + monthly * + // remainingMonths on the ROW TOTAL, which both (a) includes recurring cost + // the backend deliberately excludes, and (b) is not divided by count. The + // previous test above pins monthly_cost: 0, count: 1, which hides the bug + // entirely (both formulas agree when monthly is 0 and count is 1). These + // cases use a nonzero monthly_cost and count > 1 to actually exercise the + // divergence and pin the modal to the backend's real per-unit formula. + test('shows the backend-matching per-unit price, not the old recurring-inclusive row total', async () => { + const JUST_NOW = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); // ~1 day ago + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'ri-multi', + offering_class: 'standard', + term: 1, // 1 year = 12 months + timestamp: JUST_NOW, + count: 3, + upfront_cost: 1200, // row total across all 3 instances + monthly_cost: 50, + }), + ], + }); + + await loadHistory(); + + const sellBtn = document + .getElementById('history-list')! + .querySelector('.history-marketplace-sell-btn[data-marketplace-sell-id="ri-multi"]'); + expect(sellBtn).not.toBeNull(); + + sellBtn!.click(); + await Promise.resolve(); + await Promise.resolve(); + + const arg = (confirmDialog as jest.Mock).mock.calls[0][0] as { body: HTMLElement }; + const text = arg.body.textContent || ''; + + // Backend math (mirrors marketplaceResidualPerUnit): per-unit residual = + // 1200 * (12/12) / 3 = 400; default per-unit price = 400 * 0.95 = 380; + // total across 3 units = 1140. Monthly cost is excluded entirely. + expect(text).toContain('$380/unit'); + expect(text).toContain('$1140 total for 3 units'); + // The old buggy formula (upfrontRemaining + monthly*remainingMonths, row + // total, no count division) produced totalValue=1800, listPrice=1710 -- + // a price the backend never actually lists at. Guard against regressing. + expect(text).not.toContain('$1710'); + }); + + test('shows default price as unavailable for a no-upfront RI instead of a fabricated price', async () => { + const JUST_NOW = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'ri-no-upfront', + offering_class: 'standard', + term: 1, + timestamp: JUST_NOW, + count: 1, + upfront_cost: 0, // No Upfront Standard RI + monthly_cost: 200, + }), + ], + }); + + await loadHistory(); + + const sellBtn = document + .getElementById('history-list')! + .querySelector('.history-marketplace-sell-btn[data-marketplace-sell-id="ri-no-upfront"]'); + expect(sellBtn).not.toBeNull(); + + sellBtn!.click(); + await Promise.resolve(); + await Promise.resolve(); + + const arg = (confirmDialog as jest.Mock).mock.calls[0][0] as { body: HTMLElement }; + const text = arg.body.textContent || ''; + + // A no-upfront RI has no residual to prorate; the backend now rejects the + // default schedule rather than silently listing at $0, so the modal must + // say pricing is unavailable rather than showing a nonzero recurring-cost- + // derived price (the old bug) or a misleading "$0" (would look free). + expect(text).toContain('Default list price'); + expect(text).toContain('unavailable'); + expect(text).not.toContain('$0'); + }); }); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index c70c94226..1aa48e7b6 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -1394,17 +1394,26 @@ function wireRowActionHandlers(container: HTMLElement): void { : 0; const remainingMonths = Math.max(0, Math.round(termMonths - elapsedMonths)); const upfront = purchase.upfront_cost ?? 0; - const monthly = purchase.monthly_cost ?? 0; - // Prorate the upfront cost to its residual value over the remaining - // term. Using the full upfront overstates the listing value for a - // partially elapsed RI (a 36-month RI at month 6 retains 30/36 of its - // upfront value). Mirrors resolveMarketplacePriceSchedule in - // internal/api/handler_marketplace.go, which drops the upfront term to - // 0 when the original term is unknown (<=0); we do the same here. - const upfrontRemaining = termMonths > 0 ? upfront * (remainingMonths / termMonths) : 0; - const totalValue = upfrontRemaining + monthly * remainingMonths; - const listPrice = totalValue * AWS_MARKETPLACE_BUYER_DISCOUNT; - const netProceeds = listPrice * AWS_MARKETPLACE_NET_FACTOR; + const count = purchase.count > 0 ? purchase.count : 1; + // Mirror marketplaceResidualPerUnit + resolveMarketplacePriceSchedule's + // default branch in internal/api/handler_marketplace.go EXACTLY, so + // this preview can never diverge from what the backend actually lists: + // - upfront-only: recurring (monthly) cost is deliberately excluded + // because the buyer assumes the recurring obligation post-transfer; + // - per instance: upfront_cost is the row total for `count` instances, + // but the AWS Marketplace price is per instance, so divide by count; + // - prorated: the upfront residual is scaled by remaining/original + // term (a 36-month RI at month 6 retains only 30/36 of its value); + // - zero when unpriceable: a no-upfront RI (upfront <= 0) or an + // unknown term (termMonths <= 0) has no residual to prorate, which + // is exactly when the backend now rejects the default schedule + // with an error instead of silently listing at $0. + const perUnitResidual = termMonths > 0 && upfront > 0 + ? (upfront * (remainingMonths / termMonths)) / count + : 0; + const listPricePerUnit = perUnitResidual * AWS_MARKETPLACE_BUYER_DISCOUNT; + const listPriceTotal = listPricePerUnit * count; + const netProceedsTotal = listPriceTotal * AWS_MARKETPLACE_NET_FACTOR; const summaryEl = document.createElement('dl'); summaryEl.className = 'marketplace-pricing-summary'; @@ -1420,9 +1429,19 @@ function wireRowActionHandlers(container: HTMLElement): void { addRow('Region', purchase.region || '-'); addRow('Resource type', purchase.resource_type || '-'); addRow('Remaining term', remainingMonths === 1 ? '1 month' : `${remainingMonths} months`); - addRow('Default list price', formatCurrency(listPrice)); - addRow(`AWS fee (${AWS_MARKETPLACE_FEE_PERCENT}%)`, formatCurrency(listPrice * (AWS_MARKETPLACE_FEE_PERCENT / 100))); - addRow('Estimated net proceeds', formatCurrency(netProceeds)); + if (listPricePerUnit > 0) { + addRow('Default list price', count > 1 + ? `${formatCurrency(listPricePerUnit)}/unit (${formatCurrency(listPriceTotal)} total for ${count} units)` + : formatCurrency(listPriceTotal)); + addRow(`AWS fee (${AWS_MARKETPLACE_FEE_PERCENT}%)`, formatCurrency(listPriceTotal * (AWS_MARKETPLACE_FEE_PERCENT / 100))); + addRow('Estimated net proceeds', formatCurrency(netProceedsTotal)); + } else { + // No default price can be computed (no upfront cost or unknown + // term) -- listing will be rejected server-side unless a custom + // price_schedule is supplied. Say so instead of showing a + // misleading $0 or fabricated price. + addRow('Default list price', 'unavailable (no upfront cost or unknown term)'); + } bodyEl.appendChild(summaryEl); } diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 4d5be54fb..8dbcd89c2 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -628,8 +628,13 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM remainingMonths = 1 // defensive: should not happen for an active RI } listPrice := marketplaceResidualPerUnit(remainingMonths, originalTerm, count, upfrontCost) * awsMarketplaceBuyerDiscountFactor - if listPrice < 0 { - listPrice = 0 + if listPrice <= 0 { + // A no-upfront RI (upfrontCost <= 0) or an unknown/elapsed term + // (originalTerm <= 0) makes marketplaceResidualPerUnit return 0, which + // would otherwise produce a $0 listing sent straight to AWS -- the same + // value the supplied-schedule branch above rejects with "price must be + // positive". Fail loud instead of silently listing the RI for free. + return nil, fmt.Errorf("cannot compute a default listing price for this RI (no upfront cost or unknown term to prorate); supply an explicit price_schedule") } return []MarketplacePriceTier{ {TermMonths: int64(remainingMonths), Price: listPrice}, diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index 9e06f550e..65f5c30b3 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -523,6 +523,29 @@ func TestResolveMarketplacePriceSchedule_PerUnitFloor(t *testing.T) { assert.InDelta(t, 21.0, schedule[0].Price, 0.001) } +// TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected verifies the +// fix for the $0 default listing bug: a no-upfront RI (upfrontCost <= 0) +// hitting the default-schedule branch (no supplied price_schedule) must +// return an error instead of silently computing a $0 listing price. +// marketplaceResidualPerUnit returns 0 whenever upfrontCost <= 0, and the +// pre-fix code sent that straight through as {Price: 0} -- the exact value +// the supplied-schedule branch above rejects with "price must be positive". +// This FAILS on the pre-fix code (no error, schedule[0].Price == 0) and +// PASSES after (explicit error, nil schedule). +func TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected(t *testing.T) { + schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, 0) + require.Error(t, err, "a no-upfront RI must not silently produce a $0 default listing") + assert.Nil(t, schedule) + assert.Contains(t, err.Error(), "cannot compute a default listing price") + + // Same failure mode when the term is unknown (originalTerm <= 0), e.g. an + // imported/external row where the contract term could not be resolved. + schedule, err = resolveMarketplacePriceSchedule(nil, 12, 0, 1, 1200) + require.Error(t, err, "an unknown term must not silently produce a $0 default listing") + assert.Nil(t, schedule) + assert.Contains(t, err.Error(), "cannot compute a default listing price") +} + // TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected verifies the // Defect A term-range guardrail: a tier whose TermMonths exceeds the RI's // remaining months is rejected outright (an out-of-range term would let a