0

I have written the following Opportunity trigger that is working but I know it's definitely not bulkified.

It's definitely got some duplication and when any user has any more than 40/50 OpportunityLineItems, it starts to hit SOQL Limits.

I'm just looking for a bit of guidance as to how I could make it a little more efficient and trim some of the redundant code off of it.

Essentially, I am trying to check all OpportunityLineItems and see if a 'Sovos Code' already exists for the same Country and Account (although this is a given as there is only 1 x Account per Opportunity). Where a Code doesn't exist, I want to create one. I then want to assign all these codes to OpportunityLineItems, based on the Country on the OpportunityLineItem.

As always any help would be very much appreciated!

    trigger New_New_Sovos on Opportunity (after insert, after update) {

    List<Sovos_Code__c> Sovoslist = new List <Sovos_Code__c>();
    List<OpportunityLineItem> OLIlist = New List<OpportunityLineItem>();

    set<Id> OLIid = new set<id>();

    for (Opportunity opp : Trigger.New) {
        If (Trigger.IsInsert && (opp.StageName == 'Proposal/Price Quote' || opp.StageName == 'Negotiation' || opp.StageName == 'Verbal Agreement' ||
                opp.StageName == 'LOE Requested' || opp.StageName == 'LOE' || opp.StageName == 'Closed Won')
                ||
                (opp.StageName == 'Proposal/Price Quote' || opp.StageName == 'Negotiation' || opp.StageName == 'Verbal Agreement' ||
                        opp.StageName == 'LOE Requested' || opp.StageName == 'LOE' || opp.StageName == 'Closed Won')
                && (opp.StageName != Trigger.oldMap.get(opp.Id).StageName ||
                opp.Service_Roll_Up__c != Trigger.OldMap.get(Opp.Id).Service_Roll_Up__c))

            OLIid.add(opp.id);
    }

    List<OpportunityLineItem> OLI = [
            Select id, country__c, accountId__c, Sovos_code__c, Revenue_Stream__c, OpportunityId
            from OpportunityLineItem
            Where Opportunityid = :OLIid AND Country__c != 'a002000000QeJI8' AND
            (Revenue_Stream__c = 'Declaration' OR Revenue_Stream__c = 'Retrospective Return')
    ];

    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            for (Sovos_Code__c so : [
                    Select id, Sovos_Code__c, Country__c, Account__c
                    from Sovos_Code__c
                    Where Account__c = :ol.AccountId__c AND Country__c = :ol.Country__c
            ]) {
                if (ol.Sovos_Code__c == null &&
                        (ol.Country__c == so.Country__c) && (ol.AccountId__c == so.Account__c)) {
                    ol.Sovos_Code__c = so.Sovos_Code__c;
                    OLIList.add(ol);
                }
            }
        }
        update OLIList;

        Map<String, OpportunityLineItem> uniqueCountry = new map<String, OpportunityLineItem>();
        for (OpportunityLineItem OL1 : OLI) {
            if (!uniqueCountry.containsKey(OL1.Country__c)) {
                uniqueCountry.put(OL1.Country__c, OL1);
            }
        }
        OpportunityLineItem[] uniqueList = uniqueCountry.values();

        for (OpportunityLineItem ol : uniqueList) {
            if (ol.Sovos_Code__c == null) {

                Sovos_Code__c sc = new Sovos_Code__c ();

                sc.Account__c = opp.AccountId;
                sc.Country__c = ol.Country__c;

                SovosList.add(sc);
            }
        }
        insert SovosList;
    }

    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            for (Sovos_Code__c soA : [
                    Select id, Sovos_Code__c, Country__c, Account__c
                    from Sovos_Code__c
                    Where Account__c = :ol.AccountId__c AND Country__c = :ol.Country__c
            ]) {
                if ((ol.Country__c == soa.Country__c) && (ol.AccountId__c == soa.Account__c)
                        && ol.Sovos_Code__c == null) {
                    ol.Sovos_Code__c = soa.Sovos_Code__c;
                    OLIList.add(ol);
                }
            }
        }
    }
    update OLIList;
}

Thanks!

Dan

1 Answer 1

4

You are doing 2 DML's in for loop. Which is unbulkified, You have to postpone the DML by storing in list till the end of for loop:

trigger New_New_Sovos on Opportunity (after insert, after update) {

    List<Sovos_Code__c> Sovoslist = new List <Sovos_Code__c>();
    List<OpportunityLineItem> OLIlist = New List<OpportunityLineItem>();

    set<Id> OLIid = new set<id>();

    for (Opportunity opp : Trigger.New) {
        If (Trigger.IsInsert && (opp.StageName == 'Proposal/Price Quote' || opp.StageName == 'Negotiation' || opp.StageName == 'Verbal Agreement' ||
                opp.StageName == 'LOE Requested' || opp.StageName == 'LOE' || opp.StageName == 'Closed Won')
                ||
                (opp.StageName == 'Proposal/Price Quote' || opp.StageName == 'Negotiation' || opp.StageName == 'Verbal Agreement' ||
                        opp.StageName == 'LOE Requested' || opp.StageName == 'LOE' || opp.StageName == 'Closed Won')
                && (opp.StageName != Trigger.oldMap.get(opp.Id).StageName ||
                opp.Service_Roll_Up__c != Trigger.OldMap.get(Opp.Id).Service_Roll_Up__c))

            OLIid.add(opp.id);
    }

    List<OpportunityLineItem> OLI = [
            Select id, country__c, accountId__c, Sovos_code__c, Revenue_Stream__c, OpportunityId
            from OpportunityLineItem
            Where Opportunityid = :OLIid AND Country__c != 'a002000000QeJI8' AND
            (Revenue_Stream__c = 'Declaration' OR Revenue_Stream__c = 'Retrospective Return')
    ];

    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            for (Sovos_Code__c so : [
                    Select id, Sovos_Code__c, Country__c, Account__c
                    from Sovos_Code__c
                    Where Account__c = :ol.AccountId__c AND Country__c = :ol.Country__c
            ]) {
                if (ol.Sovos_Code__c == null &&
                        (ol.Country__c == so.Country__c) && (ol.AccountId__c == so.Account__c)) {
                    ol.Sovos_Code__c = so.Sovos_Code__c;
                    OLIList.add(ol);
                }
            }
        }


        Map<String, OpportunityLineItem> uniqueCountry = new map<String, OpportunityLineItem>();
        for (OpportunityLineItem OL1 : OLI) {
            if (!uniqueCountry.containsKey(OL1.Country__c)) {
                uniqueCountry.put(OL1.Country__c, OL1);
            }
        }
        OpportunityLineItem[] uniqueList = uniqueCountry.values();

        for (OpportunityLineItem ol : uniqueList) {
            if (ol.Sovos_Code__c == null) {

                Sovos_Code__c sc = new Sovos_Code__c ();

                sc.Account__c = opp.AccountId;
                sc.Country__c = ol.Country__c;

                SovosList.add(sc);
            }
        }

    }



    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            for (Sovos_Code__c soA : [
                    Select id, Sovos_Code__c, Country__c, Account__c
                    from Sovos_Code__c
                    Where Account__c = :ol.AccountId__c AND Country__c = :ol.Country__c
            ]) {
                if ((ol.Country__c == soa.Country__c) && (ol.AccountId__c == soa.Account__c)
                        && ol.Sovos_Code__c == null) {
                    ol.Sovos_Code__c = soa.Sovos_Code__c;
                    OLIList.add(ol);
                }
            }
        }
    }
    update OLIList;
    insert SovosList;
}

Now coming to your SOQL in For Loop:

for (Sovos_Code__c so : [
                    Select id, Sovos_Code__c, Country__c, Account__c
                    from Sovos_Code__c
                    Where Account__c = :ol.AccountId__c AND Country__c = :ol.Country__c
            ]) 

I would recommend creating a formula field, on Sovos_Code__c named as AccountAndCountry which will be concatination of AccountId__c+Country__c. You can also get this field indexed for performance.

Removing SOQL outta for loop , it will be something like

List<String> accountCountryCode = new List<String>();
   Map<String, Sovos_Code__c> sovosCodeMap = new Map<String, Sovos_Code__c>();


    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            accountCountryCode.add(ol.AccountId__c +ol.Country__c);
        }
    }

    //Single Queryy outside for Loop

    for(Sovos_Code__c soA : [Select id, Sovos_Code__c,AccountAndCountry__C, Country__c, Account__c
                    from Sovos_Code__c
                    Where  AccountAndCountry__C IN : accountCountryCode] ){
        sovosCodeMap.put(soA.AccountAndCountry__C , soa);
    }

    for (Opportunity opp : Trigger.New) {
        for (OpportunityLineItem ol : OLI) {
            Sovos_Code__c soA = sovosCodeMap.get(ol.AccountId__c +ol.Country__c);
            if(soA != null){
                ol.Sovos_Code__c = soA.Sovos_Code__c;
                OLIList.add(ol);
            }
        }
    }
1
  • Thank you for getting back to me! Unfortunately, I am still having trouble piecing this together. I can't seem to add the records and and update them in the same trigger while applying the SOQL out of the loop. I have been playing around with this for a while. Any further assistance would be welcome...
    – Dan Bailey
    Feb 13, 2019 at 15:32

You must log in to answer this question.

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