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.

1 Answer 1


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, 2019 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, 2019 at 6:05

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .