From 84fd0edc7f1aa3c174d637633bb65b5150a690b1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 02:10:17 +0300 Subject: [PATCH] fix(plans): 404 missing plan and reconcile stale scheduled-purchase Edit (#1403) Editing a scheduled purchase whose plan no longer exists surfaced a generic "Failed to load plan details" and left the orphaned row on screen. The PR #780 fix (Edit passes plan_id, not the execution id) is intact and verified; the residual cause of the identical error is a scheduled-purchase row that outlived its plan: the plan was deleted (ON DELETE SET NULL detaches the execution) or the caller's account scope changed. Clicking Edit then loads GET /plans/{id}, which failed in two ways: - Backend: getPlan returned GetPurchasePlan's wrapped config.ErrNotFound raw. The router's IsNotFoundError only matches the api-package sentinel, so a missing plan surfaced as 500 instead of 404 (unlike updatePlan / patchPlan). Route it through mapCreatePlanStorageError so it maps to 404. - Frontend: editPlan swallowed the failure with a generic toast and never reconciled the list. It now returns a success boolean, shows an actionable message on a 404, and the scheduled-purchase Edit handler refetches the planned-purchases list so the orphaned row is dropped. Regression tests at both layers fail on the pre-fix code and pass after. --- frontend/src/__tests__/plans.test.ts | 28 ++++++++++++++++++++++ frontend/src/plans.ts | 27 ++++++++++++++++++--- internal/api/handler_plans.go | 13 +++++++++- internal/api/handler_plans_test.go | 36 ++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index c5953af0e..244efce70 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -1024,6 +1024,34 @@ describe('Plans Module', () => { // getPlan must NOT be called when planId is empty. expect(api.getPlan).not.toHaveBeenCalled(); }); + + test('edit action reconciles the list when the plan is gone (#1403)', async () => { + // Regression for #1403: a scheduled-purchase row can outlive its plan + // (deleted, or the caller's account scope changed). Clicking Edit passes + // the correct plan_id (the #773/#780 fix is intact), but GET /plans/{id} + // now returns 404. The UI must surface an actionable message AND refresh + // the planned-purchases list so the orphaned row is dropped, instead of + // dead-ending on the generic "Failed to load plan details" toast. + const notFound = Object.assign(new Error('HTTP 404'), { status: 404 }); + (api.getPlan as jest.Mock).mockRejectedValue(notFound); + // Clear the getPlannedPurchases call made during beforeEach's loadPlans + // so the assertion below counts only the reconcile triggered by the edit. + (api.getPlannedPurchases as jest.Mock).mockClear(); + + const editBtn = document.querySelector('[data-action="edit"]') as HTMLButtonElement; + editBtn?.click(); + + await new Promise(resolve => setTimeout(resolve, 50)); + + // The right plan FK was used (guards against a #773-class regression). + expect(api.getPlan).toHaveBeenCalledWith('plan-1'); + // Actionable, plan-gone message — NOT the generic dead-end. + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ message: 'This plan is no longer available. It may have been deleted.', kind: 'error' }), + ); + // The stale row is reconciled by refetching the planned purchases. + expect(api.getPlannedPurchases).toHaveBeenCalledTimes(1); + }); }); describe('resume action for paused purchase', () => { diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 3a7df75a4..102f433c1 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -790,7 +790,12 @@ async function handlePlannedPurchaseAction(action: string, purchaseId: string, p console.warn('edit action ignored: missing plan id'); return; } - await editPlan(planId); + // If the plan can't be loaded (deleted or no longer accessible while + // the row was on screen), reconcile the list so the orphaned row is + // dropped instead of leaving a dead Edit button (issue #1403). + if (!(await editPlan(planId))) { + await loadPlannedPurchases(); + } return; case 'disable': { // Use styled async dialog (11-L2) instead of blocking browser confirm(). @@ -1100,7 +1105,11 @@ async function togglePlan(planId: string, enabled: boolean): Promise { } } -async function editPlan(planId: string): Promise { +// editPlan loads the plan and opens the edit modal pre-filled. Returns true +// when the modal opened, false when the plan could not be loaded (e.g. it was +// deleted or is no longer accessible). Callers that render the plan in a list +// use the false result to reconcile a now-stale row (issue #1403). +async function editPlan(planId: string): Promise { try { const backendPlan = await api.getPlan(planId) as unknown as BackendPlan; @@ -1179,9 +1188,21 @@ async function editPlan(planId: string): Promise { wirePlanRangeInputs(); const planModal = document.getElementById('plan-modal'); if (planModal) openModal(planModal); + return true; } catch (error) { console.error('Failed to load plan:', error); - showToast({ message: 'Failed to load plan details', kind: 'error' }); + // A missing/inaccessible plan comes back as 404 (issue #1403): the + // scheduled-purchase row outlived its plan (deleted, or the caller's + // account scope changed). Surface an actionable message instead of the + // generic "Failed to load plan details", and signal failure so the + // caller can reconcile the stale row. Other errors keep the generic + // message (network blip, transient 5xx) since the plan may still exist. + const status = (error as { status?: number }).status; + const message = status === 404 + ? 'This plan is no longer available. It may have been deleted.' + : 'Failed to load plan details'; + showToast({ message, kind: 'error' }); + return false; } } diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 95851a68d..95be37e5f 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -171,9 +171,20 @@ func (h *Handler) getPlan(ctx context.Context, req *events.LambdaFunctionURLRequ return nil, err } + // Map a missing plan to 404 (mirrors updatePlan / patchPlan / + // getPlanForPurchaseCreation). GetPurchasePlan wraps config.ErrNotFound, + // which the router's IsNotFoundError check does NOT recognize (it matches + // only the api-package sentinel), so a raw return here surfaced a deleted + // plan as a 500. A scheduled-purchase row can outlive its plan (issue + // #1403: ON DELETE SET NULL detaches the execution, or the user's account + // scope changes), and the Edit button then loads GET /plans/{id}; the + // frontend needs a 404 to distinguish "plan is gone" from a server error + // and reconcile the stale row instead of dead-ending on a generic message. plan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { - return nil, err + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to load plan", + "getPlan: GetPurchasePlan failed") } // Ensure plan has NextExecutionDate calculated diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 653972175..10c383ee7 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -3,6 +3,7 @@ package api import ( "context" "errors" + "fmt" "testing" "time" @@ -310,6 +311,41 @@ func TestHandler_getPlan(t *testing.T) { assert.Equal(t, "12345678-1234-1234-1234-123456789abc", resultPlan.ID) } +// TestHandler_getPlan_NotFound_MapsTo404 is the backend half of the #1403 fix. +// A scheduled-purchase row can outlive its plan (ON DELETE SET NULL detaches +// the execution, or the caller's account scope changes), and the Edit button +// then loads GET /plans/{id}. GetPurchasePlan wraps config.ErrNotFound, which +// the router's IsNotFoundError sentinel does NOT match, so the pre-fix getPlan +// returned the raw error and the router mapped it to 500. It must be a 404 +// (like updatePlan / patchPlan / getPlanForPurchaseCreation) so the frontend +// can tell "plan is gone" from a server error and reconcile the stale row. +func TestHandler_getPlan_NotFound_MapsTo404(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + planID := "12345678-1234-1234-1234-123456789abc" + // GetPurchasePlan wraps config.ErrNotFound exactly as the Postgres store does. + mockStore.On("GetPurchasePlan", ctx, planID). + Return(nil, fmt.Errorf("%w: purchase plan %s", config.ErrNotFound, planID)) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + _, err := handler.getPlan(ctx, req, planID) + + ce, ok := IsClientError(err) + require.True(t, ok, "a missing plan must map to a ClientError, not propagate raw (500)") + assert.Equal(t, 404, ce.code, "a missing plan must be 404, not 500") + mockStore.AssertExpectations(t) +} + func TestHandler_updatePlan(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore)