chore(promotion): Improve performances [1] (#12129)

**What**
Reduce database queries when possible and use proper data structure and aggregation when possible in order to reduce performance decrease overall

.changeset/nine-otters-carry.md

@@ -0,0 +1,5 @@
+---
+"@medusajs/promotion": patch
+---
+
+chore(promotion): Improve performances [1]

packages/modules/promotion/src/services/promotion-module.ts

@@ -144,24 +144,25 @@ export default class PromotionModuleService
     config?: FindConfig<PromotionDTO>,
     sharedContext?: Context
   ): Promise<PromotionDTO[]> {
+    // Ensure we share the same now date across all filters
+    const now = new Date()
     const activeFilters = {
+      status: PromotionStatus.ACTIVE,
       $or: [
         {
-          status: PromotionStatus.ACTIVE,
           campaign_id: null,
           ...filters,
         },
         {
-          status: PromotionStatus.ACTIVE,
           ...filters,
           campaign: {
             ...filters?.campaign,
             $and: [
               {
-                $or: [{ starts_at: null }, { starts_at: { $lte: new Date() } }],
+                $or: [{ starts_at: null }, { starts_at: { $lte: now } }],
               },
               {
-                $or: [{ ends_at: null }, { ends_at: { $gt: new Date() } }],
+                $or: [{ ends_at: null }, { ends_at: { $gt: now } }],
               },
             ],
           },
@@ -172,12 +173,10 @@ export default class PromotionModuleService
     return this.listPromotions(activeFilters, config, sharedContext)
   }
 
-  @InjectManager()
+  @InjectTransactionManager()
   async registerUsage(
     computedActions: PromotionTypes.UsageComputedActions[],
-    @MedusaContext()
+    @MedusaContext() sharedContext: Context = {}
-    @MedusaContext()
-    sharedContext: Context = {}
   ): Promise<void> {
     const promotionCodes = computedActions
       .map((computedAction) => computedAction.code)
@@ -225,16 +224,19 @@ export default class PromotionModuleService
           continue
         }
 
-        campaignBudgetData.used = MathBN.add(
+        // Calculate the new budget value
+        const newUsedValue = MathBN.add(
           campaignBudgetData.used ?? 0,
           computedAction.amount
         )
 
         if (
           campaignBudget.limit &&
-          MathBN.gt(campaignBudgetData.used, campaignBudget.limit)
+          MathBN.gt(newUsedValue, campaignBudget.limit)
         ) {
           continue
+        } else {
+          campaignBudgetData.used = newUsedValue
         }
 
         campaignBudgetMap.set(campaignBudget.id, campaignBudgetData)
@@ -248,25 +250,30 @@ export default class PromotionModuleService
           continue
         }
 
-        const campaignBudgetData = {
+        const newUsedValue = MathBN.add(campaignBudget.used ?? 0, 1)
-          id: campaignBudget.id,
-          used: MathBN.add(campaignBudget.used ?? 0, 1),
-        }
 
+        // Check if it exceeds the limit and cap it if necessary
         if (
           campaignBudget.limit &&
-          MathBN.gt(campaignBudgetData.used, campaignBudget.limit)
+          MathBN.gt(newUsedValue, campaignBudget.limit)
         ) {
-          continue
+          campaignBudgetMap.set(campaignBudget.id, {
+            id: campaignBudget.id,
+            used: campaignBudget.limit,
+          })
+        } else {
+          campaignBudgetMap.set(campaignBudget.id, {
+            id: campaignBudget.id,
+            used: newUsedValue,
+          })
         }
 
-        campaignBudgetMap.set(campaignBudget.id, campaignBudgetData)
-
         promotionCodeUsageMap.set(promotion.code!, true)
       }
+    }
 
+    if (campaignBudgetMap.size > 0) {
       const campaignBudgetsData: UpdateCampaignBudgetDTO[] = []
-
       for (const [_, campaignBudgetData] of campaignBudgetMap) {
         campaignBudgetsData.push(campaignBudgetData)
       }
@@ -278,7 +285,7 @@ export default class PromotionModuleService
     }
   }
 
-  @InjectManager()
+  @InjectTransactionManager()
   async revertUsage(
     computedActions: PromotionTypes.UsageComputedActions[],
     @MedusaContext() sharedContext: Context = {}
@@ -329,11 +336,13 @@ export default class PromotionModuleService
           continue
         }
 
-        campaignBudgetData.used = MathBN.sub(
+        // Calculate new used value and ensure it doesn't go below 0
+        const newUsedValue = MathBN.sub(
           campaignBudgetData.used ?? 0,
           computedAction.amount
         )
 
+        campaignBudgetData.used = MathBN.lt(newUsedValue, 0) ? 0 : newUsedValue
         campaignBudgetMap.set(campaignBudget.id, campaignBudgetData)
       }
 
@@ -345,16 +354,21 @@ export default class PromotionModuleService
           continue
         }
 
+        // Calculate new used value and ensure it doesn't go below 0
+        const newUsedValue = MathBN.sub(campaignBudget.used ?? 0, 1)
+        const usedValue = MathBN.lt(newUsedValue, 0) ? 0 : newUsedValue
+
         campaignBudgetMap.set(campaignBudget.id, {
           id: campaignBudget.id,
-          used: MathBN.sub(campaignBudget.used ?? 0, 1),
+          used: usedValue,
         })
 
         promotionCodeUsageMap.set(promotion.code!, true)
       }
+    }
 
+    if (campaignBudgetMap.size > 0) {
       const campaignBudgetsData: UpdateCampaignBudgetDTO[] = []
-
       for (const [_, campaignBudgetData] of campaignBudgetMap) {
         campaignBudgetsData.push(campaignBudgetData)
       }
@@ -377,23 +391,48 @@ export default class PromotionModuleService
     const computedActions: PromotionTypes.ComputeActions[] = []
     const { items = [], shipping_methods: shippingMethods = [] } =
       applicationContext
-    const appliedItemCodes: string[] = []
+
-    const appliedShippingCodes: string[] = []
     const codeAdjustmentMap = new Map<
       string,
-      PromotionTypes.ComputeActionAdjustmentLine[]
+      {
+        items: PromotionTypes.ComputeActionAdjustmentLine[]
+        shipping: PromotionTypes.ComputeActionAdjustmentLine[]
+      }
     >()
+
+    // Pre-process items and shipping methods to build adjustment map efficiently
+    for (const item of items) {
+      if (!item.adjustments?.length) continue
+
+      for (const adjustment of item.adjustments) {
+        if (!isString(adjustment.code)) continue
+
+        if (!codeAdjustmentMap.has(adjustment.code)) {
+          codeAdjustmentMap.set(adjustment.code, { items: [], shipping: [] })
+        }
+
+        codeAdjustmentMap.get(adjustment.code)!.items.push(adjustment)
+      }
+    }
+
+    for (const shippingMethod of shippingMethods) {
+      if (!shippingMethod.adjustments?.length) continue
+
+      for (const adjustment of shippingMethod.adjustments) {
+        if (!isString(adjustment.code)) continue
+
+        if (!codeAdjustmentMap.has(adjustment.code)) {
+          codeAdjustmentMap.set(adjustment.code, { items: [], shipping: [] })
+        }
+
+        codeAdjustmentMap.get(adjustment.code)!.shipping.push(adjustment)
+      }
+    }
+
+    const appliedCodes = Array.from(codeAdjustmentMap.keys())
+
     const methodIdPromoValueMap = new Map<string, number>()
-    // Keeps a map of all elgible items in the buy section and its eligible quantity
+
-    const eligibleBuyItemMap = new Map<
-      string,
-      ComputeActionUtils.EligibleItem[]
-    >()
-    // Keeps a map of all elgible items in the target section and its eligible quantity
-    const eligibleTargetItemMap = new Map<
-      string,
-      ComputeActionUtils.EligibleItem[]
-    >()
     const automaticPromotions = preventAutoPromotions
       ? []
       : await this.listActivePromotions(
@@ -402,48 +441,17 @@ export default class PromotionModuleService
           sharedContext
         )
 
-    // Promotions we need to apply includes all the codes that are passed as an argument
-    // to this method, along with any automatic promotions that can be applied to the context
     const automaticPromotionCodes = automaticPromotions.map((p) => p.code!)
     const promotionCodesToApply = [
       ...promotionCodes,
       ...automaticPromotionCodes,
+      ...appliedCodes,
     ]
 
-    items.forEach((item) => {
+    const uniquePromotionCodes = Array.from(new Set(promotionCodesToApply))
-      item.adjustments?.forEach((adjustment) => {
-        if (isString(adjustment.code)) {
-          const adjustments = codeAdjustmentMap.get(adjustment.code) || []
-
-          adjustments.push(adjustment)
-
-          codeAdjustmentMap.set(adjustment.code, adjustments)
-          appliedItemCodes.push(adjustment.code)
-        }
-      })
-    })
-
-    shippingMethods.forEach((shippingMethod) => {
-      shippingMethod.adjustments?.forEach((adjustment) => {
-        if (isString(adjustment.code)) {
-          const adjustments = codeAdjustmentMap.get(adjustment.code) || []
-
-          adjustments.push(adjustment)
-
-          codeAdjustmentMap.set(adjustment.code, adjustments)
-          appliedShippingCodes.push(adjustment.code)
-        }
-      })
-    })
 
     const promotions = await this.listActivePromotions(
-      {
+      { code: uniquePromotionCodes },
-        code: [
-          ...promotionCodesToApply,
-          ...appliedItemCodes,
-          ...appliedShippingCodes,
-        ],
-      },
       {
         take: null,
         order: { application_method: { value: "DESC" } },
@@ -458,42 +466,51 @@ export default class PromotionModuleService
           "campaign",
           "campaign.budget",
         ],
-      }
+      },
+      sharedContext
     )
 
     const existingPromotionsMap = new Map<string, PromotionTypes.PromotionDTO>(
       promotions.map((promotion) => [promotion.code!, promotion])
     )
 
-    // We look at any existing promo codes applied in the context and recommend
+    for (const [code, adjustments] of codeAdjustmentMap.entries()) {
-    // them to be removed to start calculations from the beginning and refresh
+      for (const adjustment of adjustments.items) {
-    // the adjustments if they are requested to be applied again
-    const appliedCodes = [...appliedShippingCodes, ...appliedItemCodes]
-
-    for (const appliedCode of appliedCodes) {
-      const adjustments = codeAdjustmentMap.get(appliedCode) || []
-      const action = appliedShippingCodes.includes(appliedCode)
-        ? ComputedActions.REMOVE_SHIPPING_METHOD_ADJUSTMENT
-        : ComputedActions.REMOVE_ITEM_ADJUSTMENT
-
-      adjustments.forEach((adjustment) =>
         computedActions.push({
-          action,
+          action: ComputedActions.REMOVE_ITEM_ADJUSTMENT,
           adjustment_id: adjustment.id,
-          code: appliedCode,
+          code,
         })
-      )
+      }
+
+      for (const adjustment of adjustments.shipping) {
+        computedActions.push({
+          action: ComputedActions.REMOVE_SHIPPING_METHOD_ADJUSTMENT,
+          adjustment_id: adjustment.id,
+          code,
+        })
+      }
     }
 
-    // We sort the promo codes to apply with buy get type first as they
+    const sortedPromotionsToApply = promotions
-    // are likely to be most valuable.
+      .filter(
-    const sortedPermissionsToApply = promotions
+        (p) =>
-      .filter((p) => promotionCodesToApply.includes(p.code!))
+          promotionCodes.includes(p.code!) ||
+          automaticPromotionCodes.includes(p.code!)
+      )
       .sort(ComputeActionUtils.sortByBuyGetType)
 
-    for (const promotionToApply of sortedPermissionsToApply) {
+    const eligibleBuyItemMap = new Map<
-      const promotion = existingPromotionsMap.get(promotionToApply.code!)!
+      string,
+      ComputeActionUtils.EligibleItem[]
+    >()
+    const eligibleTargetItemMap = new Map<
+      string,
+      ComputeActionUtils.EligibleItem[]
+    >()
 
+    for (const promotionToApply of sortedPromotionsToApply) {
+      const promotion = existingPromotionsMap.get(promotionToApply.code!)!
       const {
         application_method: applicationMethod,
         rules: promotionRules = [],
@@ -524,9 +541,7 @@ export default class PromotionModuleService
           )
 
         computedActions.push(...computedActionsForItems)
-      }
+      } else if (promotion.type === PromotionType.STANDARD) {
-
-      if (promotion.type === PromotionType.STANDARD) {
         const isTargetOrder =
           applicationMethod.target_type === ApplicationMethodTargetType.ORDER
         const isTargetItems =
@@ -620,11 +635,20 @@ export default class PromotionModuleService
     const promotionsData: CreatePromotionDTO[] = []
     const applicationMethodsData: CreateApplicationMethodDTO[] = []
     const campaignsData: CreateCampaignDTO[] = []
-    const existingCampaigns = await this.campaignService_.list(
+
-      { id: data.map((d) => d.campaign_id).filter((id) => isString(id)) },
+    const campaignIds = data
-      { relations: ["budget"] },
+      .filter((d) => d.campaign_id)
-      sharedContext
+      .map((d) => d.campaign_id)
-    )
+      .filter((id): id is string => isString(id))
+
+    const existingCampaigns =
+      campaignIds.length > 0
+        ? await this.campaignService_.list(
+            { id: campaignIds },
+            { relations: ["budget"] },
+            sharedContext
+          )
+        : []
 
     const promotionCodeApplicationMethodDataMap = new Map<
       string,
@@ -672,7 +696,6 @@ export default class PromotionModuleService
 
       if (!campaignData && !campaignId) {
         promotionsData.push({ ...promotionData })
-
         continue
       }
 
@@ -783,24 +806,28 @@ export default class PromotionModuleService
         }
       }
 
-      await this.createPromotionRulesAndValues_(
+      if (promotionCodeRulesDataMap.has(promotion.code)) {
-        promotionCodeRulesDataMap.get(promotion.code) || [],
+        await this.createPromotionRulesAndValues_(
-        "promotions",
+          promotionCodeRulesDataMap.get(promotion.code) || [],
-        promotion,
+          "promotions",
-        sharedContext
+          promotion,
-      )
+          sharedContext
+        )
+      }
     }
 
     const createdApplicationMethods =
-      await this.applicationMethodService_.create(
+      applicationMethodsData.length > 0
-        applicationMethodsData,
+        ? await this.applicationMethodService_.create(
-        sharedContext
+            applicationMethodsData,
-      )
+            sharedContext
+          )
+        : []
 
-    const createdCampaigns = await this.createCampaigns(
+    const createdCampaigns =
-      campaignsData,
+      campaignsData.length > 0
-      sharedContext
+        ? await this.createCampaigns(campaignsData, sharedContext)
-    )
+        : []
 
     for (const campaignData of campaignsData) {
       const promotions = campaignData.promotions
@@ -808,30 +835,36 @@ export default class PromotionModuleService
         (c) => c.campaign_identifier === campaignData.campaign_identifier
       )
 
-      if (!campaign || !promotions || !promotions.length) {
+      if (campaign && promotions && promotions.length) {
-        continue
+        await this.addPromotionsToCampaign(
+          { id: campaign.id, promotion_ids: promotions.map((p) => p.id) },
+          sharedContext
+        )
       }
-
-      await this.addPromotionsToCampaign(
-        { id: campaign.id, promotion_ids: promotions.map((p) => p.id) },
-        sharedContext
-      )
     }
 
     for (const applicationMethod of createdApplicationMethods) {
-      await this.createPromotionRulesAndValues_(
+      const targetRules = methodTargetRulesMap.get(
-        methodTargetRulesMap.get(applicationMethod.promotion.id) || [],
+        applicationMethod.promotion.id
-        "method_target_rules",
-        applicationMethod,
-        sharedContext
       )
+      if (targetRules && targetRules.length > 0) {
+        await this.createPromotionRulesAndValues_(
+          targetRules,
+          "method_target_rules",
+          applicationMethod,
+          sharedContext
+        )
+      }
 
-      await this.createPromotionRulesAndValues_(
+      const buyRules = methodBuyRulesMap.get(applicationMethod.promotion.id)
-        methodBuyRulesMap.get(applicationMethod.promotion.id) || [],
+      if (buyRules && buyRules.length > 0) {
-        "method_buy_rules",
+        await this.createPromotionRulesAndValues_(
-        applicationMethod,
+          buyRules,
-        sharedContext
+          "method_buy_rules",
-      )
+          applicationMethod,
+          sharedContext
+        )
+      }
     }
 
     return createdPromotions

packages/modules/promotion/src/utils/compute-actions/buy-get.ts

@@ -50,6 +50,17 @@ export function getComputedActionsForBuyGet(
 ): PromotionTypes.ComputeActions[] {
   const computedActions: PromotionTypes.ComputeActions[] = []
 
+  if (!itemsContext) {
+    throw new MedusaError(
+      MedusaError.Types.INVALID_DATA,
+      `"items" should be present as an array in the context to compute actions`
+    )
+  }
+
+  if (!itemsContext?.length) {
+    return computedActions
+  }
+
   const minimumBuyQuantity = MathBN.convert(
     promotion.application_method?.buy_rules_min_quantity ?? 0
   )
@@ -58,11 +69,11 @@ export function getComputedActionsForBuyGet(
     itemsContext.map((i) => [i.id, i])
   )
 
-  if (!itemsContext) {
+  if (
-    throw new MedusaError(
+    MathBN.lte(minimumBuyQuantity, 0) ||
-      MedusaError.Types.INVALID_DATA,
+    !promotion.application_method?.buy_rules?.length
-      `"items" should be present as an array in the context to compute actions`
+  ) {
-    )
+    return computedActions
   }
 
   const eligibleBuyItems = filterItemsByPromotionRules(
@@ -70,6 +81,10 @@ export function getComputedActionsForBuyGet(
     promotion.application_method?.buy_rules
   )
 
+  if (!eligibleBuyItems.length) {
+    return computedActions
+  }
+
   const eligibleBuyItemQuantity = MathBN.sum(
     ...eligibleBuyItems.map((item) => item.quantity)
   )
@@ -79,9 +94,12 @@ export function getComputedActionsForBuyGet(
     does not match up to the minimum buy quantity set on the promotion, return early.
   */
   if (MathBN.gt(minimumBuyQuantity, eligibleBuyItemQuantity)) {
-    return []
+    return computedActions
   }
 
+  const eligibleItemsByPromotion: EligibleItem[] = []
+  let accumulatedQuantity = MathBN.convert(0)
+
   /*
     Eligibility of a BuyGet promotion can span across line items. Once an item has been chosen
     as eligible, we can't use this item or its partial remaining quantity when we apply the promotion on
@@ -89,35 +107,19 @@ export function getComputedActionsForBuyGet(
 
     We build the map here to use when we apply promotions on the target items.
   */
+
   for (const eligibleBuyItem of eligibleBuyItems) {
-    const eligibleItemsByPromotion =
-      eligibleBuyItemMap.get(promotion.code!) || []
-
-    const accumulatedQuantity = eligibleItemsByPromotion.reduce(
-      (acc, item) => MathBN.sum(acc, item.quantity),
-      MathBN.convert(0)
-    )
-
-    // If we have reached the minimum buy quantity from the eligible items for this promotion,
-    // we can break early and continue to applying the target items
     if (MathBN.gte(accumulatedQuantity, minimumBuyQuantity)) {
       break
     }
 
-    const eligibleQuantity = MathBN.sum(
-      ...eligibleItemsByPromotion
-        .filter((buy) => buy.item_id === eligibleBuyItem.id)
-        .map((b) => b.quantity)
-    )
-
     const reservableQuantity = MathBN.min(
       eligibleBuyItem.quantity,
-      MathBN.sub(minimumBuyQuantity, eligibleQuantity)
+      MathBN.sub(minimumBuyQuantity, accumulatedQuantity)
     )
 
-    // If we have reached the required minimum quantity, we break the loop early
     if (MathBN.lte(reservableQuantity, 0)) {
-      break
+      continue
     }
 
     eligibleItemsByPromotion.push({
@@ -128,93 +130,126 @@ export function getComputedActionsForBuyGet(
       ).toNumber(),
     })
 
-    eligibleBuyItemMap.set(promotion.code!, eligibleItemsByPromotion)
+    accumulatedQuantity = MathBN.add(accumulatedQuantity, reservableQuantity)
   }
 
+  // Store the eligible buy items for this promotion code in the map
+  eligibleBuyItemMap.set(promotion.code!, eligibleItemsByPromotion)
+
+  // If we couldn't accumulate enough items to meet the minimum buy quantity, return early
+  if (MathBN.lt(accumulatedQuantity, minimumBuyQuantity)) {
+    return computedActions
+  }
+
+  // Get the number of target items that should receive the discount
+  const targetQuantity = MathBN.convert(
+    promotion.application_method?.apply_to_quantity ?? 0
+  )
+
+  // If no target quantity is specified, return early
+  if (MathBN.lte(targetQuantity, 0)) {
+    return computedActions
+  }
+
+  // Find all items that match the target rules criteria
   const eligibleTargetItems = filterItemsByPromotionRules(
     itemsContext,
     promotion.application_method?.target_rules
   )
 
-  const targetQuantity = MathBN.convert(
+  // If no items match the target rules, return early
-    promotion.application_method?.apply_to_quantity ?? 0
+  if (!eligibleTargetItems.length) {
-  )
+    return computedActions
+  }
 
-  /*
+  // Track quantities of items that can't be used as targets because they were used in buy rules
-    In this loop, we build a map of eligible target items and quantity applicable to these items.
+  const inapplicableQuantityMap = new Map<string, BigNumberInput>()
 
-    Here we remove the quantity we used previously to identify eligible buy items
+  // Build map of quantities that are ineligible as targets because they were used to satisfy buy rules
-    from the eligible target items.
+  for (const buyItem of eligibleItemsByPromotion) {
-    
+    const currentValue =
-    This is done to prevent applying promotion to the same item we use to qualify the buy rules.
+      inapplicableQuantityMap.get(buyItem.item_id) || MathBN.convert(0)
-  */
+    inapplicableQuantityMap.set(
-  for (const eligibleTargetItem of eligibleTargetItems) {
+      buyItem.item_id,
-    const inapplicableQuantity = MathBN.sum(
+      MathBN.add(currentValue, buyItem.quantity)
-      ...Array.from(eligibleBuyItemMap.values())
-        .flat(1)
-        .filter((buy) => buy.item_id === eligibleTargetItem.id)
-        .map((b) => b.quantity)
     )
+  }
 
+  // Track items eligible for receiving the discount and total quantity that can be discounted
+  const targetItemsByPromotion: EligibleItem[] = []
+  let targetableQuantity = MathBN.convert(0)
+
+  // Find items eligible for discount, excluding quantities used in buy rules
+  for (const eligibleTargetItem of eligibleTargetItems) {
+    // Calculate how much of this item's quantity can receive the discount
+    const inapplicableQuantity =
+      inapplicableQuantityMap.get(eligibleTargetItem.id) || MathBN.convert(0)
     const applicableQuantity = MathBN.sub(
       eligibleTargetItem.quantity,
       inapplicableQuantity
     )
 
-    const fulfillableQuantity = MathBN.min(targetQuantity, applicableQuantity)
+    if (MathBN.lte(applicableQuantity, 0)) {
+      continue
+    }
+
+    // Calculate how many more items we need to fulfill target quantity
+    const remainingNeeded = MathBN.sub(targetQuantity, targetableQuantity)
+    const fulfillableQuantity = MathBN.min(remainingNeeded, applicableQuantity)
 
-    // If we have reached the required quantity to target from this item, we
-    // move on to the next item
     if (MathBN.lte(fulfillableQuantity, 0)) {
       continue
     }
 
-    const targetItemsByPromotion =
+    // Add this item to eligible targets
-      eligibleTargetItemMap.get(promotion.code!) || []
-
     targetItemsByPromotion.push({
       item_id: eligibleTargetItem.id,
-      quantity: MathBN.min(fulfillableQuantity, targetQuantity).toNumber(),
+      quantity: fulfillableQuantity.toNumber(),
     })
 
-    eligibleTargetItemMap.set(promotion.code!, targetItemsByPromotion)
+    targetableQuantity = MathBN.add(targetableQuantity, fulfillableQuantity)
+
+    // If we've found enough items to fulfill target quantity, stop looking
+    if (MathBN.gte(targetableQuantity, targetQuantity)) {
+      break
+    }
   }
 
-  const targetItemsByPromotion =
+  // Store eligible target items for this promotion
-    eligibleTargetItemMap.get(promotion.code!) || []
+  eligibleTargetItemMap.set(promotion.code!, targetItemsByPromotion)
 
-  const targettableQuantity = targetItemsByPromotion.reduce(
+  // If we couldn't find enough eligible target items, return early
-    (sum, item) => MathBN.sum(sum, item.quantity),
+  if (MathBN.lt(targetableQuantity, targetQuantity)) {
-    MathBN.convert(0)
+    return computedActions
-  )
-
-  // If we were able to match the target requirements across all line items, we return early.
-  if (MathBN.lt(targettableQuantity, targetQuantity)) {
-    return []
   }
 
+  // Track remaining quantity to apply discount to and get discount percentage
   let remainingQtyToApply = MathBN.convert(targetQuantity)
+  const applicablePercentage = promotion.application_method?.value ?? 100
 
+  // Apply discounts to eligible target items
   for (const targetItem of targetItemsByPromotion) {
+    if (MathBN.lte(remainingQtyToApply, 0)) {
+      break
+    }
+
     const item = itemsMap.get(targetItem.item_id)!
     const appliedPromoValue =
       methodIdPromoValueMap.get(item.id) ?? MathBN.convert(0)
     const multiplier = MathBN.min(targetItem.quantity, remainingQtyToApply)
-    const applicableAmount = MathBN.mult(
+
-      MathBN.div(item.subtotal, item.quantity),
+    // Calculate discount amount based on item price and applicable percentage
-      multiplier
+    const pricePerUnit = MathBN.div(item.subtotal, item.quantity)
-    )
+    const applicableAmount = MathBN.mult(pricePerUnit, multiplier)
-    const applicablePercentage = promotion.application_method?.value ?? 100
     const amount = MathBN.mult(applicableAmount, applicablePercentage).div(100)
 
-    const newRemainingQtyToApply = MathBN.sub(remainingQtyToApply, multiplier)
+    if (MathBN.lte(amount, 0)) {
-
+      continue
-    if (MathBN.lt(newRemainingQtyToApply, 0) || MathBN.lte(amount, 0)) {
-      break
-    } else {
-      remainingQtyToApply = newRemainingQtyToApply
     }
 
+    remainingQtyToApply = MathBN.sub(remainingQtyToApply, multiplier)
+
+    // Check if applying this discount would exceed promotion budget
     const budgetExceededAction = computeActionForBudgetExceeded(
       promotion,
       amount
@@ -222,15 +257,16 @@ export function getComputedActionsForBuyGet(
 
     if (budgetExceededAction) {
       computedActions.push(budgetExceededAction)
-
       continue
     }
 
+    // Track total promotional value applied to this item
     methodIdPromoValueMap.set(
       item.id,
       MathBN.add(appliedPromoValue, amount).toNumber()
     )
 
+    // Add computed discount action
     computedActions.push({
       action: ComputedActions.ADD_ITEM_ADJUSTMENT,
       item_id: item.id,

packages/modules/promotion/src/utils/compute-actions/line-items.ts

@@ -75,17 +75,35 @@ function applyPromotionToItems(
   allocationOverride?: ApplicationMethodAllocationValues
 ): PromotionTypes.ComputeActions[] {
   const { application_method: applicationMethod } = promotion
+
+  if (!applicationMethod) {
+    return []
+  }
+
   const allocation = applicationMethod?.allocation! || allocationOverride
-  const computedActions: PromotionTypes.ComputeActions[] = []
-  const applicableItems = getValidItemsForPromotion(items, promotion)
   const target = applicationMethod?.target_type
 
+  if (!items?.length || !target) {
+    return []
+  }
+
+  const computedActions: PromotionTypes.ComputeActions[] = []
+
+  const applicableItems = getValidItemsForPromotion(items, promotion)
+
+  if (!applicableItems.length) {
+    return computedActions
+  }
+
   const isTargetShippingMethod = target === TargetType.SHIPPING_METHODS
   const isTargetLineItems = target === TargetType.ITEMS
   const isTargetOrder = target === TargetType.ORDER
+  const promotionValue = applicationMethod?.value ?? 0
+  const maxQuantity = isTargetShippingMethod
+    ? 1
+    : applicationMethod?.max_quantity!
 
   let lineItemsTotal = MathBN.convert(0)
-
   if (allocation === ApplicationMethodAllocation.ACROSS) {
     lineItemsTotal = applicableItems.reduce(
       (acc, item) =>
@@ -95,22 +113,27 @@ function applyPromotionToItems(
         ),
       MathBN.convert(0)
     )
+
+    if (MathBN.lte(lineItemsTotal, 0)) {
+      return computedActions
+    }
   }
 
-  for (const item of applicableItems!) {
+  for (const item of applicableItems) {
-    const appliedPromoValue = appliedPromotionsMap.get(item.id) ?? 0
+    if (MathBN.lte(item.subtotal, 0)) {
-    const maxQuantity = isTargetShippingMethod
+      continue
-      ? 1
+    }
-      : applicationMethod?.max_quantity!
 
     if (isTargetShippingMethod) {
       item.quantity = 1
     }
 
+    const appliedPromoValue = appliedPromotionsMap.get(item.id) ?? 0
+
     const amount = calculateAdjustmentAmountFromPromotion(
       item,
       {
-        value: applicationMethod?.value ?? 0,
+        value: promotionValue,
         applied_value: appliedPromoValue,
         max_quantity: maxQuantity,
         type: applicationMethod?.type!,
@@ -130,7 +153,6 @@ function applyPromotionToItems(
 
     if (budgetExceededAction) {
       computedActions.push(budgetExceededAction)
-
       continue
     }
 
@@ -143,9 +165,7 @@ function applyPromotionToItems(
         amount,
         code: promotion.code!,
       })
-    }
+    } else if (isTargetShippingMethod) {
-
-    if (isTargetShippingMethod) {
       computedActions.push({
         action: ComputedActions.ADD_SHIPPING_METHOD_ADJUSTMENT,
         shipping_method_id: item.id,
@@ -164,24 +184,39 @@ function getValidItemsForPromotion(
     | PromotionTypes.ComputeActionContext[TargetType.SHIPPING_METHODS],
   promotion: PromotionTypes.PromotionDTO
 ) {
+  if (!items?.length || !promotion?.application_method) {
+    return []
+  }
+
   const isTargetShippingMethod =
     promotion.application_method?.target_type === TargetType.SHIPPING_METHODS
 
-  return (
+  const targetRules = promotion.application_method?.target_rules ?? []
-    items?.filter((item) => {
+  const hasTargetRules = targetRules.length > 0
-      const isSubtotalPresent = "subtotal" in item
-      const isQuantityPresent = "quantity" in item
-      const isPromotionApplicableToItem = areRulesValidForContext(
-        promotion?.application_method?.target_rules!,
-        item,
-        ApplicationMethodTargetType.ITEMS
-      )
 
-      return (
+  if (isTargetShippingMethod && !hasTargetRules) {
-        isPromotionApplicableToItem &&
+    return items.filter(
-        (isQuantityPresent || isTargetShippingMethod) &&
+      (item) => item && "subtotal" in item && MathBN.gt(item.subtotal, 0)
-        isSubtotalPresent
+    )
-      )
+  }
-    }) || []
+
-  )
+  return items.filter((item) => {
+    if (!item || !("subtotal" in item) || MathBN.lte(item.subtotal, 0)) {
+      return false
+    }
+
+    if (!isTargetShippingMethod && !("quantity" in item)) {
+      return false
+    }
+
+    if (!hasTargetRules) {
+      return true
+    }
+
+    return areRulesValidForContext(
+      promotion?.application_method?.target_rules!,
+      item,
+      ApplicationMethodTargetType.ITEMS
+    )
+  })
 }

packages/modules/promotion/src/utils/validations/promotion-rule.ts

@@ -55,20 +55,44 @@ export function areRulesValidForContext(
   context: Record<string, any>,
   contextScope: ApplicationMethodTargetTypeValues
 ): boolean {
-  return rules.every((rule) => {
+  if (!rules?.length) {
-    const validRuleValues = rule.values?.map((ruleValue) => ruleValue.value)
+    return true
+  }
 
-    if (!rule.attribute) {
+  const isItemScope = contextScope === ApplicationMethodTargetType.ITEMS
+  const isShippingScope =
+    contextScope === ApplicationMethodTargetType.SHIPPING_METHODS
+
+  return rules.every((rule) => {
+    if (!rule.attribute || !rule.values?.length) {
       return false
     }
 
-    const valuesToCheck = pickValueFromObject(
+    const validRuleValues = rule.values
-      fetchRuleAttributeForContext(rule.attribute, contextScope),
+      .filter((v) => isString(v.value))
-      context
+      .map((v) => v.value as string)
-    )
+
+    if (!validRuleValues.length) {
+      return false
+    }
+
+    let ruleAttribute = rule.attribute
+    if (isItemScope) {
+      ruleAttribute = ruleAttribute.replace(
+        `${ApplicationMethodTargetType.ITEMS}.`,
+        ""
+      )
+    } else if (isShippingScope) {
+      ruleAttribute = ruleAttribute.replace(
+        `${ApplicationMethodTargetType.SHIPPING_METHODS}.`,
+        ""
+      )
+    }
+
+    const valuesToCheck = pickValueFromObject(ruleAttribute, context)
 
     return evaluateRuleValueCondition(
-      validRuleValues.filter(isString),
+      validRuleValues,
       rule.operator!,
       valuesToCheck
     )
@@ -76,83 +100,49 @@ export function areRulesValidForContext(
 }
 
 /*
-  The context here can either be either:
+  Optimized evaluateRuleValueCondition by using early returns and cleaner approach
-  - a cart context
+  for evaluating rule conditions.
-  - an item context under a cart
-  - a shipping method context under a cart
-
-  The rule's attributes are set from the perspective of the cart context. For example: items.product.id
-  
-  When the context here is item or shipping_method, we need to drop the "items."" or "shipping_method."
-  from the rule attribute string to accurate pick the values from the context.
 */
-function fetchRuleAttributeForContext(
-  ruleAttribute: string,
-  contextScope: ApplicationMethodTargetTypeValues
-): string {
-  if (contextScope === ApplicationMethodTargetType.ITEMS) {
-    ruleAttribute = ruleAttribute.replace(
-      `${ApplicationMethodTargetType.ITEMS}.`,
-      ""
-    )
-  }
-
-  if (contextScope === ApplicationMethodTargetType.SHIPPING_METHODS) {
-    ruleAttribute = ruleAttribute.replace(
-      `${ApplicationMethodTargetType.SHIPPING_METHODS}.`,
-      ""
-    )
-  }
-
-  return ruleAttribute
-}
-
 export function evaluateRuleValueCondition(
   ruleValues: string[],
   operator: string,
   ruleValuesToCheck: (string | number)[] | (string | number)
-) {
+): boolean {
-  if (!Array.isArray(ruleValuesToCheck)) {
+  const valuesToCheck = Array.isArray(ruleValuesToCheck)
-    ruleValuesToCheck = [ruleValuesToCheck]
+    ? ruleValuesToCheck
-  }
+    : [ruleValuesToCheck]
 
-  if (!ruleValuesToCheck.length) {
+  if (!valuesToCheck.length) {
     return false
   }
 
-  return ruleValuesToCheck.every((ruleValueToCheck: string | number) => {
+  switch (operator) {
-    if (operator === "in" || operator === "eq") {
+    case "in":
-      return ruleValues.some((ruleValue) => ruleValue === `${ruleValueToCheck}`)
+    case "eq": {
+      const ruleValueSet = new Set(ruleValues)
+      return valuesToCheck.every((val) => ruleValueSet.has(`${val}`))
     }
-
+    case "ne": {
-    if (operator === "ne") {
+      const ruleValueSet = new Set(ruleValues)
-      return ruleValues.some((ruleValue) => ruleValue !== `${ruleValueToCheck}`)
+      return valuesToCheck.every((val) => !ruleValueSet.has(`${val}`))
     }
-
+    case "gt":
-    if (operator === "gt") {
+      return valuesToCheck.every((val) =>
-      return ruleValues.some((ruleValue) =>
+        ruleValues.some((ruleVal) => MathBN.gt(val, ruleVal))
-        MathBN.convert(ruleValueToCheck).gt(MathBN.convert(ruleValue))
       )
-    }
+    case "gte":
-
+      return valuesToCheck.every((val) =>
-    if (operator === "gte") {
+        ruleValues.some((ruleVal) => MathBN.gte(val, ruleVal))
-      return ruleValues.some((ruleValue) =>
-        MathBN.convert(ruleValueToCheck).gte(MathBN.convert(ruleValue))
       )
-    }
+    case "lt":
-
+      return valuesToCheck.every((val) =>
-    if (operator === "lt") {
+        ruleValues.some((ruleVal) => MathBN.lt(val, ruleVal))
-      return ruleValues.some((ruleValue) =>
-        MathBN.convert(ruleValueToCheck).lt(MathBN.convert(ruleValue))
       )
-    }
+    case "lte":
-
+      return valuesToCheck.every((val) =>
-    if (operator === "lte") {
+        ruleValues.some((ruleVal) => MathBN.lte(val, ruleVal))
-      return ruleValues.some((ruleValue) =>
-        MathBN.convert(ruleValueToCheck).lte(MathBN.convert(ruleValue))
       )
-    }
+    default:
-
+      return false
-    return false
+  }
-  })
 }