Close recruitment optimization #126
Conversation
Adding comments
Remove comments
Batch job to move campaign members to new campaign.
Test Classes for BZ_CloseRecruitmentBatchJob and BZ_CloseRecruitmentSchedule
Updated test class for BZ_CloseRecrutimentController_TEST
Added new fields to VF page to populate fields in the new campaign.
Code changes
Code changes
Code changes
| /** | ||
| * Handles BZ_CloseRecruitmentController actions of the campaign related CampaignMembers processing for large records. | ||
| */ | ||
| global class BZ_CloseRecruitmentBatchJob implements Database.Batchable<sObject> { |
There was a problem hiding this comment.
Why does this have to be global? Isn't public sufficient?
There was a problem hiding this comment.
Standard syntax for a batch class use to be global but I have updated the access modifier to public.
| * Handles BZ_CloseRecruitmentController actions of the campaign related CampaignMembers processing for large records. | ||
| */ | ||
| global class BZ_CloseRecruitmentBatchJob implements Database.Batchable<sObject> { | ||
| global final String Query; |
There was a problem hiding this comment.
If we declare the variable Query global, doesn't that mean everywhere in all Apex code across the entire org this can only exist once? That seems messy. Shouldn't we declare the access level to be the minimum amount possible? E.g. if the Query variable only needs to be accessed inside the BZ_CloseRecruitmentBatchJob, then it should be private.
| * Handles BZ_CloseRecruitmentController actions of the campaign related CampaignMembers processing for large records. | ||
| */ | ||
| global class BZ_CloseRecruitmentBatchJob implements Database.Batchable<sObject> { | ||
| global final String Query; |
There was a problem hiding this comment.
Also, why is this capitalized? The general convention we use in the code base is that classes are capitalized, all class member variables start with m_ and are private, if we need to expose a class member publicly, we use a property with a get; set;
| * Used to collect the records or objects to be passed to the interface method execute for processing. | ||
| */ | ||
| global Database.QueryLocator start(Database.BatchableContext batchableContext){ | ||
| return Database.getQueryLocator(query); |
There was a problem hiding this comment.
Where is the query parameter passed to getQueryLocator(query) defined? Is this the same Query variable set in the constructor? If so, let's be consistent with the case of the variables.
There was a problem hiding this comment.
It is referring to the same variable as apex code is not case sensitive. Changes have been made to be consistent.
|
|
||
| //Check if campaign members List is not null and size is greater than zero | ||
| if(campaigns!=null && campaigns.size()>0){ | ||
| system.debug('campaigns list--'+campaigns.size()); |
There was a problem hiding this comment.
For all system.debug() calls, can we prefix the call with the name of the class and method it was logged from? Have a look at all the other places we use system.debug() in the code. This example would be BZ_CloseRecruitmentBatchJob.execute(): 'campaigns list size='+campaigns.size()
|
Generally, the extensions we use are |
| } | ||
| System.Debug('BZ_CloseRecruitmentDatabaseChangesQueueable.execute(): end'); | ||
| catch(Exception ex) |
There was a problem hiding this comment.
Why catch and throw? Why not just let it bubble up?
There was a problem hiding this comment.
What do you mean by bubble up?
| try{ | ||
| if(m_campaign!=null) | ||
| { | ||
| // Campaign Details Section |
There was a problem hiding this comment.
This is not nearly as maintainable as the prior code. Before, when we used the BZ_ClonePlusController it dynamically copied all Tasks/Activities/CampaignMembers/Fields. Now, whenever we add a new field to a Campaign that we want preserved, we have to update this code. If copying over every field manually is the only way we can do this with the new approach, we need a way to detect when a new field is added to the Campaign but not added to this section. Maybe add a test that looks at all the fields on a Campaign Member, excludes any we know we don't care about, and then tests that all those left are copied over after CloseRecruitement? That way, if we add a new field and forget to add a line here, the test fails unless we either copy it or explicitly exclude it.
There was a problem hiding this comment.
I have made major changes to this part of code. BZ_ClonePlusController is being used to dynamically copy only the campaign.
| Boolean foundNoTimeTask = false; | ||
| Boolean foundWaitlistReinviteTask = false; | ||
| Boolean foundNoTimeReinviteTask = false; | ||
| for (Task t : tasksNotPurged) |
There was a problem hiding this comment.
What happened to all the tests around the proper Tasks/Activities being copied over?
There was a problem hiding this comment.
This functionality is tested in BZ_CloseRecruitmentControllerBatchJob_Test.
| campaign.EndDate=Date.today(); | ||
| insert campaign; | ||
| // insert 100 CampaignMembers | ||
| List<CampaignMember> campaignMembersToInsert = new List<CampaignMember>(); |
There was a problem hiding this comment.
I think a more maintainable way to handle this is to get the list of CampaignMembers that should be copied over to the new Campaign and running ClonePlusController on each of them to clone their Activities/Tasks/Fields and then put them in the new campaign.
Added two methods to clone a set of objectIds
The following are the changes made: