Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Simplify isRenew handling on batch for membership #20791

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 7, 2021

Overview

[REF] Simplify isRenew handling on batch for membership

Before

is_renew is set and it is not obvious why

After

Tracked down the usage to email receipt, replaced with a getter

Technical Details

This starts the process of using getters to get data from the row and also
marks the no-longer shared emailReceipt function as no longer deprecated
(since it was copied back to this form early).

Clean Money moved to 'stdiseRow'

I forsee more logic being moved into isRenew as we have some odd stuff to figure it out hidden away

Comments

@civibot
Copy link

civibot bot commented Jul 7, 2021

(Standard links)

@civibot civibot bot added the master label Jul 7, 2021
@@ -733,12 +740,8 @@ private function processMembership(array $params) {
continue;
}
$value['contact_id'] = $params['primary_contact_id'][$key];
foreach ($value as $fieldKey => $fieldValue) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved into standardiseRow as this is part of the form-transform to sanity

@@ -865,10 +866,6 @@ private function processMembership(array $params) {
* true if mail was sent successfully
* @throws \CRM_Core_Exception|\API_Exception
*
* @deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was true before we split the function onto both forms.

@eileenmcnaughton
Copy link
Contributor Author

CRM_Batch_Form_EntryTest::testMembershipRenewalDates
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2021-07-07'
+'2015-01-01'

@eileenmcnaughton eileenmcnaughton force-pushed the batch_renew branch 2 times, most recently from 4492058 to 112eacb Compare July 7, 2021 09:01
This starts the process of using getters to get data from the row and also
marks the no-longer shared emailReceipt function as no longer deprecated
(since it was copied back to this form early).

Clean Money moved to 'stdiseRow'
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb if you have more time this one is also on my path to cleaning up membership stuff

* @return bool
*/
private function currentRowIsRenew(): bool {
return $this->currentRowIsRenewOption === 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit this is not a part of the refactoring change but should that options need to be registered in CRM_Core_SelectValues and later

private function currentRowIsRenew(): bool {
   return $this->currentRowIsRenewOption === CRM_Core_SelectValues::membershipModes()['Renew Membership'];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb yeah it's been hard enough to figure what it means :-)

Actually the value is distinct to this form I think so we need to make it clear within this class rather than outside the class (I'll keep thinking about ways to do that)

@monishdeb
Copy link
Member

r-run passed. The patch looks good. Merging now.

@monishdeb monishdeb merged commit b33ecf0 into civicrm:master Jul 16, 2021
@eileenmcnaughton eileenmcnaughton deleted the batch_renew branch July 16, 2021 06:38
@eileenmcnaughton
Copy link
Contributor Author

thanks @monishdeb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants