fix bugs in order
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -96,6 +96,7 @@ export class OrdersCrone {
|
||||
status: {
|
||||
$in: [
|
||||
OrderStatus.SHIPPED,
|
||||
OrderStatus.DELIVERED_TO_RECEPIENT,
|
||||
],
|
||||
},
|
||||
updatedAt: { $lte: cutoff },
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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]: 'لغو شده',
|
||||
|
||||
@@ -44,8 +44,8 @@ export class OrdersService {
|
||||
private static readonly STATUS_TRANSITIONS: Record<OrderStatus, readonly OrderStatus[]> = {
|
||||
[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')
|
||||
|
||||
@@ -29,5 +29,5 @@ export interface OrderPaymentContext {
|
||||
method: PaymentMethodEnum;
|
||||
gateway: PaymentGatewayEnum | null;
|
||||
merchantId: string | null;
|
||||
restaurantDomain: string | null;
|
||||
shopDomain: string | null;
|
||||
}
|
||||
|
||||
@@ -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<Payment> {
|
||||
const payment=await this.paymentRepository.findOne({ id: paymentId });
|
||||
async findOneOrFail(paymentId: string): Promise<Payment> {
|
||||
const payment = await this.paymentRepository.findOne({ id: paymentId });
|
||||
if (!payment) {
|
||||
throw new NotFoundException(PaymentMessage.PAYMENT_NOT_FOUND);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user