2

I have a trigger to insert a new record in custom object'Case Status History' whenever the Status of a case is updated. It is working fine with a single record, but when I change status of 4 cases at same time time instead of creating ONE Case Status History record Per Case it is creating 4 records for each case. I am unable to understand where I am going wrong in my trigger.

Trigger

Trigger CaseTrigger2 on Case (After Update) 
{
    IF(Trigger.isAfter)
    {    
        IF(Trigger.isUpdate)
        {
            For(Case C : Trigger.New)
            {
                IF((C.Record_Type_Name__c == 'Delivery Escalation' || C.Record_Type_Name__c.startsWith('CSP'))  && (Trigger.oldMap.get(C.Id).Status != Trigger.newMap.get(C.Id).Status))
                {
                    CaseAuditClass2.CaseAuditMethod(Trigger.Old,Trigger.New);
                }  
            }
        }
    }
}

Trigger Helper Class

Public Class CaseAuditClass2
{
    //Insert Audit Records
    Public Static Void CaseAuditMethod(List<Case> CaseOldIds , List<Case> CaseNewIds)
    {       
        List<Case_Status_History__c> CSHList = New List<Case_Status_History__c>();
        Set<Id> CaseIds = New Set<Id>();

        For(Case C : CaseNewIds)
        {
            CaseIds.add(C.Id);
        }               

        For(Case CO : CaseOldIds)
        {
            For(Case CN : CaseNewIds)
            {
                IF((CO.Id == CN.Id) && (CO.Last_Status_Change__c != CN.Last_Status_Change__c))
                {
                    Case_Status_History__c CSHVal = New Case_Status_History__c();
                    Decimal Num = 0;

                    CSHVal.Parent_Case__c = CO.Id;
                    CSHVal.Status_Value__c = CO.Status;
                    CSHVal.New_Status_Value__c = CN.Status;
                    CSHVal.Status_Start_Time__c = CO.Last_Status_Change__c;
                    CSHVal.Status_End_Time__c = CN.Last_Status_Change__c;                                          

                    List<CaseTeamMember> CTMList = [Select Id,MemberId,Member.Name,TeamRoleId,TeamRole.Name,ParentId from CaseTeamMember Where ParentId =: CaseIds];

                    //To assign CSO and DA Names
                    For(CaseTeamMember CTM : CTMList)
                    {
                    IF(CTM.TeamRole.Name == 'CSO')
                    {
                        CSHVal.CSO_Name__c = CTM.MemberId;
                    }
                    IF(CTM.TeamRole.Name == 'DA')
                    {
                        CSHVal.DA_Name__c = CTM.MemberId;
                    }}

                //Add all the CaseStatusHistory Records to the List.
                CSHList.add(CSHVal);
                }
            }             
        }
        Insert CSHList;
    }
}

Please help me in knowing where I am going wrong.

Thanks!

4

when I change status of 4 cases at same time time instead of creating ONE Case Status History record Per Case it is creating 4 records for each case.

This happens because of the way you wrote your trigger:

For(Case C : Trigger.New)
{
    IF((C.Record_Type_Name__c == 'Delivery Escalation' || C.Record_Type_Name__c.startsWith('CSP'))  && (Trigger.oldMap.get(C.Id).Status != Trigger.newMap.get(C.Id).Status))
    {
        CaseAuditClass2.CaseAuditMethod(Trigger.Old,Trigger.New);
    }  
}

It will loop through all 4 records, then for each of them it will call CaseAuditClass2.CaseAuditMethod() method. A possible workaround would be to create a boolean before your loop that will check if your criteria is true for at least one of the records and after your loop you can call this method if needed. Example:

boolean callThatMethod = false;
For(Case C : Trigger.New)
{
    IF((C.Record_Type_Name__c == 'Delivery Escalation' || C.Record_Type_Name__c.startsWith('CSP'))  && (Trigger.oldMap.get(C.Id).Status != Trigger.newMap.get(C.Id).Status))
    {
        callThatMethod = true;
    }  
}
if (callThatMethod) {
    CaseAuditClass2.CaseAuditMethod(Trigger.Old,Trigger.New);
}

Like this your method will only be called once per transaction.

Also, you have a query in a loop in your helper class:

List<CaseTeamMember> CTMList = [Select Id,MemberId,Member.Name,TeamRoleId,TeamRole.Name,ParentId from CaseTeamMember Where ParentId =: CaseIds];

Just move it outside of the for-loops

2

You are calling your helper method once for each case in your trigger context so a list is created and inserted for every case in the trigger context.

Take a look below at how to bulkify

Trigger

Trigger CaseTrigger2 on Case (After Update) {
    if(Trigger.isAfter && Trigger.isUpdate){

         //Use a list to collect the cases that you need to create Status history records for
         //This way you can pass the list in to your helper method and only call the method once
         list<Case> casesNeedingNewStatusHistory = new list<Case>();
         For(Case C : Trigger.New){
              if((C.Record_Type_Name__c == 'Delivery Escalation' || C.Record_Type_Name__c.startsWith('CSP'))  && (Trigger.oldMap.get(C.Id).Status != Trigger.newMap.get(C.Id).Status)){
                   //Instead of calling your helper method each time in the loop
                   //just add the case to your list
                   casesNeedingNewStatusHistory.add(C);              
              }  
         }
    }
 //now that you have built your list, call your helper method once
 CaseAuditClass2.CaseAuditMethod(casesNeedingNewStatusHistory,Trigger.oldMap);
}

Helper Class

