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%

Answer 1

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.