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

Fix re-calculation of payment dates on pledge #19976

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

jitendrapurohit
Copy link
Contributor

Overview

Fix re-calculation of payment dates on pledge

Before

Payment schedule on the pledge is not recalculated when the start date is changed on a pending pledge. To replicate -

  • Create a pledge of $1200 for 12 installments to start on 4th April, 2021.
  • The pledge payments will be created for 4th April, 4 May, 4 June...
  • Update the pledge start date to 1st May (instead of 4th April) => Save.
  • The pledge payment dates are unchanged and are still starting from 4 April, 4 May....

After

If the pledge start date is changed to 1st May, the payment dates are recalculated and are updated to start fro 1st May, 1st June, 1st July....and so on.

Technical Details

This PR partially reverts #19297.

isPaymentsRequireRecalculation() checks the existing db values of the pledge and returns true only if it mismatches the filled values.

19297 moved the call AFTER the pledge is saved. So it is now comparing the "updated" values with the passed params which will always match and hence no recalculation is done in any case.

Comments

@eileenmcnaughton thoughts?

@civibot
Copy link

civibot bot commented Apr 6, 2021

(Standard links)

@civibot civibot bot added the master label Apr 6, 2021
@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit your analysis makes sense - let's see how jenkins feels about it

@megaphonetech-dennis
Copy link
Contributor

megaphonetech-dennis commented Apr 6, 2021

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: I replicated the issue as described by the author and confirmed the buggy behavior. I downloaded and applied the patch. I then created a few additional pledges, made edits to those pledges, and reviewed the payment schedules. Everything appeared correct.

@eileenmcnaughton
Copy link
Contributor

Thank you @megaphonetech-dennis for the review - I'm happy to merge based on your analysis.

Thanks for the patch @jitendrapurohit

@eileenmcnaughton
Copy link
Contributor

This should really have gone against the rc to be in 5.36 #19990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants