So I'm new to writing Apex Classes and Triggers - and I know I still have to "bulkify" my code but I can't understand why the logic isn't working here. Basically I'm trying to make an trigger on opportunity that loops through the contacts associated, and sets a contact custom field "In_Pipe__c" to "true" if the contact has any opportunities "New (Renewable)" and not closed. Not sure what I'm missing here but any help would be appreciated! Thanks!!

trigger Is_In_Pipe_v3 on Opportunity (after update, after insert) {
for (Opportunity currentOpp : Trigger.new) {
    List<OpportunityContactRole> oppContact = [SELECT ContactID FROM OpportunityContactRole WHERE OpportunityID = :currentOpp.Id];
        for (OpportunityContactRole currentContact : oppContact) {

            List<Opportunity> oppList =  [ SELECT Id, Name
                                From Opportunity
                                Where Id IN (SELECT OpportunityId
                                    FROM OpportunityContactRole
                                    WHERE ContactId = :currentContact.Id)
                                AND Type = 'New (Renewable)'
                                AND StageName != 'Closed Won'
                                AND StageName != 'Closed Lost' ];
             Contact thisContact = [SELECT Id From Contact Where Id IN (SELECT ContactId FROM OpportunityContactRole)];

    if (oppList.size() > 0) {
        thisContact.InPipe__c = true;
    else {
        thisContact.InPipe__c = false;

} }


The challenge with triggers across multiple objects is that there are many cases to consider such as:

  • the Opportunity changing to match the condition
  • the Opportunity changing to not match the condition
  • the Opportunity being deleted
  • the OpportunityContactRole being inserted or updated
  • the OpportunityContactRole being deleted

A trigger of the following form will (I think - tests need adding to be more sure) handle the first two. (If all 5 conditions are reasonable you will need to write further triggers for those.)

trigger Is_In_Pipe_v3 on Opportunity (after update, after insert) {
    // Find the Opportunites where the key fields have changed
    Set<Id> changedOppIds = new Set<Id>();
    for (Opportunity newOpp : Trigger.new) {
        Opportunity oldOpp = Trigger.oldMap.get(newOpp.Id);
        Boolean isNew = newOpp.Type == 'New (Renewable)'
                && newOpp.StageName != 'Closed Won'
                && newOpp.StageName != 'Closed Won';
        Boolean isOld = oldOpp.Type == 'New (Renewable)'
                && oldOpp.StageName != 'Closed Won'
                && oldOpp.StageName != 'Closed Won';
        if (isOld != isNew) {
    if (changedOppIds.size() > 0) {
        List<Contact> contacts = new List<Contact>();
        for (AggregateResult ar : [
                select ContactId i, count(OpportunityId) c
                from OpportunityContactRole
                where OpportunityId in :changedOppIds
                and Opportunity.Type = 'New (Renewable)'
                and Opportunity.StageName not in ('Closed Won', 'Closed Lost')
                group by ContactId
                ]) {
           Id contactId = (Id) ar.get('i');
           Integer oppCount = (Integer) ar.get('c');
           contacts.add(new Contact(Id = contactId, InPipe__c = oppCount > 0));
        update contacts;

The "bulkification" is that the work is minimised by only looking at specific changes to Opportunity, there are no queries inside loops, and the minimum amount of looping is done in Apex (by making use of the databases aggregation capability). Performance will be pretty linear depending on how many Contacts are affected by the change, rather than rising exponentially as more Contacts are affected because of nested loops.

The above code is making use of Relationship Queries. These allow you to query across multiple objects in one go, reducing the need to connect together separate queries in code. If you are going to be writing many triggers, it is worth understanding these well.

The other important technique to get used to when writing triggers is the use of maps and sets; see e.g. Common Bulk Trigger Idioms.

|improve this answer|||||

You aren't calling update on thisContact :) You should add each contact you are updating to a list of contacts and then call update listOfContacts. Be sure to handle any exceptions with a try/catch block.

|improve this answer|||||
  • Ah that was silly of me! :) And good point on the try/catch block...but even when I added update thisContact; it seems to only work the first time an Opportunity changes from prospect to closed. If I change it back to prospect the In_Pipe__c checkbox won't "re-check." – amizzo Apr 24 '14 at 17:59
  • Is the contact you are testing this with associated with any other opportunities that are not closed? Your oppList would then contain opportunities. – Kyle Apr 24 '14 at 18:15
  • Hmm something funky's going on. It was working when I first added the update thisContact; line to the code but now nothing is changing. If I manually run the SOQL query for a certain contact ID in the developer console it returns 0, which means In_Pipe__c should be false. But the flag stays checked no matter how many times I try to update the Opportunity. It seems like it's just retaining the previous value of the loop or something – amizzo Apr 24 '14 at 19:11
  • Well one thing you want to avoid (and that you are doing a lot) is using SOQL queries inside of for loops. Best practice is to do your queries outside of the loops and then loop over the returned records. You can still filter your queries by the ids in your trigger map or list by doing Opportunity.id IN :Trigger.new. That is definitely worth changing, and maybe it will be easier to debug then too. – Kyle Apr 24 '14 at 19:15
  • I'd also also add to Keith C's answer that if you are going to be adding any other triggers to Opportunity then it would be wise to put your logic into an Opportunity Trigger Handler class. This will give you more control over the order that your trigger code is executed, as well as simplifying the trigger itself. For example – Kyle Apr 25 '14 at 16:38

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.