Is this trigger working how it is required to?

Question

What this trigger is to do:

Get a related list of Opportunity Line Items IF the stage on Opportunity = Closed Won. Once I get this related list, I need to create a new Opportunity with the account field on Opportunity being equal to the Dealership__c field on Opportunity Line Item. So, for example, a dealership 'Test Dealership 1' (Which is an account type lookup) will have some opportunity line items associated with it in the Related List on an Opportunity like so:

Order Item: Wifi
Quantity: 2
Ratecard: 30
Sales Price: 360
Total Price: 720
Dealership: Test Dealership 1

Order Item: Mobile
Quantity: 4
Ratecard: 69
Sales Price: 828
Total Price: 3312
Dealership: Test Dealership 1

And there can also be another Dealership 'Test Dealership 2'

Order Item: Laptop
Quantity: 8
Ratecard: 630
Sales Price: 630
Total Price: 5040
Dealership: Test Dealership 2

What needs to happen:

One new opportunity created with account name = Dealership 1, this opportunity should have a related list of opportunity line items with products Wifi and Mobile.

Then another new opportunity should be created with account name = Dealership 2 with a related list of products containing Laptop as an opportunity line item.

Here is my code:

trigger ParentOppToChild on Opportunity (after update) { Map<ID, Opportunity> oppList = new Map<ID, Opportunity>(); List<OpportunityLineItem> listOfOpportunityLineItems = new List<OpportunityLineItem>(); for(Opportunity opp: Trigger.new) { if(opp.StageName == 'Closed Won') { for(OpportunityLineItem oli : [Select OpportunityId, Quantity From OpportunityLineItem Where OpportunityId In : oppList.keyset()]) { Opportunity OppObj = oppList.get(oli.OpportunityId); listOfOpportunityLineItems.add(oli); } Update listOfOpportunityLineItems; for(OpportunityLineItem lineItem : listOfOpportunityLineItems) { if(lineItem.Dealership__c != Null) { Opportunity newOpp = new Opportunity(); newOpp.Account_holder_name__c = lineItem.Dealership__c; newOpp.Name = lineItem.Dealership__c; newOpp.CloseDate = Date.today(); newOpp.StageName = 'Prospecting'; newOpp.ForecastCategoryName = 'Commit'; } } } } }

Is my code on the right track to doing this? If anyone can help or let me know what I basically need to do I would really appreciate it. Thanks in advance!

Test class:

@isTest public class ParentOppToChildTest { static testMethod void triggerTestMethod() { test.startTest(); Id pricebookId = Test.getStandardPricebookId(); //Create product Product2 prod = new Product2(Name = 'Product X', ProductCode = 'Pro-X', isActive = true); insert prod; //Create pricebook entry PricebookEntry pbEntry = new PricebookEntry(Pricebook2Id = pricebookId,Product2Id = prod.Id,UnitPrice = 100.00,IsActive = true); insert pbEntry; Opportunity oppty = new Opportunity(); Opportunity opp = new Opportunity(AccountId='0012500000C5G06', Name='Test Opp',StageName='Closed Won', CloseDate=Date.today()); insert opp; //Create opportunity line item OpportunityLineItem oli = new OpportunityLineItem(OpportunityId = opp.Id, Dealership__c = opp.AccountId, Quantity = 5,PricebookEntryId = pbEntry.Id,TotalPrice = 5 * pbEntry.UnitPrice); insert oli; system.debug(oli.Dealership__c); test.stopTest(); } }

Test coverage with this test class is 57%

Nik, have you run your code and tested it? Have you inserted some system.debug() statements to check your code's values as it runs? Are there specific problems you're running into? This site is more suited to questions around details of your code, rather than a general code review. — smohyee
– smohyee, Commented Aug 24, 2015 at 14:56
Hi @Smohyee I have run debug statements, and gone about different ways to write the code but I am not sure if this is the right way to go about doing something like this. If anyone had any idea about what I need to read up on or have a look at to do something like this it would be of great help as I have researched online for some information but I have not seen any tutorials or information for tackling something like this. — Nik
– Nik, Commented Aug 24, 2015 at 14:58
Did anything go wrong when you tried to implement the code as you've written it? — smohyee
– smohyee, Commented Aug 24, 2015 at 15:13
Start working on unit tests, and use plenty of assertions on what you want the code to do. — Adrian Larson
– Adrian Larson ♦, Commented Aug 24, 2015 at 15:14
Yes @smohyee I tried to to say that Dealership__c = 'Test Dealership' in my apex test class but it said invalid id. I then tried Dealership__r.Name but that did not work either and now I wonder whether this means I need to hardcode a numerical id into the test which I would prefer not to do. — Nik
– Nik, Commented Aug 24, 2015 at 15:20

smohyee · Accepted Answer · 2015-08-24 16:17:45Z

Here are some issues I see with your code currently.

for(OpportunityLineItem oli : [Select OpportunityId, Quantity From OpportunityLineItem Where OpportunityId In : oppList.keyset()]) { Opportunity OppObj = oppList.get(oli.OpportunityId); listOfOpportunityLineItems.add(oli); }

The above code iterates through a list of Opportunity Line Items that you pull via a SOQL query. Your SOQL query only pulls the OpportunityID and the Quantity from each line item. However, you state in your requirements that you want to use the Dealership__c field from your OLIs to create new opportunities. You'll need to include that field in your SOQL query.

Next, let's look at the two lines of code you want to execute on each OLI that you pull via that query:

 Opportunity OppObj = oppList.get(oli.OpportunityId); listOfOpportunityLineItems.add(oli);

The first line attempts to assign the ID of the opportunity of an OLI to a new Opportunity record you've created called 'OppObj'. This will result in an error: either assign an ID to an ID variable, or query the opportunity using the ID then assign that opp to the opp variable. However, this line is actually not doing anything useful in your code, because you don't do anything with OppObj at any point.

In fact, that whole section is unecessary, because it's possible to assign a SOQL query's results directly to a list you've created. So instead of just creating a list and not assigning anything, like this

 List<OpportunityLineItem> listOfOpportunityLineItems = new List<OpportunityLineItem>();

Instead, use this:

List<OpportunityLineItem> listOfOpportunityLineItems = [Select OpportunityId, Quantity From OpportunityLineItem Where OpportunityId In : Trigger.new]

Note also that this query pulls directly from Trigger.new, aka all the opps that caused this trigger to run in the first place. This prevents you from having to loop through each Opp with that top level for loop you have.

So Nik, at this point I would recommend that you take a step back and do some tutorials on triggers, SOQL queries and the like. I think your code reveals some basic misunderstanding of Apex, and unless I write the whole thing for you I don't think you're in a good position to figure out a solution without some more training.

edit: Another tip: An important consideration for how the structure of this code will look once complete is whether this needs to be scalable at all. This trigger could potentially be fired by a single opportunity being updated, or by one hundred opps (if say you used a data loading tool to update a bunch of opps simultaneously). When I was first learning to code, I found it a lot easier to understand triggers that weren't written to be scalable (aka only reliably work with one or a few opps at a time without hitting governors limits). Just beware that if you design it that way, you may run into problems with mass updates later on.

Thanks for writing all this out @smohyee, I think I've been looking at this for way too long and need to look at it again through a fresh pair of eyes and armed with some more knowledge. Thanks again — Nik
– Nik, Commented Aug 24, 2015 at 16:09

Stack Exchange Network

Is this trigger working how it is required to?

1 Answer 1

You must log in to answer this question.

Hot Network Questions