-2

In below code user__c is lookup to user, but o.owner.id == s.user__c is not taken when saving the record.

public class OpportunityTriggerHelper {

    public static void updateOpportunityWithSectorTarget(Map<Id, Opportunity> mapIdToNewOpportunity, Map<Id, Opportunity> mapIdToOldOpportunity) {
        List<Opportunity> lstOpp = mapIdToNewOpportunity.values();

        set<string> strr = new set<string>();
        for(opportunity o : lstOpp){
            if(o.CloseDate != null ) {
                strr.add(String.valueOf(o.CloseDate.year()));
            }
        }

        List<Sector_Target__c> sec       = [SELECT Id,Fiscal_Year__c ,user__c
                                            FROM   Sector_Target__c 
                                            WHERE  Fiscal_Year__c IN :strr];
        List<Opportunity> lstOppToUpdate = [SELECT Id,owner.id, Sector_Target__c,CloseDate
                                            FROM   Opportunity
                                            WHERE  Id IN :lstOpp];
        for(opportunity o : lstOppToUpdate){ 
            for(Sector_Target__c s : sec){
                if( String.valueOf(o.CloseDate.year()) == s.Fiscal_Year__c && o.owner.id == s.user__c){
                    o.Sector_Target__c = s.id;
                }
            }
        }

        OpportunityTriggerHandler.triggerDisabled = true;
        update lstOppToUpdate;
    }
}
  • 3
    Hi and welcome to SFSE. Could you please edit your question to add information about the custom Sector_Target__c object type, specifically the types for Fiscal_Year__c and User__c. It's important to provide complete detail and some information about what you have tried and the specific problem faced when asking a question. See How to Ask for further information about asking a good question. – Phil W Jan 3 at 8:58
  • Is your User__c field a lookup to the User object? – Martin Lezer Jan 3 at 9:06
  • 1
    is it an after or a before trigger? – m Peixoto Jan 3 at 12:20
2

You should prefer using lookupFieldId instead of lookupField.Id for performance reasons. It is also recommended to not have this type of nested for-loop, as you'll get CPU timeout errors in bulk transactions. Your filter for the sector targets could be more selective for better performance. You'll want to use a Map to match up the records properly.

Altogether, the code should look more like this:

public class OpportunityTriggerHelper {
    public static void updateOpportunityWithSectorTarget(Map<Id, Opportunity> mapIdToNewOpportunity, Map<Id, Opportunity> mapIdToOldOpportunity) {
        // Stuff we want to keep track of
        Opportunity[] newRecords = mapIdToNewOpportunity.values();
        Set<Id> ownerIds = new Set<Id>();
        Set<String> fiscalYears = new Set<String>();
        Map<Sector_Target__c, Sector_Target__c> sectorByYearAndUser = new Map<Sector_Target__c, Sector_Target__c>();

        // Get the user Ids and fiscal years for the filters
        for(Opportunity record: newRecords) {
            if(record.CloseDate != null) {
                fiscalYears.add(record.CloseDate.year()+'');
            }
            ownerIds.add(record.OwnerId);
        }
        // Find targets that match the user and year
        for(Sector_Target__c record: [ 
                SELECT Fiscal_Year__c, User__c 
                FROM Sector_Target__c 
                WHERE User__c = :ownerIds AND Fiscal_Year__c = :fiscalYears]) {
            sectorByYearAndUser.put(
                new Sector_Target__c(User__c = record.User__c, Fiscal_Year__c = record.Fiscal_Year__c),
                record
            );
        }
        for(Opportunity record: newRecords) {
            if(record.CloseDate != null) {
                Sector_Target__c key = new Sector_Target__c(
                    Fiscal_Year__c = record.CloseDate.year()+'', User__c = record.OwnerId
                ), 
                // match is now a simple map operation instead of if statement
                match = sectorByYearAndUser.get(key);
            if(match == null) {
                record.Sector_Target__c = null;
            } else {
                record.Sector_Target__c = match.Id;
            }
        }
    }
}

Note that I changed this to a "before insert" and "before update" scenario. This avoids the need for a recursive trigger update, massively improving performance.

| improve this answer | |
-2

Hey Why dont you just add a system debug and check if you are able to pull the owner.id value or not. if the value is not captured please change it to ownerid

Add the below line :

for(opportunity o : lstOppToUpdate){ 
            for(Sector_Target__c s : sec){
                 system.debug('oWNER : ' + o.owner.id);
                 system.debug('uSER : ' + s.user__c);
                if( String.valueOf(o.CloseDate.year()) == s.Fiscal_Year__c && o.owner.id == s.user__c){
                    o.Sector_Target__c = s.id;
                }
            }
        }
| improve this answer | |
  • Welcome to SFSE! Please read How to Answer. Right now, this answer is what we call a code dump (It's a block of code with little or no explanation). The explanation of why and something works, and how it solves the problem, is as important to the answer as the code is. The goal here is to teach people how to work around and avoid problems. That's hard to do if someone doesn't understand what the problem is. – Derek F Jan 6 at 13:41
  • This answer might also be better suited as a comment instead (though you'll need 50 reputation to be able to do that). – Derek F Jan 6 at 13:42

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