Loop variable must be of Type Id

Question

I want to setup logic in a before update method where if an Opportunity's AccountId is changed two fields on the Opportunity is updated based on the new Account record.

I found a bunch of examples, one here, where example code is combined into one large method. As I have been trying to separate my concerns I've tried to pull apart each piece into it's own so to better test and understand what is going on.

My question is how do I correctly write the update method to reference a map of Opportunities and Accounts.

Currently, I get the compile error:

Loop variable must be of type Id

On this line: for(Opportunity oppy : filtered.keyset())

public static Map<Id, Id> updatePracticeAndParentAccountFilter(Map<Id, Opportunity> oldMap, Map<Id, Opportunity> newMap)
{
    Map<Id, Id> oppys = new Map<Id, Id>();

    for(Opportunity o : newmap.values())
    {
        if(
            o.AccountId != oldMap.get(o.Id).AccountId &&
            o.AccountId != null
        )
        oppys.put(o.Id, o.AccountId);
    }
    return oppys;
} 

public static void queryAccountupdateOpportunity(Map<Id, Id> filtered)
{
    Map<Id, Account> accMap = new Map<Id, Account>();

    accMap.putAll([SELECT   Id, 
                            Practice__c,
                            ParentId
                   FROM     Account
                   WHERE    Id IN : filtered.values()]);

    for(Opportunity oppy : filtered.keyset())
    {
        Account a = AccMap.get(oppy.AccountId);
        oppy.Practice__c = a.Practice__c;
        oppy.Parent_Account__c = a.ParentId;
    }
}       

Answer 1

Here is your problem:

for (Opportunity oppy : filtered.keySet())

The key set is of the type Set<Id>, so you must use a loop variable of type Id, or change your loop collection variable.

It would probably make the most sense to have your filter method return a List<Opportunity> and just iterate through that:

public static List<Opportunity> hasAccountChanged(List<Opportunity> newRecords, Map<Id, Opportunity> oldMap)
{
    List<Opportunity> changed = new List<Opportunity>();
    for (Opportunity o : newRecords)
        if (o.AccountId != oldMap.get(o.Id).AccountId && o.AccountId != null)
            changed.add(o);
    return changed;
}
public static void myOtherMethod(List<Opportunity> records)
{
    Set<Id> accountIds = new Set<Id>();
    for (Opportunity record : records) accountIds.add(record.Accountid);
    Map<Id, Account> accounts = new Map<Id, Account>([
        SELECT ParentId, Practice__c FROM Account WHERE Id IN :accountIds
    ]);
    for (Opportunity record : records)
    {
        Account parent = accounts.get(record.AccountId);
        record.Parent_Account__c = parent.ParentId;
        record.Practice__c = parent.Practice__c;
    }
}

Answer 2

You don't need to receive Map that contains Opportunity id and Account Id as map, because Opportunity record already stores such information, so I replaced it with filtered List and then iterate in the loop this filtered records

public static List<Opportunity> updatePracticeAndParentAccountFilter(Map<Id, Opportunity> oldMap, Map<Id, Opportunity> newMap){
    List<Opportunity> oppys = new List<Opportunity>();
    for(Opportunity o : newmap.values()){
        if(
           o.AccountId != oldMap.get(o.Id).AccountId &&
           o.AccountId != null
        ) {
           oppys.add(o);
        }
    }
    return oppys;
} 

public static void queryAccountupdateOpportunity(List<Opportunity> filtered){
    Map<Id, Account> accMap = new Map<Id, Account>([
        select Id, Practice__c, ParentId
        from Account
        where Id in: getAccountIds(filtered)
    ]);
    for(Opportunity oppy : filtered){
        Account a = AccMap.get(oppy.AccountId);
        oppy.Practice__c = a.Practice__c;
        oppy.Parent_Account__c = a.ParentId;
    }
}

private static Set<Id> getAccountIds(List<Opportunity> filtered){
     Set<Id> result = new Set<Id>();
     for(Opportunity opp:filtered) {
         result.add(opp.AccountId);
     }
     result.remove(null);
     return result;
}

Answer 3

for(Opportunity oppy : filtered.keyset())
{
    Account a = AccMap.get(oppy.AccountId);
    oppy.Practice__c = a.Practice__c;
    oppy.Parent_Account__c = a.ParentId;
}

filtered.keyset() is of type Set<Id> and you are iterating over Opportunity,

Correct format should be

//Iterate Over Type of ID
for(Id objId : filtered.keyset())
{
}