Issue while inserting new Opportunity Product records on after insert trigger

Question

Here is the scenario: I have a Bundle product which when added to the opportunity should add related products in that Bundle. I have written trigger on after insert but instead of adding related products from that bundle, it adds the bundle product again that many times. Suppose we have a bundle product 'Bundle1' and related product 'Prod1' and 'Prod2'. Then on inserting Bundle1 it should add 'Prod1' and 'Prod2' also. But it adds 'Bundle1' itself 3 times. The trigger was recursive so I've added Boolean but looks like it was recursive because the same bundle product was getting added again. Can anyone tell what is the issue with the code and how to resolve.

Here is the code:

Trigger:

    trigger oppProdTrigger on OpportunityLineItem (after insert) {
    if(trigger.isAfter && trigger.isInsert){
        if(!OppLineItemHandler.isTriggered){
            OppLineItemHandler.createBundleProducts(trigger.new); 
        }
       
    }
    
}

Handler Class:

    public class OppLineItemHandler {
    public static boolean isTriggered = false;
    public static void createBundleProducts(List<OpportunityLineItem> newOpptyItems){
        List<Product2> lstBundledProducts = new List<Product2>();
        //Prod1 and Prod2 Ids ---------
        List<Id> lstBundledProdIds = new List<Id>{'01t28000003hbxmAAA','01t28000003hbxfAAA'};
            List<OpportunityLineItem> newLineItemLst = new List<OpportunityLineItem>();
        
        for(OpportunityLineItem oppLineItem : newOpptyItems){
            for(Id productId : lstBundledProdIds){
                System.debug('enter here--3--');
                OpportunityLineItem oli = new OpportunityLineItem();
                oli.OpportunityId = oppLineItem.OpportunityId;
                oli.Product2Id = productId;
                oli.Quantity = oppLineItem.Quantity;
                oli.UnitPrice = oppLineItem.UnitPrice;
                oli.PricebookEntryId = oppLineItem.PricebookEntryId;
                newLineItemLst.add(oli);       
            }
        }
        try{
            if(!newLineItemLst.isEmpty()) {
                isTriggered = true;
                insert newLineItemLst;
            }
        }catch(System.DMLException de){
          
        }
    }
}

Answer 1

When you add an OpportunityLineItem to an Opportunity, one of the most important fields to set is PricebookEntryId.

The Product2Id field should be read-only, unless your API version is < v30.0, in which case it should be updated to a current API version because API versions <= 20 are being retired on 2022-06-01, and versions <= 30 being retired one year later.

Since you're simply copying the pricebookEntryId from the existing OpportunityLineItem, you're getting more copies of that line item.

Having your target product Ids is only part of the battle here. You need to use those in conjunction with the Pricebook2Id of your Opportunity to query for the PricebookEntry records that you should be using. It should be possible to do all of this in a single query.

Set<Id> oppIds = new Set<Id>();
for(OpportunityLineItem oppLineItem : newOpptyItems){
    oppIds.add(oppLineItem.OpportunityId);
}

List<PricebookEntry> pbeList = [
    SELECT Id, Product2Id, Pricebook2Id, UnitPrice 
    FROM PricebookEntry 
    WHERE Product2Id IN :lstBundledProdIds 
        AND Pricebook2Id IN (SELECT Pricebook2Id FROM Opportunity WHERE Id IN :oppIds)
];

Things may get a bit messy if you process multiple Opportunities with different pricebooks in a single transaction.

• If that won't be the case, then you could simply create a Map<Id, PricebookEntry> where the key is the Product2Id of the PBE
• If that will be the case, then you'll need to obtain the Pricebook2Id from the Opportunity (either through a query, or through a formula field) and build/use a Map<Id, Map<Id, PricebookEntry>> (the outer map's key is the Pricebook2Id, the inner map's key is the Product2Id)

A few other suggestions

• There is no need to check if a List is empty before performing DML on it, so you should remove that if()
• Empty catch blocks, or catch blocks where you're only doing System.debug() are bad practice. You should remove the try and catch (you/your test/QA team will thank you for removing that headache).
• Simple boolean recursion prevention is typically bad practice. If you're not careful, it'll cause you to stop running your trigger after the first trigger chunk (i.e. if you DML more than 200 records in a single transaction, only the first 200 would be processed). At the very least, you'll want to set isTriggered back to false after the DML statement in your handler. Your main concern here is to not run your handler again while you're performing DML in the handler.