0

I have an issue that I am hoping others may have come across and/or can quickly and easily point out what I have done incorrectly in the below sequence of code. I am receiving the error Loop must iterate over collection: Boolean on Line 21 for (Case c: Trigger.isUpdate).

In case it is not clear, I just need to grab the AccountId from the Case when updated and input these into custom objects.

I am new to coding and any help or guidance would be greatly appreciated.

trigger CaseAfterUpdate on Case (after update) {

    //Variables
    Set<Id> accountId = new Set<Id>();
    Map<Id,Case> allAccIds = new Map<Id, Case>();
    List<MyIns__c> updateIns = new List<MyIns__c>();
    List<MySup__c> updateSup = new List<MySup__c>();
    List<MyIns__c> newIns = new List<MyIns__c>();
    List<MySup__c> newSup = new List<MySup__c>();
    List<Case> caseList = new List<Case>();

    Schema.DescribeSObjectResult caseSchema = Schema.SObjectType.Case;
    Map<String,Schema.RecordTypeInfo> caseRecordTypeInfo = caseSchema.getRecordTypeInfosByName();
    Id rtCaseDA = caseRecordTypeInfo.get('My_Record_Type').getRecordTypeId();
    system.debug('rtCasefromschema'+rtCaseDA);

    //Id rtCaseDA = [SELECT Id FROM Recordtype WHERE DeveloperName = 'My_Record_Type' AND SObjectType='Case' LIMIT 1].Id ;


    //Update Trigger
    for (Case c: Trigger.isUpdate){

        if (c.RecordTypeId == rtCaseDA && c.Status == 'Approved') {
            //Sup Approved

            accountId.add(c.AccountId);

        }

        allAccIds.put(c.AccountId, c);

    }

    //Ins
    if(allAccIds.size()>0) {

        List <MyIns__c> newIns = [SELECT Id, Id__c, Approved__c FROM MyIns__c WHERE Id__c IN :allAccIds.keyset()];
        for(MyIns__c ins : newIns ) {
            //Update the ins to approved
            for(Case cs : allAccIds.values() ) {
                if(cs.AccountId == ins.Id__c && cs.N_Ins__c == TRUE) {
                    ins.Approved__c = True;
                    updateIns.add(ins);
                }

            }

        }
        if(updateIns.size()>0){
            update updateIns;
        }
    }


    //Sup
    if(allAccIds.size()>0) {

        List <MySup__c> newSup = [SELECT Id, Id__c, Approved__c FROM MySup__c WHERE Id__c IN :allAccIds.keyset()];
        for(MySup__c sup : newSup ) {
            //Update the sup to approved
            for(Case cs : allAccIds.values() ) {
                if(cs.AccountId == sup.Id__c && cs.N_Sup__c == TRUE) {
                    sup.Approved__c = True;
                    updateSup.add(sup);
                }

            }

        }
        if(updateSup.size()>0){
            update updateSup;
        }

    }

}
2

Others have already pointed out the main issue with your code (using Trigger.isUpdate instead of Trigger.new). The documentation on Trigger Context Variables is a good reference for the future, as it not only covers what each trigger context variable is, but also tells you when you can use them (i.e. Trigger.newMap is only available after insert, both before update and after update, and after undelete).

After improving the formatting of your provided code a bit, there are other concerns that should also be addressed.

Let's work with the following snippet of your code:

if(allAccIds.size()>0) {

    List <MyIns__c> newIns = [SELECT Id, Id__c, Approved__c FROM MyIns__c WHERE Id__c IN :allAccIds.keyset()];
    for(MyIns__c ins : newIns ) {
        //Update the ins to approved
        for(Case cs : allAccIds.values() ) {
            if(cs.AccountId == ins.Id__c && cs.N_Ins__c == TRUE) {
                ins.Approved__c = True;
                updateIns.add(ins);
            }

        }

    }
    if(updateIns.size()>0){
        update updateIns;
    }
}

It's good that you keep your query (and DML) outside of all loops (that's a mistake a lot of people new to Apex make, good on you for not falling for that!).

The nested for loop and the if(recordA.field == recordB.field) is a red flag. It's not the end of the world, but there is a better way to do this. Use a map. You're actually already most of the way there. Your allAccIds variable is a Map<Id, Case> where the key of the map is an AccountId.

Instead of the second (nested) for loop over the values of allAccIds, you can simply pull the exact Case record you're looking for using allAccIds.get(ins.Id__c). Using that, your snippet would change to be

