stock does not update neither remove entry from its table when node update sku or node is removed.

adding these function to uc_stock.module make stock table consistent


/**
 * Implements hook_node_delete().
 */
function uc_stock_node_delete($node) {
  db_delete('uc_product_stock')
    ->condition('nid',$node->nid)
    ->execute();
}

/**
 * Implements hook_node_update().
 */
function uc_stock_node_update($node) {
  db_update('uc_product_stock')
    ->fields(array('nid'=>$node->nid))
    ->condition('sku',$node->model)
    ->execute();
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smartango’s picture

Sorry, this is the correct one:


/**
 * Implements hook_node_delete().
 */
function uc_stock_node_delete($node) {
  db_delete('uc_product_stock')
    ->condition('nid',$node->nid)
    ->execute();
}

/**
 * Implements hook_node_update().
 */
function uc_stock_node_update($node) {
  db_update('uc_product_stock')
    ->fields(array('sku'=>$node->model))
    ->condition('nid',$node->nid)
    ->execute();
}
smartango’s picture

sorry again, update does not work with adjustments. Could be added only delete?

smartango’s picture

this for update?


/**
 * Implements hook_node_presave().
 */
function uc_stock_node_presave($node) {
  $node->old_model = $node->model;
}

/**
 * Implements hook_node_update().
 */
function uc_stock_node_update($node) {
  db_update('uc_product_stock')
    ->fields(array('sku'=>$node->model))
    ->condition('sku',$node->old_model)
    ->execute();
}
smartango’s picture

Whole change (tested) on delete and update:


/**
 * Implements hook_node_delete().
 */
function uc_stock_node_delete($node) {
  db_delete('uc_product_stock')
    ->condition('nid',$node->nid)
    ->execute();
}

/**
 * Implements hook_node_presave().
 */
function uc_stock_node_presave($node) {
  if(isset($node->nid)) {
    $n = node_load($node->nid);
    $node->old_model = $n->model;
  }
}

/**
 * Implements hook_node_update().
 */
function uc_stock_node_update($node) {
  if(!isset($node->old_model)) return;
  db_update('uc_product_stock')
    ->fields(array('sku'=>$node->model))
    ->condition('sku',$node->old_model)
    ->execute();
}

the test is:
1. create product with sku
2. activate stock level setting to a number (say 1)
3. change product sku and update
4. look at stock level if it is still to 1
passed

longwave’s picture

If you could write that test in code as a SimpleTest that would be awesome - it would prove that this works, and ensure that it is never broken again in the future.

longwave’s picture

One thing your code may not take into account: what if the new SKU already exists in the stock table, attached to another product or an attribute combination of the same product?

TR’s picture

Isn't this a duplicate of a number of other stock issues, such as #315585: nodeapi support for stock levels and customer viewable stock levels? Any proposed fix needs to be aware of those other issues and the concerns raised there.

smartango’s picture

what shoud be done? check and report to original model if it exists in stock table?

/**
 * Implements hook_node_presave().
 */
function uc_stock_node_presave($node) {
  if(isset($node->nid)) {
    $n = node_load($node->nid);
    $r  = db_select('uc_product_stock','stock')
         ->fields('stock')
         ->condition('sku',$node->model)
         ->fetchAssoc();
    if($r && $node->model != $n->model) $node->model = $n->model;
    else $node->old_model = $n->model;
  }
}

I have to check and test this code.

smartango’s picture

@TR patches at #315585: nodeapi support for stock levels and customer viewable stock levels are not correct for me, it update stock table for all entry with this nid, but there could be attributes combination which lead to the same nid and differents sku. (also those use nodeapi which is not available in drupal 7)

But definitively it is related to.

smartango’s picture

I miss execute(). This is the whole change set


/**
 * Implements hook_node_delete().
 */
function uc_stock_node_delete($node) {
  db_delete('uc_product_stock')
    ->condition('nid',$node->nid)
    ->execute();
}

/**
 * Implements hook_node_presave().
 */
function uc_stock_node_presave($node) {
  if(isset($node->nid)) {
    $n = node_load($node->nid);
    $r  = db_select('uc_product_stock','stock')
      ->fields('stock')
      ->condition('sku',$node->model)
      ->execute()
      ->fetchAssoc();
    if($r && $node->model != $n->model) {
      $node->model = $n->model;
      drupal_set_message(t('SKU in use, not updated'));
    } else $node->old_model = $n->model;
  }
}

/**
 * Implements hook_node_update().
 */
function uc_stock_node_update($node) {
  if(!isset($node->old_model)) return;
  db_update('uc_product_stock')
    ->fields(array('sku'=>$node->model))
    ->condition('sku',$node->old_model)
    ->execute();
}

tested
I think one would prefer to add validation to form for sku.

Anyway this works and is a step forward.

smartango’s picture

Status: Active » Needs review
FileSize
1.43 KB

the patch

smartango’s picture

a patch with test of stock consistency.

@longwave on comment 6, consistency in this patch do not take consider attribute module, for that there is another issue opened, but this patch (or something that pass the test) has to be included before, or I can make a whole patch for all, but someone suggested me to divide thing in two parts (sorry, I cant recall), the other part is in http://drupal.org/node/1415590

maksim24’s picture

What about in 6.x-2.10 version?

smartango’s picture

Priority: Normal » Major

maksim24 try http://drupal.org/node/315585 , we are very lazy on applying this kind of patch, not great interest on this, I think.

I raise priority, if it does not hurt to much

debo7debo’s picture

It didn't work for me, so I queued it for retesting.

#12: ubercart-stock_consistency-1398448-12.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, ubercart-stock_consistency-1398448-12.patch, failed testing.

debo7debo’s picture

Status: Needs work » Needs review
TR’s picture

The patch doesn't apply anymore - it needs to be re-rolled. That's what the testbot output is saying. Testing and retesting won't help anything.

smartango’s picture

this is the updated code (7.x-3.x)

smartango’s picture

Toc toc.
@debo7debo, @TR could it be committed? is it something I have to do myself?
I get forbidden, I am reading drupal git handbook, just thought I can not commit without maintainer approval, and I do not know what is the policy for sending patch.

This patch add code and a test case, and do not invalidate the other tests, the code is relly short, there are some other tests suggested for this?

smartango’s picture

rjlang’s picture

I've tested patch #19. It works properly for product nodes that have their own SKU. However, if the product node has adjustments that have their own SKU(s) (e.g., multiple sizes for a shirt product) altering the adjustment SKU on the node adjustment edit page does not change the SKU in the stock table.

For this use case, updating the stock table needs to be made to happen within or in connection with function uc_product_adjustments_form_submit().

I've used hook_form_FORM_ID_alter() to insert an additional submission function into uc_product_adjustments_form() that checks for changes and updates the stock table. This seems to work. I've rolled these changes together with those from #19 in the attached patch.

Philou88’s picture

Thanks rjlang for your patch #22, I've tested it and it works very nice except when you have attributes.

First Test: You create a content, then you add attribute to your SKU product, then you adjust the SKU with extensions, then you display the SKU stock.
The SKU stock with attribute are fine.
And you find a line with the original SKU product that we do not use because the stock is in the attributes!
I think we need to delete this line with original SKU because we don't use it.

Second Test : When you modify the SKU product, the SKU stock with attribute do not change.
Only the SKU stock without attribute is modified.
Normally, if we modify the SKU product, the SKU stock with attribute needs to be updated. Isn't it ?

Philou88’s picture

Sorry rjlang, but it does't work in multilingual, when you save the SKU Product in the other language, you find the two messages:
Notice: Undefined property: stdClass::$original in uc_stock_node_presave() (line 346 of /homepages/.../sites/all/modules/ubercart/uc_stock/uc_stock.module).
Notice: Trying to get property of non-object in uc_stock_node_presave() (line 346 of /homepages/.../sites/all/modules/ubercart/uc_stock/uc_stock.module).

And the system told me that the SKU product is "in use, not updated" and my content "has been created." without the mandatory SKU product.

rjlang’s picture

@Phil88 #23:

Regarding "First Test," "And you find a line with the original SKU product that we do not use because the stock is in the attributes!". Let's suppose you have a product whose base SKU is P, and then you add attributes with SKUs P-1 and P-2. Now, if you don't want to track stock item P, only P-1 and P-2, then under the "Stock" tab, you should uncheck stock item P, and only leave P-1 and P-2 checked. If someone has a use case where they track both the base SKU and attributed SKUs, we should not arbitrarily break support for this usage (which was previously supported). Let the user decide which SKUs they want to track.

Regarding "Second Test", "When you modify the SKU product, the SKU stock with attribute do not change. Only the SKU stock without attribute is modified." This is entirely consistent and correct. If you modify the SKU of a product, you need to decide whether you as store manager are going to modify the SKUs of the attributed products and how to modify them. The addition of extensions to SKUs for attributes is always done manually according to the choice of the user; we cannot assume that each user is going to use the same system for modifying SKUs for attributes.

Conclusion: I think the original behavior is the correct behavior.

@Phil88 #24: I"ll look into this. I've not done anything with multilingual support before, so it may take a little while while I bone up on that topic.

smartango’s picture

I opened another report, complete of tests for attributes consistency. I have done so, at the same time I opened this, because someone sudjested to divide the two parts.
Now you can of course use your time to rewrite your solution, but I have to be honest saying you are solving an already solved problem.
check my opened issue, i'm not at work now

Philou88’s picture

Thanks rjlang and smartango for your feeback,

#23 First Test: Yes, perfect and good logic and keep history, I will use ‘active’ mark.
My first thoughts were the mark ‘active’ is only when you want to follow the product with a threshold, and a publish product has automatically valid stocks but, perhaps, not for all the attributes we add. The best way will be to create a new flag ‘valid stock’ just near ‘active stock’ but, as smartango dixit, it’s another topic. I had also thought for this new flag to get easiest extracts from views, more comprehensive for the customers.

#23 Second Test: Your way is nice and I adopt it.

#24 Multilingual: I'm waiting for your birth in multilingual, I'm already ready in starting block to test your new patch, many thanks in advance,

smartango’s picture

I regret for my last comment, I did not recall that here https://drupal.org/node/1415590 tests are missing,
also rjlang's #22 is the way I like (form_alter way).

On tests matter I would suggest these :

MultipleProduct:
insert a product without variants and model: product0-xxl
then a product with variant, model: product0, variants: product0-l, product0-exxl

now change the second variant product0-exxl to product0-xxl
(there is a sku duplication problem)

Or something like that lead my patch to add nid condition, can you check?
I closed the #1415590: stock and attributes consistency as duplicate of this

longwave’s picture

smartango’s picture

@longwave I do not think so, in fact #745912: Stock values lost when SKU is altered relates to 6.x-2.4 branch, not 7.x-3.x.

julinewbie’s picture

Total newbie here having this exact issue (same as OP)

Not familiar with git or patches. Can someone direct me to how to update the .module file with the most up to date tweak?

Your help would be very much appreciated! Stock and SKU's in a mess!

smartango’s picture

@julinewbie paste the code at #10 at the end of uc_stock.module file (without and ), at least this solve the problem with removing product and sku.

Then ... @longwave why not commit patch at @22 ? does it invalidate existing tests? I think that the patch could be committed while leaving this bug open for missing tests

julinewbie’s picture

thank you! I will give that a go....

julinewbie’s picture

I can't thank you enough! It's worked! :o)

