CRM-21183: Updating Partially paid contribution to Completed doesn't …

[jitendrapurohit]

…update membership

Overview

Update membership when contribution is updated from partially paid to completed.

Before

Membership is unaffected when related contribution is updated from partially paid to completed.

After

Membership is updated to New or its calculated status in above case.

Technical Details

Current functionality handles the contribution status switch between Pending -> Completed. It should also consider Partially paid status.

Comments

Unit test added.

---

• CRM-21183: Updating Partially paid contribution to Completed doesn't update membership

[pradpnayak]

how about (!in_array($contributionStatuses[$previousContriStatusId], array('Pending', 'Partially paid'))) ??

[jitendrapurohit]

Done 👍

[pradpnayak]

Can use labels??

[pradpnayak]

Can use labels??

[eileenmcnaughton]

Stylistically I think it's better to resolve $previousContributionStatus early on when we are looking at it more than& compare that - ie

$previousContributionStatus = CRM_Core_Pseudoconstant:::getName('CRM_Contribution_BAO_Contribution', 'contribution_status_id', $contributionStatuses[$previousContriStatusId]);

& From then on

if ($previousContributionStatus === 'Pending')

etc

Overall I think this can be merged once @pradpnayak OKs it -

[pradpnayak]

I will QA and post my result by tomorrow.

[eileenmcnaughton]

thanks

[eileenmcnaughton]

I would note there is additional discussion about this PR on JIRA

[pradpnayak]

The patch works fine. Membership status is changed to 'New' when contribution status is changed from Partially Paid to Completed. Here is what i did

Test case 1: UI

Step1. Created offline pending pay later Membership

Step2: Changed status of Contribution from Pending(pay later) to Partially paid

Step3: Changed status of Contribution from Partially paid Completed.

However there was no payment recorded for this contribution as you can see in the attached image under step 3.

[pradpnayak]

Test case 2:

1. Created offline membership with pending pay later membership (using UI)
2. changed the contribution status from Pending(pay later) to Partially paid (using UI)
3. Changed the contribution status from Partially Paid to Completed using Contribution api.
   $result = civicrm_api3('Contribution', 'create', array( 'sequential' => 1, 'id' => 7, 'contribution_status_id' => "Completed", ));
   Result:
   Contribution status was changed to Completed but Membership was still in Pending status.

[jitendrapurohit]

Thanks for the review comments @pradpnayak. Following my comment on the issue, API's too doesn't support the update of membership with status, even with Pending. Confirming that it isn't a regression but a doubtful workflow from previous versions(4.6 etc).

Not sure what the exact behaviour should be in case of API, but I've seen places which use completetransaction for the same.

Re this PR - it only extends the existing functionality which was working for Pending but not for Partially paid.

[eileenmcnaughton]

@pradpnayak I'm having trouble interpreting your review. You say "The patch works fine" but then identify some issues. Is this

• a partial fix
• a fix that causes new issues
• a fix for the immediate issue that has highlighted other issues.

If this is fixing something & not making other things worse I lean towards merge as it locks in the fix with a unit test. However, I don't want to merge something that is a step backwards

[pradpnayak]

@eileenmcnaughton The patch works fine when contribution status is changed from UI, membership status is changed correctly. However when we change the status of contribution using Contribution api the status of membership is not changed i.e the patch works as expected when status change done through UI form but not through api call.

The other issue i noticed during the test was no payment was recorded when status was changed to Completed from Partially paid. As @JoeMurray mentioned in issue that we need to stop this type of transitions i.e changing Partially paid contribution to completed directly. But i believe you or @monishdeb will be covering this fix in #10776 and forcing user to record payment for contribution status like Pending, Partially paid, Pending refund rather changing the status directly.

[eileenmcnaughton]

@pradpnayak so does that mean this is a partial fix & should be merged on that basis?

(I agree that the completetransaction api is IMHO the only reliable way to change the status of a contribution to Completed)

[pradpnayak]

Yes agreed, this is safe to merge.

[eileenmcnaughton]

thanks!

[pradpnayak]

:)