search

craftcms / commerce

Fully integrated ecommerce for Craft CMS.
https://craftcms.com/commerce
Other
218 stars 170 forks source link

No error generated in cart if invalid coupon code added #2491

Closed peteeveleigh closed 3 years ago

peteeveleigh commented 3 years ago

Adding a non-existent coupon code is accepted by the cart. No discount is applied but no error is generated.

To reproduce, try adding a completely made-up coupon code to the cart and check cart.getErrors() or cart.getFirstError('couponCode')

I did notice that there is something added to cart.notices

I do have a number of other sites where this has never been an issue so can't rule out some weirdness specific to this site but there are no custom event listeners at play and it's pretty much "straight out of the box" as far as the cart is concerned.

Expected behavior The add-to-cart form submission fails and generates an error - which in turn shows the flash message from the failMessage param of the update cart form.

Additional info

lukeholder commented 3 years ago

Is updating the coupon code on the order the only thing you are doing in the request?

peteeveleigh commented 3 years ago

Hi Luke,

Yes the coupon code is in its own form. As shown below:

<form method="POST" class="form form--inline form--white" data-coupon-form>
    <input type="hidden" name="action" value="commerce/cart/update-cart">
    {{ redirectInput('shop/basket') }}
    {{ csrfInput() }}
    {{ hiddenInput("successMessage", "Added coupon code" | hash) }}
    {{ hiddenInput('failMessage', 'Error adding cart.'|hash) }}

    <div class="form__row">
        <label class="form__label" for="couponCode">Voucher code</label>
        {% if cart.couponCode == 'hmsample2020' %}
        <p class="basket-coupon-message">Please note, we are able to supply a maximum of three handmade wall tile samples per order.</p>
        {% endif %}

        <div class="form__input-wrap">
            <input class="form__input   {% if cart.getFirstError('couponCode') %}form__input--error{% endif %}" id="couponCode" placeholder="" type="text" name="couponCode" value="" autocomplete="off">
            {# Debug output #}
            {% if cart.hasErrors() %}
                <p>{% dd(cart.hasErrors()) %}</p>
            {% endif %}
        </div>
    </div>

    <div class="form__row form__row--submit">
        <button type="submit" class="button btn btn--block btn--primary">Add</button>
    </div>
</form>

The form is being submitted normally and not via Ajax. There are no other listeners intercepting its behaviour.

bossanova808 commented 3 years ago

I can confirm this one...we are applying it via ajax, posting to update-cart as normal, a veru minimal request...we have no coupon code broken of course...

{
       ....CSRF hooplah....
    "redirect": "6bb2a73aac3479c68e0946e4455624e2c1b840bc2b6356999a51ba01ee4dc58d/cart",
    "action": "commerce/cart/update-cart",
    "couponCode": "broken"
}

...and the response

{
    "success": true,
    "cart": {
        "number": "5ed1b024fe6ce4ecc709997b8c36f4ae",
        "reference": null,
        "couponCode": null,
        "isCompleted": false,
        "dateOrdered": null,
        "datePaid": null,
        "dateAuthorized": null,
        "currency": "AUD",
        "gatewayId": 1,
        "lastIp": "119.17.136.216",
        "message": null,
        "returnUrl": null,
        "cancelUrl": null,
        "orderStatusId": null,
        "orderLanguage": "en-AU",
        "orderSiteId": 1,
        "origin": "web",
        "billingAddressId": 57936,
        "shippingAddressId": 57936,
        "makePrimaryShippingAddress": null,
        "makePrimaryBillingAddress": null,
        "shippingSameAsBilling": false,
        "billingSameAsShipping": false,
        "estimatedBillingAddressId": null,
        "estimatedShippingAddressId": null,
        "estimatedBillingSameAsShipping": false,
        "shippingMethodHandle": "pickup",
        "shippingMethodName": "Pickup From IS",
        "customerId": 170,
        "registerUserOnOrderComplete": null,
        "paymentSourceId": null,
        "storedTotalPrice": 379,
        "storedTotalPaid": 0,
        "storedItemTotal": 379,
        "storedItemSubtotal": 0,
        "storedTotalShippingCost": 0,
        "storedTotalDiscount": 0,
        "storedTotalTax": 0,
        "storedTotalTaxIncluded": 34.45,
        "id": 258272,
        "tempId": null,
        "draftId": null,
        "revisionId": null,
        "isProvisionalDraft": false,
        "uid": "2a544839-4999-4843-8292-7159290f363c",
        "siteSettingsId": 258266,
        "fieldLayoutId": 2515,
        "contentId": 92501,
        "enabled": true,
        "archived": false,
        "siteId": 1,
        "title": null,
        "slug": null,
        "uri": null,
        "dateCreated": {
            "date": "26/7/2021",
            "time": "9:52 am"
        },
        "dateUpdated": {
            "date": "26/7/2021",
            "time": "9:54 am"
        },
        "dateLastMerged": null,
        "dateDeleted": null,
        "trashed": false,
        "canonicalId": 258272,
        "ref": null,
        "status": "enabled",
        "structureId": null,
        "url": null,
        "orderNotes": null,
        "beagleScore": null,
        "tracking": null,
        "orderType": null,
             ....a whole lot of other stuff.....
    "message": "Cart updated."
}
nfourtythree commented 3 years ago

Hi @fantasticmachine & @bossanova808

As of 3.3.0 validation issues surrounding coupon codes are now added to the cart notices (available on the cart variable and in the JSON response).

This was done specifically to tackle some bugs/checkout flow for customers. For example people were unable to add things to their cart if they returned to the site after they had previously entered a coupon and in that time it had expired/become invalid.

Because the update-cart endpoint can take all manner of input the methodology we have employed means that the cart can still be updated successfully (with things like adding to cart, update addresses etc) while removing/prevent adding an invalid coupon.

More information on notices can be found in the docs: https://craftcms.com/docs/commerce/3.x/orders-carts.html#order-notices

Hope this helps, thanks!

peteeveleigh commented 3 years ago

Hmm. This is going to mean some work for us to update sites where we were using a Flash messaging system.

Can you confirm that {% do cart.clearNotices(null, 'couponCode') %} is actually working? I'm trying to test this now and it doesn't appear to clear the messages at all.

bossanova808 commented 3 years ago

(I've seen nothing but mess from the cart notices system till now, all sorts of odd behaviour, so I've been ignoring it).

Really feel like an invalid coupon code should continue to return an error TBH, just doesn't make sense on the face of it to happily return 'cart updated' when...it hasn't.

peteeveleigh commented 3 years ago

I think I can live with the cart notices method but I'm unsure how to work it if the messages are persistent. Seems to make things really difficult.

We've really tried to use a unified messaging system on our sites where flash messages get displayed. With things like invalid codes etc generating an error and thus a flash message, this worked nicely. I've just had a quick tinker and can't really see how I can make this work with a mix of notices and flash messages.

nfourtythree commented 3 years ago

Can you confirm that {% do cart.clearNotices(null, 'couponCode') %} is actually working? I'm trying to test this now and it doesn't appear to clear the messages at all.

clearNotices only removes them in memory you will need to save the element after doing so: {% do craft.app.elements.saveElement(cart) %}. This might be a little unclear from the docs, so will look to get that sorted.

Really feel like an invalid coupon code should continue to return an error TBH, just doesn't make sense on the face of it to happily return 'cart updated' when...it hasn't.

As mentioned in the previous comment these changes were made to provide a better checkout flow for customers and prevent them from hitting any points where they might receive validation errors not related to the part of the cart they are updating. We believe the current trade-off of checking the notices for couponCode outweighs the potential checkout breaking scenario mentioned.

We are, of course, looking into how we can improve all of this in the future. Better determine what action is taking place etc. We have a lot of people asking for specific endpoints and also a lot of people asking for just one endpoint to post data to during checkout. These, along with validation and cart/order state management is something we will be evaluating as we progress.

I think I can live with the cart notices method but I'm unsure how to work it if the messages are persistent. Seems to make things really difficult.

You can clear notices when posting to the update-cart endpoint (mentioned in the docs). This means you could conditionally add this hidden field if needed, therefore when the form is submitted the notices will be cleared.

We've really tried to use a unified messaging system on our sites where flash messages get displayed. With things like invalid codes etc generating an error and thus a flash message, this worked nicely. I've just had a quick tinker and can't really see how I can make this work with a mix of notices and flash messages.

I understand they are two different concepts but they certainly have some similarities and could definitely coexist in a project.

peteeveleigh commented 3 years ago

Thanks for the explanation.

bossanova808 commented 3 years ago

So...working on this...and this really is not great from an ajax perspective.

With an invalid coupon code submitted by a user, we get back a cart var. with an array of notices (there might e.g. be a shipping notice for whatever reason at this point).

Search through that array to find the coupon code error...show the user a message - so they then type in a correct coupon code....

...in the meantime, we'll have had to hit the server to clear the coupon code, right? Because there's no way to tell whether the notice is 'fresh' or not, what request it was connected to...I suppose I could add a clear notices input to the form, but then I might be inadvertently clearing some other notice. So for a nice ajax experience, we're left with having to post to the server in the background to clear this notice. That's just not great.

Really, the response to the POST to update-cart should be an error, when there's an error. And an invalid coupon code is an error. The behaviour was better and clearer before.

bossanova808 commented 3 years ago

(Is there an input to clear just one type of notice?)

philipboomy commented 2 years ago

Yeah I am not happy about this. If I ajax post and I get success back I assume its success but it's not in this case. Same goes with with a normal form. If I post a coupon code and I get cart updated back I assume cart was updating without issues but it didn't as coupon code failed. According to this https://craftcms.com/docs/commerce/3.x/coupon-codes.html#using-a-coupon I should be getting an error but I am not. I assume the docs are wrong. Having to look through notices now and then having to clear them is very messing.

mattstein commented 2 years ago

Updated the docs to better cover errors and notices.

bossanova808 commented 2 years ago

That Matt, that's nice and clear.