0
\$\begingroup\$

I have this controller and some test cases. Any corrections/advice are appreciated to make them better.

public with sharing class MTX_MatrixDetailViewController {

    public transient String activities {get;set;}

    public MTX_MatrixDetailViewController(){
        
        String candidateId =  ApexPages.currentPage().getParameters().get('id');

         Map<String, String> filters = new Map<String, String>();
        
        List<LightningActivity> actList = LightningActivityAccessor.getLightningActivitiesByType('Contact', candidateId, filters, 0);
                
        //sort activities
        actList = LightningActivityAccessor.sort(actList,'date','DESC');

        activities = JSON.serialize(actList);
        
    }
    
    @AuraEnabled 
    public static MTX_MatrixWrapper fetchCandidateDetails(String candidateId, String matrixId){

        MTX_MatrixWrapper wrapper = new MTX_MatrixWrapper();

        wrapper.candidateRecord = MTX_MatrixAccessor.getCandidatesById(new List<String>{candidateId})[0];

        wrapper.matrixCandidate = MTX_MatrixAccessor.getMatrixCandidatesById(new List<String>{matrixId})[0];

        wrapper.currentMatrix = String.isNotBlank(wrapper.matrixCandidate.Matrix__c) ? MTX_MatrixAccessor.getMatrixById(wrapper.matrixCandidate.Matrix__c) : new MTX_Matrix__c( Name = Label.MTX_Candidates_Shared_with_Me );

        wrapper.preferredMobile = MTX_MatrixAccessor.getPreferedMobileForCandidate(wrapper.candidateRecord.Master_People__c);

        wrapper.candidates = MTX_MatrixAccessor.getMatrixCandidatesById(new List<String>{matrixId});
        
       List<Matrix_User_Setting__c> userSettings = MTX_MatrixAccessor.getSettingsForCurrentUser(UserInfo.getUserId());

        wrapper.userSettings = userSettings.isEmpty() == false ? userSettings[0] : MTX_MatrixService.createMatrixUserSetting(UserInfo.getUserId());
        /*** XEngine ***/
        XEngineUtils.postMatrixCandidatesEvent(wrapper.candidates, XEngineUtils.MTX_CAND_CANDIDATE_DETAILS);
        /*** XEngine ***/
        
        return wrapper;
    } 
    
    @AuraEnabled 
    public static MTX_Matrix_Note__c getNewMatrixNote(String candidateId){
        
        return new MTX_Matrix_Note__c(Candidate__c = candidateId);
    }
    
    @AuraEnabled 
    public static MTX_Matrix_Note__c saveNote(MTX_Matrix_Note__c note){
        
        return MTX_MatrixService.saveNote(note);
    }
    
    @AuraEnabled 
    public static MTX_Matrix_Note__c getNoteById(Id noteId){
        
/*** XEngine ***/
        XEngineUtils.postMatrixNoteEvent(noteId, XEngineUtils.NOTE_VIEW);
        /*** XEngine ***/

        return MTX_MatrixAccessor.getMatrixNoteById(noteId);
    }
    
    @AuraEnabled 
    public static void deleteSelectedNote(MTX_Matrix_Note__c note){
        
        MTX_MatrixService.deleteCandidateNote(note);
    }

}

Here are my test classes:

@isTest
public class MTX_MatrixDetailViewControllerTest {
    
    
    //create test data 
    @testSetup static void setup() {
        
        //setup FO full users (not inserted yet)
        List<User> matrixUsersToGivePermission = MatrixTestFactory.createMatrixUsers(2);
        
        //give FO full users matrix permission set (will insert the users)
        matrixUsersToGivePermission = MatrixTestFactory.giveMatrixPermission(matrixUsersToGivePermission);
        
        //setup spotlight
        MatrixTestFactory.setupEliseConnection();
        
    }
    
    
    static testMethod void gettingnewNote{
        List<Contact> candidates = MatrixTestFactory.getCandidates(); 
        List<User> matrixUsers = MatrixTestFactory.getMatrixUsers();
        Test.startTest();
        //MTX_Matrix_Note__c nt = new MTX_Matrix_Note__c();
        System.runAs(matrixUsers[0]) {
            MTX_MatrixService.getNewMatrixNote( candidates[0].Id );            
        }
        Test.stopTest();        
    } 
    
    static testMethod void savingtheNote{
        //create a method for for getting notes in MatrixTestFactory
        List<MTX_Matrix_Note__c> notes = MatrixTestFactory.getNotes();
        List<User> matrixUsers = MatrixTestFactory.getMatrixUsers();
        Test.startTest();
        //MTX_Matrix_Note__c nt = new MTX_Matrix_Note__c();
        System.runAs(matrixUsers[0]) {
            MTX_MatrixService.saveNote(notes[0] );            
        }   
        Test.stopTest();
    }
    
    static testMethod void deletingspecificNote{
        List<User> matrixUsers = MatrixTestFactory.getMatrixUsers();
        //create a method for for getting notes in MatrixTestFactory
        List<MTX_Matrix_Note__c> notes = MatrixTestFactory.getNotes();
        Test.startTest();
        //MTX_Matrix_Note__c nt = new MTX_Matrix_Note__c();
        System.runAs(matrixUsers[0]) {
            MTX_MatrixService.deleteCandidateNote(notes[0]);            
        }
        Test.stopTest();
    }
    static testMethod void retrievingNoteiD{
        List<User> matrixUsers = MatrixTestFactory.getMatrixUsers();
        //create a method for for getting notes in MatrixTestFactory
        List<MTX_Matrix_Note__c> notes = MatrixTestFactory.getNotes();
        Test.startTest();
        //MTX_Matrix_Note__c nt = new MTX_Matrix_Note__c();
        System.runAs(matrixUsers[0]) {
            MTX_MatrixService.deleteCandidateNote( notes[0]);            
        }
        Test.stopTest();
    }
    
}

Side note: Matrix Test Factory creates my test data, and when I call  MTX_MatrixService.deleteCandidateNoteMTX_MatrixService.saveNote, MTX_MatrixService.getNewMatrixNote and MTX_MatrixAccessor.getMatrixNoteById, they are defined as

public static MTX_Matrix_Note__c saveNote(MTX_Matrix_Note__c note){

        boolean addNote;
        if(note.Id != null) { addNote = true; } else { addNote = false; }

        upsert note;

        /*** XEngine ***/
        if(!addNote){
            XEngineUtils.postMatrixNoteEvent(note.Id, XEngineUtils.NOTE_ADD);
        } else {
            XEngineUtils.postMatrixNoteEvent(note.Id, XEngineUtils.NOTE_UPDATE);
        }
        /*** XEngine ***/

        return note;
    }

    public static void deleteCandidateNote(MTX_Matrix_Note__c note){

        string matrixNoteJson = XEngineUtils.getMatrixNoteJson(note, XEngineUtils.NOTE_DELETE);

        delete note;

         /*** XEngine ***/
        XEngineUtils.postEventJson(matrixNoteJson);
        /*** XEngine ***/
    }
/*retrieves single matrix note content by note id*/
    public static MTX_Matrix_Note__c getMatrixNoteById(Id noteId){

        List<MTX_Matrix_Note__c> notes = new List<MTX_Matrix_Note__c>();

        notes = [SELECT Id, Note__c, CreatedById, CreatedBy.Name, LastModifiedDate FROM MTX_Matrix_Note__c WHERE Id =: noteId];

        return notes.isEmpty() == false ? notes[0] : null;
    }
