6

Apex n00b here. This has been gnawing me up over past few weeks. I have this very simple before insert / update trigger on Opportunity that auto-selects the Price Book based on a dropdown value containing Sales Office (State) location info.

Here's my Trigger:

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


    for( Opportunity opp : Trigger.new ) {
        // Change Price Book
        // New York
        if( opp.Campus__c == 'NYC' )
          opp.Pricebook2Id = PB_NYC; // contains a Pricebook's ID

        // Atlanta
        if( opp.Campus__c == 'ATL' )
          opp.Pricebook2Id = PB_ATL; // contains another Pricebook's ID

    }

}

Here's my Test Class:

@isTest (SeeAllData = true)
public class SelectPriceBookTestClass {

    static testMethod void validateSelectPriceBook() {

        // Pricebook IDs
        ID PB_NYC =  'xxxx';
        ID PB_ATL =  'xxxx';

        // New Opp
        Opportunity opp = new Opportunity();
        opp.Name = 'Test Opp';
        opp.Office__c = 'NYC';
        opp.StageName = 'Quote';       

        // Insert
        insert opp;

        // Retrive inserted opportunity
        opp = [SELECT Pricebook2id FROM Opportunity WHERE Id =:opp.Id];
        System.debug( 'Retrieved Pricebook Id: ' + opp.Pricebook2Id );

        // Change Campus
        opp.Office__c = 'ATL';
        // Update Opportunity
        update opp;

        // Retrive updated opportunity
        opp = [SELECT Pricebook2id FROM Opportunity WHERE Id =:opp.Id];
        System.debug( 'Retrieved Updated Pricebook Id: ' + opp.Pricebook2Id );        

        // Test
        System.assertEquals( PB_ATL, opp.Pricebook2Id );

    }
}

Test runs report 0% coverage.

Please suggest a fix for this.

Also on similar lines I have this other before insert trigger that sets the Owner of an Event same as the Owner of the parent Lead.

Here's the code:

trigger AutoCampusTourOwner on Event( before insert ) {

    for( Event evt : Trigger.new ) {
        // Abort if other kind of Event
        if( evt.Subject != 'Visit' )
            return;        

        // Set Owner Id
        Lead parentLead = [SELECT OwnerId FROM Lead WHERE Id = :evt.WhoId];
        evt.OwnerId = parentLead.OwnerId;

    }
}

This too is causing 0% coverage - my guess is it's got something to do with the for loops in both. I know I'm seriously flouting DML rules by invoking SOQL query inside a for loop - but for my purposes it should be fine, as these Events are created manually and only 1 at a time. So there are no scopes of governor limits kicking in due to bulk inserts.

The code in both cases works a 100%. PLEASE suggest a fix for the test cases.

Thank you.

  • In the first trigger you're checking for the campus__c field in the if condition, but in the test class you're setting the 'NYC' value for the office__c field. Change that field in the test class to Campus__c. – Sam Oct 10 '13 at 13:47
  • is your test class not supposed to be called Test_NameofYourTrigger, i.e., Test_SelectPriceBook? followed by on the next line ' > public static testMethod void UnitTest() ' – Bart Oct 10 '13 at 13:52
  • @theGreatDanton Sorry - that's a typo. It should all be Office__c. I've modified the code - with same results. – miCRoSCoPiC_eaRthLinG Oct 11 '13 at 15:09
  • @Bartley Is that the present method of doing things? I picked up this syntax from all the examples out there on the web - but am not quite sure whether my code uses the most updated syntax. SF doesn't produce any code errors though. – miCRoSCoPiC_eaRthLinG Oct 11 '13 at 15:11
4

Firstly it's good that you recognised the issue with the soql query inside the foor loop, however it is worth fixing, although they are being created manually you never know if later on down the track you need to data load or something changes to that effect, besides it's good practice to do it right the first time. Consider changing to something like this unless anyone else has something more efficient. This way we only have 1 query.

trigger AutoCampusTourOwner on Event( before insert ) 
{
    map<ID,ID> mapLeadtoOwnerId = new map<ID,ID>();

    for( Event evt : Trigger.new )
    {
        // Abort if other kind of Event
        if( evt.Subject != 'Visit' )
        {
            // put ownerId into map
            mapLeadtoOwnerId.put(evt.WhoId, null);
        }
    }

    // Add the ownerIds to the map
    for(Lead parentLead = [SELECT OwnerId FROM Lead WHERE Id IN mapLeadtoOwnerId.keyset()])
    {
        mapLeadtoOwnerId.put(parentLead.Id, parentLead.OwnerId);
    }

    // Loop through again now that we have the OwnerIds
    for( Event evt : Trigger.new)
    {
        if(evt.Subject != 'Visit')
        {
            evt.OwnerId = mapLeadtoOwnerId.get(evt.WhoId);
        }
        }
}

For the test coverage if at all possible I would suggest avoiding using (SeeAllData = true), instead create your test data in your method, also use test.starttest() and test.stoptest() annotation to signal where you want your test to start and end.

I would also probably change the first trigger to get the correct pricebook from a map as well rather than via an ID and check if the value has changed I.e.

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

    map<String,Id> mapPriceBooks = new map<String,Id>();

    for(PriceBook2 sPricebook : [SELECT Id, Name FROM PriceBook2])
    {
        mapPriceBooks.add(sPriceBook.Name, sPriceBook.Id);
    }

      for( Opportunity opp : Trigger.new ) {
    // Change Price 

    if(Trigger.isInsert)
    {
        // New York
        if( opp.Campus__c == 'NYC' )
          opp.Pricebook2Id = mapPriceBooks.get(PB_NYC); // returns the correct Pricebook's ID

        // Atlanta
        if( opp.Campus__c == 'ATL' )
          opp.Pricebook2Id = mapPriceBooks.get(PB_ATL); // returns the correct Pricebook's ID
    }
    else if(Trigger.isUpdate)
    {
        Opportunity beforeUpdate = Trigger.oldMap.get(opp.Id);

        // Check if value has changed
        if(opp.Campus__c != beforeUpdate.Campus__c)
        {
            // New York
            if( opp.Campus__c == 'NYC' )
              opp.Pricebook2Id = mapPriceBooks.get(PB_NYC); // returns the correct Pricebook's ID

            // Atlanta
            if( opp.Campus__c == 'ATL' )
              opp.Pricebook2Id = mapPriceBooks.get(PB_ATL); // returns the correct Pricebook's ID
        }
    }
}

Maybe try those 3 things and see how you go. Also when you say that it is causing 0% test coverage is there any failures or is it just not covering any of the triggers lines?

| improve this answer | |
  • totally agree here with @MitchHunt. Even though 'For your purposes' the SOQL in the for loop should be fine. you really should always write your code using best practices, as it could be run in any situation, not just the situation you are planning for at this time. Things change and this is the type of thing where you will be coming back months or years from now to bulkify this, so you should just bulkify it from the beginning – Chris Duncombe Oct 10 '13 at 14:52
  • Re SeeAllData - unless something has changed, you can't create PriceBooks, or at least the default Pricebook in a test context...which makes testing code involving Pricebooks challenging... – BritishBoyinDC Oct 10 '13 at 16:34
  • @Mitch Hunt Thank you very much. The first routine worked like a charm (AutoCampusTour) with the fixed you suggested. Still working on the other one. – miCRoSCoPiC_eaRthLinG Oct 15 '13 at 8:43
  • @"Mitch Hunt" The second example still keeps failing. Since I can't paste more code here, the Trigger is at: pastebin.com/Trh1ap2q and the Test Class is at: pastebin.com/HvWGWxxZ. Please advice. – miCRoSCoPiC_eaRthLinG Oct 15 '13 at 12:17
  • @"miCRoSCoPiC_eaRthLinG" I will have a look at it in the morning for you. – Mitch Hunt Oct 15 '13 at 13:06

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.