1

I have this after trigger and I can't figure out why it isn't updating the field. I have the field write to a list then once done with the Trigger.New I then update the list. But when I test I can't get the field to populate with anything.

More detail on the problem I am trying to create a lookup to an Object that I will let a department own. The object has the fields they use to split and define regions. They can enter this information into the record in this custom object and then this trigger will take in this case a lead, and look to find the region that it matches to. This way for small changes to the region it can all be managed by the end user with no need to update any Apex.

The reason I thought it needed to be an After trigger is it relies on a different Before trigger to populate one of the fields that is used to select the region. After getting enough written to test a single region I created a region record then updated a Lead record to match. No error pops up, and no update is made.

Looking for some help on what might be wrong and where I should make my next stab. Trying to learn to fish.

trigger SDRRegionLead on Lead(after insert, after update) {

// Has to be an After trigger as it relys on LookupRegionsLead trigger to fire first
// Build a map of the SDR Region Object

//after trigger means I need to update via a List as the record is already locked
List<Lead> leadUpdate = new List<Lead>();

Map < Id, SDR_Region__c > regionmap =  new Map < Id, SDR_Region__c > ();

for (Lead newLead : Trigger.New){
    If(Trigger.isUpdate) {
        Lead oldLead = Trigger.oldmap.get(NewLead.id);

        // Do new vs old check so it doesnt always fire
        //Broken into each category so it will only run based on set rules of what MIGHT change for each lookup
        //Geography__c = Field Sales Region

        If(oldLead.NumberOfEmployees !=  newLead.NumberOfEmployees || oldLead.Geography__c !=  newLead.Geography__c) {
            for (ID idKey : regionmap.keyset()) {
                SDR_Region__c SDRR = regionmap.get(idKey);
                if (newLead.Geography__c != NULL) {
                    if (newLead.Geography__c == SDRR.Field_Sales_Region__c && (newLead.NumberOfEmployees >= SDRR.Number_of_Employees_Lower_Fence__c ||  newLead.NumberOfEmployees == SDRR.Number_of_Employees_Lower_Fence__c) &&  newLead.NumberOfEmployees <= SDRR.Number_of_Employees_Upper_Fence__c) {
                        newLead.SDR_Region__c = SDRR.SDR_Region_Name__c;
                        leadUpdate.add(newLead);
                    }
                }
            }
        }
        Else If(oldLead.NumberOfEmployees !=  newLead.NumberOfEmployees || oldLead.Corporate_Sales_Region__c !=  newLead.Corporate_Sales_Region__c) {
            for(ID idKey : regionmap.keyset()){
                SDR_Region__c SDRR = regionmap.get(idKey);
                if (newLead.Corporate_Sales_Region__c != NULL){
                    if (newLead.Corporate_Sales_Region__c == SDRR.Corporate_Sales_Region__c && (newLead.NumberOfEmployees >= SDRR.Number_of_Employees_Lower_Fence__c ||  newLead.NumberOfEmployees == SDRR.Number_of_Employees_Lower_Fence__c) &&  newLead.NumberOfEmployees <= SDRR.Number_of_Employees_Upper_Fence__c) {
                        newLead.SDR_Region__c  = SDRR.SDR_Region_Name__c;
                        leadUpdate.add(newLead);
                    }
                }
            }
        }
        Else If(oldLead.NumberOfEmployees != newLead.NumberOfEmployees || oldLead.Corporate_HQ_State__c != newLead.Corporate_HQ_State__c || (newLead.Corporate_HQ_State__c == NULL && oldLead.State != newLead.State)) {
            for(ID idKey : regionmap.keyset()){
                SDR_Region__c SDRR = regionmap.get(idKey);
                if (newLead.Corporate_HQ_State__c != NULL){
                    if(SDRR.States__c.containsIgnoreCase(newLead.Corporate_HQ_State__c) && (newLead.NumberOfEmployees >= SDRR.Number_of_Employees_Lower_Fence__c ||  newLead.NumberOfEmployees == SDRR.Number_of_Employees_Lower_Fence__c) &&  newLead.NumberOfEmployees <= SDRR.Number_of_Employees_Upper_Fence__c){
                        newLead.SDR_Region__c  = SDRR.SDR_Region_Name__c;
                        leadUpdate.add(newLead);
                    }
                }
            }
        }

    }
 }

update leadUpdate;
}
  • 3
    Hi Dan, I was going to write an answer, but it's not really an answer, so here goes: your code doesn't see any updates to do because the fields involved aren't being updated. It's just as well, because the moment that there was a valid update, you'd crash from an error. Not only can you not modify Trigger.new, you also cannot modify any field of any record in Trigger.new in an after update, nor can you perform another update on any record in Trigger.new while the trigger is running (only Deletes are valid operations). You need to rethink how you're trying to solve your problem. – sfdcfox Apr 26 '16 at 0:10
  • I guess I don't understand why it doesn't see the change happen. Also I change the field but never force the update as I add it to a list then outside of the Trigger.New start the field update. I can find a bunch of stuff on how-to for Before triggers but not much on After. I guess another solution would be to re-write the trigger this trigger is dependent on to, make its changes then after it makes it changes run this as the 2nd part of that trigger. – Dan Apr 26 '16 at 1:15
  • 3
    for pete's sake... I disagree that this should be closed. I spent 20 minutes sifting through Dan's code and writing up an answer to his question. I have solved Dan's problem. Teachable moment, let me post my answer ffs ;) – krigi Apr 26 '16 at 5:45
  • I would love a teachable moment on After triggers. After digging around and unable to understand After Triggers I started to re-write the Trigger this one would rely on to make this more of a 2nd stage of the same trigger so they can just both be before triggers. That said I would still love a deeper understanding of After triggers and why this failed. – Dan Apr 26 '16 at 8:07
  • @DasUberDanger go for it. – Samuel De Rycke Apr 26 '16 at 11:11
1

@Dan, here is a very comprehensive answer as well, including functional code for your consideration. Yes, there are several things wrong with your initial code...

Issue 1) You cannot modify Trigger.new in after trigger: you are attempting to do that several different places. Here is one example where you are attempting to modify Trigger.new:

newLead.SDR_Region__c = SDRR.SDR_Region_Name__c;

If you had properly wrapped your DML in try/catch (you didn't), and cleared up the other issues (I'll point them out later) you would have seen an error like this if you made a "triggering" Lead update via the GUI:

Error:Apex trigger SDRRegionLead caused an unexpected exception, contact your administrator: SDRRegionLead: execution of AfterUpdate caused by: System.FinalException: Record is read-only: Trigger.SDRRegionLead: line 34, column 1

Pro tip: always try/catch your DML!!!

Issue 2) You are trying to set an Id field (Lead.SDR_Region__c) with a Name field: line 33 or thereabouts is one example. Ignoring the fact that you are trying to set Trigger.new, consider your code:

newLead.SDR_Region__c = SDRR.SDR_Region_Name__c;

... I believe this should be:

newLead.SDR_Region__c = SDRR.Id

Pro tip: compartmentalize your logic. Don't boil the ocean all at once. Slow down. Focus on small piece of functionality, test your code, make sure it does exactly what you think it does (liberally apply "System.debug" statements throughout your code) before you move on to the next logic block.

Issue 3) Your initial map is always null, this map will always be empty on line 9:

Map < Id, SDR_Region__c > regionmap =  new Map < Id, SDR_Region__c > ();

I believe what you meant to do is:

Map < Id, SDR_Region__c > regionmap =  new Map < Id, SDR_Region__c > ([SELECT Id, Corporate_Sales_Region__c, Field_Sales_Region__c, Number_of_Employees_Lower_Fence__c, Number_of_Employees_Upper_Fence__c, SDR_Region_Name__c, States__c FROM SDR_Region__c ]);

Issue 4: understand the trigger context variables! Trigger.new is a list AND a map. That is a powerful concept. Using this knowledge, you can rewrite your entry loop. This code on line 11:

