Skip to content

fix(api/marketplace): convert RI term years->months in resale pricing (follow-up to #808) - #1447

Merged
cristim merged 2 commits into
mainfrom
fix/808-followup-marketplace-term-unit
Jul 17, 2026
Merged

fix(api/marketplace): convert RI term years->months in resale pricing (follow-up to #808)#1447
cristim merged 2 commits into
mainfrom
fix/808-followup-marketplace-term-unit

Conversation

@cristim

@cristim cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #808. The marketplaceList handler has a term-unit mismatch on a critical money path: 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"), but the handler was passing row.Term unchanged to two functions that both operate in months.

Defect 1 - computeRemainingMonths: parameter is named termMonths int. With row.Term=3 (years) and a purchase 6 months old, the function computed max(1, 3-6) = 1 instead of max(1, 36-6) = 30.

Defect 2 - resolveMarketplacePriceSchedule: the originalTerm parameter is documented as months. With originalTerm=3 (years), the per-unit residual was 3600 * (1/3) = $1,200 instead of 3600 * (30/36) = $3,000. Default listing price: ~$1,140 instead of ~$2,850.

Side effect: checkSuppliedScheduleFloor rejected any supplied schedule whose term_months > 1 (e.g. the correct {term_months: 30}) with "term_months exceeds the RI's remaining term".

Root cause

savePurchaseHistory stores rec.Term (years) directly into purchase_history.term. The marketplaceList handler consumed it as months at two call sites without converting. The existing test standardRow() used Term: 12 (12 years, nonsensical but accidentally masked the bug because computeRemainingMonths treated 12 as 12 months).

Fix

  • Guard: row.Term <= 0 returns an explicit error (no silent fallback per policy).
  • Boundary conversion: termMonths := row.Term * 12 at the entry point where years enter the pricing math.
  • Both computeRemainingMonths and resolveMarketplacePriceSchedule now receive termMonths.
  • standardRow() corrected to Term: 3 (realistic 3-year RI).

Tests added

  • TestComputeRemainingMonths: unit test confirming the helper treats its termMonths param as months.
  • TestMarketplaceList_TermYearsConvertedToMonths: end-to-end regression with Term:3, timestamp 6 months ago, UpfrontCost:3600. Asserts remaining ~30 months, default price ~$2,850, AWS schedule term ~30 months, and a supplied {term_months:30, price:2500} schedule is accepted. Also documents the pre-fix broken inputs.
  • TestMarketplaceList_InvalidTermReturnsError: verifies Term:0 returns an explicit error.

Money impact

For a 3yr RI sold 6 months in with $3,600 upfront:

Pre-fix Post-fix
remainingMonths 1 ~30
Default listing price ~$1,140 ~$2,850
Supplied {term_months:30} rejected accepted

Test plan

  • All existing marketplace tests pass with Term:3 in standardRow()
  • TestComputeRemainingMonths passes
  • TestMarketplaceList_TermYearsConvertedToMonths passes post-fix (fails pre-fix)
  • TestMarketplaceList_InvalidTermReturnsError passes
  • go vet clean, gofmt clean, gocyclo -over 10 clean, gosec clean

… (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.
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/m Days type/bug Defect labels Jul 17, 2026
@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 03477d90-ef48-4ac3-b523-38e0e0ade011

📥 Commits

Reviewing files that changed from the base of the PR and between 8f9347e and 58c7045.

📒 Files selected for processing (4)
  • frontend/src/__tests__/history-marketplace-sell-button.test.ts
  • frontend/src/history.ts
  • internal/api/handler_marketplace.go
  • internal/api/handler_marketplace_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/808-followup-marketplace-term-unit

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

… 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.
@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit dd569a0 into main Jul 17, 2026
20 checks passed
@cristim
cristim deleted the fix/808-followup-marketplace-term-unit branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant