Single Email Message bulkification is not working in Apex

Question

Description: When I deactivate multiple users, say a,b,c, the following code is triggered to send emails to their respective managers. but in my case the manager receives the email with only the username of the last user for all emails i.e for a, b, c.

the expected scenario is that the manager should receive email for all 3 users mentioning their corresponding names in the mail as opposed to a single user name in all emails.

Could you please help me with what changes need to be done to make this piece of code work as expected

Apex Code:

public without sharing class ChangeOpportunityOwner {       
    @Future
    public static void changeOwner(List<Id> usrIds){
        Boolean isUpdate=false;
        List<Messaging.MassEmailMessage> messages=new list<Messaging.SingleEmailMessage>();
        Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
        Map<Id, Boolean> mapUserToActive = new Map<Id, Boolean>();
        try{
            for (User u1 : [SELECT Id, isActive FROM User]) { 
                mapUserToActive.put(u1.Id, u1.isActive);
            }
            List <user>usrs= [Select Id,Name,ManagerId,IsActive from user where Id=:usrIds];
            List<Opportunity> finalOptList=new List<Opportunity>();
            for(User u:usrs){   
                List<Opportunity>optys=[Select Id,OwnerId,StageName from Opportunity where OwnerId=:u.Id]; 
                for(Opportunity opt:optys){
                    if(opt.StageName!='Closed Won' && opt.StageName!='Closed Lost')
                    {
                        if (mapUserToActive.get(u.ManagerId)== true && mapUserToActive.get(u.ManagerId)!=null) {
                            opt.OwnerId=u.ManagerId;
                        }
                        else{ 
                            User usr= [Select Id,Name,ManagerId from user where Name='Test Manager'];
                            opt.OwnerId=usr.Id;
                        }
                        finalOptList.add(opt);
                        // Set list of people who should get the email
                        List<String> sendTo = new List<String>();
                        sendTo.add(u.ManagerId);
                        mail.setToAddresses(sendTo);
                        //  Set who the email is sent from
                        mail.setReplyTo('[email protected]');
                        mail.setSenderDisplayName('Customer Service');
                        // Set email contents - you can use variables!
                        mail.setSubject('Open Opportunities transferred to you');
                        String body = 'Dear Manager,<br> ';
                        body +='<br>';
                        body += 'Please note that '+u.name+'  has been deactivated as a Salesforce User hence all of its <br> Open Opportunities has been assigned to you if any.<br>';
                        body +='<br>';
                        body +='Kind regards,<br>';
                        body +='<br>';
                        body +='SFDC Support';
                        mail.setHtmlBody(body);

                        isUpdate=true;
                    }
                }
                messages.add(mail);
                //In the below debugs I can see all 3 users inserted properly in the email template body
                system.debug('messages:'+messages);
                system.debug('Mail:'+mail);
            }
            if(isUpdate)
                system.debug('message1:'+messages);//in this debug when I receive an email,it always displays user c's information.which is last user.
            Messaging.sendEmail(messages,false);

            if(isUpdate)
            {
                update finalOptList;
            }
        }
        catch(Exception e)
        {
            system.debug('An Exception Occured: '+e.getMessage());
        }
    }


    @InvocableMethod(label='Change Opportunity Owner' description='It Change the owner of opportunity when owner is inactive')
    public static void changeOwnerFuture(List<Id> usrIds)
    {
        changeOwner(usrIds);
    }
}

Updating the code below:

        public without sharing class ChangeOpportunityOwner {       
        @Future
        public static void changeOwner(List<Id> usrIds){
        Boolean isUpdate=false;
        List<Messaging.SingleEmailMessage> messages=new list<Messaging.SingleEmailMessage>();
        Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
        Map<Id, Boolean> mapUserToActive = new Map<Id, Boolean>();
        map<Id,user>idToUser= new map<Id,User>([Select Id,Name,ManagerId,IsActive from user where Id IN :usrIds]);
        List<Opportunity> finalOptList=new List<Opportunity>();
        set<Id>mgrIds = new set<Id>();
           for (User u1 : idToUser.values()) { 
                    mgrIds.add(u1.ManagerId);
                    // collect the ids of the managers
              }
        /*for (User u1 : [SELECT Id, isActive FROM User]) { 
        mapUserToActive.put(u1.Id, u1.isActive);
        }*/

        map<Id,user>idToManager = new map<Id,User>([Select Id,Name,ManagerId,IsActive from user where Id IN :mgrIds]);

              for (User u2 : idToManager.values()) { 
                    mapUserToActive.put(u2.Id, u2.isActive);
                    // you can add them to your map if you need the map
              }
        //List <user>usrs= [Select Id,Name,ManagerId,IsActive from user where Id=:usrIds];

        for(User u: idToUser.values()){   
        List<Opportunity>optys=[Select Id,OwnerId,StageName from Opportunity where OwnerId=:u.Id]; 
        for(Opportunity opt:optys){
        if(opt.StageName!='Closed Won' && opt.StageName!='Closed Lost')
        {
        if (mapUserToActive.get(u.ManagerId)== true && mapUserToActive.get(u.ManagerId)!=null) {
        opt.OwnerId=u.ManagerId;
        }
        else{ 
        User usr= [Select Id,Name,ManagerId from user where Name='Test Manager'];
        opt.OwnerId=usr.Id;
        }
        finalOptList.add(opt);
        list<string> userStore=new List<String>();
        userStore.add(u.Name);
        system.debug('userStore:'+userStore);
        // Set list of people who should get the email
        List<String> sendTo = new List<String>();
        sendTo.add(u.ManagerId);
        mail.setToAddresses(sendTo);
        //  Set who the email is sent from
        mail.setReplyTo('[email protected]');
        mail.setSenderDisplayName('Customer Service');
        // Set email contents - you can use variables!
        mail.setSubject('Open Opportunities transferred to you');
        String body = 'Dear Manager,<br> ';
        body +='<br>';
        body += 'Please note that '+userStore+'  has been deactivated as a Salesforce User hence all of its <br> Open Opportunities has been assigned to you if any.<br>';
        body +='<br>';
        body +='Kind regards,<br>';
        body +='<br>';
        body +='SFDC Support';
        mail.setHtmlBody(body);

        //isUpdate=true;
        }
        }
        messages.add(mail);
        system.debug('messages:'+messages);
        system.debug('Mail:'+mail);
        }
        //if(isUpdate)
        //messages1 = new List<Messaging.SingleEmailMessage>(messages);
        system.debug('message1:'+messages);
        Messaging.sendEmail(messages,true);

        //if(isUpdate)
        //{
        update finalOptList;
        //}


        }


        @InvocableMethod(label='Change Opportunity Owner' description='It Change the owner of opportunity when owner is inactive')
        public static void changeOwnerFuture(List<Id> usrIds)
        {
        changeOwner(usrIds);
        }
        } 

It still displays the Latest users info i.e.User C. Please suggest what is wrong in the updated code..

Answer 1

First, it doesn't appear to me that you need your try-catch block at all. Instead, simply query for the users you need (not everyone in your org).

      map<Id,user>idToUser= new map([Select Id,Name,ManagerId,IsActive from user where Id IN :usrIds]);
      set<Id>mgrIds = new set();
      for (User u1 : idToUser.values()) { 
            mapUserToActive.put(u1.Id, u1.isActive);
            // you can create this map if you need it, not certain that it's necessary since you have this above in the map you queried
            mgrIds.add(u1.ManagerId);
            // collect the ids of the managers
      }

      // now query for the managers
      map<Id,user>idToManager = new map([Select Id,Name,ManagerId,IsActive from user where Id IN :mgrIds]);

      for (User u1 : idToManager.values) { 
            mapUserToActive.put(u1.Id, u1.isActive);
            // you can add them to your map if you need the map
      }

      list<user>allUsers = new list();
      // get a combined list of users and managers
      allUsers.addAll(usrs);
      allUsers.addAll(managers);          

      // query for all of the opportunities at one time
      list<Opportunity> finalOptList=new List<Opportunity>();
      map<Id,Opportunity>idToOpportunity=new map([Select Id,OwnerId,StageName from Opportunity where OwnerId in :userIds]);
      // you can optionally group this query by ownerId

      // create a map of opportunity Ids to OwnerId
      for(Opportunity opt:optys){
          if(opt.StageName!='Closed Won' && opt.StageName!='Closed Lost')
          {
              oppIdToOwnerId.put(opt.Id,opt.OwnerId);
          }

Now you should have everything you need to operate in a single for loop on user (or userID) to create your emails. You may want to create a map though of <Id,list<string>> to hold your ManagerId compared to the userIds (or names) that report to the manager. that's the only thing else I can think of that might be helpful to you. You may also want to do something similar for userId to list<OpporutnityId> or similar. I'll leave it to you to work out those details as that should be simple enough with what you have here.

EDIT:

Based on your revised code, here's what I'd recommend you do to solve your issues:

    public without sharing class ChangeOpportunityOwner {       
    @Future
    public static void changeOwner(List<Id> usrIds){
    Boolean isUpdate=false;
    List<Messaging.SingleEmailMessage> messages=new list<Messaging.SingleEmailMessage>();
    Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
    Map<Id, Boolean> mapUserToActive = new Map<Id, Boolean>();

    map<Id,user>idToUser= new map<Id,User>([Select Id,Name,ManagerId,IsActive from user where Id IN :usrIds]);

    List<Opportunity> finalOptList=new List<Opportunity>();
    set<Id>mgrIds = new set<Id>();

    for (User u1 : idToUser.values()) { 
                mgrIds.add(u1.ManagerId);
                // collect the ids of the managers
    }

    map<Id,user>idToManager = new map<Id,User>([Select Id,Name,ManagerId,IsActive from user where Id IN :mgrIds]);

    for (User u2 : idToManager.values()) { 
        mapUserToActive.put(u2.Id, u2.isActive);
    }

    map<Id,Opportunity>idToOpportunity=new map([Select Id,OwnerId,StageName from Opportunity where OwnerId in :userIds groupBy OwnerId]);


    // create a map of opportunity Ids to OwnerId
    map<Id,Id>oppIdToOwnerId = new map<Id,Id>();

    for(Id opId:idToOpportunity.keyset()){
        if(idToOpportunity.get(opId).StageName!='Closed Won' && idToOpportunity.get(opId).StageName!='Closed Lost')
        {
            oppIdToOwnerId.put(opId,idToOpportunity.get(opId).OwnerId);
            // create map of OppIds to OwnerIds for Opps of Interest
        } 
    }

    //get list of opp Ids for each ownerId & put into map
    string OppIdStr = '';
    list<Id>UserId = new list<Id>();
    integer i = 0;
    map<Id,string>usrIdToOppIdString = new map<Id,string>();
    for(Id oId : oppIdToOwnerId.keyset()){            
        if(i=0){
            UserId[0] = oppIdToOwnerId.get(oId);
            OppIdStr = oId + ', ';
            usrIdToOppIdString.put(UserId[0],OppIdStr);
            if(mapUserToActive.get(UserId[0]
        }                 
        else if(i>0 && (oppIdToOwnerId.get(oId) == UserId[0]){
            UserId[i] = oppIdToOwnerId.get(oId).OwnerId;
            OppIdStr +=  ', '+ oId;
            usrIdToOppIdString.put(UserId[i],OppIdStr);
        }
        else{
            UserId.clear();
            i=0;
            UserId[0]=oppIdToOwnerId.get(oId);
            OppIdStr = oId + ', ';
            usrIdToOppIdString.put(UserId[0],OppIdStr);
       }
       i++;

    }


    //Run for loop on oppIdToOwnerId map         
    String UserNames = '';
    for(Id oId:oppIdToOwnerId.keyset()){   
        id managerId = idToUser.get(idToOpportunity.get(oId).OwnerId).ManagerId
        if (mapUserToActive.get(ManagerId)== true ) {
        // the above cannot be null. Would get null pointer exception 
        // when attempting to put into map if value from query was null
            idToOpportunity.get(oId).OwnerId=ManagerId;
            if(!managerIdToUserName.contains(ManagerId)){
                userNames += idToUser.get(idToOpportunity.get(oId).OwnerId).Name + ', ';
                managerIdToUserName.put(ManagerId,userNames);
            }
            else if(managerIdToUserName.contains(ManagerId) &&  !managerIdToUserName.get(ManagerId)userNames.contains(idToUser.get(idToOpportunity.get(oId).OwnerId).Name){
                userNames = managerIdToUserName.get(managerId);
                userNames += idToUser.get(idToOpportunity.get(oId).OwnerId).Name + ', ';
                managerIdToUserName.put(ManagerId,userNames);

            }

        }
        else{ 
            User usr= [Select Id,Name,ManagerId from user where Name='Test Manager'];
            idToOpportunity.get(oId).OwnerId=usr.Id;
            // are you certain this shouldn't be usr.ManagerId ?
            // this scenario seems unlikely, but you know your org best

            if(managerIdToUserName.contains(usr.Id) && !managerIdToUserName.get(usr.Id).contains(idToUser.get(idToOpportunity.get(oId).OwnerId).Name){
                userNames = managerIdToUserName.get(usr.Id);
                userNames += idToUser.get(idToOpportunity .get(oId).OwnerId).Name + ', ';
                managerIdToUserName.put(usrId,userNames);


            }
            else if(!managerIdToUserName.contains(usr.Id)){
                userNames = idToUser.get(idToOpportunity.get(oId).OwnerId).Name + ', ';
                managerIdToUserName.put(usrId,userNames);
            }
        }

    finalOptList.add(idToOpportunity.get(oId));
    }

    // Now create the emails using the maps in a for loop either on ManagerId 
    // or on Opportunity Id if you want them combined into one per manager

Will leave the rest to you.

AVOID ITERATING ON MAP VALUES as you were on this line: for(User u: idToUser.values()){.

I do not recommend using nested SOQL queries as you do in your current code. Instead, take them outside as I've shown above in both examples to create your maps.

These are deeply nested maps and you must keep them straight which I know can be difficult to do. There are probably cleaner ways to do some of this, but I don't have time to write all of your code for you. Use this as a basis for finishing writing your code. All you need do now is finish the loop for your email.

Answer 2

I believe you are getting the correct debug and wrong output because the object reference is maintained which results in same name in all emails.

I believe it can be fixed by adding a line mail = new Messaging.SingleEmailMessage(); before mail.setToAddresses(sendTo);

finalOptList.add(opt);
// Set list of people who should get the email
List<String> sendTo = new List<String>();
sendTo.add(u.ManagerId);
/************ADD THIS LINE****************/
mail = new Messaging.SingleEmailMessage();
/*****************************************/

mail.setToAddresses(sendTo);
//  Set who the email is sent from
mail.setReplyTo('[email protected]');
mail.setSenderDisplayName('BD Customer Service');