Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recurring Donations Batch Filtering #6971

Merged
merged 44 commits into from
Jul 12, 2022

Conversation

lparrott
Copy link
Contributor

@lparrott lparrott commented Jun 3, 2022

Critical Changes

Changes

  • We added new settings that enable filtering of the Recurring Donation Batch jobs, based on the number of days since the child Opportunities were modified.

Issues Closed

Community Ideas Delivered

Features Intended for Future Release

Features for Elevate Customers

New Metadata

Deleted Metadata

Integer dateRange = getOpportunityModifiedDateRange();
Date earliestModifiedDate = currentDate.addDays(-dateRange);
rdFilterWhereClause = 'Id IN (SELECT npe03__Recurring_Donation__c'
+ 'FROM Opportunity WHERE LastModifiedDate > :earliestModifiedDate)';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lparrott - any chance we can switch this to use SystemModStamp instead of LastModifiedDate since SystemModStamp is indexed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I have a list of questions to ask you and JJ, one revolves around potentially reusing the "Incremental Mode Field Override" setting for this query, which defaults to SystemModStamp. One downside: since there's only one field, it would be used for all incremental batches.

@lparrott lparrott changed the title Recurring Donations Batch Filtering based on Opportunity Modified Date Recurring Donations Batch Filtering Jun 15, 2022
Copy link
Contributor

@force2b force2b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work on this. Mostly some small comments and suggestions

Copy link
Contributor

@force2b force2b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the previous set of changes. Two comments/questions related to the custom settings fields and possible null values. I'm pretty sure I remember that null custom settings fields - mainly after adding new fields - used to cause null pointer exceptions for existing customers. It's been a while, but I think this might still be true. Some form of null check (and/or the new null-safe operator) might be useful.

force-app/main/default/classes/CRLP_Batch_Base.cls Outdated Show resolved Hide resolved
force-app/main/default/classes/CRLP_Batch_Base.cls Outdated Show resolved Hide resolved
@lparrott lparrott requested a review from force2b June 22, 2022 14:56
Copy link
Contributor

@force2b force2b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@daniel-fuller
Copy link
Contributor

@npsp-reedestockton @force2b I had to commit a change on this PR to resolve a query exception that was occurring in the start() method of the batch due to bind variable scoping. Wanted to get a review on the commit. 397f6b4

@daniel-fuller daniel-fuller self-assigned this Jul 5, 2022
@@ -121,6 +121,7 @@ public with sharing class STG_PanelRDBatch_CTRL extends STG_Panel {
if (isRD2Enabled) {
RD2_OpportunityEvaluation_BATCH rdBatchJob = new RD2_OpportunityEvaluation_BATCH();
Integer batchSize = rdBatchJob.batchSize;
// rdBatchJob.closedFilterEnabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this line is commented out vs. just removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for 2GP testing since we can't execute the batch from anonymous apex and only from NPSP Settings. I plan to uncomment this once 2GP testing is completed. We didn't want the batch filter to apply when running it from NPSP Settings which is why we are setting to false here.

<?xml version="1.0" encoding="UTF-8"?>
<CustomField xmlns="http://soap.sforce.com/2006/04/metadata">
<fullName>RecurringDonationLastNDays__c</fullName>
<defaultValue>31</defaultValue>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure 31 is a good default value for this. Most RDs are monthly, and If a customer is using Enhanced RDs there will usually be one Opportunity that has been updated in the last 31 days. Most of the RDs that will be bypassed with this default will be closed RDs. If that's the intent, cool, otherwise this is too high for the default incremental mode value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielfuller0814, please let me know if you change the Default Value from 31. Thanks!

Copy link
Contributor

@ErinWiedemer ErinWiedemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielfuller0814 This looks good to me. Just let me know if you decide to change the default value of the RD Modified in Last Number of Days field.

@daniel-fuller
Copy link
Contributor

@ErinWiedemer we decided to keep the default value at 31

@daniel-fuller daniel-fuller merged commit 4dbbb20 into feature/240 Jul 12, 2022
@daniel-fuller daniel-fuller deleted the feature/240__rdBatchFiltering branch July 12, 2022 21:43
@salesforce-org-metaci salesforce-org-metaci bot mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants