Remove unused paymentInstrumentID from forms and fix regression #17589
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)
|
|
@mattwire this doesn't quite work - if you look at the Participant Payment form for the manual processor it correctly has the payment_instrument_id
The form doesn't actually load that part with this patch. The issue really is that we don't want to ask the processor about the paymentInstrumentID I guess - we want to take if from
|
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Jun 14, 2020
Per civicrm#17589 it is necessary that we do NOT pass the wrong payment_instrument_id to the payment processor. BUT we have to provide accurate defaults for manual payments. This is a narrower fix than - back office membership form - back office participant form - back office contribution form and in all cases the Manual processor still loaded correctly. I don't feel I can say this is the last fix in this area but it stands alone as a sensible fix to do and also one that should address the immediate issue. Note there is some weirdness in a second function in EventFees - that function should GO IMHO - but I have not yet reached it in UI testing to confirm if other changes need to be made.
|
@mattwire closing this as the immediate issue is fixed and this has a fatal flaw at this stage. |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Jun 15, 2020
Per civicrm#17589 it is necessary that we do NOT pass the wrong payment_instrument_id to the payment processor. BUT we have to provide accurate defaults for manual payments. This is a narrower fix than - back office membership form - back office participant form - back office contribution form and in all cases the Manual processor still loaded correctly. I don't feel I can say this is the last fix in this area but it stands alone as a sensible fix to do and also one that should address the immediate issue. Note there is some weirdness in a second function in EventFees - that function should GO IMHO - but I have not yet reached it in UI testing to confirm if other changes need to be made.
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
As identified by https://lab.civicrm.org/extensions/smartdebit/-/issues/2 there is a core regression around the paymentInstrumentID using the default instead of the payment processor.
As pointed out by @eileenmcnaughton (#17510) we should be using the payment processor
paymentInstrumentID. Prior to 5.21 (when propertyBag was introduced) we were because of a bug insetPaymentInstrumentID()which ignored the incoming parameter. That was fixed with propertyBag which meant that we started using the default paymentInstrumentID (incorrectly in most cases).Going through the code I identified that in most cases we were getting a default and assigning it to the form (even supporting
payment_processor_idas a URL parameter) but then ignoring it when submitting the form. The only time we do want it to work is when recording a payment without a payment processor - and that is the only time the instrument select element actually appears on the form and the value is available during postProcess.Before
Default paymentInstrumentID used when it shouldn't be.
After
Cleaned up setting of
paymentInstrumentIDand not overriding the payment processor anymore.Technical Details
buildPaymentForm()is only relevant if we have a payment processor. So we should never pass a paymentInstrumentID.$form->paymentInstrumentIDis read but never set. So we can remove it throughout.Comments