Repository navigation
fix(purchases): add 'paused' to valid_status CHECK constraint so Pause succeeds - #779
Merged
cristim merged 2 commits intoMay 28, 2026
Merged
Conversation
…to valid_status The Pause button surfaced the raw Postgres CHECK constraint error to the user. Added 'paused' to the valid_status CHECK constraint via a new migration (000055), and added a regression test that verifies ineligible-status transitions return a clean 409. Closes #772.
cristim
added
triaged
Contributor
|
Warning Review limit reached
More reviews will be available in 2 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds support for pausing scheduled purchase executions by introducing the ChangesPaused Status Support
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/api/handler_purchases_test.go (1)
988-989: ⚡ Quick winAssert
mockAuthexpectations to avoid silent test false-positives.
ValidateSessionis expected but never asserted. If auth is bypassed in a regression, this test can still pass.Proposed patch
- t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + })Also applies to: 996-997
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases_test.go` around lines 988 - 989, Test is missing assertions for mockAuth leading to silent false-positives; add expectations assertion for mockAuth after test execution. Specifically, after the existing t.Cleanup call that asserts mockStore (and at the other location around lines 996-997), call mockAuth.AssertExpectations(t) (or the equivalent mockAuth.AssertExpectations in those tests) to ensure the expected ValidateSession call was actually invoked; ensure you place the assertion in each test that sets up mockAuth so ValidateSession failures are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 988-989: Test is missing assertions for mockAuth leading to silent
false-positives; add expectations assertion for mockAuth after test execution.
Specifically, after the existing t.Cleanup call that asserts mockStore (and at
the other location around lines 996-997), call mockAuth.AssertExpectations(t)
(or the equivalent mockAuth.AssertExpectations in those tests) to ensure the
expected ValidateSession call was actually invoked; ensure you place the
assertion in each test that sets up mockAuth so ValidateSession failures are
caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22a6af2b-3407-4138-9972-ebd0850a89a4
📒 Files selected for processing (3)
internal/api/handler_purchases_test.gointernal/database/postgres/migrations/000055_add_paused_status.down.sqlinternal/database/postgres/migrations/000055_add_paused_status.up.sql
…pause regression CR nitpick: `ValidateSession` was expected on `mockAuth` but never asserted in the t.Cleanup block, so a regression that bypasses auth could leave this test falsely green. Add `mockAuth.AssertExpectations(t)` alongside the existing `mockStore.AssertExpectations(t)` so both surfaces are verified. Matches the project-wide "always assert mock expectations" rule.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 28, 2026
Closed
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
…Resume) After #779/#772 added the paused DB status, clicking Pause made the scheduled purchase silently disappear from the list with no feedback. The planned-purchases list read GetPendingExecutions, whose status set (pending, notified) the scheduler relies on to decide what to FIRE and which excludes paused, so paused rows dropped out. Switch the list handler to GetExecutionsByStatuses with [pending, notified, paused] so paused executions stay listed (with the existing Paused badge and Resume button) while the scheduler keeps using the narrower GetPendingExecutions and never fires paused rows. Restore the soonest-first ordering. Add success toasts on Pause and Resume. Pause stays distinct from Disable plan (#774, whole plan) and Cancel (terminal): it is reversible and scoped to a single execution; the plan stays enabled. Resume (paused -> pending) and the 409 on ineligible transitions already existed from #772. Tests: backend asserts the paused status set is requested, paused rows stay returned, and ordering is ascending; frontend asserts the paused row renders visibly with a badge and that Pause/Resume fire success toasts.
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
…Resume) (#904) * fix(planned-purchases): Pause is visible + reversible (badge, toast, Resume) After #779/#772 added the paused DB status, clicking Pause made the scheduled purchase silently disappear from the list with no feedback. The planned-purchases list read GetPendingExecutions, whose status set (pending, notified) the scheduler relies on to decide what to FIRE and which excludes paused, so paused rows dropped out. Switch the list handler to GetExecutionsByStatuses with [pending, notified, paused] so paused executions stay listed (with the existing Paused badge and Resume button) while the scheduler keeps using the narrower GetPendingExecutions and never fires paused rows. Restore the soonest-first ordering. Add success toasts on Pause and Resume. Pause stays distinct from Disable plan (#774, whole plan) and Cancel (terminal): it is reversible and scoped to a single execution; the plan stays enabled. Resume (paused -> pending) and the 409 on ineligible transitions already existed from #772. Tests: backend asserts the paused status set is requested, paused rows stay returned, and ordering is ascending; frontend asserts the paused row renders visibly with a badge and that Pause/Resume fire success toasts. * feat(config/store): add GetPlannedExecutions with ASC ordering GetExecutionsByStatuses uses ORDER BY scheduled_date DESC + LIMIT, which is correct for History (newest-first) but truncates the soonest rows when used for the Planned Purchases list and total pending/notified/paused rows exceed MaxListLimit. An in-memory ASC re-sort of the already-truncated subset cannot recover what LIMIT dropped at the DB. Add a dedicated GetPlannedExecutions method that mirrors the GetExecutionsByStatuses shape (same status filter, same limit clamping, same scan logic via queryExecutions) but flips the ORDER BY to scheduled_date ASC NULLS LAST, id ASC. The secondary id ASC sort keeps the ordering stable when multiple rows share a scheduled_date; NULLS LAST is defensive against a future schema relaxation (today scheduled_date is NOT NULL). GetExecutionsByStatuses is left untouched so its other callers (History queries that want newest-first) keep their semantics. pgxmock regression coverage anchors the SQL contract: - TestPGXMock_GetPlannedExecutions_UsesASCOrdering asserts the query has ASC + NULLS LAST + id ASC + LIMIT $2 via a strict regex matcher, so a regression to DESC or a dropped secondary sort fails the test. - TestPGXMock_GetPlannedExecutions_EmptyStatuses guards the short-circuit. - TestPGXMock_GetPlannedExecutions_LimitClamping covers the negative -> DefaultListLimit and over-MaxListLimit -> MaxListLimit clamps. All store mocks (api, purchase, scheduler, analytics, server health) gain a GetPlannedExecutions method so the StoreInterface contract is satisfied across the codebase. Refs CR on #904. * fix(api/purchases): use GetPlannedExecutions to avoid truncating soonest rows Handler.getPlannedPurchases was calling GetExecutionsByStatuses (ORDER BY scheduled_date DESC + LIMIT $2) and then re-sorting ASC in-memory. When the planned set (pending + notified + paused) exceeds MaxListLimit, the DB returns only the LATEST rows; the in-memory sort just re-orders that already-truncated subset. The soonest rows, exactly the ones the user has to act on, can be omitted from the response entirely. Switch the handler to the new GetPlannedExecutions store method (ASC at the SQL level so LIMIT keeps the soonest rows), and drop the post-fetch sort.SliceStable since the DB now returns rows in the correct order. The "sort" import stays (still used for tuple normalisation elsewhere in the file). End-to-end regression coverage: - TestHandler_getPlannedPurchases_SoonestRowsNotTruncated seeds 5 rows in ASC order, asserts all 5 reach the response in ASC order, and uses AssertNotCalled to guard against any future refactor re-introducing a parallel GetExecutionsByStatuses call on this code path. - TestHandler_getPlannedPurchases_PausedStaysVisible updated to reflect the new ASC-from-store contract (was returning DESC then re-sorting). Closes CR finding on #904.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
P1 fix for QA Planned 6.1: clicking "Pause" on a scheduled purchase surfaced the raw Postgres CHECK-constraint violation to the user:
Root cause
The
valid_statusCHECK constraint onpurchase_executionswas missing the'paused'value.pausePlannedPurchasecallsTransitionExecutionStatus, which issuesUPDATE ... SET status = 'paused'. Postgres rejected it with SQLSTATE 23514.Fix
New migration
000055_add_paused_statusdrops and re-adds the constraint withpausedincluded (mirrors the pattern from000013_add_running_status).New
valid_statusset (post-migration):'pending', 'running', 'notified', 'approved', 'cancelled', 'completed', 'failed', 'paused'No handler logic changes were needed -- the handler already converts any
TransitionExecutionStatuserror into a 409 with a clear message. The constraint was the only gap.Files changed
internal/database/postgres/migrations/000055_add_paused_status.up.sqlinternal/database/postgres/migrations/000055_add_paused_status.down.sqlinternal/api/handler_purchases_test.go(regression test for ineligible-source-status 409)Test plan
TestHandler_pausePlannedPurchase_IneligibleStatusasserts 409 + "cannot be paused" message when source status isn't pause-eligible.golang-migrate up; click Pause on a scheduled purchase, confirm toast says paused and row's status flips.Closes #772.
Summary by CodeRabbit
Tests
Chores