The issue:
Updating SKU in product edit page leads to old SKU displaying in stock reports. This also potentially may lead to incorrect stock valued being displayed for custom written modules and uc_stock itself (e.g. uc_stock_is_active($sku) & uc_stock_level($sku) functions)

How to reproduce:
1. ubercart 6.x-2.x-dev
2. uc_stock activated
3. At least one product with stock activated already created
4. Edit that product and change SKU
5. Go to Stock tab (in product edit form) and activate stock for new SKU
6. Stock reports will have old SKU and new SKU listed, linked to the same product (which is a bug)

Solution:
Implement hook_nodeapi in uc_stock module to handle product update and delete (via product edit form).
The code below should be added to the bottom of uc_stock.module. Sorry for no patch - don't have access to CVS repo.

/**
 * 
 * Implementation of hook_nodeapi
 * 
 * This is required to remove old stock information on product delete and SKU change (during product update process)
 * Otherwise all this information still coming up in stock reports
 */
function uc_stock_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
	// If node is not product - exit here
	if(uc_product_is_product($node) !== TRUE) return;
	 
	switch($op) {
		case 'delete':
			uc_stock_remove_old_sku($node->nid);
			break;
		case 'presave':
			if($node->nid > 0) { // Only work with existing nodes - not new ones
				$oldSkus = uc_stock_get_sku($node->nid);
//				var_dump($oldSkus); var_dump($node->model); exit;
				if($node->model != $oldSkus) {
					uc_stock_remove_old_sku($node->nid);
				}
			}
			break;
//		case 'update':
//			break;
		case 'load': // Just for programming convenience adding stock to $node object
			$stock = uc_stock_level($node->model);
			return array('stocklevel' => $stock);
			break;
	}
}

/**
 * Getting SKU from ubercart (not Stock module) tables based on node id of the node being edited
 * @param $nid: node id (int)
 * @return string SKU
 */
function uc_stock_get_sku($nid) {
	$query = db_query("SELECT model FROM {uc_products} WHERE nid = %d LIMIT 1", $nid);
	$sku = db_fetch_object($query);
	
	return $sku->model;
}

/**
 * Deleting SKU data from Stock module tables
 * @param $nid
 * @return bool mysql result
 */
function uc_stock_remove_old_sku($nid) {
	return db_query("DELETE FROM uc_product_stock WHERE nid = %d", $nid);
}
CommentFileSizeAuthor
#13 uc_stock_sku_change-737790-13.patch1.42 KBm.stenta
#6 ubercart-DRUPAL-6--2.patch1.94 KBivrh
#4 ubercart-DRUPAL-6--2$.patch2.35 KBivrh

Comments

tr’s picture

tr commented
Status: Needs review » Needs work

Marking #613424: SKU - Not Updating and #602196: Sku doesn't change in uc_product_stock as duplicates of this issue. Any fix here should check to see that it addresses any points raised in those other threads.

Moving to "needs work" because 1) there's no patch here yet, and 2) need to check fix against those other issues.

giorgosk’s picture

giorgosk
Location Chios, Greece 🇬🇷
commented

please see this issue patch and review #745912: Stock values lost when SKU is altered

ivrh’s picture

ivrh commented

Have checked all other issues mentioned and checked the one in #2 (the closest one). Solution proposed there is partial as it does not handle deletion of products.
My functions above are complete addition to the uc_stock module and can be simply entered at the end of the file.
This solution has also been tested in commercial and live store, proven to work.

I do not have CVS/ubercart setup so cannot provide patch against head version.

ivrh’s picture

ivrh commented
Status: Needs work » Patch (to be ported)
StatusFileSize
new ubercart-DRUPAL-6--2.patch2.35 KB

Submitting patch. This patch should fix issues outlined in #2 and I am not sure about the rest, but it looks to me like additional bugs.

tr’s picture

tr commented
Status: Patch (to be ported) » Needs work

You should use "needs review" for patches that need review. "to be ported" is for patches that have been committed to one version of a project and need to be backported to a previous version.

I haven't tested your patch yet, but a quick glance shows it has coding standards issues. See http://drupal.org/coding-standards

ivrh’s picture

ivrh commented
Status: Needs work » Needs review
StatusFileSize
new ubercart-DRUPAL-6--2.patch1.94 KB

Updating:

The duplicates outlined in comment #1 (1st one) seems to be the same issue with stock reports displaying old SKUs. The issue there is closed. Second issue in #1 seems the same, but with not much information and is closed as well.

Comment #2 seems to outline the same issue and provides patch which provides only partial solution.

My patch is complete solution against this stock issue and the patch attached is against the latest dev version dated August 13 2010.

