Data in trigger.new
and trigger.newMap
is read-only when you're in an after
trigger (and old/oldMap are always read only. Not an issue here, but worth mentioning).
Collections (lists, maps, sets) store data by reference1. So when you set opp.StageName
, you're attempting to write data to the object instances stored in trigger.new
. Putting it into a different collection does not break the reference.
Since you're trying to update the same records that are being processed by the trigger, you should consider having your trigger work on before insert
and before update
instead. That'll allow you to adjust those records (which are read-write in before triggers) without the need for DML. That's the big benefit of before triggers.
Records in trigger.new
don't have Ids in before insert
, but that shouldn't be much of an issue.
The other thing worth noting is that you are using the if(data1 == data2)
in a nested loop anti-pattern. Nested loops aren't inherently evil, but if you see if(data1 == data2)
in the nested loop, that's a big red flag. You should instead put data into a Map
.
So your handler code should look like
public static void handleOpportunityUpdates(List<Opportunity> newOpportunities) {
//Map<Id, Opportunity> oppUpdates = new Map<Id, Opportunity>();
Map<Id, Opportunity> oppsById = new Map<Id, Opportunity>();
// You can have an Id set if you want to, but we can just as easily use
// the keyset of the map instead.
//Set<Id> oppIds = new Set<Id>();
// Looping over data to gather Ids first is a good practice
// It helps us avoid having queries inside of loops
// Good on you for getting into that habit early!
for (Opportunity opp : newOpportunities) {
//oppIds.add(opp.Id);
// We can take this opportunity (no pun intended) to set the default state
// If an Opp has no quotes, you want the stage to be a certain value
// Since trigger.new is being passed in for newOpportunities, and because
// collections store references, and because we're in a before trigger,
// this will modify the data in trigger.new and end up being the value
// on your record (assuming there are no quotes)
opp.StageName = 'Value Proposition';
// opp.Id will be null in the "before insert" context, but that's not
// a problem
// .put()-ing the same key into a map multiple times just overwrites
// the data in the map
oppsById.put(opp.Id, opp);
}
// Small change here, using the map's keyset for the Ids
List<Quote> relatedQuotes = [SELECT Id, IsSyncing, Status, OpportunityId FROM Quote WHERE OpportunityId IN :oppsById.keySet()];
// There is no longer a need for this loop over the opps
//for (Opportunity opp : newOpportunities) {
// We've taken care of this default value elsewhere, so this is not needed
// if (!relatedQuotes.isEmpty()) {
// opp.StageName = 'Value Proposition';
// }
// No need to check for empty/null here
// A query will _never_ return null, just perhaps an empty list
// Looping over an empty list does not result in an error,
// the code inside the loop will just not be executed in that case
for (Quote quote : relatedQuotes) {
// We can use the map to get the appropriate Opportunity record
Opportunity opp = oppsById.get(quote.OpportunityId);
// This is the red flag, the if(data1 == data2) inside of a nested
// loop that we want to avoid
//if (quote.OpportunityId == opp.Id) {
if (quote.IsSyncing && quote.Status == 'Presented') {
opp.StageName = 'Proposal/Price Quote';
} else if (quote.Status == 'Accepted') {
opp.StageName = 'Closed/Won';
}
//}
}
// The opp is already in our map, so we don't need to add it again
//oppUpdates.put(opp.Id, opp);
}
// No DML needed, since you should be calling this as part of a before trigger
//if (!oppUpdates.isEmpty()) {
// update oppUpdates.values();
//}
}
An alternate approach to getting the related quotes for an Opportunity would be to use a parent-child subquery (see the relevant documentation)
Also worth mentioning is that you don't want to use SObject's .clone()
or new Opportunity()
here, because those are two ways to get a completely independent instance of your SObjects. To take advantage of the power of before triggers, you need to make changes to the same instance of the SObjects contained in trigger.new
or trigger.newMap
.
You probably want to use ORDER BY
in your query for Quotes so that, as an example, if an Opportunity has multiple Quotes related to it you can guarantee that you set the StageName based on the most recent Quote.
1: It's a bit more complicated than that, but it's close enough to correct for this discussion