fix: normalized the region and logged the otp properly

apps/edr-freight-api/src/modules/companies/dto/update-profile.dto.ts

@@ -1,4 +1,5 @@
-import { IsString, IsOptional, IsEmail, MaxLength, IsEnum } from 'class-validator';
+import { IsString, IsOptional, IsEmail, MaxLength, IsEnum, IsIn } from 'class-validator';
+import { ETHIOPIAN_REGIONS, type EthiopianRegion } from '@edr/types';
 import { CompanyNationality } from '../entities/company.entity';
 import { IsValidPhone } from '../../../common/validators/is-phone-number.validator';
 import { IsTin } from '../../../common/validators/is-tin.validator';
@@ -138,10 +139,14 @@ export class UpdateProfileDto {
   @MaxLength(50)
   renewedTo?: string;
 
+  // Zone/woreda/kebele below stay free text: there is no authoritative dataset
+  // of Ethiopian zones/woredas/kebeles in the platform yet, and eTrade returns
+  // them uncoded. Only region is a closed set today.
   @IsOptional()
-  @IsString()
-  @MaxLength(100)
-  region?: string;
+  @IsIn(ETHIOPIAN_REGIONS as unknown as string[], {
+    message: "region must be a recognised Ethiopian region",
+  })
+  region?: EthiopianRegion;
 
   @IsOptional()
   @IsString()

apps/edr-freight-api/src/modules/companies/services/etrade.service.ts

@@ -6,6 +6,7 @@ import {
   ETradeCompanyInfo,
   ETradeBusinessInfo,
   CompanyRegistrationData,
+  normalizeRegion,
 } from "@edr/types";
 
 @Injectable()
@@ -108,7 +109,11 @@ export class ETradeService {
       renewedFrom: businessInfo.RenewedFrom,
       renewalDate: businessInfo.RenewalDate,
       renewedTo: businessInfo.RenewedTo,
-      region: businessInfo.AddressInfo?.Region || "",
+      // eTrade returns uncoded uppercase text and sometimes a zone name in the
+      // Region slot. Map it onto the canonical list; an unresolved value yields
+      // "" so the form asks the user to pick rather than failing validation on
+      // save with a value they never typed.
+      region: normalizeRegion(businessInfo.AddressInfo?.Region) ?? "",
       zone: businessInfo.AddressInfo?.Zone || "",
       woreda: businessInfo.AddressInfo?.Woreda || "",
       kebele: businessInfo.AddressInfo?.Kebele || "",

apps/edr-freight-api/src/modules/companies/services/region-normalization.spec.ts

@@ -0,0 +1,54 @@
+import { ETHIOPIAN_REGIONS, normalizeRegion } from '@edr/types';
+
+/**
+ * normalizeRegion lives in @edr/types (no jest there), but it exists to keep
+ * eTrade autofill from feeding UpdateProfileDto a region its @IsIn will reject.
+ * That contract is an API concern, so it is guarded here.
+ */
+describe('normalizeRegion', () => {
+  it('passes through every canonical region unchanged', () => {
+    for (const region of ETHIOPIAN_REGIONS) {
+      expect(normalizeRegion(region)).toBe(region);
+    }
+  });
+
+  it.each([
+    ['ADDIS ABABA', 'Addis Ababa'],
+    ['Addis ababa', 'Addis Ababa'],
+    ['  addis   ababa  ', 'Addis Ababa'],
+    ['oromoia', 'Oromia'],
+    ['OROMIYA', 'Oromia'],
+    ['gambella', 'Gambela'],
+    ['TIGRAI', 'Tigray'],
+    ['benishangul gumuz', 'Benishangul-Gumuz'],
+  ])('resolves the variant %s', (input, expected) => {
+    expect(normalizeRegion(input)).toBe(expected);
+  });
+
+  it('maps a zone name in the region slot back to its parent region', () => {
+    // eTrade's own placeholder data does this — "EASTERN TIGRAY" is a zone.
+    expect(normalizeRegion('EASTERN TIGRAY')).toBe('Tigray');
+    expect(normalizeRegion('North Wollo')).toBe('Amhara');
+  });
+
+  it.each([
+    ['a city, not a region', 'Arba Minch'],
+    ['unknown text', 'Nowhere Land'],
+    ['empty', ''],
+    ['whitespace only', '   '],
+    ['null', null],
+    ['undefined', undefined],
+  ])('returns null for %s rather than guessing', (_label, input) => {
+    expect(normalizeRegion(input as string | null | undefined)).toBeNull();
+  });
+
+  it('never returns a value outside the canonical set', () => {
+    const samples = ['ADDIS ABABA', 'oromoia', 'EASTERN TIGRAY', 'garbage', ''];
+    for (const s of samples) {
+      const out = normalizeRegion(s);
+      if (out !== null) {
+        expect(ETHIOPIAN_REGIONS).toContain(out);
+      }
+    }
+  });
+});

apps/edr-freight-api/src/modules/health/health.controller.ts

@@ -0,0 +1,108 @@
+// health.controller.ts
+
+import { Controller, Get, HttpStatus, Res } from "@nestjs/common";
+import { ApiOperation, ApiTags } from "@nestjs/swagger";
+import { InjectDataSource } from "@nestjs/typeorm";
+import { Public } from "@edr/api-common";
+import { Response } from "express";
+import { DataSource } from "typeorm";
+
+import { EmailClientService } from "../notifications/email-client.service";
+import { SmsClientService } from "../notifications/sms-client.service";
+
+type CheckStatus = "ok" | "error" | "unknown";
+
+/**
+ * Readiness normally stays green when only the broker is down.
+ *
+ * A 503 pulls the pod out of the load balancer, which would take booking,
+ * tracking and billing offline because SMS is unreachable — a strictly worse
+ * outcome than degraded notifications. The broker check is therefore reported,
+ * not enforced, and `READINESS_REQUIRES_BROKER=true` opts into hard-failing for
+ * deployments where a silent OTP black hole is the greater risk.
+ */
+const READINESS_REQUIRES_BROKER =
+  process.env.READINESS_REQUIRES_BROKER === "true";
+
+@ApiTags("Health")
+@Controller("health")
+export class HealthController {
+  constructor(
+    @InjectDataSource()
+    private readonly dataSource: DataSource,
+    private readonly smsClient: SmsClientService,
+    private readonly emailClient: EmailClientService,
+  ) {}
+
+  @Get()
+  @Public()
+  @ApiOperation({ summary: "Liveness probe" })
+  liveness() {
+    return { status: "ok", timestamp: new Date().toISOString() };
+  }
+
+  @Get("ready")
+  @Public()
+  @ApiOperation({
+    summary:
+      "Readiness probe — database plus SMS/email broker connectivity. Broker failures report as degraded unless READINESS_REQUIRES_BROKER=true.",
+  })
+  async readiness(@Res() res: Response) {
+    const startedAt = Date.now();
+
+    let database: { status: CheckStatus; latencyMs: number; error?: string };
+    try {
+      await this.dataSource.query("SELECT 1");
+      database = { status: "ok", latencyMs: Date.now() - startedAt };
+    } catch (error) {
+      database = {
+        status: "error",
+        latencyMs: Date.now() - startedAt,
+        error: error instanceof Error ? error.message : "Unknown error",
+      };
+    }
+
+    // `null` from the client means the connection manager was not reachable
+    // through Nest's internals — surfaced as "unknown" so a shape change in
+    // @nestjs/microservices degrades to honest ignorance, not a false "ok".
+    const toStatus = (connected: boolean | null): CheckStatus =>
+      connected === null ? "unknown" : connected ? "ok" : "error";
+
+    const broker = {
+      sms: { status: toStatus(this.smsClient.brokerConnected) },
+      email: { status: toStatus(this.emailClient.brokerConnected) },
+      // Every OTP, and every booking/billing notification, publishes through
+      // these. `error` here means codes are being generated and silently dropped.
+      enabled: process.env.RABBITMQ_ENABLED !== "false",
+    };
+
+    const brokerDown =
+      broker.sms.status === "error" || broker.email.status === "error";
+    const failed =
+      database.status === "error" ||
+      (READINESS_REQUIRES_BROKER && brokerDown);
+
+    const status = failed ? "error" : brokerDown ? "degraded" : "ok";
+
+    return res
+      .status(failed ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK)
+      .json({
+        status,
+        timestamp: new Date().toISOString(),
+        checks: { database, broker },
+      });
+  }
+
+  @Get("info")
+  @Public()
+  @ApiOperation({ summary: "App info — version, environment, uptime" })
+  info() {
+    return {
+      name: "edr-freight-api",
+      version: process.env.npm_package_version ?? "1.0.0",
+      environment: process.env.NODE_ENV ?? "development",
+      uptimeSeconds: Math.floor(process.uptime()),
+      timestamp: new Date().toISOString(),
+    };
+  }
+}

apps/edr-freight-api/src/modules/health/health.module.ts

@@ -0,0 +1,14 @@
+// health.module.ts
+
+import { Module } from "@nestjs/common";
+
+import { HealthController } from "./health.controller";
+import { NotificationsModule } from "../notifications/notifications.module";
+
+@Module({
+  // NotificationsModule exports the SMS/email clients; the readiness probe reads
+  // their broker connection state rather than opening a second connection.
+  imports: [NotificationsModule],
+  controllers: [HealthController],
+})
+export class HealthModule {}

apps/edr-freight-api/src/modules/notifications/broker.util.spec.ts

@@ -0,0 +1,90 @@
+import { Logger } from '@nestjs/common';
+import { ClientProxy } from '@nestjs/microservices';
+import { NEVER, Observable, throwError } from 'rxjs';
+
+import { isBrokerConnected, publishConfirmed } from './broker.util';
+
+/**
+ * `ClientProxy.emit()` returns a cold Observable that, for RMQ, completes without
+ * emitting once `dispatchEvent` settles — and rejects if the publish fails. These
+ * fakes reproduce each of those three shapes.
+ */
+function clientEmitting(source: Observable<unknown>): ClientProxy {
+  return { emit: jest.fn().mockReturnValue(source) } as unknown as ClientProxy;
+}
+
+describe('publishConfirmed', () => {
+  const logger = { error: jest.fn() } as unknown as Logger;
+
+  beforeEach(() => jest.clearAllMocks());
+
+  it('is true when the publish completes (broker confirmed)', async () => {
+    // Completes with no value — the success shape, and the case that throws
+    // EmptyError without a defaultIfEmpty.
+    const client = clientEmitting(new Observable<never>((s) => s.complete()));
+    await expect(publishConfirmed(client, 'send-sms', {}, logger)).resolves.toBe(true);
+  });
+
+  it('is false when the publish never settles, rather than hanging', async () => {
+    // A broker that is down: amqp-connection-manager buffers the publish and the
+    // promise would never resolve. The timeout is what stops one dead broker from
+    // hanging every caller of sendSms/sendEmail.
+    const client = clientEmitting(NEVER);
+    await expect(publishConfirmed(client, 'send-sms', {}, logger, 20)).resolves.toBe(
+      false,
+    );
+    expect(logger.error).toHaveBeenCalled();
+  });
+
+  it('is false when the publish errors', async () => {
+    const client = clientEmitting(throwError(() => new Error('channel closed')));
+    await expect(publishConfirmed(client, 'send-email', {}, logger)).resolves.toBe(
+      false,
+    );
+    expect(logger.error).toHaveBeenCalled();
+  });
+});
+
+describe('isBrokerConnected', () => {
+  /** Stands in for `ClientProxy.unwrap()`, which returns the AmqpConnectionManager. */
+  function clientUnwrapping(manager: unknown): ClientProxy {
+    return { unwrap: () => manager } as unknown as ClientProxy;
+  }
+
+  it('reports the connection manager state', () => {
+    expect(isBrokerConnected(clientUnwrapping({ isConnected: () => true }))).toBe(
+      true,
+    );
+    expect(isBrokerConnected(clientUnwrapping({ isConnected: () => false }))).toBe(
+      false,
+    );
+  });
+
+  it('is false when unwrap throws — the client never connected', () => {
+    // ClientRMQ.unwrap() throws "Not initialized" while its internal client is
+    // null, which is what a failed boot-time connect leaves behind. That is a
+    // real down signal and must not be softened to "unknown".
+    const uninitialised = {
+      unwrap: () => {
+        throw new Error('Not initialized. Please call the "connect" method first.');
+      },
+    } as unknown as ClientProxy;
+    expect(isBrokerConnected(uninitialised)).toBe(false);
+  });
+
+  it('is null — not a guess — when the manager lacks isConnected or it throws', () => {
+    // Guards the health endpoint against reporting "ok" if amqp-connection-manager
+    // or Nest changes shape and the accessor we rely on disappears.
+    expect(isBrokerConnected(clientUnwrapping(null))).toBeNull();
+    expect(isBrokerConnected(clientUnwrapping({}))).toBeNull();
+    expect(
+      isBrokerConnected(
+        clientUnwrapping({
+          isConnected: () => {
+            throw new Error('boom');
+          },
+        }),
+      ),
+    ).toBeNull();
+  });
+});

apps/edr-freight-api/src/modules/notifications/broker.util.ts

@@ -0,0 +1,92 @@
+// broker.util.ts
+
+import { Logger } from "@nestjs/common";
+import { ClientProxy } from "@nestjs/microservices";
+import { defaultIfEmpty, lastValueFrom, timeout } from "rxjs";
+
+/**
+ * How long to wait for a publisher confirm before giving up on a message.
+ *
+ * Load-bearing, not a nicety: when the broker is unreachable
+ * amqp-connection-manager buffers the publish and retries it on reconnect, so the
+ * underlying promise never settles. Without a bound, one dead broker turns every
+ * caller of sendSms/sendEmail into a hung request.
+ */
+export const PUBLISH_CONFIRM_TIMEOUT_MS = Number(
+  process.env.RABBITMQ_PUBLISH_TIMEOUT_MS ?? 5000,
+);
+
+/**
+ * Publish an event and wait for RabbitMQ to confirm it.
+ *
+ * `ClientProxy.emit()` returns a *cold* Observable. Called without subscribing —
+ * as this codebase did everywhere — nothing forces the publish to be observed, so
+ * the caller reports success whether or not the broker ever accepted the message.
+ * Awaiting it drives `dispatchEvent`, which resolves only once
+ * amqp-connection-manager's ChannelWrapper has a publisher confirm.
+ *
+ * So `true` here means the broker took ownership of the message. It still says
+ * nothing about the consumer, the SMS gateway, or delivery to a handset — those
+ * remain outside this process's knowledge.
+ */
+export async function publishConfirmed(
+  client: ClientProxy,
+  pattern: string,
+  payload: unknown,
+  logger: Logger,
+  timeoutMs: number = PUBLISH_CONFIRM_TIMEOUT_MS,
+): Promise<boolean> {
+  try {
+    // `emit` completes without emitting a value, so lastValueFrom needs a default
+    // or it rejects with EmptyError on the success path.
+    await lastValueFrom(
+      client
+        .emit(pattern, payload)
+        .pipe(timeout(timeoutMs), defaultIfEmpty(undefined)),
+    );
+    return true;
+  } catch (error) {
+    logger.error(
+      `broker.publish.failed pattern='${pattern}' timeoutMs=${timeoutMs}: ${
+        error instanceof Error ? error.message : String(error)
+      }`,
+      error instanceof Error ? error.stack : undefined,
+    );
+    return false;
+  }
+}
+
+/**
+ * Whether the client's connection manager currently believes it is connected.
+ *
+ * Uses `ClientProxy.unwrap()` — Nest's public accessor for the underlying
+ * transport client, which for `ClientRMQ` is the `AmqpConnectionManager`. Calling
+ * `connect()` instead cannot answer this: it resolves against a *disconnected*
+ * manager too, so it never distinguishes up from down.
+ *
+ * Three outcomes, deliberately distinct:
+ *  - `false` when the manager reports disconnected, or when `unwrap()` throws
+ *    because the client was never initialised (a failed boot-time connect leaves
+ *    it null — genuinely down, not unknown);
+ *  - `null` when the manager exists but has no `isConnected`, i.e. the library
+ *    shape changed under us — the health endpoint reports "unknown" rather than
+ *    quietly claiming health;
+ *  - `true` only on an explicit positive from the manager.
+ */
+export function isBrokerConnected(client: ClientProxy): boolean | null {
+  let manager: unknown;
+  try {
+    manager = client.unwrap<unknown>();
+  } catch {
+    // "Not initialized. Please call the connect method first." — no connection
+    // was ever established, which is a real down signal, not an unknown one.
+    return false;
+  }
+  const probe = manager as { isConnected?: () => boolean } | null;
+  if (!probe || typeof probe.isConnected !== "function") return null;
+  try {
+    return probe.isConnected();
+  } catch {
+    return null;
+  }
+}

apps/edr-freight-api/src/modules/notifications/email-client.service.ts

@@ -6,6 +6,7 @@ import {
 } from "@nestjs/common";
 import { ClientProxy } from "@nestjs/microservices";
 import { SendEmailDto } from "./dtos/email.dto";
+import { isBrokerConnected, publishConfirmed } from "./broker.util";
 
 @Injectable()
 export class EmailClientService implements OnApplicationBootstrap {
@@ -33,19 +34,34 @@ export class EmailClientService implements OnApplicationBootstrap {
       this.logger.warn(`RABBITMQ disabled — skipped EMAIL to=${dto.to}`);
       return { queued: false };
     }
-    this.emailClient.emit("send-email", {
-      to: dto.to,
-      subject: dto.subject,
-      text: dto.text,
-      html: dto.html,
-      appKey: "IFHCRS-LICENSE-MANAGEMENT",
-    });
-    // Fire-and-forget enqueue: confirms hand-off to RabbitMQ, NOT delivery.
+    const queued = await publishConfirmed(
+      this.emailClient,
+      "send-email",
+      {
+        to: dto.to,
+        subject: dto.subject,
+        text: dto.text,
+        html: dto.html,
+        appKey: "IFHCRS-LICENSE-MANAGEMENT",
+      },
+      this.logger,
+    );
+    // Publisher-confirmed: RabbitMQ has taken ownership of the message. Still NOT
+    // delivery — the consumer and the SMTP hop are downstream and invisible here.
     this.logger.log(
-      `EMAIL queued to RabbitMQ [${process.env.EMAIL_QUEUE ?? "email_queue"}] pattern='send-email'`,
+      `EMAIL publish to RabbitMQ [${process.env.EMAIL_QUEUE ?? "email_queue"}] pattern='send-email' confirmed=${queued}`,
     );
     // Recipient + content are PII — debug only.
     this.logger.debug(`EMAIL payload to=${dto.to} subject="${dto.subject}"`);
-    return { queued: true };
+    return { queued };
+  }
+
+  /**
+   * Connection state for the health endpoint. `null` means the broker client did
+   * not expose its manager — reported as "unknown" rather than assumed healthy.
+   */
+  get brokerConnected(): boolean | null {
+    if (!this.enabled) return false;
+    return isBrokerConnected(this.emailClient);
   }
 }

apps/edr-freight-api/src/modules/notifications/sms-client.service.ts

@@ -6,6 +6,7 @@ import {
 } from "@nestjs/common";
 import { ClientProxy } from "@nestjs/microservices";
 import { BulkMessagesDto, SingleMessageDto } from "./dtos/sms.dto";
+import { isBrokerConnected, publishConfirmed } from "./broker.util";
 
 @Injectable()
 export class SmsClientService implements OnApplicationBootstrap {
@@ -14,7 +15,7 @@ export class SmsClientService implements OnApplicationBootstrap {
   constructor(
     @Inject("SMS_SERVICE")
     private smsClient: ClientProxy,
-  ) {}
+  ) { }
 
   private readonly enabled = process.env.RABBITMQ_ENABLED !== "false";
 
@@ -26,7 +27,7 @@ export class SmsClientService implements OnApplicationBootstrap {
         this.logger.log("connected to SMS service");
       })
       .catch((err) => {
-        console.error("Error happened at SMS service", err);
+        this.logger.error("Error happened at SMS service", err);
       });
   }
 
