Items in a users cart are not removed when the item becomes unpublished

Not by design, but probably just because no one ever considered that case. Needs review now.

uc_cart_get_contents() or a hook_cart_item implementation may be a better place to do this, as then the items will be removed on the next page load after they are unpublished, rather than waiting for the user to hit the cart page.

Another use case that needs covering: you should not be able to even add an item to the cart if it is unpublished. Right now if you have a cached version of a page that lists a recently unpublished product, you can click the Add To Cart button and it'll be added, allowing you to buy a product that the store owner had "pulled off the shelf", so to speak.

Regarding the patch, at the very least the following line needs to be improved to match the coding standards:

  if(!$status){

This is a bit more important than originally reported.

Marked #1366326: Unpublished products can be checked out as duplicate

Simple patch for 7.x. But would any existing sites be relying on the current behaviour? Should we introduce a permission or setting to control this feature?

I don't think that's the best option - it should block the item from being added, not delete the record after-the-fact.

#6 handles the case of the item being added to the cart while being published then becoming unpublished before reaching checkout - and cart items can live for days, weeks or even months. The caching case you describe in #3 arguably covers a much shorter timespan, and is technically covered by this patch, but I agree we should perform this check at add to cart time as well.

I've also realised it should probably use node_access() instead of checking $node->status.

@longwave: Good point on the "cart never ends" thing, there are probably other checks that ought to be done with that in mind, e.g. whether the item is in stock, etc. There probably also should be a drupal_set_message to inform the visitor as to what has happened.

I can confirm that a modified version of #6 works for D6. Changing

db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = :id", array(':id' => $item->cart_item_id));
to
db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = %d", $item->cart_item_id);

This is perfect, as a client of mine just had two orders go through for products that don't exist! If this could be applied for the D6 branch as well, that would be awesome. I can submit in patch form if necessary.

Sorry, I didn't check the comments in #8 and #9 before I posted. That said, patch #6 does work for D7 (just tested) for the simple use case I needed.

Patch #6 still works great for the general use case. Can we get that committed and then move towards a more thorough solution?

Seems to have been fixed by #1351104: Need to clear line items as well as products for cart orders. Trying to checkout with an unpublished item in the cart shows "Some of the products in this order are no longer available." It'd be nice to indicate which products are the problem, since they are still in the cart, preventing it from going through.

May be worth porting this to Ubercart 2.x, as #10 says.

In D6 I added this code at line 1460 just before return $item; but nothing changed. An unpublished item remained in the cart. Did I do that right?

  // Permanently remove unpublished products from the cart. Added by Daniel 2012-03-09.
  if (!$product->status) {
    db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = %d", $item->cart_item_id);
    return;
  }

Something must have switched in the 7.x-3.0 release as the patch in #6 no longer applies... and I can confirm that it's still possible to add items to the cart.

I spoke too soon... that seems to only happen if you're logged in as admin, which confused me. But when I use cart links, etc as a regular authenticated or anonymous user, it does prevent it from being added to the cart.

So I guess I leave this open as to whether or not this should happen for admins? If yes, then this is definitely a 'closed' ticket... but maybe a warning should appear that you have unpublished nodes in your cart and normal users wouldn't get that?

This issue is about unpublishing items that are already in the cart, not adding unpublished items in the first place, which I believe has always worked how you describe; if a user has access to the node, they can add it to their cart.

Ok cool... then closing for now.

This is also happening in Ubercart 2 (just got a complaint from a client about it today). Anyone got a patch for Ubercart 2.x??

I'm not a coder, but I can follow code a bit. I have to get this working on my D6 site. For some reason, !$product->status evaluates to FALSE. I have a product that I published, added to cart, unpublished, view cart and it's still there. Am I close to getting it right?

function uc_cart_get_item($cart_item_id) {
  $result = db_query("SELECT c.*, n.title, n.vid FROM {node} n INNER JOIN {uc_cart_products} c ON n.nid = c.nid WHERE c.cart_item_id = '%d'", $cart_item_id);
  $item = db_fetch_object($result);
  if (empty($item)) {
    return;
  }
  $product = node_load($item->nid);
  $item->cost = $product->cost;
  $item->price = $product->sell_price;
  $item->weight = $product->weight;
  $item->data = unserialize($item->data);
  $item->module = $item->data['module'];
  $item->model = $product->model;

  // Invoke hook_cart_item() with $op = 'load' in enabled modules.
  foreach (module_list() as $module) {
    $func = $module .'_cart_item';
    if (function_exists($func)) {
      // $item must be passed by reference.
      $func('load', $item);
    }
  }

  // Permanently remove unpublished products from the cart. Added by Daniel 2012-03-09.
  if (!$product->status) {
   // db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = %d", $item->cart_item_id);
    db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = '%d'", $cart_item_id);
  }

  return $item;
}

I'm using UC 7.x-3.2 and if a product is unpublished by admin it stays in the shopping cart and customers can't go to checkout. I tried to insert the patch in #6 to uc_cart/uc_cart.module but can't find where to add it.

#24 describes the same thing as #13, perhaps we should improve this to automatically remove those items rather than stop the user reaching checkout.

Ideally, removing unpublished items from the cart would be an option. There is an edge use case selling unique items. You want to automatically unpublish the item as soon as it goes into someone's cart so that nobody else can try to buy it. You wouldn't want to remove the item from the cart in this case.

Of course, that's adding UI and code complexity for a feature only a few people would use, so it's a judgement call as to whether to do this. Maybe add a Rule action to remove an item from a cart/order, and a have a (default) reaction rule to trigger it when a product is unpublished. The edge case people can re-configure the rule.

I ran into this problem recently. I tried a few things with trying to remove items automatically but as a quick kludge (it just gives a warning and tells the user which product is not available any more). I put into uc_cart.module at line 1054

          if ($item->status == 0) {
$missing_msg="Sorry! Item ".$item->model." ".$item->title." is no longer available. Please remove this item before checkout.";
drupal_set_message($missing_msg);
        }

Just after foreach ($items[$cid] as $item) {

So it looks like this

 foreach ($items[$cid] as $item) {
          if ($item->status == 0) {
$missing_msg="Sorry! Item ".$item->model." ".$item->title." is no longer available. Please remove this item before checkout.";
drupal_set_message($missing_msg);
        }
          $item->order = $order;

Better attempt at an implementation on this, with a test. The code now reacts to a node update, whenever a node is unpublished any matching cart items are also deleted.

Reviews and tests welcome; not sure this is the best approach, especially for existing sites that rely on the existing behaviour.

@longwave: I think it would be better to not handle this in hook_node_update, but to instead check via the visitor's page load, that way the visitor could be informed of the change rather than things seeming to randomly disappear.

So here's a different approach, but doing this in uc_cart_get_contents() feels wrong. I am also wary of affecting custom implementations that may rely on unpublished items, so I wonder if implementing this through Rules would be better; there doesn't seem to be a useful event to do this on, though.

Oops, this doesn't permanently remove the item from the cart.

@longwave: It also tries to load $node->title if $node is empty ;-)

uc_cart_node_delete() should catch that case already, but I guess we really should treat deleted nodes and unpublished nodes in the same way.

@Damien: any thoughts about using Rules for this instead of hard coding it somewhere?

I think it'd be more cumbersome to use Rules.

#28 works great!

#28 worked great for me as well, thanks!

I think we should never delete unpublished products form carts because if we republish the product again we still want our customer to have that product in the cart!!!
Better approach, I think, is the one to not display the item and maybe still keep unpublished products in customer cart even after checkout to remind somehow the customer that the product is again available.
We should delete a product from customer cart when the product node is deleted only because than it means that the product it doesn't exists anymore.

@fiftyz: I don't think it's good to surprise the user - what if you republish the cart item just before the user gets to checkout, the item will suddenly appear back in their cart and they may pay for it without realising. Also, what if the user completes checkout while the item is still unpublished, and then you publish it - should it appear in their cart then? Simply deleting the item as soon as it becomes unpublished seems much simpler.

@longwave: Yes, that could be an issue so maybe you are right.

I think the best experience for the user would be that unpublished/deleted nodes are removed from the cart (not hidden! see #38) on cart load (either block, page or on checkout) and that the user sees a message that the product is no longer available.

To make the experience even better an extra module could hook into that and present a contact form where the user can submit it's email address so they can be notified if the product or something similar is available again.

I used a modified version of the patch at #6 in the function cart_get_item($item).

  if(!node_access('view', $product)){
      db_query("DELETE FROM {uc_cart_products} WHERE cart_item_id = '%d'", $item->cart_item_id);
      drupal_set_message(t('<strong>@product-title</strong> is no longer available and has been removed from your shopping cart.', array('@product-title' => $product->title)));
      return;
  }

@ Garbo: Where do you install your code? in uc_cart.module? there is no function called uc_cart_get_item.
Thanks

@yakbuttertea

in function uc_cart_get_item() in /uc_cart/uc_cart.module

This is a rework of patch from #30 fixing issues from #31 & #32. It modifies uc_cart_get_contents() and checks for products that are in cart that have been deleted or unpublished. Deleted products should already removed by uc_cart_node_delete() but the extra check here added so little code that I thought I should include it. I agree that unpublished products should not remain in cart as mentioned in #38 so this patch removes them and shows a warning message.

if the product is deleted then warning is "A product is no longer available..." otherwise if unpublished it shows "[product->title] is no longer available...". Test is also included

@longwave, You mentioned a concern about "affecting custom implementations that may rely on unpublished items". Why would anyone want unpublished products in the cart?

Thanks for working on this!

I think my original concern was that there could be an existing site out there using View Unpublished or similar to allow certain users access to unpublished nodes/products for whatever reason, and this might stop them doing that. But because your patch uses node_access() instead of explicitly checking $node->status then there are no problems here.

Overall I think this looks good, though there is a minor security concern. It's generally considered bad practice to leak information does the user should not have access to - in this case, the unpublished node title. The thinking behind this is that the site admin may have retitled the node as well as unpublishing it, and the admin would not expect any users to have access to the new title - but it might get inadvertently exposed here. As removal is likely to be a rare case anyway I wonder if is better to just show a single, generic message if any items were removed from the cart by this code?

(This, I suppose, is an argument for using Rules instead for this feature, because the admin could customise the message if they wished, including exposing the title where available!)

@longwave, Thanks for the response. As to your security concern about leaking information: I tested current operation, without the patch, and found that the user sees the unpublished node title when they view their cart. So either the leaks already exists or it's not considered a leak. Either way this patch does not expose any data that the customer did not already have access to. I think it's valuable for the customer to see what product was removed rather than just seeing a generic message that something was removed.

Uploading patch #43 again for the new testbot - no changes.

Still the same patch from #43, but re-rolled against the current Ubercart 7.x-3.x HEAD.

And again, same patch as #48 but with a few coding standards fixes that were identified by the test.