function readOnly(count){ }
Sign Up ›
Starting November 20, the site will be set to read-only. On December 4, 2023,
forum discussions will move to the Trailblazer Community.
Learn More

Browse by Topic
ShowAll Questionssorted byDate Posted
+ Start a Discussion
RelaxItsJustCodeRelaxItsJustCode 

Looking to twist the mind of a true guru, Please help if you can.

trigger CaseCloseNoOpen on Case (Before update) 
{
    


    List<Id> caseIds = new List<Id>();
    
        Id caseRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Case' AND SObjectType = 'Case'].Id;

        Id TaskRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Task' AND SObjectType = 'Task'].Id;

    for(Case c : Trigger.New)
    {
           if(c.RecordTypeId == caseRecordTypeId && (c.Status == 'Completed' || c.Status == 'Closed'))
       {
            caseIds.add(c.Id);
       }
      
    }
    
       
       for(Training_Information__c customobj : [Select Id, Status__c,Case__c From Training_Information__c Where Case__c in: caseIds])
        {
            if(customobj.Status__c == 'Pending' || customobj.Status__c == 'Scheduled')
            {
                Trigger.newMap.get(customobj.Case__c).addError('Training has not been completed.');
            }
        }

    for(Development_Request__c customobj : [Select Id, Stage__c,Case__c From Development_Request__c Where Case__c in: caseIds])
        {
            if(customobj.Stage__c != 'Completed' && customobj.Stage__c != 'Cancelled DR/ VOID' && customobj.Stage__c != 'Draft')
            {
                Trigger.newMap.get(customobj.Case__c).addError('There are open Development Request associated with this Case.');
            }
        }  

    for(Task T : [Select Id, Status, WhatId, RecordTypeId From Task Where WhatId in: caseIds])
        {
            if(T.RecordTypeId == TaskRecordTypeId && (T.Status != 'Completed' && T.Status != 'Discarded'))
            {
                Trigger.newMap.get(T.WhatId).addError('There are still open PM Task on this Case.');
            }
        }
              
}

 How can we make this trigger better?  It is a trigger made to do the validation of no open task, training information records or development request records, so that a case which is being used for project management can't be closed until all of these three LOOKUP related objects are all resolved/Completed.

 

The above code works but I have questions like would it make sense to use a trigger to pass the newmap + oldmap into a class to do all of the heavy lifting?

 

Also, when evaluating please consider which approach would be easier to get code coverage on and why?

 

Please help if you can.

 

Thank you,

Steve Laycock

Best Answer chosen by Admin (Salesforce Developers) 
Andrew WilkinsonAndrew Wilkinson

Steve,

 

I saw that you had posted earlier as well. I decided that I could give you some insight into your question. I work for a consulting firm and as a best practice we keep our triggers skinny and pass the lists or maps from the trigger into static methods on a class. The only logic allowed in a trigger are simple such as isInsert or isBefore. Handled this way the trigger is easily maintainable and one trigger can be defined for each object. This allows the triggers to be scalable and maintainable. When more logic needs to be performed on an object it is easy to pass another method into the trigger to handle the logic. The individual methods segregate the code and allow for easy debugging and if needed turning off a specific portion of a trigger. As for testing, the tests will be the same. Inserts, updates or deletes will be performed in your unit test and these DML operations will run through your trigger. Really the benefit comes from maintainability, scalability, and reusability. You will be able to reuse the methods found on the class. Just some thoughts. I am sure there are many arguments for or against this. But in my experience I have found that having a sort of service class to hold the methods for a trigger is beneficial.

All Answers

Andrew WilkinsonAndrew Wilkinson

Steve,

 

I saw that you had posted earlier as well. I decided that I could give you some insight into your question. I work for a consulting firm and as a best practice we keep our triggers skinny and pass the lists or maps from the trigger into static methods on a class. The only logic allowed in a trigger are simple such as isInsert or isBefore. Handled this way the trigger is easily maintainable and one trigger can be defined for each object. This allows the triggers to be scalable and maintainable. When more logic needs to be performed on an object it is easy to pass another method into the trigger to handle the logic. The individual methods segregate the code and allow for easy debugging and if needed turning off a specific portion of a trigger. As for testing, the tests will be the same. Inserts, updates or deletes will be performed in your unit test and these DML operations will run through your trigger. Really the benefit comes from maintainability, scalability, and reusability. You will be able to reuse the methods found on the class. Just some thoughts. I am sure there are many arguments for or against this. But in my experience I have found that having a sort of service class to hold the methods for a trigger is beneficial.

This was selected as the best answer
RelaxItsJustCodeRelaxItsJustCode

Andrew, thank you I wasn't asking to prove that I was right, I wanted to know the pros and cons just like you have provided for me.

 

I really appriciate your input.  You def won this one.

 

Thank you, have a great weekend.

 

Steve Laycock
SFAdmin5SFAdmin5

great response andrew
RelaxItsJustCodeRelaxItsJustCode
trigger CaseTrigger on Case (before update,before insert,before delete, after update, after insert, after delete)
{
     Profile p = [SELECT Name FROM Profile WHERE ID =:UserInfo.getProfileId()];
          if(p.Name != 'System Administrator') 
          {
               if (Trigger.isInsert && Trigger.isbefore)
                { 
                }
               if (Trigger.isUpdate && Trigger.isbefore)
                {
                     CaseFunctions.CaseValidationBeforeComplete(Trigger.new, Trigger.oldMap, Trigger.newMap);
                } 
               if (Trigger.isDelete && Trigger.isbefore)
                {
                }
               if (Trigger.isInsert && Trigger.isafter)
                { 
                }
               if (Trigger.isUpdate && Trigger.isafter)
                {
                } 
               if (Trigger.isDelete && Trigger.isafter)
                {
                }
          }
              
}

 

