It appears that a node's information in {uc_product_stock} is not deleted when the node is deleted. This caused some problems when our client created a product which used a particular SKU, then deleted that node and created a new one which used the same SKU. The stock editing page (node/%node/edit/uc-stock) shows stock info for the new product and altering the values seems to work, but the product doesn't show on the stock reports page (admin/store/reports/stock).

Simply deleting the old information for that SKU from the database table fixed the problem.

Pardon the lack of patch, but I'm afraid I'm not aware enough of just how the stock system works to know where this should go.

CommentFileSizeAuthor
#6 delete_stock.patch1.34 KBIsland Usurper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Interesting... so, maybe this is a bug on the report itself? I don't think stock information should just be deleted when a node is deleted, because multiple nodes can have product SKU variations that use the same SKU. This is part of the weakness of our 1:many node:product relationship.

cha0s’s picture

Assigned: Unassigned » cha0s
Status: Active » Needs review

Well... I guess we have a couple options.

1) Make nid a primary key too, so that we can have more than one nid per SKU. That would decouple different nodes from the same SKU. This is only okay if we don't support 'linking' nodes' stock together who have the same SKU. I support this way.

2) Otherwise, if we need to let multiple nodes share the same SKU, we should probably overwrite the shared stock's nid with the node who was last edited. IMO this is nasty...

Vetkhy’s picture

I think it is a real bug, becasue, e.g., we deleted node with some SKU, and after than we want to add another node with this SKU, so we can't manage stock for new node.

asak’s picture

I may be encountering the same issue here... not sure since not heavily tested.. but i had problems with the stock, disabled stock modules, re-imported products (same SKU..), and now the products still show "out of stock" even though stock module is off...

does that even make sense...? ;)

Vetkhy’s picture

asak, try to clear uc_product_stock table manually or uninstall stock module.

Island Usurper’s picture

Version: 6.x-2.0-beta5 » 6.x-2.0-rc3
Assigned: cha0s » Island Usurper
FileSize
1.34 KB

Though I want SKUs to be unique across products eventually, that's not something I want to change right now. So I propose a third option: if the deleted product is the only one with a particular SKU, then delete the stock data. But if another product has the same SKU, then make its nid the one on that row of the table.

Technically, this patch isn't complete because it doesn't check the other nodes' extra SKUs from the uc_product_adjustments table. It's just really hard or resource intensive to check that the deleted node's adjusted SKUs are different from all of the possible SKUs other nodes have.

As long as no products share SKUs at the same time, there shouldn't be any problems, so I want that to be considered a best practice until that actually gets enforced by the code.

j0rd’s picture

I also do not believe the stock information is deleted, when you update a products SKU. This might want to be another case you look into as well. Perhaps best solution is to move current stock of the product to new SKU.

RSTaylor’s picture

I would suggest keying products by nid AND sku. That way you can have multiple skus per product and multiple products with the same sku, and each combination would have its own database entry so this should no longer be a problem.

Initially I too thought that SKUs should be unique, but now I see the fact that they don't have to be as a great feature. It means that you can have multiple sellers/suppliers and don't have to worry about what to do if two products from different suppliers have the same SKU...except that as it is currently, your stock reports will be mixed up.

j0rd’s picture

Makes sense Taylor. Allowing allowing multiple SKU's is important if you have two sellers, who might share the same SKU's on different products. Think ubercart_marketplace.

Island Usurper’s picture

Status: Needs review » Needs work

So we need to do something else, then?

j0rd’s picture

Alright.

I've run into this problem again with another project running the most current version of Ubercart. =(

I really think we should be tracking stock over NID and SKU. This can be done in the future...but here's what needs to get done now, to fix this bug.

We have a bug, where once an SKU and stock is added into the database it is never removed. It is not removed when the product is deleted. It is not removed if the product changes it's SKU to something else. In the case of updating, the new ones are added and the old one is left to sit "stale"...even though it no longer relates to the product.

If you go ahead and create another product using a "stale" SKU which already exists in the database, it's stock entry will never get added into the uc_product_stock table. This has come up in all my projects, in which my clients actually use stock. We need to resolve this critical bug in the stock system.

ANother Related Issue
It also appears, that if you update a product and change it's SKU, this new SKU is not added into the uc_product_stock table and the old SKU is left in there. This is another bug which is related to our stock issue here and is again causing issues with the end users of my websites.

From the end users perspective
I'm going to have to fall back to Dries comment on the 80/20 Pareto principle. I would assume that 80% of end users who are configuring their store, are not interested in having two products share the same SKU, but occasionally, they will mess up and this will happen. In my opinion, we should warn them that this is going on, or as Island Usurper mentions in #6, enforced in code.

TR’s picture

Issue tags: +uc_stock

Tagging

GiorgosK’s picture

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

davidw’s picture

Thanks for hashing this out.
It now makes much more sense. Currently, when I have to update a product SKU, I zero the stock quantity, disable the stock tracking for that SKU, and then delete the node, and re-create it with a different SKU :( Crazy, I know but I had a bad situation with an important customer that can not be repeated, and it's the only way I could be sure that my product SKU and stock information were correct.

TR’s picture

joanpc’s picture

There are plenty of issues on the same subject. Without saving the attributes on the stock table there's no way to change sku names without losing the stock levels.

So by the moment the best way to proceed IMHO is to delete all the orphaned stock rows in this 3 cases:
1 when a node is deleted
2 when a node sku is updated
3 when new attributes sku are entered in the adjustments pane.

I wrote the following code: (just add it to the end of uc_stock.module)

/*
 *  Implementation of hook_nodeapi
 */
 
function uc_stock_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  if (! uc_product_is_product($node)) return;
  switch ($op) {  
    case 'delete':
        db_query("DELETE FROM {uc_product_stock} WHERE nid = %d", $node->nid);
    break;
    case 'update':
        uc_stock_delete_orphaned($node->nid, true);
    break;
  } 
}

function uc_stock_form_alter(&$form, $form_state, $form_id) { 
  if ($form_id == "uc_product_adjustments_form") {
      $form['submit']['#submit'] = array('uc_product_adjustments_form_submit', 'uc_stock_product_adjustments_form_submit');
  }
}

function uc_stock_product_adjustments_form_submit($form, &$form_state) {
    uc_stock_delete_orphaned($form_state['values']['nid']);
}

/*
 *  Delete orphaned stock entries
 */
function uc_stock_delete_orphaned($nid, $reset = NULL) {
      $node = node_load($nid);  
      $skus = uc_product_get_models($node, NULL, $reset);
 
      // Get skus in uc_product_stock
      $q = db_query("SELECT sku FROM {uc_product_stock} WHERE nid = %d", $node->nid );
      // Check for orphaned skus and delete them
      while ($stock = db_fetch_array($q)) {
        if (!in_array($stock['sku'], $skus)) db_query("DELETE FROM {uc_product_stock} WHERE nid = %d AND sku = '%s'", $node->nid, $stock['sku']);
      }
}
hanoii’s picture

hanoii’s picture

I think #16 looks quite right, thoughts?

mirocow’s picture

subscribe

ratinakage’s picture

subscribe..

PieterDC’s picture

I just added

 /**
 * Implementation of hook_nodeapi().
 */
function uc_stock_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  if ($op == 'delete' && uc_product_is_product($node->type)) {
    db_query("DELETE FROM {uc_product_stock} WHERE nid = %d", $node->nid);
  }
} 

But after reading this thread, I'm sure that I'm missing some use cases :-(

j0rd’s picture

@PeiterDC is this in Drupal 6 or Drupal 7 Ubercart?

PieterDC’s picture

@jOrd Drupal 6

sarhansg’s picture

#16 worked perfectly for me. Will continue testing and update you if further issues arise.

smartango’s picture

I have this problem in drupal 7, does

function uc_stock_node_delete($node) {
  db_query("DELETE FROM {uc_product_stock} WHERE nid = %d", $node->nid);
} 

could fix? or is a wanted behaviour? (want to be 7ish using db_delete?)

smartango’s picture

be warned on this: http://drupal.org/node/1386828 sku changes!

TR’s picture

Component: Code » Stock
Issue tags: -uc_stock

Changing component.

lcampanis’s picture

#16 is the way to go.

Update the SKU, then Set the Stock tracking to active and update the stock.

Hopefully this will be implemented properly by Ubercart at some point. :)

kopeboy’s picture

Issue summary: View changes

is #16 working on Drupal 7 also?

smartango’s picture

for Drupal 7 see the patch at #1398448-22: stock sku consistency on node update/delete
it solve inconsistency and

TR’s picture

Version: 6.x-2.0-rc3 » 8.x-4.x-dev
Assigned: Island Usurper » Unassigned