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

Clean up code to determine line items for membership batch entry #20779

Merged
merged 1 commit into from
Jul 6, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Clean up code to determine line items for membership batch entry

Before

whoa

After

Whee

Technical Details

This is towards switching this code to use the order api as it currently uses the BAO to create it's contributions

Comments

Builds on #20778

@civibot
Copy link

civibot bot commented Jul 6, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

I suspect this is the underlying e-notice - closing until I've fixed

Stacktrace
CRM_Member_Form_MembershipTest::testSubmitRecur
CiviCRM_API3_Exception: Illegal string offset 'line_total'

/home/jenkins/bknix-dfl/build/core-20779-4u7oy/web/sites/all/modules/civicrm/api/api.php:134
/home/jenkins/bknix-dfl/build/core-20779-4u7oy/web/sites/all/modules/civicrm/CRM/Member/Form/Membership.php:1153
/home/jenkins/bknix-dfl/build/core-20779-4u7oy/web/sites/all/modules/civicrm/CRM/Member/Form.php:526
/home/jenkins/bknix-dfl/build/core-20779-4u7oy/web/sites/all/modules/civicrm/tests/phpunit/CRM/Member/Form/MembershipTest.php:795
/home/jenkins/bknix-dfl/build/core-20779-4u7oy/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:256
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

@colemanw
Copy link
Member

colemanw commented Jul 6, 2021

@eileenmcnaughton I'm not sure why the commit from #20778 is still showing up in this PR. Can you try rebasing?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw done

This removes a chunk of weird code & further consolidates on the internal BAO_Order helper.

Note the test cover is in CRM_Batch_Form_EntryTest, which has
validateFinancials enabled
@@ -742,22 +760,45 @@ public function getLineItem($index): array {
*/
protected function fillMembershipLine(array $lineItem): array {
$fields = $this->getPriceFieldsMetadata();
$field = reset($fields);
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 incorrectly assumed there would only be one possible field - but it's one per membership organization (the field in the membership_type table)

*
* @return void
*/
protected function addTotalsToLineBasedOnOverrideTotal(int $financialTypeID, array &$lineItem): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extraction

@eileenmcnaughton
Copy link
Contributor Author

@colemanw are you comfortable merging this - it's less scary & more tested than it looks - but I agree it looks a bit scary :-)

@colemanw
Copy link
Member

colemanw commented Jul 6, 2021

Ok I see this is mostly code cleanup and test coverage is good.

@colemanw colemanw merged commit 6d2cfff into civicrm:master Jul 6, 2021
@colemanw colemanw deleted the batch2 branch July 6, 2021 23:25
@eileenmcnaughton
Copy link
Contributor Author

@colemanw yeah - it was test cover that made me realise this code is doing something weird :-)

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