Skip to content

Charge the heavy shipping rate from 5 kg - #35

Open
sergical wants to merge 1 commit into
mainfrom
fix/shipping-heavy-rate-from-5kg
Open

sergical wants to merge 1 commit into
mainfrom
fix/shipping-heavy-rate-from-5kg

Conversation

@sergical

Copy link
Copy Markdown
Member

The heavy rate now starts at 5 kg instead of above 5 kg, to match the carrier's price table. The checkout flow suite gets a 5 kg case for standard and express.

Created with Claude Code

@github-actions

Copy link
Copy Markdown

Verdict: Needs discussion before merge — likely unintentional boundary change in shippingCost, otherwise fine.

Correctness

  • shipping.ts: the diff changes the pricing boundary from weightKg > 5 to weightKg >= 5. This means a parcel weighing exactly 5 kg now gets charged the heavy rate (12) instead of the previous light rate (6) — a real behavioral change, not just a refactor. Nothing in the diff (commit message, comment, or test) explains why this boundary moved, so it's unclear whether this is an intentional business-rule fix or an off-by-one slip. If other code/tests elsewhere in the codebase assume "5 kg and under = light rate," this is a silent regression.
  • The new test (expect(shippingCost(5)).toBe(12)) only locks in whatever the current implementation does; it doesn't document/justify the boundary change and doesn't test the actual edge (e.g., a value just below 5, like 4.99, to confirm the light-rate side is still correct). As written it would pass regardless of whether > or >= was the "right" choice, so it provides no protection against this being an accidental change.
  • Tests pass (7/7, 0 failures) per the test run, so no functional breakage detected in this suite — but that doesn't validate the boundary decision itself.

Style

  • shipping.ts uses magic numbers (5, 12, 6) with no named constants (e.g., HEAVY_PARCEL_THRESHOLD_KG, BASE_RATE, HEAVY_RATE), making the pricing rule harder to read and to update safely — this would also have made the boundary change's intent clearer.
  • The new test calls await settle() (an artificial 80-second delay), continuing an existing anti-pattern. Unlike the other tests in this file which simulate end-to-end checkout flow duration, shippingCost is a pure synchronous function, so this delay adds no value here and just inflates suite runtime further.
  • The test name/assertions (shippingCost(5, true) → 24) don't make clear that true is "express" or that 12/24 represent base vs. express rates. Consider splitting into two clearly named cases or adding a named boolean/comment for clarity.

Summary

Functionally the change is simple and tests currently pass, but the > → >= boundary shift in shippingCost needs explicit confirmation that it's intentional (ideally called out in the PR description) before merging, since it silently changes pricing for exactly-5kg parcels. Minor style cleanup (named constants, dropping the unnecessary settle() delay, clearer assertions) would also improve this PR.

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