@@ -35,34 +36,61 @@ export class SmsClientService implements OnApplicationBootstrap {
       this.logger.warn(`RABBITMQ disabled — skipped SMS`);
       return { queued: false };
     }
-    this.smsClient.emit("send-sms", {
-      to: dto.to,
-      text: dto.message,
-      appKey: "IFHCRS-LICENSE-MANAGEMENT",
-    });
-    // Fire-and-forget enqueue: confirms hand-off to RabbitMQ, NOT delivery.
+    const queued = await publishConfirmed(
+      this.smsClient,
+      "send-sms",
+      {
+        to: dto.to,
+        text: dto.message,
+        appKey: "IFHCRS-LICENSE-MANAGEMENT",
+      },
+      this.logger,
+    );
+    // Publisher-confirmed: RabbitMQ has taken ownership of the message. Still NOT
+    // delivery — the consumer, the SMS gateway and the carrier are all downstream
+    // of this and invisible from here.
     this.logger.log(
-      `SMS queued to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='send-sms'`,
+      `SMS publish to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='send-sms' confirmed=${queued}`,
     );
     // Recipient + content are PII — debug only.
     this.logger.debug(`SMS payload to=${dto.to} text="${dto.message}"`);
-    return { queued: true };
+    return { queued };
   }
 
   async sendBulkMessages(dto: BulkMessagesDto): Promise<{ queued: boolean }> {
     if (!this.enabled) {
-      this.logger.warn(`RABBITMQ disabled — skipped BULK SMS (${dto.messages?.length ?? 0} messages)`);
+      this.logger.warn(
+        `RABBITMQ disabled — skipped BULK SMS (${dto.messages?.length ?? 0} messages)`,
+      );
       return { queued: false };
     }
-    const messages = (dto.messages ?? []).map((m) => ({ to: m.to, text: m.message, from: m.from }));
-    this.smsClient.emit("ozeking-bulk-sms", {
-      messages,
-      appKey: "IFHCRS-LICENSE-MANAGEMENT",
-    });
+    const messages = (dto.messages ?? []).map((m) => ({
+      to: m.to,
+      text: m.message,
+      from: m.from,
+    }));
+    const queued = await publishConfirmed(
+      this.smsClient,
+      "ozeking-bulk-sms",
+      {
+        messages,
+        appKey: "IFHCRS-LICENSE-MANAGEMENT",
+      },
+      this.logger,
+    );
     this.logger.log(
-      `BULK SMS queued to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='ozeking-bulk-sms' count=${messages.length}`,
+      `BULK SMS publish to RabbitMQ [${process.env.SMS_QUEUE ?? "sms_queue"}] pattern='ozeking-bulk-sms' count=${messages.length} confirmed=${queued}`,
     );
     this.logger.debug(`BULK SMS payload messages=${JSON.stringify(messages)}`);
-    return { queued: true };
+    return { queued };
+  }
+
+  /**
+   * Connection state for the health endpoint. `null` means the broker client did
+   * not expose its manager — reported as "unknown" rather than assumed healthy.
+   */
+  get brokerConnected(): boolean | null {
+    if (!this.enabled) return false;
+    return isBrokerConnected(this.smsClient);
   }
 }

