diff --git a/ORDER_MODULE_REVIEW.md b/ORDER_MODULE_REVIEW.md deleted file mode 100644 index 826699a..0000000 --- a/ORDER_MODULE_REVIEW.md +++ /dev/null @@ -1,276 +0,0 @@ -# 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/cart/providers/cart-item.service.ts b/src/modules/cart/providers/cart-item.service.ts index 338efd9..0e36157 100644 --- a/src/modules/cart/providers/cart-item.service.ts +++ b/src/modules/cart/providers/cart-item.service.ts @@ -57,12 +57,18 @@ export class CartItemService { } /** - * Add or increment item in cart + * Add or increment item in cart (validates stock against existing + new quantity) */ async addOrIncrementItem(cart: Cart, variantId: string, shopId: string, quantityToAdd: number): Promise { - const variant = await this.validationService.validateAndGetVariant(variantId, shopId, quantityToAdd); + const index = this.getItemIndex(cart, variantId); + const existingQuantityInCart = index >= 0 ? cart.items[index].quantity : 0; - const index = this.getItemIndex(cart, variant.id); + const variant = await this.validationService.validateAndGetVariant( + variantId, + shopId, + quantityToAdd, + existingQuantityInCart, + ); if (index < 0) { cart.items.push(this.createCartItem(variant, quantityToAdd)); diff --git a/src/modules/cart/providers/cart-validation.service.ts b/src/modules/cart/providers/cart-validation.service.ts index 3c06814..8a3c273 100644 --- a/src/modules/cart/providers/cart-validation.service.ts +++ b/src/modules/cart/providers/cart-validation.service.ts @@ -29,15 +29,28 @@ export class CartValidationService { ) { } /** - * Validate product exists and belongs to shop + * Validate variant exists, belongs to shop, and has sufficient stock. + * @param existingQuantityInCart - current quantity of this variant already in cart (for add/increment) */ - async validateAndGetVariant(variantId: string, shopId: string, quantity: number): Promise { - const variant = await this.productService.findOrFailVariant(variantId) + async validateAndGetVariant( + variantId: string, + shopId: string, + quantityToAdd: number, + existingQuantityInCart: number = 0, + ): Promise { + const variant = await this.productService.findOrFailVariant(variantId); if (variant.product.shop.id !== shopId) { throw new BadRequestException(CartMessage.PRODUCT_NOT_BELONGS_TO_SHOP); } + if (variant.stock != null) { + const totalRequested = existingQuantityInCart + quantityToAdd; + if (totalRequested > variant.stock) { + throw new BadRequestException(CartMessage.INSUFFICIENT_STOCK); + } + } + return variant; } diff --git a/src/modules/orders/providers/orders.service.ts b/src/modules/orders/providers/orders.service.ts index dbd6c00..0c4df99 100644 --- a/src/modules/orders/providers/orders.service.ts +++ b/src/modules/orders/providers/orders.service.ts @@ -20,7 +20,7 @@ import { Payment } from 'src/modules/payments/entities/payment.entity'; import { StatusTransitionRef } from '../interface/order.interface'; import { EventEmitter2 } from '@nestjs/event-emitter'; import { OrderCreatedEvent, OrderStatusChangedEvent } from '../events/order.events'; -import { OrderMessage } from 'src/common/enums/message.enum'; +import { CartMessage, OrderMessage } from 'src/common/enums/message.enum'; import { UserService } from 'src/modules/users/providers/user.service'; import { ShopService } from 'src/modules/shops/providers/shops.service'; import { DeliveryService } from 'src/modules/delivery/providers/delivery.service'; @@ -370,6 +370,10 @@ export class OrdersService { throw new BadRequestException(OrderMessage.PRODUCT_NOT_BELONGS_TO_SHOP); } + if (variant.stock != null && cartItem.quantity > variant.stock) { + throw new BadRequestException(CartMessage.INSUFFICIENT_STOCK); + } + orderItemsData.push({ variant, quantity: cartItem.quantity, diff --git a/src/modules/payments/services/payments.service.ts b/src/modules/payments/services/payments.service.ts index e93de5c..f8fa639 100644 --- a/src/modules/payments/services/payments.service.ts +++ b/src/modules/payments/services/payments.service.ts @@ -353,7 +353,7 @@ export class PaymentsService { if (shopId) { params.push(shopId); - restaurantFilter = `AND o.restaurant_id = ?`; + restaurantFilter = `AND o.shop_id = ?`; } // MikroORM uses ? placeholders for parameter binding diff --git a/src/modules/products/dto/create-product.dto.ts b/src/modules/products/dto/create-product.dto.ts index 604beb3..c80c476 100644 --- a/src/modules/products/dto/create-product.dto.ts +++ b/src/modules/products/dto/create-product.dto.ts @@ -31,8 +31,14 @@ export class CreateVariantDto { @Min(1000) @IsNotEmpty() @ApiProperty() - price: number + price: number; + @IsOptional() + @IsInt() + @Min(0) + @Type(() => Number) + @ApiPropertyOptional({ description: 'Available stock; omit or null for unlimited', example: 10 }) + stock?: number | null; } export class CreateProductDto { @@ -61,6 +67,7 @@ export class CreateProductDto { id: '', value: 'قرمز', price: 100000, + stock: 10, }] }) variants: CreateVariantDto[] diff --git a/src/modules/products/entities/variant.entity.ts b/src/modules/products/entities/variant.entity.ts index cc44641..84087bc 100644 --- a/src/modules/products/entities/variant.entity.ts +++ b/src/modules/products/entities/variant.entity.ts @@ -14,4 +14,7 @@ export class Variant extends BaseEntity { @Property({ type: 'decimal', precision: 10, scale: 0, nullable: true }) price?: number; + /** Available quantity; null means unlimited */ + @Property({ type: 'int', nullable: true, default: null }) + stock: number | null = null; } diff --git a/src/modules/products/providers/product.service.ts b/src/modules/products/providers/product.service.ts index 5557dac..d1c5197 100644 --- a/src/modules/products/providers/product.service.ts +++ b/src/modules/products/providers/product.service.ts @@ -59,9 +59,10 @@ export class ProductService { product, value: variant.value, price: variant.price, - }) + stock: variant.stock ?? null, + }); - em.persist(variantRecord) + em.persist(variantRecord); } await em.flush(); @@ -136,7 +137,7 @@ export class ProductService { const updatedVariantIds = new Set(); for (const variant of variants) { - const { id, value, price } = variant; + const { id, value, price, stock } = variant; if (id) { // Update existing variant @@ -146,7 +147,7 @@ export class ProductService { } // Update the variant fields - this.em.assign(existingVariant, { value, price }); + this.em.assign(existingVariant, { value, price, stock: stock ?? null }); updatedVariantIds.add(id); } else { // Create new variant @@ -154,6 +155,7 @@ export class ProductService { product, value, price, + stock: stock ?? null, }); this.em.persist(variantRecord); } diff --git a/src/seeders/data/products.data.ts b/src/seeders/data/products.data.ts index 5839e18..1bf2eb4 100644 --- a/src/seeders/data/products.data.ts +++ b/src/seeders/data/products.data.ts @@ -10,7 +10,7 @@ export interface ProductData { discount?: number; score?: number; isSpecialOffer?: boolean; - varrinats: { price: number, value: string | null }[] + varrinats: { price: number; value: string | null; stock?: number | null }[] attribute?: null | string } diff --git a/src/seeders/data/shops.data.ts b/src/seeders/data/shops.data.ts index b83e766..81230b2 100644 --- a/src/seeders/data/shops.data.ts +++ b/src/seeders/data/shops.data.ts @@ -23,7 +23,7 @@ export const shopsData: ShopData[] = [ { name: 'ژیوان', slug: 'zhivan', - domain: 'https://dkala-front.dev.danakcorp.com/zhivan', + domain: 'https://dkala.danakcorp.com/zhivan', isActive: true, phone: '', logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1638870352717_61a3661f37a0d33354a6d210.png', @@ -42,7 +42,7 @@ export const shopsData: ShopData[] = [ { name: 'بوته', slug: 'boote', - domain: 'https://dkala-front.dev.danakcorp.com/boote', + domain: 'https://dkala.danakcorp.com/boote', isActive: true, phone: '', logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1641199914860_61d2b72b2e8bd97126a70b5f.png', diff --git a/src/seeders/products.seeder.ts b/src/seeders/products.seeder.ts index 7e12976..04d2caf 100644 --- a/src/seeders/products.seeder.ts +++ b/src/seeders/products.seeder.ts @@ -62,6 +62,7 @@ export class ProductsSeeder { product, value: variant.value ?? '', price: variant.price, + stock: variant.stock ?? null, }); em.persist(variantRecord); });