woocommerce / woocommerce-gateway-stripe

The official Stripe Payment Gateway for WooCommerce
https://wordpress.org/plugins/woocommerce-gateway-stripe/
235 stars 206 forks source link

Consolidate validation #446

Closed roykho closed 6 years ago

roykho commented 6 years ago

First reported here https://wordpress.org/support/topic/stripe-wrong-custom-checkout-validation-notices-v-4-0-0/

Currently the default WC checkout form validation and the embedded Stripe credit card form validation is not "in-sync". Meaning sometimes you will see the WC error on the normal top location and sometimes you will see the CC error below the CC form location. In addition, if there are custom fields that are made required such as the order comments/notes, when clicking on Place Order while not filling in the CC or the order comments/notes, an error will show only for the CC and not for the order notes.

The issue is there are two different methods of validation happening, one from WC core and one that Stripe uses. In addition, Stripe's embedded CC fields also has its own inline validation to check if CC is correct format, expiration is not past..etc.

It would be better user experience to consolidate the error notices in one location while still maintaining CC inline dynamic validation.

roykho commented 6 years ago

Some enhancements were done in this commit https://github.com/woocommerce/woocommerce-gateway-stripe/commit/a53ae1307a856473e32bf22a5d4ea12ddb60bba9

Will be released soon.

ghost commented 6 years ago

For show error notices like WooCommerce (UX homogenization), you can use the same text and the field name in bold:

class-wc-stripe-order-handler.php replace line 402 with: $errors->add( 'validation', sprintf( __( '%s is a required field.', 'woocommerce' ), '<strong>'.$error_field.'</strong>' ) );

Other error texts on same file are less important, but you can also use WooCommerce's.

roykho commented 6 years ago

done here https://github.com/woocommerce/woocommerce-gateway-stripe/commit/6cf2fc4000b4ab2e3b5cd06be3dc3a0294ebf3ad

nathanwmac commented 6 years ago

I am experiencing this and it has caused an issue for the validation of my custom checkout fields.

I can see that the when you are entering a new card using the stripe gateway it will ajax submit to wc_stripe_validate_checkout first and then if all is good it calls the wc_checkout ajax action.

The issue i have is most of my custom fields are required but only if a checkbox is checked. Just the same as how the shipping fields work. My checkout flow is now broken because wc doesn't perform required validation on my custom fields and this lets me decide to validate it as required or not depending on the checkbox but due to the stripe validation indiscriminately validating my custom required fields this has now caused an issue.

There are also don't seem to be any hooks available to intercept and remedy this.

roykho commented 6 years ago

Hi @nathanwmac did you try the master branch?

nathanwmac commented 6 years ago

@roykho Yes. I just grabbed the latest master and my issue is present.

roykho commented 6 years ago

@nathanwmac how are you adding custom required fields?

nathanwmac commented 6 years ago

@roykho im using the "woocommerce_before_order_notes" hook and displaying fields using the woocommerce_form_field function. Im not adding fields to the checkout fields array/object.

roykho commented 6 years ago

@nathanwmac ok how does WC validate your custom fields if it is not apart of the checkout fields? I am trying to understand how you're using it to know if we can add a hook somewhere.

nathanwmac commented 6 years ago

@roykho WC doesn't validate my fields, i validate them on the woocommerce_checkout_process hook. The normal WC validation doesn't validate my fields even for required which is what i want but the validation in the stripe plugin IS validating my fields as required when i want it to leave them alone so i can validate them on the woocommerce_checkout_process hook.

roykho commented 6 years ago

@nathanwmac ok understood. If you could paste the code you're using to add the field and to validate at checkout process, I will investigate this further for you.

nathanwmac commented 6 years ago

@roykho i can't post my exact code as it's pretty heavily reliant and tied to the rest of my code so i broke it out in to the bare minimum to produce the issue.


function display_extra_checkout_fields( $checkout )
{ 
    $fields = get_extra_checkout_fields();
    if($fields){

        echo '<h3>Extra Fields</h3>';

        echo '<div class="si-tenant-fields">';

            woocommerce_form_field('extra_same_as_billing', array(
                'type'        => 'checkbox',
                'class'       => array('form-row-wide tm-same-as-toggle-cont'),
                'input_class' => array('tm-same-as-toggle'),
                'label'       => __('Same as billing details'),
                'required'    => false,
            ), '1');

            foreach($fields as $field_key => $fieldData){
                $value = WC()->checkout->get_value( 'extra_' .$field_key );
                woocommerce_form_field('extra_' .$field_key, $fieldData, $value);
            }
        echo '</div>';
    }
}

