add stock to variant

This commit is contained in:
2026-02-14 15:41:44 +03:30
parent ec828717b5
commit 942094118c
11 changed files with 52 additions and 292 deletions
-276
View File
@@ -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
@@ -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<void> { async addOrIncrementItem(cart: Cart, variantId: string, shopId: string, quantityToAdd: number): Promise<void> {
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) { if (index < 0) {
cart.items.push(this.createCartItem(variant, quantityToAdd)); cart.items.push(this.createCartItem(variant, quantityToAdd));
@@ -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<Variant> { async validateAndGetVariant(
const variant = await this.productService.findOrFailVariant(variantId) variantId: string,
shopId: string,
quantityToAdd: number,
existingQuantityInCart: number = 0,
): Promise<Variant> {
const variant = await this.productService.findOrFailVariant(variantId);
if (variant.product.shop.id !== shopId) { if (variant.product.shop.id !== shopId) {
throw new BadRequestException(CartMessage.PRODUCT_NOT_BELONGS_TO_SHOP); 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; return variant;
} }
@@ -20,7 +20,7 @@ import { Payment } from 'src/modules/payments/entities/payment.entity';
import { StatusTransitionRef } from '../interface/order.interface'; import { StatusTransitionRef } from '../interface/order.interface';
import { EventEmitter2 } from '@nestjs/event-emitter'; import { EventEmitter2 } from '@nestjs/event-emitter';
import { OrderCreatedEvent, OrderStatusChangedEvent } from '../events/order.events'; 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 { UserService } from 'src/modules/users/providers/user.service';
import { ShopService } from 'src/modules/shops/providers/shops.service'; import { ShopService } from 'src/modules/shops/providers/shops.service';
import { DeliveryService } from 'src/modules/delivery/providers/delivery.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); 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({ orderItemsData.push({
variant, variant,
quantity: cartItem.quantity, quantity: cartItem.quantity,
@@ -353,7 +353,7 @@ export class PaymentsService {
if (shopId) { if (shopId) {
params.push(shopId); params.push(shopId);
restaurantFilter = `AND o.restaurant_id = ?`; restaurantFilter = `AND o.shop_id = ?`;
} }
// MikroORM uses ? placeholders for parameter binding // MikroORM uses ? placeholders for parameter binding
@@ -31,8 +31,14 @@ export class CreateVariantDto {
@Min(1000) @Min(1000)
@IsNotEmpty() @IsNotEmpty()
@ApiProperty() @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 { export class CreateProductDto {
@@ -61,6 +67,7 @@ export class CreateProductDto {
id: '', id: '',
value: 'قرمز', value: 'قرمز',
price: 100000, price: 100000,
stock: 10,
}] }]
}) })
variants: CreateVariantDto[] variants: CreateVariantDto[]
@@ -14,4 +14,7 @@ export class Variant extends BaseEntity {
@Property({ type: 'decimal', precision: 10, scale: 0, nullable: true }) @Property({ type: 'decimal', precision: 10, scale: 0, nullable: true })
price?: number; price?: number;
/** Available quantity; null means unlimited */
@Property({ type: 'int', nullable: true, default: null })
stock: number | null = null;
} }
@@ -59,9 +59,10 @@ export class ProductService {
product, product,
value: variant.value, value: variant.value,
price: variant.price, price: variant.price,
}) stock: variant.stock ?? null,
});
em.persist(variantRecord) em.persist(variantRecord);
} }
await em.flush(); await em.flush();
@@ -136,7 +137,7 @@ export class ProductService {
const updatedVariantIds = new Set<string>(); const updatedVariantIds = new Set<string>();
for (const variant of variants) { for (const variant of variants) {
const { id, value, price } = variant; const { id, value, price, stock } = variant;
if (id) { if (id) {
// Update existing variant // Update existing variant
@@ -146,7 +147,7 @@ export class ProductService {
} }
// Update the variant fields // Update the variant fields
this.em.assign(existingVariant, { value, price }); this.em.assign(existingVariant, { value, price, stock: stock ?? null });
updatedVariantIds.add(id); updatedVariantIds.add(id);
} else { } else {
// Create new variant // Create new variant
@@ -154,6 +155,7 @@ export class ProductService {
product, product,
value, value,
price, price,
stock: stock ?? null,
}); });
this.em.persist(variantRecord); this.em.persist(variantRecord);
} }
+1 -1
View File
@@ -10,7 +10,7 @@ export interface ProductData {
discount?: number; discount?: number;
score?: number; score?: number;
isSpecialOffer?: boolean; isSpecialOffer?: boolean;
varrinats: { price: number, value: string | null }[] varrinats: { price: number; value: string | null; stock?: number | null }[]
attribute?: null | string attribute?: null | string
} }
+2 -2
View File
@@ -23,7 +23,7 @@ export const shopsData: ShopData[] = [
{ {
name: 'ژیوان', name: 'ژیوان',
slug: 'zhivan', slug: 'zhivan',
domain: 'https://dkala-front.dev.danakcorp.com/zhivan', domain: 'https://dkala.danakcorp.com/zhivan',
isActive: true, isActive: true,
phone: '', phone: '',
logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1638870352717_61a3661f37a0d33354a6d210.png', logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1638870352717_61a3661f37a0d33354a6d210.png',
@@ -42,7 +42,7 @@ export const shopsData: ShopData[] = [
{ {
name: 'بوته', name: 'بوته',
slug: 'boote', slug: 'boote',
domain: 'https://dkala-front.dev.danakcorp.com/boote', domain: 'https://dkala.danakcorp.com/boote',
isActive: true, isActive: true,
phone: '', phone: '',
logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1641199914860_61d2b72b2e8bd97126a70b5f.png', logo: 'https://dmenu.danakcorp.com/uploads/images/store/store_1641199914860_61d2b72b2e8bd97126a70b5f.png',
+1
View File
@@ -62,6 +62,7 @@ export class ProductsSeeder {
product, product,
value: variant.value ?? '', value: variant.value ?? '',
price: variant.price, price: variant.price,
stock: variant.stock ?? null,
}); });
em.persist(variantRecord); em.persist(variantRecord);
}); });