Philou88’s picture

I use the patch #22 with only one language for a site and in multilingual for another site and it works perfectly.

If you have more than one language, the only thing that you cannot do is to modify the SKU. You can modify the first SKU but not the second SKU or the third SKU with the same value because the system tells you "in use, not updated", in fact, the system manages like double or something like that.

Any way, it's good patch because it tells you perfectly something right 'do not modify the SKU' to get a good management of the products sold by the web site, mainly in multilingual!

smartango’s picture

Issue summary: View changes

well actually reviewing the patch, this code:

function uc_stock_node_presave($node) {
  if(uc_product_is_product($node)) {
    $r  = db_select('uc_product_stock','stock')
      ->fields('stock')
      ->condition('sku',$node->model)
      ->execute()
      ->fetchAssoc();
    if($r) { // wayback: do not update
      $newmodel = $node->model;
      $node->model = $node->original->model;
      drupal_set_message(t('SKU @newmodel in use, not updated',array('@newmodel'=>$newmodel)));
    }
  }
}

would display a message even when the product model is not changed, say when
$node->model == $node->original->model
@rjlang, do you agree?

but it is only a message, no incorrect effects.

rjlang’s picture

@smartango,

Yes, I agree. (And it does that in practice, since our production system uses the patch and I see the msg periodically.)

