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

Improve this question

1 Answer 1

Reset to default
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);
            }
        }
    }
Improve this answer
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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.