add_action( 'woocommerce_checkout_process', 'validate_extra_checkout_fields' );
function validate_extra_checkout_fields()
{   
    $fields = get_extra_checkout_fields();
    if($fields){

        // Check if same as is checked
        $sameAs = false;
        if(isset($_POST['extra_same_as_billing']) && $_POST['extra_same_as_billing'] == '1'){
            $sameAs = true;
        }

        foreach($fields as $field_key => $fieldData){

            if($sameAs && isset($fieldData['custom_attributes']['data-sameas']) && !empty($fieldData['custom_attributes']['data-sameas'])){
                continue;
            }

            if(isset($fieldData['required']) && $fieldData['required'] && (!isset($_POST[$field_key]) || empty($_POST[$field_key])) ){
                wc_add_notice( 'Extra - ' . $fieldData['label'] . ' is required', 'error' );
            }
        }
    }
}

function get_extra_checkout_fields()
{
    return array(
        'first_name' => array(
            'type'        => 'text',
            'class'       => array('form-row-first'),
            'input_class' => array(''),
            'label'       => __('First Name'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_first_name',
            )
        ),
        'last_name' => array(
            'type'        => 'text',
            'class'       => array('form-row-last'),
            'input_class' => array(''),
            'label'       => __('Last Name'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_last_name',
            )
        ),
        'company' => array(
            'type'        => 'text',
            'class'       => array('form-row-wide'),
            'input_class' => array(''),
            'label'       => __('Company name'),
            'required'    => false,
            'custom_attributes' => array(
                'data-sameas' => 'billing_company',
            )
        ),
        'address_1' => array(
            'type'        => 'text',
            'class'       => array('form-row-wide address-field'),
            'input_class' => array(''),
            'label'       => __('Street address'),
            'required'    => true,
            'placeholder' => "House number and street name",
            'custom_attributes' => array(
                'data-sameas' => 'billing_address_1',
            )
        ),
        'address_2' => array(
            'type'        => 'text',
            'class'       => array('form-row-wide address-field'),
            'input_class' => array(''),
            'required'    => false,
            'placeholder' => "Apartment, suite, unit etc. (optional)",
            'custom_attributes' => array(
                'data-sameas' => 'billing_address_2',
            )
        ),
        'city' => array(
            'type'        => 'text',
            'class'       => array('form-row-wide address-field'),
            'input_class' => array(''),
            'label'       => __('Suburb'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_city',
            )
        ),
        'state' => array(
            'type'        => 'state',
            'class'       => array('form-row-wide address-field'),
            'input_class' => array(''),
            'label'       => __('State'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_state',
            )
        ),
        'postcode' => array(
            'type'         => 'text',
            'class'        => array('form-row-wide address-field'),
            'input_class'  => array(''),
            'label'        => __('Postcode'),
            'autocomplete' => 'postal-code',
            'required'     => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_postcode',
            )
        ),
        'phone' => array(
            'type'        => 'tel',
            'class'       => array('form-row-first'),
            'input_class' => array(''),
            'label'       => __('Phone'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_phone',
            )
        ),
        'email' => array(
            'type'        => 'email',
            'class'       => array('form-row-last'),
            'input_class' => array(''),
            'label'       => __('Email address'),
            'required'    => true,
            'custom_attributes' => array(
                'data-sameas' => 'billing_email',
            )
        )
    );
}```
roykho commented 6 years ago

@nathanwmac Ok please checkout master branch. I've added this filter wc_stripe_validate_checkout_required_fields which you can use to unset any fields which you don't want to validate.

nathanwmac commented 6 years ago

@roykho That solves my issue. Thanks heaps.

ghost commented 6 years ago

I validate extra fields on the woocommerce_checkout_process hook too, as @nathanwmac.

How use the new filters from 4.0.2 to prevent this issue?

Thanks

nathanwmac commented 6 years ago

You just need to add a filter and remove the items you don't want validated.

add_filter( 'wc_stripe_validate_checkout_required_fields', 'stripe_validate_checkout_unset_required_fields' );
function stripe_validate_checkout_unset_required_fields($required_fields)
{
    // If you already know the name of the fields then just unset them.
    if(isset($required_fields['FIELD_NAME'])){
        unset($required_fields['FIELD_NAME']);
    }

   // If you need to look through them to find specific ones then loop over the field and unset the ones you want.
    foreach($required_fields as $field_name => $field_value)
    {
        $custom_field_name = 'my_field_name';
        if($field_name == $custom_field_name){
            unset($required_fields[$field_name]);
        }
    }
}
ghost commented 6 years ago

Thanks @nathanwmac. Because I need to check if a custom field is filled and validate it conditionally, I used two functions applied to wc_stripe_validate_checkout_required_fields filter:

/**
 * Conditionally remove a checkout field > Stripe
 * https://github.com/woocommerce/woocommerce-gateway-stripe/issues/446#issuecomment-358811534
 */
add_filter( 'wc_stripe_validate_checkout_required_fields', 'stripe_wc_custom_checkout_fields_unset' );

function stripe_wc_custom_checkout_fields_unset ($required_fields) {
    // If you already know the name of the fields then just unset them.
    if(isset($required_fields['custom_field'])) {
    $billing_country = array('ES','US');
        if ( !in_array($required_fields['billing_country'], $billing_country ) ) {
            unset($required_fields['custom_field']);
        }
    }
   return $required_fields;
}
/**
 * Validating custom field > Stripe
 */
add_filter( 'wc_stripe_validate_checkout_required_fields', 'stripe_wc_custom_checkout_fields_validate' );

function stripe_wc_custom_checkout_fields_validate ($required_fields) {
    // If you already know the name of the fields then just unset them.
    if(isset($required_fields['custom_field']) && !empty($required_fields['custom_field'])) {
        $billing_country = array('ES','US');
        if ( in_array($required_fields['billing_country'], $billing_country, true) ) {
            $falso = true;
            $custom_field = $required_fields['custom_field'];
            if (strlen($custom_field)<100) {
                $falso = true;
            }
            else {
                $falso = false;
            }
            if ( $falso ) { wc_add_notice( sprintf( __( '%s is not a valid custom field.', 'my-domain' ), '<strong>' . __( 'Custom Field', 'my-domain' ) . '</strong>' ) ,'error' ); }
        }
    }
   return $required_fields;
}

Now, works as expected, all fields are validated at the same time. @roykho, I'm not sure if is the best way to do this, but if Stripe is validating the fields separately, I think I have to perform two conditional unsets and validations (one for other payment methods -WC- and another for Stripe).

roykho commented 6 years ago

@nathanwmac note that we may change this behavior as I see it is causing some issues. Please have a read at this issue here to understand why we had to add in extra validation https://github.com/woocommerce/woocommerce-gateway-stripe/issues/161

But now I am not so sure if that was the best decision.

ghost commented 6 years ago

I solved the problem (I hope) with the filter and functions on https://github.com/woocommerce/woocommerce-gateway-stripe/issues/446#issuecomment-359103719

After reading https://github.com/woocommerce/woocommerce-gateway-stripe/issues/161, seems that using/extending WC default validation can be the better way.

Thanks.

roykho commented 6 years ago

I have removed the validation from checkout for NON Stripe Modal checkouts. This means it is back to how it used to function pre 4.0.0. I would suggest you give this branch a test and see if it will work with your current setup https://github.com/woocommerce/woocommerce-gateway-stripe/tree/validation

clifgriffin commented 6 years ago

Assuming this will also fix Stripe trying to validate other gateway's inputs? :)

https://www.dropbox.com/s/hd5ms1rjv360i8z/Screenshot%202018-02-08%2014.12.34.png?dl=0

roykho commented 6 years ago

@clifgriffin will definitely double check that :)

clifgriffin commented 6 years ago

@roykho Sorry for the duplicate issue. My reading of this issue is that it had a different focus / goal so I didn't want to muddy the waters.

Just to concisely express our desired outcome in this ticket: We want a consistent interface / event model for intercepting and handling all errors.

Thanks :-)

ghost commented 6 years ago

@roykho, tested branch https://github.com/woocommerce/woocommerce-gateway-stripe/issues/446#issuecomment-363604340

With credit card fields directly on the page & inline credit card form

With modal credit card form

roykho commented 6 years ago

Thanks for testing.

Card validation occurs before the WooCommerce form conditional required/validation, something that does not happen with the master version

Yes that is how it was pre 4.0. At least this way it doesn't break the checkout people were reporting after 4.0.

roykho commented 6 years ago

I don't know the best way to satisfy both parties here. I rather it be less user friendly than it breaking checkouts like if you have custom fields.

roykho commented 6 years ago

Adding additional note here. Sure we can move the Stripe credit card dynamic real time validation errors to show at the same place/location as the WC ones but then that is even worse experience for customers as they probably can't even see it since its up top.

So at the end of the day, it isn't too bad if they see the CC error first on submit if it isn't filled out. In normal use case, customers will follow the flow of the form anyhow.

Adding validation to satisfy only merchants who want to use Stripe Checkout (Modal) isn't practical at this point. Not to mention essentially we're doing double validation, once before the modal opens and again after submission. So I am now leaning towards removing any "pre" validation and just let WC handle it on submit.

Any thoughts?

ghost commented 6 years ago

I think that the payment experience for any method should be as similar as possible, so better if you can use WooCommerce's native validation. Less code and easy setup.

If Stripe needs some additional validation, can't it be added individually through 'woocommerce_checkout_process'?

Anyway, you can also use documented hooks for conditional require/validate, similar to the ones I currently have.

roykho commented 6 years ago

I think that the payment experience for any method should be as similar as possible, so better if you can use WooCommerce's native validation. Less code and easy setup.

So its because people who use Stripe Checkout (Modal) wants a validation before the popup modal shows. That is what caused this whole issue.

ghost commented 6 years ago

I tested 4.0.6 and 4.0.7 and validation works on standard mode without additional code 👍

On 4.0.6 changelog, appears:

* Remove - Checkout validation and let WC handle it.
* Add - Hook `wc_stripe_validate_modal_checkout` to enable 3rd party checkout validation.
* Add - Hook `wc_stripe_validate_modal_checkout_action`.

If anyone wants to validate on modal type, we must use "wc_stripe_validate_modal_checkout" or "wc_stripe_validate_modal_checkout_action"? What of both is equivalent to old "wc_stripe_validate_checkout_required_fields"?

Maybe can be a good idea add some info about validation and that hooks to https://docs.woocommerce.com/document/stripe/

This issue must be closed?

Thanks!

roykho commented 6 years ago

Yeah added those hooks in case people still want some custom validation, they can add that.

I don't want to close this issue yet as I am still trying to find a way to display the errors better. If I cannot find a way then we can close.

iamgabrielma commented 6 years ago

Commenting here to followup with 970907-zen when/if this enhancement request can be done.

tevyaw commented 6 years ago

I'm not a developer so I don't fully follow the whole conversation here. But I do know quite a bit about WP and WooCommerce. So is issue #559 included in this? I had the Stripe checkout setup on a site and noticed that on mobile (Android Chrome), without anything in any of the fields, clicking the pay button would open Stripe checkout in a new tab. Is that what's being addressed here? I went back to the inline Stripe payment form for now, for that reason. But am just curious if this issue covers that as well?

roykho commented 6 years ago

No, we will no longer do any validation prior to Stripe modal popup.

tevyaw commented 6 years ago

So you're saying it's not a bug, that's the way it's supposed to work? How is that a good user experience? Or am I misunderstanding? Can they leave the fields blank and proceed to complete checkout? Or will they have to go back when the Stripe modal fails?

roykho commented 6 years ago

Its not a bug. And no its not a good UX but nothing we can do at this time. They can't checkout without filling up all the required fields so there is no issue there. But in normal flow, why would a customer click on checkout before filling out the fields? The button is at the end no?

iamgabrielma commented 6 years ago

992502-zen reported the same, adding it here to followup in case we change this UX behavior.

daviddarke commented 6 years ago

@roykho if the fields are something like 'create username' for signed out users, this needs validating. Getting the green tick from a stripe popup (before payment is actually complete) is super confusing for users. They will think they have paid twice.

roykho commented 6 years ago

@daviddarke You have many choices. Turn off Stripe checkout OR implement this on your end. We have put in hooks to allow this. It is stated in the changelogs.

This debate has been going on for awhile now and there are pros/cons to each method. However I rather the UX be a little poor than it breaking the checkout.

nmarsh33 commented 6 years ago

Do you have some code that does the pre-validation of the other WC required fields before the modal pops up? I'm messing around with those hooks, but I can't seem to get it to work.