3

I have the following Trigger, Class & Unit Test

trigger WE_MasterOpportunityTrigger on Opportunity (
    before insert, after insert, 
    before update, after update, 
    before delete, after delete)
{    
    if (Trigger.IsAfter) {
        if (Trigger.isInsert) {
            // class needs to execute in After context so that values are still populated when 
            // an Opportunity is created through Lead conversion
            WE_OppUpdates oppPreparation = new WE_OppUpdates(Trigger.oldMap, Trigger.newMap);
            oppPreparation.updateOpportunities();
        } 
        if (Trigger.isUpdate) {
            WE_OppUpdates oppPreparation = new WE_OppUpdates(Trigger.oldMap,Trigger.newMap);
            oppPreparation.updateOpportunities();
        }
    }
}

public class WE_OppUpdates {

    // variables
    Map<Id, Opportunity> oldOpps;
    Map<Id, Opportunity> newOpps;
    Date cd;
    Opportunity oldOpp;
    List<Opportunity> oppsToUpdate = new List<Opportunity>();

    // constructor
    public WE_OppUpdates(
        Map<Id, Opportunity> oldTriggerOpps, 
        Map<Id, Opportunity> newTriggerOpps) {
            oldOpps = oldTriggerOpps;
            newOpps = newTriggerOpps;
        }

    public void updateOpportunities() {

                if (Trigger.isInsert){

                    List<Opportunity> insertedOpps = [SELECT Id, CloseDate, Ramp_Profile__c FROM Opportunity
                                                       WHERE Id IN :newOpps.keySet()];

                    for (Opportunity o : insertedOpps){

                            cd = o.CloseDate;

                            if (o.Ramp_Profile__c == 'Immediate'){
                                o.Implementation_Revenue__c = cd.addDays(15);
                                o.Revenue_Commencement__c = o.Implementation_Revenue__c.addMonths(1).toStartOfMonth();
                                // etc
                            } else if (o.Ramp_Profile__c == 'Standard'){
                                // set different values 
                            } 
                    }
                    update oppsToUpdate;
                }

                if (Trigger.isUpdate){

                    List<Opportunity> updatedOpps = [SELECT Id, RecordTypeId, CloseDate, Ramp_Profile__c FROM Opportunity
                                                      WHERE Id IN :newOpps.keySet()];

                    for (Opportunity o : updatedOpps){

                            cd = o.CloseDate;

                            Opportunity oldOpp = oldOpps.get(o.Id);

                            system.debug('oldOpp Ramp Profile is ' + oldOpp.Ramp_Profile__c);
                            system.debug('newOpp Ramp Profile is ' + o.Ramp_Profile__c);
                            system.debug('oldOpp Close Date is ' + oldOpp.CloseDate);
                            system.debug('newOpp Close Date is ' + o.CloseDate);

                            if (oldOpp.CloseDate != o.CloseDate
                                ||
                                oldOpp.Ramp_Profile__c != o.Ramp_Profile__c){
                                    if (o.Ramp_Profile__c == 'Immediate'){
                                        o.Implementation_Revenue__c = cd.addDays(15);
                                        }
                                        oppsToUpdate.add(o);

                                    } else if (o.Ramp_Profile__c == 'Medium'){
                                        // set different values
                                    }
                            } 
                    }
                    update oppsToUpdate;
    }
}

@isTest
public class WE_OppUpdatesTest{

    @testSetup
    static void dataSetup() {

            List<Opportunity> opportunities1 = WE_TestDataUtility.createOpportunities(
                25,                                 // noOpps
                '<*>',                              // identifier
                date.today().addMonths(1),          // closeDate
                'Standard');                        // rampProfile

            insert Opportunities1;
    }

    static testMethod void testOppUpdates(){

        List <Opportunity> opportunities = [SELECT Id,RecordTypeId,Name,CloseDate,Ramp_Profile__c FROM Opportunity WHERE Name LIKE '%<*>%'];

        Test.startTest();

        for(Opportunity o : opportunities){
            o.CloseDate = o.CloseDate.addmonths(1);
            o.Ramp_Profile__c = 'Immediate';

        }
        update opportunities;
        Test.stopTest();
    }
}

When I run the test my debug lines in the isUpdate section show that the values for the Ramp_Profile__c & CloseDate fields are unchanged.

My understanding is that the two maps should contain the old & new values so perhaps I'm not passing the values through to my Class correctly or is this issue occurring because of my SOQL query or something else?

  • why not use a before insert/update to do this and avoid the whole query and DML sequence? – cropredy Dec 20 '15 at 19:38
  • @crop1645 In my org we don't execute Before Triggers on Lead conversion & this code needs to execute whenever a new Opportunity is created - including as part of a Lead's conversion. Changing the setting would activate all Validation Rules & required fields during Lead conversion so this is the better alternative. – Alex S Dec 20 '15 at 19:42
  • 1
    @AlexS Correct me if I'm wrong, but it appears that your code should blow up instead of working correctly. It should be after insert, before update. In the after insert trigger, do an update to the records, and in a before update trigger, do whatever changes you're doing. As this stands, it appears that you'll get a DML exception whenever the close date changes (recursive update to record already in a trigger). Your after insert trigger can be as simple as update Trigger.new.deepClone(true); – sfdcfox Dec 20 '15 at 20:40
  • Based on your posted code, you appear to have a recursion issue. I don't know how your handler class knows the execution context or prevents the trigger from firing again once it's been fired in an execution context. See my answer. – crmprogdev Dec 20 '15 at 20:49
  • @sfdcfox I don't get that error (possibly because of the SOQL queries? - I'm no expert). Could you please post the update Trigger.new.deepClone(true); as an answer in the context of the rest of my code. I've not used the deepClone function before so I'm not sure how that would change what I have already.. – Alex S Dec 20 '15 at 20:49
3

Here's how you can change your after-update trigger to a before-update trigger:

public class WE_OppUpdates {
    static Boolean recursiveUpdate = false;
    public static void recursiveInsert(Opportunity[] records) {
        recursiveUpdate = true;
        update records.deepClone(true);
    }
    public static void updateOpportunities(Opportunity[] oldRecords, Opportunity[] newRecords) {
        for(Integer index = 0, size = newRecords.size(); index < size; index++) {
            Opportunity oldRecord = oldRecords[index], newRecord = newRecords[index];
            if(recursiveUpdate || 
               oldRecord.CloseDate != newRecord.CloseDate || 
               oldRecord.Ramp_Profile__c != newRecord.Ramp_Profile__c) {
                   if(newRecord.Ramp_Profile__c == 'Immediate') {
                       // ...
                   } else if(newRecord.Ramp_Profile__c == 'Medium') {
                       // ...
                   }
            }
        }
    }
}

And, of course, change your trigger as appropriate:

if(Trigger.isInsert && Trigger.isAfter) WE_OppUpdates.recursiveInsert(Trigger.new);
if(Trigger.isUpdate && Trigger.isBefore) WE_Updates.updateOpportunities(Trigger.old, Trigger.new);

After that, you'll find your code is easier to test, and recursion is a lot easier to control.

  • Just to check a couple of points - I can see from the documentation the deepClone allows the preservation of Ids, time stamps & auto numbers which is good but does doing a clone like this impact related records at all? Also, is the purpose of the deepClone to avoid a DML statement and therefore not trigger an update or does it have a different application? Thanks. – Alex S Dec 20 '15 at 23:43
  • 1
    @AlexS Related records aren't included in trigger context, so there's nothing to worry about there. The deepClone call is necessary because performing DML on Trigger.old, Trigger.new, Trigger.oldMap, Trigger.newMap, or any record within either of the collections, is forbidden. However, when you clone, you remove this internal flag that the DML statements use to determine if you're trying to perform an operation on those context records. – sfdcfox Dec 20 '15 at 23:47
  • @AlexS My apologies. I forgot a return type (void). There may be a few other silly typos as well. – sfdcfox Dec 21 '15 at 0:10
  • My first attempt's not looking too good, I've posted the complete class in this Github Gist, along with the trigger. I don't have code coverage from lines 33-71 and 88-135 in the class. I know that I'm asking for more than I should here but if you're able to take a look at a high level and make sure that the structure's not completely wrong (I'm not asking for help spotting errors line by line!), that would be huge help. I've never combined two sets of constructors in one class before. – Alex S Dec 21 '15 at 0:59
  • @AlexS You're still constructing your class, while my demo was a static function, but you mixed that with the recursive insert call I suggested. Net result: 33-71 don't get executed because this is never called on insert, and 88-135 don't get called because we're now using static methods. Delete lines 18-74, then change your trigger in the Trigger.isBefore/Trigger.isUpdate path to call the static version of the function (lines 11-12). WE_OppUpdates.updateOpportunities(Trigger.old, Trigger.new) should do the trick. – sfdcfox Dec 21 '15 at 1:32
1

From looking at your code, it appears to me that when you insert new records in your test class, your trigger fires causing an update of Opportunity.

            if (Trigger.isInsert){

                ....

                for (Opportunity o : insertedOpps){

                        .....

                        } else if (o.Ramp_Profile__c == 'Standard'){
                            // set different values 
                        } 
                }
     // ==>     update oppsToUpdate;

When the above happens, this causes the trigger to fire again in the same execution context. Unless you add a boolean flag of some kind to prevent it, the trigger will fire again and do an after update in this execution context. When that happens, you'll see the same trigger.oldas existed the first time the trigger fired in the AfterInsert context.

This happens before you ever do the update in your test class and may be what you're seeing in your debug logs. This same recursion will occur in your AfterUpdate handler code if you don't do something to prevent it.

More than anything, I recommend you add debug statements in your test class to obtain the values of your important variables after insertion of Opportunities1 and following your query to get list opportunities. You'll also benefit from inserting debug statements in your code after update opportunities; (can be placed after Test.stopTest(); if you prefer).

Once you have things sorted, your class should include assertions of what you expect those values to be instead of the debug statements following Test.stopTest();

  • Thanks, yes the Insert does cause the Trigger to fire & Execute the Update too. But I don't hit the recursion issue for some reason. I did try adding a boolean to stop that - it was changed from False to True after either Update and then checked in the alternative context but it didn't seem to have any effect.. – Alex S Dec 20 '15 at 20:54
  • Should the boolean be static and checked in my trigger or could I just include it in the class? – Alex S Dec 20 '15 at 20:58
  • Where did you locate the boolean and is it static? Depending on the location, you may or may not want it to be static and you may need two of them, one for isInsert and one for isUpdate if located in your handler class. If in your trigger, you'd only need one to prevent re-entry. If in the class, don't use static, if in the trigger, you'll want to use static. – crmprogdev Dec 20 '15 at 20:58
  • I'll update the post to show you a couple of options that I tried. At first, I tried a non-static boolean which had no effect, then I made it static but that stopped the class updating some of the records at all. – Alex S Dec 20 '15 at 20:59
  • 1
    Am surprised your code executes properly without that context. You can pass it to your class as a boolean variable. Dan Appleman covers it in his book Advanced Apex Programming. See this blog post for some guidance developer.salesforce.com/page/…. – crmprogdev Dec 20 '15 at 22:46

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.