I am very new to writing SOQL or APEX triggers

I am currently writing an APEX trigger, that would update OpportunityLineItems whenever a Product2 is updated. Basically, it currently looks like this.

trigger COGS_Update on Product2 (after update) {
  System.Debug('>>>> In COGS trigger');
  for (Product2 p : Trigger.new) {
    OpportunityLineItem[] myLineItem = [SELECT Id, Name, UnitPrice, Quantity, Gross_Profit__c FROM OpportunityLineItem WHERE ProductCode = :p.ProductCode];
    for (OpportunityLineItem oli : myLineItem) {
        oli.Gross_Profit__c = (oli.UnitPrice - p.cost__c) * oli.Quantity;
    update myLineItem;

Basically, whenever a product is updated right now, it updates the OpportunityLineItems that use the associated product (updating specifically the custom Gross_Profit__c field).

This all works. However, the next part is I would like to specifically update the OpportunityLineItem only if the associated Opportunity is open itself.

I'm not sure what's the best way to get all the data I need in one query because now I also need to query the IsClosed field from an Opportunity. I tried just querying all opportunities that aren't closed and getting OpportunityLineItems inside. My one quirk is that this seems like it's querying a lot of unnecessary opportunities (ones that will have no OpportunityLineItems to update).

Opportunity[] Ops = [SELECT Id, 
                     (SELECT Name, UnitPrice, Quantity, Gross_Profit__c FROM OpportunityLineItems WHERE ProductCode = :p.ProductCode) 
                     FROM Opportunity WHERE IsClosed = false];

So I am curious what others think is a good approach to this specific issue.


When you have a reverse relationship, such that you can use a nested select, that means you have a direct relationship between that child and the parent.

In this case you have OpportunityLineItem and Opportunity related via a field on OpportunityLineItem itself - the field is called OpportunityId and the relationship field is called Opportunity.

You can, therefore, query the line items in this way to only match those with an open opportunity:

SELECT Id, Name, UnitPrice, Quantity, Gross_Profit__c FROM OpportunityLineItem WHERE ProductCode = :p.ProductCode AND Opportunity.IsClosed = FALSE

Note, however, that your trigger has an anti-pattern in it, where you are querying line items in a loop. You can't do this since a trigger can be called with up to 200 items and it may be called multiple times in a single session/request. You are limited to far fewer SOQL queries per session/request.

You need to change the code to do something like:

trigger COGS_Update on Product2 (after update) {
  System.Debug('>>>> In COGS trigger');

  // Get a collection of the unique product codes across the products
  // being updated
  Set<String> codes = new Set<String>();

  for (Product2 product : Trigger.new) {

  OpportunityLineItem[] lineItems = [SELECT Id, Name, UnitPrice, Quantity, Gross_Profit__c, ProductCode FROM OpportunityLineItem WHERE ProductCode IN :codes AND Opportunity.IsClosed = FALSE];
  Map<String, List<OpportunityLineItem>> lineItemsByCode = new Map<String, List<OpportunityLineItem>>();

  // Get all the queried line items into a map here the key is the product code
  // A given line item has one product code, but a given product code could have
  // multiple line items
  for (OpportunityLineItem lineItem : lineItems) {
    List<OpportunityLineItem> lineItemsForCode = lineItemsByCode.get(lineItem.ProductCode);

    if (lineItemsForCode == null) {
      // This is the first line item with this code
      lineItemsByCode.put(lineItem.ProductCode, new List<OpportunityLineItem> { lineItem };
    else {
      // This is a second or subsequent line item with the code

  // Collect a single list of updates. Note that this will go wrong if you are
  // updating more than one product with the same code since we don't make sure
  // to only update a given line item once. You can handle this by tracking the
  // item IDs and making sure we only update a given item once. That said, you
  // need to decide whether you will take the first or last product's details
  // into account in this calculated update
  List<OpportunityLineItem> updates = new List<OpportunityLineItem>();

  for (Product2 p : Trigger.new) {
    OpportunityLineItem[] myLineItem = lineItemsByCode.get(p.ProductCode);

    for (OpportunityLineItem oli : myLineItem) {
        oli.Gross_Profit__c = (oli.UnitPrice - p.cost__c) * oli.Quantity;


  update updates;

(Code not compiled, you may find typos - the basic structure is sound though, I believe)

  • While this is all very good to know, I'm currently expecting to only update one product at a time. This might be a dumb question but is still important to leave these provisions that assume multiple products are being updated at once? – Cameron Aug 19 '19 at 23:49
  • In a word, yes. Unless it is only ever you that uses the org and you never perform any bulk operations against products you cannot be sure what will happen. – Phil W Aug 20 '19 at 6:05

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.