1

suggestion required: Below trigger updates a parent record (an Account) fields based on changes to its related child records (Opportunities). The trigger works well and updates the parent record correctly. However, there's one situation where it doesn't work as intended:

Scenario: There is an account (a1) has only one opportunity (opp1). If I update opportunity opp1 to different account (a2), the trigger updates a2's fields as expected, but (a1) old account doesn't get updated. In this case, I want a1's fields to be updated to null or 0 because opp1 has been moved to a2, leaving a1 with no opportunities.

I understand why this is happening. the trigger operates after an update, and it uses an query on the opportunities. When o1 is moved to a2, there are no opportunities left for a1, so the query doesn't return anything return for a1

I could solve this by changing the trigger to use a query on the parent records. However, my question is whether there's a way to fix this in the current code without making significant changes. basically, I don't want to use sub-query.

trigger oppTri on Opportunity (after insert ,after undelete,after delete,after Update ) {
    List<Opportunity> opp = trigger.isDelete ? trigger.old : trigger.new ;
    if(trigger.isAfter)
    {
       Map<ID,Account> mapAcIDAggOppVal = new Map<ID,Account>();
       set<ID> accID = new set<ID>();
       if(trigger.isInsert || trigger.isUndelete || trigger.isDelete)
       {
         for(Opportunity oppeach :opp)
            {
               if(oppeach.AccountID != null)
                {
                   accID.add(oppeach.AccountID);
                }
            }
       }
        if(trigger.isUpdate)
        {
             for(Opportunity oppeach :trigger.old)
            {
               if(oppeach.AccountID != null)
                {
                   accID.add(oppeach.AccountID);//old accID
                   accID.add(trigger.newMap.get(oppeach.Id).AccountID);//new accID
                }
            }
        }
        if(!accID.isEmpty())
        {

               for(AggregateResult ar : [select max(Amount) maxAmt ,avg(amount) avgAmt ,AccountID from Opportunity where AccountID in :accID 
                                          group By AccountID] )
                {
                     mapAcIDAggOppVal.put((ID)ar.get('AccountID') ,new Account(ID =(ID)ar.get('AccountID'),
                                                                               avgOfAppOpp__c = (Decimal)ar.get('avgAmt'), 
                                                                               MaxAmountHolder__c = (Decimal)(ar.get('maxAmt')) 
                                                                              ));
                }       
        }
        Update mapAcIDAggOppVal.values();
    }
}

1 Answer 1

2

The Max Amount is probably better handled via a rollup summary field. No matter if you go that route or keep both in your trigger here, the rest of the answer remains the same.

In a nutshell, what your code is missing here is an explicit default state. Since you're not creating a default state, the default state is "whatever the current state of the Account is".

Whenever you're gathering the Id for an Account, you can take that opportunity (no pun intended) to set the avgOfAppOpp__c and MaxAmountHolder__c to 0. That becomes your "default state". The rest of the logic then becomes "but for this Account, this is the actual state".

List<Opportunity> opp = trigger.isDelete ? trigger.old : trigger.new;

// This approach has us updating _every_ record that comes our way
//  We only need this Map (which we can use to filter the query later)
Map<Id, Account> mapAcIDAggOppVal = new Map<Id, Account>();
//Set<Id> accIdSet = new Set<Id>();

// Because you did the ternary to choose between the old and new lists, you
//   don't need this if.
// It can run for updates as well (to get us the "new" state)
//if(trigger.isInsert || trigger.isUndelete || trigger.isDelete) {
for(Opportunity oppeach :opp){
    // My preference is to just put things into maps and sets and
    //   not worry about nulls inside of the loop (because it's
    //   really easy to remove them afterwords)
    Id acctId = oppeach.AccountId;

    // Build our default state
    // If the AggregateResult doesn't have a result for this Id,
    //   then this is the proper state of that particular Account
    mapAcIDAggOppVal.put(acctId, new Account(
        Id = acctId,
        avgOfAppOpp__c = 0,
        MaxAmountHolder__c = 0
    ));
}

// Specifically in the update scenario, we need to get data from the old list
if(trigger.isUpdate){
    for(Opportunity oppeach :trigger.old){
        Id acctId = oppeach.AccountId;

        // Build our default state for the old Account
        mapAcIDAggOppVal.put(acctId, new Account(
            Id = acctId,
            avgOfAppOpp__c = 0,
            MaxAmountHolder__c = 0
        ));
    }
}

// Now that we're done looping, we can get rid of the null Account
mapAcIDAggOppVal.remove(null);

// No need to check if the set (or the map in this reworked code) is empty
// ...unless you specifically want to avoid using a query in that scenario
//if(!mapAcIDAggOppVal.isEmpty()){
for(AggregateResult ar : [SELECT MAX(Amount) maxAmt, AVG(Amount) avgAmt, AccountId FROM Opportunity WHERE AccountID IN :mapAcIDAggOppVal.keySet() 
                                          GROUP BY AccountId]){
    mapAcIDAggOppVal.put((Id)ar.get('AccountId'), new Account(
        Id = (Id)ar.get('AccountId'),
        avgOfAppOpp__c = (Decimal)ar.get('avgAmt'), 
        MaxAmountHolder__c = (Decimal)(ar.get('maxAmt')
    ));
}

update mapAcIDAggOppVal.values();

It's also recommended to keep logic outside of triggers by using a trigger framework (or, if nothing else, just keeping the logic in a class and calling the class from your trigger).

Doing that allows us to test the logic in isolation, and without the need to perform DML (outside of test setup and the DML contained in the code being tested). For code that doesn't perform update/delete/undelete or make queries, it allows us to skip performing any DML in the test (which means the test will be very fast to execute).

Having the logic in its own class/method that takes some lists (and/or maps) as input also breaks the dependency on trigger context variables (which is also important for ease of testing and isolation).

2
  • Thank you very much, @Derek. Your help has been really effective. I have a related question: Why is it necessary to remove null values from the map mapAcIDAggOppVal? instead of that we can also do null check before insertion of data in map right ?? Regarding the handler, I was simply practicing a few scenarios. That's why I didn't include a handler class or a framework. In a project work, I usually rely on a framework. Aug 26, 2023 at 15:17
  • @Gauravkumar Yep, you can have an if(oppeach.AccountId == null){ continue; } or something similar inside of the loops. I just prefer to do that work outside of the loop (doing less work in each iteration of the loop, slightly less typing).
    – Derek F
    Aug 26, 2023 at 17:15

You must log in to answer this question.

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