public class CaseFunctions
{
   public static void CaseValidationBeforeComplete(Case[] caseRecs,Map<Id,Case> oldMap, Map<Id,Case> newMap)
   {
        List<Id> caseIds = new List<Id>();
        
        Id caseRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Case' AND SObjectType = 'Case'].Id;

        Id TaskRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Task' AND SObjectType = 'Task'].Id;
        
        for (Case c : caseRecs){      
            Case newRec = newMap.get(c.Id);
            if(c.RecordTypeId == caseRecordTypeId && (c.Status == 'Completed' || c.Status == 'Closed'))
            {
                 caseIds.add(c.Id);
            }
        }
        for(Training_Information__c customobj : [Select Id, Status__c,Case__c From Training_Information__c Where Case__c in: caseIds])
        {
            if(customobj.Status__c == 'Pending' || customobj.Status__c == 'Scheduled')
            {    
                 newMap.get(customobj.Case__c).addError('Training has not been completed.');
            }     
        }    
        
        for(Development_Request__c customobj : [Select Id, Stage__c,Case__c From Development_Request__c Where Case__c in: caseIds])
        {
            if(customobj.Stage__c != 'Completed' && customobj.Stage__c != 'Cancelled DR/ VOID' && customobj.Stage__c != 'Draft')
            {
                newMap.get(customobj.Case__c).addError('There are open Development Request associated with this Case.');
            }
        }  

        for(Task T : [Select Id, Status, WhatId, RecordTypeId From Task Where WhatId in: caseIds])
        {
            if(T.RecordTypeId == TaskRecordTypeId && (T.Status != 'Completed' && T.Status != 'Discarded'))
            {
                newMap.get(T.WhatId).addError('There are still open PM Task on this Case.');
            }
        }
    }    
}

 Andrew what do you think of this solution to the issue?  I built out the trigger for all scenarios, and am now using a class with a function to do what I need done now with room to grow any Case based apex need all out of the one trigger with the follow up additions to the class if and when need be.  So what do you think?
Andrew WilkinsonAndrew Wilkinson

I added a couple comments.

 

public class CaseFunctions
{   
public static void CaseValidationBeforeComplete(Case[] caseRecs,Map<Id,Case> oldMap, Map<Id,Case> newMap)
   {
        List<Id> caseIds = new List<Id>();
   //possibly move these as a static method map     
        Id caseRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Case' AND SObjectType = 'Case'].Id;

        Id TaskRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Task' AND SObjectType = 'Task'].Id;
        
        //changing this to newmap values instead of list...you can get rid of the list as a parameter
        for (Case c : newMap.values()){  
        	//you don't need this. it isn't doing anything
            //Case newRec = newMap.get(c.Id);
            if(c.RecordTypeId == caseRecordTypeId && (c.Status == 'Completed' || c.Status == 'Closed'))
            {
                 caseIds.add(c.Id);
            }
        }
        for(Training_Information__c customobj : [Select Id, Status__c,Case__c From Training_Information__c Where Case__c in: caseIds])
        {
            if(customobj.Status__c == 'Pending' || customobj.Status__c == 'Scheduled')
            {    
                 newMap.get(customobj.Case__c).addError('Training has not been completed.');
            }     
        }    
        
        for(Development_Request__c customobj : [Select Id, Stage__c,Case__c From Development_Request__c Where Case__c in: caseIds])
        {
            if(customobj.Stage__c != 'Completed' && customobj.Stage__c != 'Cancelled DR/ VOID' && customobj.Stage__c != 'Draft')
            {
                newMap.get(customobj.Case__c).addError('There are open Development Request associated with this Case.');
            }
        }  

        for(Task T : [Select Id, Status, WhatId, RecordTypeId From Task Where WhatId in: caseIds])
        {
            if(T.RecordTypeId == TaskRecordTypeId && (T.Status != 'Completed' && T.Status != 'Discarded'))
            {
                newMap.get(T.WhatId).addError('There are still open PM Task on this Case.');
            }
        }
    }

 

So when working with record types sometimes it is easier to have a static method on the service class that gets the record types...for example,

 

 public static Map<String, RecordType> recordTypesNameMap
    {
        get
        {
            if(recordTypesNameMap == null){
            	recordTypesNameMap = new Map<String,RecordType>();
                for(RecordType rt: [Select Id, Name from RecordType where sObjectType = 'Case' ])
                recordTypesNameMap.put(rt.Name, rt);
            }
            return recordTypesNameMap;

        }
        
        private set;
    }

 Then in your code you do the following...

if(c.RecordTypeId == recordTypesNameMap.get('PM Case').Id)

 

In this way your record type comparisons are always set up for any method in any class. You would create a service class for task and do the same thing with recordtypes. But it looks good. It will handle your bulk operations and does what you need it to do with the cross object validation.
RelaxItsJustCodeRelaxItsJustCode

Awesome!  Thank you very much.

 

Have a great Thanksgiving!

 

Steve Laycock
RelaxItsJustCodeRelaxItsJustCode

So how would you do that but turn the sObject into a variable that you pass in from the class calling the service class?  Also, how would you pass it?

 

Thanks,

Steve Laycock