8

I am new to Salesforce . I am having a bit of problem wriiting a trigger, any help would be appriciated.

I want to Write a trigger that will set the name of an Opportunity record when it is created and when the fields below are updated. The opportunity name should contain the following, each separated by a space: Account Name,Owner Last Name, Opportunity Close Date.

trigger SetOpportunityName on Opportunity (before insert, before update) {
for (Opportunity op: Trigger.new) {
   op.Name = op.Account.Name+' '+Op.Owner.LastName+' '+op.ClosedDate;
   system.debug('*****************************' + op.Name);    
    }

But I am getting op.Account.Name and Op.Owner.LastName values as null.

I know there is a mistake, Please guide me.

|improve this question
  • This sounds like a good use for Workflows. Is that an option for you? – Mike Chale Jan 28 '14 at 20:18
  • No I want to use Trigger :) – SFDC Geek Jan 28 '14 at 20:29
12

You need to query for those values since they don't come down natively in the trigger. Anything that is related will be null (like op.Account.Name)

So I would gather up all the account and OwnerIds and then do separate queries outside of the for loop (otherwise you will have other issues), and then loop again to set the values... Also the owner could be a group so this may be tricky :-P

Set<Id> ownerIds = new Set<Id>();
Set<Id> AccountIds = new Set<Id>();
for (Opportunity op : Trigger.new) {
   ownerIds.add(op.OwnerId);
   accountIds.add(op.AccountId);
}

Map<Id, Account> accountMap = new Map<Id, Account>([SELECT Name FROM Account WHERE Id IN :accountIds]);
Map<Id, User> userMap = new Map<Id, User>([SELECT Name, LastName FROM User WHERE Id IN :OwnerIds]);
Map<Id, Group> groupMap = new Map<Id, Group>([SELECT Name FROM Group WHERE Id IN :OwnerIds]);

String sName;
for (Opportunity op : Trigger.new) {

  // add the account name
  sName = accountMap.get(op.AccountId).Name;

  // add either the group or user name
  if (userMap.get(op.OwnerId) != null) {
    sName += ' ' + userMap.get(op.OwnerId).LastName;
  } else if (groupMap.get(op.OwnerId) != null) {
    sName += ' ' + groupMap.get(op.OwnerId).Name;
  }

  // set the name :-)
  op.Name = sName;
}
|improve this answer
  • I have faced the Error: Compile Error: Illegal assignment from LIST<Account> to MAP<Id,Account> – SFDC Geek Jan 28 '14 at 20:13
  • sorry let me edit the answer to fix it – SalesForceGirl Jan 28 '14 at 20:15
  • the problem was in the creation of the maps :-P its fixed now: Map<Id, Account> accountMap = new Map<Id, Account>([SELECT Name FROM Account WHERE Id IN :accountIds]); Map<Id, User> userMap = new Map<Id, User>([SELECT Name FROM User WHERE Id IN :OwnerIds]); Map<Id, Group> groupMap = new Map<Id, Group>([SELECT Name FROM Group WHERE Id IN :OwnerIds]); – SalesForceGirl Jan 28 '14 at 20:16
  • If I want to fetch only last name of the User then what I have to do? – SFDC Geek Jan 28 '14 at 20:30
  • since the owner could be a group this may not work on all, but change the query on user to get Lastname instead of name. I'll edit the post to show – SalesForceGirl Jan 28 '14 at 20:31
6

There's absolutely nothing wrong with SalesForceGirl's solution, but I don't see the advantage in creating maps when you can just query those fields directly from the opportunity. Simpler is always better imo. You can always refactor later if the need arises.

Obviously this is dependent on whether an owner is always a user:

for (Opportunity op : [select Account.Name, Owner.LastName from Opportunity where Id in :Trigger.new]) {
    Trigger.newMap.get(op.Id).Name = op.Account.Name + ' ' + op.Owner.LastName;
}
|improve this answer
  • 1
    this reduces a for loop :-) – SalesForceGirl Jan 28 '14 at 23:05
  • I don't think this actually works. When you set the fields here, you are setting it relative to that specific list. Not the trigger. Am I incorrect? – dphil Feb 11 '14 at 18:13
  • Actually -I think you're right dphil. I'm going to change my answer now – willard Feb 12 '14 at 0:30
2

If one knows about Salesforces limits and best coding practices you NEVER write a select statement in a loop.

Always use in memory maps in a trigger.

If you consult for a client that has numerous classes and triggers like that you'll never deploy to production without eventually hitting SOQL limits.

SalesforceGirl is 100% correct in the trigger code but I would go with a workflow where those are all standard objects and accessible. Managers and admins can edit those easily if need be.

|improve this answer
  • You would not necessarily do a workflow, as a matter of fact, because of the potential echo effect caused by field updates causing recursive trigger calls, some of which may result in clients with "numerous classes and triggers" not being able to use opportunities. We have different tools for different purposes. It is true that I would also use workflow rules for this unless it were for a client that I knew had triggers I would have to potentially fix in order to do the workflow rule. – sfdcfox Jan 29 '14 at 2:58
  • Using workflow criteria to set an opportunity name you most certainly can. Evaluate the rule when a record is created, and every time it’s edited. Example... (Opportunity: Created By equals Admin User) or (Opportunity: Opportunity Name equals Opportunity Name).. We have this with a great amount of custom code for a specific client as they always want their opp names to stay the same. And I forgot the custom field used :) Update opportunity name with CustomerNiceName__c + Account + CloseDate – user5553 Jan 29 '14 at 3:03
  • sfdcfox I see what your getting at. In many cases we try to empower the client if possible and maybe wrap the trigger from firing. Many people want to manage their instance and remove triggers that are not necessary if you can help it. – user5553 Jan 29 '14 at 3:10
  • 1
    "Configuration over code." That's my mantra. However, we've got both tools, and we have to remember that there are times one is appropriate over the other (and it's usually hard to convince a client of the correct tool to use...). – sfdcfox Jan 29 '14 at 3:13
  • 2
    for(subject record: [select ... ]) is NOT a query in a for loop. It's actually a queryMore statement that reduces heap usage and allows the use of server cursors. This is actually preferred over maps if you only need the result once. A query in a for loop is for(...:...) { subject[] records = [...]; /* do stuff */ }, those are bad. – sfdcfox Jan 29 '14 at 3:17

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.