fix(purchases): exclude cancelled rows from summarizePurchaseHistory (closes #625)#821
fix(purchases): exclude cancelled rows from summarizePurchaseHistory (closes #625)#821cristim wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTest-only changes in ChangesHistory handler test updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
Rate Limit Exceeded
|
|
@coderabbitai full review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and diff for PR `#821`
gh pr view 821 --repo LeanerCloud/CUDly --json title,body,files,state,baseRefName,headRefName
echo "---DIFF---"
gh pr diff 821 --repo LeanerCloud/CUDlyLength of output: 5116
Review of PR
|
| Location | Before | After | Verdict |
|---|---|---|---|
TestSummarizePurchaseHistory_CancelledExcludedFromKPIs doc comment |
regression test for issue #736`` |
regression test for issues #625and#736`` |
✅ Accurate |
TestSummarizePurchaseHistory_CancelledExcludedFromKPIs — 3 assert.InDelta messages |
(issue #736) |
(issues #625, #736) |
✅ Accurate |
TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs doc comment |
QA reproduction scenario from issue #736`` |
QA reproduction scenario from issues #625and#736`` |
✅ Accurate |
TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs — 4 assert.Equal messages |
(issue #736) |
(issues #625, #736) |
✅ Accurate |
Assessment
- No logic risk. The
"cancelled"guard inhandler_history.gois untouched and was already correct (landed in PR#737). - Audit trail is now complete. Adding
#625to the test comments closes the gap between the original issue and the implementing PR. - Consistent style. All seven affected strings are updated uniformly.
Nothing to flag. The PR does exactly what it says and nothing more.
[approve]
🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
- cancelled->canceled, Cancelling->Canceling, cancelling->canceling, synthesised->synthesized, honour->honor in comments/test messages; Status:"cancelled" DB enum values suppressed with //nolint:misspell - prealloc: preallocate baseline slice with cap 4 in TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs - govet fieldalignment: suppress anonymous test-table struct in TestHandler_getHistory_FilterValidation with //nolint:govet
|
@coderabbitai review |
✅ Action performedReview finished.
|
- Remove nolint:govet by reordering test-case struct fields to optimal alignment (map+string+string+int = 40 bytes, down from 48). - Annotate three nolint:misspell directives on "cancelled" with the DB-schema-value exception note referencing migration 000001. - Drop redundant misspell suppress from the append line (nolint:gocritic retained for the appendAssign check added in an earlier pass).
Summary
summarizePurchaseHistoryalready excludescancelledrows from dollar KPI totals via a dedicatedswitchcase (landed in PR fix(api/history): exclude cancelled executions from KPI aggregation (closes #736) #737, which cited bug(api/purchases): Total Upfront/Monthly/Annual Savings KPIs include cancelled purchases #736 but not the original duplicate bug(purchases): summarizePurchaseHistory counts cancelled rows into dollar totals #625).What the fix does (from #737)
summarizePurchaseHistoryininternal/api/handler_history.gonow has an explicit"cancelled"case thatcontinues past the dollar-accumulation lines, so cancelled rows contribute 0 toTotalUpfront,TotalMonthlySavings, andTotalAnnualSavings.Test plan
go test ./internal/api/... -run TestSummarizePurchaseHistorypasses (2 tests)go build ./...succeedsTestSummarizePurchaseHistory_CancelledExcludedFromKPIs: fixture with 3 active rows + 2 cancelled asserts totals sum only the active rowsTestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs: adding a cancelled row to a baseline set leaves all KPI totals unchangedSummary by CodeRabbit