Skip to content

Commit 1ae5175

Browse files
cristimcristim
committed
fix(marketplace): correct RI listing price-floor math and reachability
Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
1 parent 2dc0735 commit 1ae5175

12 files changed

Lines changed: 430 additions & 100 deletions

‎frontend/src/__tests__/history-marketplace-sell-button.test.ts‎

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
/**
2+
* History inline "Sell on Marketplace" button tests (issue #292, FINDING D).
3+
*
4+
* Regression guard for the "dead Sell button" gap: canSellOnMarketplace used to
5+
* require offering_class === 'standard', but nothing ever wrote a 'standard'
6+
* row end-to-end -- CUDly stamps 'convertible' on its own EC2 purchases, and
7+
* externally-created Standard RIs arrive with an EMPTY offering_class until the
8+
* backend lazily populates it from AWS on the list call. So the button never
9+
* rendered for the very case the feature targets.
10+
*
11+
* The fix shows the Sell button for completed AWS EC2 rows whose offering_class
12+
* is 'standard' OR still unknown (empty), and lets the backend
13+
* populateOfferingClass + gate (it 400s a fetched 'convertible') decide. The
14+
* backend authorizeSessionSell remains the real security boundary; these tests
15+
* only verify the UX gate.
16+
*
17+
* Tested matrix:
18+
* 1. completed aws/ec2 row, offering_class 'standard' -> shown.
19+
* 2. completed aws/ec2 row, offering_class EMPTY (unknown) -> shown (fix).
20+
* 3. completed aws/ec2 row, offering_class 'convertible' -> hidden.
21+
* 4. completed aws/ec2 row with an active listing -> hidden.
22+
* 5. completed aws/rds row, offering_class EMPTY -> hidden.
23+
* 6. completed azure/compute row, offering_class EMPTY -> hidden.
24+
* 7. anonymous (no current user cached) -> hidden.
25+
*/
26+
27+
import { loadHistory } from '../history';
28+
29+
jest.mock('../api', () => ({
30+
getHistory: jest.fn(),
31+
createMarketplaceListing: jest.fn(),
32+
cancelMarketplaceListing: jest.fn(),
33+
}));
34+
35+
jest.mock('../navigation', () => ({
36+
switchTab: jest.fn(),
37+
}));
38+
39+
jest.mock('../utils', () => ({
40+
formatCurrency: jest.fn((val) => `$${val || 0}`),
41+
formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')),
42+
formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)),
43+
escapeHtml: jest.fn((str) => str || ''),
44+
escapeHtmlAttr: jest.fn((str) => str || ''),
45+
amortizedMonthly: jest.fn((monthly) => monthly),
46+
populateAccountFilter: jest.fn(() => Promise.resolve()),
47+
}));
48+
49+
jest.mock('../confirmDialog', () => ({
50+
confirmDialog: jest.fn(),
51+
}));
52+
53+
jest.mock('../toast', () => ({
54+
showToast: jest.fn(),
55+
}));
56+
57+
jest.mock('../state', () => ({
58+
getCurrentUser: jest.fn(),
59+
getCurrentProvider: jest.fn().mockReturnValue(''),
60+
setCurrentProvider: jest.fn(),
61+
getCurrentAccountIDs: jest.fn().mockReturnValue([]),
62+
setCurrentAccountIDs: jest.fn(),
63+
subscribeProvider: jest.fn().mockReturnValue(() => {}),
64+
subscribeAccount: jest.fn().mockReturnValue(() => {}),
65+
getAmortizeUpfront: jest.fn().mockReturnValue(false),
66+
setAmortizeUpfront: jest.fn(),
67+
subscribeAmortizeUpfront: jest.fn().mockReturnValue(() => {}),
68+
}));
69+
70+
import * as api from '../api';
71+
import { getCurrentUser } from '../state';
72+
73+
// Administrators group GUID -- mirrors ADMINISTRATORS_GROUP_ID in
74+
// frontend/src/permissions.ts. Without this, isAdmin() returns false and
75+
// canAccess('admin', '*') in canSellOnMarketplace's fallback branch does not
76+
// grant. Seeded-group GUID, not the label string.
77+
const ADMIN_GROUP_ID = '00000000-0000-5000-8000-000000000001';
78+
const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMIN_GROUP_ID] };
79+
80+
// A recent purchase with plenty of term left so the remaining-months guard in
81+
// canSellOnMarketplace (needs >= 1 month remaining) always passes.
82+
const RECENT = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000).toISOString(); // -30 days
83+
84+
function setupDOM(): void {
85+
while (document.body.firstChild) document.body.removeChild(document.body.firstChild);
86+
87+
const mkInput = (id: string): HTMLInputElement => {
88+
const el = document.createElement('input');
89+
el.type = 'date';
90+
el.id = id;
91+
return el;
92+
};
93+
const mkSelect = (id: string): HTMLSelectElement => {
94+
const el = document.createElement('select');
95+
el.id = id;
96+
const opt = document.createElement('option');
97+
opt.value = '';
98+
opt.textContent = 'All';
99+
el.appendChild(opt);
100+
return el;
101+
};
102+
const mkDiv = (id: string): HTMLDivElement => {
103+
const el = document.createElement('div');
104+
el.id = id;
105+
return el;
106+
};
107+
108+
document.body.appendChild(mkInput('history-start'));
109+
document.body.appendChild(mkInput('history-end'));
110+
document.body.appendChild(mkSelect('history-provider-filter'));
111+
document.body.appendChild(mkSelect('history-account-filter'));
112+
document.body.appendChild(mkDiv('history-summary'));
113+
document.body.appendChild(mkDiv('history-list'));
114+
document.body.appendChild(mkDiv('purchases-approval-queue'));
115+
}
116+
117+
// A completed AWS EC2 RI row with a long remaining term. Overrides tune the
118+
// offering_class / provider / service / listing_state for each case.
119+
function makeRow(overrides: Record<string, unknown>) {
120+
return {
121+
purchase_id: 'ri-1',
122+
timestamp: RECENT,
123+
provider: 'aws',
124+
service: 'ec2',
125+
resource_type: 't3.large',
126+
region: 'us-east-1',
127+
count: 1,
128+
term: 36,
129+
upfront_cost: 1200,
130+
estimated_savings: 300,
131+
plan_name: '',
132+
status: 'completed',
133+
offering_class: 'standard',
134+
...overrides,
135+
};
136+
}
137+
138+
function sellIds(): (string | undefined)[] {
139+
const list = document.getElementById('history-list')!;
140+
const buttons = list.querySelectorAll<HTMLButtonElement>('.history-marketplace-sell-btn');
141+
return Array.from(buttons).map((b) => b.dataset['marketplaceSellId']);
142+
}
143+
144+
describe('History inline Sell on Marketplace button (issue #292)', () => {
145+
beforeEach(() => {
146+
setupDOM();
147+
jest.clearAllMocks();
148+
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
149+
});
150+
151+
test('shows Sell for a completed AWS EC2 row with offering_class=standard', async () => {
152+
(api.getHistory as jest.Mock).mockResolvedValue({
153+
summary: {},
154+
purchases: [makeRow({ purchase_id: 'ri-standard', offering_class: 'standard' })],
155+
});
156+
157+
await loadHistory();
158+
159+
expect(sellIds()).toEqual(['ri-standard']);
160+
});
161+
162+
test('shows Sell for a completed AWS EC2 row with EMPTY offering_class (the reachability fix)', async () => {
163+
(api.getHistory as jest.Mock).mockResolvedValue({
164+
summary: {},
165+
purchases: [makeRow({ purchase_id: 'ri-unknown', offering_class: '' })],
166+
});
167+
168+
await loadHistory();
169+
170+
// The backend lazily populates + gates; the UI must surface the button so
171+
// an externally-created Standard RI is actually listable end-to-end.
172+
expect(sellIds()).toEqual(['ri-unknown']);
173+
});
174+
175+
test('hides Sell for a known convertible RI', async () => {
176+
(api.getHistory as jest.Mock).mockResolvedValue({
177+
summary: {},
178+
purchases: [makeRow({ purchase_id: 'ri-convertible', offering_class: 'convertible' })],
179+
});
180+
181+
await loadHistory();
182+
183+
expect(sellIds()).toEqual([]);
184+
});
185+
186+
test('hides Sell when a listing is already active (Cancel is shown instead)', async () => {
187+
(api.getHistory as jest.Mock).mockResolvedValue({
188+
summary: {},
189+
purchases: [
190+
makeRow({ purchase_id: 'ri-active', offering_class: 'standard', listing_state: 'active', listing_id: 'ril-1' }),
191+
],
192+
});
193+
194+
await loadHistory();
195+
196+
expect(sellIds()).toEqual([]);
197+
});
198+
199+
test('hides Sell for a non-EC2 AWS row even when offering_class is unknown', async () => {
200+
(api.getHistory as jest.Mock).mockResolvedValue({
201+
summary: {},
202+
purchases: [makeRow({ purchase_id: 'rds-unknown', service: 'rds', offering_class: '' })],
203+
});
204+
205+
await loadHistory();
206+
207+
expect(sellIds()).toEqual([]);
208+
});
209+
210+
test('hides Sell for a non-AWS row even when offering_class is unknown', async () => {
211+
(api.getHistory as jest.Mock).mockResolvedValue({
212+
summary: {},
213+
purchases: [
214+
makeRow({ purchase_id: 'az-unknown', provider: 'azure', service: 'compute', offering_class: '' }),
215+
],
216+
});
217+
218+
await loadHistory();
219+
220+
expect(sellIds()).toEqual([]);
221+
});
222+
223+
test('hides Sell when no user is cached (anonymous)', async () => {
224+
(getCurrentUser as jest.Mock).mockReturnValue(null);
225+
(api.getHistory as jest.Mock).mockResolvedValue({
226+
summary: {},
227+
purchases: [makeRow({ purchase_id: 'ri-anon', offering_class: '' })],
228+
});
229+
230+
await loadHistory();
231+
232+
expect(sellIds()).toEqual([]);
233+
});
234+
});

‎frontend/src/history.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,14 @@ function retryThresholdReached(p: HistoryPurchase): boolean {
609609
//
610610
// Conditions:
611611
// * row must be a completed purchase (status "completed" or absent);
612-
// * offering_class must be "standard";
612+
// * row must be an AWS EC2 RI (marketplace is EC2-only);
613+
// * offering_class must be "standard" OR still unknown (empty). CUDly stamps
614+
// "convertible" on its own EC2 purchases, but externally-created Standard
615+
// RIs (and pre-migration rows) have an empty offering_class until the
616+
// backend lazily populates it from AWS on the list call. The backend
617+
// definitively gates -- it 400s a fetched "convertible" -- so we show the
618+
// button for unknown-class EC2 rows and let the backend decide, otherwise
619+
// the feature is unreachable end-to-end for the very case it targets;
613620
// * no active listing already (listing_state != "active"); and
614621
// * admin, or non-admin user (sell-own covers their own accounts --
615622
// we can't efficiently check per-account ownership client-side, so
@@ -618,7 +625,12 @@ function retryThresholdReached(p: HistoryPurchase): boolean {
618625
function canSellOnMarketplace(p: HistoryPurchase): boolean {
619626
const status = normalizeStatus(p).toLowerCase();
620627
if (status !== 'completed') return false;
621-
if ((p.offering_class || '').toLowerCase() !== 'standard') return false;
628+
// Marketplace listing is AWS EC2 Standard-RI only. Gate on provider/service
629+
// so unknown-class rows for non-EC2 providers never show the Sell button.
630+
if ((p.provider || '').toLowerCase() !== 'aws') return false;
631+
if ((p.service || '').toLowerCase() !== 'ec2') return false;
632+
const offeringClass = (p.offering_class || '').toLowerCase();
633+
if (offeringClass !== 'standard' && offeringClass !== '') return false;
622634
if ((p.listing_state || '').toLowerCase() === 'active') return false;
623635
const user = getCurrentUser();
624636
if (!user) return false;

0 commit comments

Comments
 (0)