I am re-uploading fixed patch as in #4 taking into account suggestions in #5.

Testing scenario:
1. Create a product and give it SKU
2. Save product and visit stock tab
3. Activate stock and enter any number of products in stock, save.
4. Open stock report and ensure your new product is there showing correct stock value and SKU
5. Visit this product page again and edit it. Give it different SKU
6. Save, visit stock tab. Activate stock and enter number of items in stock. I advise to enter number different to p.3. Save.
7. Visit stock report page and ensure that your product as in p.4 has new SKU and displaying correct stock amount as in p.6.
8. Ensure the old SKU entered in p.1 is not appearing in stock report.

tr’s picture

tr commented
Status: Needs review » Needs work

You need to get rid of the tabs in your patch.

Your new functions uc_stock_get_sku($nid) and uc_stock_remove_old_sku($nid) both assume that each nid has one and only one SKU. That is not the case. Every attribute/option combination can be assigned its own SKU via the Adjustments tab on the product edit page. So in general there are many SKUs for each nid, with stock possibly being tracked separately for each SKU (depending on the "active" checkbox in the Stock tab).

But the big problem I see is that you're doing nothing to ensure database integrity for old orders. Orders already in the database using the old SKUs will refer to non-existent SKUs once you've changed a product SKU. That's not good.

I might go so far as to say that, because a SKU uniquely identifies a product, the SKU should never be changed. Instead, you should create a new product with a new SKU, even if it's largely a duplicate of the previous product. In which case, the solution to this problem would be quite different - simply don't allow a SKU to be changed once set.

ivrh’s picture

ivrh commented
Status: Needs work » Closed (won't fix)

I see now. From your description and in particular to keep tracks of old orders I don't see a good solution except of not allowing SKU change once product submitted. My patch is only usable for products without attributes (and this is exact usage scenario we have at the moment for shopping carts with this patch applied).

frost’s picture

frost commented

i have a live system and we've experienced similar issues. Am willing to test a patch when one is available. Note however that we aren't using attributes or options so our test wouldn't cover all scenarios.

sorry, i just noticed the status is closed/won't fix so i guess there won't be a patch! ;-)

SpiesInOrbit’s picture

SpiesInOrbit commented

subscribe

The order.module patch ubercart-DRUPAL-6--2.patch worked for me, but I don't have attributes turned on...yet. Not sure I understand the "won't fix".

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

Probably related to #381464: Orphan stock information in the database leading to wrong reports

Mellonedain’s picture

Mellonedain commented

Hi!

I detect right solution: Create all relation based on node id.
For full fix we need to change primary key in table us_products_stock to column nid (not sku).

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Component: Code » Stock
Assigned: ivrh » Unassigned
Status: Closed (won't fix) » Needs review
StatusFileSize
new uc_stock_sku_change-737790-13.patch1.42 KB

Attached is a new patch which takes the same approach as the previous patches, but simplifies and tries to address some of the issues outlined by TR in comment #7.

Note, this issue also depends on the patch that I submitted in comment #10 in #1637336: UC Stock pages display wrong SKU, it seems to be ignoring revisions. You should apply both if you need the ability to change SKUs.

What this patch does:

When a product node is updated, it checks to see if the PRIMARY SKU (aka model) of the product has changed. If so, it makes sure that the new SKU is reflected in the product's PRIMARY stock record in {uc_product_stock}. By "PRIMARY", I mean the SKU that is editable in a product's node edit form (node/%/edit) (in other words, it doesn't touch SKUs created in the "Adjustments" tab of a product).

It looks for the model by revision id, so it should find the most recent one properly. But don't forget to also apply the patch from comment #10 in #1637336: UC Stock pages display wrong SKU, it seems to be ignoring revisions.

What this patch does NOT do:

It does NOT do anything to "Adjustments" SKUs of a product (non-primary SKUs that are assigned to specific combinations of attributes and options). This functionality can be added separately if necessary, or added to this patch if someone else needs it.

It also does NOT do anything to ensure database integrity for old orders. As such, the patch is provided for folks who are aware of this limitation, and need to change their SKUs (this is the case for me, because the workflow we have developed involves changing the SKU of products BEFORE they are published and before orders are created with them).

I would like to understand where the database integrity issues would arise. I don't have a great understanding of all the pieces, so it would be helpful if someone who does explain it here for everyone's benefit. Then maybe this patch can be improved and, ultimately, merged into HEAD.

Also, as for D7... it should be pretty much the same... with a few changes:

  • Change hook_nodeapi() to hook_node_update()
  • Use D7's database API methods.
  • Test to see if $node->old_vid is still available in D7. If not, find an alternative.