Extract out remaining usage of MembershipPayment #32222
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
mattwire
force-pushed
the
membership
branch
5 times, most recently
from
f30b24c to
7b735e9
Compare
| @@ -101,7 +101,7 @@ class CRM_Batch_Form_Entry extends CRM_Core_Form { | |||
| */ | |||
| public function getCurrentRowContributionID(): int { | |||
| if (!isset($this->currentRowContributionID)) { | |||
| $this->currentRowContributionID = CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipPayment', $this->getCurrentRowMembershipID(), 'contribution_id', 'membership_id'); | |||
| $this->currentRowContributionID = CRM_Member_BAO_MembershipPayment::getLatestContributionIDFromLineitemAndFallbackToMembershipPayment($this->getCurrentRowMembershipID()); | |||
There was a problem hiding this comment.
Oh wow - I think we could ditch the fallback & just use the line item - this is a slightly obscure screen & there are enough other places that rely on people having good data re line item entity_id that I don't think this is the place we need to bend over backwards for bad data
There was a problem hiding this comment.
Yeah, we probably could. Except that this getFieldValue logic pops up in various places and causes obscure bugs. I've come across sites missing lineitems and sites missing MembershipPayment records and a mixture of both... so I feel like the safer option is to extract out this logic and leave the fallback in initially. We might add some kind of upgrader/check before we can completely remove the MembershipPayment fallback. See #32244 for the extraction of just this function for easier review!
| 'contribution_id' => $membershipContribution->id, | ||
| ]; | ||
| civicrm_api3('MembershipPayment', 'create', $membershipPaymentParams); | ||
| CRM_Member_BAO_MembershipPayment::legacyMembershipPaymentCreate($membership->id, $membershipContribution->id); |
There was a problem hiding this comment.
I wonder if we could check if this does anything & maybe remove it. I'm pretty sure creating the line item does this
There was a problem hiding this comment.
Yeah, the extracted function is only called in one place. But my thinking is that it was easier to extract everything related to MembershipPayment first and then it should be easier to justify just removing stuff. Eg. if getLatestContributionIDFromLineitemAndFallbackToMembershipPayment() function is used everywhere we can probably justify just removing this call to MembershipPayment.create on the basis that we don't need the MembershipPayment record to be created at all as we can verify that a Membership LineItem is created and we know that the LineItem will be checked first.
…first and only fallback to MembershipPayment if not found via lineItem
…ution via lineItem first
Overview
MembershipPaymententity is used to map Contributions to Memberships. But it is deprecated in favour of LineItems which provide the same information but in a more flexible manner.However, we still have various bits of logic in CiviCRM either creating or relying on
MembershipPaymentrecords existing so we end up with some bizarre bugs and inconsistencies that are difficult to debug.This PR takes the approach of extracting all MembershipPayment interaction out into functions in the
CRM_Member_BAO_MembershipPaymentclass which gives us the ability to "turn off" all interaction with theMembershipPaymentrecords or make consistent changes through the whole system.Eg.
CRM_Member_BAO_MembershipPayment::getLatestContributionIDFromLineitemAndFallbackToMembershipPayment()(split out into #32244) takes over all the places were we were usinggetFieldValue()to get a Contribution ID from a membership ID - see the PR for details. The logic was flawed previously. With this change it makes it really easy in future to simply turn off checking the MembershipPayment records.Before
Various bits of MembershipPayment record logic in various places in the code.
After
All bits of MembershipPayment record extracted out to functions in
CRM_Member_BAO_MembershipPayment. It's now a much simpler task to identify if they are actually required and to further deprecate/remove them.Technical Details
Explained above.
Comments