From b082f634823590120028fa0863d88eebfceb7d5e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 19:50:35 +0300 Subject: [PATCH 1/2] fix(api/marketplace): convert RI term years->months in resale pricing (follow-up to #808) purchase_history.term is stored in years (1 or 3), confirmed by migration 000007 ("valid terms are 0, 1, or 3 (years)") and execution.go formatting it as "%dyr". The marketplaceList handler was passing row.Term unchanged to computeRemainingMonths (param: termMonths int) and resolveMarketplacePriceSchedule (param: originalTerm, documented as months), silently treating years as months. Impact for a 3-year RI sold 6 months in: pre-fix: remainingMonths = max(1, 3-6) = 1; default price ~= $1,140 (3600 * 1/3 * 0.95) post-fix: remainingMonths = 30; default price ~= $2,850 (3600 * 30/36 * 0.95) A caller-supplied {term_months: 30} schedule was also rejected pre-fix (30 > 1 remaining), preventing sellers from specifying the correct term. Fix: guard row.Term <= 0 (error, not silent fallback), then multiply by 12 at the boundary where years enter the pricing math and pass termMonths to both call sites. Update standardRow() in tests from Term:12 (nonsensical 12 years, accidentally masked the bug) to Term:3. Add TestComputeRemainingMonths unit test and TestMarketplaceList_TermYearsConvertedToMonths end-to-end regression test that fails pre-fix and passes post-fix. --- internal/api/handler_marketplace.go | 13 +- internal/api/handler_marketplace_test.go | 149 ++++++++++++++++++++++- 2 files changed, 157 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index fb0afd296..4d5be54fb 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -142,16 +142,25 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio return nil, err } + // purchase_history.term is stored in years (1 or 3); both + // computeRemainingMonths and resolveMarketplacePriceSchedule operate in + // months. Reject a zero or negative term rather than silently computing + // garbage on this money path (no-silent-fallbacks policy). + if row.Term <= 0 { + return nil, fmt.Errorf("purchase has invalid term %d (expected 1 or 3 years); cannot compute marketplace pricing", row.Term) + } + termMonths := row.Term * 12 + // Compute actual remaining months from the purchase timestamp and total // term so the default price schedule reflects real remaining value // rather than the full contract term (which overprices older RIs). - remainingMonths := computeRemainingMonths(row.Timestamp, row.Term) + remainingMonths := computeRemainingMonths(row.Timestamp, termMonths) // Validate and normalise the price schedule. Row-total UpfrontCost and the // instance Count are passed so pricing is computed per instance; recurring // (monthly) cost is deliberately excluded because the Marketplace buyer // assumes recurring charges post-transfer (issue #292 money-path review). - schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.Term, row.Count, row.UpfrontCost) + schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, termMonths, row.Count, row.UpfrontCost) if err != nil { return nil, NewClientError(400, err.Error()) } diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index c504b5ef0..9e06f550e 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -101,9 +101,12 @@ func standardRow() *config.PurchaseHistoryRecord { CloudAccountID: &acct, Region: "us-east-1", Count: 3, - Term: 12, - UpfrontCost: 1200, - Timestamp: time.Now(), + // Term is stored in years (1 or 3) in purchase_history. The handler + // multiplies by 12 before passing to computeRemainingMonths and + // resolveMarketplacePriceSchedule, which both work in months. + Term: 3, + UpfrontCost: 1200, + Timestamp: time.Now(), } } @@ -604,3 +607,143 @@ func TestMarketplaceList_EmptyOfferingClassFetchedConvertible(t *testing.T) { assert.Contains(t, err.Error(), "Standard") cfgStore.AssertExpectations(t) } + +// --- Regression tests for the #808 follow-up: term years->months unit conversion --- + +// TestComputeRemainingMonths exercises the helper directly, confirming it treats +// its termMonths parameter as months (not years). +func TestComputeRemainingMonths(t *testing.T) { + // Zero purchase time -> defensive floor. + assert.Equal(t, 1, computeRemainingMonths(time.Time{}, 36), "zero time should return 1") + + // Non-positive term -> defensive floor. + assert.Equal(t, 1, computeRemainingMonths(time.Now(), 0), "zero term should return 1") + assert.Equal(t, 1, computeRemainingMonths(time.Now(), -1), "negative term should return 1") + + // Fresh purchase (elapsed ~ 0): 36-month term should return ~36. + r := computeRemainingMonths(time.Now(), 36) + assert.InDelta(t, 36, r, 1, "fresh 36-month RI should have ~36 months remaining") + + // 1-year RI: fresh purchase, 12-month term should return ~12. + r = computeRemainingMonths(time.Now(), 12) + assert.InDelta(t, 12, r, 1, "fresh 12-month RI should have ~12 months remaining") + + // 6 months elapsed on a 36-month term -> ~30 remaining. + sixMonthsAgo := time.Now().Add(-6 * 30 * 24 * time.Hour) + r = computeRemainingMonths(sixMonthsAgo, 36) + assert.InDelta(t, 30, r, 2, "36-month RI bought 6 months ago should have ~30 months remaining") + + // Fully elapsed term -> floor at 1, never zero or negative. + old := time.Now().Add(-40 * 30 * 24 * time.Hour) + assert.Equal(t, 1, computeRemainingMonths(old, 36), "expired RI should floor to 1") +} + +// TestMarketplaceList_TermYearsConvertedToMonths is the end-to-end regression +// test for the #808 follow-up money bug. purchase_history.term is stored in +// years (1 or 3); the handler was passing it unchanged to computeRemainingMonths +// (which expects months) and to resolveMarketplacePriceSchedule (originalTerm +// documented as months). For a 3-year RI sold 6 months in: +// +// pre-fix: remainingMonths = max(1, 3-6) = 1; default price ~= $1,140 (3600*1/3*0.95) +// post-fix: remainingMonths ~= 30; default price ~= $2,850 (3600*30/36*0.95) +// +// A caller-supplied schedule with term_months=30 was rejected pre-fix because +// remainingMonths was 1 (30 > 1); post-fix it is accepted. +func TestMarketplaceList_TermYearsConvertedToMonths(t *testing.T) { + t.Run("default schedule price reflects true remaining value", func(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() + row.Term = 3 // 3-year RI (stored in years) + row.Timestamp = time.Now().Add(-6 * 30 * 24 * time.Hour) // purchased ~6 months ago + row.UpfrontCost = 3600 + row.Count = 1 + + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", config.ListingStateActive). + Return(nil) + + ec2 := &stubMarketplaceEC2{} + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + resp, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.NoError(t, err) + typed, ok := resp.(*MarketplaceListResponse) + require.True(t, ok) + + require.Len(t, typed.PriceSchedule, 1) + // remainingMonths must be ~30 (36 total - 6 elapsed), not 1 (the pre-fix value + // produced by treating 3 years as 3 months and flooring the negative result). + assert.InDelta(t, 30, int(typed.PriceSchedule[0].TermMonths), 2, + "schedule term_months should reflect real remaining months (~30), not the pre-fix value of 1") + // Default price: residual = 3600*(30/36)=3000, discounted by 0.95 = ~2850. + // Pre-fix price: residual = 3600*(1/3)=1200, price = 1200*0.95 = 1140. + assert.InDelta(t, 2850.0, typed.PriceSchedule[0].Price, 150, + "default price should be ~$2,850 (30/36 of $3,600 * 0.95), not ~$1,140 (the pre-fix value)") + assert.Greater(t, typed.PriceSchedule[0].Price, 2000.0, + "default price must be well above the pre-fix ~$1,140 value") + + // The AWS PriceScheduleSpecification.Term must reflect the real remaining months. + require.Len(t, ec2.lastCreateReq.PriceSchedule, 1) + assert.InDelta(t, 30, int(ec2.lastCreateReq.PriceSchedule[0].Term), 2, + "AWS PriceScheduleSpecification.Term must be real remaining months (~30), not years") + + cfgStore.AssertExpectations(t) + }) + + t.Run("supplied schedule with real remaining term accepted post-fix", func(t *testing.T) { + // Pre-fix: remainingMonths=1 (years=3 misread as months), so tier term=30 > 1 + // triggered "term_months exceeds the RI's remaining term". Post-fix: + // remainingMonths=30 and term=36, so 30 <= 30 is valid and $2,500 clears the + // 5% floor (~$150 on a $3,000 prorated residual). + schedule, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 30, Price: 2500}, + }, 30, 36, 1, 3600) + require.NoError(t, err, + "a {TermMonths:30, Price:2500} schedule must be accepted when remainingMonths=30 (post-fix)") + require.Len(t, schedule, 1) + assert.Equal(t, int64(30), schedule[0].TermMonths) + assert.InDelta(t, 2500.0, schedule[0].Price, 0.001) + }) + + t.Run("pre-fix: supplied schedule with term 30 rejected when remaining is 1", func(t *testing.T) { + // This sub-test documents the pre-fix breakage: when remainingMonths was + // computed as 1 (3 years misread as 3 months, then 3-6=-3, floored to 1), + // a legitimate 30-month schedule was rejected. This test FAILS on pre-fix + // code and PASSES after the fix (because post-fix remainingMonths=30 makes + // a 30-month tier valid, so the call below -- which directly exercises the + // old broken inputs -- must still reject it). + _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 30, Price: 2500}, + }, 1, 3, 1, 3600) + require.Error(t, err, + "pre-fix inputs (remainingMonths=1, originalTerm=3 months) must reject a 30-month tier") + assert.Contains(t, err.Error(), "exceeds the RI's remaining term") + }) +} + +// TestMarketplaceList_InvalidTermReturnsError verifies the no-silent-fallbacks +// rule: a purchase_history row with term=0 (invalid -- valid years are 1 or 3) +// must produce an explicit error, not a garbage price computed from 0*12=0 months. +func TestMarketplaceList_InvalidTermReturnsError(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() + row.Term = 0 // invalid: purchase_history.term must be 1 or 3 (years) + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + + h := newMarketplaceHandler(cfgStore, authSvc, &stubMarketplaceEC2{}) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid term") + cfgStore.AssertExpectations(t) +} From 58c704598fe590b6c6c420332585e7c9519599e5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:01:39 +0300 Subject: [PATCH 2/2] fix(frontend/marketplace): convert RI term years->months in Sell gate and consent modal (follow-up to #808) Mirror of the backend years-as-months fix on the frontend. purchase_history.term is stored in years (1 or 3), but frontend/src/history.ts consumed it as months in two places on the Sell-on-Marketplace path: - canSellOnMarketplace (~line 663): computed remainingMonths = term - elapsedMonths with term in years, so a 3-year RI was treated as 3 months and the Sell button vanished after ~3 months of elapsed time. - Consent/pricing modal (~line 1210): computed the residual with term in years, so the resale price summary shown to the user was ~1/3 of the real value (e.g. a 3yr RI 6 months in showed ~$0/underpriced instead of ~$2,850 on $3,600 upfront). Fix: convert term years->months at the boundary (termYears * 12) before computing remainingMonths/residual in both spots, and guard term <= 0. Correct the existing makeRow() test fixture from term:36 (36 years, nonsensical, masked the bug) to term:3. Add a modal-residual regression test asserting a 3yr RI ~6 months in shows ~30 months remaining and a ~$2,850 list price; it fails pre-fix (the gate hides the button) and passes post-fix. Parallels the backend TestMarketplaceList_TermYearsConvertedToMonths. --- .../history-marketplace-sell-button.test.ts | 68 ++++++++++++++++++- frontend/src/history.ts | 24 +++++-- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index 9da8ae1b1..cc878cc17 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -69,6 +69,7 @@ jest.mock('../state', () => ({ import * as api from '../api'; import { getCurrentUser } from '../state'; +import { confirmDialog } from '../confirmDialog'; // Administrators group GUID -- mirrors ADMINISTRATORS_GROUP_ID in // frontend/src/permissions.ts. Without this, isAdmin() returns false and @@ -125,7 +126,9 @@ function makeRow(overrides: Record) { resource_type: 't3.large', region: 'us-east-1', count: 1, - term: 36, + // term is stored in YEARS (1 or 3) in purchase_history; canSellOnMarketplace + // and the pricing modal multiply by 12 to get months. + term: 3, upfront_cost: 1200, estimated_savings: 300, plan_name: '', @@ -232,3 +235,66 @@ describe('History inline Sell on Marketplace button (issue #292)', () => { expect(sellIds()).toEqual([]); }); }); + +// Regression for the #808 follow-up years-as-months bug in the consent modal. +// purchase_history.term is stored in YEARS; the Sell gate and the pricing modal +// must multiply by 12 before computing remaining term / residual. Parallels the +// backend TestMarketplaceList_TermYearsConvertedToMonths. +describe('Marketplace consent modal residual proration (issue #808 follow-up)', () => { + // ~6 months ago (6 * 30.4375 days) so a 3-year (36-month) RI has ~30 months left. + const SIX_MONTHS_AGO = new Date(Date.now() - 6 * 30.4375 * 24 * 60 * 60 * 1000).toISOString(); + + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + // Resolve false so the click handler stops after showing the pricing modal + // (we only assert on the modal body, not the createMarketplaceListing call). + (confirmDialog as jest.Mock).mockResolvedValue(false); + }); + + test('shows the correctly-prorated residual for a 3yr RI ~6 months in', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'ri-3yr', + offering_class: 'standard', + term: 3, // 3 years + timestamp: SIX_MONTHS_AGO, + upfront_cost: 3600, + monthly_cost: 0, + }), + ], + }); + + await loadHistory(); + + // The gate itself only passes after the years->months fix: pre-fix a 3-year + // RI was treated as 3 months, so 3 - 6 elapsed < 1 hid the button entirely. + const sellBtn = document + .getElementById('history-list')! + .querySelector('.history-marketplace-sell-btn[data-marketplace-sell-id="ri-3yr"]'); + expect(sellBtn).not.toBeNull(); + + sellBtn!.click(); + // Flush the async click handler up to the awaited confirmDialog call. + await Promise.resolve(); + await Promise.resolve(); + + expect(confirmDialog as jest.Mock).toHaveBeenCalledTimes(1); + const arg = (confirmDialog as jest.Mock).mock.calls[0][0] as { body: HTMLElement }; + const text = arg.body.textContent || ''; + + // termMonths = 3 * 12 = 36; remaining = round(36 - 6) = 30. + expect(text).toContain('30 months'); + // Residual = 3600 * (30/36) = 3000; list price = 3000 * 0.95 = 2850. + // formatCurrency is mocked as `$${val || 0}`, so 2850 -> "$2850". + expect(text).toContain('Default list price'); + expect(text).toContain('$2850'); + // Pre-fix (had the button rendered) remaining=0 => residual=0 => "$0", + // and pre-gate-fix the button would not render at all. Guard against a + // regression back to the $0 / ~1/3 value. + expect(text).not.toContain('$0'); + }); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 0e68ae0b1..10e9fad83 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -658,10 +658,14 @@ function canSellOnMarketplace(p: HistoryPurchase): boolean { return false; } // Guard against listing a matured RI: compute remaining months from the - // purchase timestamp and the total term. term is in months; timestamp is - // the purchase date. We require at least 1 full month remaining. - const termMonths = typeof p.term === 'number' ? p.term : Number(p.term) || 0; - if (termMonths <= 0) return false; + // purchase timestamp and the total term. purchase_history.term is stored in + // YEARS (1 or 3), so convert to months before comparing against elapsed + // months (mirrors the row.Term * 12 conversion in handler_marketplace.go). + // Without the conversion a 3-year RI was treated as 3 months and the Sell + // button vanished after ~3 months. We require at least 1 full month remaining. + const termYears = typeof p.term === 'number' ? p.term : Number(p.term) || 0; + if (termYears <= 0) return false; + const termMonths = termYears * 12; const purchaseMs = new Date(p.timestamp).getTime(); if (!Number.isFinite(purchaseMs)) return false; const elapsedMonths = (Date.now() - purchaseMs) / (1000 * 60 * 60 * 24 * 30.4375); @@ -1207,7 +1211,13 @@ function wireRowActionHandlers(container: HTMLElement): void { bodyEl.className = 'marketplace-pricing-modal-body'; if (purchase) { - const termMonths = typeof purchase.term === 'number' ? purchase.term : Number(purchase.term) || 0; + // purchase_history.term is stored in YEARS (1 or 3); convert to months + // before computing the remaining term and residual so the price summary + // shown to the user reflects real remaining value rather than ~1/3 of it + // (a 3-year RI was previously treated as 3 months). Mirrors the + // row.Term * 12 conversion in internal/api/handler_marketplace.go. + const termYears = typeof purchase.term === 'number' ? purchase.term : Number(purchase.term) || 0; + const termMonths = termYears > 0 ? termYears * 12 : 0; const purchaseMs = new Date(purchase.timestamp).getTime(); const elapsedMonths = Number.isFinite(purchaseMs) ? (Date.now() - purchaseMs) / (1000 * 60 * 60 * 24 * 30.4375) @@ -1217,8 +1227,8 @@ function wireRowActionHandlers(container: HTMLElement): void { 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 12-month RI at month 6 has only half its - // upfront value left). Mirrors resolveMarketplacePriceSchedule in + // 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;