backend requirements onthe xustomer app
This commit is contained in:
928
docs/customer-app-api.md
Normal file
928
docs/customer-app-api.md
Normal file
@@ -0,0 +1,928 @@
|
||||
# Customer App API (`doormile_cx`) — change record & integration guide
|
||||
|
||||
**Audience:** the `doormile_cx` app developer, plus anyone reviewing this before
|
||||
it goes to production.
|
||||
**Backend:** Go + Fiber, `api.doormile.com/api/v1`, namespace `/customer/*`.
|
||||
**Status:** code complete, compiler- and unit-verified, **not yet run against a
|
||||
real database**. Read §11 before deploying.
|
||||
|
||||
| | |
|
||||
|---|---|
|
||||
| OpenAPI 3.1 spec | [`openapi-customer.yaml`](openapi-customer.yaml) |
|
||||
| Staging seed | [`../seed_customer_app.sql`](../seed_customer_app.sql) |
|
||||
| Requirements this answers | *Doormile — Backend Requirements (Customer App v1)* |
|
||||
| Reviewed | Once, by a reviewer with the Flutter apps but not this repo — **§13** records every finding and what changed |
|
||||
|
||||
---
|
||||
|
||||
## 0. The one question first: does this affect the Miler app or the consoles?
|
||||
|
||||
# YES.
|
||||
|
||||
Not "no". Anyone telling you otherwise has not read the diff. The customer work
|
||||
could not be built without touching shared code, because the thing it changes —
|
||||
one pickup producing many orders — is a fact the rider app and the consoles both
|
||||
have to cope with.
|
||||
|
||||
Here is the complete list, with severity. Nothing is omitted.
|
||||
|
||||
### 🔴 Behaviour changes those clients WILL observe
|
||||
|
||||
| # | What changed | Who sees it | Action needed |
|
||||
|---|---|---|---|
|
||||
| 1 | **Booking references are now `DM-482913`**, not `DM-BK-A1B2C3D4-48291`. New bookings only; existing rows keep their old strings. | Admin console, hub console, Miler app — anywhere a booking number is displayed | None in code (nothing parses the format). Tell ops the format changed. |
|
||||
| 2 | **Tracking numbers are now `DMX10482913`**, not `DM-TRK-A1B2C3D4-48291`. New consignments only. | Same as above, plus any printed label or customer-facing tracking link | Same. Check label templates for a hardcoded width. |
|
||||
| 3 | **`GET /miler/bookings` can return more than one row per booking.** A customer-app pickup with 3 destinations becomes 3 stops *after* collection — each with its own `consignmentid`, `trackingno`, address and COD. Before collection it is still 1 row. Console/express bookings are still always 1 row. | Miler app | **A multi-file change, not a one-liner. See §0.6.** |
|
||||
| 4 | **`POST /miler/bookings/{id}/pickup-complete` gained a `consignments` array** listing every order the pickup produced. All previous top-level fields still describe the first order, unchanged. | Miler app | Purely additive — safe to ignore, but a multi-destination build should read it. |
|
||||
| 5 | **`POST /miler/bookings/{id}/parcel` accepts an optional `photos: []` per parcel** (storage keys from `/miler/uploads/sign`). Response shape unchanged. | Miler app | Optional. Without photos the customer receipt shows a weight and no evidence. |
|
||||
| 6 | **`PickupBooking` JSON gained 10 fields** (`slotid`, `customerstage`, `customerstatus`, `estimateminrupees`, `estimatemaxrupees`, `routekm`, `pickuptitle`, `pickupsub`, `cancelreason`, `destinations`) and `BookingParcel` gained `bookingdestinationid`. These serialise on every admin/hub endpoint that returns the model raw. | Admin + hub consoles | Additive JSON. Safe for a JS client that reads named keys. |
|
||||
| 7 | **Every response now carries an `X-Request-Id` header.** | Everyone | None. Log it — it is how a support report gets correlated. |
|
||||
| 8 | **Unknown paths under `/customer/*` answer `401`, not `404`.** So the retired `doormile_customer_app` calling `POST /customer/login` gets `401 "authorization header is required"`. | The old customer app | Expected — that app is being retired. Pre-existing Fiber group behaviour, not introduced here. |
|
||||
|
||||
### 🟡 Internal changes with no client-visible contract change
|
||||
|
||||
| # | What changed | Why it is safe |
|
||||
|---|---|---|
|
||||
| 9 | Three miler endpoints now resolve a consignment through `bookingdestinations` instead of `pickupbookings.consignmentid` | Bug fix. Identical result for a single-destination booking; the old query returned *nothing* for orders 2..N. See §5. |
|
||||
| 10 | `assignMilerTx` and `commitAssignment` record a customer stage | Guarded: returns immediately for any booking whose source is not `Customer_App`. An express booking does zero extra writes. |
|
||||
| 11 | `AcceptMilerAssignment`, `BookingReachedCustomer`, `MilerCancelAssignment`, `MilerStartDelivery`, `MilerInwardConsignmentAtHub`, `MilerDeliverConsignment` record customer stages | Same guard. Response bodies unchanged. |
|
||||
| 12 | `middlewares.Idempotency` scopes anonymous callers differently | Authenticated key format is **byte-identical** to before (`idem:<uid>:<key>`), so no in-flight rider key is orphaned at deploy. See §5.4. |
|
||||
| 13 | Request logs gained `client`, `platform`, `requestid` fields | Log-only. |
|
||||
| 14 | `utils.GenerateToken` delegates to a new `GenerateTokenWithTTL` | Still 24h for miler/admin/hub. Only the customer surface passes a different TTL. |
|
||||
| 15 | `internal/storage` gained `PresignGet` | New function; nothing existing calls it. |
|
||||
| 16 | `middlewares/city_gate.go` gained an exported `PincodeInOperatingCity` | New function; `CityGateMiddleware` itself is unchanged. |
|
||||
| 17 | 9 new tables + 11 new columns via `AutoMigrate`; 2 sequences, 2 indexes | All additive and nullable/defaulted. See §11 for the deploy note. |
|
||||
|
||||
### 🟢 Deliberately NOT changed, though I was tempted
|
||||
|
||||
Two things I changed while building, then **reverted**, because they were miler
|
||||
behaviour and out of scope:
|
||||
|
||||
- **Rider availability after pickup-complete.** I had made a rider stay
|
||||
`Picked_Up` when a hyperlocal parcel goes straight to `Out_for_Delivery`. The
|
||||
endpoint has always set them `Available` there. The existing code comment says
|
||||
the opposite of what the code does — a pre-existing inconsistency I have
|
||||
flagged rather than silently "fixed", because changing it alters live rider
|
||||
availability and the dispatch pool. **Ops decision, not mine.**
|
||||
- **`POST /miler/consignments/{id}/deliver` error status.** I had changed a
|
||||
"not yours" failure from `404` to `403` for consistency with start-delivery.
|
||||
Reverted: the deployed rider app was built against `404` and its exact
|
||||
messages. Only the broken *lookup* is fixed.
|
||||
|
||||
---
|
||||
|
||||
## 0.5 Page-by-page: exactly which screens are affected
|
||||
|
||||
Read from the client source in this workspace (`doormile_crm` = admin console,
|
||||
`doormile_hub_console` = hub console), not inferred from the API. The Miler app
|
||||
is not in this workspace, so its rows are derived from the endpoints it calls and
|
||||
must be confirmed against the Flutter source.
|
||||
|
||||
Legend: 🔴 needs a code change · 🟡 visible change, no code change · 🟢 fixed by
|
||||
this work · ⚪ no impact
|
||||
|
||||
### Miler app (Flutter — verify against the app source)
|
||||
|
||||
| Screen | Endpoint behind it | Impact |
|
||||
|---|---|---|
|
||||
| **Home / my stops** | `GET /miler/bookings` | 🔴 **A pickup can now be several rows.** After collection, a 3-destination customer booking returns 3 stops sharing one `bookingid`. **Key the list on `consignmentid`.** New fields `destinationseq` / `destinationcount` give you "Stop 2 of 3", and `trackingno`, `recipientname`, `recipientphone` are now per stop. Before collection it is still exactly 1 row, and console/express bookings are always 1 row. |
|
||||
| **Pickup — complete** | `POST /miler/bookings/{id}/pickup-complete` | 🟡 Response gained `consignments[]` (every order the visit produced). All existing top-level fields still describe the first order and are unchanged, so an old build keeps working. |
|
||||
| **Pickup — weigh/photograph parcels** | `POST /miler/bookings/{id}/parcel` | 🟡 Each parcel now accepts an optional `photos: []` of storage keys from `/miler/uploads/sign`. Response shape unchanged. Without photos the customer's receipt shows a weight with no evidence behind it. |
|
||||
| **Delivery — deliver / skip** | `POST /miler/consignments/{id}/deliver` | ⚪ Contract unchanged. The internal lookup was broken for orders 2..N and is fixed; status codes and messages are byte-identical. |
|
||||
| **Base handover** | `POST /miler/consignments/{id}/inward-at-hub` | ⚪ Contract unchanged; same lookup fix. |
|
||||
| **Earnings** | `GET /miler/earnings` | 🟢 **Fixed before it shipped.** Customer-app jobs would have recorded ₹0 — see §5.6. |
|
||||
| **Any screen showing a tracking number** | — | 🟡 `DMX10482913` instead of `DM-TRK-A1B2C3D4-48291`. Ten characters shorter, so nothing overflows. |
|
||||
| Duty, breaks, notifications, support, profile, bases | — | ⚪ Untouched. |
|
||||
|
||||
### Admin console (`doormile_crm`)
|
||||
|
||||
Only **one** of its screens reads booking shape at all. Verified by grep across
|
||||
`src/pages`.
|
||||
|
||||
| Screen | File | Impact |
|
||||
|---|---|---|
|
||||
| **Orders list** | `src/pages/bookings/Bookings.jsx` | 🟡 Four things: (1) the **Booking No** column now shows `DM-482913` — column is `proportional(1.5)`, the string is shorter, nothing overflows; (2) **search** filters `bookingno` by substring, so typing `DM-BK` finds no new bookings — tell ops; (3) the **Delivery City** column reads the flat `deliverycity`, which is **destination 0 only** — a 3-destination pickup shows "Chennai" and does not hint at the other two; (4) the **Price** column reads `serviceoptions[0].estimatedprice` and 🟢 **would have shown "N/A" for every customer booking** until §5.6 was fixed. |
|
||||
| **Cancel order** (from the Orders list) | `POST /admin/bookings/{id}/cancel`, `.../bulk-cancel` | 🟢 **Fixed before it shipped.** An ops cancel never reached the customer's app — see §5.7. No console change needed; the fix is backend-side. |
|
||||
| Dashboard, Tenants, Customers, Pricing, Riders, Reports, Invoices, Team, Settings, Survey, auth & error screens | — | ⚪ None of them read booking or consignment shape. |
|
||||
|
||||
### Hub console (`doormile_hub_console`)
|
||||
|
||||
| Screen | File | Reads | Impact |
|
||||
|---|---|---|---|
|
||||
| **Inbound** | `src/pages/operations/Inbound.jsx` | `trackingno` | 🟡 Tracking-number format change is visible in the table. Destination cell already ellipsises at `maxWidth: 200px`. |
|
||||
| **Routing** | `src/pages/operations/Routing.jsx` | `trackingno` | 🟡 Format change visible in the package list and the detail card (monospace, `0.85rem`). Search-by-tracking-number still works — it matches the value the API returns, not a pattern. |
|
||||
| **Order Assignment** | `src/pages/operations/OrderAssignment.jsx` | `bookingid`, `deliveryaddress` | 🟡 The row id uses the numeric `bookingid`, so the ID-format change does not touch it. But `drop` reads the flat `deliveryaddress`, which is **destination 0 only** — staff assigning a 3-destination pickup see one address. Not broken; incomplete. |
|
||||
| **Rider Routes** | `src/pages/operations/RiderRoutes.jsx` | `bookingid`, `consignmentid`, `parcels` | 🟡 Reads `consignmentid` off the booking, which names only the **first** order. A rider carrying three parcels from one visit may render as one stop on this screen. |
|
||||
| **Tracking Map** | `src/pages/operations/TrackingMap.jsx` | `bookingid` | ⚪ Numeric id only. |
|
||||
| **Dashboard** | `src/pages/Dashboard.jsx` | `parcels` | ⚪ Counts only. Every parcel still hangs off the booking, so the totals stay correct. |
|
||||
| **Dispatch** | `src/pages/operations/Dispatch.jsx` | `parcels` | ⚪ As above. |
|
||||
| **Riders** | `src/pages/operations/Riders.jsx` | `parcels` | ⚪ As above. |
|
||||
| Hub Settings, Login, Signup | — | ⚪ Untouched. |
|
||||
|
||||
### What nobody has to change today
|
||||
|
||||
Multi-destination pickups **only exist once the new customer app is live**. Every
|
||||
booking in the database today has zero destination rows and therefore takes the
|
||||
single-leg path everywhere — the rider queue returns one row, the consoles show
|
||||
one address, and the fan-out never runs. The 🟡 "destination 0 only" rows above
|
||||
become real on the day the first three-destination pickup is booked, not on the
|
||||
day this deploys.
|
||||
|
||||
The exceptions that are live immediately: the **ID formats** (§0 items 1–2) and
|
||||
the extra **JSON fields** (§0 item 6).
|
||||
|
||||
---
|
||||
|
||||
## 0.6 The rider-app change, scoped properly
|
||||
|
||||
An earlier draft of this document said "key the list on `consignmentid`" as
|
||||
though that were a small edit. It is not, and the correction came from a review
|
||||
with access to the Flutter source that this backend repo does not have. Recorded
|
||||
here in full because the item is assigned to the app team and was understated.
|
||||
|
||||
**The rider app's identity key is `orderid`, not `bookingid`.** The adapter sets
|
||||
it at `lib/data/api_config.dart:321`:
|
||||
|
||||
```dart
|
||||
'orderid': ref ?? id, // ref = bookingreference, id = bookingid
|
||||
```
|
||||
|
||||
Both of those are **booking-level**, and all three destination rows of one
|
||||
pickup carry the same `bookingid` and the same `bookingreference`. So `orderid`
|
||||
is identical across the three — and `orderid` is what dedupes:
|
||||
|
||||
- `lib/data/accepted_store.dart:527` — accepted stops live in a `Map` keyed on
|
||||
`orderid`. Three stops collapse to one, and because that store is
|
||||
SharedPreferences-backed, **the collapse survives an app restart.**
|
||||
- `lib/data/work_repository.dart:314,321` and `order_events.dart` — per-order
|
||||
event stamps key on `orderid`, so orders 2..N overwrite order 1's clocks.
|
||||
|
||||
`consignmentid` is available to the adapter (`api_config.dart:443`), so the
|
||||
change is feasible — but it lands across `accepted_store`, `work_repository`,
|
||||
`order_events` and `assignment_lookup`.
|
||||
|
||||
**Until it ships, a multi-destination pickup shows the rider one stop instead of
|
||||
three** — the same customer-visible failure §5.1 fixed on the server. The server
|
||||
being right is not enough here.
|
||||
|
||||
**`destinationseq` / `destinationcount` are invisible to the app today.**
|
||||
`pickupFromBooking` builds a fixed map, so a field the adapter does not name is a
|
||||
field the app can never see (the file says as much around line 583). "Stop 2 of
|
||||
3" needs those two keys added to the adapter as well.
|
||||
|
||||
### The mitigation that needs no app release
|
||||
|
||||
`maxDestinations` is **server configuration**, not a constant — the client reads
|
||||
it from `GET /customer/config/booking-limits` and adapts. Setting
|
||||
`customerbookinglimits.maxdestinations = 1` keeps every pickup single-destination
|
||||
until the rider build lands, which means:
|
||||
|
||||
- the customer app can ship and book end to end today;
|
||||
- the fan-out code stays dormant, so no order can be stranded;
|
||||
- lifting the cap later is one `UPDATE`, with no deploy on any side.
|
||||
|
||||
**Recommendation: seed it at 1 and raise it only once a rider build that keys on
|
||||
`consignmentid` is live.** This is the same discipline
|
||||
`MILER_HUB_HANDOVER_ENABLED` already applies in this codebase — do not turn on a
|
||||
server behaviour the deployed rider app cannot complete.
|
||||
|
||||
---
|
||||
|
||||
## 0.7 Fan-out row audit — field by field
|
||||
|
||||
Requested audit, run against the code and locked down by tests. `GET /miler/bookings`
|
||||
after collection, one row per order.
|
||||
|
||||
| Field | Source | Destination-specific? | Test |
|
||||
|---|---|---|---|
|
||||
| `consignmentid` | `bookingdestinations.consignmentid` | ✅ | `TestFanoutTrackingAndConsignmentIdsStayWithTheirStop` |
|
||||
| `trackingno` | `bookingdestinations.trackingno` | ✅ | same |
|
||||
| `deliveryaddress` | built from the destination's building / street / landmark / district / state | ✅ | `TestFanoutStopsHaveDistinctAddresses` |
|
||||
| `deliverylatitude` | destination pin, else district centre | ✅ | `TestFanoutStopsHaveDistinctCoordinates` |
|
||||
| `deliverylongitude` | same | ✅ | same |
|
||||
| `recipientname` | `bookingdestinations.recipientname` | ✅ | `TestFanoutRecipientsStayWithTheirStop` |
|
||||
| `recipientphone` | `bookingdestinations.recipientphone` | ✅ | same |
|
||||
| `collectionamt` | consignment `codamount − codcollected`, falling back to the destination's own `codamount` | ✅ **newly added** | `TestFanoutCodIsNotCopiedAcrossDestinations` |
|
||||
| `destinationseq` | `bookingdestinations.seq` | ✅ | `TestFanoutRouteOrderFollowsDestinationSeq` |
|
||||
| `destinationcount` | number of stops on the pickup | ✅ uniform per booking | `TestFanoutDestinationCountIsUniform` |
|
||||
|
||||
**Three findings from running this audit:**
|
||||
|
||||
1. **`collectionamt` did not exist.** The endpoint emitted `codamount` only.
|
||||
Added as an additional key rather than a rename — `codamount` is what this
|
||||
endpoint has always sent and the deployed rider build parses it, so removing
|
||||
it would break every stop on every existing device. Both are written from one
|
||||
variable and cannot drift.
|
||||
|
||||
2. **COD was not on the stop at all.** It was resolved in the handler from the
|
||||
consignment map, so it could not be unit-proven, and it fell back to **0** if
|
||||
the consignment row failed to load. A rider shown ₹0 collects nothing. The
|
||||
stop now carries the destination's own `codamount` as the fallback.
|
||||
|
||||
3. **Stop ordering was not guaranteed.** `milerStopsForBooking` iterated the
|
||||
slice as given and relied entirely on the caller's `ORDER BY seq`. It worked,
|
||||
but the ordering *is the route the rider drives*, and leaving it as an
|
||||
unstated precondition means the next caller silently reorders someone's
|
||||
afternoon. It now sorts by `seq` on a copy.
|
||||
|
||||
**Booking-level fallback is confined to one place and one case:** a booking with
|
||||
**no destination rows** — every console/express booking and every row written
|
||||
before this table existed — produces exactly one stop from the booking's flat
|
||||
columns. A booking that *has* destination rows never reads them. Destination 0's
|
||||
coordinates are used as a fallback for stop 0 only, and only when that
|
||||
destination has no pin of its own, because that is the same row mirrored.
|
||||
|
||||
---
|
||||
|
||||
## 1. v1 requirements → what actually shipped
|
||||
|
||||
The requirements doc described the target. This is the delta between it and the
|
||||
code as it now stands.
|
||||
|
||||
### 1.1 Endpoints — all 22 requirement endpoints implemented
|
||||
|
||||
Route arithmetic, since three numbers in this document have to reconcile:
|
||||
**28 registered customer routes** = 8 pre-auth (4 auth + 4 catalogue) + 20
|
||||
authenticated. The 22 below is the count of endpoints the *requirements* asked
|
||||
for; the remaining 6 are the profile and saved-address routes in the table under
|
||||
it. The OpenAPI spec now documents all 28 (24 paths / 28 operations).
|
||||
|
||||
| § | Endpoint | Status | Deviation from the requirement |
|
||||
|---|---|---|---|
|
||||
| 4.1 | `POST /customer/auth/otp/request` | ✅ | Answers identically whether or not the account exists — telling an anonymous caller "no account found" would make this a directory of who is registered. |
|
||||
| 4.2 | `POST /customer/auth/signup` | ✅ | Existing phone treated as sign-in, as specified. |
|
||||
| 4.3 | `POST /customer/auth/otp/verify` | ✅ | Idempotency-Key honoured. |
|
||||
| 4.4 | `POST /customer/auth/refresh` | ✅ | Rotation. A **revoked** token replayed revokes the whole chain. |
|
||||
| 4.4 | `POST /customer/auth/logout` | ✅ | No `refreshToken` in the body ⇒ signs out everywhere. |
|
||||
| 4.4 | `GET /customer/auth/me` | ✅ | — |
|
||||
| 5.1 | `GET /customer/serviceability/states` | ✅ | + ETag/304. |
|
||||
| 5.2 | `GET .../states/{code}/districts` | ✅ | + ETag/304. |
|
||||
| 5.3 | `GET /customer/pickup-slots` | ✅ | Slot ids encode their date (`slot_20260905_t1`) so a stale cached slot resolves to *that* day and is refused, not silently booked today. |
|
||||
| 5.4 | `GET /customer/config/booking-limits` | ✅ | Accepts `lat`/`lng` for per-city limits. |
|
||||
| 6.1 | `GET /customer/places/reverse-geocode` | ✅ | On upstream failure returns a coordinate label, **not** an error. |
|
||||
| 6.2 | `GET /customer/places/search` | ✅ | Empty `q` ⇒ saved + recent places. |
|
||||
| 7 | `POST /customer/fare/estimate` | ✅ | — |
|
||||
| 9.1 | `POST /customer/bookings` | ✅ | — |
|
||||
| 9.2 | `GET /customer/bookings` | ✅ | **Keyset** pagination, not offset. |
|
||||
| 9.3 | `GET /customer/bookings/{reference}` | ✅ | + ETag/304 for polling. |
|
||||
| 9.4 | `POST .../cancel` | ✅ | Cancelling an already-cancelled booking returns **200**, not 409. |
|
||||
| 9.5 | `PATCH .../destinations/{index}` | ✅ | — |
|
||||
| 9.6 | `GET /customer/orders/{trackingId}` | ✅ | Returns the full booking object (the requirement offered a slimmer shape; the client already parses this one). |
|
||||
| 10 | `POST /customer/devices` | ✅ | + `DELETE /customer/devices/{token}`. |
|
||||
| 10 | Push on milestone change | ✅ | `on_the_way` and `order_created` deliberately silent, per §10. |
|
||||
| 11 | `POST /customer/ops/bookings/{ref}/stage` | ✅ | Double-gated. Walks every intermediate stage rather than jumping. |
|
||||
|
||||
**Also kept** (not in the requirements, not removed): `GET/PUT
|
||||
/customer/profile`, `GET/POST/PUT/DELETE /customer/locations` — these answer §13.5
|
||||
"saved addresses".
|
||||
|
||||
### Identifier scrambling — sequence-backed, but not readable
|
||||
|
||||
Both identifiers come off a Postgres sequence, because a sequence is the only
|
||||
generator here that can promise uniqueness: the columns are `UNIQUE`, and a
|
||||
random 8-digit id collides with ~43% probability by the ten-thousandth parcel —
|
||||
a rider unable to complete a pickup.
|
||||
|
||||
But a raw sequence is readable. `DMX10000042` and `DMX10000043` are visibly
|
||||
adjacent, so anyone holding two numbers learns the throughput between them, and
|
||||
anyone holding one can guess its neighbours. So the index goes through a keyed
|
||||
4-round Feistel permutation before it is formatted
|
||||
(`controllers/cxIdentifierScramble.go`):
|
||||
|
||||
```
|
||||
seq 10004200 -> DMX35031894 seq 100000 -> DM-499505
|
||||
seq 10004201 -> DMX31070203 seq 100001 -> DM-341775
|
||||
seq 10004202 -> DMX62788115 seq 100002 -> DM-106366
|
||||
```
|
||||
|
||||
A Feistel network is a bijection for **any** round function, so uniqueness is
|
||||
untouched. `TestBookingScrambleIsCollisionFree` proves it **exhaustively across
|
||||
all 900,000 booking references**; the tracking test covers 200,000 consecutive
|
||||
values; `TestFeistelIsAPermutation` checks the primitive itself exhaustively.
|
||||
|
||||
Deliberately **not** the obvious one-liner (multiply by a coprime). That is also
|
||||
a bijection, but it is *linear* — and a multi-destination pickup hands one
|
||||
customer three consecutive sequence values, so the differences between their
|
||||
three tracking numbers would all equal the multiplier. A single booking would
|
||||
reveal the mapping and make the whole range walkable.
|
||||
`TestOneBookingDoesNotLeakTheMapping` rejects that design.
|
||||
|
||||
Past the fixed-width range (900,000 bookings / 90,000,000 orders) the identifier
|
||||
grows a digit rather than wrapping onto one already issued. Uniqueness is never
|
||||
traded for appearance.
|
||||
|
||||
**This is not a security boundary.** Every route that resolves a tracking number
|
||||
is already authenticated and owner-scoped — that is what stops a stranger reading
|
||||
someone's parcel. This removes the leak in the identifier itself, so
|
||||
authorisation is not the only thing standing between an outsider and your volume
|
||||
figures. Key: `CX_ID_SCRAMBLE_KEY`; a working default ships so the scrambling can
|
||||
never be silently off.
|
||||
|
||||
### 1.2 Contract decisions the requirements left open
|
||||
|
||||
| Question | Decision | Consequence for the app |
|
||||
|---|---|---|
|
||||
| §3.2 field naming | **camelCase**, as preferred | **No mapping layer needed.** Served from purpose-built projections, so `/miler/*` keeps its lowercase keys. |
|
||||
| §3.3 timestamps | **Epoch milliseconds, UTC, integer** | `DateTime.fromMillisecondsSinceEpoch(int)` works as-is. |
|
||||
| §3.1 envelope | Payload in `data` on **every** response, auth included | One parser for the whole surface. |
|
||||
| §9.6 order shape | Full booking object | ⚠️ **Not "reuse `Booking.fromJson`" as written earlier.** That parser exists but today reads only `reference`, `stage`, `cancellable`, `createdAt`, `pickup`, `slotId` and four destination fields. It ignores `status`, `miler`, `deliveryAgent`, `fare`, `amountPaid`, `deliveredAt`, `history`, `verification`, `details`, per-destination `stage`, `routeKm` and `expectedDelivery`. The server shape is right; the client parser has to be finished. Appendix B of the requirements said so and this document should not have glossed it. |
|
||||
| §9.3 `miler.phone` | Proxy when `MILER_CALL_PROXY` is set, real number otherwise | **Currently unset ⇒ real number.** See §10.2. |
|
||||
| Identifier format | Sequence-backed `DM-######` / `DMX########` | Matches the designs. |
|
||||
|
||||
### 1.3 Timestamps — the defect that would have shipped silently
|
||||
|
||||
This database stores **IST wall-clock digits** in its timestamp columns (the DSN
|
||||
sets `TimeZone=Asia/Kolkata`; `utils.DBNow` exists because of it). Calling
|
||||
`.UnixMilli()` on a value read back from those columns is **off by 5h30m** — the
|
||||
same class of fault as a naive local string with a `Z` on it, which already
|
||||
produced "yesterday's work shown as today" on the Miler app.
|
||||
|
||||
Every timestamp leaving `/customer/*` goes through `utils.EpochMillis`, which
|
||||
reinterprets the wall clock in IST first. It is correct for **both** shapes the
|
||||
driver can produce (UTC-tagged-with-IST-digits, and correctly `+05:30`-tagged),
|
||||
so a future column-type change cannot silently shift the tracking screen.
|
||||
Asserted in `utils/epoch_test.go`.
|
||||
|
||||
---
|
||||
|
||||
## 2. The structural change: one pickup → many orders
|
||||
|
||||
`pickupbookings` carried exactly **one** delivery address in its own columns.
|
||||
There was nowhere to put a second, so the "one visit, three orders" model in §1
|
||||
of the requirements was not expressible at all.
|
||||
|
||||
```
|
||||
Pickup booking DM-482913
|
||||
├── destination 0 : Chennai, TN · 2 packages
|
||||
├── destination 1 : Ernakulam, KL · 1 package
|
||||
└── destination 2 : Bengaluru Urban, KA· 1 package
|
||||
│ miler collects everything in ONE visit
|
||||
▼
|
||||
pickup-complete fans out → 3 consignments, 3 tracking numbers
|
||||
DMX10482913 DMX10559120 DMX10662004
|
||||
```
|
||||
|
||||
**The compatibility rule that makes this safe — do not break it:** destination 0
|
||||
is mirrored onto the booking's flat `delivery*` columns. The Miler app, the hub
|
||||
console, the routing code and the hyperlocal check all read those columns and
|
||||
**none of them changed**. A booking with *no* destination rows — every
|
||||
console-created express booking and every pre-existing row — produces exactly one
|
||||
consignment through the same loop, byte-for-byte as before.
|
||||
|
||||
Single-destination is one leg, not a special case, in either direction.
|
||||
|
||||
---
|
||||
|
||||
## 3. File-by-file: what changed and why
|
||||
|
||||
### 3.1 New files (23)
|
||||
|
||||
| File | Lines | Purpose |
|
||||
|---|---|---|
|
||||
| `models/customer_app.go` | 241 | The 9 new tables. `BookingDestination` is the one that makes multi-destination expressible. |
|
||||
| `internal/cxstage/stage.go` | 405 | **The core of §8.3.** Records one append-only stage row per transition, in the caller's transaction. Owns the slowest-order rollup and the `Release` walk-back. |
|
||||
| `controllers/cxBookingView.go` | 744 | The canonical §9.3 booking object. Every read that returns a booking goes through `renderCxBooking`; `loadCxBundle` batches so a tracking poll is a fixed number of queries. |
|
||||
| `controllers/cxBookingController.go` | 819 | §9.1–9.6 — create, list, detail, cancel, patch, order-by-tracking-id. |
|
||||
| `controllers/cxAuthController.go` | 614 | §4 — OTP over phone/email, rotation-backed refresh, E.164 normalisation. |
|
||||
| `controllers/cxCatalogueController.go` | 516 | §5 — serviceability, capacity-aware slots, limits, ETag support. |
|
||||
| `controllers/cxPlacesController.go` | 350 | §6 — geocoder proxy + Redis cache. |
|
||||
| `controllers/cxFareController.go` | 266 | §7 — estimate range, multi-stop uplift. |
|
||||
| `controllers/cxPickupFanout.go` | 251 | Splits a booking into the journeys created at pickup-complete. Decides single-vs-fan-out; nothing downstream needs to know which. |
|
||||
| `controllers/cxOpsController.go` | 160 | §11 QA stage override, double-gated. |
|
||||
| `controllers/cxConsignmentHooks.go` | 100 | Maps a consignment status onto a customer stage for one order. |
|
||||
| `controllers/cxDeviceController.go` | 85 | §10 push registration. |
|
||||
| `controllers/cxIdentifiers.go` | 80 | Sequence-backed `DM-######` / `DMX########`. |
|
||||
| `utils/epoch.go` | 103 | IST→epoch conversion. **Read §1.3.** |
|
||||
| `utils/response_cx.go` | 97 | The customer envelope. Separate from `utils.OK/Fail` on purpose. |
|
||||
| `internal/sms/sms.go` | 105 | The seam for an SMS gateway. **No provider is wired in.** |
|
||||
| `middlewares/requestid.go` | 33 | `X-Request-Id` echo. |
|
||||
| `seed_customer_app.sql` | 153 | Staging data mirroring the client mock. |
|
||||
| `controllers/cxHttp_test.go` | 606 | HTTP status-code + envelope contract tests. |
|
||||
| `controllers/cxCustomerApp_test.go` | 473 | Pure-logic unit tests. |
|
||||
| `routes/routes_customer_test.go` | 323 | Routing + **the regression guard proving miler/console are untouched.** |
|
||||
| `internal/cxstage/stage_test.go` | 126 | Stage rollup and cancellation-window tests. |
|
||||
| `utils/epoch_test.go` | 116 | Timestamp conversion tests. |
|
||||
|
||||
### 3.2 Modified files (20)
|
||||
|
||||
| File | Δ | What changed | Why |
|
||||
|---|---|---|---|
|
||||
| `controllers/customerController.go` | +98 / −593 | PIN auth + single-destination booking handlers **deleted**; profile and locations kept, moved to the customer envelope | Replaced by the new surface; the old request shape is no longer a valid booking |
|
||||
| `controllers/milerController.go` | +593 | `BookingPickupComplete` rewritten for fan-out; `BookingParcelConfirm` accepts photos + writes transactionally; stage hooks added to accept/reached/cancel | §8.3 derivation and the §1 fan-out |
|
||||
| `controllers/milerAppController.go` | +410 | `MilerGetMyBookings` emits one stop per order after collection; `MilerDeliverConsignment` + `MilerStartDelivery` lookups fixed; stage hooks | **Without this, orders 2..N are undeliverable** — see §5.1 |
|
||||
| `routes/routes.go` | +82 | Customer routes replaced: 19 → 28 | The new contract |
|
||||
| `models/booking.go` | +60 | 10 columns on `PickupBooking`, 1 on `BookingParcel`, `Destinations` relation | Storage for the customer projection |
|
||||
| `constants/constants.go` | +71 | 9 stage keys, 3 statuses, 4 actor types, rank/cancellable helpers | The wire contract; never inline these |
|
||||
| `migrations/migrate.go` | +55 | 9 tables, 2 sequences, 2 indexes | Schema |
|
||||
| `internal/storage/spaces.go` | +74 | `PresignGet` | Signed parcel-photo URLs |
|
||||
| `dto/auth.go` | −41 | Retired customer PIN DTOs removed | Dead after the auth replacement |
|
||||
| `middlewares/logger.go` | +38 | Logs `client`, `platform`, `requestid` | §3 "accept and log" |
|
||||
| `middlewares/idempotency.go` | +34 | Anonymous-caller scoping | **Security fix** — see §5.4 |
|
||||
| `controllers/logisticsHandoverController.go` | +24 | Consignment→booking lookup fixed; `in_transit` recorded | Fan-out correctness |
|
||||
| `middlewares/city_gate.go` | +18 | Exported `PincodeInOperatingCity` | **Security fix** — see §5.3 |
|
||||
| `internal/assignment/crm_assignment.go` | +17 | `assigned` recorded on auto-assign | Most B2C bookings get a rider this way |
|
||||
| `controllers/booking_assignment_service.go` | +16 | `assigned` recorded on manual assign | Console assignment must reach the customer too |
|
||||
| `config/config.go` | +13 | `GEOCODER_URL`, `GEOCODER_EMAIL` | §6 proxy |
|
||||
| `utils/helper.go` | +13 | `GenerateTokenWithTTL` | 1h access tokens for customers only |
|
||||
| `main.go` | +7 | `RequestID()` registered | §3 |
|
||||
| `controllers/otpController.go` | −128 | **Deleted** | Its two routes were customer-only and are superseded |
|
||||
| `CLAUDE.md` | +147 | Project memory updated | Continuity |
|
||||
|
||||
---
|
||||
|
||||
## 4. Stage derivation (§8.3)
|
||||
|
||||
The operational writes already existed and were already correct. What did not
|
||||
exist was any record of *when* a parcel reached a stage in the vocabulary the
|
||||
customer sees. A consignment status says where a parcel is **now**; it cannot say
|
||||
when it got there.
|
||||
|
||||
| Existing Miler write | Customer stage produced |
|
||||
|---|---|
|
||||
| `assignMilerTx` / `commitAssignment` | `assigned` |
|
||||
| `POST /miler/assignments/{id}/accept` | `on_the_way` (re-asserts `assigned`; deduped) |
|
||||
| `POST /miler/bookings/{id}/reached` | `arrived` — **cancellation closes here**, recorded in the same transaction as the arrival fact so there is no window to cancel through |
|
||||
| `POST /miler/bookings/{id}/parcel` | populates `verification` (weight per destination + photos) |
|
||||
| `POST /miler/bookings/{id}/pickup-complete` | `picked_up`, then `order_created` **per destination** |
|
||||
| `POST /miler/consignments/{id}/inward-at-hub` | `in_transit`, per order |
|
||||
| `POST /miler/consignments/{id}/start-delivery` | `out_for_delivery`, per order |
|
||||
| `POST /miler/consignments/{id}/deliver` | `delivered`, per order |
|
||||
| `POST /miler/bookings/{id}/cancel` | **`Release`**, not cancel |
|
||||
|
||||
**Four rules worth knowing as the app developer:**
|
||||
|
||||
1. **Nothing is backfilled.** A booking that predates this work has a short
|
||||
history. A short honest history beats a long invented one — the customer
|
||||
cannot tell which entries were guessed.
|
||||
2. **A booking rolls up from its SLOWEST order.** Taking the maximum would show
|
||||
"Delivered" while one of three parcels was still at a hub.
|
||||
3. **`on_the_way` comes from accept, not the GPS stream.** Deriving it from
|
||||
location pings means re-deriving on every ping. Distance and ETA are still
|
||||
live from the rider's Redis position.
|
||||
4. **`Release` is the one place a stage moves backwards.** A miler cancelling
|
||||
returns the pickup to the pool. Without walking the stage back, the customer
|
||||
keeps seeing a rider card for someone who is not coming. The `assigned` /
|
||||
`on_the_way` history rows stay — those things happened.
|
||||
|
||||
### §8.4 gaps — all five closed
|
||||
|
||||
| Gap in the requirements | Status |
|
||||
|---|---|
|
||||
| No per-destination consignment id | ✅ `bookingdestinations.consignmentid` + `trackingno` |
|
||||
| No COD amount on the booking | ✅ `bookingdestinations.codamount` → `consignments.codamount`. **Per destination**, because COD is collected per delivery |
|
||||
| No per-stop attribution for stages 6–8 | ✅ Every per-order stage records against its destination |
|
||||
| No customer-side cancel | ✅ Server-enforced through `arrived` |
|
||||
| Parcel weight/photos not exposed | ✅ `verification` block, photos as signed URLs |
|
||||
|
||||
---
|
||||
|
||||
## 5. Bugs found and fixed (not requested — found because the fan-out forced every consignment lookup to be re-read)
|
||||
|
||||
### 5.1 🔴 The rider could never deliver orders 2..N
|
||||
|
||||
`GET /miler/bookings` emitted one row per booking, keyed on
|
||||
`pickupbookings.consignmentid` — a column that names only the **first** order.
|
||||
On a three-destination pickup, two parcels would sit in the rider's bag with no
|
||||
stop, no deliver button and no way to close them.
|
||||
|
||||
**Without this fix the entire multi-destination feature would have created orders
|
||||
nobody could deliver.** `milerStopsForBooking` now emits one stop per order after
|
||||
collection (and still exactly one visit before it — the rider goes to the door
|
||||
once). Tested in `controllers/cxCustomerApp_test.go`.
|
||||
|
||||
### 5.2 🔴 Three endpoints broken for orders 2..N (same root cause)
|
||||
|
||||
- **`MilerDeliverConsignment`** returned `404 "assigned consignment not found"` —
|
||||
the rider **could not complete the delivery at all**.
|
||||
- **`MilerStartDelivery`** sent no push and no receiver OTP.
|
||||
- **`MilerInwardConsignmentAtHub`** left the assignment open (so the rider could
|
||||
not go off duty — `MilerEndDuty` refuses on an open assignment) and recorded
|
||||
the leg's distance and earnings as **zero**.
|
||||
|
||||
All three now resolve through `cxDestinationForConsignment`.
|
||||
|
||||
### 5.3 🟠 `CityGateMiddleware` was a no-op for customer bookings
|
||||
|
||||
It sniffs the request body for `pickuppincode`. The new request shape does not
|
||||
carry one — its pickup is a `title/sub/lat/lng` from the place search — so
|
||||
**every customer booking sailed past the operating-city gate.** Now checked in
|
||||
the handler against the pincode resolved from the coordinates; a pickup outside
|
||||
an operating city returns `422 unserviceable`.
|
||||
|
||||
### 5.4 🔴 Idempotency key collision (introduced by me, caught in review)
|
||||
|
||||
Keys were namespaced by user id. On the **unauthenticated**
|
||||
`POST /customer/auth/otp/verify` that id is `0` — so two customers who happened
|
||||
to pick the same `Idempotency-Key` would collide and **the second would be handed
|
||||
the first's access token, refresh token and customer record.**
|
||||
|
||||
Fixed: anonymous callers are scoped by request path + body hash. The
|
||||
authenticated key format is left byte-identical, so no in-flight rider key is
|
||||
orphaned at deploy.
|
||||
|
||||
### 5.9 🟠 Fan-out row audit: three gaps closed
|
||||
|
||||
Found by auditing every row-level field the rider app reads, rather than
|
||||
assuming the fan-out was complete:
|
||||
|
||||
- **`collectionamt` was never emitted** — only `codamount`. Added alongside it,
|
||||
not as a rename, so the deployed build keeps working.
|
||||
- **COD fell back to 0** when a consignment row did not load, instead of to the
|
||||
destination's own figure. A rider shown ₹0 collects nothing.
|
||||
- **Stop ordering depended on the caller's `ORDER BY`.** Now guaranteed inside
|
||||
`milerStopsForBooking`, because the ordering is the route.
|
||||
|
||||
Also hardened at the same time: `CreateCxBooking` now rejects an absurd
|
||||
destination count **before** any database work (`cxAbsoluteMaxDestinations`),
|
||||
because the district lookup built a `WHERE districtcode IN (...)` from unbounded
|
||||
caller input; and `cxDefaultMaxDestinations` dropped from 5 to **1**, so a
|
||||
missing configuration row resolves to the safest cap rather than the most
|
||||
permissive — a gate that opens when its config is absent is not a gate.
|
||||
|
||||
### 5.10 🟠 Two different mistakes shared one error message
|
||||
|
||||
`POST /customer/bookings` answered an **empty** `destinations` array with
|
||||
*"Every destination needs a serviceable state and district"* — the same string
|
||||
it uses when a destination is present but missing its state or district.
|
||||
|
||||
The contract renders `message` verbatim, so a customer who had added nothing was
|
||||
told to go and fix details on destinations they did not have. The message
|
||||
described a problem they did not have and hid the one they did. Both cases also
|
||||
carried the same `error.code`, so the app could not tell them apart either.
|
||||
|
||||
Now *"Add at least one destination"*, matching what `/customer/fare/estimate`
|
||||
already said for the identical mistake. `TestEmptyDestinationsSaysAddOneNotFixTheirDetails`
|
||||
asserts the two endpoints agree and that the message names the actual fix.
|
||||
|
||||
Found by reading the messages rather than the status codes — every one of them
|
||||
is customer-facing copy, and a 400 being *correct* says nothing about whether it
|
||||
is *useful*.
|
||||
|
||||
### 5.5 🟡 Pre-existing, flagged not fixed
|
||||
|
||||
`BookingPickupComplete`'s comment says a hyperlocal parcel keeps the rider
|
||||
`Picked_Up`; the code sets them `Available`. This is the default path today.
|
||||
Changing it alters live rider availability and the dispatch pool — **ops
|
||||
decision.**
|
||||
|
||||
---
|
||||
|
||||
### 5.6 🔴 Rider earnings would have read ₹0 on every customer job (introduced by me, caught while mapping screens)
|
||||
|
||||
`CreateCxBooking` stored the estimate in its own new columns and did **not**
|
||||
create a `BookingServiceOption` row — but three places still read one:
|
||||
|
||||
- `MilerDeliverConsignment` and `MilerInwardConsignmentAtHub` copy
|
||||
`Estimatedprice` onto `BookingAssignment.ridercharges` when a leg closes.
|
||||
- `GET /miler/earnings` sums that column.
|
||||
- The admin console's Orders list renders `serviceoptions[0].estimatedprice` as
|
||||
its **Price** column.
|
||||
|
||||
So every customer-app job a rider completed would have recorded **zero earnings**
|
||||
on their Earnings screen, and the admin Orders list would have shown **"N/A"**
|
||||
for the price of every customer booking. Fixed: the booking now creates a service
|
||||
option carrying the midpoint of the band the customer was shown, which is what
|
||||
`lookupDoormilePrice` returns everywhere else in this codebase.
|
||||
|
||||
### 5.7 🔴 An ops cancellation never reached the customer
|
||||
|
||||
`AdminCancelBooking`, `AdminBulkCancelBookings` and `AdminUpdateBookingStatus`
|
||||
all cancel by writing `pickupbookings.status` directly. None of them knows the
|
||||
customer projection exists — and because `customerstatus` is written as
|
||||
`"active"` at booking time, the projection's empty-string fallback never fired.
|
||||
|
||||
**A customer whose pickup ops cancelled would have kept seeing it as active and
|
||||
cancellable, indefinitely.** They would also still have received stage
|
||||
notifications for it.
|
||||
|
||||
Fixed in two places, deliberately: the projection now treats the **operational**
|
||||
status as the authority on cancellation, which covers all three paths and any
|
||||
added later; and the two real cancel paths call `cxstage.Cancel` so the customer
|
||||
gets a reason and the audit trail records which ops user did it. Both are no-ops
|
||||
for console-created bookings.
|
||||
|
||||
### 5.8 🟠 An expired slot was reported as a full one
|
||||
|
||||
`CreateCxBooking` answered a slot whose window had already passed with
|
||||
`409 "That pickup window just filled up"`. That is untrue and points the
|
||||
customer at the wrong recovery — they need to re-fetch the slot list, not retry
|
||||
for a place in a queue. A client that caches slots for a session and is left open
|
||||
across midnight hits this on the first booking of the day.
|
||||
|
||||
Now `400 invalid` with *"That pickup time has passed — pick a new slot"*, which
|
||||
is what the contract already maps to "Pick a pickup slot". A stale **date** is
|
||||
also now caught by `CxSlotDateIsPast` before any database access, since the slot
|
||||
id carries its own date — the cheapest validation in the path, and the one that
|
||||
actually fires. `409` is reserved for the genuine capacity race.
|
||||
|
||||
Found by a reviewer noticing that §5.3 and §7.7 of this document could not both
|
||||
be comfortable at once. They were right.
|
||||
|
||||
## 6. Tests
|
||||
|
||||
`go build ./...`, `go vet ./...`, `go test ./...` all pass.
|
||||
**305 passing assertions/subtests**, all runnable with no database.
|
||||
|
||||
| File | What it proves |
|
||||
|---|---|
|
||||
| `controllers/cxHttp_test.go` | **Status codes and envelopes through a real router.** Every validation path answers 4xx with a machine-readable `error.code` and customer-safe English — never 500, never a router 404. Malformed JSON is 400. All 9 contract error codes map to the right status. `CxInternal` never leaks the cause. The QA override is invisible unless both gates are open. |
|
||||
| `routes/routes_customer_test.go` | All **20** authenticated routes exist and return **401 without a token** (not 404, not 500). All refuse roles 1/3/4/5/6 with **403**. Pre-auth routes are reachable and answer from the handler. Retired PIN routes no longer mint a credential. **`TestMilerSurfaceIsUnchanged` / `TestConsoleSurfaceIsUnchanged` are the regression guard for §0.** |
|
||||
| `controllers/cxCustomerApp_test.go` | Phone normalisation (7 spellings → one stored value, so a build change cannot create a duplicate account); the fan-out stop split; timeline dedup and per-order timestamping; `deliveredAt` waiting for the last parcel; pickup title/sub never empty. |
|
||||
| `internal/cxstage/stage_test.go` | Slowest-order rollup; stage ordering as a wire contract; the cancellation window closing exactly after `arrived`. |
|
||||
| `utils/epoch_test.go` | IST→epoch for both driver taggings; null stays null; slot window and day formatting. |
|
||||
|
||||
### What the tests do NOT prove
|
||||
|
||||
**No happy path is asserted.** A `200` from `CreateCxBooking` needs Postgres,
|
||||
Redis and NATS. Mocking them would test the mock. Everything from the handler
|
||||
inwards is still unverified — that is the integration pass in §11.
|
||||
|
||||
---
|
||||
|
||||
## 7. Answers to the requirements' §13 open questions
|
||||
|
||||
*(“§13” here and in §9 means section 13 of the requirements document. Section 13
|
||||
of THIS document, at the bottom, is the review log.)*
|
||||
|
||||
**1. Payment.** v1 is cash/UPI at the door, settled outside the app. No payment
|
||||
step exists and there is no payment contract to hand you. `amountPaid` is the sum
|
||||
of `bookingpayments` rows the miler recorded, in whole rupees, present from
|
||||
`picked_up`. Two different pots of money: the **pickup fee** is Doormile's,
|
||||
collected once per visit and recorded against the first order; **COD at the
|
||||
destination** is the customer's own collection, per destination. Doormile is the
|
||||
carrier, never the seller. ⚠️ *No settlement/remittance flow exists — money can be
|
||||
collected and recorded, nothing pays it back to the customer.*
|
||||
|
||||
**2. Masked calling.** Implemented as a switch: `MILER_CALL_PROXY` returns a proxy
|
||||
when set, the rider's real number otherwise. **Recommendation: set it before
|
||||
launch.** A customer handed a rider's personal mobile has it permanently.
|
||||
⚠️ *Currently unset.*
|
||||
|
||||
**3. Partial pickup.** Not modelled, not guessed at. Pickup-complete is
|
||||
all-or-nothing. The fan-out makes it *expressible* (legs are built per
|
||||
destination), but there is no stage key, no miler endpoint and no screen.
|
||||
⚠️ *Product decision needed.*
|
||||
|
||||
**4. Failed delivery / reattempt.** Exists operationally —
|
||||
`POST /miler/consignments/{id}/skip` increments `attemptcount` and leaves the
|
||||
parcel `Out_for_Delivery`. **Deliberately not mapped to a customer stage**: there
|
||||
is no key for it in the nine, and an unknown key renders as `booked` on the
|
||||
client. So today a failed attempt is **invisible to the customer** — their parcel
|
||||
stays "Out for delivery". ⚠️ *This is the most customer-visible gap in v1. Needs a
|
||||
tenth stage key + a client release.*
|
||||
|
||||
**5. Saved addresses / notification preferences / payment methods.** Saved
|
||||
addresses **built** (capped at 10, same `title`/`sub` shape as a place search
|
||||
result). Notification preferences **not built**. Payment methods **not built**,
|
||||
correctly, while v1 settles at the door.
|
||||
|
||||
**6. Support.** Not built for the customer. `/miler/support` exists and the shape
|
||||
would extend. ⚠️ *A phone number is the cheapest thing that is not a dead end.*
|
||||
|
||||
**7. Slot capacity semantics.** **Per zone.** A window's remaining capacity counts
|
||||
live bookings whose `preferredpickupfrom` falls inside it, within 12km of the
|
||||
pickup point, excluding cancelled and completed. The list read is advisory and
|
||||
cached 30s; capacity is **re-checked at confirm** and a lost race returns `409`.
|
||||
**The client does not need to re-fetch before confirming.**
|
||||
|
||||
**8. Pricing.** Reuses the existing `doormile_pricing` slab (zone × service type ×
|
||||
weight band → min/max), with a distance-and-weight fallback for unpriced lanes.
|
||||
Additional destinations carry a **+35% uplift each**. Weight assumed 3kg/package
|
||||
until weighed. ⚠️ *The 35% is a placeholder, and there is **no breakdown when
|
||||
`amountPaid` exceeds `fare.max`** — the receipt can show `amountPaid − fare.min`
|
||||
as a weight adjustment, but nothing explains an overage. Product owns both.*
|
||||
|
||||
**9. Cancellation fee.** **Confirmed free through `arrived`.** No fee is computed,
|
||||
charged or recorded anywhere in the cancel path. The UI copy is accurate. From
|
||||
`picked_up` onward the server returns `409`.
|
||||
|
||||
**10. Retention.** **Not implemented — no policy exists to implement.** Parcel
|
||||
photos stored indefinitely, PII stored indefinitely, `bookingstageevents` never
|
||||
pruned. The 30-minute signed-URL TTL limits *link* lifetime, not object lifetime.
|
||||
⚠️ *This is the answer most likely to matter to a regulator and it is currently
|
||||
blank.*
|
||||
|
||||
**11. Tenancy.** **Logistics only in v1.** Customer JWTs carry `tenantid: 0` and
|
||||
B2C bookings leave `pickupbookings.tenantid` null. The customer app does not
|
||||
switch mode on `tenantid` the way the Miler app does. ⚠️ *Decide before revenue
|
||||
reporting depends on it.*
|
||||
|
||||
---
|
||||
|
||||
## 8. Non-functional (§3.5)
|
||||
|
||||
- **Idempotency** — `POST /customer/bookings` and `POST /customer/auth/otp/verify`.
|
||||
24h replay.
|
||||
- **Caching** — serviceability `ETag`/`If-None-Match` → 304, `max-age=300`. Slots
|
||||
`max-age=30`. Booking detail supports 304 (it is polled).
|
||||
- **Rate limits** — OTP ≤5 per identifier per hour, ≤3 verify attempts per code,
|
||||
30s resend cooldown that does **not** consume one of the five. Plus the shared
|
||||
10/minute per-IP credential budget.
|
||||
- **Pagination** — `?limit=&cursor=`, default 20, max 50. **Keyset**, so a new
|
||||
booking landing mid-scroll cannot show the same row twice. `total` reflects the
|
||||
filtered tab.
|
||||
- **PII** — phone masked in SMS logs; parcel photos as signed URLs.
|
||||
- **Audit** — actor type, actor id, source endpoint and timestamp on every
|
||||
transition.
|
||||
- **Tenancy** — every customer read is scoped by `appcustomerid` in the query
|
||||
itself, never by a check afterwards.
|
||||
- ⚠️ **Latency is UNMEASURED.** Reads are batched (`loadCxBundle` is a fixed query
|
||||
count regardless of page size) and pricing is Redis-warmed — but p95 ≤ 400ms is
|
||||
an argument, not a measurement.
|
||||
|
||||
---
|
||||
|
||||
## 9. Deliverables (§11)
|
||||
|
||||
| Asked for | Status |
|
||||
|---|---|
|
||||
| OpenAPI 3.1 spec | ✅ `openapi-customer.yaml` — 24 paths, 28 operations, validated. Covers every registered route including profile and saved addresses. |
|
||||
| Postman collection | ❌ Not produced. The OpenAPI file imports directly into Postman. |
|
||||
| Staging seed mirroring the mock | ✅ TN/KL/KA open, PY serviceable with no open district, Madurai/Kozhikode/Mangaluru unavailable with reasons, slot `t5` at capacity 0 |
|
||||
| Test accounts + fixed staging OTP | ✅ Two accounts seeded; `CX_STAGING_OTP=1234`, refused when `ENV=production` |
|
||||
| Force a booking to any stage | ✅ Double-gated; walks every intermediate stage |
|
||||
| Documented error responses | ✅ Implemented verbatim; asserted in tests |
|
||||
| Error injection | ❌ Not built. Happy-path states are forceable; failure states are not. |
|
||||
| Written answer to the requirements' §13 | ✅ §7 above |
|
||||
|
||||
---
|
||||
|
||||
## 10. Environment variables
|
||||
|
||||
```bash
|
||||
GEOCODER_URL=https://nominatim.openstreetmap.org # place search / reverse geocode upstream
|
||||
GEOCODER_EMAIL=ops@doormile.com # Nominatim policy requires a contact
|
||||
MILER_CALL_PROXY= # ⚠️ empty exposes the rider's real number
|
||||
CX_STAGING_OTP=1234 # refused when ENV=production
|
||||
CX_ID_SCRAMBLE_KEY= # keys the identifier permutation; a working default ships
|
||||
CX_ALLOW_STAGE_OVERRIDE=false # also requires ENV != production
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 11. Before this goes to production — read this
|
||||
|
||||
**Blockers, stated plainly:**
|
||||
|
||||
1. 🔴 **No SMS provider exists.** `internal/sms` is the seam (a `Sender`
|
||||
interface, a logging sink, `sms.Register()`). Until a gateway is plugged in,
|
||||
**OTP codes go to the application log and nowhere else.** Real customer
|
||||
sign-in is blocked on this. It is the same wall the previous end-to-end test
|
||||
hit. `sms.Configured()` reports it.
|
||||
2. 🔴 **No integration test has touched any of this.** The 305 passing tests cover
|
||||
pure logic, routing, status codes and envelopes. Nothing has run against a
|
||||
real Postgres, Redis or NATS. **A clean compile says the code is well-formed,
|
||||
not that it works.**
|
||||
3. 🟠 **The migration has not run against a real database.** It is additive
|
||||
(`AutoMigrate` + `CREATE SEQUENCE`/`CREATE INDEX IF NOT EXISTS`) and Postgres
|
||||
11+ adds nullable/defaulted columns without a table rewrite — but *should be
|
||||
safe* is not *has been observed*. Run it on staging first and watch
|
||||
`pickupbookings`.
|
||||
4. 🟠 **`MILER_CALL_PROXY` is unset**, so the tracking screen will show riders'
|
||||
personal mobile numbers.
|
||||
5. 🟠 **No retention policy** for parcel photos or PII.
|
||||
6. 🟡 **Failed delivery is invisible to the customer** (§7.4).
|
||||
7. 🟡 **Latency unmeasured** (§8).
|
||||
8. 🔴 **Multi-destination pickups must stay capped at 1 destination** until a
|
||||
rider build keying on `consignmentid` ships (§0.6). The cap is a database
|
||||
value, not code — verify it, do not assume it.
|
||||
|
||||
**Suggested deploy order:**
|
||||
|
||||
1. Staging: deploy, let `AutoMigrate` run, apply `seed_customer_app.sql`, set
|
||||
`CX_STAGING_OTP=1234`.
|
||||
2. **Confirm `maxdestinations = 1`** in `customerbookinglimits` (the seed sets
|
||||
it). This is the gate that keeps the fan-out dormant until the rider app can
|
||||
handle it — see §0.6. Nothing else prevents a stranded parcel.
|
||||
|
||||
```sql
|
||||
SELECT applocationid, maxpackages, maxdestinations FROM customerbookinglimits;
|
||||
```
|
||||
3. Smoke the customer surface end to end — this is the pass that has never
|
||||
happened.
|
||||
4. **Regression-test the Miler app against staging**, specifically: pickup-complete
|
||||
on a single-destination booking, deliver, inward-at-hub, and off-duty. Those
|
||||
are the paths §0 items 3–5 and §5.2 touch.
|
||||
5. Plug in the SMS gateway before any production customer traffic.
|
||||
6. **Only after a rider build keying on `consignmentid` is live**, raise the cap
|
||||
and test a multi-destination pickup — the genuinely new path:
|
||||
|
||||
```sql
|
||||
UPDATE customerbookinglimits SET maxdestinations = 5 WHERE applocationid IS NULL;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 11.5 Client-side prerequisites — none of §12 works until these land
|
||||
|
||||
§12 below is a correct set of instructions for a client that can make requests.
|
||||
Today it cannot, and these are outstanding on the app side (all four are in
|
||||
Appendix B of the requirements, and this document should have carried them
|
||||
forward rather than assuming them done):
|
||||
|
||||
1. **`pubspec.yaml` has no `http` and no `shared_preferences`.** Every rule about
|
||||
headers, idempotency keys and token rotation presumes a network layer that is
|
||||
not yet added.
|
||||
2. **`Booking.fromJson` parses roughly a third of §9.3.** See the §9.6 row above.
|
||||
3. **`PickupSlot.fromJson` drops `tag`, `milersNearby` and `caption`.** The
|
||||
server sends all three; the picker will not show any of them.
|
||||
4. **`AppState.loadSlots` caches for the whole session and `startBooking` clears
|
||||
only `districtCache`** (`lib/state/app_state.dart:89`). An app left open
|
||||
across midnight books against yesterday's slot. The server now answers that
|
||||
with `400` and *"That pickup time has passed — pick a new slot"* rather than
|
||||
the misleading "just filled up" it used to (§5.8) — but the client fix is to
|
||||
clear `slotsCache` in `startBooking` so the customer never reaches that error.
|
||||
|
||||
---
|
||||
|
||||
## 12. Quick reference for the app developer
|
||||
|
||||
- **Base:** `https://api.doormile.com/api/v1`
|
||||
- **Auth:** `Authorization: Bearer <accessToken>` on everything except
|
||||
`/customer/auth/otp/request`, `/auth/signup`, `/auth/otp/verify`,
|
||||
`/auth/refresh`, and the four catalogue reads.
|
||||
- **Access token 1h, refresh 60 days, rotated on every use.** A revoked refresh
|
||||
token replayed kills the whole chain — always store the newest one.
|
||||
- **Send `X-Client: doormile-cx/<version>+<build>` and `X-Platform: android|ios`.**
|
||||
They are logged and are the only way to tell one build's failures from another's.
|
||||
- **Send `Idempotency-Key` on booking create and OTP verify.**
|
||||
- **Every timestamp is epoch milliseconds, UTC, integer.**
|
||||
- **Every string in `message` is safe to render verbatim.** Branch on
|
||||
`error.code`, never on the message text.
|
||||
- **`pickup`, `slotId`, `destinations[].stateName` and `districtName` are never
|
||||
null.** The rest of `details` may be absent — that is the "Not added" state, not
|
||||
an error.
|
||||
- **`trackingId` and `stage` on a destination are null until `order_created`.**
|
||||
There is no order to track before the parcels are collected.
|
||||
- **An unknown stage key renders as `booked`.** If you see that on a moving
|
||||
parcel, the backend sent a key this build does not know — tell us; it means a
|
||||
new stage shipped without a client release.
|
||||
|
||||
---
|
||||
|
||||
## 13. Review log
|
||||
|
||||
This document has been reviewed once, by a reviewer with access to the two
|
||||
Flutter apps but **not** to this Go backend. That split matters and is why the
|
||||
review was useful: every claim it made about the apps was checkable by them and
|
||||
not by me, and every claim about this repo was checkable by me and not by them.
|
||||
The findings below are recorded verbatim in substance, with what was done about
|
||||
each.
|
||||
|
||||
**Standing caveat the reviewer stated and this document repeats:** the 305 tests,
|
||||
the fan-out, the idempotency fix and the epoch conversion are unverified from
|
||||
their side. Nothing in §11 has changed — this code has still never run against a
|
||||
real database.
|
||||
|
||||
### 13.1 Findings — all upheld
|
||||
|
||||
| # | Finding | Status | What changed |
|
||||
|---|---|---|---|
|
||||
| 1 | **The rider-app action item is materially understated.** The app's identity key is `orderid` (`api_config.dart:321`, `'orderid': ref ?? id`), not `bookingid`. Both are booking-level, so all three fan-out rows collapse — in `accepted_store.dart:527` (SharedPreferences-backed, survives restart) and in `work_repository.dart:314,321` / `order_events.dart`. Keying on `consignmentid` touches four files. | **Upheld.** Consistent with what this server sends: the three rows share `bookingid` *and* `bookingreference`. | New **§0.6** replaces the one-line framing. §0 item 3 now points at it. Mitigation added: `maxdestinations` seeded at **1**, and a deploy-checklist step to verify it. |
|
||||
| 2 | `destinationseq` / `destinationcount` are invisible — `pickupFromBooking` builds a fixed map, so a field the adapter does not name cannot reach the app. | **Upheld.** | Recorded in §0.6. Adding fields server-side does not help until the adapter names them. |
|
||||
| 3 | **"Reuse `Booking.fromJson`" is optimistic.** It parses only `reference`, `stage`, `cancellable`, `createdAt`, `pickup`, `slotId` and four destination fields — ignoring `status`, `miler`, `deliveryAgent`, `fare`, `amountPaid`, `deliveredAt`, `history`, `verification`, `details`, per-destination `stage`, `routeKm`, `expectedDelivery`. | **Upheld, and it was avoidable** — Appendix B of the requirements said exactly this and this document glossed it. | §1.2's §9.6 row rewritten to state the gap instead of assuming the parser. |
|
||||
| 4 | **Slot staleness is a trap handed to the client.** §5.3 and §7.7 could not both be comfortable: `AppState.loadSlots` caches for a session, `startBooking` clears only `districtCache` (`app_state.dart:89`), so an app open across midnight books yesterday's slot. | **Upheld — and it exposed a server defect.** The server answered `409 "That pickup window just filled up"`, which is untrue and points at the wrong recovery. | **Code fixed** (§5.8): now `400` with *"That pickup time has passed — pick a new slot"*, plus a new DB-free `CxSlotDateIsPast` guard that runs before any query. Test added. Client-side fix (clear `slotsCache`) recorded in §11.5. |
|
||||
| 5 | §12's header/idempotency/rotation rules assume a client that can make requests; `pubspec.yaml` has no `http`, no `shared_preferences`. | **Upheld.** | New **§11.5** lists the client prerequisites ahead of §12. |
|
||||
| 6 | `PickupSlot.fromJson` drops `tag`, `milersNearby`, `caption` — the server now sends all three. | **Upheld.** | §11.5 item 3. |
|
||||
| 7 | §3.2 contains a literal unfilled placeholder: `customerController.go │ −691/+?`. | **Upheld.** | Corrected to the real churn, **+98 / −593**. |
|
||||
| 8 | The counts do not reconcile: 19→28 routes, 8 exempt from auth ⇒ 20 authenticated, but §6 asserts 22. | **Upheld.** Verified: 28 registered = 8 pre-auth + **20** authenticated; the routing test has exactly 20 entries. | §6 corrected to 20. §1.1 now states the arithmetic explicitly so the three numbers reconcile on the page. |
|
||||
| 9 | The OpenAPI file's 22 operations imply `/customer/profile` and `/customer/locations` are absent, though §1.1 relies on them to answer §13.5. | **Upheld, and the worst of the three** — the app developer works from the spec and would not have found saved addresses. | **6 endpoints added to the spec** with `SavedAddress` / `SavedAddressInput` schemas. Now **24 paths / 28 operations**, matching the 28 registered routes exactly. |
|
||||
|
||||
### 13.2 What the review confirmed as correct
|
||||
|
||||
Recorded because it is the half that did not change, and it is the half the app
|
||||
is built against:
|
||||
|
||||
- camelCase throughout — no mapping layer needed on the client.
|
||||
- Epoch-millis UTC — `DateTime.fromMillisecondsSinceEpoch` works as-is.
|
||||
- Payload always in `data`, auth included.
|
||||
- `pickup`, `slotId`, `stateName`, `districtName` non-null — the client types
|
||||
these non-nullable and would throw otherwise.
|
||||
- "An unknown stage key renders as `booked`" is accurate against `models.dart`
|
||||
(`JourneyStage.fromKey` falls back to `booked`).
|
||||
- The §1.3 IST timestamp defect is real and is the bug that already bit the
|
||||
Miler app.
|
||||
- §8.4's five gaps match independently.
|
||||
- The two reverts — rider availability, and the 404→403 change — were the right
|
||||
call.
|
||||
- §11's honesty about what is unverified was called the most useful section here.
|
||||
It has not been softened.
|
||||
|
||||
### 13.3 Reviewer's verdict, unedited in substance
|
||||
|
||||
> The backend contract is sound and answers the requirements; the impact
|
||||
> assessment is accurate in direction but under-scopes the rider-app work, and
|
||||
> its "reuse the existing parser" claims describe the client we intend to have,
|
||||
> not the one in the repo. Nothing here contradicts the requirements doc.
|
||||
|
||||
Both criticisms are now addressed in the text: §0.6 for the rider-app scope,
|
||||
§11.5 and the §9.6 row for the parser claims.
|
||||
|
||||
### 13.4 What still cannot be checked from either side
|
||||
|
||||
- Nobody has run this against a real Postgres, Redis or NATS.
|
||||
- The reviewer could not verify the Go changes; this document's author could not
|
||||
open the Flutter apps. **Nothing in §0.6 or §11.5 has been executed** — those
|
||||
file and line references come from the review, not from this repo.
|
||||
- The single integration pass in §11 remains the thing that would close both
|
||||
gaps at once.
|
||||
Reference in New Issue
Block a user