fkarczeski’s picture

@rjlang,

Thank you for this patch! it saved me a great deal of headaches.

Just for future reference, I updated the message to be an error message on the uc_stock_node_presave function. Also, on the adjustments_form_submit function, it tries to update the sku on the stock table without checking if it already exists, that created a PDO exception for me and didn't break very gracefully ;)

I updated the code to use the same check you used in the presave function to check for an existing SKU when updating an adjustment SKU. This is what my version looks like:

<?php
function uc_stock_form_uc_product_adjustments_form_submit($form, &$form_state) {
  foreach ($form_state['values']['body'] as $key => &$value) {
    $default_model = $form_state['values']['default'];
    $old_model = !empty($form_state['values']['original_body'][$key]['model']) ? $form_state['values']['original_body'][$key]['model']['#default_value'] : '';
    $new_model = !empty($value['model']) ? $value['model'] : '';
    if ($old_model != $new_model) {
    	// Check if sku already exists in the stock table.
    	$r  = db_select('uc_product_stock','stock')
	      ->fields('stock')
	      ->condition('sku',$new_model)
	      ->execute()
	      ->fetchAssoc();
	    if($r) { // wayback: do not update
	      $value['model'] = $old_model;
	      form_set_error('', t('SKU "@new_model" in use, stock table was not updated. Please go back to SKU "@old_model" and address the duplicate SKUs on the stock table before updating this product.',array('@new_model' => $new_model, '@old_model' => $old_model)));
	    } else {
	      db_update('uc_product_stock')
	        ->fields(array('sku' => $new_model))
	        ->condition('sku', $old_model)
	        ->execute();
	      drupal_set_message(t('Updated stock table: changed sku @old_model to @new_model', array('@old_model' => $old_model, '@new_model' => $new_model)));	    	
	    }
    }
  }
}
?>

Unfortunately, it seems that the product has already been updated at this point; so the correct way to fix this would be to revert that attribute SKU back to the $old_model before returning the error. I currently don't have more time to spend on this, especially since we do not use attributes that much. I just figured I would let other people know my findings in case they hit this page looking for an answer like I did.

Thanks again for the patch, it works great!

mikemoretti’s picture

This patch doesn't seem to work for me. Instead I had to add a condition to the "Decrement stock on order submission" rule that checks if they are NOT a user with admin role. Depending on how others have their roles/users setup for allowing people to create orders you may have to do something different.