Skip to content

fix(api/history): exclude cancelled executions from KPI aggregation (closes #736) - #737

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/736-cancel-kpi-exclusion
May 27, 2026
Merged

fix(api/history): exclude cancelled executions from KPI aggregation (closes #736)#737
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/736-cancel-kpi-exclusion

Conversation

@cristim

@cristim cristim commented May 26, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #736 (P1). The summarizePurchaseHistory aggregator in internal/api/handler_history.go had a switch p.Status covering pending/notified/approved/running/paused/failed/expired but no case for cancelled — rows fell through to the default branch which incremented TotalCompleted, TotalUpfront, TotalMonthlySavings, and (via *12) TotalAnnualSavings. Result: cancelling a pending purchase added its costs and savings to the KPIs as if it had completed.

Cancel mutation was correct (transitions to status='cancelled'); fix is solely in the aggregation switch.

analytics_postgres.go's HistorySummary path is unaffected — it queries purchase_history which only contains rows written after successful purchase, so cancelled executions never reach it.

Test plan

  • Regression tests TestSummarizePurchaseHistory_CancelledExcludedFromKPIs and ..._CancelPendingDoesNotChangeKPIs pass
  • Full internal/api suite: 1233 pass, 0 fail
  • go build ./... clean
  • Manual: cancel a pending purchase in the UI; KPIs stay unchanged

Source of finding: QA verification spreadsheet row 278 (Approval queue step 1.6).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where cancelled purchases were incorrectly included in financial totals and completion metrics. Cancelled purchases are now properly excluded from all dollar KPIs.
  • Documentation

    • Clarified that dollar totals exclude non-completed purchase states.

Review Change Stack

summarizePurchaseHistory had no case for the "cancelled" status in its
switch, so cancelled rows fell through to the default path and added
their UpfrontCost and EstimatedSavings to TotalUpfront,
TotalMonthlySavings, and TotalAnnualSavings -- as if the purchase had
been approved. A cancelled purchase represents zero committed spend and
zero realized savings, so it must be excluded from all three aggregates
(and from TotalCompleted).

Fix: add `case "cancelled": continue` to the switch, matching the
existing pattern for pending/failed/expired rows. Update the comment on
HistorySummary.TotalPurchases and the inline comment in the loop to
document the cancelled exclusion.

Regression tests (issue #736):
- TestSummarizePurchaseHistory_CancelledExcludedFromKPIs: N completed
  rows + 1 pending + 2 pre-existing cancelled rows; asserts only the
  completed rows contribute to the dollar totals.
- TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs: mirrors
  the QA reproduction scenario -- baseline KPI totals captured, then
  a cancelled row added; asserts all three KPIs and TotalCompleted are
  unchanged.
@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 labels May 26, 2026
@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1d9ed194-b962-402c-815c-104480a5a680

📥 Commits

Reviewing files that changed from the base of the PR and between 6fcf6a6 and caf07f7.

📒 Files selected for processing (3)
  • internal/api/handler_history.go
  • internal/api/handler_history_test.go
  • internal/api/types.go

📝 Walkthrough

Walkthrough

Updated summarizePurchaseHistory to exclude cancelled purchases from dollar KPI totals and completed counts. Added inline and type documentation to clarify which non-completed states are excluded from aggregation. Two new unit tests validate the exclusion behavior and confirm that cancelling a pending purchase does not alter existing KPI sums.

Changes

Cancelled Purchase Exclusion from KPI Totals

Layer / File(s) Summary
KPI type documentation
internal/api/types.go
HistorySummary documentation clarified to explicitly list all non-completed states (pending, in-progress, failed, expired, cancelled) that are excluded from dollar totals.
Cancelled status exclusion in KPI aggregation
internal/api/handler_history.go
summarizePurchaseHistory adds a case "cancelled" branch to skip cancelled purchases from all dollar KPIs and TotalCompleted, with expanded inline documentation listing all excluded non-completed states.
Cancelled status KPI exclusion tests
internal/api/handler_history_test.go
Two unit tests validate that cancelled rows are excluded from dollar totals and completed count, and that cancelling a pending purchase leaves existing KPI totals unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • LeanerCloud/CUDly#736: This PR directly implements the fix for cancelled purchases incorrectly contributing to dollar KPIs by excluding them from TotalUpfront, TotalMonthlySavings, TotalAnnualSavings, and TotalCompleted.

Possibly related PRs

  • LeanerCloud/CUDly#623: Both PRs modify internal/api/handler_history.go's summarizePurchaseHistory to change which purchase execution statuses are excluded from KPI and total calculations.

Suggested labels

effort/s, type/bug

Poem

🐰 A cancelled dream now rests at rest,
No longer swells the savings quest—
The ledger now counts true and fair,
While pending counts show what's still there,
A rabbit's fix, both swift and right!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements the core fix required by #736: a case for cancelled status in the switch statement excluding cancelled purchases from dollar KPIs and TotalCompleted. However, only API unit tests were added; UI tests and API integration tests specified in #736 requirements are missing. Add API integration test and UI test covering the cancel flow scenario (create approved purchases + 1 pending, cancel pending, assert KPIs unchanged) as required by issue #736.
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the primary change: excluding cancelled executions from KPI aggregation in the purchase history handler.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing #736: the handler logic excludes cancelled status, tests validate the fix, and documentation is updated to reflect the behaviour.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/736-cancel-kpi-exclusion

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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

@cristim

cristim commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

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 commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

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 commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

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 commented May 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

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 4791743 into feat/multicloud-web-frontend May 27, 2026
4 checks passed
@cristim
cristim deleted the fix/736-cancel-kpi-exclusion branch May 27, 2026 08:57
cristim added a commit that referenced this pull request Jun 8, 2026
summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625
cristim added a commit that referenced this pull request Jun 19, 2026
summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625
cristim added a commit that referenced this pull request Jul 17, 2026
summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625
cristim added a commit that referenced this pull request Jul 17, 2026
summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625
cristim added a commit that referenced this pull request Jul 17, 2026
summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625
cristim added a commit that referenced this pull request Jul 17, 2026
…loses #625) (#821)

* fix(purchases): cite #625 in cancelled-KPI regression tests

summarizePurchaseHistory already excludes cancelled rows from dollar
totals (landed in #737 against #736). Issue #625 describes the same
bug -- this commit updates the two existing regression-test comments
and assertion messages to cite both issues so the PR can formally
close #625.

Closes #625

* test(purchases): fix misspell/prealloc/govet in handler_history_test

- 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

* fix(lint): fix govet/misspell nolints in handler_history_test

- 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant