Skip to content

Commit e605a19

Browse files
Banyel3claude
andcommitted
fix: address the per-slice review findings (2 P2 + input/robustness P3s)
From the /review of the separated slice PRs. (The NODE_ENV, remittance-double-pay, and idempotency-scope findings the reviewers raised were on frozen historical slices — already fixed at HEAD.) P2: - assignRider (manual admin dispatch) now gates on a VERIFIED rider profile, not just the RIDER role — parity with auto-dispatch, so a draft/rejected rider who hasn't proven license/ID is never handed platform cash. (repo.isRiderVerified) - zones.isCovered fails CLOSED when zones exist but all are deactivated (was falling back to the full pilot ring, silently reopening coverage). Distinguishes empty-table (bootstrap → pilot ring) from all-off (admin closed → no coverage). P3: - orders ?status query validated via ParseEnumPipe (garbage → 400, not Prisma 500). - onboarding proof keys (shop permit/photos, rider license/id) must live under the caller's uploads/<uid>/ prefix — a crafted key can no longer point the admin's presigned-GET at another user's private object. (shared assertOwnedKey) - rider backfill migration: existing RIDER users get a VERIFIED profile (idempotent) so the new dispatch gate doesn't strand pre-existing riders. - shop-facing remittance strips paidByUid (an admin's Firebase UID). - admin-shops addService/addMember map the concurrent-dup P2002 to 409, not 500. - /geocode/search throttled 20/min (billable TomTom call, was global-only). - platform-config envNum accepts a deliberate 0 (presence check, not !== 0). Noted, not fixed (dev-only / negligible): auth bearer-before-bypass dev 500, capacity-race returns 409, pagination cursor positional side-channel. 290 api unit tests. Integration + backfill migration validated by CI (local Docker unavailable this session). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent db104e8 commit e605a19

16 files changed

Lines changed: 118 additions & 21 deletions
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
-- Backfill: any pre-existing RIDER user without a profile was admin-provisioned
2+
-- and trusted, so give them a VERIFIED profile — otherwise the onboarding-D
3+
-- dispatch gate (riderProfile status VERIFIED) would make them permanently
4+
-- undispatchable. Idempotent (NOT EXISTS); a no-op on a DB with no such users.
5+
INSERT INTO "RiderProfile" ("id", "userId", "status", "verifiedAt", "createdAt")
6+
SELECT gen_random_uuid()::text, u."id", 'VERIFIED', now(), now()
7+
FROM "User" u
8+
WHERE 'RIDER' = ANY(u."roles")
9+
AND NOT EXISTS (SELECT 1 FROM "RiderProfile" p WHERE p."userId" = u."id");

