CRM-21198 Update membership status appropriately when adding a payment #11006
Conversation
eileenmcnaughton
changed the title
eileenmcnaughton
force-pushed
the
cont
branch
from
cf42873 to
f318b19
Compare
eileenmcnaughton
force-pushed
the
cont
branch
from
f318b19 to
04f719d
Compare
|
Jenkins re test this please |
1 similar comment
|
Jenkins re test this please |
|
just noting that i have had to stop the Jenkins Job twice because it seems to get into a loop |
|
Ok, I am taking this PR form here. |
|
@eileenmcnaughton I have updated the PR, can you please check now? |
eileenmcnaughton
force-pushed
the
cont
branch
from
824b194 to
03fe69d
Compare
|
I just fixed the style error & @seamuslee001 I tried re-running tests locally to see if it would loop but both |
|
@eileenmcnaughton I think what @seamuslee001 have earlier reported was due to wrong order of |
…t completes the contribution
eileenmcnaughton
force-pushed
the
cont
branch
from
03fe69d to
aec171f
Compare
|
@monishdeb I've squashed it all in now. I also brought across the test & commented out the lines that don't pass (relate to the other fixes). I was able to see the test fail when the membership type was not being updated and pass with this patch when it was being updated |
| @@ -76,8 +76,8 @@ public function tearDown() { | |||
| public function testInvoiceSentOnIPNPaymentSuccess() { | |||
| $this->enableTaxAndInvoicing(); | |||
|
|
|||
| $pendingStatusID = CRM_Core_OptionGroup::getValue('contribution_status', 'Pending', 'name'); | |||
| $completedStatusID = CRM_Core_OptionGroup::getValue('contribution_status', 'Completed', 'name'); | |||
| $pendingStatusID = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Pending'); | |||
There was a problem hiding this comment.
not sure why this was not failing the tests on jenkins. It did locally
| $dao->free(); | ||
|
|
||
| // Unclear why this is here but this function is overloaded by repeattransaction. | ||
| if ($contributionStatus === 'Pending') { |
There was a problem hiding this comment.
simplification of this IF is the only thing changed in the extraction
eileenmcnaughton
changed the title
|
Hopefully it will pass this time 😄👍 |
|
@colemanw can you review and merge this PR? |
|
@colemanw for clarity - in the review process I reworked the PR & then @monishdeb added to that & we collaborated in the review process to get to this version - hence I can no longer merge the result of the review process. |
|
Looks good. Thanks for all your hard work Eileen & Monish 🥇 |
|
|
||
| unset($dates['end_date']); | ||
| $membershipParams['status_id'] = CRM_Utils_Array::value('id', $calcStatus, 'New'); | ||
| //we might be renewing membership, |
There was a problem hiding this comment.
So for a renewing member, their status will change from pending to current once the partial payment is completed. Correct?
There was a problem hiding this comment.
Yes... please test...
There was a problem hiding this comment.
@eileenmcnaughton Yes it works as expected.
Overview
Per #10999 this updates a membership when a payment is added to the contribution, leading to the contribution being completed.
Before
Adding a payment to a partially paid contribution with a membership will cause the membership to be completed but the membership will remain pending. Screencast:
After
The membership will be updated to 'new' (as appropriate) when the contribution is completed via the record payment form. This is happening in the BAO so the (underused) payment api would also be fixed in this scenario. Screencast:
Technical Details
This is primarily an extraction of code from completeOrder which is then also used for this path. Hopefully, over time we will consolidate further but we have been extracting & tidying up completeOrder & adding testing since 4.2 so it is not straightforward
Comments
@monishdeb -this is now a replacement for the previous PR - it holds the collaborated changes + the test with the parts that relate to not-yet-reviewed fixes commented out