0

I have this code for a trigger, that according to my logic, should break on the second run, given an if statement I setup, but I'm still getting an error related to maximum trigger depth. Here is my trigger:

trigger OppProductRollupTriggers on OpportunityLineItem (after insert,after update) {
    
    List<OpportunityLineItem> allPLI = new List<OpportunityLineItem>();
        
    allPLI = [SELECT Id,Name,OpportunityId,Opportunity.Custom_Product__c,Quantity,Product2.Family from OpportunityLineItem WHERE ID IN:Trigger.newMap.keySet()];
    System.debug(allPLI);
    
    for ( OpportunityLineItem oli : allPLI) {        
        IF(oli.Name.contains('P1') || oli.Product2.Family.contains('Fam1') || oli.Product2.Family.contains('FAM2') ||  oli.Name.contains('P2')){            
            IF(oli.Opportunity.Custom_Product__c ==FALSE)
            {                
                oli.Opportunity.Custom_Product__c= TRUE;
                update oli;
            }
            else{
                return;
            }
        }
    }
}

The line I believe should break the loop, is the one that says if the Custom_Product__c == FALSE, then do the update, if the update was already done(meaning it will be True, just return. Any ideas why I'm still getting an error?

2
  • 4
    You need to use static variables to prevent recursion. This should answer your question. What is a good pattern for recursion blocking in generic trigger handlers?
    – arut
    Nov 3, 2020 at 6:34
  • 1
    There are a number of issues with this code; 1. you are not bulkifying the updates for the data, 2. you are updating the OLI not the Opportunity, 3. you use "return" which prevents all later records in a bulkified insert or update from being processed, and (least importantly) 4. you do the processing of an OLI even if no relevant fields have changed as part of an update which is wasteful.
    – Phil W
    Nov 3, 2020 at 8:02

3 Answers 3

5

Sherwood Zhao's answer identifies the why of the recursion; you keep updating the line item instead of the related opportunity. However there are a number of other issues here too.

Try:

trigger OppProductRollupTriggers on OpportunityLineItem (after insert, after update) {
    List<OpportunityLineItem> olis = new List<OpportunityLineItem>();

    olis = [
            SELECT Id, Name, OpportunityId, Opportunity.Custom_Product__c, Quantity, Product2.Family
                    FROM OpportunityLineItem
                    WHERE Id IN :Trigger.newMap.keySet()
    ];

    Set<Id> oppIds = new Set<Id>();

    for (OpportunityLineItem oli : olis) {
        // This makes use of the fact that if there are multiple line items that reference the same Opportunity
        // then the SOQL query actually returns a single in-memory Oppo shared by all the line items
        if (!oli.Opportunity.Custom_Product__c &&
                (oli.Name.contains('P1') || oli.Product2.Family.contains('Fam1') ||
                        oli.Product2.Family.contains('FAM2') || oli.Name.contains('P2'))) {
            // Setting this to true prevents the above if condition from considering other line items where
            // the opportunity has already been seen in this trigger execution
            oli.Opportunity.Custom_Product__c = true;

            // Remember just which opportunities need updating. Given the above point this is a little
            // over the top, but using a set is a nice way to ensure you do only try to update a given
            // object once in a given DML operation (otherwise you'll get an exception)
            oppIds.add(oli.OpportunityId);
        }
    }

    if (!oppIds.isEmpty()) {
        // Now we know which opportunities need to have the flag set, so let's generate the data needed
        // and do the DML
        List<Opportunity> opps = new List<Opportunity>();

        for (Id id : oppIds) {
            opps.add(new Opportunity(Id = id, Custom_Product__c = true));
        }

        update opps;
    }
}

Just be aware that this processing is only performed on insert or update of a line item and will not happen because the related opportunity has been updated or when the product families change.

3
  • Hey, Thanks for the solution, this works, and do not provide any errors. My only question would be now, if we were to update the Opportunity with a value from the OLI, say the TotalPrice of the Oli to a Custom Field at the Opportunity, how could I do that? Given that this process is fixed to do the update of just updating a checkbox to true, and not referencing between Oppts and OLIs
    – Neto0291
    Nov 4, 2020 at 15:42
  • The code currently only builds the Opportunity data to be updated right at the end. Instead of just using a set of IDs, you'd want to build a set of data to be updated. However, since you can have multiple OLIs and these are not master detail children (so you can't use a rollup), if you want a total across all the OLIs you would need to start managing the total for the opp INCREMENTALLY. When an OLI is added, add its price to the opp total. When an OLI is deleted, subtract its price from the opp total. When an OLI's price changes, subtract the old price and add the new to the opp total.
    – Phil W
    Nov 4, 2020 at 16:13
  • That means you must ensure you query the Opps' total values when querying the OLIs and use this when generating the opp data to be updated.
    – Phil W
    Nov 4, 2020 at 16:14
1

Custom_Product__c is Opportunity's field, right? You should update your Opportunity record, not OpportunityLineItem.

0
-2

change code to following :

trigger OppProductRollupTriggers on OpportunityLineItem (after insert,after update) {
    
    List<OpportunityLineItem> allPLI = new List<OpportunityLineItem>();
    
    allPLI = [SELECT Id,Name,OpportunityId,Opportunity.Custom_Product__c,Quantity,Product2.Family from OpportunityLineItem WHERE ID IN:Trigger.newMap.keySet()];
    System.debug(allPLI);
    list <Opportunity> updateList = new list <Opportunity>();
    list<id> oppIds = new list<id>();
    for ( OpportunityLineItem oli : allPLI) {
        IF(oli.Name.contains('P1') || oli.Product2.Family.contains('Fam1') || oli.Product2.Family.contains('FAM2') ||  oli.Name.contains('P2')){
            IF(oli.Opportunity.Custom_Product__c ==FALSE)
            {
                //check if same opportunity is already added in update list then dont add same opportunity update again.
                if(!oppIds.contains(oli.OpportunityId)){
                    Opportunity oppObj = new Opportunity();
                    oppObj.id = oli.OpportunityId;
                    oppObj.Custom_Product__c= TRUE;
                    updateList.add(oppObj);
                    oppIds.add(oli.OpportunityId);
                }
            }
        }  
    }
    update updateList;
}

Custom_Product__c is opportunity field and you can not direct update parent object file like oli.Opportunity.Custom_Product__c= TRUE; this.. you have to create one reference for parent object and then update that reference to update parent record.

6
  • 2
    This is still 1. not bulkifying the updates to opportunities and 2. skipping other opps that should be processed due to using return.
    – Phil W
    Nov 3, 2020 at 8:14
  • yes Phil, i have bulkified now. thanks for pointing out. Nov 3, 2020 at 8:17
  • 1
    You could still have multiple OLIs referencing the same Opp, and you will add them all to the update list. (Actually you don't, because of how SOQL returns a common instance, but still, that is a hidden point in your code.) Still, you have "return" which will break processing.
    – Phil W
    Nov 3, 2020 at 8:17
  • Phil i have added multiple opportunity id check as well. Nov 3, 2020 at 8:25
  • 1
    Still the return will break processing of line items.
    – Phil W
    Nov 3, 2020 at 8:52

Not the answer you're looking for? Browse other questions tagged .