0

Can someone help me break down the following trigger? I am trying to better understand it, then learn how to write test code for coverage so I can deploy a package/app with a variety of triggers in them. So far I have been working through trigger and test code trailheads, but the learning curve is steep. Looking for anyone to provide a baby version explanation that I can build off of.

trigger updateCommissionOnPlanChange on Commission_Factor__c (before insert, before update) {

    for(Commission_Factor__c plan : Trigger.New) {
        try {
            if(!calcCommission.hasBeenHandled) {
                calcCommission.hasBeenHandled = true;
                system.debug('Running with new/changed commission plan values:');
                system.debug(Trigger.newMap.get(plan.Id).Bonus_Image_Mgmt_Agreement_Not_Fixed__c);
                List<Commission_Plan_assignment_junction__c> planAssignments = [SELECT Id, User__c FROM Commission_Plan_assignment_junction__c WHERE Commission_Plan__c = :plan.Id];
                for(Commission_Plan_assignment_junction__c planAssignment : planAssignments) {
                    List<SBQQ__Quote__c> quotes = [SELECT Id, Earned_Commission__c FROM SBQQ__Quote__c WHERE SBQQ__SalesRep__c = :planAssignment.User__c];
                    for(SBQQ__Quote__c q : quotes) {
                        Decimal newCommission = calcCommission.withQuoteFromPlanTrigger(q, Trigger.newMap.get(plan.Id));
                        q.Earned_Commission__c = newCommission;
                        update q;
                        system.debug('-------------- NEW COMMISSION CALCULATED ------------------');
                        system.debug(newCommission);                                    
                    }
                }                
            }
        } catch(Exception e) {
            system.debug('-------------- Trigger Failed ----------------');
            system.debug(e);
        }        
    }
}
3
  • 5
    This trigger is going to fail while handling large amount of data since you are doing SOQL in for loop and also DML
    – d_k
    Apr 27, 2017 at 5:50
  • Also I think you haven't posted the entire code
    – d_k
    Apr 27, 2017 at 5:57
  • I didn't write this, but this was all the code for this particular trigger.
    – vsgro
    Apr 28, 2017 at 16:54

3 Answers 3

0

tl;dr

I wouldn't bother testing this trigger at this point in time. It has serious problems that need to be addressed before it can be reliably used.

That said, most of what I think you need to understand to test such a trigger is what data you need to set up in order to get output that you can make assertions against.

Coverage is always a secondary goal when writing tests. If you write proper tests to cover a variety of possible situations, the code coverage will follow. If you aren't using System.assertEquals() or System.assertNotEquals(), your test isn't a proper test.

For your tests, you will need to create at least the following:

  • One Commission_Factor__c record
  • One Commission_Plan_assignment_junction__c record, with Commission_Plan__c set to the Id of the Commission_Factor__c record you created earlier and User__c set to some user.
  • One SBQQ__Quote__c record with SBQQ__SalesRep__c set to the same user that you used in the Commission_Plan_assignment_junction__c record you created earlier

There may be more setup that you need to perform, but we can't know if there is (or what would be required) given the code you've shared with us so far.

The very long version

The answers so far have offered improvements to your trigger, which you should absolutely consider, but haven't managed to break it down as per your request.

So, let's get started.

In a few simple sentences, this trigger does the following: It takes Commission_Factor__c records being inserted or updated and, for each record, gathers the related Commission_Plan_assignment_junction__c records (there can be more than one). If then finds all the SBQQ__Quote__c records related to the Commission_Plan_assignment_junction__c records (based on the SalesRep/User fields), and recalculates the commission using a helper class calcCommission. Commission is calculated based on the SBQQ_Quote__c record, and the current Commission_Factor__c record being worked on.

The very first line contains enough content for me to comment on.

trigger updateCommissionOnPlanChange on Commission_Factor__c (before insert, before update) {
   // code in here
}

This is the definition of a trigger. It starts with the trigger keyword and is followed by the name of the trigger ('uppdateCommissionOnPlanChange' in this case), the on keyword, and the API name of the SObject that the trigger acts on (Commission_Factor__c). The API name can never contain spaces (there are a few other rules too), and for custom objects always ends in '__c'.

