From 586588c7f126e690062abe0a8c52db2157e263d5 Mon Sep 17 00:00:00 2001 From: morteza-mortezai Date: Mon, 9 Feb 2026 16:54:16 +0330 Subject: [PATCH] fix bugs in order --- ORDER_MODULE_REVIEW.md | 276 ++++++++++++++++++ src/modules/orders/crone/order.crone.ts | 1 + .../orders/interface/order.interface.ts | 2 +- .../orders/listeners/order.listeners.ts | 2 +- .../orders/providers/orders.service.ts | 26 +- src/modules/payments/interface/payment.ts | 2 +- .../payments/services/payments.service.ts | 8 +- 7 files changed, 293 insertions(+), 24 deletions(-) create mode 100644 ORDER_MODULE_REVIEW.md diff --git a/ORDER_MODULE_REVIEW.md b/ORDER_MODULE_REVIEW.md new file mode 100644 index 0000000..826699a --- /dev/null +++ b/ORDER_MODULE_REVIEW.md @@ -0,0 +1,276 @@ +# Order Module Review - Business & Logical Bugs + +## Critical Bugs + +### 1. **SQL Column Name Mismatch (CRITICAL)** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 414, 426, 470, 495) + +**Issue**: Raw SQL queries use `restaurant_id` column, but the entity uses `shop` relationship. MikroORM generates `shop_id` column for `@ManyToOne(() => Shop)`, not `restaurant_id`. + +**Impact**: SQL queries will fail with "column does not exist" errors. + +**Code**: +```typescript +WHERE o.restaurant_id = ? // ❌ Wrong column name +``` + +**Fix**: Should be `shop_id` or use MikroORM query builder instead of raw SQL. + +--- + +### 2. **Price Inconsistency Between Cart and Order** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 365-385) + +**Issue**: In `buildOrderItemsData()`, prices are fetched fresh from database (`variant.price`), ignoring cart prices. If prices changed between adding to cart and checkout, user pays different amount than shown. + +**Impact**: +- User sees one price in cart, pays different price +- Potential financial discrepancies +- Poor user experience + +**Code**: +```typescript +unitPrice: variant.price || 0, // ❌ Uses DB price, not cart price +discount: variant.product.discount || 0, // ❌ Uses DB discount, not cart discount +``` + +**Fix**: Use prices from `cartItem` instead of fetching from database. + +--- + +### 3. **Cart Cleared Outside Transaction** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 125-127) + +**Issue**: Cart is cleared AFTER transaction completes but BEFORE `payOrder()` is called. If `payOrder()` fails, cart is already cleared but payment might not be initiated. + +**Impact**: +- User loses cart items even if checkout fails +- Potential data inconsistency +- Poor user experience + +**Code**: +```typescript +const order = await this.em.transactional(async em => { + // ... order creation +}); + +await this.cartService.clearCart(userId, shopId); // ❌ Outside transaction + +const { paymentUrl } = await this.paymentsService.payOrder(order.id); // If this fails, cart is already cleared +``` + +**Fix**: Clear cart only after successful payment initiation, or include in transaction. + +--- + +## High Priority Bugs + +### 4. **Missing Product/Variant Availability Validation** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 369-374) + +**Issue**: No check if product/variant is still active (`isActive`) or available when building order items. + +**Impact**: +- Users can order inactive/unavailable products +- Business rule violations +- Potential fulfillment issues + +**Code**: +```typescript +const variant = await this.em.findOne(Variant, { id: cartItem.productId }, { populate: ['product', 'product.shop'] }); +if (!variant) throw new NotFoundException(OrderMessage.PRODUCT_NOT_FOUND); +// ❌ Missing: Check if variant.product.isActive +// ❌ Missing: Check if variant is still available +``` + +**Fix**: Add validation for `variant.product.isActive` and any availability flags. + +--- + +### 5. **Missing Stock/Inventory Check** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 365-385) + +**Issue**: No validation for product stock/inventory quantity before creating order. + +**Impact**: +- Overselling products +- Orders for out-of-stock items +- Fulfillment failures + +**Fix**: Add stock quantity validation if inventory management exists. + +--- + +### 6. **Race Condition in Checkout** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 66-134) + +**Issue**: Multiple users can checkout simultaneously with same cart items. No locking mechanism to prevent overselling. + +**Impact**: +- Concurrent checkouts can oversell limited stock +- Multiple orders for same limited items +- Inventory inconsistencies + +**Fix**: Implement row-level locking or optimistic locking for variant/product during checkout. + +--- + +### 7. **Status Transition Logic Inconsistency** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 44-52, 282-288) + +**Issue**: `STATUS_TRANSITIONS` map allows `PENDING_PAYMENT` → `PREPARING`, but `canTransition()` method restricts this to Cash payments only. The map doesn't reflect actual business rules. + +**Impact**: +- Confusing code logic +- Potential for incorrect status transitions +- Maintenance issues + +**Code**: +```typescript +// Line 45: Map allows this transition +[OrderStatus.PENDING_PAYMENT]: [OrderStatus.PAID, OrderStatus.CANCELED, OrderStatus.PREPARING], + +// Line 282-288: But logic restricts it +if (from == OrderStatus.PENDING_PAYMENT && to == OrderStatus.PREPARING && paymentMethod !== PaymentMethodEnum.Cash) { + return false; // ❌ Contradicts STATUS_TRANSITIONS map +} +``` + +**Fix**: Remove `PREPARING` from `PENDING_PAYMENT` transitions in the map, or document the special case clearly. + +--- + +## Medium Priority Bugs + +### 8. **Cron Job Logic Incomplete** +**Location**: `src/modules/orders/crone/order.crone.ts` (lines 87-170) + +**Issue**: +- Comment says "shipped/delivered" but only checks `SHIPPED` status +- Uses `updatedAt` which might not reflect when status was changed +- Should check `history` array for actual status change time + +**Impact**: +- `DELIVERED` orders never auto-complete +- Orders might complete too early or too late +- Inaccurate completion timing + +**Code**: +```typescript +// Line 89: Comment mentions delivered +// run every 15 minutes to complete orders that have been in shipped/delivered statuses + +// Line 96-100: But only checks SHIPPED +status: { + $in: [OrderStatus.SHIPPED], // ❌ Missing DELIVERED +}, +updatedAt: { $lte: cutoff }, // ❌ Should check history array for status change time +``` + +**Fix**: Include `DELIVERED` status and check `history` array for accurate status change time. + +--- + +### 9. **History Array Direct Mutation** +**Location**: `src/modules/orders/providers/orders.service.ts` (line 240) + +**Issue**: History array is mutated directly. If flush fails, history might be inconsistent. + +**Impact**: +- Potential data inconsistency +- History might not reflect actual status changes + +**Code**: +```typescript +order.history.push({ status: toStatus, changedAt: new Date(), desc: desc || null }); // ❌ Direct mutation +await this.em.persistAndFlush(order); +``` + +**Fix**: Create new array or use immutable approach. Consider transaction rollback handling. + +--- + +### 10. **Missing Validation for Zero/Negative Quantities** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 93-109) + +**Issue**: No validation that `quantity` is positive when creating order items. + +**Impact**: +- Orders with zero or negative quantities +- Invalid order data +- Potential calculation errors + +**Fix**: Add validation for `quantity > 0` in `buildOrderItemsData()`. + +--- + +### 11. **Payment Amount Set at Creation** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 111-117) + +**Issue**: Payment amount is set to `order.total` at order creation. If order total changes later, payment amount won't match. + +**Impact**: +- Payment amount might not match order total +- Potential refund/adjustment issues + +**Note**: This might be intentional (snapshot at creation), but should be documented. + +--- + +### 12. **Decimal Precision Loss** +**Location**: `src/modules/orders/entities/order.entity.ts` (lines 54, 60, 63, 66, 69, 72, 75) + +**Issue**: All decimal fields use `scale: 0`, meaning no decimal places. This can cause rounding issues for currency. + +**Impact**: +- Loss of precision for fractional currency amounts +- Rounding errors +- Potential financial discrepancies + +**Code**: +```typescript +@Property({ type: 'decimal', precision: 10, scale: 0, default: 0 }) // ❌ scale: 0 +``` + +**Fix**: Use appropriate scale (e.g., `scale: 2` for currency). + +--- + +## Low Priority / Code Quality Issues + +### 13. **Inconsistent Error Handling** +**Location**: Various locations + +**Issue**: Some methods throw exceptions, others return null/undefined. Inconsistent error handling patterns. + +--- + +### 14. **Missing Transaction for Status Change** +**Location**: `src/modules/orders/providers/orders.service.ts` (lines 225-256) + +**Issue**: `changeOrderStatus()` doesn't use explicit transaction, though it modifies order and emits events. + +**Impact**: If event emission fails, order status is changed but notification might not be sent. + +--- + +### 15. **Potential Null Reference** +**Location**: `src/modules/orders/providers/orders.service.ts` (line 247) + +**Issue**: `order.user?.id || ''` - if user is null, empty string is used, which might cause issues downstream. + +--- + +## Recommendations + +1. **Fix SQL column names immediately** - This is breaking production +2. **Use cart prices instead of DB prices** - Critical for financial accuracy +3. **Add product availability checks** - Prevent ordering unavailable items +4. **Implement proper transaction boundaries** - Ensure data consistency +5. **Add stock/inventory validation** - Prevent overselling +6. **Fix status transition logic** - Make it consistent and clear +7. **Improve decimal precision** - Use appropriate scale for currency +8. **Add comprehensive validation** - Validate all inputs before order creation +9. **Implement locking mechanism** - Prevent race conditions +10. **Add unit tests** - Cover all edge cases and business rules + diff --git a/src/modules/orders/crone/order.crone.ts b/src/modules/orders/crone/order.crone.ts index 50042e8..6833b08 100644 --- a/src/modules/orders/crone/order.crone.ts +++ b/src/modules/orders/crone/order.crone.ts @@ -96,6 +96,7 @@ export class OrdersCrone { status: { $in: [ OrderStatus.SHIPPED, + OrderStatus.DELIVERED_TO_RECEPIENT, ], }, updatedAt: { $lte: cutoff }, diff --git a/src/modules/orders/interface/order.interface.ts b/src/modules/orders/interface/order.interface.ts index c785e22..01b62ee 100644 --- a/src/modules/orders/interface/order.interface.ts +++ b/src/modules/orders/interface/order.interface.ts @@ -16,7 +16,7 @@ export enum OrderStatus { PENDING_PAYMENT = 'pendingPayment', PAID = 'paid', PREPARING = 'preparing', - DELIVERED = 'delivered', + DELIVERED_TO_RECEPIENT = 'deliveredToRecepient', SHIPPED = 'shipped', COMPLETED = 'completed', CANCELED = 'canceled', diff --git a/src/modules/orders/listeners/order.listeners.ts b/src/modules/orders/listeners/order.listeners.ts index 63db9e2..de68ca1 100644 --- a/src/modules/orders/listeners/order.listeners.ts +++ b/src/modules/orders/listeners/order.listeners.ts @@ -35,7 +35,7 @@ export class OrderListeners { [OrderStatus.PENDING_PAYMENT]: 'در انتظار پرداخت', [OrderStatus.PAID]: 'پرداخت شده', [OrderStatus.PREPARING]: 'در حال آماده‌سازی', - [OrderStatus.DELIVERED]: 'تحویل شده', + [OrderStatus.DELIVERED_TO_RECEPIENT]: 'تحویل به پیشخوان', [OrderStatus.SHIPPED]: 'ارسال شده', [OrderStatus.COMPLETED]: 'تکمیل شده', [OrderStatus.CANCELED]: 'لغو شده', diff --git a/src/modules/orders/providers/orders.service.ts b/src/modules/orders/providers/orders.service.ts index 8034c3e..e34f3de 100644 --- a/src/modules/orders/providers/orders.service.ts +++ b/src/modules/orders/providers/orders.service.ts @@ -44,8 +44,8 @@ export class OrdersService { private static readonly STATUS_TRANSITIONS: Record = { [OrderStatus.PENDING_PAYMENT]: [OrderStatus.PAID, OrderStatus.CANCELED, OrderStatus.PREPARING], [OrderStatus.PAID]: [OrderStatus.PREPARING, OrderStatus.CANCELED], - [OrderStatus.PREPARING]: [OrderStatus.DELIVERED, OrderStatus.SHIPPED, OrderStatus.CANCELED], - [OrderStatus.DELIVERED]: [OrderStatus.COMPLETED, OrderStatus.CANCELED], + [OrderStatus.PREPARING]: [OrderStatus.SHIPPED, OrderStatus.DELIVERED_TO_RECEPIENT, OrderStatus.CANCELED], + [OrderStatus.DELIVERED_TO_RECEPIENT]: [OrderStatus.COMPLETED, OrderStatus.CANCELED], [OrderStatus.SHIPPED]: [OrderStatus.COMPLETED, OrderStatus.CANCELED], [OrderStatus.COMPLETED]: [OrderStatus.CANCELED], [OrderStatus.CANCELED]: [], @@ -244,8 +244,8 @@ export class OrdersService { OrderStatusChangedEvent.name, new OrderStatusChangedEvent( orderId, - order.user?.id || '', - String(order?.orderNumber) || '', + order.user.id, + String(order.orderNumber), shopId, previousStatus, toStatus, @@ -297,14 +297,6 @@ export class OrdersService { ) { return false; } - if ( - from == OrderStatus.PREPARING && - to == OrderStatus.DELIVERED && - deliveryMethod !== DeliveryMethodEnum.DeliveryCourier && - deliveryMethod !== DeliveryMethodEnum.CustomerPickup - ) { - return false; - } if (paymentMethod === PaymentMethodEnum.Online) { if (to === OrderStatus.PREPARING && from !== OrderStatus.PAID) return false; @@ -393,7 +385,7 @@ export class OrdersService { $in: [ OrderStatus.PAID, OrderStatus.PREPARING, - OrderStatus.DELIVERED, + OrderStatus.DELIVERED_TO_RECEPIENT, OrderStatus.SHIPPED, OrderStatus.COMPLETED, ], @@ -411,7 +403,7 @@ export class OrdersService { ` SELECT COUNT(DISTINCT o.user_id) as count FROM orders o - WHERE o.restaurant_id = ? + WHERE o.shop_id = ? `, [shopId], ); @@ -423,7 +415,7 @@ export class OrdersService { SELECT COALESCE(SUM(p.amount), 0)::numeric as total FROM payments p INNER JOIN orders o ON p.order_id = o.id - WHERE o.restaurant_id = ? AND p.status = 'paid' + WHERE o.shop_id = ? AND p.status = 'paid' `, [shopId], ); @@ -467,7 +459,7 @@ export class OrdersService { COUNT(oi.id) as item_count FROM orders o LEFT JOIN order_items oi ON oi.order_id = o.id - WHERE o.restaurant_id = ? + WHERE o.shop_id = ? GROUP BY o.id, o.status, o.created_at ORDER BY o.created_at DESC LIMIT 10 @@ -492,7 +484,7 @@ export class OrdersService { INNER JOIN orders o ON oi.order_id = o.id INNER JOIN variants v ON oi.variant_id = v.id INNER JOIN products f ON v.product_id = f.id - WHERE o.restaurant_id = ? + WHERE o.shop_id = ? AND o.created_at >= ? AND o.created_at <= ? AND o.status NOT IN ('pendingPayment', 'canceled') diff --git a/src/modules/payments/interface/payment.ts b/src/modules/payments/interface/payment.ts index 6fd70c2..6dfb49f 100644 --- a/src/modules/payments/interface/payment.ts +++ b/src/modules/payments/interface/payment.ts @@ -29,5 +29,5 @@ export interface OrderPaymentContext { method: PaymentMethodEnum; gateway: PaymentGatewayEnum | null; merchantId: string | null; - restaurantDomain: string | null; + shopDomain: string | null; } diff --git a/src/modules/payments/services/payments.service.ts b/src/modules/payments/services/payments.service.ts index 0d0b0a5..e70137e 100644 --- a/src/modules/payments/services/payments.service.ts +++ b/src/modules/payments/services/payments.service.ts @@ -83,7 +83,7 @@ export class PaymentsService { method: pm.method, gateway: pm.gateway ?? null, merchantId: pm.merchantId ?? null, - restaurantDomain: pm.shop.domain ?? null, + shopDomain: pm.shop.domain ?? null, }; } @@ -170,7 +170,7 @@ export class PaymentsService { amount: ctx.amount, orderId: ctx.order.id, merchantId: ctx.merchantId!, - domain: ctx.restaurantDomain!, + domain: ctx.shopDomain!, }); payment.gateway = ctx.gateway; @@ -280,8 +280,8 @@ export class PaymentsService { payment.order.status = OrderStatus.CANCELED; } - async findOneOrFail(paymentId: string): Promise { - const payment=await this.paymentRepository.findOne({ id: paymentId }); + async findOneOrFail(paymentId: string): Promise { + const payment = await this.paymentRepository.findOne({ id: paymentId }); if (!payment) { throw new NotFoundException(PaymentMessage.PAYMENT_NOT_FOUND); }