apps/edr-freight-api/src/modules/otp/otp.controller.ts

@@ -41,7 +41,13 @@ export class OtpController {
     @Body("email")
     email?: string
   ) {
-    return this.otpService.sendOtp(toTarget(phone, email));
+    // `delivered` stays server-side: this route is @Public(), and whether our
+    // broker accepted the publish is infrastructure state an anonymous caller has
+    // no need for. It is on the `otp.dispatch` log line instead.
+    const { success, message } = await this.otpService.sendOtp(
+      toTarget(phone, email)
+    );
+    return { success, message };
   }
 
   // ---------------------------------------------------------------------------

apps/edr-freight-api/src/modules/otp/otp.service.spec.ts

@@ -11,8 +11,15 @@ describe('normalizeOtpTarget', () => {
     expect(normalizeOtpTarget({ phone: '0712345678' }).phone).toBe('+251712345678');
   });
 
-  it('passes email targets through untouched', () => {
-    expect(normalizeOtpTarget({ email: 'a@b.com' })).toEqual({ email: 'a@b.com' });
+  it('canonicalises email case and surrounding whitespace to one key', () => {
+    const forms = ['a@b.com', 'A@B.com', '  a@B.COM  ', 'A@b.COM'];
+    const keys = forms.map((email) => normalizeOtpTarget({ email }).email);
+    expect(new Set(keys)).toEqual(new Set(['a@b.com']));
+  });
+
+  it('keeps an already-normalised email stable (idempotent)', () => {
+    const once = normalizeOtpTarget({ email: ' User@Example.COM ' }).email!;
+    expect(normalizeOtpTarget({ email: once }).email).toBe(once);
   });
 
   it('keeps an already-normalised number stable (idempotent)', () => {
@@ -40,8 +47,10 @@ describe('OtpService — send/verify agree across phone formats', () => {
         rows.delete(row.phone ?? row.email!);
       }),
     };
-    const sms = { sendSms: jest.fn().mockResolvedValue(undefined) };
-    const email = { sendEmail: jest.fn().mockResolvedValue(undefined) };
+    // Both clients return `{ queued }` — the service reads it to tell a published
+    // code apart from one the transport silently dropped.
+    const sms = { sendSms: jest.fn().mockResolvedValue({ queued: true }) };
+    const email = { sendEmail: jest.fn().mockResolvedValue({ queued: true }) };
     const service = new OtpService(repo as never, sms as never, email as never);
     return { service, rows };
   }
@@ -58,4 +67,16 @@ describe('OtpService — send/verify agree across phone formats', () => {
       service.verifyOtpForAction({ phone: '0986680099' }, stored),
     ).resolves.toEqual({ success: true });
   });
+
+  it('verifies a code sent to User@X.com when verify is called with user@x.com', async () => {
+    const { service, rows } = makeService();
+    await service.sendOtp({ email: '  User@Example.COM ' });
+    const stored = [...rows.values()][0]!.otp;
+
+    [...rows.values()][0]!.updatedAt = new Date();
+
+    await expect(
+      service.verifyOtpForAction({ email: 'user@example.com' }, stored),
+    ).resolves.toEqual({ success: true });
+  });
 });

apps/edr-freight-api/src/modules/otp/otp.service.ts

@@ -22,7 +22,18 @@ export type OtpTarget = { phone?: string; email?: string };
  * Email targets pass through untouched.
  */
 export function normalizeOtpTarget(target: OtpTarget): OtpTarget {
-  if (target.email || !target.phone) return target;
+  if (target.email) {
+    // Same contract as the phone branch below: the string stored on send and the
+    // one looked up on verify must be byte-identical, or the code is invisible to
+    // the verifier. Addresses reach us from a raw `@Body("email")` with no DTO or
+    // ValidationPipe, so `User@X.com`, `user@x.com` and a copy-paste with a
+    // trailing space are three different keys for one mailbox. Domains are
+    // case-insensitive (RFC 1035); local-parts are formally case-sensitive
+    // (RFC 5321 §2.4) but no mail provider in practice treats them so, and
+    // matching what users expect beats matching the letter of the spec here.
+    return { email: target.email.trim().toLowerCase() };
+  }
+  if (!target.phone) return target;
   const raw = target.phone.trim();
   const digits = raw.replace(/[^\d+]/g, '');
   if (digits.startsWith('+')) return { phone: digits };
@@ -61,6 +72,9 @@ export class OtpService {
     // Store under the canonical E.164 key so verify (which normalises the same
     // way) always finds this row regardless of how either side typed the number.
     const target = normalizeOtpTarget(rawTarget);
+    const channel = target.email ? "email" : "sms";
+    const label = this.targetLabel(target);
+    const startedAt = Date.now();
     try {
       // The verification code is generated server-side — never supplied by the
       // caller — so the OTP stays a secret known only to the server and the
@@ -78,6 +92,14 @@ export class OtpService {
         await this.otpRepository.createOtp(target, otp);
       }
 
+      // `rotate` means a code already existed for this target and was replaced —
+      // the previous one is now dead. A user holding a slow-to-arrive SMS and
+      // typing its code will fail against the row; this line is how that shows up
+      // in the log rather than as an unexplained "invalid OTP" report.
+      this.logger.log(
+        `otp.issue channel=${channel} target=${label} action=${existing ? "rotate" : "create"}`,
+      );
+
       // NOTE: do NOT reset the brute-force attempt counter on send. Clearing it
       // here let an attacker wipe the per-target guess budget just by calling
       // /otp/send between guesses. The counter is cleared only when the code is
@@ -86,40 +108,97 @@ export class OtpService {
       // /otp/verify routes (a NestJS ThrottlerGuard / @Throttle) — none exists
       // in the codebase yet.
 
-      if (target.email) {
-        // send email (queued to RabbitMQ via the shared Email service)
-        await this.emailClient.sendEmail({
-          to: target.email,
-          subject: "Your EDR Freight verification code",
-          text: `Your verification code is ${otp}`,
-        });
-      } else {
-        // send sms (queued to RabbitMQ via the shared SMS service)
-        await this.smsClient.sendSms({
-          to: target.phone as string,
-          message: `Your verification code is ${otp}`,
-        });
+      // Both clients report hand-off, not delivery — capture it rather than
+      // discarding it, so "queued=false" is distinguishable from a code that was
+      // published fine and lost downstream at the carrier.
+      const { queued } = target.email
+        ? await this.emailClient.sendEmail({
+            to: target.email,
+            subject: "Your EDR Freight verification code",
+            text: `Your verification code is ${otp}`,
+          })
+        : await this.smsClient.sendSms({
+            to: target.phone as string,
+            message: `Your verification code is ${otp}`,
+          });
+
+      this.logger.log(
+        `otp.dispatch channel=${channel} target=${label} queued=${queued} latencyMs=${
+          Date.now() - startedAt
+        }`,
+      );
+
+      if (!queued) {
+        // The row is committed and we are about to answer "OTP sent successfully",
+        // but nothing left this process. Without this line the only symptom is a
+        // user who never receives a code — indistinguishable from carrier loss,
+        // and the misleading success response makes it look like our side worked.
+        this.logger.error(
+          `otp.dispatch.dropped channel=${channel} target=${label} rabbitmqEnabled=${
+            process.env.RABBITMQ_ENABLED ?? "unset"
+          } — transport reported no hand-off; no code will arrive for this send`,
+        );
       }
 
+      // SECURITY: this logs a live credential in cleartext. Anyone with read
+      // access to the log stream can complete a password reset or a contract
+      // signature for the address on the same line. Kept deliberately (log
+      // aggregation is the debugging path for flaky SMS here) — if that tradeoff
+      // is ever revisited, gate on an env flag rather than deleting the line, so
+      // dev keeps its workflow.
       this.logger.log(`OTP send for ${target.email ?? target.phone}: ${otp}`);
       return {
         success: true,
 
+        // Distinguishes "we published it" from "the transport is a no-op". The
+        // HTTP response shape is unchanged; the controller drops this field.
+        delivered: queued,
+
         message: "OTP sent successfully",
       };
     } catch (error) {
       // Log the real cause (DB/SMS/email failure) with its stack so a deployed
       // "Failed to send OTP" 400 is diagnosable from the API logs, not opaque.
       this.logger.error(
-        `Failed to send OTP to ${target.email ?? target.phone}: ${
-          error instanceof Error ? error.message : String(error)
-        }`,
+        `otp.dispatch.failed channel=${channel} target=${label} latencyMs=${
+          Date.now() - startedAt
+        }: ${error instanceof Error ? error.message : String(error)}`,
         error instanceof Error ? error.stack : undefined,
       );
       throw new BadRequestException("Failed to send OTP");
     }
   }
 
+  /**
+   * Correlation key shared by every `otp.*` line for one address, so a send and
+   * its later verify can be joined with a single grep. The raw target is used
+   * because the code itself is already logged in cleartext above — hashing the
+   * address while printing the credential next to it would buy nothing.
+   */
+  private targetLabel(target: OtpTarget): string {
+    return target.email ?? target.phone ?? "unknown";
+  }
+
+  /**
+   * One line per verify exit path. `result` is a closed set — ok | invalid |
+   * expired | exhausted | not_found — so failures can be counted by reason
+   * instead of inferred from error strings that the frontend also depends on.
+   */
+  private logVerify(
+    target: OtpTarget,
+    mode: "simple" | "action",
+    result: "ok" | "invalid" | "expired" | "exhausted" | "not_found",
+    detail?: string,
+  ) {
+    const line = `otp.verify channel=${
+      target.email ? "email" : "sms"
+    } target=${this.targetLabel(target)} mode=${mode} result=${result}${
+      detail ? ` ${detail}` : ""
+    }`;
+    if (result === "ok") this.logger.log(line);
+    else this.logger.warn(line);
+  }
+
   // ---------------------------------------------------------------------------
   // Verify OTP
   // ---------------------------------------------------------------------------
@@ -134,6 +213,9 @@ export class OtpService {
 
     // not found
     if (!otpData) {
+      // No row for this key. Most often a normalisation mismatch or a code that
+      // was already consumed/burned — not necessarily a caller who never asked.
+      this.logVerify(target, "simple", "not_found");
       throw new BadRequestException(
         target.email ? "Email address not found" : "Phone number not found",
       );
@@ -145,6 +227,12 @@ export class OtpService {
     if (ageMs > this.ACTION_OTP_TTL_MS) {
       await this.otpRepository.deleteOtp(otpData);
       this.actionAttempts.delete(key);
+      this.logVerify(
+        target,
+        "simple",
+        "expired",
+        `ageMs=${ageMs} ttlMs=${this.ACTION_OTP_TTL_MS}`,
+      );
       throw new BadRequestException(
         "Verification code has expired. Request a new one.",
       );
@@ -157,17 +245,30 @@ export class OtpService {
       if (attempts >= this.MAX_ACTION_ATTEMPTS) {
         await this.otpRepository.deleteOtp(otpData);
         this.actionAttempts.delete(key);
+        this.logVerify(
+          target,
+          "simple",
+          "exhausted",
+          `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`,
+        );
         throw new BadRequestException(
           "Too many incorrect attempts. Request a new code.",
         );
       }
       this.actionAttempts.set(key, attempts);
+      this.logVerify(
+        target,
+        "simple",
+        "invalid",
+        `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`,
+      );
       throw new BadRequestException("Invalid OTP");
     }
 
     // single-use: consume the code on success so it can't be replayed.
     await this.otpRepository.deleteOtp(otpData);
     this.actionAttempts.delete(key);
+    this.logVerify(target, "simple", "ok", `ageMs=${ageMs}`);
 
     return {
       success: true,
@@ -210,6 +311,7 @@ export class OtpService {
     const key = this.targetKey(target);
 
     if (!otpData) {
+      this.logVerify(target, "action", "not_found");
       throw new BadRequestException(
         target.email
           ? "No verification code was requested for this email"
@@ -223,6 +325,7 @@ export class OtpService {
       await this.otpRepository.deleteOtp(otpData);
       this.actionAttempts.delete(key);
 
+      this.logVerify(target, "action", "expired", `ageMs=${ageMs} ttlMs=${ttlMs}`);
       throw new BadRequestException(
         "Verification code has expired. Request a new one.",
       );
@@ -235,18 +338,31 @@ export class OtpService {
         await this.otpRepository.deleteOtp(otpData);
         this.actionAttempts.delete(key);
 
+        this.logVerify(
+          target,
+          "action",
+          "exhausted",
+          `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`,
+        );
         throw new BadRequestException(
           "Too many incorrect attempts. Request a new code.",
         );
       }
 
       this.actionAttempts.set(key, attempts);
+      this.logVerify(
+        target,
+        "action",
+        "invalid",
+        `attempts=${attempts}/${this.MAX_ACTION_ATTEMPTS} ageMs=${ageMs}`,
+      );
       throw new BadRequestException("Invalid verification code");
     }
 
     // single-use: consume on success
     await this.otpRepository.deleteOtp(otpData);
     this.actionAttempts.delete(key);
+    this.logVerify(target, "action", "ok", `ageMs=${ageMs}`);
 
     return { success: true };
   }