```
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Jul 19, 2019 at 16:01

2 Answers 2

4
\$\begingroup\$

Your unit tests contain no assertions about the behavior of your code, so they prove nothing other than that the code does not crash in this specific situation. These are commonly referred to as smoke tests.

There are three critical elements for any unit test, even before you start thinking about the overall coverage of your application's code paths:

  1. Control environment and set up test data.
  2. Execute functionality.
  3. Validate results.

You are only doing (1) and (2) so far.

I'd recommend reviewing Salesforce Stack Exchange's canonical QA, How do I write an Apex unit test?, for more information and resources. In particular, you should definitely complete Unit Testing on the Lightning Platform on Trailhead.

\$\endgroup\$
5
  • \$\begingroup\$ Wouldn't this question be better migrated to Code Review? It's not really on topic here, per se. \$\endgroup\$ May 23, 2019 at 15:01
  • \$\begingroup\$ Good call, @AdrianLarson - thanks for getting that. \$\endgroup\$
    – David Reed
    May 23, 2019 at 15:12
  • \$\begingroup\$ @AdrianLarson I humbly think we should leave it in Salesforce. We're starting to see code review requests in the community, and apex unit testing is specific to SF. Also, a lot of people in the community won't monitor this forum as much as the salesforce forum so posters won't get as much help, and other people won't benefit from our posts \$\endgroup\$ May 23, 2019 at 15:13
  • \$\begingroup\$ I disagree, and we've had some back-and-forth in the Meta about this. So far, the community has been pretty consistent in deciding that code review is off topic for our SE. We can certainly open it up for further discussion. \$\endgroup\$ May 23, 2019 at 15:19
  • \$\begingroup\$ Thanks for all the feedback, I decided that I am going to add something like System.assert(result.isSuccess());, to determine if the note actually deleted or saved \$\endgroup\$
    – Deja Bond
    May 23, 2019 at 18:36
2
\$\begingroup\$

Here are some code comments for you

For your constructor here are some changes I'd suggest and the reasoning behind them

public transient String activities {get;set;}
public static final String CONTACT = 'Contact';
public MTX_MatrixDetailViewController(){

    String candidateId =  ApexPages.currentPage().getParameters().get('id'); // change this type to an Id not a String.        
    //change to this and delete the filters declaration. Also, check if candidateId is not null.. maybe the id was not passed 
    if (String.isNotBlank(candidateId)){
        List<LightningActivity> actList = LightningActivityAccessor.getLightningActivitiesByType(
            CONTACT, 
            candidateId,  
            new Map<String, String>() , 
            0); 

        //sort activities
        actList = LightningActivityAccessor.sort(actList,'date','DESC');
        activities = JSON.serialize(actList);

    }
}

Here are some comments on the fetchCandidateDetails. Note, that I'm suggesting a couple of things - error checking and some stylistic stuff. This looks great otherwise!

@AuraEnabled 
public static MTX_MatrixWrapper fetchCandidateDetails(String candidateId, String matrixId){

    // you ought to check to make sure this data is valid. If you're doing this in the caller already, you can disregard.
    if(String.isBlank(candidateId) || String.isBlank(matrixId)){
        throw new MTX_Exception('Insuffinent Canditate Details'); 
    }

    MTX_MatrixWrapper wrapper = new MTX_MatrixWrapper();

    wrapper.candidateRecord = MTX_MatrixAccessor.getCandidatesById(new List<String>{candidateId})[0];

    wrapper.matrixCandidate = MTX_MatrixAccessor.getMatrixCandidatesById(new List<String>{matrixId})[0];

    wrapper.currentMatrix = String.isNotBlank(wrapper.matrixCandidate.Matrix__c) ? 
        MTX_MatrixAccessor.getMatrixById(wrapper.matrixCandidate.Matrix__c) : 
        new MTX_Matrix__c( Name = Label.MTX_Candidates_Shared_with_Me );  // moved to different lines. easier to read

    wrapper.preferredMobile = MTX_MatrixAccessor.getPreferedMobileForCandidate(wrapper.candidateRecord.Master_People__c);

    wrapper.candidates = MTX_MatrixAccessor.getMatrixCandidatesById(new List<String>{matrixId});

   List<Matrix_User_Setting__c> userSettings = MTX_MatrixAccessor.getSettingsForCurrentUser(UserInfo.getUserId());

    wrapper.userSettings = userSettings.isEmpty() == false ? 
        userSettings[0] : 
        MTX_MatrixService.createMatrixUserSetting(UserInfo.getUserId());  // moved to different lines. easier to read

    /*** XEngine ***/
    XEngineUtils.postMatrixCandidatesEvent(wrapper.candidates, XEngineUtils.MTX_CAND_CANDIDATE_DETAILS);
    /*** XEngine ***/

    return wrapper;
} 

The rest of your code looks good. Your Test code has some concerns. You're running tests for code coverage but you're not asserting anything. Also, you're only testing the happy path from what I gather.

For assertions - assert behavior that you expect to happen for your test set up.

For happy paths, test for other paths - negative, bulk, and boundary...So, for example you have a method getNewMatrixNote(candidateId) and your test case method is gettingnewNote(). What if I had a candidateId that was bad? That's why you need to have negative and boundary tests.

\$\endgroup\$
1
  • \$\begingroup\$ Hi @Ohana yes, I realized that the assertions would be the validation and that I was missing it. I'll do some research on positive and negative test, trailhead has some units on it. \$\endgroup\$
    – Deja Bond
    May 23, 2019 at 18:24

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.