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

Fix retrieving contribution_status_id for manual payment processor #16018

Merged
merged 1 commit into from
Jan 18, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Dec 3, 2019

Overview

I'm not sure if this line ever actually gets hit but if it does it won't work because you need to pass in contribution_status_id and not status_id.

Before

NULL contribution status ID would be retrieved.

After

Pending contribution status ID would be retrieved.

Technical Details

Pseudoconstant requires contribution_status_id not status_id.

Comments

Found while working on some payment related stuff!

@civibot
Copy link

civibot bot commented Dec 3, 2019

(Standard links)

@civibot civibot bot added the master label Dec 3, 2019
@seamuslee001
Copy link
Contributor

@mattwire test failure looks like it might relate but not 100% sure mate

api_v3_ContributionTest::testpayLater

https://test.civicrm.org/job/CiviCRM-Core-PR/30956/testReport/junit/(root)/api_v3_ContributionTest/testpayLater/

@mattwire
Copy link
Contributor Author

mattwire commented Dec 3, 2019

@seamuslee001 Probably related failure yes, thanks :-( Any thoughts on this one @eileenmcnaughton

@mattwire
Copy link
Contributor Author

mattwire commented Jan 9, 2020

@seamuslee001 @eileenmcnaughton This should just be a simple typo fix - if it passes could you check/merge?

@seamuslee001
Copy link
Contributor

@mattwire same test failure has occurred as before

@mattwire
Copy link
Contributor Author

@eileenmcnaughton I picked this out as an obvious typo in the manual payment processor rather than a real-world failure. However, fixing it seems to break tests and I can't see why (it's to do with the pay later flag). So I don't know if this is a test error or a problem with the manual processor. And I don't actually use the manual processor anywhere. Any thoughts?

@seamuslee001
Copy link
Contributor

@mattwire i managed to get it to pass again, the issue is that i had to change https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/Contribution/Utils.php#L183 I would note that sets this function to something other than FALSE https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L1727

@mattwire mattwire force-pushed the manualcontributionstatus branch from fda3080 to ac1493e Compare January 18, 2020 12:44
@mattwire
Copy link
Contributor Author

@seamuslee001 Think I finally got it! There was a later code comparison that made the same mistake and returned false - so false == false. Now Pending == Pending!

@seamuslee001
Copy link
Contributor

I agree with this and as far as i can tell the only time that if was true was for manual processor and tested by the ispayLater test.

@seamuslee001 seamuslee001 merged commit b6a5d70 into civicrm:master Jan 18, 2020
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.

3 participants