Fix this trigger: Opportunity Renewal

// Automatically create a Renewal Opp for closed won deals
trigger CreateRenewal on Opportunity (after update) {

// Create a Map to store all renewal opps for bulk inserting
List renewals = new List();

for (Opportunity opp : Trigger.new) {
// Only create renewal opps for closed won deals
if (opp.StageName == ‘Closed Won’) {
Opportunity renewal = new Opportunity();
renewal.AccountId = opp.AccountId;
renewal.Name = opp.Name + ‘Renewal’;
renewal.CloseDate = opp.CloseDate + 365; // Add a year
renewal.StageName = ‘Open’;
renewal.RecordTypeId = opp.RecordTypeId;
renewal.OwnerId = opp.OwnerId;
renewals.add(renewal);
}
}

// Bulk insert all renewals to avoid Governor Limits
if (renewals.size() > 0 )
insert renewals;
}

// Automatically create a Renewal Opp for closed won deals
trigger CreateRenewal on Opportunity (before update) {

// Create list to store all new opportunitites
List renewals = new List();

for (Opportunity opp : Trigger.new) {
// Only create renewal opps for closed won deals
if (opp.StageName.equalsIgnoreCase(‘Closed Won’)) {
Opportunity renewal = new Opportunity();
renewal.AccountId = opp.AccountId;
renewal.Name = opp.Name + ‘Renewal’;
renewal.CloseDate = opp.CloseDate.addDays(365) ; // Add a year
renewal.StageName = ‘Open’;
renewal.RecordType = Schema.SObjectType.Opportunity.getRecordTypeInfosByName().get(‘Renewal’).getRecordTypeId();
renewal.OwnerId = opp.OwnerId;
renewals.add(renewal);
}
}

// Bulk insert all renewals to avoid Governor Limits
insert renewals;
}

trigger CreateRenewal on Opportunity (after update) {

List renewals=new List();

for(Opportunity opp: Trigger.new)
{
if(opp.stageName==’Closed Won’)
{
Opportunity renewal= new Opportunity();
renewal.AccountId= opp.AccountId;
renewal.Name= opp.Name + ‘Renewal’;
renewal.CloseDate= opp.CloseDate+365;
renewal.StageName=’Open’;
renewal.RecordType__c=’Renewal’;
renewal.OwnerId= opp.OwnerId;
renewals.add(renewal);

}
insert renewals;

}
}

Problems

1) Assuming it’s 365 days, Close Date should use addYears(1) since you can’t just add 365 to the date.

2) Have to use RecordTypeId instead of RecordType since Record Type is an SObject.

3) Opportunities consist of Opportunities and Opportunity Line Items.
The renewal only consists of the Opportunities in the example. Even though
the requirement doesn’t say so, I’d think that the renewal should list the
products to renew as well, their quantity, and the unit price for each.

4) Uses the wrong stage, “Closed”, instead of “Closed Won” and it doesn’t see
if the opportunity was updated to closed won.

5) When appending “Renewal” to the name, it should have a space between for
readability. If my Opp name is “My Opp” and it’s closed won, the
renewal name will be “My OppRenewal”.

6) Even though the trigger is currently using “Before Update”, it should check
for that condition to ensure the code is only running for that in case
additional trigger events are added later.

7) Using the renewal.Id makes no sense because the Id is null since it hasn’t
been inserted yet. A simple list of Opportunities to insert would suffice.

8) The StageName should be something else such as “Proposal/Price Quote”.

9) One can’t insert a map. It has to be a list.

10) Before invoking “insert” or any dml operation, see if there’s anything to do first.
Technically, the insert will still pass with an empty list but it takes more time
than is actually needed.

11) The AccountId can’t be assigned a String of ‘opp.AccountId’.

12) NitPicky…. I’d name the trigger “CreateRenewalOpportunity or make it
a more generic trigger that delegates to a “Trigger Handler”.

13) There’s no test code. Each trigger has to have at least 1% code coverage.

Solution

/*
Disclaimer: This code is uncompiled and untested.

The following assumes that
1) Multi-Currency isn’t enabled
2) Only English needs to be supported.
3) Opportunity Line Items should be renewed also
4) Only one opportunity trigger is used instead of creating one for each
“Opportunity Feature”.
5) Test Code will be created later….
*/

trigger OpportunityTrigger on Opportunity (before update){

if (Trigger.IsBefore && Trigger.IsUpdate){
OpportunityTriggerHandler.createRenewalOpportunities(Trigger.oldMap, Trigger.newMap);
}

}

public class OpportunityTriggerHandler {

private static Id RenewalOppRecordTypeIdPriv = null;

private static Id RenewalOppRecordTypeId {
get {
if (RenewalOppRecordTypeIdPriv == null){
Schema.DescribeSObjectResult R = opportunity.SObjectType.getDescribe();
RenewalOppRecordTypeIdPriv = r.getRecordTypeInfosByName().get(‘Renewal’).getRecordTypeId();
}

return RenewalOppRecordTypeIdPriv;
}
}

public static void createRenewalOpportunities(Map oldOpps, Map newOpps){
Map renewalOpps = new Map();

for (Opportunity newOpp : Trigger.New){
Opportunity oldopp = Trigger.oldMap.get(newOpp.Id);

if (isNowClosedWon(oldOpp, newOpp){

Opportunity renewalOpp = createRenewalOpp(newOpp);
renewalOpps.put(newOpp.Id, renewalOpp);
}
}

// insert only if there are items to insert.
if (renewalOpps.size() > 0){
insert renewalOpps.values();
}
else {
// No opps became closed won so return immediately. Don’t unnecessarily
// do more work than is necessary.
return;
}

List closedWonOppLines = getClosedWonOppLines(renewalOpps.keySet());
List renewalOppLines = new List();

for (OpportunityLineItem closedWonOppLine : closedWonOppLines){
// Deep clone without preserving the id, timestamps, or autonumber
OpportunityLineItem renewalOppLine = closedWonOppLine.clone(false, true, false, false);

// Link the renewal opportunity line item to the appropriate renewal opportunity
Id renewalOppId = renewalOpps.get(closedWonOppLine.OpportunityId).Id;
renewalOppLine.OpportunityId = renewalOppId;

renewalOppLines.add(renwalOppLine);
}

if (renewalOppLines.size() > 0){
insert renewalOppLines;
}
}

private static Boolean isNowClosedWon(Opportunity oldOpp, Opportunity newOpp){
return oldOpp.StageName != newOpp.StageName &&
newOpp.StageName == ‘Closed Won’;
}

private static Opportunity createRenewalOpp(Opportunity closedWonOpp){
Opportunity renewalOpp = new Opportunity();

renewalOpp.AccountId = closedWonOpp.AccountId;
renewalOpp.Name = closedWonOpp.Name + ‘ Renewal’;
renewalOpp.CloseDate = closedWonOpp.CloseDate.addYears(1);
renewalOpp.StageName = ‘Proposal/Price Quote’;
renewalOpp.RecordTypeId = RenewalOppRecordTypeId;
renewalOpp.OwnerId = opp.OwnerId;

return renewalOpp;
}

private List getClosedWonOppLines(Set closedWonOppIds){
return [Select Id,
Name,
OpportunityId,
PricebookEntryId,
Product2Id,
Quantity,
UnitPrice,
TotalPrice
from OpportunityLineItem
where OpportunityId = :closedWonOppIds];
}
}

trigger CreateRenewal on Opportunity (after update) {

list renewals = new list();
id recTypeId = Schema.Sobjecttype.Opportunity.getRecordTypeInfosByName().get(‘Renewal’).getRecordTypeId();

for (Opportunity opp : Trigger.new)
{
if (opp.isWon)
{
Opportunity renewal = new Opportunity();
renewal.AccountId = opp.AccountId;
renewal.Name = opp.Name + ‘Renewal’;
renewal.CloseDate = opp.CloseDate.addDays(365);
renewal.StageName = ‘Qualification’;
renewal.RecordTypeId = recTypeId;
renewals.add(renewal);
}
}
insert renewals;
}

Changes :
1. Before update to after update
2. map to list
3. Changed if condition from if (opp.StageName.contains(‘Closed’)) to if (opp.isWon)
4. Created new variable(recTypeId) of type id to save the id of record type Renewal.
5. Changed the value for record type from enewal.RecordType = ‘Renewal’ to renewal.RecordTypeId = recTypeId;
6. Changed date value from renewal.CloseDate = opp.CloseDate + 365 to renewal.CloseDate = opp.CloseDate.addDays(365);
7. Changed renewals.put(renewal.Id, renewal) to renewals.add(renewal);
8. removed the single quote for renewal.AccountId = ‘opp.AccountId’; to renewal.AccountId = opp.AccountId;

trigger CreateRenewal on Opportunity (after update) {

list renewals = new list();//list is my preference but a map would work

for (Opportunity opp : Trigger.new) {

//only create renewal opp for deals that have just recently been closed won.
//Otherwise everytime the record was editied a renewal opportunity would be generated.

Opportunity priorvalue = Trigger.oldMap.get(opp.Id); //grab the priorvalue oppty id.

boolean alreadyClosed = priorvalue.StageName.equals(‘Closed Won’);//create flag to catch opportunities that had already been closed won.

if (opp.StageName ==’Closed Won’ && !alreadyClosed)

{

Opportunity renewal = new Opportunity();
renewal.AccountId = opp.AccountId;
renewal.Name = opp.name + ‘ Renewal’;
renewal.CloseDate = opp.CloseDate + 365;
renewal.StageName = ‘Open’;
renewal.RecordTypeID = ‘012U000000012Q0’; //change to recordtypeID
renewal.OwnerId = opp.OwnerId;

renewals.add(renewal); //add new opp(s) to list
}
}

insert renewals;

}

In the earlier trigger its written “renewals.put(renewal.Id, renewal);”
is taken inside the for loop it shud be outside the for loop where map is declared .

Ideal steps which should be followed are:

step1.Create set of records to stagename=closed won.
step 2 Create a map with id store the opportunity.
step 3 loop through trigger.new and add business logic

trigger CreateRenewal on Opportunity (before update,before insert)
{

//create a set of records

set oSet = new set();
if( trigger.isInsert|| trigger.isUpdate)
{
for(opportunity o : trigger.new)
{
if (o.StageName ==’Closed Won’)
{
oSet.add(o.AccountId);
}
}
}

// Create a Map to store all renewal opps for bulk inserting
list oppid = [SELECT AccountId,CloseDate ,Name , StageName,OwnerId FROM opportunity
WHERE AccountId IN : oSet];
Map renewals = new Map ();

// Only create renewal opps for closed won deals
{ for(opportunity u : oppid)
{
renewals.put(u.Id,u);
}
}

//trigger.new and logic

list addhere = new list();
for(opportunity o : trigger.new)
{
if(o.StageName == ‘Closed Won’)
{

opportunity oppis = renewals.get(o.Id);
if (oppis != null)
{
o.AccountId = oppis.AccountId;
o.Name = oppis.Name + ‘Renewal’;
o.CloseDate = oppis.CloseDate + 365; // Add a year
o.StageName = ‘Open’;
o.OwnerId = oppis.OwnerId;
}
}
}

// Bulk insert all renewals to avoid Governor Limits
insert addhere;
}

trigger CreateRenewal on Opportunity (after update, after insert) {

List renewals = new List();

for (Opportunity opp : Trigger.new) {
// Only create renewal opps for closed won deals
if (opp.StageName.contains(‘Closed’)) {
Opportunity renewal = new Opportunity();
renewal.Account = opp.Account;
renewal.Name = opp.Name + ‘Renewal’;
renewal.CloseDate = opp.CloseDate + 365; // Add a year
renewal.StageName = ‘Open’;
renewal.RecordType = ‘Renewal’;
renewal.Owner = opp.Owner;
renewals.add(renewal);
}
}

insert renewals;
}

How does this look?

trigger CreateRenewal on Opportunity (before update) {

List lstRenewals = new List();

for (Opportunity opp : Trigger.new) {
// Only create renewal opps for closed won deals
if (opp.StageName.contains(‘Closed’)) {
Opportunity renewal = new Opportunity();
renewal.AccountId = opp.AccountId;
renewal.Name = opp.Name + ‘Renewal’;
renewal.CloseDate = opp.CloseDate + 365; // Add a year
renewal.StageName = ‘Open’;
renewal.Type = ‘Renewal’;
renewal.OwnerId = opp.OwnerId;
lstRenewals.add(renewal);
}
}

insert lstRenewals;
}