fix(orders): block change-drop when escrow is already funded

[nyxsky404]

Closes #607

The change-drop handler was recalculating and persisting a new total_amount without checking whether escrow was already funded. Once escrowRelease fires on delivery it releases the original on-chain amount, not the updated total, so the driver payout diverges from the Supabase record.

The Escrow.sol contract itself reverts on a second deposit() for the same bookingId ("Escrow exists"), so there's no path to adjust the on-chain amount after funding anyway. The handler now returns 409 early if escrow_status === 'funded', surfacing the constraint explicitly with a recovery hint to cancel and rebook.

Summary by CodeRabbit

• Bug Fixes
  • Prevented changing an order’s drop location after the escrow is marked as funded. Requests are now rejected with an HTTP 409 response, including clear error messaging and recovery guidance.
• Tests
  • Added an integration test to confirm /api/orders/:id/change-drop is blocked when escrow is already funded.

[github-actions]

🎉 Thank you for your contribution! Your pull request has been received and will be reviewed shortly.

If you enjoy the project, please consider giving the repository a ⭐. You can also follow my GitHub profile to stay updated on future open-source projects.

Thanks for being part of the community! 🚀

[coderabbitai]

📝 Walkthrough

Walkthrough

A guard clause is added to the PUT /orders/:id/change-drop endpoint in orderRoutes.js. When the order's escrow_status equals 'funded', the handler now returns HTTP 409 with a structured error and recovery message, preventing drop location changes after escrow is locked on-chain. An integration test validates this behavior.

Changes

Block change-drop when escrow is funded

Layer / File(s)	Summary
409 guard for funded escrow and validation test backend/api/src/routes/orderRoutes.js, backend/api/test/integration/orders.test.js	Adds an early-return check at the top of the change-drop handler: if order.escrow_status === 'funded', responds immediately with HTTP 409, an error string, and a recovery instruction to cancel and rebook instead. Integration test asserts the 409 response and validates presence of error and recovery fields in the response body.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• KanishJebaMathewM/Truxify#511: Introduces or modifies the change-drop endpoint in the same route file that this PR guards against funded-escrow changes.
• KanishJebaMathewM/Truxify#513: Introduces the escrow integration that sets escrow_status to 'funded' on bid acceptance — the field this PR's guard reads.

Suggested labels

type:testing, blockchain

Poem

🐇 Hippity-hop, the escrow is set,
No changing the drop once the funds are met!
A 409 guards what the contract has locked,
The driver's fair pay shall never be mocked.
Rebook and reroute if you must, little friend —
The rabbit keeps ledgers balanced to the end! 🌟

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(orders): block change-drop when escrow is already funded' directly and accurately describes the main change: preventing drop location changes after escrow funding with a 409 response.
Linked Issues check	✅ Passed	The PR implements the proposed fix from #607: it blocks change-drop operations when escrow_status is 'funded' (returning HTTP 409) and includes recovery guidance, matching all coding objectives.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the requirements in #607: the backend handler adds the escrow_status check, and the test validates the 409 response with error and recovery fields; no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[Copilot]

Pull request overview

Blocks drop-location changes once an order’s escrow has already been funded, preventing the database total_amount from diverging from the immutable on-chain escrow amount (and therefore avoiding incorrect driver payouts on escrow release).

Changes:

• Add an early 409 Conflict return in the PUT /orders/:id/change-drop handler when order.escrow_status === 'funded'.
• Surface an explicit recovery hint to cancel and rebook since the on-chain amount can’t be adjusted post-funding.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

backend/api/test/integration/orders.test.js (1)

1832-1834: ⚡ Quick win

Strengthen response-contract assertions for error/recovery.

Right now this passes as long as keys exist. Since this endpoint’s UX depends on explicit recovery guidance, assert message content (or stable substrings) to prevent silent regressions.

Proposed test hardening

     expect(res.status).toBe(409);
-    expect(res.body).toHaveProperty('error');
-    expect(res.body).toHaveProperty('recovery');
+    expect(res.body.error).toBe('Drop location cannot be changed after escrow has been funded.');
+    expect(res.body.recovery).toContain('Cancel this order');
+    expect(res.body.recovery).toContain('rebook');

🤖 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 `@backend/api/test/integration/orders.test.js` around lines 1832 - 1834, The
test assertions in the integration test for orders endpoint are only checking
for the existence of error and recovery properties without validating their
content. Strengthen the assertions for res.body.error and res.body.recovery to
validate that they contain actual meaningful message content, such as checking
that they are non-empty strings or contain expected substrings relevant to the
conflict resolution guidance. This prevents silent regressions where these
properties might exist but be empty or contain incorrect values.

🤖 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 `@backend/api/test/integration/orders.test.js`:
- Around line 1832-1834: The test assertions in the integration test for orders
endpoint are only checking for the existence of error and recovery properties
without validating their content. Strengthen the assertions for res.body.error
and res.body.recovery to validate that they contain actual meaningful message
content, such as checking that they are non-empty strings or contain expected
substrings relevant to the conflict resolution guidance. This prevents silent
regressions where these properties might exist but be empty or contain incorrect
values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c0456fe7-b5d8-4823-8a74-dc1bd1bcb526

📥 Commits

Reviewing files that changed from the base of the PR and between 21899a4 and 4a9ddc8.

📒 Files selected for processing (1)

• backend/api/test/integration/orders.test.js

[nyxsky404]

CI failures are pre-existing — backend test failures (expected 500, got 422) are caused by the wallet-address guard added in an earlier PR, not this change. Flutter failures are unrelated infra issues present on main.