dev/financial#77 ++ Make contribution_id mandatory for PaymentProcessor.pay, pass invoiceID #15639
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
…or.pay, pass incoieID Replaces civicrm#15477 & also resolves https://lab.civicrm.org/dev/financial/issues/77 by requiring contribution_id for PaymentProcessor.pay as a way to ensure that it is only called after the order is created per our preferred flow (people could still get past this but it feels like they would at least know they werre hacking our process & take responsibility for any issues if it breaks or we suddenly start enforcing that it is a valid contribution. This also sets some of the recommended variables. Note that I had to use a few more lines to ensure we were always setting contactID, contributionRecurID to an integer. I do think this stricter approach is better but it wound up more verbose
mattwire
reviewed
Oct 28, 2019
| $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']); | ||
| $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $params['payment_processor_id']])); | ||
| try { | ||
| $processor->setContributionID($params['contribution_id']); | ||
| $processor->setInvoiceID($params['invoice_id'] ?? ''); |
There was a problem hiding this comment.
@eileenmcnaughton What did we agree in terms of invoice_id? This means that we'll end up with an empty invoice ID if not passed in via params. Maybe that's ok!
There was a problem hiding this comment.
@mattwire yep - it has to be passed in! We talked about Order.create adding one here https://lab.civicrm.org/dev/financial/issues/78 but haven't resolved that & although I marked it 'high' here https://lab.civicrm.org/dev/financial/issues/76 I'm re-thinking that & am probably not seeing it as something to worry about until some of the other things have been done
|
@eileenmcnaughton Apart from the query on invoice_id this is good to merge |
|
@mattwire FYI the invoice_id one shouldn't have a negative effect - it just means that it will be passed through if available |
wmfgerrit
pushed a commit
to wikimedia/wikimedia-fundraising-crm
that referenced
this pull request
Feb 4, 2021
Ever since civicrm/civicrm-core#15639 was implemented we have been hacking it back out of our civicrm code base each point version update. We did have a chance in Barcelona to veto this change but we committed to being good code citizens & updating our code to comply. That day has not yet come so I'm changing the hack to require less ongoing maintenance until we reach the point of becoming good code citizens. This hack will not need to be re-applied regularly and should be no less risky than our previous hack Bug: T272061 Change-Id: I42e6fc795ecf6fd4224d2631e3422685749699ce
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Replaces #15477 & also resolves https://lab.civicrm.org/dev/financial/issues/77
As discussed in Barcelona - requiring contribution_id for PaymentProcessor.pay as a way to ensure that it is only called after the order is created
per our preferred flow (people could still get past this by passing a random number - but it feels like they would at least know they were hacking our process & take responsibility for any issues if it breaks or we suddenly start enforcing that
it is a valid contribution.
This also sets some of the recommended variables.
Before
contribution_idnot required by API PaymentProcessor.pay.After
contribution_idrequired by API PaymentProcessor.pay.Technical Details
See https://lab.civicrm.org/dev/financial/issues/53.
Technical Details
@mattwire it seems the test for https://lab.civicrm.org/dev/financial/issues/77 could easily also cover #15477 so I incorporated it.
Note that I had to use a few more lines to ensure we were always setting contactID, contributionRecurID to an integer. I do think this stricter approach is better but it wound up
more verbose
Comments