Trigger to Prevent Users from Changing Opportunity Stage if Conditions Aren't Met

Question

I'm trying to write a trigger to prevent users from changing an opportunity stage if the following isn't met.

1. Opportunity doesn't have a contact with the role Champion.
2. Opportunity doesn't have an event with the type New Business Meeting.
3. Latest New Business Meeting doesn't have an RSM. I'm having trouble bulkifying the trigger. Here's my code.

Trigger

trigger OpportunityTrigger on Opportunity (before update) {
    if (Trigger.isBefore) {
        if (Trigger.isUpdate) {
            for (Opportunity opp : Trigger.New)
            {
                List<ID> OppID = New List<ID>();
                Opportunity oldOpp = Trigger.oldMap.get(opp.ID);
                if(opp.StageName == 'Discovery' && oldOpp.StageName == 'Scope' && opp.Link_to_Value_Pyramid__c != null){
                    OppID.add(opp.Id);
                    OpportunityHandler.stage2To3(Trigger.new);
                }
                if(opp.StageName == 'Discovery' && oldOpp.StageName == 'Scope' && opp.Link_to_Value_Pyramid__c == null){                                             
                    opp.addError('Unable to save. Please fill in the Link to Value Pyramid field');
                }
            }
        }
   }
}

Trigger Handler

public class OpportunityHandler {
    public static void stage2To3(List<Opportunity> listOfNewOpportunities){
       Set<Id> oppsWithChampions = new Map<Id, SObject>(
            [SELECT Opportunity.Id
             FROM OpportunityContactRole 
             WHERE Role='Champion' AND Opportunity.Id IN :listOfNewOpportunities]).keySet();
        
      List<Event> oppsWithEvents = 
        [SELECT WhatId 
         FROM Event 
         WHERE Type = 'New Business Meeting'
         AND WhatId IN: oppsWithChampions];

      Map<Id, AggregateResult> maxEvents =  new Map<Id, AggregateResult> 
       ([SELECT WhatId Id, Max(ActivityDateTime) ActivityDate
         FROM Event
         WHERE Type = 'New Business Meeting'
         AND WhatId IN: oppsWithChampions
         GROUP BY WhatId]);

        Set<Id> whatIds = new Set<Id>();
        for (AggregateResult ar : [SELECT WhatId, Max(ActivityDateTime) ActivityDate
                           FROM Event
                           WHERE Type = 'New Business Meeting'
                           AND WhatId IN: oppsWithChampions
                           //AND ActivityDate = LAST_N_DAYS:35
                           GROUP BY WhatId]){
           whatIds.add((ID)ar.get('WhatId'));
        }

        List<Opportunity> oppNotUpdated  = [SELECT Id FROM Opportunity 
                                           WHERE Id IN : listOfNewOpportunities
                                           AND Id NOT IN : whatIds]; 
       for(Opportunity o:listOfNewOpportunities){        
         for(Opportunity o:oppNotUpdated){
            o.addError('Please fill out the required information before changing the opportunity stage.');
        }
     }
  }
}

I have tried the following code but I get 'Invalid bind expression type of OpportunityContactRole does not match domain of foreign key'. How do I filter the listOfNewOpportunities to only include the ones that do not meet the criteria.

Answer 1

When you use List<SObject> as a filter value in SOQL, it pulls the Id field. Since you are passing List<OpportunityContactRole>, this filter will not look for What.Type = 'Opportunity', which is ostensibly what you are looking for. The basic problem is you need a Set<Id> opportunityIds or List<Opportunity>. I would opt for the former with:

Set<Id> opportunityIds = new Map<Id, SObject>([
    SELECT OpportunityId Id FROM OpportunityContactRole
    WHERE ...
]).keySet();

This approach can be a bit confusing for the uninitiated, but it doesn't do anything to complicated under the covers. The thing you need to understand to use it is that Map<Id, SObject> constructor can accept AggregateResult, and you can alias fields in aggregate queries to Id, which will then be used for the map key. This pattern uses governors very efficiently. Specifically it will be hard to beat on query rows and CPU time.

Once you have the Set<Id>, solving your current stumbling block should be a simple exercise.

---

On a minor tangent, I highly recommend moving all of your logic out of your trigger body and into classes instead. Basic trigger structure should look like:

public trigger MyObject on MyObject(/*events*/)
{
    MyObjectTriggerHandler handle = new MyObjectTriggerHandler(trigger.new, trigger.oldMap);
    switch on trigger.operationType
    {
        when BEFORE_INSERT { handle.beforeInsert(); }
        // AFTER_INSERT, BEFORE_UPDATE, AFTER_UPDATE, etc.
    }
}

And ideally your handler should do nothing but delegate to other service/filter layers.

public class MyHandler
{
    final List<MyObject__c> newRecords;
    final Map<Id, MyObject__c> oldMap;
    public MyHandler(List<MyObject> newRecords, Map<Id, SObject> oldMap)
    {
        this.newRecords = newRecords;
        this.oldMap = oldMap;
    }
    
    public void beforeInsert()
    {
        MyValidationService.addError(
            MyFilterService.isInvalidBecauseXYZ(newRecords);
        );
    }
    
    public void beforeUpdate()
    {
        MyValidationService.addError(
            MyFilterService.isInvalidBecauseXYZ(newRecords, oldMap);
        );
    }
}