The last part of this line defines the events that the trigger should fire on (i.e. when the trigger should be run). They always start with 'before' or 'after', and you can use 'insert', 'update', 'delete', and 'undelete'.

There are some guidelines on when to use a before trigger and when to use an after trigger. The developer documentation page on Triggers contains some helpful information. From that page

There are two types of triggers:

  • Before triggers are used to update or validate record values before they’re saved to the database.
  • After triggers are used to access field values that are set by the system (such as a record's Id or LastModifiedDate field), and to affect changes in other records, such as logging into an audit table or firing asynchronous events with a queue. The records that fire the after trigger are read-only.

Just for sake of completeness, I'll mention that the curly braces {} define a block of code. It's a feature of nearly all the languages I've worked in. It's also tied to the concept of 'scope', which I won't get into (this answer will already be long enough).

Moving on to the next set of lines

for(Commission_Factor__c plan : Trigger.New) {
    try {
        // more code in here
    } catch (Exception e){
       system.debug('-------------- Trigger Failed ----------------');
       system.debug(e); 
    }
 }

The for(/*stuff*/) defines a 'for' loop. Loops are a very basic component of programming, so I won't go over the details. The code inside the loop definition is another matter.

(Commission_Factor__c plan : Trigger.New)

This follows the syntax <Type> <variable name> :<collection> and defines the variable that will be available inside the loop (i.e. inside the loop's block, which defines the loop's scope). Commission_Factor__c is the type of the loop variable, plan is the name of the loop variable, and :Trigger.New is the collection that we're looping over.

Trigger.New is what's known as a trigger context variable. They are global, static variables that can be referenced at any time, but will only contain useful information when a trigger is being run. There is more information in the documentation on trigger context variables (including information on which trigger events each variable is available in).

Trigger.New is a variable of type List<SObject>, and contains a copy of the records that are involved in a given invocation of the trigger (i.e. records that were either inserted or updated, in your case). It's good to keep in mind that only one trigger event in a trigger is active at any point in time. If you've inserted and updated records in the same stroke (by performing a DML upsert), Trigger.New will either contain the records being inserted, or the records being updated, never both at the same time. The values in Trigger.New and Trigger.NewMap are the values which will eventually be saved back to the underlying database (once the trigger has finished).

The other benefit of the new trigger context variables is that you can alter the values (only alter, not add or remove entire records) in a before trigger event. Doing so allows you to avoid using a DML statement (of which we can only use 100 in any given transaction). This can be a lifesaver, as performing DML causes triggers to run, which in turn can cause other triggers to run (which can very quickly start eating into the other limits that we have to keep in mind, like the limit of 100 SOQL queries per transaction).

One big caveat to using a before insert trigger is that inside such a trigger, Trigger.New won't contain the Id of the record being inserted (because it technically hasn't been inserted and assigned an Id yet). Trigger.NewMap is also unavailable in before insert triggers. If you need to use the Id of a record being inserted (say you're inserting a new Case, and want to insert a CaseComment right away), you'll need to use an after insert trigger to do so.

The try{ // code}catch(Exception e){ // more code} is called, simply enough, a try-catch block. It is used to handle exceptions (things like dividing something by 0) that would cause the execution of code to stop immediately. If an exception is unhandled (or if an exception can't be caught, as is the case when you go over one of the various governor limits), you'll get an ugly error message and/or an email from Salesforce (telling you that you encountered an exception). Handling exceptions allows you the chance to notify specific people, or make the error message a bit nicer. In some limited cases, you may be able to recover from the exception and continue executing code, but that practice is uncommon (if you could recover from it, it probably shouldn't have been an exception).

Finally, for this section of code, system.debug(//some message here);. Calling system.debug() prints a message to the execution log (which is probably viewed easiest using the developer console from within Salesforce). It's useful for debugging to figure out which portions of your code are being executed, inspecting individual records or variables to see what data they contain at a given point in your code, and more.

Finally, new code to analyze.

if(!calcCommission.hasBeenHandled) {
    calcCommission.hasBeenHandled = true;
    system.debug('Running with new/changed commission plan values:');
    system.debug(Trigger.newMap.get(plan.Id).Bonus_Image_Mgmt_Agreement_Not_Fixed__c);
    List<Commission_Plan_assignment_junction__c> planAssignments = [SELECT Id, User__c FROM Commission_Plan_assignment_junction__c WHERE Commission_Plan__c = :plan.Id];
    // more code we'll get to later
}

This if statement is attempting to guard against running the code contained within it more than once. This is a fairly common pattern, and can also be used to prevent infinite recursion, but I don't think it's implemented quite right here. It's hard to say for sure without seeing the calcCommission class. Given its position in the overall code, this would cause you to run the commission calculation for the first record that is triggered, but not for any records after that.

Having a single boolean variable to guard against re-running code works for things that are only run once, a single-shot event. If you want to guard against re-running code for a given record in a collection, changing this from a boolean to a Set<Id> is what I'd do. Using this in your trigger might look like this

// Once again, be aware that this won't work before insert because the record
//   from trigger.new won't have an Id yet.
if(!calcCommission.recHasBeenHandled.contains(plan.id){
    calcCommission.recHasBeenHandled.add(plan.Id);
    // other code same as before
}

The last line of code in this chunk is

List<Commission_Plan_assignment_junction__c> planAssignments = [SELECT Id, User__c FROM Commission_Plan_assignment_junction__c WHERE Commission_Plan__c = :plan.Id];

Pretty straightforward. You define a list, give the variable a name, and then perform a query and assign the result (which will be of type List<Commission_Plan_assignment_junction__c>) to your variable. The types match, so everything is fine.

Again, keep in mind that plan.Id will be null in a before insert trigger. This may result in planAssignments being empty, or full of records that you don't want (if any records have Commission_Plan__c == null).

As mentioned by the other answers, queries inside of loops are bad. Salesforce runs triggers in chunks of up to 200 records. If you have more than 200 records that will go through a trigger in a single transaction, Salesforce breaks them up into multiple runs (all runs are part of the same transaction, and thus each will contribute towards your governor limits). Even if you think that you'll never insert/update more than 100 records at one time, pulling queries (and DML) out of loops is a good idea. Failing to do so will limit the amount of code that you can make that interacts with this SObject (and intricate interactions are the source of the coolest and most useful things that Salesforce can do).

Moving on to the final block of code to analyze:

for(Commission_Plan_assignment_junction__c planAssignment : planAssignments){
    List<SBQQ__Quote__c> quotes = [SELECT Id, Earned_Commission__c FROM SBQQ__Quote__c WHERE SBQQ__SalesRep__c = :planAssignment.User__c];
    for(SBQQ__Quote__c q : quotes) {
        Decimal newCommission = calcCommission.withQuoteFromPlanTrigger(q, Trigger.newMap.get(plan.Id));
        q.Earned_Commission__c = newCommission;
        update q;
        system.debug('-------------- NEW COMMISSION CALCULATED ------------------');
        system.debug(newCommission);                                    
    }
}

More looping and more queries in loops (and DML in a 3rd level loop, which is extremely bad). The high-level view is that each Commission_Factor__c has one or more Commission_Plan_assignment_junction__c. From there each Commission_Plan_assignment_junction__c can have multiple SBQQ__Quote__c records.

This gets us down to the code purpose of this trigger. For each plan, we want to find all of its planAssignments. From there, we want to find all of the related quotes, and calculate the commission. The quotes are then updated.

The calculation is done by a helper class calcCommission, specifically the withQuoteFromPlanTrigger() method.

There isn't much more to say about the code, at least not without getting into rudiments of programming, so I'll stop here.

Now for some frank thoughts

As is, this trigger is not fit for use. If someone brought this to me for review, I would immediately fail it and suggest starting over from scratch. It will fail to run on anything but the most bare of orgs.

Having even as little as 4 Commission_Factor__c records, 5 Commission_Plan_assignment_junction__c records, and 6 SBQQ_Quote__c records could be enough to cause this trigger to run over the SOQL and DML limits.

When I look at this trigger, I get the feeling that someone is trying to make things more complicated than they need to be. I would suggest taking a look at using a hierarchy custom setting to set commission percentages instead (no idea if that'd work for you, but it's worth exploring).

1
  • thanks so much derek. I'm going to process this over the weekend.
    – vsgro
    Apr 28, 2017 at 14:09
0

This is not the correct code but a better version of your code: Change the context to fit your requirement

trigger updateCommissionOnPlanChange on Commission_Factor__c (after insert, after update) {

    Set<Id> plans = new Set<Id>();
    Set<Id> users = new Set<Id>();
    for(Commission_Factor__c plan : Trigger.New) {
        //not sure what calcCommission is
        if(!calcCommission.hasBeenHandled) {
            calcCommission.hasBeenHandled = true;
            plans.add(plan.Id);
        }
    }
    List<Commission_Plan_assignment_junction__c> planAssignments = [SELECT Id, User__c FROM Commission_Plan_assignment_junction__c WHERE Commission_Plan__c IN :plans];
    for(Commission_Plan_assignment_junction__c planAssignment : planAssignments) {
        users.add(planAssignment.User__c);
    }
    List<SBQQ__Quote__c> quotes = [SELECT Id, Earned_Commission__c FROM SBQQ__Quote__c WHERE SBQQ__SalesRep__c = :users];
    for(SBQQ__Quote__c q : quotes) {
        // not sure what this is
        //Decimal newCommission = calcCommission.withQuoteFromPlanTrigger(q, Trigger.newMap.get(plan.Id));
        q.Earned_Commission__c = newCommission;
    }
    update quotes;
}
0

I think there are lot of issues with your trigger itself. As @d_k pointed out, you've couple of SOQL written in loop which will give you 101 SOQL limit exception. Also your code should handle Trigger.IsInsert and Trigger.IsUpdate because record Id's are not generated until record gets inserted. So it's likely that your trigger will get failed.

You should bulkify your code using List. Below code is the bulkified version of your trigger. Though it is not tested but I guess it will give some idea about how it should be:

trigger updateCommissionOnPlanChange on Commission_Factor__c (before insert, before update) {
    if(Trigger.isUpdate) {
        Set<String> setPlanId = new Set<String> ();
        Set<String> setplanAssignmentId = new Set<String> ();
        for(Commission_Factor__c plan : Trigger.New) {
            if(!calcCommission.hasBeenHandled) {
                calcCommission.hasBeenHandled = true;
                setPlanId.add(plan.Id);
            }
        }
        List<Commission_Plan_assignment_junction__c> planAssignments = [SELECT Id, User__c FROM Commission_Plan_assignment_junction__c WHERE Commission_Plan__c = IN setPlanId];

        if (planAssignments.size () > 0) {
            for(Commission_Plan_assignment_junction__c planAssignment : planAssignments) {
                setplanAssignmentId.add(planAssignment.User__c);
            }
        }

        List<SBQQ__Quote__c> quotes = [SELECT Id, Earned_Commission__c FROM SBQQ__Quote__c WHERE SBQQ__SalesRep__c = IN setplanAssignmentId];

        if(quotes.size () > 0) {
            // nested loop can be avoided using Map<Commission_Factor__c,SBQQ__Quote__c>
            for(Commission_Factor__c plan : Trigger.New) {
                for(SBQQ__Quote__c q : quotes) {
                    Decimal newCommission = calcCommission.withQuoteFromPlanTrigger(q, Trigger.newMap.get(plan.Id));
                    q.Earned_Commission__c = newCommission;

                    system.debug('-------------- NEW COMMISSION CALCULATED ------------------');
                    system.debug(newCommission);                                    
                }
            }
            update quotes;
        }
    }

}

For writing test classes you should first create test data. In your case you should create some records for objects SBQQ__SalesRep__c and Commission_Plan_assignment_junction__c. Both these records should be bind to Commission_Factor__c so that SOQL queries in your trigger will fetch them and eventually those loops will get executed and hence code coverage would be increased.

2
  • How are you getting plan.Id when its a before Insert trigger?
    – d_k
    Apr 27, 2017 at 6:12
  • @d_k Now it should be fine Apr 27, 2017 at 6:18

You must log in to answer this question.

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