This is going to sound silly, I know, but hear me out first (and at the very least I found a small bug that can be fixed).

I'm working for a client that uses the backend order admin interface almost exclusively (80% of their orders are entered manually, 20% come in via checkout). I set them up with a way of adding special fees for each product, which are displayed as products themselves under the product they are associated with in the product list of the order. Sometimes, the client wants to give a customer a discount by marking the quantity of the fee as "0". Unfortunately, Ubercart does not allow that.

I know what you're thinking... why not just delete the fee product? Or set it to a price of zero? Or add a line item that subtracts the price of the fee from the overall total. Trust me, that's what I said to my client. But they made a good point: with a quantity of zero, you can still show the customer that the fee WOULD have been applied, AND show them how much it was going to be (the unit price), but make it clear that you are NOT charging them that fee at all.

So, I was curious about where in the uc_order module the logic was that does not allow a quantity of zero. What I found, is that during the submit function for the order edit form, when it is updating the products (for example, after I set the quantity from 1 to 0), it only updates the product if "the product is not being deleted AND the product has a quantity greater than 0". Otherwise, it adds a message to the log: "[product title] has been removed."

Code:

if (!isset($product['remove']) && intval($product['qty']) > 0) {
        $product['data'] = unserialize($product['data']);
        $product = (object)$product;
        $order->products[] = $product;

        if (module_exists('uc_stock')) {
          $temp = $product->qty;
          $product->qty = $product->qty - $qtys[$product->order_product_id];
          uc_stock_adjust_product_stock($product, 0, $order);
          $product->qty = $temp;
        }
      }
      else {
        $log['remove_'. $product['nid']] = $product['title'] .' removed from order.';
      }

And herein lies the bug I mentioned earlier: Ubercart DOES NOT delete products from the order that have a quantity of zero. But it says that it does in the log. So if you edit an order, set a product quantity to 0, and then save, the order edit page will be reloaded and the quantity will NOT be set to 0 (so basically nothing happens). But then if you look at the order log, it says that the product was removed.

So... I got to thinking: it seems that Ubercart is assuming that a product with a quantity of zero would have been deleted somewhere else in the code, but it isn't. And I needed the ability to have products with a zero quantity, so why not just remove that one condition? That works fine and dandy, and now I can set quantities to zero in the order editor.

BUT: the subtotal is still thinking that there is 1 product in the order. Odd... why would it think that 0 products is actually 1 product? So I dug into the line item code that generates the subtotal, and found the following assumption:

'qty' => ($product->qty) ? $product->qty : 1,

... weird. Why would it assume that there is 1 product? Why not just assume that there are zero products, if $product->qty doesn't return anything?

So anyway, long story short, here's a patch that allows quantities of zero in orders.

Obviously we can debate whether or not this makes sense to be committed to Ubercart, but it helped me, so maybe the patch will help someone else too.

CommentFileSizeAuthor
#3 allow_zero_qty-1366576-2.patch2.64 KBm.stenta
#1 allow_zero_qty-1366576-1.patch2.68 KBm.stenta
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented
FileSize
allow_zero_qty-1366576-1.patch2.68 KB

(the patch)

Status: Needs review » Needs work

The last submitted patch, allow_zero_qty-1366576-1.patch, failed testing.

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented
Status: Needs work » Needs review
FileSize
allow_zero_qty-1366576-2.patch2.64 KB

Oops, just realized that the above patch undid a change made in another recent commit. My bad. Attached is a fixed version.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Category: bug » feature

I am not entirely convinced this is useful for many stores, but I guess if it only affects the order editing pages for admins then it could be committed.

The part I am most concerned about is

-        'qty' => ($product->qty) ? $product->qty : 1,
+        'qty' => ($product->qty) ? $product->qty : 0,

I wonder in what circumstances $product->qty could be 0/empty in the first place, to warrant defaulting it to 1, or if this is just defensive code that was never actually needed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

#3: allow_zero_qty-1366576-2.patch queued for re-testing.

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented

@longwave, re: defaulting to 1...

Yea, I'm not sure. I couldn't think of any reason why defaulting the quantity to 1 made sense... unless it was to prevent something weird from happening elsewhere. I'll do a quick search through the commit history to see if I can isolate when that code was added. Maybe there will be a clue...

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented

Here is the commit that initially adds that code: http://drupalcode.org/project/ubercart.git/commit/ee621888349b0ec55731e3...

And here's the diff specifically for uc_order.module: http://drupalcode.org/project/ubercart.git/blobdiff/b27a4c51d793cfdb5406...

