function readOnly(count){ }
Sign Up ›
Starting November 20, the site will be set to read-only. On December 4, 2023,
forum discussions will move to the Trailblazer Community.
Learn More

Browse by Topic
ShowAll Questionssorted byDate Posted
+ Start a Discussion
Shruthi NarsiShruthi Narsi 

Trigger to roll up

I have written a trigger to roll up opportunity amount on quota. The code is working fine. I have a problem in line 34.
 If I add two new opportunity with amoth as 100 and 100. The roll up is 200.
Again if I edit it recaclulates and adds 200 which is euql to 400. This logic is wrong how to correct it.

trigger Roll on Opportunity (before insert,before update,after insert,after update){
    Set<Id> setOpportunityIds=new Set<Id>();
    
    for(Opportunity c:Trigger.new)
        if(c.StageName == 'Closed Won' && c.Closed_Date__c == 'January' )
        
        
        setOpportunityIds.add(c.id);
    
    
    
    List<Quota__c> ListOfAllOrgs = [select id,RecordTypeId, Actual_Amount__c,Opportunity__c,Quota_Amount__c from Quota__c];
    List<Quota__c> lstCommissionToUpdate=new List<Quota__c>();
    
    for(AggregateResult result:[Select Quota__c,SUM(Amount) From Opportunity 
                                WHERE Id IN :setOpportunityIds GROUP BY Quota__c LIMIT 1 ])
    {
        
        for(Quota__c org : ListOfAllOrgs)
            
        {
            
                    System.debug('Amount value in Org is' + Decimal.ValueOf(String.ValueOf(result.get('expr0'))));
            if(result.get('Quota__c') == org.Id && org.Actual_Amount__c == null )
            {
                if(  org.RecordTypeId == '0122v000002KFRAAA4')
                {
                    org.Actual_Amount__c = 0;
                }
                else{
                    org.Actual_Amount__c = org.Actual_Amount__c + Decimal.ValueOf(String.ValueOf(result.get('expr0'))); 
                    
                    
                    if(result.get('Quota__c') == org.Id && org.Actual_Amount__c != null )
                    {
                        if(  org.RecordTypeId == '0122v000002KFRAAA4')
                        {
                            org.Actual_Amount__c = 0;
                        }
                        else{
                            org.Actual_Amount__c = org.Actual_Amount__c + Decimal.ValueOf(String.ValueOf(result.get('expr0'))); 
                            
                            System.debug('Amount value in Org is' + Decimal.ValueOf(String.ValueOf(result.get('expr0'))));
                        }
                        
                        
                        
                        
                        lstCommissionToUpdate.add(org);
                        System.debug('Amount value in Org is' + Decimal.ValueOf(String.ValueOf(result.get('expr0'))));
                    }
                }
            }
            if(lstCommissionToUpdate.size()>0){
                
                update lstCommissionToUpdate;
                System.debug('Amount value in Org is' + Decimal.ValueOf(String.ValueOf(result.get('expr0'))));
            }
        }
    }
}
Best Answer chosen by Shruthi Narsi
Andrew GAndrew G
It is difficult to code for custom objects in someone else's org, as I can't compile code, so I will supply some snippets with an explanation.
For recordType Ids, the following will work - replace the string "myRecordTypeName" with the name of your Record Type. 
    String rtName = "myRecordTypeName";
    Id quotaRecordTypeId = Schema.getGlobalDescribe().get(Quota__c).getDescribe().getRecordTypeInfosByName().get(rtName).getRecordTypeId();

  //with the above I can then test for recordtypeId as 
if (tempQuota.RecordTypeId = quotaRecordTypeId) {
  //do something
}

I note also you are doing a query for all the Quota__c records, why not just get those that are affected by the Trigger? 
    Set<Id> setQuotaIds = new Set<Id>();
    for (Opportunity o : Trigger.new) {
        if (o.StageName == 'Closed Won' && o.Closed_Date__c == 'January')        {
            setOpportunityIds.add(o.Id);
            setQuotaIds.add(o.Quota__c);
        }
    }
//why get all the quota records, why not get just the sub set of required quota
    List<Quota__c> listQuotas = [SELECT Id,RecordTypeId, Actual_Amount__c,Quota_Amount__c FROM Quota__c WHERE Id IN :setQuotaIds];

You are then looping within a loop - not the most economical why of processing records, why not covert the list to a Map , and then you can test for keys rather than looping the list.  Remember you were grabbing ALL the Quota__c records.  So if there are 1000 Quota__c records, and there is one one Opportunity, you are doing 1000 loops for not real reason.
SO: 
    Map<Id,Quota__c> mapQuotas = new Map<Id,Quota__c>();
    for (Quota__c q : listQuotas )    {
            mapQuotas.put(Id, q );
    }

