I just discovered this bug, while I can try and submit a patch for problem, I predict that this will require a deep look at several modules and might even cross some by design decissions so I want to hear some thoughts first, as it might be simpler for the developers to discuss/ fix this problem.

While using the Views bonus module for exporting stock information on a CSV I found that there were old information there (information that is not editable or available on any ubercart configuration screen). After looking at both the database and what the user has been doing, I realized that the stock database is not properly updated when something changes on either the attribute, options or adjustment pages of the product. The screens are with the new options or combinations or even SKU changes, but all the old stock information (of previous SKUs, or previous combinations) are still there.

What do you guys think? So far the quickest solution I found was to run a maintenance db query on cron run.

This is what I come up with as the maintenance query:

  db_query("DELETE ups FROM {uc_product_stock} ups LEFT JOIN {uc_product_adjustments} upa on ups.sku = upa.model LEFT JOIN {uc_products} up ON up.model = ups.sku WHERE isnull(upa.model) AND isnull(up.model)");
CommentFileSizeAuthor
#5 uc_stock-381464-5.patch496 bytesmarxarelli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hanoii’s picture

This is the proper query, the previous one was still leaving some orphaned information.

    db_query("DELETE ups FROM {uc_product_stock} ups LEFT JOIN {uc_product_adjustments} upa on ups.sku = upa.model AND ups.nid = upa.nid LEFT JOIN {uc_products} up ON up.model = ups.sku AND ups.nid = up.nid WHERE isnull(upa.model) AND isnull(up.model)");
cha0s’s picture

Status: Active » Postponed (maintainer needs more info)

I'm not sure if the correct solution is to remove that information from the database as you're doing, or to simply ignore it on a case-by-case basis. For instance Lyle recently suggested we should report SKUs that are no longer referenced by the adjustments table, even though they aren't sellable anymore, that sales info may be desired for tax reporting, etc.

So, I think your view is what should change, excluding stock on SKUs that meet the conditions here.

P.S. Where's the module you refer to in this issue? Could I get a link please?

hanoii’s picture

marxarelli’s picture

Old SKUs are retained in uc_orders_product.model from the time the record is created. There's your history.

Keeping old orphaned stock records doesn't seem to serve any purpose. For one, there's no time/date info that will give it relevance because you can't know when the records became orphaned—when the uc_product_adjustments record was deleted. Without date/time info, there's no application to tax records.

IMHO, this bug is valid.

As for when to delete the records, the best time would be when the adjustment is deleted. However, there's no hook for that, so maybe just do it in uc_stock_edit_form_submit():

<?php
function uc_stock_edit_form_submit($form_id, $form_values) {

  // ...

  // Delete orphaned stock.
  $skus = "('".implode("','", array_map('db_escape_string',
    uc_stock_skus($form_values['nid'])))."')";
  db_query("DELETE FROM {uc_product_stock} WHERE nid = %d AND sku NOT IN $skus",
    $form_values['nid']);

  drupal_set_message(t('Stock settings saved.'));
}

?>

Disclaimer: I haven't tested this yet. ;)

marxarelli’s picture

FileSize
496 bytes

I've done basic testing with the above implementation and it seems to work as expected. Here's a patch against the current drupal5 branch.

hanoii’s picture

This still happens on 6.x-2.x. I think something should be done here. I gave it a second thought because I ran into the exact same problem on a D6 site, meaning orphaned stock left in the stock db. I re-read #2 and although there might be something right there, there's nothing I can do on the view because I can't match those orphaned SKUs into nothing I can use on a view, even if I code it myself.

If orphaned SKUs are to be stored in the db on purpose for history (which I am not sure if this is really useful), there should be something there that indicates that the SKUs is orphaned so eventually, some filter can be applied to the views.

I also noticed something on the maintenance query which was wrong, as SKUs can come to the stock table from other places besides the produce adjustments, actually as per this function: $skus = uc_product_get_models($node);

So I think this is a valid bug or at least task to do because leaving records in the db with no real purpose are more harm than good.

Giving a thought to @marxarelli patch, I think that the cleaning should be done somewhere else, because there's a chance that SKUs are actually removed from the adjustments table (for instance, if new attributes are added) and the stock form may not be submitted at all, but I don't know if there's such a place. And because SKUs can come from different places, although I've never seen this happening, maybe something more global can be use for cleaning/setting SKU orphaned information.

I thought of nodeapi and found that there's none for uc_stock. Found #315585: nodeapi support for stock levels and customer viewable stock levels which is relatedto this, but I reviewed the patch there but I think it refers to more parts of the code than needed and I found a few bugs in that patch as well.

I was about to submit a patch with one but I realized that when adjustments/stock or any other ubercart product forms are saved there's no node_save, and then nothing we can hook on. Maybe executing a node_save (although I know it's unnecessary) would be a good idea so other modules (even ubercart core ones) can benefit? Or defining an productapi hook for product operations? Might be a bit too much but I think this is an issue and it might be good to follow it up a little.

