Issue with APEX Trigger.OldMap. OldMap is giving new value of the field(Stage) instead of the Old Value

Question

 trigger MasterOpportunityTrigger on Opportunity(before insert, after insert, 
before update, after update, 
before delete, after delete,after undelete) 
{
if (Trigger.IsAfter) 
{

  else if (Trigger.isUpdate && !triggerHelper.reEntry) 
  {
     triggerHelper.reEntry = true;

    list<Id>createIds = new list<Id>();
    for(Opportunity o:trigger.new)
    {
        //Check Opportunity is Closed('Approved')/changed frm other stage to approved
        system.debug('New stageName ->'+o.StageName +' '+'Old stageName->'+rigger.oldmap.get(o.Id).stageName);
        if(o.StageName == 'Approved' && o.stageName != trigger.oldmap.get(o.Id).stageName)
        {
            createIds.add(o.Id);
        }
    }
 // If records in createIds, send Ids along with trigger.oldmap and trigger.newmap to your class
    if(!createIds.isEmpty())
    {           
        system.debug()
        //Create a Sponsor
        CreateSponsors CSp = new CreateSponsors(createIds,trigger.oldmap,trigger.newmap);
        CSp.CSponsor();
        system.debug('*************CreateSponsors Trigger Execute');     

    }

}
}
  }
         // Class to create Sponsor record
     public class CreateSponsors 
    {     
    Map<Id, Opportunity> oldOpps;

    Map<Id, Opportunity> newOpps;

    List<Opportunity> Oplst = new List<Opportunity>();

    public CreateSponsors(List<Id> Opids,Map<Id, Opportunity> oldTriggerOpps, Map<Id, Opportunity> newTriggerOpps) 
    {
            oldOpps = oldTriggerOpps);
            newOpps = newTriggerOpps
        }

        if(newTriggerOpps.size()>0)
        {


        for(Opportunity Op : [Select id,CampaignId,Sponsored_Event__c,Sponsorship_Program__c,AccountId from Oppportunity WHERE
            ID In : Opids])
        {   
            Oplst.add(Op);
        }

    }


    public void CSponsor()
    {
        Map<ID,Schema.RecordTypeInfo> rt_Map = Opportunity.sObjectType.getDescribe().getRecordTypeInfosById();

        List<Sponsor__c> Sponlist = new List<Sponsor__c>();

            for(Opportunity Op : Oplst)
            {
                //system.debug('New Opportunity Stage->'+Op.StageName +' '+ 'New Opportunity Stage->'+oldOpps.get(Op.Id).StageName);
                //if(Op.StageName == 'Approved' && Op.StageName != oldOpps.get(Op.Id).StageName) 
                {  
                    if(rt_map.get(Op.recordTypeID).getName().containsIgnoreCase('Sponsorship') && Op.Number_of_Sponsors__c == 0) 
                    {
                        system.debug('********entry*******');
                        Sponsor__c Sp = new Sponsor__c();
                        Sp.Sponsor_Name__c = Op.AccountId;
                        Sp.Sponsored_Event__c = Op.CampaignId;
                        Sp.Sponsorship_Opportunity__c = Op.id;
                        Sp.Sponsorship_Program__c = Op.Sponsorship_Name__c;
                        Sponlist.add(Sp);
                    }
                }
            }
            if(Sponlist.size()>0)
            {
                system.debug('********entry1*******');
                insert Sponlist;
            }
        }
    }

Answer 1

Since this is an AfterUpdate trigger, if you have any workflow that fires on Opportunity, one of the issues you'd be having with this trigger would likely be one that's related to recursion. After your workflow fires, your AfterUpdate trigger will fire one more time. The original values in trigger.new and trigger.old will still be the same as they were when the trigger was initiated. In your scenario, that would cause your trigger to fire a second time.

I agree with @KeithC that your trigger needs to be checking the field values before calling your class CreateSponsors. It also needs to be using a boolean variable to prevent recursion so it doesn't try to run the class a 2nd time if the trigger is called in the same execution context. See Triggers and Order of Execution for more on this.

You're wondering how to fix this. I don't know about your rollups, but would assume that they only need to be run once, so here's some suggestions on how to implement this code-wise in your afterUpdate code section:

  else if (Trigger.isUpdate && !triggerHelper.reEntry) 
  {
    // set the static boolean to true to prevent reentry
    triggerHelper.reEntry = true;

    //Roll Up amount to Grant
    GrantRollUp GrR = new GrantRollUp();
    GrR.RollUpMth(Trigger.new);
    system.debug('*************GrantRollUp Trigger Execute');

    // Roll Up amount to SponsorShip
    SponsorRollUp Sp = new SponsorRollUp(); 
    Sp.SponsorUpMth(Trigger.new);
    system.debug('*************SponsorRollUp Trigger Execute');

    list<Id>createIds = new list<Id>();
    //Create a Sponsor
    for(Opportunity o:trigger.new)
    {
        if(o.stage != trigger.oldmap.get(o.Id).stage)
        {
            createIds.add(o.Id);
        }
    }

    // If records in createIds, send Ids along with trigger.oldmap and trigger.newmap to your class
    if(!createIds.isEmpty())
    {           
        CreateSponsors CSp = new CreateSponsors(createIds,trigger.oldmap,trigger.newmap);
        CSp.CSponsor();
        system.debug('*************CreateSponsors Trigger Execute');     
    }

  }

Helper class for your public static boolean:

Public class triggerHelper{

  Public static boolean reEntry = false;

}

Change your CreateSponsors constructor to something like below so it can process the incoming Ids of records:

public CreateSponsors(List<Id>CreateIds, Map<Id, Opportunity> oldTriggerOpps, Map<Id, Opportunity> newTriggerOpps)

Further down in your class, rather than iterating on newOpps.values(), you'll need to change your code. You can either test for the Opps that contain an Id that's in CreateIds before proceeding through your loop. Or, you could just iterate on the Ids in the list and pull the field values from newOpps using newOpps.get(crId).FieldName or similar (crId is the iterator here).

Edit

The syntax I gave you originally won't properly evaluate the boolean. I normally evaluate it independently rather than using an && condition. You have two choices to correct the issue.

One option would be to change the existing line to (note: normally I do this from a dispatcher class & not from a base trigger which makes this easier):

else if (Trigger.isUpdate && triggerHelper.reEntry == false)
{
    // set the static boolean to true to prevent reentry
    boolean entry = new triggerHelper.reEntry; //need a new instance of the class
    triggerHelper.reEntry = true;

The other would be to nest the boolean == false check inside the else if as follows which is how I'd suggest doing it:

else if (Trigger.isUpdate)
{
   if(!triggerHelper.reEntry)
   { 
       // set the static boolean to true to prevent reentry
       boolean entry = new triggerHelper.reEntry; //need a new instance of the class
       triggerHelper.reEntry = true;

       .... rest of existing code

   } // end if

}// end else if

I had meant to add yesterday that it's important not to try to move this static boolean to your helper class. It needs to stay here rather than in your helper class. If located in your helper class, it would affect any other instances of the helper that might get called by another instance of your trigger, preventing them from being able to execute. That's why it needs to be located where it's at here in the trigger that calls it. It's also why you want to have the triggerHelper class to call it from.