0

I have a trigger on the OpportunityLineItem object that we use to update 4 revenue fields with specific calculations when the record is either created or updated. For some reason it seems the trigger is not always firing when records are created. Below is the trigger's code, which I modified to use the same calculation of "oli.UnitPrice * oli.Quantity" for each field update to make it easier to read here... the real calculations are a bit long.

trigger RevenueBookingsHandler_TRG on OpportunityLineItem (after insert, after update) {

    List<OpportunityLineItem> oliUpdate = new list<OpportunityLineItem>();

    for(OpportunityLineItem oli : Trigger.New) {

        if(Recursivehandler.runOnce()) {

            // Default
            if(oli.Calculation_Type__c == 'Default' || test.IsRunningTest()) {
                OpportunityLineItem oliCalc = new OpportunityLineItem();
                    oliCalcId = oli.Id;
                    oliCalcNet_New_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcNet_Revenue_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Non_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcAnnual_Contract_Value__c = oli.UnitPrice * oli.Quantity;

                if(!test.IsRunningTest()) oliUpdate.add(oliCalc);

            }

            // CC Product
            if(oli.Calculation_Type__c == 'CC Product' || test.IsRunningTest()) {
                OpportunityLineItem oliCalc = new OpportunityLineItem();
                    oliCalcId = oli.Id;
                    oliCalcNet_New_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcNet_Revenue_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Non_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcAnnual_Contract_Value__c = oli.UnitPrice * oli.Quantity;

                if(!test.IsRunningTest()) oliUpdate.add(oliCalc);

            }

            // Registration & Flat Fee
            if(oli.Calculation_Type__c == 'Registration & Flat Fee' || test.IsRunningTest()) {
                OpportunityLineItem oliCalc = new OpportunityLineItem();
                    oliCalcId = oli.Id;
                    oliCalcNet_New_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcNet_Revenue_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Non_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcEst_Recurring_Bookings_Calc__c = oli.UnitPrice * oli.Quantity;
                    oliCalcAnnual_Contract_Value__c = oli.UnitPrice * oli.Quantity;

                if(!test.IsRunningTest()) oliUpdate.add(oliCalc);

            }
        }
    }

    if(!oliUpdate.isEmpty()) Update oliUpdate;
}

I'm fairly new to apex coding, and have a feeling it might be related to the recursive handler I have in there. This was added as I was dealing with some "maximum trigger depth exceeded" issues when testing the trigger.

Any thoughts as to why this does not always fire on record creation, or what I might be able to change to make the code more reliable?

UPDATE:

Adding the Recursivehandler code below.

public Class Recursivehandler{
    private static boolean run = true;
    public static boolean runOnce(){
        if(run){
            run=false;
            return true;
        }else{
            return run;
        }
    }
}
  • Why are you preventing the trigger from operating in a test context? Your unit tests might provide some clues here. What does Recursivehandler.runOnce() do? – David Reed Feb 20 '18 at 15:29
  • You seem to be blocking yourself with the recursion prevention mechanism. Without knowing the internals of it, assisting you will be difficult. Do you have any other triggers on OpportunityLineItem? Your naming convention for this trigger implies you are not following One Trigger Per Object. – Adrian Larson Feb 20 '18 at 15:38
  • 1
    Yep, the recursion prevention mechanism is a prime suspect. The simplest implementation for recursion prevention, just using a static Boolean, will cause issues if you're trying to insert or update more than 200 OpportunityLineItem records (or if you use it in another part of the apex transaction that involves this trigger, or several other conditions). Please edit your question to include the code for Recursivehandler, and if you can, include more details about the volume of records you're handling at a given time. – Derek F Feb 20 '18 at 15:46
  • Also, are the calculations complex enough or different enough (between your three calculation types) that they can't be achieved through a formula field? I assume that you're further rolling these fields up to the Opportunity. As long as a formula field only references data stored on the same object it's defined on, you will be able to include the formula in a rollup summary field. – Derek F Feb 20 '18 at 15:51
  • I have added the code for the Recursivehandler. Again, I am very new to Apex coding and learning as I go in an inherited Org. The trigger is setup as an After trigger because we have CPQ implemented and a few Opportunity Product fields are not synced with the related Quote Line fields until after the record is created/updated. The volume of records is small.... maybe just a few that are being created via CPQ syncing the Quote Lines with the Opportunity Products. – Brandon Feb 20 '18 at 20:41
0

In general, there is some overcomplication. Yes, the recursive piece is suspect, but I'm also looking at your code and wondering why you're doing the following:

You're trying to look at OLIs that are created or edited, and you want to update fields on those OLIs. Not on other ones. But your trigger is an after trigger, and you're also making new OLIs (yes, with the same ID, but still...) and are updating those records.

Perhaps you can change the trigger to a before trigger, and just update the fields you want on the OLIs based on whatever calculations you have to make.

Oh, and then you can see if your recursion problems (and as Adrian said, your probable lack of one trigger per object) are causing any hiccups.

0

As I suspected, your recursion prevention mechanism is just a simple static Boolean.

With this approach, you will return true once, and only once.

If you call Recursivehandler.runOnce() in any other classes that execute before the trigger that you've provided does, then your trigger won't run.

Similarly, if you try to insert more than 200 OpportunityLineItem (OLI from here on) records at once (the Apex Dataloader and generating OLI records directly via Apex come to mind), then your trigger will only run for (up to) the first 200 records. However, that's assuming that your recursion check is outside of a loop. With you performing your recursion check inside a loop, you will only run your trigger for a single OLI.

Usually, when people want to prevent recursion, they want to prevent individual records from being processed twice rather than preventing a block of code from being run twice. A simple static Boolean won't help us here, we need a Set<Id>

The Set class provides us the contains() method, which is pretty descriptive of what it does. If the value you pass to contains() is contained in the set, the method will return true, otherwise it returns false.

There are other ways of using this approach, but the most "drop-in" replacement for you would be to alter your Recursivehandler class to satisfy the following pseudo-code:

public Class Recursivehandler{
    private static Set<Id> = new Set<Id>();

    public static boolean runOnce(), needs to take an Id as an argument{
        if the Id does _not_ exist in the set
            then add this Id to the set, and return true
        else
            the Id does exist in the set, and we should return false
    }
}

That's an improvement on the situation, but it's not perfect. If you want to use this recursion protection class in other triggers, you'll run into the same issue you are running into now. Once the Id is in the set, runOnce() will always return false. The next step you could take (if you do want to use this class to protect from recursion in multiple classes/triggers) would be to use a Map<String, Set<Id>> instead of a simple Set<Id>.

The idea is that you would create a single Set<Id> per class/trigger that you want to protect, and store that in a map where the key is the name of the class/trigger that you called runOnce() from. As the introspection/reflection capabilities of Apex are quite limited, this would mean that you would need to create a string literal (i.e. 'RevenueBookingsHandler_TRG') and pass that to your runOnce() method (the Id would still need to be passed as a parameter as well).

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.