Use order api when creating a recurring membership from the Membershi… #20077
Conversation
|
(Standard links)
|
fef238d to
ca52f33
Compare
|
Closing for now CRM_Member_Form_MembershipTest::testSubmitRecur /home/jenkins/bknix-dfl/build/core-20077-ogly/web/sites/all/modules/civicrm/api/api.php:134 |
999ec2c to
626b5f7
Compare
|
@monishdeb @kcristiano this is the PR that actually switches us over to using the Order api for the membership create (specifically in the recurring flow) - it also enables us to add one of the particularly weird chunks of code in the Membership BAO With this done I can finally fix membership bao create to not use $ids & finish that part of the clean up & then figure out the next part that needs to be done |
|
@monishdeb @kcristiano are either of you able to find time to review / test this one? |
|
I guess I could put up a sub-pr to simplify it more |
|
@eileenmcnaughton I have taken a walk through it, I have built a tarball with the PR and deployed on test sites. So far so good. I do want to push on the recurring and a good |
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
|
@kcristiano yes - I built up the test cover in advance of doing the refactor! |
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
This is intended to simplify civicrm#20077 by switching to using functions to retrieve membership (as an array) and membershipID rather than passing around variables
|
@kcristiano I separated out #20153 - which is more 'just a refactor' - and once merged the actual changes in this should be pretty readable. I'll rebase this over that if it won't mess up your testing (ie rebase it into this PR & out again once merged) - but not if it will invalidate testing to date |
|
.... oops wrong thread
|
626b5f7 to
c01e303
Compare
| $memInfo = array_merge($params, ['membership_id' => $membership->id]); | ||
| $params['contribution'] = self::recordMembershipContribution($memInfo); | ||
| } | ||
|
|
||
| // Add/update MembershipPayment record for this membership if it is a related contribution | ||
| // @todo remove this - called from one remaining place in CRM_Member_Form_Membership | ||
| if (!empty($params['relate_contribution_id'])) { |
There was a problem hiding this comment.
Everything to do with 'relate_contribution_id' is the weird magic that made it work despite doing things all out of order - getting rid of this sticky tape is 'the goal'
c01e303 to
c2fb6c1
Compare
…p backoffice form This removes the need for some 'magic' code from the membership BAO that was really only there to support the fact that this code was doing some particularly convoluted manoevering in order to share code with the front end form (since unshared). Not this adds a feature Matt requested - the created membership id is returned. This is only done for memberships at the moment but could be other entities too as test cover is added (the membership tests fail without the change in this PR so it has cover in the context it is added
c2fb6c1 to
45a6ec4
Compare
|
@monishdeb please make this a priority. @eileenmcnaughton this is a very big achievement to get this PR together! Thanks. Just to confirm the scope for run testing: we should be able to create and renew memberships with recurring payments on the backend. Presumably different types of payment processors from IPN callback ones to EFT ones should all work. |
|
This PR only touches new membership and this code change focusses on new recurring payment processor memberships (recurring is not available on this form if not a payment processor membership) Note that I think the logic could be fairly easily extended to all payment processor payments on this form (new membership) - although I had thought to include that in the following release |
|
I have tested this patch in my local, and it works fine. Tested with auto-renewal membership in live mode and the financial entries were recorded correctly. Doesn't seem to cause any breakage/regression. The Order.create API change make sense. Checked the related unit-test coverage and passed on local. Overall Developer standards |
|
Thanks @monishdeb - yay |
Overview
Use order api + payment api when creating a recurring membership from the Membership back office form
Before
Layers of cludges to get it to do the right thing - there is a scary thing going on where the form passes the BAO a value to tell it to create the membership payment records - but only for the first membership (because the first one creates them all & it crashes if you do it for the second)
https://github.com/civicrm/civicrm-core/pull/20077/files#diff-7bf0a482fe0659604e1fc7d265db9eb60b0fcc0e24557f19a68747d402393fd2L352-L362
After
The flow we have been recommending is actually used - albeit narrowly - for card+recurring cases only
Order.create
Payment.create
Not this adds a feature Matt previously requested - ie the created membership id is returned from the order api. This
is only done for memberships at the moment in order to manage the scope but would be the pattern for other entities too as test cover is added (the pre-existing tests do specifically cover this in the context of this change - ie I added it to make the tests pass)
Technical Details
This removes the need for some 'magic' code from the membership BAO that was really only
there to support the fact that this code was doing some particularly convoluted
manoevering in order to share code with the front end form (since unshared).
Comments