For now, just a simple nodeapi for uc_stock at least for deletion might be good:

/**
 * Implementation of hook_nodeapi().
 */
function uc_stock_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  if (uc_product_is_product($node)) {
    switch($op) {
      case 'delete':
        // TODO: Decide if there should rather be kept or marked as orphaned (orphaned column?) ?
        db_query("DELETE FROM {uc_product_stock} WHERE nid = %d", $node->nid);
        break;
    }
  }
}
fenstrat’s picture

Version: 5.x-1.7 » 6.x-2.x-dev
Status: Postponed (maintainer needs more info) » Active

I'm struggling to think of a use case where stock data should be kept.

The stock level for a product is directly tied to that product. If a product is deleted then any stock levels it has should be removed as well.

Sounds straight forward, and certainly targetable as shown in uc_stock_nodeapi(), but issues arrise due to the joys of the non unique nature of SKU's.

rszrama’s picture

Status: Active » Postponed

"I'm struggling to think of a use case where stock data should be kept."

My standard reason has simply been that because Ubercart doesn't force you to use a unique SKU for every node and/or adjusted SKU, a particular SKU might exist in more than one place in the store. I suppose this can be accommodated if we check for the SKU's existence elsewhere before deleting it. However, at this point, I don't want someone relying on the fact that stock works the way it does to go delete a product w/ multiple SKUs and recreate it as separate products (a use case that even my example above doesn't accommodate) to find that the stock data is wiped. It seems like it'd be best to incorporate this change into a larger overhaul and document the differences in behavior from the (admittedly) lame current system.

fenstrat’s picture

Fair call. Let's see if we can get it right for 3.x

steve.m’s picture

In our case, we only use options and adjustments to track stock for different variations of items, so we definitely need stock tracking tied to options and SKUs

This has been my fix for removing stock tracking when an item's options are changed:

function MODULENAME_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case 'uc_object_options_form' :
      $form['#submit'][] = 'MODULENAME_uc_object_options_form_submit';
      break;
  }
}

function MODULENAME_uc_object_options_form_submit($form, &$form_state) {
  // determine extant SKUs
  $skus = array();
  $sql  = "SELECT model FROM {uc_product_adjustments} WHERE nid = %d";
  $result = db_query($sql, $form_state['values']['id']);
  while ($row = db_fetch_object($result)) {
    $skus[] = '"' . $row->model . '"';
  }
  // delete stock for absent SKUs
  if (!empty($skus)) {
    $sql = "DELETE FROM {uc_product_stock} WHERE nid = %d AND sku NOT IN (" . join(',', $skus) . ")";
    db_query($sql, $form_state['values']['id']);
    $deleted = db_affected_rows();
    if ($deleted > 0) drupal_set_message("$deleted orphan stock records deleted.");
  }
}

This works for us because if we have options, they have unique skus, and those skus have stock records.

hanoii’s picture

Issue tags: +uc_stock

adding tags

hanoii’s picture

#423262: Stock information not deleted with node related to this, evenmore, #16 seems like a good approach. This issue might still be useful to someone looking for a query to cleanup orphaned records left from at least the stock/adjustment screens.

hanoii’s picture

the query I tried to use here get's a lot more complicated when using revisions for products, I have come up with the following which seems to be working, use it carefully and I am sure it can be reworked better:

  db_query("CREATE TEMPORARY TABLE {tmp_uc_product_stock_orphaned} SELECT ups.sku FROM {uc_product_stock} ups LEFT JOIN {uc_product_adjustments} upa on ups.sku = upa.model AND ups.nid = upa.nid LEFT JOIN {uc_products} up ON up.model = ups.sku AND ups.nid = up.nid LEFT JOIN {node} n ON n.nid = up.nid AND n.vid = up.vid group by ups.sku, up.nid HAVING (SUM(IF(upa.model IS NOT NULL,1,0)) = 0 AND SUM(IF(up.model IS NOT NULL,1,0)) = 0) OR (SUM(IF(upa.model IS NOT NULL,1,0)) = 0 AND SUM(IF(up.vid IS NOT NULL,1,0)) > 0 AND SUM(IF(n.vid IS NOT NULL,1,0)) = 0)");
    db_query("DELETE FROM {uc_product_stock} WHERE sku IN (SELECT sku FROM {tmp_uc_product_stock_orphaned})");
micheleannj’s picture

Subscribing.
This is real bug if you're using Feeds to import products and update stocks as sometimes you need to delete all feed items and re-import which leaves you with a ton of old data in uc_products_stock that's never updated...
Trying the manual fix now.

sarhansg’s picture

How do I set up a db maintenance query on cron run? I'm using a cPanel hosting service.

I've also found a solution here.

TR’s picture

Component: Code » Stock
Issue tags: -uc_stock

Changing component.

hanoii’s picture

Adding #1398448: stock sku consistency on node update/delete as a related issue, which is a similar thing for D7.