The change in question is at the bottom of that diff.

I traced that change to this issue: #518916: Use uc_price types instead of locations

Specifically, it is added by a patch provided by Island Usurper in comment #28: https://drupal.org/node/518916#comment-2003008

The primary purpose of the change seems to be geared towards using uc_price() for calculating the order total. It still doesn't explain why defaulting the quantity to 1 would be necessary.

Looking at the uc_price() function (http://api.lullabot.com/uc_price) it seems that it also has some code in it for defaulting the quantity to 1. But I think that could stay there, and this change wouldn't affect it, because if a quantity of 0 is passed in, it would work and you would end up with a price of 0, as would be expected.

If this code was put in there defensively to prevent something somewhere else, there isn't any mention of it... it looks to me like it was just put in there with some other changes, without much reason, and has been there ever since. I think it would be safe to patch it.

Marc Angles’s picture

Marc Angles CreditAttribution: Marc Angles commented

Hi,

I'm scratching my head with a weird bug. All the packages quantities are now set to 0 in my website.

This is preventing us to save nodes and probably a lot of other issues we cannot see for the moment...

So I'm looking for code to correct this quickly.

Meanwhile, I found this discussion and raise my hand to ask why this was not pushed in the last release...

Now this patch does not apply anymore, and the fact that it was not committed makes me wonder if there is another issue preventing it from being committed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

This was not committed because it was a feature request, and the maintainers have not had time to fully test it yet.

This issue is about being able to use 0 as a quantity when editing orders; your question is about package quantities, which are very different - I suggest opening a new support request with full information, though I can think of no reason why they should all suddenly be set to 0.

TR’s picture

TR CreditAttribution: TR commented
Status: Needs review » Needs work

I personally don't think this feature should be added. It makes no sense from a business and accounting point of view. What you're trying to do is to give a discount equal to the cost of the product, and your client wanting to do this by setting the quantity to 0 is just a hack that's unjustifiable in the larger picture because the notion of selling "0" of an item makes no sense. How would you describe this order in English? You'd say "I'm giving you one of these products for free." You would NOT say "I'm selling you quantity 0 of this product for $9.99 each." The code should reflect the first sentence, not the second.

It's reasonable to default to 1 because when the admin adds a product to the order we can assume that the order contains at least one. The admin can increase the quantity if necessary, or set the quantity 0 to remove it from the order. (The fact that it doesn't actually remove it currently is a bug IMO - I think the message and the intent is clear that setting quantity to 0 means is *should* be removed. That's what happens on the /cart page for example ...) I think it's silly to allow the admin to set the quantity to 0 yet still keep that product in the order - that's not the way any other cart works.

From an accounting point of view, the product should be listed along with another line item removing the cost. Look at Amazon for instance - if you qualify for free shipping they show you the cost of the shipping then add a line item subtracting that shipping, which they do for the exact reason your client wants, which is to let the customer know how much he's getting for free. This is also how it *must* be entered in any accounting system - this is the entire basis of double entry accounting. There needs to be two line items because they have to be accounted for separately - the full price needs to be accounted for and reported under gross sales, and you might even have to remit sales taxes based on the full price, while the discount is categorized in business expenses. Maybe a little business that does their own books and doesn't adhere to the law or accounting standards can get away with this, but I don't see why Ubercart should change to make this possible.

Your client wants this, so I guess you should do it if he won't consider a different way, but this is not something we should enable for general use. If an easier way of making a product "free" is needed, I would vastly prefer something that was part of the discounts module, maybe an added checkbox on the create order page that allows the admin to designate a free product. But setting the quantity to 0 is not something most admins would try if they want to give a free product - they'd only do this if they wanted to remove the product from the order. It would create a usability problem if we did otherwise.

TR’s picture

TR CreditAttribution: TR commented

(double post)

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented
Status: Needs work » Closed (won't fix)

Fair enough. :-)

m.stenta’s picture

m.stenta
he/him
Primary language English
CreditAttribution: m.stenta commented
Issue summary: View changes

Added code.

millenniumtree’s picture

millenniumtree
he/him
Location Holualoa, HI
CreditAttribution: millenniumtree commented

On the other hand...

What if you're building a site that's integrated with a point of sale system where you want to process a 'return', but still have it show the product in the web order?
This is exactly the situation we have, and this inability to handle a zero quantity is incredibly annoying. I'm going to have to hack UC now to handle it.

Zero does not equal one.