if(allAccIds.size()>0) {

    // The Id__c variable is a bit concerning to me.
    // I assume it's a lookup field, in which case a more descriptive field name
    //   that tells you _what_ object it's a lookup to would be more helpful (if
    //   not for you, then for your boss/coworkers/successors).
    // If it's not a lookup field, and just a text field to act as a pseudo-polymorphic
    //   field, having a different name (like Rel_Id__c) would still be helpful.
    // If you were to build a fieldset, or try to include this field on a page layout,
    //   'ID__c' would likely appear as 'Id' or 'ID', very similar to the standard
    //   Id field (making choosing the correct field essentially a coin flip).
    List <MyIns__c> newIns = [SELECT Id, Id__c, Approved__c FROM MyIns__c WHERE Id__c IN :allAccIds.keyset()];
    for(MyIns__c ins : newIns ) {
        //Update the ins to approved

        // Fetching the case from the map is a much better idea than iterating
        //   over your map and trying to manually match cs.AccountId to ins.ID__c.
        // A Map<Id, Case> will only ever associate a particular Id to one Case.
        // If you need to handle instances where an Account can have multiple
        //   Cases related to it, then you'd need a Map<Id, List<Case>>
        Case cs = allAccIds.get(ins.Id__c);

        //if(cs.AccountId == ins.Id__c && cs.N_Ins__c == TRUE) {
        // Fetching a specific value from the map using ins.Id__c means we don't
        //   need to check if cs.AccountId == ins.Id__c anymore.
        // if(var == true) is a bit redundant, and be replaced by if(var)
        if(cs.N_Ins__c){
            ins.Approved__c = True;
            updateIns.add(ins);
        }

    }

    // This check is unnecessary.
    // Salesforce might do some questionable things at times (and they do), but
    //   Apex does have the feature that a DML call on an empty list won't
    //   actually cause triggers to run (it might not even count against your
    //   100 DML call governor limit, though I'm not 100% certain of that)
    //if(updateIns.size()>0){
    update updateIns;
    //}
}

One of the things I mentioned deserves more explanation. A Map<Id, SObject> will only ever contain one SObject (Case, in your situation). If you have the following code

// Assuming case1 and case2 are previously declared, and are related to the same Account
Map<Id, Case> myMap = new Map<Id,Case>();
map.put(case1.AccountId, case1);
map.put(case2.AccountId, case2);

// This assertion will fail, because when you perform the second put(),
//   the previous value of the map (case1) was overwritten with a new value (case 2)
System.assertEquals(myMap.get(case1.AccountId), case1);

Having more than one Case related to an Account is pretty common (at least it is in my org). If you want to handle that situation, then you need to use a Map<Id, List<Case>>, and your code will change again.

// I err on the side of more descriptive variable names.
// This one is a bit on the long side
Map<Id, List<Case>> accountIdsToRelatedCasesMap = new Map<Id, List<Case>>();
for(Case c :Trigger.new){
    // Initializing a collection (map) of collections (lists) requires a bit more consideration
    //   than a collection (map) of single items.
    // There are other ways to do it, but this is my preferred method
    if(!accountIdsToRelatedCasesMap.containsKey(c.AccountId)){
        // In this if statement, we know this is the first time we've encountered
        //   this particular Id.
        // Initialize a new list, and put it in the map for later use.
        accountIdsToRelatedCasesMap.put(c.AccountId, new List<Case>());
    }

    // Outside of the if statement, at this point, we are guaranteed to have something in the map
    //   for the current Case's AccountId, so the following line is safe.
    // <Map var name>.get(<id>) returns a list
    // We can add new items to that list without storing the list in a temporary variable,
    //   and the list stored in the Map is automatically updated.
    accountIdsToRelatedCasesMap.get(c.AccountId).add(c);
}

if(accountIdsToRelatedCasesMap.size()>0) {

    List <MyIns__c> newIns = [SELECT Id, Id__c, Approved__c FROM MyIns__c WHERE Id__c IN :accountIdsToRelatedCasesMap.keyset()];
    for(MyIns__c ins : newIns ) {
        //Update the ins to approved

        // This variable may or may not be required.
        // It'll make more sense in a little bit.
        Boolean allTrue = true;

        // The nested for loop makes a return here, because we do have multiple
        //   cases (potentially, the important thing is that we're working with a List<Case>)
        //   that we need to deal with
        for(Case cs : allAccIds.get(ins.Id__c) ) {
            //if(cs.AccountId == ins.Id__c && cs.N_Ins__c == TRUE) {
            // By building our Map<Id, List<Case>>, we ensured that every Case
            //   in the list for a particular Account Id is related to that Account.
            // That means that the check for cs.AccountId == ins.Id__c is still not required
            if(cs.N_Ins__c){
                ins.Approved__c = True;
                updateIns.add(ins);

                // In this situation, it may be possible that more than one Case
                //   will have N_Ins__c ==  true.
                // Adding the same record to a list more than once will cause
                //   issues when you perform DML on that list.
                // To avoid that, you need to decide what should happen when
                //   you have multiple cases that might not agree with each other.
                // Specifically, does a single true value of N_Ins__c mean you
                //   want to approve the MyIns__c record, or do all of your cases
                //   need to have N_Ins__c == true?

                // In the case where a single true is enough, we want to 'break'
                //   the inner loop so we guarantee that we don't add the current
                //   MyIns__c record to the update list a second time.
                break;
            }

            // In the case where all related Cases need to be true, a single false
            //   is enough to tell us that we can stop work for this MyIns__c record
            // This code is commented out because the two different situations
            // (one cs.N_Ins__c is true vs all are true) are  mutually exclusive
            // You need to choose which one of these strategies you want to use,
            //   and only use the code for that strategy.
            //if(!cs.N_Ins__c){
            //    allTrue = false;
            //    break;
            //}

        }

        // The final part of the "all cases must be true" path.
        // Since we need to check all of the cases to determine whether or not
        //   to execute this code, it needs to be outside of the loop over cases.
        //if(allTrue){
        //    ins.Approved__c = true;
        //    updateIns.add(ins);
        //}

    }

    // This check is unnecessary.
    // Salesforce might do some questionable things at times (and they do), but
    //   Apex does have the feature that a DML call on an empty list won't
    //   actually cause triggers to run (it might not even count against your
    //   100 DML call governor limit, though I'm not 100% certain of that)
    //if(updateIns.size()>0){
    update updateIns;
    //}
}
  • 1
    Derek - since you've gone all in here with an excellent tutorial, you might as well point out that the last if statement: if(updateIns.size()>0) is unnecessary and OP can just use update updateIns; - if list is empty SFDC does nothing. – cropredy Oct 24 '17 at 18:46
  • @cropredy right, missed that one. Updating now. – Derek F Oct 24 '17 at 18:52
  • Hi Derek, using your code is fine, until I tried this against a Custom Setting, without luck. Should this work? – Stuart Oct 26 '17 at 23:38
  • @Stuart I can't say too much given so little information. The general ideas I presented should work fine, though there will likely be differences in the actual code if Custom Settings are involved. At any rate, it sounds like something you should ask a new question for. – Derek F Oct 27 '17 at 1:28
4

The error is here:

for (Case c: Trigger.isUpdate){  // This is line 21

You most likely meant to iterate over Trigger.new:

for (Case c: Trigger.new){

Which is a list that contains the cases which are being updated.

2

You are iterating over a boolean variable at this line.

for (Case c: Trigger.isUpdate)

Trigger.isUpdate will return either true or false. This is used to identify if an update operation is being carried out.

We run a for loop on a collection type, viz. list, set and maps. Here instead of Trigger.isUpdate, you either need to iterate over the context variable Trigger.new(if you want to refer new values) or Trigger.old(if you want to refer old values)

So the code should be like this:

for (Case c: Trigger.new)

or

for (Case c: Trigger.old) 
1

Please exchange Trigger.isUpdate to Trigger.New

1-Trigger.isUpdate is a condition to check record is update or not.

2- You want to iterate so you can use Trigger.New

1

Trigger.isupdate is a bollean variable, either it will return true or false. You have to use Trigger.New to get list newly inserted or updated records.

0

If I were to complete a very similar update (just using different fields), but instead of a custom object update a custom setting, what changes would I need to make to the code provided? I have tried the same method, but it does not seem to update the record.

if(accountIdsToRelatedCasesMap.size()>0) {

List <MyCustomSetting__c> newCS = [SELECT Id, Field1__c, Field2__c FROM MyCustomSetting__c WHERE Field1__c IN :accountIdsToRelatedCasesMap.keyset()];
for(MyCustomSetting__c cs : newCS ) {
    Boolean allTrue = true;

    for(Case c : allAccIds.get(cs.Field1__c) ) {
        if(c.N_Ins__c){
            cs.Approved__c = True;
            updateCS.add(cs);

            break;
        }

    }

}

update updateCS;

}

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.