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/core#1953 Ensure that Contribution pages do not fail validation o… #18144

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

seamuslee001
Copy link
Contributor

…n credit cards when a zero dollar price is offered

Overview

This fixes a regression caused by #16488 which was causing the required form rule to be applied to the credit card field no matter the price amount

Before

Zero dollar contribution on a public contribution form cannot be submitted

After

Can be submitted

Technical Details

I have added a required class on various of the credit card fields so that JQuery validation can still do the validation whilst removing the adding of the quickform rule on these fields

ping @eileenmcnaughton @mattwire @KarinG

@civibot
Copy link

civibot bot commented Aug 14, 2020

(Standard links)

@civibot civibot bot added the 5.29 label Aug 14, 2020
…n credit cards when a zero dollar price is offered
@KarinG
Copy link
Contributor

KarinG commented Aug 14, 2020

Ok testing this!

karins-MBP:d8civicrm.local sysadmin$ composer require civicrm/civicrm-asset-plugin:'~1.0.0' civicrm/civicrm-{core,packages,drupal-8}:'~5.28.0'

Reproduced the issue on Event Registration: note one MUST have a default Country and Province set otherwise you don't make it to this bug.

image

karins-MBP:civicrm-core sysadmin$ curl https://patch-diff.githubusercontent.com/raw/civicrm/civicrm-core/pull/18144.diff --output 18144.diff

karins-MBP:civicrm-core sysadmin$ cat 18144.diff |patch -p1
patching file CRM/Core/Payment.php
patching file CRM/Core/Payment/Form.php
patching file templates/CRM/Core/BillingBlock.tpl

Post:

image

Also tested $5 option -> with Dummy and with iATS TEST88 -> all good:

image

@seamuslee001
Copy link
Contributor Author

Adding merge on pass based on @KarinG 's testing

@eileenmcnaughton
Copy link
Contributor

Just a quick check - this still works if there are multiple processors? I'm thinking about a case where some fields are required for the default processor but not one of the others, which gets chosen, or the user changes selected processor a couple of times & jquery scurries to keep up. I recall there have been past trickinesses around that sort of thing

@seamuslee001
Copy link
Contributor Author

AFAIK yes if the billing block gets hidden then Jquery correctly ignores the hidden fields

@seamuslee001 seamuslee001 merged commit 0481af7 into civicrm:5.29 Aug 14, 2020
@seamuslee001 seamuslee001 deleted the dev_core_1953 branch August 14, 2020 23:04
@@ -831,7 +831,7 @@ public function getPaymentFormFieldsMetadata() {
'size' => 20,
'maxlength' => 20,
'autocomplete' => 'off',
'class' => 'creditcard',
'class' => 'creditcard, required',
Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 I don't think this should have a comma here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep true pushing a commit now

@demeritcowboy
Copy link
Contributor

JQuery validation can still do the validation whilst removing the adding of the quickform rule

Just want to add my two cents about the technical details and what is and isn't being removed. This isn't removing server-side validation completely in the case where there is a fee, and the fields are still getting validated server-side, just from another place (CRM_Core_Payment::validatePaymentInstrument() when it calls validateMandatoryFields()). If it wasn't you could just bypass javascript by using the browser console or a hand-crafted POST, and when I do that I can see it's still getting validated on the server.

I'm not against javascript validation as an additional enhancement. Just nervous about the above technical details being misread as a generic license that "jquery validation is ok as a lone method of validation".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants