0

I'm a beginner with code and try to do the following:

When an opportunity is closed won, then create a Task under all opportunities that are child of the same account.

My problem with the code below is that it links all the new tasks to the same opportunity. So if the account has 3 opportunities, 3 new tasks will be linked to the closed won opportunity, and not 1 task under each opportunity as I'd like to. This is because I assign the new task whatId to the ID of the opportunity which triggered the code: newtask.WhatId = myOpp.id;.

How to create 1x task under each opportunity ID part of the OppList?

Thanks!

trigger OpportunityTrigger on Opportunity (after update) {

List<Task> taskList = new List<Task>(); // to store tasks that will be created at the end 
Set<id> accountIDs = new Set<id>();  // to store account IDs

//Gets the list of all accounts in which one Opp is changed as Closed Won
for(Opportunity opp : Trigger.new) // trigger fires for oppty that have a stage change and where new stage = Closed Won. 
{
    if( Trigger.oldMap.get(opp.id).stagename != Trigger.newMap.get(opp.id).stagename  // Is this is really needed??
       && Trigger.newMap.get(opp.id).stagename == 'Closed Won')  // this is for sure needed
    {
        accountIDs.add(opp.accountid); // add the Account ID of the opp to the AccountIDs set variable
    }
}

//Get the list of all those accounts along with their list of opp which are not closed won.
Map<id,Account> acctWithOppMap = new Map<id,Account>([SELECT Id,(SELECT Id,ownerid,stagename 
                                                                 FROM Opportunities where stagename !='Closed Won') FROM Account 
                                                                WHERE Id IN :accountIDs]);

//Loop through accounts saved in acctWithOppMap
for(Account ac : acctWithOppMap.values())
{
    //Loop through opportunities of these accounts
    for(Opportunity op : ac.Opportunities)
    {

        //Create the task parameters
        Task tk = new Task();
        tk.Subject = 'Test Task 22';
        tk.OwnerId = op.ownerid;
        tk.whatid = op.id;

        //Add the task to the list
        taskList.add(tk);

    }
}

// create tasks
insert tasklist; }
1

You're assigning the wrong WhatId:

newtask.WhatId = myOpp.id;

Your inner loop over the Opportunities attached to the Account has the loop variable opp. You would want to assign the WhatId to opp.Id.

You probably also want to check whether opp.Id == myOpp.Id, otherwise that Opportunity will get a task too.

Another issue with this trigger is that it is not bulkified and will immediately start throwing LimitsException if you were to bulk update more than 100 Opportunities. (You may not think that's a likelihood now, but I've been bitten more times than I can count by code that makes that assumption!)

You need to do some basic bulkification of this trigger to remove your Account query from the Opportunity loop. The typical way to do this is to accumulate Opportunity Account Ids in a Set, query for them in a single step, and then use a Map<Id, Account> to access them as you loop over the changed Opportunities a second time. It would look something like this, in order:

  1. Create Set<Id> to store Account ids.
  2. Loop over Trigger.new and put all Account ids into that set.
  3. Query for Accounts using a subquery to get Opportunity data (something like [SELECT Id, (SELECT Id FROM Opportunities) FROM Account WHERE Id IN :ids]). Save this data in a Map<Id, Account>.
  4. Create a List<Task> to store inserts.
  5. Iterate over Trigger.new again. For each Opportunity, get its Account from the Map.
  6. For that Account, loop over a.Opportunities, where the Account is a.
  7. For each of those Opportunities, if its Id is not in Trigger.newMap.keySet(), construct a Task and add it to the list.
  8. Outside all of the loops, do a single insert taskList.

Taking some time to work through Bulk Apex Triggers will reward you in working with code like this.

  • I've followed the instructions and I think I'm OK with points 1 to 4. I struggle with point 5 / 6 and 7 though. Not sure if my start with point 5 is correct and then how to continue. Could you help again? Maybe it's best if you can give me the right code and I can use it as a basis to play around with it – Xavier T. Mar 31 '18 at 10:49
  • 1
    Hey Xavier, I see that you are a newbie and you're struggling mightily and in good faith with Apex, but I don't think it's a useful learning tool if I write your code for you. (It's also typically discouraged on SFSE). Frustrating as it is, I think based on what I see in your code that you need to continue working with Trailhead or graduate to the Apex Developer Guide to build your conceptual vocabulary and comfort with logic in code. As a next step in what you've written, look very carefully at all of your loops - some are not doing what you think, and some code doesn't belong inside one. – David Reed Mar 31 '18 at 11:31
  • Hi David - I worked on the code with a friend and added the new version here. It seems to be doing the job but would like to have your opinion once again to make sure I'm following best practices. Thanks! – Xavier T. Apr 5 '18 at 18:16

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.