Public Class CaseAuditClass2{
    //Insert Audit Records
    Public Static Void CaseAuditMethod(List<Case> casesNeedingStatus , map<Id,Case> oldMap)
    {       
        List<Case_Status_History__c> CSHList = New List<Case_Status_History__c>();
        Set<Id> CaseIds = New Set<Id>();

        For(Case C : CaseNewIds){
            CaseIds.add(C.Id);
        }               

        //Moving the SOQL outside the For loop as best practice
        List<CaseTeamMember> CTMList = [Select Id,MemberId,Member.Name,TeamRoleId,TeamRole.Name,ParentId from CaseTeamMember Where ParentId =: CaseIds];

        //Since you now have a map of the old values, you don't have to loop through them, 
        //you can just access the old values through the map
            For(Case CN : casesNeedingNewStatusHistory){
                IF(CN.Last_Status_Change__c != oldMap.get(CN.Id).Last_Status_Change__c)
                {
                    Case_Status_History__c CSHVal = New Case_Status_History__c();
                    Decimal Num = 0;

                    CSHVal.Parent_Case__c = oldMap.get(CN.Id).Id;
                    CSHVal.Status_Value__c = oldMap.get(CN.Id).Status;
                    CSHVal.New_Status_Value__c = CN.Status;
                    CSHVal.Status_Start_Time__c = oldMap.get(CN.Id).Last_Status_Change__c;
                    CSHVal.Status_End_Time__c = CN.Last_Status_Change__c;                                          

                    //To assign CSO and DA Names
                    For(CaseTeamMember CTM : CTMList)
                    {
                    IF(CTM.TeamRole.Name == 'CSO')
                    {
                        CSHVal.CSO_Name__c = CTM.MemberId;
                    }
                    IF(CTM.TeamRole.Name == 'DA')
                    {
                        CSHVal.DA_Name__c = CTM.MemberId;
                    }}

                //Add all the CaseStatusHistory Records to the List.
                CSHList.add(CSHVal);
                }
            }             

        Insert CSHList;
    }
}

Main Points

  1. Always move SOQL outside of for loops in order to avoid governor limits
  2. Try to only make a single call to helper methods that take collections as parameters (when possible)
  3. Utilize newMap and oldMap trigger context variables when you can

Hope this helps

1

As a first observation I am not sure why you are doing the following

 For(Case C : Trigger.New)
            {
                IF((C.Record_Type_Name__c == 'Delivery Escalation' || C.Record_Type_Name__c.startsWith('CSP'))  && (Trigger.oldMap.get(C.Id).Status != Trigger.newMap.get(C.Id).Status))
                {
                    CaseAuditClass2.CaseAuditMethod(Trigger.Old,Trigger.New);
                    }  
}

I would suggest Calling the CaseAuditClass2.CaseAuditMethod(Trigger.Old,Trigger.New); only once and inside your apex class have the IF((C.Record_Type_Name__c == 'Delivery Escalation'....) validation

Additionally another suggestion would be to create a constructor in your helper Class that initializes the Trigger.new and Trigger.old values one time so that you won't have to use it in as a parameter each time you call a function.

0

In order to bulkify it please move SOQL outside the loop. Final code would be as follow.

Public Class CaseAuditClass2
    {
        //Insert Audit Records
        Public Static Void CaseAuditMethod(List<Case> CaseOldIds , List<Case> CaseNewIds)
        {       
            List<Case_Status_History__c> CSHList = New List<Case_Status_History__c>();
            Set<Id> CaseIds = New Set<Id>();

            For(Case C : CaseNewIds)
            {
                CaseIds.add(C.Id);
            }  

            List<CaseTeamMember> CTMList = [Select Id,MemberId,Member.Name,TeamRoleId,TeamRole.Name,ParentId from CaseTeamMember Where ParentId =: CaseIds];

            For(Case CO : CaseOldIds)
            {
                For(Case CN : CaseNewIds)
                {
                    IF((CO.Id == CN.Id) && (CO.Last_Status_Change__c != CN.Last_Status_Change__c))
                    {
                        Case_Status_History__c CSHVal = New Case_Status_History__c();
                        Decimal Num = 0;

                        CSHVal.Parent_Case__c = CO.Id;
                        CSHVal.Status_Value__c = CO.Status;
                        CSHVal.New_Status_Value__c = CN.Status;
                        CSHVal.Status_Start_Time__c = CO.Last_Status_Change__c;
                        CSHVal.Status_End_Time__c = CN.Last_Status_Change__c;                                          



                        //To assign CSO and DA Names
                        For(CaseTeamMember CTM : CTMList)
                        {
                            IF(CTM.TeamRole.Name == 'CSO')
                            {
                                CSHVal.CSO_Name__c = CTM.MemberId;
                            }
                            IF(CTM.TeamRole.Name == 'DA')
                            {
                                CSHVal.DA_Name__c = CTM.MemberId;
                            }
                        }

                    //Add all the CaseStatusHistory Records to the List.
                        CSHList.add(CSHVal);
                    }
                }             
            }
            Insert CSHList;
        }
    }
  • This did not help Kiran :( – Ruchi Apr 5 '16 at 11:38
  • Hi Ruchi, In trigger you have applied some check but inspite of that check you are passing all cases. So I would suggest first filter out all the records that you need to pass and then only call method of handler. – Kiran Machhewar Apr 5 '16 at 11:41
  • 1
    What @Kiran has referenced in this answer is a best practice. It does not solve your current issue, but moving SOQL outside of loops is best practice. The main issue in your code is that you are calling a helper class which performs DML inside of a loop. therefore for every record in your loop, your helper method is inserting records. – Chris Duncombe Apr 5 '16 at 12:02

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.