1

A few months ago, I asked this question: Question regarding "RunOnce" Booleans on AfterUpdate methods in a Salesforce Transaction

The answer provided indicated that I should move my RunOnce boolean to the Trigger (rather than have it in the helper class method), so that each set of 200 records passed/batched to the trigger would have a fresh reset of the Boolean.

So - I moved it from the class up to the trigger.

Since test classes are a different scenario than real life most of the time (doing initial data setup/insert DML (in @testSetup in this case), and then updating records and doing additional update DML), and since the test class' DML was encountering a RunOnce boolean, I was taught to have my test class reset my RunOnce Boolean to false before any DML in my test class, via this post I made earlier this month: Test Code for Conditional Event Deletion is not running expected lines

So - I did that too.


However, even with the changes implemented from these 2 previous posts, I am still encountering coverage issues when attempting to pass more than 200 records through this RunOnce boolean. I know for a fact that the RunOnce boolean is causing the hiccup, because commenting it out of the trigger brings test coverage back to 92% - I also know that it's specifically with more than one batch of 200 records because if I drop the number of created Events to 199 it also gives 92%.

Ultimate Problem/Question: My Class (code below) only cares about Events that were just changed to Status__c = 'Cancelled', but when I run my test class with more than 200 records I only get 21% coverage because I never even make it past the oldMap/newMap comparison (if(e.Status__c == 'Cancelled' && oldEventMap.get(e.Id).Status__c != 'Cancelled'){) so of course the rest of my code which is reliant on this condition is no longer being run/covered.

Why are my subsequent batches of 200 records still passing over this RunOnce if() block and interior method (other than the obvious fact that the boolean is true), when I am resetting the RunOnce boolean before every DML statement? Have I gone wrong in bulkifying this test class or method and how should I approach fixing it since obviously commenting out the RunOnce boolean isn't a fix?


CODE:

Test Class (other testMethods omitted since they are nearly identical to accountEvents() ):

@isTest
private class EventCancelHandler_TEST {

    @testSetup static void setup() {
        List<Event> eventsToInsert = new List<Event>();

        Account a1 = new Account(Name = 'MTEV Test Account');
        insert a1;

        Lead l1 = new Lead(Company = 'MTEV Test Lead',
                           LastName = 'McTester',
                           FirstName = 'Test');
        insert l1;

        Contact c1 = new Contact(FirstName = 'MTEV First',
                                 LastName = 'MTEV Last',
                                 AccountId = a1.Id);
        insert c1;


        List<Event> accountEvents = TestDataFactory.createEvents(a1.Id,202,false);
        eventsToInsert.addAll(accountEvents);
        List<Event> leadEvents = TestDataFactory.createEvents(l1.Id,204,false);
        eventsToInsert.addAll(leadEvents);
        List<Event> nullEvents = TestDataFactory.createEvents(null,206,false);
        eventsToInsert.addAll(nullEvents);
        List<Event> contactEvents = TestDataFactory.createEvents(c1.Id,208,false);
        eventsToInsert.addAll(contactEvents);

        if(!eventsToInsert.isEmpty()){
            ValidatorClass.CancelledEventTriggerFired = false;
            insert eventsToInsert;
        }
    }  

    @isTest static void accountEvents() {
        // Implement test code
        List<Event> eventsToUpdate = new List<Event>();
        Account acct = [SELECT Id FROM Account WHERE Name = 'MTEV Test Account' LIMIT 1];
        List<Event> accountEvents = [SELECT Subject FROM Event WHERE WhatId = :acct.Id];

        Test.startTest();
            if(!accountEvents.isEmpty()){
                for(Event ev : accountEvents){
                    ev.Status__c = 'Cancelled';
                    ev.Cancellation_Type__c = 'Millennium S.I. Initiated';
                    ev.Cancellation_Reason__c = 'Booking Error';
                    eventsToUpdate.add(ev);
                }
            }

            if(!eventsToUpdate.isEmpty()){
                ValidatorClass.CancelledEventTriggerFired = false;
                update eventsToUpdate;
            }
        Test.stopTest();
    }
}

Trigger (other after insert/after update portions of trigger omitted as they didn't seem relevant and would take up space - can provide if necessary):

trigger EventTrigger on Event (after insert, after update)
{
        if(trigger.isAfter && trigger.isUpdate)
        {
            if(ValidatorClass.CancelledEventTriggerFired == false){
                EventCancelHandler.cancelledEvent(Trigger.newMap, Trigger.oldMap); // copies contents from cancelled events to the Cancellation History Custom Object and then deletes the cancelled event.
                ValidatorClass.CancelledEventTriggerFired = true;
            }
        }
}

Class (full class code, System.debugs omitted for space saving):

public without sharing class EventCancelHandler
{

    public static final String account_prefix = Schema.SObjectType.Account.getKeyPrefix();
    public static final String lead_prefix = Schema.SObjectType.Lead.getKeyPrefix();
    public static final String contact_prefix = Schema.SObjectType.Contact.getKeyPrefix();

    public static void cancelledEvent(Map<Id,Event> newEventMap,Map<Id,Event> oldEventMap)
    {

        Map<Id, Event> eventMap = new Map<Id, Event>([select Id, Activity__c,WhatId,WhoId,Confirmation_Status__c,Status__c,Cancellation_Reason__c,Cancellation_Type__c,Confirmed_By__c,Confirmed_On__c,Department__c,StartDateTime,EndDateTime,Event_Length_in_Minutes__c,Description,Event_Notes__c,Location,Subject from event where id in: newEventMap.keySet()]);
        List<Event> eventsToDelete = new List<Event>();
        List<Cancellation_History__c> cancellations = new List<Cancellation_History__c>();
        List<Cancellation_History__c> cancelsToInsert = new List<Cancellation_History__c>();
        List<Event> eventList = new List<Event>();

        for(event e: eventMap.values()){
            if(e.Status__c == 'Cancelled' && oldEventMap.get(e.Id).Status__c != 'Cancelled'){
                eventsToDelete.add(e); // Add to List to Delete
                eventList.add(e); // Add to List of Events to Create Cancellation History for Later
            }
        }

        if(!eventList.isEmpty()){
            cancelsToInsert = createCancelHistory(eventList); // Create Cancellation History Records based on eventList
        }

        if(!eventsToDelete.isEmpty()){
            delete eventsToDelete;
        }

        if(!cancelsToInsert.isEmpty()){
            insert cancelsToInsert;
        }

    } // end of cancelledEvent()

    public static List<Cancellation_History__c> createCancelHistory(List<Event> events){
    Map<Id,Account> contactAccountMap = new Map<Id,Account>();
    Set<Id> contactIDs = new Set<Id>();
    List<Cancellation_History__c> cancelRecords = new List<Cancellation_History__c>();
    List<Contact> contacts = new List<Contact>();

    // Build a Set of Contact Ids from Events linked to Contacts
    for(Event e : events){
        if(e.WhoId != null && ((String)e.WhoId).startsWith(contact_prefix)){
            contactIDs.add(e.WhoId);
        } 
    }

    // If contactIDs set is not empty, query for contacts matching IDs in Set
    if(!contactIDs.isEmpty()){
        contacts = [SELECT firstName,lastName,AccountId,Account.Name FROM Contact WHERE Id in :contactIDs];
    }

    // Loop through Contacts List to build map of Contacts to Accounts
    for(Contact c : contacts){
        if(c.Account != null){
            if(!contactAccountMap.containsKey(c.Id)){
                contactAccountMap.put(c.Id,c.Account);
            }
        }
    }

    for(Event e : events){
        Decimal eventLength = null;

        if(e.Event_Length_in_Minutes__c == null){
            eventLength = 0;
        } 
        else{
            eventLength = Decimal.ValueOf(e.Event_Length_in_Minutes__c);
        }
        Cancellation_History__c cancellation = new Cancellation_History__c(Activity__c = e.Activity__c,
                                                                           Cancellation_Type__c = e.Cancellation_Type__c,
                                                                           Cancellation_Reason__c = e.Cancellation_Reason__c,
                                                                           Cancelled_By__c = UserInfo.getName(),
                                                                           Cancelled_On__c = System.Now(),
                                                                           Confirmed_By__c = e.Confirmed_By__c,
                                                                           Confirmed_On__c = e.Confirmed_On__c,
                                                                           Department__c = e.Department__c,
                                                                           End__c = e.EndDateTime,
                                                                           Start__c = e.StartDateTime,
                                                                           Event_Details__c = e.Description,
                                                                           Event_Length_In_Minutes__c = eventLength,
                                                                           Event_Notes__c = e.Event_Notes__c,
                                                                           Subject__c = e.Subject
                                                                           );

        // Assign the Cancellation Record to Account or Lead depending on what Event was linked to
        if(e.WhoId != null && ((String)e.WhoId).startsWith(lead_prefix)){
            cancellation.Lead__c = e.WhoId;
        }
        else if(e.WhoId != null && ((String)e.WhoId).startsWith(contact_prefix)){
            cancellation.Contact__c = e.WhoId;
            if(contactAccountMap.containsKey(e.WhoId)){
                cancellation.Account__c = contactAccountMap.get(e.WhoId).Id;
            }
        }
        if(e.WhatId != null && ((String)e.WhatId).startsWith(account_prefix)){
            cancellation.Account__c = e.WhatId;
        }
        cancelRecords.add(cancellation);
    }
        if(!cancelRecords.isEmpty()){
            return cancelRecords;
        }        
        else{
            return null;
        }
    } // end of createCancelHistory()

} // end of class EventCancelHandler

Many thanks for your time and assistance, as always.

4
  • You might consider a Set<Id> processedRecords instead of a black/white Boolean flag.
    – Adrian Larson
    Sep 27, 2016 at 20:20
  • Hey Adrian, you mean replacing the ValidatorClass Boolean with a Set<Id>, storing the records in the Set when I loop them, and then checking in the trigger if the ID is in the set rather than if the boolean is true/false? Is this considered a better practice in general? or just an easier way to possibly dodge whatever the heck is happening here? Sep 27, 2016 at 20:23
  • More robust, yes. Triggers will batch on chunks of 200 records. I can add am answer in a bit if no one beats me to it. I'm certainly not the first to come up with the idea.
    – Adrian Larson
    Sep 27, 2016 at 20:27
  • Yes I do know that triggers batch on chunks of 200, which is why I was under the impression that each batch of 200 would reset that Boolean. I assume that is true and that my implementation is decent, but for some reason something else in my process (or even maybe another after insert method) is setting that little guy back to true and I'm having trouble tracking it. I would greatly appreciate an answer from you (or anyone else) related to your suggestion of a Set since I only have experience with RunOnce bools. Would love to see an example. Thanks as always! Sep 27, 2016 at 20:30

1 Answer 1

2

Your trigger is doing exactly what you designed it to do! It will only execute for one invocation. Since the trigger batches in chunks of 200 records, that means only the first 200 get processed.

Analogous Pattern

Consider for a moment if you had set up your trigger as follows:

Handler

public class EventTriggerHandler
{
    public static Integer invocations = 0;
    public static Integer executions = 0;
}

Trigger

trigger Event on Event (after insert)
{
    if (EventTriggerHandler.invocations == 0)
        EventTriggerHandler.executions++;
    EventTriggerHandler.invocations++;
}

Behavior

If you insert 1000 records with the above in place, the final value for EventTriggerHandler.invocations will be 5, but the value of EventTriggerHandler.executions will just be 1!

This behavior is very similar to what's going on with your Boolean flag. A trigger invocation will only enter the if block if it is the first invocation in the transaction.

Recommended Pattern

What I recommend instead is tracking a Set<Id> processedRecords something like the following.

Handler

public class EventTriggerHandler
{
    static final Set<Id> processedRecords = new Set<Id>();
    final List<MyObject__c> newRecords;
    final Map<Id, MyObject__c> oldMap;
    public EventTriggerHandler(Map<Id, MyObject__c> newMap, Map<Id, MyObject__c> oldMap)
    {
        newRecords = new List<MyObject__c>();
        oldMap = new Map<Id, MyObject__c>();
        for (Id recordId : newMap.keySet())
        {
            if (!processedRecords.contains(recordId))
            {
                newRecords.add(newMap.get(recordId));
                oldMap.put(recordId, oldMap.get(recordId));
            }
            processedRecords.add(recordId);
        }
    }
    
    public void afterInsert()
    {
        // do stuff
    }
    public void afterUpdate()
    {
        // do stuff
    }
}

Trigger

trigger Event on Event (after insert, after update)
{
    EventTriggerHandler handle = new EventTriggerHandler(trigger.newMap, trigger.oldMap);
    if (trigger.isAfter)
    {
        if (trigger.isInsert) handle.afterInsert();
        if (trigger.isUpdate) handle.afterUpdate();
    }
}

Behavior

Now, if you process 1000 records, each record can only be processed once, but will not block the processing of any other record. You can process well over 200 records and expect each record to run through your handler exactly once.

While a handler pattern is not necessary and doesn't really affect your solution, I find it makes dealing with recursive behavior much less confusing, among many other benefits.

3
  • 1
    Thanks as always for taking time to lay out a good answer for us. Considering what you've said, I now believe that the answer that I received to this question: link that I asked a few months ago is incorrect. You actually had commented on it, but the person who answered indicated that doing the Boolean in the trigger (rather than class) would allow for me to run more than 200 records through the method. Your answer here contradicts is, can you confirm? Sep 28, 2016 at 0:07
  • @Morgan No, his answer is correct. Re-read what option #2 states. The logic will not fire for your 201st record. It won't fire for your 999th record for the same reason. Nor any records in between. So when you insert 205 records, you will have 5 that are processed with the recursion blocker already set.
    – Adrian Larson
    Sep 28, 2016 at 2:41
  • Right, got it. I see that we're interpreting the question differently - and I believe I am in the wrong based on my original question posed. My intent with that question was to confirm whether MY scenario (putting it in the class) would cause 201 to fail, and I thought he was a.) confirming, and b.) explaining how I could avoid such a problem if I put the boolean in the trigger instead of the class. I see now that he was just confirming #2 and suggesting that IF i wanted that type of design (I don't), I could put it in the trigger. I will implement this answer tomorrow, cheers! Sep 28, 2016 at 3:56

You must log in to answer this question.

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