for (Lead newLead : Trigger.New){

should be re-written like so:

for(Lead newLead : [SELECT Id, NumberOfEmployees, Geography__c, SDR_Region__c FROM Lead WHERE Id IN: trigger.newMap.keySet()]) {

THIS. IS. HUGE. This changes "newLead" from being a direct reference to Trigger.new context variable (bad news bears... no updates allowed to that special variable) into a simple reference to a bunch of Lead records. But not just any Lead records... specifically, the records that were just DML'd and hence available to you in your Trigger context variables.

So, all that being said I'll give you back some functional, reworked code. I reworked your first IF. I have left debug statements throughout to make a point: you need to start doing this, debugging your code isn't so bad. Your IF/ELSE blocks are on you :) happy coding.

trigger SDRRegionLead on Lead(after insert, after update) {
List<Lead> leadUpdate = new List<Lead>();

Map < Id, SDR_Region__c > regionmap =  new Map < Id, SDR_Region__c > ([SELECT Id, Corporate_Sales_Region__c, Field_Sales_Region__c, Number_of_Employees_Lower_Fence__c, Number_of_Employees_Upper_Fence__c, SDR_Region_Name__c, States__c FROM SDR_Region__c ]);

for(Lead newLead : [SELECT Id, NumberOfEmployees, Geography__c, SDR_Region__c FROM Lead WHERE Id IN: trigger.newMap.keySet()]) {

If(Trigger.isUpdate) {
    Lead oldLead = Trigger.oldmap.get(NewLead.id);
    If(oldLead.NumberOfEmployees !=  newLead.NumberOfEmployees || oldLead.Geography__c !=  newLead.Geography__c) {
        System.debug('Hit 1! ' + regionmap);
        for (ID idKey : regionmap.keyset()) {
            System.debug('Hit 2! ' + idKey);

            SDR_Region__c SDRR = regionmap.get(idKey);
            System.debug('SDRR: ' + SDRR);

            if (newLead.Geography__c != NULL) {
                System.debug('in your BASE!');

                if (newLead.Geography__c == SDRR.Field_Sales_Region__c && (newLead.NumberOfEmployees >= SDRR.Number_of_Employees_Lower_Fence__c ||  newLead.NumberOfEmployees == SDRR.Number_of_Employees_Lower_Fence__c) &&  newLead.NumberOfEmployees <= SDRR.Number_of_Employees_Upper_Fence__c) {
                    System.debug('omg WAY in your BASE!!');
                    newLead.SDR_Region__c = SDRR.Id;
                    leadUpdate.add(newLead);
                }
            }
        }
      }
    }
 }

try {
    update leadUpdate;
} catch (Exception e) {
    System.debug('OOPS its on FIRE: ' + e);
}
}
  • Thank you! As for #2 it isnt an ID field just a String field. More or less moving the string from one object record to another based on a lookup. Also yeah I forgot to populate my Map. I found that out when I moved it over to a before inside the other trigger. This is very helpful in understanding and just general best practices. Thank you very much. I'm now in the process of rebuilding the previous Trigger that populates the field I need for this trigger then moving them into the same sequenced before trigger. – Dan Apr 26 '16 at 16:52
  • #1 is either poorly worded or incorrect. OP never tries to set trigger.new = someValue... – Adrian Larson Apr 26 '16 at 16:57
  • @Adrian Larson: OP loops through trigger.new (e.g. for(Lead newLead : trigger.new) ) and attempts to update newLead inside that loop. Intentional or not this is a clear attempt to set (i.e. modify) a read-only list. Perhaps I should add the word "modify"? I'm not sure what you are reacting to. Wording makes perfect sense to me. Am I incorrect? Do you see this differently? – krigi Apr 26 '16 at 17:32
  • It's not setting the list. It's setting a field on a record within the list. Setting the list would mean assigning it a new value as in my first comment. And you can modify trigger.new, just not in an after trigger. – Adrian Larson Apr 26 '16 at 17:34
  • Yep. We're clearly in an after trigger, so my answer is fine within that context. I'll go ahead use the word "modify" (instead of set), clarity is good. I'll reword #1, thanks for your feedback. – krigi Apr 26 '16 at 17:43
1

Here are some issues with your code.

  1. Use before events to modify data on records being operated on via DML.
  2. You can never update records directly from the trigger context.
  3. You have unnecessary Boolean logic.
  4. Your code should be moved out of the trigger body.

The simplest change you can make is to make this a before trigger. You're not doing anything on insert, but that's somewhat tangential at this point. When you are in a before trigger, you can change field values on the trigger context records and those changes will commit to the database. For instance, you could do:

trigger Lead on Lead (before insert, before update)
{
    for (Lead record : trigger.new) record.Name = 'Duplicate!!!';
}

This will actually set the Name values to that and bind to the database. I wouldn't recommend pushing this trigger to production, but it illustrates the point I am trying to make. You don't need an additional DML operation to bind these changes. They "tag along" so to speak.


Related to the above, you cannot ever operate on records from the trigger context. If, for some reason, you really need to, you can update records from the trigger context by changing their reference using deepClone. Note that clone will not work, because it creates a shallow copy with the same reference.

trigger Lead on Lead (after insert, after update)
{
    update trigger.new; // causes error
    List<Lead> clones = trigger.new.deepClone();
    update clones;
}

Here is an illustration of your logic:

Boolean A, B, C, D;
if (A || B) // operation 1
else if (A || C) // operation 2
else if (A || D) // operation 3

Your second and third branches will never have A be true, because if it were it would have hit the first branch. You can simplify to:

Boolean A, B, C, D;
if (A || B) // operation 1
else if (C) // operation 2
else if (D) // operation 3

I left this off at first, but I think it may help you down the road. Your triggers will be much easier to maintain if you keep as much code out of them as possible. The simplest way to do so is to implement a Service Layer, and make calls to that from your trigger. The Handler Pattern can also help, but it is more complicated when you're just getting started. I added in how I would simplify separation of concerns using the Selector library as well. Here's the basic idea:

public with sharing class LeadServices
{
    public static Select.Filter hasEmployeeCountChanged()
    {
        return Select.Field.hasChanged(Lead.NumberOfEmployees);
    }
    public static void setRegion(List<Lead> filterResults)
    {
        // populate field here
    }
}

public trigger Lead on Lead (before insert, before update)
{
    if (trigger.isBefore)
    {
        if (trigger.isInsert)
        {
            // if any insert logic becomes necessary
        }
        if (trigger.isUpdate)
        {
            LeadServices.setRegion(
                LeadServices.hasEmployeeCountChanged()
                    .filter(trigger.new, trigger.oldMap)
            );
            LeadServices.someOtherMethod(someOtherFilterResult);
            //etc.
        }
    }
}
  • Thank you for the information. The info about the after trigger is super helpful. As for the Boolean logic they reference different A's so I'm unsure how I have the same logic for each. – Dan Apr 26 '16 at 15:43
  • @Dan A is equal to oldLead.NumberOfEmployees != newLead.NumberOfEmployees, which is the same for all three... – Adrian Larson Apr 26 '16 at 15:45
  • Oh dang it! I totally missed that. Thanks! Have to rethink that as if Number of Emp changes I want it to re-calculate as that might change what region it belongs to. Back to the drawing board. – Dan Apr 26 '16 at 15:47
  • Not exactly elegant but I think this is more of what I was aiming for. If((oldLd.NumberOfEmployees != newLd.NumberOfEmployees && oldLd.Geography__c == newLd.Geography__c) || oldLd.Geography__c != newLd.Geography__c) – Dan Apr 26 '16 at 15:53
  • @Dan Take a look at the Selector library, it makes writing filters much simpler. You should also look at trigger frameworks, code in your trigger body makes it much harder to maintain. – Adrian Larson Apr 26 '16 at 15:59

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.