I can choose any value I want on the cart page, and the only point at which I'm alerted to an out-of-range value is when I try to go to the checkout page. I'm not sure what the right strategy is, given Ubercart's preponderance of hooks, but it seems like you should be able to add a custom validation function via FAPI.

CommentFileSizeAuthor
#2 921850-uc_product_minmax_cleanup-2.patch19.77 KBneilnz
#1 uc_product_minmax_cleanup_0.patch18.61 KBHorsePunchKid

Comments

HorsePunchKid’s picture

HorsePunchKid commented
Status: Active » Needs review
StatusFileSize
new uc_product_minmax_cleanup_0.patch18.61 KB

Attached is a patch that fixes this problem and numerous others. I'm sorry it's not a more targeted patch, but it was difficult to get done what I needed without touching a lot of the rest of the code. Here are some of the changes:

* Prevent "Add to cart" from working if it would cause illegal quantities in the cart.
* Allow a minimum of zero (i.e. no minimum)
* Significant collapsing and refactoring of redundant code (20 fewer lines despite increased functionality), e.g. a function to load minmax values based on a nid
* Better error messages
* Proper indentation and Drupal code conventions

It needs more testing, but it seems to be working well for me.

neilnz’s picture

neilnz commented
StatusFileSize
new 921850-uc_product_minmax_cleanup-2.patch19.77 KB

I had a couple of issues with your patch, so here's a revised one with:

  • In hook_add_to_cart() it increments the quantity counter by one for each matching product it finds in the cart, instead of by $item->qty, so if there's 5 in the cart with a limit of 5, and you try to add one more, it allows it (because it counts the 5 in the cart already as 1)
  • Adding a list as the failure message doesn't look very nice when there's just one message (theme-dependent). I've changed it to it only uses a list if there's more than one error message (which should be rare)

Otherwise nice patch, thanks!

arski’s picture

arski commented

this is awesome!

Can I make a related suggestion - maybe it could be possible to remove the quantity field completely if min=max, i.e. if one is only allowed to buy X of one item, not more not less - the field and the whole validation stuff then becomes kind of pointless.

Otherwise this is great! :)

Cheers

arski’s picture

arski commented

hey again,

First of all, there is a line like "$message[] = 'test';" in the patch, which should really not be there as it prevents any kind of addition to the cart.

Another thing is that sadly this patch doesn't do it for me :( I can still update the cart with invalid quantities, and they only get caught when I proceed to checkout.

Thanks in advance for fixing this!

Cheers

arski’s picture

arski commented

Hey,

found the issue: in the new function uc_product_minmax_cart_validate, you're 3 times referring to
$minmax->multiple
whereas it should be
$minmax->pmin_multiple

Do you mind fixing that please as I'm too lazy to create two instances to do a new patch.. thanks! :)

fred0’s picture

fred0 commented
Status: Needs review » Fixed

Thanks all for the patches and reports.
I implemented these changes and fixed a few minor issues as noted and that I found..
Additionally, the uc_product_minmax_form_uc_cart_view_form_alter function from HorsePunchKid's patch works for both cart update and checkout and made the original (from the 2 UC 1.0 modules I merged to make this one) cart validation code redundant. I removed that older code so, please test and let me know it works for you.

fred0’s picture

fred0 commented

All,

I am working on the D7 port and have a couple questions.
First, what is the purpose of the a minimum of zero? I implemented it in D6 with the patch, but I'm not sure why anyone would want that.
Second, to arski's suggestion that we remove the qty field on the product page if min=max, since that would also have to rely on the uc default add to cart value, does anyone have an opinion on if this is truly a useful feature (it would have to work if min=max=default add to cart) or is does anyone think that having to set a value outside the product feature interface just add confusion?

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.