0

I have recently been working on a trigger to insert Cases based on OpportunityLineItems. Namely if a certain type of OpportunityLineItem is added to the Opportunity when the Opportunity is Closed Won, Case records are created based on those Opportunity Line Items.

The code is shown below (I am sure you will all tell me there is a lot of duplication but I'm learning so go steady on me).

trigger AutoCreateRecordsTest on Opportunity (before Insert, Before Update) {
    for(Opportunity O: trigger.new){
        if(o.isWon == true && o.HasOpportunityLineItem == true){

            String opptyId = o.Id;
            OpportunityLineItem[] OLI = [Select UnitPrice, Quantity, PricebookEntry.Product2Id, TotalPrice, Name, PricebookEntry.Product2.Name, Description, Country__c, CurrencyIsoCode  
                                      From OpportunityLineItem
                                      where OpportunityId = :opptyId]; 

            {
                Case c = new Case();
                for(OpportunityLineItem ol: OLI){
                    if(ol.PricebookEntry.Product2.Name == 'De-Registration'){
                        c.AccountId = o.AccountId;
                        c.Country__c = ol.Country__c;
                        c.CurrencyIsoCode = ol.CurrencyIsoCode;
                        c.Related_Opportunity__c =  o.Id;
                        c.Subject = ol.Name + ' ' + ol.Country__c;
                        c.Type = 'De-Registration';
                        c.origin = 'Client';
                        c.Priority = 'Medium';
                        c.Status = 'New Service';
                        c.Due_Date__c = o.CloseDate;
                        c.Amount__c = ol.TotalPrice;            
                    }
                    List<Case> FindParentCase = [Select Id, Country__c, RecordTypeID, AccountId FROM Case WHERE Country__c = :c.Country__c AND AccountId = :c.AccountId AND RecordTypeId = '01220000000FpC8'];  
                    for(case ca: FindParentCase){
                        c.ParentId = ca.Id; 

                    } 
                }

                insert c;
            }
            {
                Case c1 = new Case();  
                for(OpportunityLineItem ol: OLI){
                    if(ol.PricebookEntry.Product2.Name == 'Change of Company Details'){
                        c1.AccountId = o.AccountId;
                        c1.Country__c = ol.Country__c;
                        c1.CurrencyIsoCode = ol.CurrencyIsoCode;
                        c1.Related_Opportunity__c =  o.Id;
                        c1.Subject = ol.Name + ' ' + ol.Country__c;
                        c1.Type = 'Change of Company Details';
                        c1.origin = 'Client';
                        c1.Priority = 'Medium';
                        c1.Status = 'New Service';
                        c1.Due_Date__c = o.CloseDate;
                        c1.Amount__c = ol.TotalPrice;
                    }
                    List<Case> FindParentCase = [Select Id, Country__c, RecordTypeID, AccountId FROM Case WHERE Country__c = :c1.Country__c AND AccountId = :c1.AccountId AND RecordTypeId = '01220000000FpC8'];  
                    for(case ca: FindParentCase){
                        c1.ParentId = ca.Id;
                    } 
                }  
                insert c1;
            }

            {
                Case c2 = new Case();   
                for(OpportunityLineItem ol: OLI){     
                    if(ol.PricebookEntry.Product2.Name == 'Compliance Hours'){   
                        c2.AccountId = o.AccountId;
                        c2.Country__c = ol.Country__c;
                        c2.CurrencyIsoCode = ol.CurrencyIsoCode;
                        c2.Related_Opportunity__c =  o.Id;
                        c2.Subject = o.Name;
                        c2.Type = 'Compliance Hours';
                        c2.origin = 'Client';
                        c2.Priority = 'Medium';
                        c2.Status = 'New Service';
                        c2.Due_Date__c = o.CloseDate;
                        c2.Amount__c = ol.TotalPrice;
                    }
                    List<Case> FindParentCase = [Select Id, Country__c, RecordTypeID, AccountId FROM Case WHERE Country__c = :c2.Country__c AND AccountId = :c2.AccountId AND RecordTypeId = '01220000000FpC8'];  
                    for(case ca: FindParentCase){
                        if(ol.PricebookEntry.Product2.Name == 'Compliance Hours'){
                            c2.ParentId = ca.Id;
                        } 
                    }
                }
                insert c2;
            }


        }

    }
}

I know there are probably some savings I could make in how the code is structured but the main issue I am having concerns the creation of a case for the 'C2' variable.

Despite there not being an OpportunityLineItem named 'Compliance Hours' a Case still seems to be created. Now the case doesn't have any of the variables assigned - it is created as basically a blank case, but with a Parent ID record associated. The issue is that I wouldn't expect this Case record to be created at all.

  • I'm kind of surprised that code compiled. You have an initializer block in a for loop. – Adrian Larson Feb 13 '18 at 14:58
  • 1
    When it comes to posting code here, it really does help if you make the indentation consistent. Without consistent indentation, it can be very hard to tell which code is part of which class/method/if/loop/etc... – Derek F Feb 13 '18 at 15:02
1

There are enough structural issues with this trigger that in all honesty it doesn't make sense to debug it. You'll do far better in the long run by starting fresh with a better trigger architecture before you begin trying to debug.

Here's a few suggestions for how to begin again.

  • Keep in mind that you should never perform SOQL or DML operations in a loop. The typical pattern for a trigger like this would be:
    • Iterate through your incoming objects (Opportunity or what have you) once, to accumulate their Ids or other criteria.
    • Perform one SOQL query for child objects (OpportunityLineItem or what have you) based upon the Ids you located.
    • Iterate through the queried set of objects. Based upon each object, do or do not perform work; if you do, accumulate any objects to be updated or inserted in a collection. Typically you'll use a List for new objects to insert and a Map<Id, Type> for objects to be updated.
    • Perform exactly one DML operation per sObject type at the very end of the trigger to commit your work.
  • Don't hard-code ID values. Always use a SOQL query or describe call to get your RecordTypeId values.
  • If you find yourself copying and pasting the same code, that's a good time to immediately refactor and determine how to make the code generic. Performance is critical in a trigger and copy-pasted code can often contribute to inefficiency, in addition to being harder to debug, as you see here.
    • I am not sure why you have so many anonymous blocks ({} pairs without a control statement). Are you using them to create lexical scope so you can reuse variable names? I really wouldn't recommend that practice.
  • 1
    Thanks all for the feedback! I think it is clear I have a lot of work to do here. @DavidReed, your advice is succinct and exactly what I needed. I will go away and rework this entirely using your advice. I may return but if I do it it will be with code that mirrors your advice. Thanks all for your responses. If anyone else still wants to weigh in, feel free! Thanks, Dan – Dan Bailey Feb 13 '18 at 17:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.