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

Add multiple products to cart via items-to-cart query arg. Closes #17477, Closes #16742 #17855

Closed

Conversation

@jtsternberg
Copy link
Contributor

@jtsternberg jtsternberg commented Nov 22, 2017

Adding multiple products:

?items-to-cart=1&add[<product1-id>]=<product1-qty>&add[<product2-id>]=<product2-qty>

Adding multiple variable products looks the same as the above, except
replace product-id with variation-id.

Adding multiple grouped products looks like:

?items-to-cart=1&add[<group-id>][<groupproduct1-id>]=<groupproduct1-qty>&add[<group-id>][<groupproduct2-id>]=<groupproduct2-qty>
…erce#17477

Adding multiple products:
```
?items-to-cart=1&add[<product1-id>]=<product1-qty>&add[<product2-id>]=<product2-qty>
```

Adding multiple variable products looks the same as the above, except
replace product-id with variation-id.

Adding multiple grouped products looks like:
```
?items-to-cart=1&add[<group-id>][<groupproduct1-id>]=<groupproduct1-qty>&add[<group-id>][<groupproduct2-id>]=<groupproduct2-qty>
```
@jtsternberg jtsternberg changed the title Add multiple products to cart via items-to-cart query arg. Closes #17477 Add multiple products to cart via items-to-cart query arg. Closes #17477, Closes #16742 Nov 22, 2017
@jtsternberg
Copy link
Contributor Author

@jtsternberg jtsternberg commented Nov 22, 2017

So there's a few @since X.X.X tags which need to be updated. Also, the query variable names can obviously be changed if needed.

@codecov
Copy link

@codecov codecov bot commented Nov 22, 2017

Codecov Report

No coverage uploaded for pull request base (master@16152bf). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17855   +/-   ##
=========================================
  Coverage          ?   34.84%           
  Complexity        ?    12412           
=========================================
  Files             ?      342           
  Lines             ?    50875           
  Branches          ?        0           
=========================================
  Hits              ?    17725           
  Misses            ?    33150           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
includes/class-wc-form-handler.php 0% <0%> (ø) 303 <66> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16152bf...07d617c. Read the comment docs.

@mikejolley
Copy link
Member

@mikejolley mikejolley commented Dec 14, 2017

Was the _multiple method required? It would be nice to have more reuse but can understand if thats not possible.

One ideas would be to keep the current add-to-cart=x param but support an array/comma separated values. And then rather than assume everything in the query string is for 1 item, make the data array based also.

So for variations as an example, add-to-cart=x would be the variation ID, and then supporting data could be passed in a numeric array separately. So for all add to cart events you'd have something like add-to-cart=10,data[10]=allsupportingdatahere. Perhaps even the same for qtys. It would of course need to support old structure and new structure but it might allow more reuse?

Just a thought. I can look in more detail next week if that doesn't make sense.

@jtsternberg
Copy link
Contributor Author

@jtsternberg jtsternberg commented Dec 18, 2017

@mikejolley I understand what you're saying, and may have defaulted to that approach, but I incorrectly assumed based on the other issue that a separate query var was preferable. It, of course, could be refactored to work the way you mention, but I will not have time to get back to this until next month some time.

@jtsternberg
Copy link
Contributor Author

@jtsternberg jtsternberg commented Dec 18, 2017

Update: I didn't actually assume it, but used a separate query var based on your feedback: #17477 (comment) :) (I totally get that determing the optimal solution as a maintainer is not a simple problem, so no condemnation, but figured it would be helpful to explain my reasoning)

@mikejolley
Copy link
Member

@mikejolley mikejolley commented Jun 27, 2018

FYI started refactoring form classes (#20607) to make these handlers cleaner.

@mikejolley mikejolley removed this from the 3.5.0 milestone Jun 27, 2018
@mikejolley mikejolley added this to the 3.6.0 milestone Jun 27, 2018
@mikejolley mikejolley removed this from the 3.6.0 milestone Jan 9, 2019
@mikejolley mikejolley added this to the 4.0.0 milestone Jan 9, 2019
@mikejolley mikejolley added this to Refactor in Next major release Todos Mar 6, 2019
@mikejolley
Copy link
Member

@mikejolley mikejolley commented Mar 6, 2019

This PR is old/stale, blocked, and will require a major update to ship. I'm closing it and making a note on the 4.0 todo board that this will need revisiting/recoding in the future.

@mikejolley mikejolley closed this Mar 6, 2019
@mikejolley mikejolley removed this from the 4.0.0 milestone Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants