2

I've written the following trigger to run every time an Opportunity is updated and it's used to update a field in all of the Contacts associated with that opp (via the Contact Roles). However, this is currently not bulk-safe as there are SOQL queries inside of for-loops, and I can't figure out how to fix that. Can I get any help?

trigger OppStageChange on Opportunity (after update) {

for(Opportunity opp : trigger.new) {        
    Opportunity o = [SELECT id, (select contact.Id from OpportunityContactRoles) FROM Opportunity where id =:opp.id];
    System.debug(o.OpportunityContactRoles.size());
    for(Integer i = 0; i < o.OpportunityContactRoles.size(); i++)
    {
        List<Contact> c = [SELECT Id, Stage__c, Closed__c, Stage_Counter__c, Lost_Reason__c FROM Contact WHERE Id = :o.OpportunityContactRoles[i].Contact.Id];
        if(c.size() > 0)
        {
            System.debug(c[0].Id);
            if(c[0].Closed__c != TRUE)
            {
                if((opp.StageName == 'Closed Won') || (opp.StageName == 'Booked'))
                {
                    c[0].Stage__c = 'Closed Won';
                    c[0].Closed__c = TRUE;
                } else if(opp.StageName == 'Closed Lost')
                {
                    c[0].Stage__c = 'Closed Lost';
                    c[0].Closed__c = TRUE;
                    c[0].Lost_Reason__c = opp.Rejected_Reason__c;
                } else if(opp.Stage_Counter__c > c[0].Stage_Counter__c)
                {
                    c[0].Stage__c = opp.StageName;
                }
                update c;       
            }   
        }
    } 
}

}

2

There are a couple of ways you can do this.

1) Start off by getting all the data you need up front and putting it in a map to easily access later:

Map<Id, List<Id>> oppIdToRoleIdMap = new Map<Id, List<Id>>();
Set<Id> contactIds = new Set<Id>();
for (OpportunityContactRole role: [SELECT OpportunityId, ContactId FROM OpportunityContactRole WHERE OpportunityId IN :Trigger.new])
{
  List<Id> oppRoleIds = oppIdToRoleIdMap.get(role.OpportunityId);
  if (oppRoleIds == null)
  {
    oppRoleIds = new List<Id>();
    oppIdToRoleIdMap.put(role.OpportunityId, oppRoleIds);
  }
  oppRoleIds.add(role.ContactId);
  contactIds.add(role.ContactId);
}

Map<Id, Contact> contactMap = new Map<Id, Contact>{[SELECT Stage__c, Closed__c, Stage_Counter__c, Lost_Reason__c FROM Contact WHERE Id IN :contactIds};

Now as you iterate over the Opportunities:

for (Opportunity opp: Trigger.new)
{
  //This will give you ALL of the related roles to an Opportunity.  Since we queried for the Contact data
  //here you also have access to that as well.
  List<Id> contactIds = oppIdToRoleIdMap.get(opp.Id):
  for (Id contactId: contactIds)
  {
    Contact cont = contactMap.get(contactId);
    //do work here
  }
}

update contactMap.values();

2) It is also possible to do a single query on Opportunities at the start and to grab all of the related records. Then when you want to modify a Contact you would do:

Map<Id, Contact> contactMap = new Map<Id, Contact>();
for (Opportunity opp: opps)
{
  for (OpportunityContactRole role: opp.OpportunityContactRoles)
  {
    Contact cont = new Contact(Id = role.ContactId);
    //do work
    //When you do this work, you can access the contact fields via the role if you queried for them, 
    //but then set the values you need with cont.value

    //The reason I put them in a map instead of a list, is in case you get the same Contact multiple times
    //from different Opportunities
    contactMap.put(role.ContactId, cont); 
  }
}
update contactMap.values();
| improve this answer | |
  • Can you explain why you defined the map oppIdToRoleIdMap with the list as the value and when is that list of ids populated? Will you get a null value every time you do oppIdToRoleIdMap.get(role.OpportunityId) ? – Mihai Neagoe Jan 8 '15 at 15:33
  • I am populating it in the same loop it checks for it to be null. The first iteration of each different opportunity will always be null. If there are multiple roles for each contact, it won't be null for the 2nd one and on. The first will always be null. – dphil Jan 8 '15 at 15:36
  • @mihaiNeagoe I am doing the list as the value because it represents all of the related roles to each specific opp. – dphil Jan 8 '15 at 15:43
  • It's interesting how you've done it, never seen it done like that before. Also from what I see the map roleToContactIdMap is not defined in the answer? Let me see if I understood it, let`s say you have 10 different opportunites with 10 different opportunityContactRoles, it will always be null. – Mihai Neagoe Jan 8 '15 at 15:46
  • 1
    @MihaiNeagoe oops I accidentally changed its name oppIdToRoleIdMap should be that. I'm on my phone atm and having trouble editing my answer from here. – dphil Jan 8 '15 at 15:51
1
Set<ID> ids = Trigger.newMap.keySet();
List<Opportunity> oList = [SELECT id, (select contact.Id from OpportunityContactRoles) FROM Opportunity where id IN :ids]; //query more fields if you need to

this will fix your first problem(don't forget to remove that query from your loop). You should also change this line:

for(Opportunity opp : trigger.new) {

to this:

for(Opportunity o : oList) {

To fix the second query you should add a small for-loop before the other loops and get all IDs of Opportunity Contacts thing in there:

Set<Id> oppContRolesContIds = new Set<Id>();
for (Opportunity o: oList) {
  for(Integer i = 0; i < o.OpportunityContactRoles.size(); i++) {
    oppContRolesContIds.add(o.OpportunityContactRoles[i].Contact.Id);
  }
}

Now that you have a list of those IDs you can make 1 query:

List<Contact> c = [SELECT Id, Stage__c, Closed__c, Stage_Counter__c, Lost_Reason__c FROM Contact WHERE Id IN :oppContRolesContIds];
| improve this answer | |
1

Firstly, declare a List<Contact> contacts =new List<Contact>(); and a Set<Id> oppIds = trigger.newMap.keySet(); then you can use that set to query all Opportunities at once and loop through those instead of trigger.new.
You can also combine the two queries, so we'd get one query defining the loops that looks like

for(Opportunity o :  [SELECT id,  
                         (select contact.Id, contact.Stage__c, contact.Closed__c, contact.Stage_Counter__c, contact.Lost_Reason__c from OpportunityContactRoles) 
                      FROM Opportunity where id IN :oppIds]){  

Then you can subloop and grab your contact records like

for(OpportunityContactRole ocr : o.getSObjects('OpportunityContactRoles')){  
    Contact c = ocr.getSObject('Contact');
    //logic here
    contacts.add(c);
}  

and do a single call to update contacts at the very end, outside of the loops.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.