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

Not getting custom field errors on request to update-cart #1292

Closed
samhibberd opened this issue Feb 21, 2020 · 9 comments
Closed

Not getting custom field errors on request to update-cart #1292

samhibberd opened this issue Feb 21, 2020 · 9 comments

Comments

@samhibberd
Copy link

samhibberd commented Feb 21, 2020

Calling the update-cart controller via Ajax, does not return any field errors. Includes both matrix and standard custom fields, standard fields look to save ok (if valid), not got matrix fields playing ball yet, so must be failing validation somewhere. Validation when saving in the cp is working.

Update 24/02/20:

  • Updating any required plain text field (standalone or in an existing matrix bock rows field) incorrectly passes validation when passing an empty string (supplying a value correctly updates the value)
  • Saving a new matrix block row fails but with no validation errors and a cart success response returned.
  • Updating an existing matrix block row works and returns validation errors if incorrect data is supplied (unless it's a plain text field as above) like invalid email to an email field.

Craft 3.4.6.1
Commerce 3.0.10

@nfourtythree nfourtythree changed the title Not getting custom field errors on Ajax request to update-cart Not getting custom field errors on request to update-cart Feb 26, 2020
@nfourtythree
Copy link
Contributor

Hey @samhibberd

We have just added the ability to pass validateCustomFields in an update cart request.

This will give you errors (if applicable).

The update will be included in the next release.

To get this early, change your craftcms/commerce requirement in composer.json to:

"require": {
  "craftcms/commerce": "dev-develop#bcf386211cede1f3e042433dad62c6349400b1f8 as 3.0.11",
  "...": "..."
}

Then run composer update.

The following is the information that will be going in the docs with this feature.


Validating custom fields

By default, when updating the cart, custom fields are not validated.

This is to avoid showing the user errors for unrelated fields when taking an action such as adding to cart.

If you want to allow custom fields to be validated on an update cart request, you can do so by adding the following field to the form:

<input type="hidden" name="validateCustomFields" value="1">

@samhibberd
Copy link
Author

Thanks for getting this working.

After a talking through with @nfourtythree, I raised if it would potentially be better to determine if the fields[] param is set and only validate is set, I could be wrong I can't see a scenario where you would pass custom field data without wanting it validated? With this implementation, although unlikely, someone could disable validation.

@samhibberd
Copy link
Author

samhibberd commented Mar 16, 2020

Hi @nfourtythree assuming this falls under the this issue, but addresses (with custom rules defined) are also not validating when saving via commerce/cart/update-cart, correctly validate when saved via commerce/customer-addresses/save.

Would there be an equivalent validateAddressFields input or an alternative solution?

Preference would still be validate custom fields if any provided via fields[] and to validate addresses if there are custom rules defined for addresses.

@nfourtythree
Copy link
Contributor

nfourtythree commented Mar 16, 2020

Hi @samhibberd

I have just tested with the following custom validation and I was able to retrieve the error posting to commerce/cart/update-cart.

        Event::on(Address::class, Address::EVENT_DEFINE_RULES, function($event) {
            if (!empty($event->rules)) {
                $event->rules[] = [['firstName'], function($attribute, $params, Validator $validator) use ($event) {
                    if ($event->sender->firstName && $event->sender->firstName != 'Testing') {
                        $validator->addError($event->sender, $attribute, Craft::t('site', 'First name is not "Testing".'));
                    }
                }];
            }
        });

@samhibberd
Copy link
Author

My bad, I was using a custom controller which was pre-filing the firstName and lastName, values, adding some additional rules in the defineRules handler worked a dream:

public static function defineRules(DefineRulesEvent $e)
{
    $e->rules[] = [[ 'firstName', 'lastName', 'address1', 'city', 'zipCode', 'countryId' ], 'required'];
}

Regards custom fields, is this being considered:

After a talking through with @nfourtythree, I raised if it would potentially be better to determine if the fields[] param is set and only validate is set, I could be wrong I can't see a scenario where you would pass custom field data without wanting it validated? With this implementation, although unlikely, someone could disable validation.

@nfourtythree
Copy link
Contributor

nfourtythree commented Mar 16, 2020

At the moment we are sticking with the extra validateCustomFields param, as this way gives the option of doing both which you wouldn't get with the fields[] implementation.

It is definitely a valid point about someone being able to disable it. But as a developer, you will undoubtedly have checks to stop the customer from progressing any further if you do not have the data you require based on the stage they are at in the checkout. In a similar way, you wouldn't allow show the customer the payment form if they didn't have any line items.

@lukeholder
Copy link
Member

@samhibberd We has to reverse the validateCustomFields idea. And moved to a config setting validateCartCustomFieldsOnSubmission.

This defaults to false, but if set to true, it will validate any custom field that is submitted in a cart update request. It will not validate any custom field when set to false (default). It will not validate a custom field that is not submitted when set to true.

@lukeholder
Copy link
Member

In 3.0.12

@samhibberd
Copy link
Author

Brilliant, thanks team, exactly what we were hoping for. 🎉

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

No branches or pull requests

3 participants