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

dev/financial#68 Ensure that check number is correctly passed through… #15272

Merged
merged 3 commits into from
Sep 11, 2019

Conversation

seamuslee001
Copy link
Contributor

… when processing a contribution or membership batch

Overview

This is an alternate to #15264 as it alters the post processing on the batch rather than altering field names as the field_name should be contribution_check_number now

Before

Check number is only recorded against the contribution not the payment as well when submitting a contribution or membership batch

After

Check number is recorded against the contribution and the payment

ping @monishdeb @eileenmcnaughton

… when processing a contribution or membership batch
@civibot
Copy link

civibot bot commented Sep 10, 2019

(Standard links)

…m as contribution_check_number is correctly saved into the financial Transaction
@eileenmcnaughton
Copy link
Contributor

Approach makes sense - will let @monishdeb chime in

@monishdeb
Copy link
Member

@eileenmcnaughton @seamuslee001 I am happy with the patch too. Earlier I was thinking, instead of tackling in the form or BAO level we should remove the deprecated field names from the source and so I changed the field name, and let the BAO fn process it accordingly. On the other hand, if we create a profile with contribution field - check number it again renders the field name as 'contribution_check_number' and thus it will again cause the same issue after form submission until and unless we change the field name from the source. OR let BAO/API to support the old fieldname too.

@eileenmcnaughton
Copy link
Contributor

The api should support unique names & real field names at the wrapper level already - but this is bypassing the api

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

@monishdeb @eileenmcnaughton i just added a commit which alters a current contribution api test so that it submits using the unique field name contribution_check_number and then checks that check_number is still correctly stored in the financial trxn records as well as in civicrm_contribution

@@ -233,7 +233,7 @@ public function testGetTestContribution() {
*/
public function testGetContributionReturnFunctionality() {
$params = $this->_params;
$params['check_number'] = 'bouncer';
$params['contribution_check_number'] = 'bouncer';
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally both should work - maybe test both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…ied in unique field it is still correctly stored

Add in a unit test to test setting check_number with original field
@eileenmcnaughton
Copy link
Contributor

Ok - @monishdeb agreed earlier so MOP

@seamuslee001 seamuslee001 merged commit b86d087 into civicrm:master Sep 11, 2019
@seamuslee001 seamuslee001 deleted the dev_financial_68 branch September 11, 2019 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants