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
Gabe RothmanGabe Rothman 

Can someone help me to bulk safe this trigger? Not sure how to move my SOQL queries outside of their for-loops

trigger OpptyOwnerChangeAlert on Opportunity (before update) {

  List<Messaging.SingleEmailMessage> mails= new List<Messaging.SingleEmailMessage>();
  List<User> obsoleteUsers =  new List<User>();
  
  //Ensure that both Maps contain the same ID's
  system.assert(trigger.oldMap.keySet().containsAll(trigger.newMap.keySet()));  
  system.assert(trigger.newMap.keySet().containsAll(trigger.oldMap.keySet()));
    
  system.assert(trigger.oldMap.keySet().size()>0);
  
  Boolean test=false;
  
  Opportunity oldOpp = null;
  User oldOppOwner = null;
  Opportunity newOpp = null;
  User newOppOwner = null;
  
  for (Opportunity opp : trigger.new) {
    newOpp=opp;
    newOppOwner=[select Id from User where Id=:newOpp.OwnerId];
    if ((trigger.oldMap!=null)&&(trigger.oldMap.get(newOpp.Id)!=null)) {
      oldOpp=trigger.oldMap.get(newOpp.Id);
      oldOppOwner=[select Id,Email from User where Id=:oldOpp.OwnerId];
    }
    system.assert(oldOpp!=null);
    system.assert(oldOppOwner!=null);
    system.assert(newOpp!=null);
    system.assert(newOppOwner!=null);
    if (newOppOwner.Id != oldOppOwner.Id) {
       obsoleteUsers.add(oldOppOwner);   
    }
  }
    
  //obsolete_users = [Select Name, id, Email From User where Id in :obsolete_user_ids];
  for(User u: obsoleteUsers) {
    Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
    mail.setSubject('[ALERT] Someone has snaked one of your Opportunities ');
    List<String> toAdrs = new List<String>();
    toAdrs.add(u.Email);
    system.assert(u.Email!=null);
    if (u.Email==null) continue; 
    mail.setToAddresses(toAdrs);
    mail.setBccSender(false);
    mail.setUseSignature(false);
    String mailContent = 'Someone is messin with your money; ownership of'+' '+'-'+' '+ newOpp.Name +' '+'-'+' '+'has been changed. Please contact your Admin in order to arrange your grudge match in the Thunderdome' ;
    mail.setPlainTextBody(mailContent);
    mails.add(mail); 
  }
  if (mails.size()>0){
    Messaging.sendEmail(mails);
  }
}
Thanks a bunch for your help!
Best Answer chosen by Gabe Rothman
James LoghryJames Loghry
Gotcha.  In that case, you'll need just a bit more logic.  Below is an example of how you might accomplish this.. but there are other ways of accomplishing the same result.  This is just my thinking on a Monday morning pre-coffee, so take it with a grain of salt. :)

trigger OpptyOwnerChangeAlert on Opportunity (before update) {
    Set<Id> obsoluteUserIds = new Set<Id>();

    Map<Id,List<Opportunity>> oldOwnerToOpportunitiesMap = new Map<Id,List<Opportunity>>();
    List<Opportunity> oppsWithNewOwners = new List<Opportunity>();
    for (Opportunity opp : trigger.new) {
        Id oldOwnerId = oldMap.get(opp.Id).OwnerId;
        if (oldOwnerId != opp.OwnerId){
            List<Opportunity> ownersOpps = oldOwnerToOpportunitiesMap.get(oldOwnerId);
            if(ownerOpps == null){
                ownerOpps = new List<Opportunity>();
                oldOwnerToOpportunitiesMap.put(oldOwnerId,ownerOpps);
            }
            ownerOpps.add(opp);
        }
    }
     
    for(User u : [Select Id,Email From User Where Id in :obsoleteUserIds]){
        for(Opportunity opp : oldOwnerToOpportunitiesMap.get(u.Id)){
            Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
            mail.setSubject('[ALERT] Someone has snaked one of your Opportunities ');
            List<String> toAdrs = new List<String>();
            toAdrs.add(u.Email);
            if (u.Email==null) continue;
            mail.setToAddresses(toAdrs);
            mail.setBccSender(false);
            mail.setUseSignature(false);
            String mailContent = 'Someone is messin with your money; ownership of'+' '+'-'+' '+ opp.Name +' '+'-'+' '+'has been changed. Please contact your Admin in order to arrange your grudge match in the Thunderdome' ;
            mail.setPlainTextBody(mailContent);
            mails.add(mail);
        }
    }

    if (mails.size()>0){
        Messaging.sendEmail(mails);
    }
}

All Answers

James LoghryJames Loghry
Here's how you could bulkify your trigger and simplify it pretty drastically:
   
Set<Id> obsoluteUserIds = new Set<Id>();
for (Opportunity opp : trigger.new) {
    if (oldMap.get(opp.Id).OwnerId != opp.OwnerId){
        obsoluteUserIds.add(oldMap.get(opp.Id).OwnerId);
    }
}

for(User u : [Select Id,Email From User Where Id in :obsoleteUserIds]){
   //Do mailing stuffs
}
Note, I am seeing a lot of system asserts in your trigger, and this is rather.. horrifying.  System asserts should only exist in your test cases around your trigger.  Please move your System.asserts into your unit tests instead.
Gabe RothmanGabe Rothman
Thanks James!
Gabe RothmanGabe Rothman

James, one quick follow up question.  I want to be able to reference the name of the opportunity in my email alert, but the variable "opp" exists in a separate logic set from the email alert logic.  How can I reference the opportunity name in the "for(User u: ..." section of my trigger.  Here is the updated code per your previous email:

trigger OpptyOwnerChangeAlert on Opportunity (before update) {

	Set<Id> obsoluteUserIds = new Set<Id>();
	for (Opportunity opp : trigger.new) {
    	if (oldMap.get(opp.Id).OwnerId != opp.OwnerId){
        	obsoluteUserIds.add(oldMap.get(opp.Id).OwnerId);
   		 }
	}
    
   	for(User u : [Select Id,Email From User Where Id in :obsoleteUserIds]){
 	   	Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
 	   	mail.setSubject('[ALERT] Someone has snaked one of your Opportunities ');
    	List<String> toAdrs = new List<String>();
    	toAdrs.add(u.Email);
    	if (u.Email==null) continue; 
    	mail.setToAddresses(toAdrs);
    	mail.setBccSender(false);
    	mail.setUseSignature(false);
    	String mailContent = 'Someone is messin with your money; ownership of'+' '+'-'+' '+ opp.Name +' '+'-'+' '+'has been changed. Please contact your Admin in order to arrange your grudge match in the Thunderdome' ;
    	mail.setPlainTextBody(mailContent);
    	mails.add(mail); 
  	}
	if (mails.size()>0){
    	Messaging.sendEmail(mails);
  	}
}

 
James LoghryJames Loghry
Gotcha.  In that case, you'll need just a bit more logic.  Below is an example of how you might accomplish this.. but there are other ways of accomplishing the same result.  This is just my thinking on a Monday morning pre-coffee, so take it with a grain of salt. :)

trigger OpptyOwnerChangeAlert on Opportunity (before update) {
    Set<Id> obsoluteUserIds = new Set<Id>();

    Map<Id,List<Opportunity>> oldOwnerToOpportunitiesMap = new Map<Id,List<Opportunity>>();
    List<Opportunity> oppsWithNewOwners = new List<Opportunity>();
    for (Opportunity opp : trigger.new) {
        Id oldOwnerId = oldMap.get(opp.Id).OwnerId;
        if (oldOwnerId != opp.OwnerId){
            List<Opportunity> ownersOpps = oldOwnerToOpportunitiesMap.get(oldOwnerId);
            if(ownerOpps == null){
                ownerOpps = new List<Opportunity>();
                oldOwnerToOpportunitiesMap.put(oldOwnerId,ownerOpps);
            }
            ownerOpps.add(opp);
        }
    }
     
    for(User u : [Select Id,Email From User Where Id in :obsoleteUserIds]){
        for(Opportunity opp : oldOwnerToOpportunitiesMap.get(u.Id)){
            Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
            mail.setSubject('[ALERT] Someone has snaked one of your Opportunities ');
            List<String> toAdrs = new List<String>();
            toAdrs.add(u.Email);
            if (u.Email==null) continue;
            mail.setToAddresses(toAdrs);
            mail.setBccSender(false);
            mail.setUseSignature(false);
            String mailContent = 'Someone is messin with your money; ownership of'+' '+'-'+' '+ opp.Name +' '+'-'+' '+'has been changed. Please contact your Admin in order to arrange your grudge match in the Thunderdome' ;
            mail.setPlainTextBody(mailContent);
            mails.add(mail);
        }
    }

    if (mails.size()>0){
        Messaging.sendEmail(mails);
    }
}


This was selected as the best answer
Gabe RothmanGabe Rothman
Ahhh, that makes sense.  This is really helpful.  One thing I can't solve is an error on line 7: "Save error: Variable does not exist: oldMap"  Not sure why it's not recognizing oldMap within the context of a for loop on trigger.new.  Any ideas there?
James LoghryJames Loghry
Yeah, should be "Trigger.oldMap" :)
Gabe RothmanGabe Rothman
oh, duh.  I could've figured that out - thanks for doing my work for me. :-)