Commerce: Multi Add to cart forms don't add items if one quantity is 0

[nfourtythree]

Description

This is a lead on from #321 opening a new issue for easier tracking of the issue (as requested)

My Notes from the other thread:

With the update, you now can't submit a multi-add form that has rows with 0 qty. If you take the example code in the docs and tweak it

<form method="POST">
  {{ csrfInput() }}
    <input type="hidden" name="action" value="commerce/cart/update-cart">

    {% for variant in product.variants %}
        <input type="hidden" name="purchasables[{{loop.index}}][id]" value="{{ variant.purchasableId }}">
        <input type="text" name="purchasables[{{loop.index}}][qty]" value="1">
        <input type="hidden" name="purchasables[{{loop.index}}][note]" value="1">
    {% endfor %}

    <input type="submit" value="Add all variants to cart">
</form>

Then change the first qty box to be 0. You get a cart error with the validation error being you can't submit something with a qty less than 1.

Which as a data validation rule is fine, but in practice would mean that building a project you would either need to disabled the 0 qty fields on submit, or strip them from the post etc.

It would seem like in a multi-add form it is likely that qty data submitted would be a range from 0 upwards.

I know my pull request for this originally (#322) was far too simple but it was a way of not letting the data get to the point of validation.

Hope this helps, let me know if you need any more info. I came across this as my add to cart stopped working after updating

Steps to reproduce

1. Copy and paste code above within a product with variants
2. Run the page change the first qty box to 0
3. Expected outcome All products added with quantities should add
4. Actual outcome Nothing adds

Additional info

• Craft version: Craft Pro 3.0.4
• PHP version: 7.1
• Database driver & version: MySQL
• Plugins & versions: Commerce 2.0.0-beta3

[lukeholder]

Submitting a zero qty is not valid, thus the error is raised.

Your PR suggests we should silently ignore invalidly submitted data?

[nfourtythree]

@lukeholder yes but in the case of a multi-add form there are going to be many quantity boxes. That is the nature of a form of that type.

For example in a form like this (in this screenshot the 0s are placeholders)

So without adding some JS, or hooking into a pre-add PHP event there isn't a way natively to stop those 0 qty rows adding (when I say 0 that means also null/blank).

[bossanova808]

Just chiming in here Luke - our multiadd discarded (silently) any zero quantities, and I agree that raising an error here is not the way to go...pretty much by design with these as plain forms (which should be supported) you submit every product with 0 unless the user has upped the qty for one of them.

[nfourtythree]

Glad to see you agree @bossanova808, it is quite a common pattern within e-commerce systems, and the way stated above is generally how I have seen it interpreted.

Any further thoughts on this @lukeholder, would just be good to get your thoughts so I know whether to wait out an update/release or employ a workaround for the project I am currently working on.

[lukeholder]

@nfourtythree @bossanova808 OK, after some thought and your feedback, we will allow submission of zero qty within a multi-add submission, and silently ignore the item. Will be in the next beta release.

Thanks.

[nfourtythree]

Great news @lukeholder!

Thank you for taking on board this feedback, it is a massive part of why I love using Craft and Commerce for projects that we have a chance to have this kind of dialogue. Hopefully making it better for everyone.

[bossanova808]

Hey Luke

Just (finally!) using this now. I think this should be taken one step further - at the moment, if a qty is not submitted at all, the item is still added to the cart.

This comes up where we, e..g use a checkbox for 'uspell items' alongside a main item. As an example, we actually submit:

action | commerce/cart/update-cart
purchasables[0][id] | 1385
purchasables[0][qty] | 1
purchasables[2][id] | 4700
purchasables[2][qty] | 1
purchasables[3][id] | 1302
purchasables[4][id] | 4711

...what I expect is that 1385 and 4700 are added to the cart, but 1302 and 4711 are not.

Basically, I think it is incorrect to default the qty to one personally...and would change
https://github.com/craftcms/commerce/blob/c8f46357701183036d8e3d25d2c018f3c106eab3/src/controllers/CartController.php#L150

To:
$qty = (int)$request->getParam("purchasables.{$key}.qty", 0);

...to match how other systems normally do this. Basically, only items that have both an id and an _explicit_ qty > 1 should ever be added to a cart.

What do you think?

[lukeholder]

Don't think assuming zero is the right way to do it. Would rather you be explicit with zero on multi-add or don't include the purchasable ID when the box is unchecked.

[bossanova808]

Home sick right now, but definitely don't think assuming one is explicit or correct.  Not how we implemented multi and not how I've ever seen another implementation do it.

(Without a value, assume zero is pretty much standard in CS, no?)

On 15 Oct 2018, 5:39 p.m., at 5:39 p.m., Luke Holder [e-mail] wrote:

Don't think assuming zero is the right way to do it. Would rather you
be explicit with zero on multi-add or don't inlcude the purchasable ID
when the box is unchecked.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/craftcms/commerce/issues/330#issuecomment-429725641

[lukeholder]

Submitting a purchasableId by itself should add to cart _without_ needing to specify you want to add a qty of 1. This has always been the way it works and no-one has reported this not making sense.

So the same logic is being applied to adding multiple purchasables in a post array - If you submit a purchasable ID in that array then you would expect it to add to cart.

Since you are using a checkbox it makes even more sense. You would set the checkbox to name="purchasables[{{loop.index}}][id]" and the value of the checkbox as value="{{purchasable.id}}".

Then if the checkbox is not checked then the ID is not even submitted to the server and we don't even need to check the qty.

[bossanova808]

Well Commerce 2 is kinda a massive breaking change anyway....I would argue it should not have been that way, really, and it's different from how twig, and php, and web forms all generally handle null/empty things (the bigger issue). Obviously I can implement it as you say, it's just a bit inconsistent, and seems like a sensible time to fix it.

But of course...up to you!

[sjelfull]

@bossanova808 I kind of agree with @lukeholder here.

If you don't want it added at all, don't include it.

If you want to explicitly remove it, submit 0 as quantity.

Both these examples are fine in the context of a web app. You mention this is different from how Twig, PHP etc. does it, but if an ID here represent a line item, I'd say that is the intention of the action. The quantity is just a model default.

As an aside, this is also how Shopify does it. So there is that.

[bossanova808]

@sjelfull Yep still don't personally agree but the ship has obviously sailed on this anyway, and I've gone ahead and modified my forms....I just think it's an unintended behaviours thing really, I prefer things to be very explicit. But I'm not losing sleep over it either!

(Note thought that 0 does not remove - it's just ignored (as per the conversation above)).

[lukeholder]

This recent commit made the purchasables[key].id data optional. This means you can have the purchasable ID be a checkbox field to optionally submit it to the cart:

30fe48f9aaaa97fcb8c7b800793f6ab827ffc705

Does that get you closer @bossanova808 ?