Merge pull request #397 from Tria-plc/feight/fix/ui-ux-issues

Feight/fix/UI ux issues

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

@@ -49,7 +49,6 @@ const DEFAULT_DUE_DAYS = 14;
 
 /** Statuses an invoice can still be settled (paid/refunded/cancelled) from. */
 const OPEN_STATUSES: Freight.InvoiceStatus[] = [
-  Freight.InvoiceStatus.Draft,
   Freight.InvoiceStatus.Issued,
   Freight.InvoiceStatus.Pending,
   Freight.InvoiceStatus.PartiallyPaid,
@@ -285,20 +284,15 @@ export class BillingService {
 
   /**
    * Initiate gateway payment for one of the customer's own invoices. Verifies
-   * ownership, then charges whichever open invoice the source currently has
-   * (see {@link payInvoice}).
+   * ownership, then charges the invoice directly by ID (see {@link payInvoice}).
    */
   async payInvoiceForUser(
     id: string,
     userId: string,
     opts: PayInvoiceOptions = {},
   ): Promise<InitiateResponseDto> {
-    const invoice = await this.findByIdForUser(id, userId);
-    return this.payInvoice(
-      invoice.source as Freight.InvoiceSource,
-      invoice.sourceId,
-      opts,
-    );
+    await this.findByIdForUser(id, userId);
+    return this.payInvoice(id, opts);
   }
 
   /** Sealed invoice PDF for one of the customer's own invoices (ownership-checked). */
@@ -423,23 +417,89 @@ export class BillingService {
   // ── State transitions ────────────────────────────────────────────────────────
 
   /**
-   * Mark an invoice paid and link the gateway payment, then emit
-   * `${source}.invoice.paid`. Full-payment only — no partial settlement.
-   * No-op when the invoice is already paid. Pass `manager` to enlist in a
-   * caller's transaction.
+   * Run `fn` inside a transaction and only emit its returned domain event
+   * after commit. When the caller passes their own `manager`, they own commit
+   * timing — `fn`'s event fires inline as soon as it resolves (the outer
+   * transaction may still roll back afterwards; this is the caller's
+   * documented tradeoff). When no `manager` is given, this opens its own
+   * transaction and defers the emit until after that transaction commits, so
+   * listeners (e.g. booking advancement) can never observe an invoice change
+   * that then rolls back.
+   */
+  private async runTransition<T>(
+    manager: EntityManager | undefined,
+    fn: (mg: EntityManager) => Promise<{ result: T; emit?: () => void }>,
+  ): Promise<T> {
+    if (manager) {
+      const { result, emit } = await fn(manager);
+      emit?.();
+      return result;
+    }
+    let pending: (() => void) | undefined;
+    const result = await this.dataSource.transaction(async (mg) => {
+      const out = await fn(mg);
+      pending = out.emit;
+      return out.result;
+    });
+    pending?.();
+    return result;
+  }
+
+  /**
+   * Mark an invoice paid, stamp the paid timestamp, sync paid/balance amounts,
+   * append the settlement to the `payments` ledger, link the gateway payment,
+   * then emit `${source}.invoice.paid`. Full-payment only — no partial
+   * settlement. No-op when the invoice is already paid. Pass `manager` to
+   * enlist in a caller's transaction; otherwise locks the row for update and
+   * emits only after commit (see {@link runTransition}).
    */
   async markInvoiceAsPaid(
     invoiceId: string,
     paymentId: string | null = null,
     manager?: EntityManager,
+    settlement: { providerTxnId?: string; paidAt?: Date } = {},
   ): Promise<Invoice | null> {
-    return this.transition(
-      invoiceId,
-      Freight.InvoiceStatus.Paid,
-      "paid",
-      { paymentId: paymentId ?? undefined },
-      manager,
-    );
+    return this.runTransition(manager, async (mg) => {
+      const invoice = await mg.findOne(Invoice, {
+        where: { id: invoiceId },
+        lock: { mode: "pessimistic_write" },
+      });
+      if (!invoice) {
+        throw new NotFoundException(`Invoice ${invoiceId} not found`);
+      }
+      if (invoice.status === Freight.InvoiceStatus.Paid) {
+        return { result: invoice };
+      }
+
+      const paidAt = invoice.paidAt ?? settlement.paidAt ?? new Date();
+      const settledAmount = round2(
+        Number(invoice.totalAmount) - Number(invoice.paidAmount ?? 0),
+      );
+      const entry: InvoicePayment = {
+        amount: settledAmount,
+        method: "GATEWAY",
+        reference: settlement.providerTxnId ?? paymentId ?? null,
+        paidAt: paidAt.toISOString(),
+        metadata: null,
+      };
+      const payments = [...(invoice.payments ?? []), entry];
+
+      const patch = {
+        status: Freight.InvoiceStatus.Paid,
+        paymentId,
+        paidAt,
+        paidAmount: invoice.totalAmount,
+        balanceAmount: 0,
+        payments,
+      };
+      await mg.update(Invoice, { id: invoiceId }, patch as never);
+
+      const updated = { ...invoice, ...patch } as Invoice;
+      return {
+        result: updated,
+        emit: () => this.emitInvoiceEvent("paid", updated),
+      };
+    });
   }
 
   /**
@@ -451,9 +511,11 @@ export class BillingService {
    * at the warehouse counter); gateway settlement goes through
    * {@link markInvoiceAsPaid}.
    *
-   * Throws when the invoice is missing, cancelled, refunded, already fully paid,
-   * or when `amount` is not positive. Pass `manager` to enlist in a caller's
-   * transaction.
+   * Throws when the invoice is missing, cancelled, refunded, already fully
+   * paid, `amount` is not positive, or `amount` exceeds the outstanding
+   * balance. Pass `manager` to enlist in a caller's transaction; otherwise
+   * locks the row for update and emits only after commit (see
+   * {@link runTransition}).
    */
   async recordPayment(
     invoiceId: string,
@@ -466,62 +528,71 @@ export class BillingService {
       );
     }
 
-    const mg = manager ?? this.dataSource.manager;
-    const invoice = await mg.findOne(Invoice, { where: { id: invoiceId } });
-    if (!invoice) throw new NotFoundException(`Invoice ${invoiceId} not found`);
-    if (invoice.status === Freight.InvoiceStatus.Cancelled) {
-      throw new BadRequestException("Cannot pay a cancelled invoice.");
-    }
-    if (invoice.status === Freight.InvoiceStatus.Refunded) {
-      throw new BadRequestException("Cannot pay a refunded invoice.");
-    }
-    if (invoice.status === Freight.InvoiceStatus.Paid) {
-      throw new BadRequestException("Invoice is already fully paid.");
-    }
+    return this.runTransition(manager, async (mg) => {
+      const invoice = await mg.findOne(Invoice, {
+        where: { id: invoiceId },
+        lock: { mode: "pessimistic_write" },
+      });
+      if (!invoice) {
+        throw new NotFoundException(`Invoice ${invoiceId} not found`);
+      }
+      if (invoice.status === Freight.InvoiceStatus.Cancelled) {
+        throw new BadRequestException("Cannot pay a cancelled invoice.");
+      }
+      if (invoice.status === Freight.InvoiceStatus.Refunded) {
+        throw new BadRequestException("Cannot pay a refunded invoice.");
+      }
+      if (invoice.status === Freight.InvoiceStatus.Paid) {
+        throw new BadRequestException("Invoice is already fully paid.");
+      }
+      if (round2(input.amount) > Number(invoice.balanceAmount)) {
+        throw new BadRequestException(
+          `Payment of ${round2(input.amount)} exceeds the outstanding balance of ${Number(invoice.balanceAmount)}.`,
+        );
+      }
 
-    const at = input.paidAt ?? new Date();
-    const { paidAmount, balanceAmount, fullyPaid } = applySettlement(
-      invoice.totalAmount,
-      invoice.paidAmount,
-      input.amount,
-    );
-    const status = fullyPaid
-      ? Freight.InvoiceStatus.Paid
-      : Freight.InvoiceStatus.PartiallyPaid;
+      const at = input.paidAt ?? new Date();
+      const { paidAmount, balanceAmount, fullyPaid } = applySettlement(
+        invoice.totalAmount,
+        invoice.paidAmount,
+        input.amount,
+      );
+      const status = fullyPaid
+        ? Freight.InvoiceStatus.Paid
+        : Freight.InvoiceStatus.PartiallyPaid;
 
-    const entry: InvoicePayment = {
-      amount: round2(input.amount),
-      method: input.method ?? null,
-      reference: input.reference ?? null,
-      paidAt: at.toISOString(),
-      metadata: input.metadata ?? null,
-    };
-    const payments = [...(invoice.payments ?? []), entry];
+      const entry: InvoicePayment = {
+        amount: round2(input.amount),
+        method: input.method ?? null,
+        reference: input.reference ?? null,
+        paidAt: at.toISOString(),
+        metadata: input.metadata ?? null,
+      };
+      const payments = [...(invoice.payments ?? []), entry];
 
-    await mg.update(Invoice, { id: invoice.id }, {
-      paidAmount,
-      balanceAmount,
-      status,
-      payments,
-      paidAt: fullyPaid ? at : (invoice.paidAt ?? null),
-    } as never);
+      const patch = {
+        paidAmount,
+        balanceAmount,
+        status,
+        payments,
+        paidAt: fullyPaid ? at : (invoice.paidAt ?? null),
+      };
+      await mg.update(Invoice, { id: invoice.id }, patch as never);
 
-    const updated = {
-      ...invoice,
-      paidAmount,
-      balanceAmount,
-      status,
-      payments,
-      paidAt: fullyPaid ? at : (invoice.paidAt ?? null),
-    } as Invoice;
-
-    if (fullyPaid) this.emitInvoiceEvent("paid", updated);
-    return updated;
+      const updated = { ...invoice, ...patch } as Invoice;
+      return {
+        result: updated,
+        emit: fullyPaid
+          ? () => this.emitInvoiceEvent("paid", updated)
+          : undefined,
+      };
+    });
   }
 
   /**
    * Mark an invoice refunded and emit `${source}.invoice.refunded`.
-   * No-op when already refunded.
+   * No-op when already refunded. Throws when the invoice has no recorded
+   * payment (nothing to refund).
    */
   async markInvoiceAsRefunded(
     invoiceId: string,
@@ -533,12 +604,20 @@ export class BillingService {
       "refunded",
       {},
       manager,
+      (invoice) => {
+        if (!(Number(invoice.paidAmount) > 0)) {
+          throw new BadRequestException(
+            "Cannot refund an invoice with no recorded payment.",
+          );
+        }
+      },
     );
   }
 
   /**
    * Mark an invoice cancelled and emit `${source}.invoice.cancelled`.
-   * No-op when already cancelled.
+   * No-op when already cancelled. Throws when the invoice has payments
+   * recorded against it (refund it instead).
    */
   async cancelInvoice(
     invoiceId: string,
@@ -550,16 +629,23 @@ export class BillingService {
       "cancelled",
       {},
       manager,
+      (invoice) => {
+        if (Number(invoice.paidAmount) > 0) {
+          throw new BadRequestException(
+            "Cannot cancel an invoice that has payments recorded against it.",
+          );
+        }
+      },
     );
   }
 
   /**
    * Load the invoice, apply the new status (+ extra columns), then emit
-   * `${source}.invoice.<event>`. No-op (returns the invoice) when it is already
-   * in the target status. Throws when the invoice does not exist.
-   *
-   * Note: the event fires in-process synchronously. When a `manager` from an
-   * outer transaction is passed, listeners run before that transaction commits.
+   * `${source}.invoice.<event>`. No-op (returns the invoice, skipping `guard`)
+   * when it is already in the target status. Throws when the invoice does not
+   * exist or `guard` rejects the current state. Pass `manager` to enlist in a
+   * caller's transaction; otherwise locks the row for update and emits only
+   * after commit (see {@link runTransition}).
    */
   private async transition(
     invoiceId: string,
@@ -567,17 +653,27 @@ export class BillingService {
     event: string,
     extra: { paymentId?: string },
     manager?: EntityManager,
+    guard?: (invoice: Invoice) => void,
   ): Promise<Invoice | null> {
-    const mg = manager ?? this.dataSource.manager;
-    const invoice = await mg.findOne(Invoice, { where: { id: invoiceId } });
-    if (!invoice) throw new NotFoundException(`Invoice ${invoiceId} not found`);
-    if (invoice.status === status) return invoice;
+    return this.runTransition(manager, async (mg) => {
+      const invoice = await mg.findOne(Invoice, {
+        where: { id: invoiceId },
+        lock: { mode: "pessimistic_write" },
+      });
+      if (!invoice) {
+        throw new NotFoundException(`Invoice ${invoiceId} not found`);
+      }
+      if (invoice.status === status) return { result: invoice };
+      guard?.(invoice);
 
-    await mg.update(Invoice, { id: invoice.id }, { status, ...extra });
+      await mg.update(Invoice, { id: invoice.id }, { status, ...extra });
 
-    const updated = { ...invoice, ...extra, status } as Invoice;
-    this.emitInvoiceEvent(event, updated);
-    return updated;
+      const updated = { ...invoice, ...extra, status } as Invoice;
+      return {
+        result: updated,
+        emit: () => this.emitInvoiceEvent(event, updated),
+      };
+    });
   }
 
   /** Broadcast `${invoice.source}.invoice.<event>` to in-process listeners. */
@@ -601,16 +697,17 @@ export class BillingService {
   // ── Payment reconciliation (by source) ───────────────────────────────────────
 
   /**
-   * The invoice a gateway payment should settle for a source record, or null if
-   * none. This is the billing document of record for "what is owed" — callers
-   * (e.g. {@link payInvoice}) charge `invoice.totalAmount` against it rather than
-   * recomputing from the source's own total, so discounts/penalties/adjustments
-   * carried on the invoice are honored.
+   * The invoice a source record already has open, or null if it needs a new
+   * one. This is the idempotency check every `ensureInvoiceFor*` (booking,
+   * first-mile, last-mile) runs before generating — it must see DRAFT
+   * invoices too, not just issued ones, otherwise a source that already has
+   * an unissued draft gets a second, duplicate invoice minted alongside it
+   * instead of that draft being reused and then issued.
    *
    * Pass `type` to select a specific invoice when a source carries several (e.g.
    * a booking's up-front vs final charge); omit it to settle whichever single
-   * invoice is currently open. Returns the most recent matching open (unpaid,
-   * non-cancelled) invoice.
+   * invoice is currently open. Returns the most recent matching draft-or-open
+   * (unpaid, non-cancelled) invoice.
    */
   findPayable(
     source: Freight.InvoiceSource,
@@ -621,7 +718,7 @@ export class BillingService {
       where: {
         source,
         sourceId,
-        status: In(OPEN_STATUSES),
+        status: In([Freight.InvoiceStatus.Draft, ...OPEN_STATUSES]),
         ...(type ? { type } : {}),
       },
       order: { issuedAt: "DESC" },
@@ -629,56 +726,24 @@ export class BillingService {
   }
 
   /**
-   * Settle a source's currently-open invoice as paid and link the gateway
-   * payment, then emit `${source}.invoice.paid`. Resolves the open invoice then
-   * delegates to {@link markInvoiceAsPaid}. Full-payment only — no partial
-   * settlement. No-op (returns null) when the source has no open invoice.
-   *
-   * Type-blind by design: settles whichever invoice is due; any per-type reaction
-   * belongs in the `${source}.invoice.paid` handler, which reads `invoice.type`.
-   * Pass the caller's transaction `manager` to enlist in its DB transaction.
-   *
-   * NOTE: the booking flow settles via {@link payInvoice} + the `payment.succeeded`
-   * event ({@link settleByPaymentId}); this source-keyed settle is a generic helper
-   * for callers that settle by source rather than by gateway intent id.
+   * Pass `type` to select a specific invoice when a source carries several (e.g.
+   * a booking's up-front vs final charge); omit it to settle whichever single
+   * invoice is currently open. Returns the most recent matching open (unpaid,
+   * non-cancelled) invoice.
    */
-  async settlePayable(
+  findInvoice(
     source: Freight.InvoiceSource,
     sourceId: string,
-    paymentId: string | null,
-    manager?: EntityManager,
+    type?: string,
   ): Promise<Invoice | null> {
-    const mg = manager ?? this.dataSource.manager;
-    const invoice = await mg.findOne(Invoice, {
-      where: { source, sourceId, status: In(OPEN_STATUSES) },
+    return this.dataSource.getRepository(Invoice).findOne({
+      where: {
+        source,
+        sourceId,
+        ...(type ? { type } : {}),
+      },
       order: { issuedAt: "DESC" },
     });
-    if (!invoice) return null;
-
-    return this.markInvoiceAsPaid(invoice.id, paymentId, mg);
-  }
-
-  /**
-   * Refund a source's paid invoice, then emit `${source}.invoice.refunded`.
-   * Resolves the paid invoice then delegates to {@link markInvoiceAsRefunded}.
-   * No-op (returns null) when the source has no paid invoice.
-   *
-   * Pass the caller's transaction `manager` (e.g. from `payment.service.refund`)
-   * to enlist in its DB transaction.
-   */
-  async refundPayable(
-    source: Freight.InvoiceSource,
-    sourceId: string,
-    manager?: EntityManager,
-  ): Promise<Invoice | null> {
-    const mg = manager ?? this.dataSource.manager;
-    const invoice = await mg.findOne(Invoice, {
-      where: { source, sourceId, status: Freight.InvoiceStatus.Paid },
-      order: { issuedAt: "DESC" },
-    });
-    if (!invoice) return null;
-
-    return this.markInvoiceAsRefunded(invoice.id, mg);
   }
 
   /**
@@ -694,11 +759,17 @@ export class BillingService {
   async expirePayable(
     source: Freight.InvoiceSource,
     sourceId: string,
+    type?: string,
     manager?: EntityManager,
   ): Promise<Invoice | null> {
     const mg = manager ?? this.dataSource.manager;
     const invoice = await mg.findOne(Invoice, {
-      where: { source, sourceId, status: In(OPEN_STATUSES) },
+      where: {
+        source,
+        sourceId,
+        status: In(OPEN_STATUSES),
+        ...(type ? { type } : {}),
+      },
       order: { issuedAt: "DESC" },
     });
     if (!invoice) return null;
@@ -722,17 +793,29 @@ export class BillingService {
     source: Freight.InvoiceSource,
     sourceId: string,
     dueAt: Date,
+    type?: string,
     manager?: EntityManager,
   ): Promise<void> {
     const mg = manager ?? this.dataSource.manager;
     const invoice = await mg.findOne(Invoice, {
-      where: { source, sourceId, status: In(OPEN_STATUSES) },
+      where: {
+        source,
+        sourceId,
+        status: In(OPEN_STATUSES),
+        ...(type ? { type } : {}),
+      },
       order: { issuedAt: "DESC" },
     });
     if (!invoice) return;
     await mg.update(Invoice, { id: invoice.id }, { dueAt });
   }
 
+  /**
+   * Force an invoice to `status`, including issuing a still-DRAFT invoice
+   * (stamping `issuedAt`) — unlike the other transitions here, this is a
+   * blunt admin/workflow override, not a settlement. No-op when the invoice
+   * is missing or already terminal (paid/cancelled/refunded/expired).
+   */
   async updateStatus(
     invoiceId: string,
     status: Freight.InvoiceStatus,
@@ -740,29 +823,33 @@ export class BillingService {
   ): Promise<void> {
     const mg = manager ?? this.dataSource.manager;
     const invoice = await mg.findOne(Invoice, {
-      where: { id: invoiceId, status: In(OPEN_STATUSES) },
-      order: { issuedAt: "DESC" },
+      where: { id: invoiceId, status: In([Freight.InvoiceStatus.Draft, ...OPEN_STATUSES]) },
     });
     if (!invoice) return;
-    await mg.update(Invoice, { id: invoice.id }, { status });
+    await mg.update(
+      Invoice,
+      { id: invoice.id },
+      { status, issuedAt: invoice.issuedAt ?? new Date() },
+    );
   }
 
   // ── Payment initiation & settlement (the gateway boundary) ───────────────────
 
   /**
-   * Charge a source's open invoice through the payment gateway. Billing is the
-   * single place that turns "what is owed" (the invoice) into a payment intent —
-   * the domain never talks to the payment service directly. Resolves the open
-   * invoice, opens an intent for `invoice.totalAmount`, records the intent id on
-   * the invoice (the settlement correlation key), and returns the client action.
+   * Charge an invoice through the payment gateway. Billing is the single place
+   * that turns "what is owed" (the invoice) into a payment intent — the domain
+   * never talks to the payment service directly. Resolves the invoice by ID,
+   * opens an intent for `invoice.balanceAmount` (so partial payments are honored),
+   * records the intent id on the invoice (the settlement correlation key), and
+   * returns the client action.
    *
    * When the provider settles synchronously, the invoice is settled inline here —
    * after the intent id is stored — so the `payment.succeeded` correlation can
-   * never fire before the link exists. Throws when the source has no open invoice.
+   * never fire before the link exists. Throws when the invoice is not found or
+   * not in an open/payable status.
    */
   async payInvoice(
-    source: Freight.InvoiceSource,
-    sourceId: string,
+    invoiceId: string,
     opts: {
       method?: string;
       platform?: "web" | "mobile";
@@ -771,15 +858,17 @@ export class BillingService {
       failureUrl?: string;
     } = {},
   ): Promise<InitiateResponseDto> {
-    const invoice = await this.findPayable(source, sourceId);
+    const invoice = await this.dataSource.getRepository(Invoice).findOne({
+      where: { id: invoiceId, status: In(OPEN_STATUSES) },
+    });
     if (!invoice) {
       throw new NotFoundException(
-        `No open invoice to charge for ${source}:${sourceId}`,
+        `Invoice ${invoiceId} not found or not in a payable status`,
       );
     }
 
     const result = await this.payment.initiate({
-      referenceId: sourceId,
+      referenceId: invoice.sourceId,
       source: invoice.source,
       // Freight payments settle under the generic SHIPMENT reference — how the
       // payment service attributes them to the freight API. The payment ↔ invoice
@@ -788,7 +877,7 @@ export class BillingService {
       // service branches on a domain-specific reference type.
       referenceType: PaymentReferenceType.SHIPMENT,
       orderRef: invoice.invoiceNumber,
-      amountMinor: Math.round(Number(invoice.totalAmount)),
+      amountMinor: Math.round(Number(invoice.balanceAmount)),
       currency: invoice.currency,
       reason: `Payment for invoice ${invoice.invoiceNumber}`,
       method: opts.method ?? "TELEBIRR",
@@ -823,8 +912,8 @@ export class BillingService {
    */
   async settleByPaymentId(
     paymentId: string,
-    _providerTxnId?: string,
-    _paidAt?: Date,
+    providerTxnId?: string,
+    paidAt?: Date,
   ): Promise<Invoice | null> {
     const invoice = await this.dataSource.getRepository(Invoice).findOne({
       where: { paymentId, status: In(OPEN_STATUSES) },
@@ -832,6 +921,9 @@ export class BillingService {
     });
     if (!invoice) return null;
 
-    return this.markInvoiceAsPaid(invoice.id, paymentId);
+    return this.markInvoiceAsPaid(invoice.id, paymentId, undefined, {
+      providerTxnId,
+      paidAt,
+    });
   }
 }