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

Update Next Recurring Donation Installment Amount #7086

Closed
wants to merge 100 commits into from

Conversation

lparrott
Copy link
Contributor

@lparrott lparrott commented Aug 22, 2022

W-11556857
W-11556798

  • We've added back-end methods and a UI to update the Amount of the next Installment of a Recurring Donation
  • Test steps can be found in the Design Doc

Critical Changes

Changes

Issues Closed

Community Ideas Delivered

Features Intended for Future Release

Features for Elevate Customers

New Metadata

Deleted Metadata

* @return Boolean True if the Next Payment Date has changed on an Elevate connected RD
*/
public Boolean needsNextPaymentScheduleCheck() {
return changedFields.contains(npe03__Recurring_Donation__c.npe03__Next_Payment_Date__c)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since RD2 disables the CRLP for this field, I think this is a safe way to "know" that a Payment has been Paid?

@@ -267,8 +267,11 @@ public inherited sharing class PS_CommitmentRequest {
: UserInfo.getDefaultCurrency();

if (isElevateScheduleFieldsChanged(rd, oldRd)) {
// If the schedule has a Next Payment Amount, we need to send it
List<RecurringDonationSchedule__c> rdSchedules = scheduleService.getExistingSchedules(rd);
Decimal nextPaymentAmount = rdSchedules.size() > 0 ? rdSchedules[0].NextPaymentAmount__c : null;
Copy link
Contributor

@npsp-reedestockton npsp-reedestockton Aug 31, 2022

Choose a reason for hiding this comment

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

I'd be happier if we didn't have any implementation details in this class. Maybe getNextPaymentAmount in RD2_ScheduleService??

Also, is it safe to assume the amount is on the first active schedule? Will it be on both schedules for a 1st and 15th cadence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize 1st and 15th used 2 Active schedules, I'll double check that they're in the correct sort order. I can create that method, good suggestion.

@@ -95,6 +106,9 @@ public without sharing class RD2_ScheduleService {
schedule.Campaign__c = rd.npe03__Recurring_Donation_Campaign__c;

schedule.InstallmentAmount__c = rd.npe03__Amount__c;
if(nextPaymentAmount != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we end up with nextPaymentAmount on both schedules for a 1st and 15th cadence. Is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, we should only set it on the schedule that represents the next payment, I'll update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, this shouldn't hurt anything as long as we clear both of them after the one-time payment is paid. Makes the logic a bit cleaner, no need to figure out which schedule to set 🤷

@@ -187,6 +203,7 @@ public without sharing class RD2_ScheduleService {
List<RecurringDonationSchedule__c> newSchedules = buildNewSchedules(rd);
for (RecurringDonationSchedule__c newSchedule : newSchedules) {
newSchedule.StartDate__c = newStartDate;
newSchedule.NextPaymentAmount__c = nextPaymentAmount;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for putting nextPaymentAmount on more than one schedule record?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, in my testing this was always a single schedule, but I didn't account for 1st and 15th. Are there other scenarios where there will be more than 1 new schedule created?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that right now that's the only time we create more than one.

There can be multiple active schedules: 2 current if it's a 1st and 15th schedule + 2 future if the future schedule is a 1st and 15th + pause schedule. Currently, five active would be the cap I think, but there's no structural reason we would be prohibited from having more future schedules or more current schedules. Unlikely we'll do either of those things, but you never know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm refactoring this a bit, but I'm still going to set NextPaymentAmount on all new Schedules. It shouldn't hurt anything if we clear them all after that Payment is paid.

…pdateInstallment

# Conflicts:
#	force-app/main/default/classes/RD2_EntryFormController.cls
* @param nextPaymentAmount The Next Payment Amount to set on the Schedule
* @return UTIL_Http.Response Payments API response
*/
public UTIL_Http.Response handleNextPaymentAmount(npe03__Recurring_Donation__c rd, Decimal nextPaymentAmount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning to reuse the same controller class as the RD2 Entry form or this is just a placeholder until the new UI/Controller is created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking I'd use the same Class since it has everything we need. Technically it's a type of RD Entry?

@@ -445,16 +447,27 @@ public inherited sharing class RD2_OpportunityEvaluationService {
}
else {
if (isNewOpportunityCandidate(rdRecord)) {
// Prevent the Next Payment Amount from being used in later steps of this process
rd.RecurringDonationSchedules__r[0].NextPaymentAmount__c = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this chunk of code be unnecessary because NextPaymentAmount__c will not be set on new RD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually where I'm clearing NextPaymentAmount for non-Elevate RDs. My assumption is that if isNewOpportunityCandidate is true, a Payment has been made, which should have used the NextPaymentAmount, meaning we can now clear it.

*/
public Boolean needsNextPaymentScheduleCheck() {
return changedFields.contains(npe03__Recurring_Donation__c.npe03__Next_Payment_Date__c)
&& isElevateRecord();
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 we aren't clearing the next payment amount for non-elevate RD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since isNewOpportunityCandidate won't happen for Elevate RDs, I needed a different way to clear them. I'm unsure if this will work though, still need to do E2E testing.

Copy link
Contributor

@andrewyu-salesforce andrewyu-salesforce left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@npsp-reedestockton npsp-reedestockton left a comment

Choose a reason for hiding this comment

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

Looks good!

@lparrott lparrott deleted the branch feature/242 December 4, 2024 20:35
@lparrott lparrott closed this Dec 4, 2024
@lparrott lparrott deleted the feature/242__updateInstallment branch December 4, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants