Looking at your code, it shouldn't come as a surprise that it's updating all accounts (with opps).
- You query for Opportunities where AccountId != null (which is likely to be all of the Opportunities in your org)
- You iterate over them to add the account ids
- You then use a query to sum all opps on all accounts you just found
Based on the argument that your method takes, you're getting a List of accounts. If you're only interested in recalculating this sum for the Accounts that are being updated in some transaction, then there's literally no reason for you to perform the first query.
Set<Id> opportunityWithAccountSet = new Set<Id>(); // List of opportunity records without Account List<Opportunity> opportunityRecords = [SELECT Id, AccountId, Name FROM Opportunity WHERE AccountId != Null]; // Storing the Accounts with opportunities for(Opportunity oppObj : opportunityRecords){ opportunityWithAccountSet.add(oppObj.AccountId); }
can be replaced with
for(Account acct :listOfUpdatedAccounts){ opportunityWithAccountSet.add(acct.Id); }
Some other notes:
- You should never use a
static Boolean to try to prevent trigger recursion. If you update more than 200 Accounts, this code would only run on the first 200. - If you see something like
<boolean value> == true, you can (should?) replace it with simply <boolean value>. The == operator checks for equality and gives you a boolean value as a result. Since you already have a boolean, there is no need to do an equality comparison x == true returns true when x is true, and false when x is false. In other words, x == true and x are exactly equivalent (and likewise, x == false is the same as !x). - Checking things like
if(mapAcc.values()!= null && mapAcc.size() > 0) before performing DML generally provides no benefit and is a waste of effort. An initialized map will never be null (nor will the list you get from values(), and Salesforce is smart enough to not perform DML if the collection is empty. - Since you're working on accounts, and wanting to update the same accounts that are taking part in the trigger, this should be run as part of a
before update trigger. That makes the DML at the end (as well as your recursion checking) unnecessary. - The "automatic update" for records in before triggers only happens when you modify the instance of the record contained in trigger.new or trigger.newMap. Lucky for us, putting records into Lists and Maps makes a reference to the original record rather than a copy of it. If you pass
trigger.new into listOfUpdatedAccounts (or loop over trigger.new to make a list that you then pass in as an argument to your handler), then you have a reference to the record in trigger.new. Changes to a reference also change the original instance.
The one case that most people forget about when trying to "roll their own" rollup code is what happens when a record no longer has any children?. I think that's what your initial query was attempting to handle (finding out which accounts either have no opps or no longer have any opps).
There are times when figuring out what to do with that situation is hard, but in this case you can use the easy approach of "assume all Accounts have no Opps, set a default value for your rollup field, and then roll up data for accounts that do have opps".
Map<Id, Account> accountsToUpdateMap = new Map<Id, Account>(); // default the rollup field to 0 // assuming you're working in a before trigger (which you should be) for(Account acct :incomingAccountsListFromTriggerDotNew){ accountsToUpdateMap.put(acct.Id, acct); acct.Rollup_Field__c = 0; } // perform the "rollup" // If the account has no opps, then we won't update the value in the map. // Such accounts will then assume the default value when Salesforce automatically // updates the values for(AggregateResult ar :[SELECT AccountId, SUM(Amount) total FROM Opportunity WHERE AccountId IN :accountsToUpdateMap.keySet() GROUP BY AccountId]){ Account acct = accountsToUpdateMap.get((Id)ar.get('AccountId')); acct.Rollup_Field__c = (Decimal)ar.get('total'); }
trigger triggername on Account (before update)List<Account> listOfUpdatedAccountscontains all accounts? how do you obtain this list?