What is wrong with this apex trigger to cascade address updates?

Question

I've only just started using apex so I'm having trouble pinpointing what is wrong with this code. It is a trigger on accounts which is intended to cycle through all contacts belonging to an account after the account's billing address is updated, and if their mailing address is the same as the account's old billing address, update the contact's to the new one on the account.

Can anyone shed some light on what is going wrong here?

trigger CascadeAddressUpdate on Account (after update)
{
/*If the billing address is changed, check all contacts of this account 
who have their mailing address set to the billing address and update them*/
String oldBillingStreet = '';
String oldBillingCity = '';
String oldBillingState = '';
String oldBillingCountry = '';
String oldBillingPostalCode = '';

for (Account a : Trigger.new)
{
    oldBillingStreet = Trigger.oldMap.get(a.Id).BillingStreet;
    oldBillingCity = Trigger.oldMap.get(a.Id).BillingCity;
    oldBillingState = Trigger.oldMap.get(a.Id).BillingState;
    oldBillingCountry = Trigger.oldMap.get(a.Id).BillingCountry;
    oldBillingPostalCode = Trigger.oldMap.get(a.Id).BillingPostalCode;

    List<Contact> contacts = new List<Contact>([SELECT Id, 
                                         FirstName, 
                                         LastName, 
                                         MailingStreet,
                                         MailingCity, 
                                         MailingCountry, 
                                         MailingState, 
                                         MailingPostalCode 
                                         FROM Contact WHERE AccountId = :a.Id]);

    //Cycle through list of contacts
    for (Contact c : contacts)
    {
        //If the contact's address information matches the old account address information, change contact address to new account address.
        if (c.MailingStreet == oldBillingStreet)
        {
            c.MailingStreet = a.BillingStreet;
        }
        if (c.MailingCity == oldBillingCity)
        {
            c.MailingCity = a.BillingCity;
        }
        if (c.MailingState == oldBillingState)
        {
            c.MailingState = a.BillingState;
        }
        if (c.MailingCountry == oldBillingCountry)
        {
            c.MailingCountry = a.BillingCountry;
        }
        if (c.MailingPostalCode == oldBillingPostalCode)
        {
            c.MailingPostalCode = a.BillingPostalCode;
        }
    }
}    
}

Answer 1

PepeFloyd is correct about not having the SOQL inside the loop, check out the various articles on bulkifying triggers.

As to why it's not working you need to explicitly call an update on the contacts you're changing. I also think you're logic is flawed, you're potentially going to change some address elements and not others ???

The lazy (and bad way) to do this is to just add an update contacts after your loop, this will also cause governor limit issues when updating more than a few records. So the better answer is to do the following:

1. Declare a new List of contacts at the start
2. Each time you find a contact to update, add it to the list
3. Outside of the Account loop call the update of the contacts changed
4. Change your logic so you only replace addresses that exactly matched the old address

e.g. Without bulkifying

trigger CascadeAddressUpdate on Account (after update)
{
/*If the billing address is changed, check all contacts of this account 
who have their mailing address set to the billing address and update them*/
String oldBillingStreet = '';
String oldBillingCity = '';
String oldBillingState = '';
String oldBillingCountry = '';
String oldBillingPostalCode = '';
List<Contact> updateContacts = new List<Contact>();

for (Account a : Trigger.new)
{
    oldBillingStreet = Trigger.oldMap.get(a.Id).BillingStreet;
    oldBillingCity = Trigger.oldMap.get(a.Id).BillingCity;
    oldBillingState = Trigger.oldMap.get(a.Id).BillingState;
    oldBillingCountry = Trigger.oldMap.get(a.Id).BillingCountry;
    oldBillingPostalCode = Trigger.oldMap.get(a.Id).BillingPostalCode;

    Boolean contactUpdated = false;
    List<Contact> contacts = new List<Contact>([SELECT Id, 
                                         FirstName, 
                                         LastName, 
                                         MailingStreet,
                                         MailingCity, 
                                         MailingCountry, 
                                         MailingState, 
                                         MailingPostalCode 
                                         FROM Contact WHERE AccountId = :a.Id]);

    //Cycle through list of contacts
    for (Contact c : contacts)
    {
        //If the contact's address information matches the old account address information, change contact address to new account address.
        if (c.MailingStreet == oldBillingStreet &&
            c.MailingCity == oldBillingCity &&
            c.MailingState == oldBillingState &&
            c.MailingCountry == oldBillingCountry &&
            c.MailingPostalCode == oldBillingPostalCode) 
        {
            c.MailingStreet = a.BillingStreet;
            c.MailingCity = a.BillingCity;
            c.MailingState = a.BillingState;
            c.MailingCountry = a.BillingCountry;
            c.MailingPostalCode = a.BillingPostalCode;
            updateContacts.add(c);
        }
    }
}   

// Ideally try-catch this for handling errors
if (!updateContacts.isEmpty()) update updateContacts; 

}

Answer 2

You are not updating the contacts at the end. Try below code

trigger CascadeAddressUpdate on Account (before update){
    Map<Id, Account> acctsWithNewAddresses = new Map<Id, Account>();
    List<Contact> updatedContacts = new List<Contact>();

    for (Integer i = 0; i < Trigger.new.size(); i++) {
        if (   (Trigger.old[i].BillingStreet != Trigger.new[i].
                                                 BillingStreet)
            || (Trigger.old[i].BillingCity != Trigger.new[i].
                                                 BillingCity)
            || (Trigger.old[i].BillingState != Trigger.new[i].
                                                 BillingState)
            || (Trigger.old[i].BillingCountry != Trigger.new[i].
                                                 BillingCountry)
            || (Trigger.old[i].BillingPostalCode != Trigger.new[i].
                                                BillingPostalCode))  {
            acctsWithNewAddresses.put(Trigger.old[i].id,
                                      Trigger.new[i]);
        }
    }

    for (Contact c : [SELECT id, accountId, MailingCity,
                                 MailingCountry, MailingPostalCode,
                                 MailingState, MailingStreet
                          FROM contact
                          WHERE accountId 
                                in :acctsWithNewAddresses.keySet()]){
        Account parentAccount = acctsWithNewAddresses.get(c.accountId);
        c.MailingStreet = parentAccount.BillingStreet;
        c.MailingCity = parentAccount.BillingCity;
        c.MailingState = parentAccount.BillingState;
        c.MailingCountry = parentAccount.BillingCountry;
        c.MailingPostalCode = parentAccount.BillingPostalCode;

        updatedContacts.add(c);
    }
    update updatedContacts;        
}

NOTE: When writing a trigger,

• Always check for required field changes before doing any SOQLs
• Always try to perform minimum SOQL and DMLs by avoiding them inside loops.
• Try to use before triggers if you need to update fields on the trigger object itself.