Then I can be more economical with my loop: 
    for(AggregateResult result:[SELECT Quota__c,SUM(Amount) FROM Opportunity
            WHERE Id IN :setOpportunityIds GROUP BY Quota__c LIMIT 1 ]) {
        if (mapQuotas.containsKey(result.get(Quota__c))) {
            Quota__c tempQuota = mapQuotas.get(result.get(Quota__c));
            if (tempQuota.RecordTypeId = quotaRecordTypeId) {
                tempQuota.ActualAmount__c = result.get('expr0');
                lstCommissionToUpdate.add(tempQuota);
            }
        }
    }
And note that the other thing you were doing, your update method was inside the for loop for all the Quota__c records, so therefore you would have hit DML limits on any bulk update. 
Stick that size test and update outside any loops.
And technically, to be better code, stick it in a try catch statement.


Regards
Andrew

 

All Answers

Andrew GAndrew G
Is there any particular reason for the code reading as: 
org.Actual_Amount__c = org.Actual_Amount__c + Decimal.ValueOf(String.ValueOf(result.get('expr0')));
You are adding the value to the existing value in the field, which would cause the number to be "incremented" at each run of the trigger.
Can the code not be: 
org.Actual_Amount__c = Decimal.ValueOf(String.ValueOf(result.get('expr0')));


As an aside, I would also question the use of hardcoded Ids in Apex code.  Is there a reason why you can't use  
Id recordypeId = Schema.getGlobalDescribe().get(objectName).getDescribe().getRecordTypeInfosByName().get(recordTypeName).getRecordTypeId();

and also, when posting code, to make it easer to read on the forums, use the Code Snippet button
User-added image

Regards
Andrew

 
Shruthi NarsiShruthi Narsi
Hi Andrew 

Thanks for letting me know the corrections

Can you code if possible becuse I didnt understand what you are trying to tell me.
Shruthi NarsiShruthi Narsi
Hi Andrew 

Thanks for letting me know the corrections

Can you code if possible becuse I didnt understand what you are trying to tell me.
Andrew GAndrew G
It is difficult to code for custom objects in someone else's org, as I can't compile code, so I will supply some snippets with an explanation.
For recordType Ids, the following will work - replace the string "myRecordTypeName" with the name of your Record Type. 
    String rtName = "myRecordTypeName";
    Id quotaRecordTypeId = Schema.getGlobalDescribe().get(Quota__c).getDescribe().getRecordTypeInfosByName().get(rtName).getRecordTypeId();

  //with the above I can then test for recordtypeId as 
if (tempQuota.RecordTypeId = quotaRecordTypeId) {
  //do something
}

I note also you are doing a query for all the Quota__c records, why not just get those that are affected by the Trigger? 
    Set<Id> setQuotaIds = new Set<Id>();
    for (Opportunity o : Trigger.new) {
        if (o.StageName == 'Closed Won' && o.Closed_Date__c == 'January')        {
            setOpportunityIds.add(o.Id);
            setQuotaIds.add(o.Quota__c);
        }
    }
//why get all the quota records, why not get just the sub set of required quota
    List<Quota__c> listQuotas = [SELECT Id,RecordTypeId, Actual_Amount__c,Quota_Amount__c FROM Quota__c WHERE Id IN :setQuotaIds];

You are then looping within a loop - not the most economical why of processing records, why not covert the list to a Map , and then you can test for keys rather than looping the list.  Remember you were grabbing ALL the Quota__c records.  So if there are 1000 Quota__c records, and there is one one Opportunity, you are doing 1000 loops for not real reason.
SO: 
    Map<Id,Quota__c> mapQuotas = new Map<Id,Quota__c>();
    for (Quota__c q : listQuotas )    {
            mapQuotas.put(Id, q );
    }

Then I can be more economical with my loop: 
    for(AggregateResult result:[SELECT Quota__c,SUM(Amount) FROM Opportunity
            WHERE Id IN :setOpportunityIds GROUP BY Quota__c LIMIT 1 ]) {
        if (mapQuotas.containsKey(result.get(Quota__c))) {
            Quota__c tempQuota = mapQuotas.get(result.get(Quota__c));
            if (tempQuota.RecordTypeId = quotaRecordTypeId) {
                tempQuota.ActualAmount__c = result.get('expr0');
                lstCommissionToUpdate.add(tempQuota);
            }
        }
    }
And note that the other thing you were doing, your update method was inside the for loop for all the Quota__c records, so therefore you would have hit DML limits on any bulk update. 
Stick that size test and update outside any loops.
And technically, to be better code, stick it in a try catch statement.


Regards
Andrew

 
This was selected as the best answer