Record unexpectedly created via Apex Trigger

Question

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.

Answer 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.