-1

My trigger is given below. Whenever new contacts are inserted, total_salary__c on the account object should increment or decrement

trigger updateAccountSalaryNew on Contact (after insert,after update,after delete) {
    Set<Id> accountIds = new Set<Id> ();
    List<Account> lstAccountsToUpdate = new List<Account>();
    
    if(Trigger.isInsert || Trigger.isUpdate) {
        
        for (Contact objContact : trigger.new) {
        accountIds.add(objContact.AccountId);
     } 
   }   
    
   if(Trigger.isDelete ) {
      for (Contact objContact : trigger.old) {
       accountIds.add(objContact.AccountId);
       }
   }
    
    List<Account> accountsToUpdate = [SELECT Id, Number_Of_Contacts__c,Total_Salary__c, (select id,Salary__c from contacts) FROM Account WHERE Id IN :accountIds];
    
    if (!accountIds.isEmpty()) {
        for (Account objAccount : accountsToUpdate) {
            for (Contact updatedContact : Trigger.new) {
                if(objAccount != null){
                    if(Trigger.isInsert) {
                        if(objAccount.Total_Salary__c != null){
                            objAccount.Total_Salary__c += updatedContact.Salary__c;
                            objAccount.Number_Of_Contacts__c = objAccount.Contacts.size();
                            lstAccountsToUpdate.add(objAccount); 
            
                        }
                        else {
                            objAccount.Total_Salary__c = updatedContact.Salary__c;
                            objAccount.Number_Of_Contacts__c = objAccount.Contacts.size();
                            lstAccountsToUpdate.add(objAccount); 
                   
                        }
            
            
                    } 
                }
            }
        }
    }
    update lstAccountsToUpdate; 
}

this is my test class

@isTest
public class updateAccountSalaryTestNew {
  @isTest
    static void testInsert() {
        Account acc = new Account();
        acc.Name = 'Test Salary Update New Account';
        insert acc;
        
        List<Contact> conList = new List<Contact>();
        for (Integer i = 0; i < 300; i++) {
            Contact contact = new Contact(FirstName = 'Test' + i, LastName = 'Name', AccountId = acc.Id,Salary__c = 50000);
            conList.add(contact);
        }
        
        Test.StartTest();
         insert conList;
        Test.StopTest();
        
        List<Account> accountsUpdated = [SELECT Id, Number_Of_Contacts__c,Total_Salary__c FROM Account WHERE Id = :acc.Id];
        
        System.assertEquals(300, accountsUpdated[0].Number_Of_Contacts__c);
        
      
        Decimal expectedTotalSalary = 50000 * 300;
        System.assertEquals(expectedTotalSalary, acc.Total_Salary__c);
        //System.assertEquals(acc.Total_Salary__c, acc.Number_Of_Contacts__c *50000);
    
    }
}

help me to solve the error

System.DmlException: Insert failed. First exception on row 0; first error: CANNOT_INSERT_UPDATE_ACTIVATE_ENTITY, updateAccountSalaryNew: execution of AfterInsert

caused by: System.ListException: Duplicate id in list: 0015g00001SHF2yAAH

Trigger.updateAccountSalaryNew: line 44, column 1: []

1
  • You should probably look into using the Declarative Lookup Rollup Summary tool before trying to "roll your own" code for this (pun very much intended). And before anyone else suggests it, no, OP can't just use a standard rollup summary field here ("Contact" is not an option when making a rollup summary field from Account).
    – Derek F
    Nov 22, 2023 at 19:07

2 Answers 2

2

As a preface, consider using DLRS. It does what you're trying to do without you having to write any code.

That said, you have a couple of problems. First, you're not using the undelete trigger event, so your values can become out of sync if records are recovered from the recycle bin. Second, you have a for loop inside a for loop, meaning that each account will be added to the list for every contact in the trigger (that's why you got the duplicate ID error). Third, this code is horribly inefficient and would cause problems if even a modest amount of contacts are linked to the account. Fourth, the total salary field isn't even being updated correctly. You'd have to write even more code to get the correct sum. Finally, while I will write this as a trigger, it is considered a "best practice" to use a trigger framework, and place your business logic in a helper class.

Here's an optimized version of this code.

trigger updateAccountSalaryNew on Contact (after insert, after update, after delete, after undelete) {
    void extractIds(Set<Id> accountIds, Contact[] records) {
        if(records != null) {
            for(Contact record: records) {
                accountIds.add(record.AccountId);
            }
        }
    }
    Set<Id> accountIds = new Set<Id>();
    extractIds(accountIds, Trigger.old);
    extractIds(accountIds, Trigger.new);
    Map<Id, AggregateResult> contactData = new Map<Id, AggregateResult>([
        SELECT AccountId Id, SUM(Salary__c) totalSalary, COUNT(ID) totalCount
        FROM Contact
        WHERE AccountId = :accountIds
        GROUP BY AccountId
    ]);
    Account[] accountsToUpdate = new Account[0];
    for(Id accountId: accountIds) {
        AggregateResult result = contactData.get(accountId);
        Account updatedInfo = new Account(
            Id=accountId,
            Total_Salary__c = (Decimal)result?.get('totalSalary'),
            Number_Of_Contacts__c = (Integer)result?.get('totalCount')
        );
    }
    update accountsToUpdate;
}

This code first gets the account Ids, then queries all of the related contact data using an aggregate result query, maps that data to the account records, and finally updates the accounts.

Some lesser notes. First, you don't need to guard against looping over an empty list. It's faster to not check if the list is empty first. Second, the record inside an index from a query will never be null.

1
  • Thank you so much for immediate response. Its really helpful for me as I'm a newbie in salesforce
    – sanu
    Nov 23, 2023 at 6:15
2

Well, the immediate source of the error is because your code is structured like this

for (Account objAccount : accountsToUpdate) {
    for (Contact updatedContact : Trigger.new) {
        // Yes, I've removed some code
        // It is not relevant to the point I'm currently making
        lstAccountsToUpdate.add(objAccount); 
    }
}

If you have N Accounts, you're adding each one to your list one time for every Contact. There's no check for whether you've already added a given Account into that list.

A Map would be more appropriate here, and just as appropriate would be to move adding the Account to the collection to be outside of your inner loop.

This is not the only issue with your trigger, it's just the reason why you're getting that particular error.

Other issues include:

  • Your nested loop is iterating over all the Contacts in trigger.new for each Account, regardless of whether the Contact and Account are related
  • You aren't taking into consideration what happens when a Contact is updated to move to a different Account (the Account the Contact was previously related to is not updated)
  • Using += when doing a rollup is going to lead to double incrementing if used in an update context and you have any other code/workflow/flow that causes the triggers to run one more time (the safe approach is to do a full re-count/sum of all records)
  • You aren't aggregating the salary in the update/delete cases
  • Calling .size() on the results of a subquery is unsafe (you'll get an error if you have ~200 Contacts related to a single Account)

Some other things that aren't issues, but are poor practice:

  • You don't need to check accountIds.isEmpty() before iterating over accountsToUpdate. If your query returned 0 rows, the loop will simply not execute
  • You don't need the objAccount != null check inside your loop. Your loop is being fed from the results of a query, there will never be a null account
  • Using a nested loop to iterate over every element in two collections is massively inefficient. When you have a nested loop, you should do some pre-processing so that you only need to loop over the records that are relevant/related to the record in the outer loop

sfdcfox's approach is elegant (and took me a while to realize that it handles the case where an Account has no Contacts, something that is commonly forgotten by people making their own rollup code). Just for sake of giving more substance to my answer, I'll provide a slightly different approach.

trigger ManualRollup on Child__c (after insert, after update, after delete, after undelete) {
    Map<Id, Parent__c> parentsToUpdate = new Map<Id, Parent__c>();
    Set<Id> parentIdSet = new Set<Id>();
    List<Child__c> records = new List<Child__c>();

    // The first thing we need to do is to gather parent ids
    //   so we only need to perform a single query
    // trigger.new will be null in "delete" contexts, so we need to plan
    //   for that (can't just iterate over trigger.new)
    records.addAll(trigger.new == null ? new List<Child__c>() : trigger.new);
    records.addAll(trigger.old == null ? new List<Child__c>() : trigger.old);

    // "records" will contain both new and old data (if available)
    // Getting the old parent id is important for properly handling the case
    //   where a child moves between parents (which is a common thing
    //   people forget about handling)
    for(Child__c child :records) {
        parentIdSet.add(child.Parent_Lookup__c);

        // This is also a fantastic time to set up our "default state" for the parents
        // If a Parent no longer has any children, this is the data we'll use
        parentsToUpdate.put(child.Parent_Lookup__c, new Parent__c(
            Id = child.Parent_Lookup__c,
            Sum_Field__c = 0,
            Child_Count__c = 0
        ));
    }

    // Now we can execute the main query
    // Let the query do the work of summing/counting/etc...
    for(AggregateResult ar :[SELECT Parent_Lookup__c, COUNT(Id) numChildren, SUM(Number_Field__c) total FROM Child__c WHERE Parent_Lookup__c IN :parentIdSet GROUP BY Parent_Lookup__c]) {
        // The downside to using SOQL to aggregate data is that we need to do some
        //   typecasting and also be aware of potentially null values

        // We use the same parentsToUpdate map here so that we overwrite the defaults
        //   that were previously set (since in here, we know that we have data to overwrite
        //   it with)
        // SObjects are stored in collections as references, so we can just .get()
        //   from the map and update fields
        Parent__c parent = parentsToUpdate.get((Id)ar.get('Parent_Lookup__c'));

        // Apex doesn't have a null coalescing operator at time of writing
        //   so we gotta use if/else or a ternary to handle potential nulls
        parent.Sum_Field__c = (Decimal)ar.get('total') == null ? 0 : (Decimal)ar.get('total');
        parent.Child_Count__c = (Decimal)ar.get('numChildren') == null ? 0 : (Decimal)ar.get('numChildren');
    }

    parentsToUpdate.remove(null);
    update parentsToUpdate.values();
}

A tad less optimized, but perhaps a tad more readable for most people.

1
  • Thank you so much for immediate response. Its really helpful for me as I'm a newbie in salesforce
    – sanu
    Nov 23, 2023 at 6:14

You must log in to answer this question.

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