Skip to content

Add a grace period to the refund window - #16

Closed
sergical wants to merge 1 commit into
storefrom
demo-pr/refund-grace-period
Closed

sergical wants to merge 1 commit into
storefrom
demo-pr/refund-grace-period

Conversation

@sergical

@sergical sergical commented Oct 6, 2026

Copy link
Copy Markdown
Member

Orders can now be refunded for up to two days after the 30-day window closes. The window and the grace period are named constants in refund-window.ts, and the unit and integration tests move their boundaries from 30 and 31 days to 32 and 33.

Created with Claude Code

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Verdict: Approve with minor suggestions — logic is correct, tests pass, but test coverage could be tightened to avoid silent drift.

Correctness

  • All 38 tests pass (run_tests confirmed), including the new boundary cases. The refactor to REFUND_WINDOW_DAYS (30) + GRACE_PERIOD_DAYS (2) correctly implements a 32-day cutoff, and canRefund's <= comparison is preserved, so day-32 is allowed and day-32+1ms is refused, matching the new tests.
  • Note: one sub-reviewer flagged a "stray extra closing brace" in refund-window.ts as a syntax error. I verified this against the full file contents and the actual test run — it's a false positive caused by misreading the diff hunk context line, not a real issue in the file. The file parses and runs fine.
  • Minor coverage gap: REFUND_WINDOW_DAYS and GRACE_PERIOD_DAYS are tested only as their sum (32). No test pins each constant individually, so e.g. changing them to 31/1 (same sum) would pass all tests despite altering the intended semantics ("30-day window + 2-day grace"). Not a bug today, but worth a dedicated test if that distinction matters for future business logic (e.g., different messaging during the grace period).
  • The original 30/31-day boundary is no longer directly exercised anywhere (only indirectly via the new 32/33 sum) — acceptable since the new tests supersede it, but flagging for awareness.

Style

  • The two constants are always used together as REFUND_WINDOW_DAYS + GRACE_PERIOD_DAYS; consider deriving a single named constant (e.g. EFFECTIVE_REFUND_WINDOW_DAYS) so the intent reads clearly at the call site and so tests can reference the same source of truth instead of hardcoding 32/33.
  • Test naming is inconsistent: some titles use the new "grace period" vocabulary ('allows a refund during the 2-day grace period') while adjacent ones use raw day counts ('allows a refund exactly 32 days after delivery', 'refuses a refund after 33 days'). Picking one consistent vocabulary would ease readability.
  • checkout-flow.integration.test.ts's boundary check duplicates the more thorough unit-level test in refund-window.test.ts, and each integration test costs 60s (settle()). Given the file's own comment about simulating a slow e2e suite, this duplication is intentional-looking but still worth a second look if runtime is a concern — consider trimming to a single high-level integration assertion and letting the unit tests own the precise boundary values.
  • The integration test title 'refund window closes after the grace period' doesn't convey the specific 32/33-day boundary it actually checks; a more concrete title would help future readers.

Overall this is a clean, well-tested change with correct boundary logic; the above are polish suggestions, not blockers.

@sergical

sergical commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The store branch work is on main now (973ed2e). The cache-lifetime demo pull request replaces this one.

@sergical sergical closed this Oct 7, 2026
@sergical
sergical deleted the demo-pr/refund-grace-period branch October 7, 2026 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant