WIP Minor cleanup of Membership/Renewal form classes #12651
Conversation
|
(Standard links)
|
|
@mattwire I don't really trust myself to review a commit like the second one without missing something :-( We've had a few things missed on refactors of late so I'd rather break it down |
CRM/Member/Form/Membership.php
Outdated
| @@ -1496,16 +1497,15 @@ public function submit() { | |||
| $params['trxn_id'] = CRM_Utils_Array::value('trxn_id', $result); | |||
| $params['is_test'] = ($this->_mode == 'live') ? 0 : 1; | |||
| if (!empty($formValues['send_receipt'])) { | |||
| $params['receipt_date'] = $now; | |||
| $params['receipt_date'] = $params['receive_date']; | |||
There was a problem hiding this comment.
@mattwire I went through the changes in this file (only) on top of #12642 which changes the same file & this line is the only one in this file I have a reservation about - I think it should be date('YmdHis'); which is less confusing.
I can pull the changes you have made to this file into #12642 if you want to review that & then I'll try to make sense of the changes in MembershipRenewal. The early ones seem pretty straightforward but the function extraction should probably stand alone
There was a problem hiding this comment.
@eileenmcnaughton I've reviewd #12642 When it is merged I'll update this PR
| // a function this form shared with other forms. | ||
| $membershipSource = NULL; | ||
| if (!empty($this->_params['membership_source'])) { | ||
| $membershipSource = $this->_params['membership_source']; | ||
| } | ||
|
|
||
| $isPending = ($this->_params['contribution_status_id'] == 2) ? TRUE : FALSE; | ||
| $isPending = ($this->_params['contribution_status_id'] == $pendingContributionStatusId) ? TRUE : FALSE; |
There was a problem hiding this comment.
@mattwire the other is merged & this can be rebased. I've checked all the changes down to this line so far & am OK with them so if you pull those into one PR I can merge that quickly
|
@mattwire I see now that you have refactored emailReceipt into it's own function (which is good). You have made it static which I think is in order to sync with the Membership. But, I'd like to propose that the function in Membership is pretty dire. It was obviously made static to share code with the CRM_Batch_Form_Entry class - which also calls it. But I would say 2/3 of the code in that function is not genuinely shared. So, instead of copying that approach what about we start to defang that approach - eg. Which then lends itself to moving routines used by the 2 membership classes but not by the BatchEntry class outside of that function - e.g f73ad85 |
|
|
||
| list($renewMembership) = CRM_Member_BAO_Membership::processMembership( |
There was a problem hiding this comment.
This variable renaming is causing a lot of noise & would be better done before or after the other changes IMHO (I'm not 100% sure it's better TBH, although I see renew !== renewed which I would be more keen on)
There was a problem hiding this comment.
Yes, I'll update. I figured that it would be easier to merge Membership/MembershipRenewal forms in the future if the variable names match - and $renewMembership is a membership array that is not necessarily a renewal.
There was a problem hiding this comment.
Rename now in #12798
mattwire
force-pushed
the
membershiprenew_twopaymentprocessorfields
branch
2 times, most recently
from
89da241 to
1216d24
Compare
mattwire
changed the title
mattwire
force-pushed
the
membershiprenew_twopaymentprocessorfields
branch
from
1216d24 to
666efe9
Compare
|
@mattwire this one is stale now so I'll close it out - not sure what pieces are left of this work |
Overview
Ref #12695 for the email receipt cleanup
Before
Less maintainable code.
After
Slightly more maintainable code.
Comments
@eileenmcnaughton Maybe another one for you? But also good if @agileware were to take a look given their interest in fixing renewal issues.