Trigger to update fields on record create or edit

Question

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;
        }
    }
}

Answer 1

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).

Answer 2

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.