mirror of
https://github.com/Tria-plc/edr-platform.git
synced 2026-08-26 18:42:49 +00:00
Adding all the tests and fixes to the passengers app
This commit is contained in:
Muluhabt
141
docs/SOLUTIONS.md
Normal file
141
docs/SOLUTIONS.md
Normal file
@@ -0,0 +1,141 @@
|
||||
# EDR Passenger — Solutions
|
||||
|
||||
Concrete fixes for the confirmed findings in `docs/ISSUES.md`. Ordered by priority. Each references
|
||||
the exact site and the intended change. Code sketches are illustrative, not drop-in patches.
|
||||
|
||||
**Test coverage backing these:** 28 automated tests (25 API `jest` + 3 UI Playwright) reproduce the
|
||||
✅ findings. Fix a finding → its 🔴 test flips from "bug present" to failing; update the test to
|
||||
assert the corrected behavior.
|
||||
|
||||
---
|
||||
|
||||
## P0 — deploy blockers (money creation/theft, broken booking)
|
||||
|
||||
### C-10 — Authenticated `POST /bookings` is broken
|
||||
`bookings.service.ts:773` resolve unconditionally; delete the UUID-format gate:
|
||||
```ts
|
||||
// BEFORE: resolves only when passengerId is NOT a UUID (never fires for real IAM ids)
|
||||
if (dto.passengerId && !dto.passengerId.match(/^[0-9a-f-]{36}$/i)) { … }
|
||||
// AFTER: always translate the authenticated identity → the Passenger.id
|
||||
if (dto.passengerId) {
|
||||
const passenger = await this.prisma.passenger.findUnique({
|
||||
where: { iamUserId: dto.passengerId }, select: { id: true },
|
||||
});
|
||||
if (passenger) dto = { ...dto, passengerId: passenger.id };
|
||||
// else: leave as-is only if it already IS a Passenger.id (guest/admin paths)
|
||||
}
|
||||
```
|
||||
Keep the controller's "don't trust the body" intent (`bookings.controller.ts:528`) — it's correct to
|
||||
take identity from the JWT; the service just has to map iamUserId → Passenger.id. Regression test:
|
||||
`test/authed-booking-passengerid.e2e-spec.ts`.
|
||||
|
||||
### C-1 — Booking total is client-controlled
|
||||
`bookings.service.ts:863-899`: stop trusting `reviewedTotalMinor`/`seatFareMinor`. Recompute the fare
|
||||
server-side and reject a mismatch:
|
||||
```ts
|
||||
const server = fareCalculation.totalMinor;
|
||||
if (dto.reviewedTotalMinor != null && Math.abs(dto.reviewedTotalMinor - server) > 1) {
|
||||
throw new BadRequestException('Price changed — please review the updated fare');
|
||||
}
|
||||
resolvedTotalMinor = server; // never persist a client amount as the charge basis
|
||||
```
|
||||
Apply the same to `guest-booking.service.ts:206-245`.
|
||||
|
||||
### C-2 — Loyalty redemption unbounded + never deducted
|
||||
`bookings.service.ts:1705` (and `:1028/:1249/:1451`): validate + debit inside the booking transaction:
|
||||
```ts
|
||||
const acct = await tx.loyaltyAccount.findUnique({ where: { passengerId } });
|
||||
const pts = Math.min(dto.loyaltyRedemptionPoints ?? 0, acct?.pointsBalance ?? 0, MAX_REDEEM);
|
||||
const loyaltyMinor = pts * POINTS_TO_MINOR;
|
||||
await tx.loyaltyLedgerEntry.create({ data: { accountId: acct.id, delta: -pts, reason: 'REDEEMED', balanceAfter: acct.pointsBalance - pts } });
|
||||
await tx.loyaltyAccount.update({ where: { id: acct.id }, data: { pointsBalance: { decrement: pts } } });
|
||||
```
|
||||
|
||||
### C-3 — Wallet top-up: no ownership, no backing
|
||||
- `wallet.controller.ts:34`: enforce `req.user` owns `:passengerId` (or is admin) before top-up.
|
||||
- `wallet.service.ts:50`: only credit after a confirmed `PaymentIntent` (a top-up is a purchase).
|
||||
- `wallet.controller.ts:23-24`: remove `@IsPublic()` from `GET /wallet/accounts`.
|
||||
|
||||
### C-4 — Payment amount never validated
|
||||
- `intents.service.ts:541-548`: on `confirmedAmountMinor !== intent.amountMinor`, do NOT mark
|
||||
SUCCEEDED — set a `AMOUNT_MISMATCH` state and alert. Populate `confirmedAmountMinor` in each
|
||||
webhook handler (e.g. `waafi-webhook.service.ts:63`).
|
||||
- `payments.service.ts:809` `finalizePaymentSuccess`: assert the settled amount equals
|
||||
`booking.totalMinor` before confirming.
|
||||
|
||||
### C-5 — Late webhook resurrects an expired/cancelled booking
|
||||
`payments.service.ts:809` `finalizePaymentSuccess`: refuse to confirm unless the booking is still
|
||||
`PENDING_PAYMENT`; route a late success to the refund/again-available flow:
|
||||
```ts
|
||||
if (booking.status !== 'PENDING_PAYMENT') { await this.refundLatePayment(intent); return { alreadyFinalized: true }; }
|
||||
```
|
||||
|
||||
### C-6 — Wallet double-spend (no row lock)
|
||||
`payments.service.ts:461-484`: lock the row or use an atomic conditional update:
|
||||
```ts
|
||||
const res = await tx.$executeRaw`UPDATE passenger."WalletAccount"
|
||||
SET "balanceMinor" = "balanceMinor" - ${total}
|
||||
WHERE "passengerId" = ${booking.passengerId} AND "balanceMinor" >= ${total}`;
|
||||
if (res === 0) return { success: false }; // insufficient / lost the race
|
||||
```
|
||||
Regression test: `critical-repro.e2e-spec.ts` (C-6, barrier-forced interleave).
|
||||
|
||||
### C-7 — Refund computed but never disbursed
|
||||
`bookings.service.ts:2017-2027`: on `booking.cancelled`, actually disburse — credit the wallet or call
|
||||
the provider refund — and drive `refundStatus PENDING → PROCESSING → COMPLETED`. Add a reconciliation
|
||||
sweep for stuck `PENDING` rows.
|
||||
|
||||
### C-8 — Exchange-rate writes missing the ADMIN check (any passenger can write FX)
|
||||
`fare-engine/currency.controller.ts:25,32`: add `@PassengerAdmin()` (+ `@ApiBearerAuth`) to the `PUT`
|
||||
and `PATCH` handlers, matching the already-guarded `DELETE`. (Not unauthenticated — the global
|
||||
JwtGuard requires a token; the gap is the missing *authorization*. Verified live: passenger → 200.)
|
||||
|
||||
### C-9 — `@Roles('ADMIN')` is dead
|
||||
Register the guard globally so `@Roles` is enforced:
|
||||
```ts
|
||||
// app.module.ts providers
|
||||
{ provide: APP_GUARD, useClass: RolesGuard }
|
||||
```
|
||||
…or convert `configurable-fare` / `segment-fare` / `system-config` controllers to the working
|
||||
`@PassengerAdmin()`/`@PassengerStaff()` guards.
|
||||
|
||||
---
|
||||
|
||||
## P1 — pricing correctness (HIGH)
|
||||
|
||||
- **H-1 promo → negative total** (`fare-engine.service.ts:192`): `totalEtbMinor = Math.max(0, subtotal - discount)`; DTO `@Min(0) @Max(100)` on `percentOff`, `@Min(0)` on `amountOffMinor` (`promos.dto.ts:20,26`).
|
||||
- **H-2 missing FX → 1.0** (`currency.service.ts:142`): remove the silent `return 1.0` — throw / block the quote so it fails closed.
|
||||
- **H-3/H-4 FX divergence & unit confusion** (`currency.service.ts`): collapse the 4 routines into one `convert(fromMinor, from, to): {minor|major}` with one rounding + one fallback policy; give it a branded `Minor`/`Major` return type and audit every `amountMinor` assignment across the payment boundary.
|
||||
- **H-5 `percentOff:0` treated as FIXED** (`fare-engine.service.ts:185`): use `promo.percentOff != null ? … : promo.amountOffMinor`.
|
||||
- **H-6 `insuranceFeeMinor` dual meaning** (`fare-engine.service.ts:130,154,167`): split into `insuranceMultiplierBps` and `insuranceFeeMinor`; use one consistently.
|
||||
- **H-7 domestic ETB fare × USD→ETB rate** (`fare-engine.service.ts:157`): don't apply a currency conversion to a domestic base fare — separate the minor-unit scaling from FX.
|
||||
- **H-8 baggage ignores seat class** (`excess-baggage.service.ts:53`): `findFirst({ where: { seatClassId } })`.
|
||||
- **H-9 baggage/supp skip conversion + DJF rounding** (`excess-baggage.service.ts:166`, `supplementary-charges.service.ts:132`): route through `convertMinorToChargeMajor`.
|
||||
- **H-10 future-dated FX applied now** (`currency.service.ts:88,137`): add `effectiveDate: { lte: new Date() }` to the rate lookups.
|
||||
- **H-11 non-deterministic fare resolution** (`fare-engine.service.ts:84,298`): add `orderBy: { validFrom: 'desc' }`; include `validFrom` in `SegmentFareRule`'s unique key (`schema.prisma:1113`) to allow dated versions.
|
||||
- **H-12 divergent free-child rules** (`fare-engine.service.ts:172` vs `bookings.service.ts:1690` vs `:1622`): extract ONE `computeFare()` used by quote, booking, and payment.
|
||||
- **H-13 promo silently dropped → overcharge** (`review/page.tsx:587`, `search.service.ts:940-970`): book the breakdown's discounted `totalMinor`, not the client-summed undiscounted per-pax fares — or return discounted per-pax fares. Fold into the C-1 fix (server recomputes the authoritative total incl. promo).
|
||||
|
||||
---
|
||||
|
||||
## P2 — config validation & safety (MEDIUM) / UI (LOW)
|
||||
|
||||
- **M-1 negative fares**: add `@Min(0)` to `baseFareMinor` (`schedules.dto.ts:85,95`) and `basePrice` (`seat-classes.dto.ts:29`).
|
||||
- **M-2 promo bounds/date**: `@Min(0) @Max(100)` on `percentOff`, `@IsDateString()` on `validUntil` (`promos.dto.ts`); add min-spend / usage-limit / max-cap columns.
|
||||
- **M-3 `PATCH /config` arbitrary**: replace the raw body with a whitelisted, typed DTO with per-key range checks (`system-config.controller.ts:34`).
|
||||
- **M-4 past-date / double-booked schedules** (`schedules.service.ts:105,124`): reject past `departureAt`; widen the overlap check to the train across all routes.
|
||||
- **M-5 deletes ignore references** (`stations`/`seat-classes`/`currencies`/`schedules` services): add referential guards before delete/disable; wrap the schedule cascade (`schedules.service.ts:438-485`) in a transaction.
|
||||
- **M-6 non-atomic booking write** (`bookings.service.ts:883-926`): wrap create + confirmSeats + tier increment in one `$transaction`.
|
||||
- **L-1 DJF shown with 2 decimals** (`portal/src/utils/format.ts:22`): format per `CHARGE_CURRENCY_DECIMALS` (DJF = 0).
|
||||
- **L-2 client-side fare math** (`portal/src/utils/fare-utils.ts`, `review/page.tsx`): render only server-computed amounts; never submit a client-derived total (ties to C-1).
|
||||
- **L-3 loyalty accrual currency** (`payments.service.ts:1062`): accrue from the actual charged amount/currency, not ETB minor.
|
||||
|
||||
---
|
||||
|
||||
## Suggested sequencing
|
||||
|
||||
1. **C-10, C-3, C-8, C-9** — quickest high-impact (a few lines each): unblock authenticated booking, stop free wallet credit, guard FX writes, enforce roles.
|
||||
2. **C-1, C-2, C-4, C-5, C-6, C-7** — the money-integrity core (needs transactions + validation).
|
||||
3. **H-2, H-3, H-4, H-7** — the FX/units foundation others compound on.
|
||||
4. **H-1, H-5, H-8..H-12, M-1, M-2** — pricing correctness + validation.
|
||||
5. **M-3..M-6, L-1..L-3** — config safety + UI consistency.
|
||||
Reference in New Issue
Block a user