Hi, I was wondering why the weight unit isn't also stored into the {uc_order_products} table in the database. I know it's stored in the {uc_products} table but for convenience it would also be nice to store it into the {uc_order_products} table. Just like it's done with weight, price and SKU/model.
Or am I missing something crucial why this isn't done?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave’s picture

Category: task » feature

This is a reasonable request, as if the product is deleted, the weight in uc_order_products makes no sense as the units become unknown. This would also simplify the fix for #679094: Creating Packages with Revisioned Products = Wrong order weight on UPS shipping label - Suggested Fix

longwave’s picture

It would probably also make sense to (optionally?) show the units on the product list when editing an order; when mixed weight units are in use, the weight column alone isn't immediately obvious.

garbo’s picture

Hi Longwave, I totally agree with your point (#2). In my case for instance my client sells vegetables that are sold per 200 gr and potatoes that are sold per 1.5 Kg. It is very common in this situation that sometimes grams are used and with heavy products Kilograms.

TR’s picture

Version: 6.x-2.4 » 7.x-3.x-dev

The data model is messed up, but I don't think the way to fix it is to denormalize {uc_order_products} further.

{uc_order_products} is a table that relates {uc_orders} to {uc_products}. The only reason weight, price, and model are being carried around in {uc_order_products} is to create a snapshot of those values as of the time the order was placed. If a product price is later altered, we want to be able to still have the historical order data - we need to know the price of the product when the order was placed, no the current price of the product.

But this is the wrong way to deal with this requirement. What needs to be done is to enforce versioning on {uc_products} so any changes to a product price, weight, model, weight units, or any other property will be stored in {uc_products} as a new version. Then we can drop the weight, price, model, title, etc. from {uc_order_products} because {uc_order_products} will be able to store relations between orders and *specific versions* of a product, and won't need to carry around all that duplicate data. All that should be needed in a normalized {uc_order_products} is oid, nid, and vid.

This also has the advantage of letting us simply create reports like product price as a function of time, and be able to recover what the price of a product was on a certain date, which are not possible in general with our current data model.

Because this involves a significant change in the code and the data, it should be addressed in 7.x-3.x first. It's unlikely to be backported to 6.x-2.x.

Island Usurper’s picture

I've written a hook before to ensure that a new revision is always made, so that's not a problem, but completely normalizing {uc_order_products} would require us to drop the Blank Line feature from the order-edit page. I also think you would be surprised how often there are exceptions on an order that require changes to the price, weight, or title of a product. Requiring a new revision of that product, and then an immediate revert isn't really feasible in any kind of sales workflow that I can think of.

garbo’s picture

I understand the point made by TR and I think it's valid. But even if the weight (and price or SKU) are just there for snapshot reasons the weight number without the unit is useless for any purpose. So I think a choice has to be made between A: removing the weight number entirely from uc_order_product (please don't!) or B: make it useful by adding the unit.

This could maybe done for 6.x while for 7.x.3.x a normalized approach is being worked out. Then when it seems possible and there are still enough users of 6.x a backport could be made after the normalized solution for 7.x has proven it self.

longwave’s picture

Assigned: Unassigned » Island Usurper

Uhm, so I guess this was implemented in http://drupalcode.org/project/ubercart.git/commitdiff/1645d08 ?

Island Usurper’s picture

Assigned: Island Usurper » Unassigned
Status: Active » Needs review

*headdesk* Didn't mean for that to happen. And now the order edit form won't save the product weights correctly. I was going to make a patch of it once I'd worked that out, after releasing rc1. Guess I got into a rush. Sorry. :(

So here's the explanation for what that code is. While I don't know for sure that it will be back-ported to 6.x-2.x, I did write the update function to allow for it. The weight_units column is added to {uc_order_products}, and then a batch process updates them to have the current weight_units of the referenced node. Any order products with a 0 nid get the variable 'uc_weight_unit', which defaults to 'lb'.

Packaging queries were modified to get the weight units from {uc_order_products}, and the uc_order_edit_products_form() is supposed to allow managers to change the weight units of ordered products, but I didn't change the submit function. The changed rule at the end didn't look like it worked, so that change is to help make sure it can test these changes out.

Looks like rc2 will be out sooner than expected, either to fix the order form, or to take these changes out. I'd rather fix the form because we can't normalize to different versions on the {uc_products} table and keep all of the features we have.

Island Usurper’s picture

FileSize
655 bytes

Fortunately a simple fix. Going to go beat my head against my desk some more.

longwave’s picture

Not sure we need such a complicated, batched hook_update_N function, I think it could be done with a single SQL statement? But that's not that important, if it works.

Alternative patch to #9 attached; the current table layout doesn't look good so why not just add a new column entirely?

Island Usurper’s picture

I went with a batch because there can easily be several thousand product nodes and hundreds of thousands of ordered products. Yeah, a single SQL statement would work, but either a timeout or memory limit would hit somebody. I was trying to follow what core does when it updates the node table.

longwave’s picture

Status: Needs review » Fixed

Committed #10 for now, as it fixes the order edit bug and looks better than #9 to me. The display can be improved later if necessary.

Status: Fixed » Closed (fixed)

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