0

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;
    }
}       
2

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;
    }
}
  • In this approach you can't split this to 3 methods correct? Filter, query, update Because you need to reference the query in the update? Or can you? – Dan Wooding Jun 8 '17 at 18:04
  • I don't see any benefit in further separation of concerns here. The query is part of the update. If you take the paradigm to its logical extreme you will break just as badly as if you ignore it completely. You have to find balance. I pretty much aim for the most terse possible implementation (least verbose). – Adrian Larson Jun 8 '17 at 18:07
  • Okay, I may have gotten close to the line of extreme in some other pieces of code I was working on earlier in the week and that logic of separate at all costs was screwing with me on this. Thanks. – Dan Wooding Jun 8 '17 at 18:10
1

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;
}
  • You forgot to change the return type of the first method. – Adrian Larson Jun 8 '17 at 18:08
1
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())
{
}

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.