apps/api/src/maps/geocode.controller.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
UseGuards,
88
} from '@nestjs/common';
99
import { ApiBearerAuth, ApiOperation, ApiTags } from '@nestjs/swagger';
10+
import { Throttle } from '@nestjs/throttler';
1011
import type { GeocodeResult, MapsProvider } from '@wash-and-go/maps';
1112
import { RolesGuard } from '../common/guards/roles.guard';
1213
import { MAPS_PROVIDER } from './maps.constants';
@@ -38,6 +39,9 @@ export class GeocodeController {
3839

3940
// Typeahead: up to `limit` ranked candidates for an address autocomplete
4041
// (admin shop editor). Empty array on no match. Any-authenticated like geocode.
42+
// Each call is a billed TomTom request — cap it tighter than the global 60/min
43+
// so any authenticated token can't burn the maps quota via the typeahead.
44+
@Throttle({ default: { limit: 20, ttl: 60_000 } })
4145
@Get('search')
4246
@ApiOperation({ summary: 'Address autocomplete — ranked candidates' })
4347
async search(

apps/api/src/orders/orders.controller.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
Get,
55
Headers,
66
Param,
7+
ParseEnumPipe,
78
Post,
89
Query,
910
UseGuards,
@@ -138,7 +139,9 @@ export class OrdersController {
138139
@ApiOperation({ summary: 'List orders visible to the caller (paged, newest first)' })
139140
list(
140141
@CurrentUser() user: User,
141-
@Query('status') status?: OrderStatus,
142+
// Validate the enum so a garbage ?status=foo is a 400, not a Prisma 500.
143+
@Query('status', new ParseEnumPipe(OrderStatus, { optional: true }))
144+
status?: OrderStatus,
142145
@Query('q') q?: string,
143146
@Query('limit') limit?: string,
144147
@Query('before') before?: string,

apps/api/src/orders/orders.repository.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,13 @@ export class OrdersRepository {
133133
return u !== null && !u.disabledAt && u.roles.includes(role as never);
134134
}
135135

136+
// A rider is dispatchable only with a VERIFIED profile (onboarding D). Manual
137+
// admin assign gates on this too, not just auto-dispatch.
138+
async isRiderVerified(userId: string): Promise<boolean> {
139+
const p = await this.prisma.riderProfile.findUnique({ where: { userId } });
140+
return p?.status === 'VERIFIED';
141+
}
142+
136143
// ── capacity path (all take tx) ─────────────────────────────────────────
137144

138145
// T1: serialize concurrent bookings per (shop, Manila-day). Transaction-scoped

apps/api/src/orders/orders.service.spec.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ describe('OrdersService', () => {
135135
countExpressUsedByShopForDay: jest.fn().mockResolvedValue(new Map()),
136136
isShopMember: jest.fn(),
137137
userHasRole: jest.fn(),
138+
isRiderVerified: jest.fn().mockResolvedValue(true),
138139
lockShopDay: jest.fn().mockResolvedValue(undefined),
139140
countExpressOrdersForShopDay: jest.fn(),
140141
pickAutoDispatchRider: jest.fn().mockResolvedValue(null),
@@ -180,6 +181,7 @@ describe('OrdersService', () => {
180181
// giving true coverage behavior (central ZC in, Manila out) without mocking.
181182
const zones = new ZonesService({
182183
findActive: async () => [],
184+
countAll: async () => 0, // empty table → pilot-ring fallback (in-coverage)
183185
} as unknown as ZonesRepository);
184186

185187
const notifications = {
@@ -550,6 +552,14 @@ describe('OrdersService', () => {
550552
).rejects.toBeInstanceOf(BadRequestException);
551553
});
552554

555+
it('rejects assigning an unverified rider (parity with auto-dispatch gate)', async () => {
556+
repo.userHasRole.mockResolvedValue(true);
557+
repo.isRiderVerified.mockResolvedValue(false);
558+
await expect(
559+
service.assignRider(makeUser(['ADMIN']), 'o1', { riderId: 'draft-rider' }),
560+
).rejects.toBeInstanceOf(BadRequestException);
561+
});
562+
553563
it('assigns and transitions BOOKED → ASSIGNED', async () => {
554564
repo.userHasRole.mockResolvedValue(true);
555565
repo.findByIdForUpdate.mockResolvedValue(makeOrder({ status: 'BOOKED' }));

apps/api/src/orders/orders.service.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,10 @@ export class OrdersService {
495495
): Promise<Order> {
496496
const riderOk = await this.repo.userHasRole(dto.riderId, 'RIDER');
497497
if (!riderOk) throw new BadRequestException('Assignee is not a rider');
498+
// Only VERIFIED riders are dispatchable — parity with auto-dispatch, so a
499+
// draft/rejected rider (no verified license/ID) is never handed platform cash.
500+
const verified = await this.repo.isRiderVerified(dto.riderId);
501+
if (!verified) throw new BadRequestException('Rider is not verified yet');
498502

499503
return this.prisma.$transaction(async (tx) => {
500504
const order = await this.repo.findByIdForUpdate(tx, orderId);

apps/api/src/platform-config/platform-config.service.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,12 @@ export class PlatformConfigService {
8383
) {}
8484

8585
private envNum(key: string, def: number): number {
86-
const v = Number(this.env.get<string>(key));
87-
return Number.isFinite(v) && v !== 0 ? v : def;
86+
// Presence check, not `!== 0` — a deliberate 0 (e.g. SERVICE_FEE_PHP=0) is a
87+
// valid override, not "unset". Missing/blank/garbage → the code default.
88+
const raw = this.env.get<string>(key);
89+
if (raw == null || String(raw).trim() === '') return def;
90+
const v = Number(raw);
91+
return Number.isFinite(v) ? v : def;
8892
}
8993

9094
// Bootstrap defaults (env-overridable) used only when the row does not exist.

apps/api/src/remittance/remittance.service.spec.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ describe('RemittanceService', () => {
6363
expect(repo.listBatches).toHaveBeenCalledWith({
6464
shopId: { in: ['shopA', 'shopB'] },
6565
});
66-
expect(out).toEqual([{ id: 'b1' }]);
66+
// paidByUid is stripped for the shop-facing response.
67+
expect(out).toEqual([{ id: 'b1', paidByUid: null }]);
6768
});
6869
});
6970

apps/api/src/remittance/remittance.service.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,14 @@ export class RemittanceService {
102102
return batch;
103103
}
104104

105-
// Shop-facing: only the caller's own shop payout batches.
105+
// Shop-facing: only the caller's own shop payout batches. Strip paidByUid — a
106+
// shop has no business seeing which admin's Firebase UID marked the transfer
107+
// (the reference + paidAt are enough proof of payment).
106108
async listBatchesForMember(userId: string): Promise<RemittanceBatch[]> {
107109
const shopIds = await this.repo.shopIdsForMember(userId);
108110
if (shopIds.length === 0) return [];
109-
return this.repo.listBatches({ shopId: { in: shopIds } });
111+
const batches = await this.repo.listBatches({ shopId: { in: shopIds } });
112+
return batches.map((b) => ({ ...b, paidByUid: null }));
110113
}
111114

112115
async listBatches(filter: {

apps/api/src/riders/rider-onboarding.service.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
} from '@nestjs/common';
77
import { RiderProfile, User } from '@prisma/client';
88
import { PrismaService } from '../prisma/prisma.service';
9+
import { assertOwnedKey } from '../uploads/object-key';
910
import type { UpdateRiderOnboardingDto } from './dto/rider-onboarding.dto';
1011

1112
// Rider's own onboarding view.
@@ -57,6 +58,9 @@ export class RiderOnboardingService {
5758
: 'Your profile is verified',
5859
);
5960
}
61+
if (dto.licenseKey != null) assertOwnedKey(user.id, dto.licenseKey);
62+
if (dto.idKey != null) assertOwnedKey(user.id, dto.idKey);
63+
6064
const updated = await this.prisma.riderProfile.update({
6165
where: { userId: user.id },
6266
data: {

0 commit comments

Comments
 (0)