I just upgraded from the latest beta to the full version yesterday.

Now Ubercart doesn't respect the option to disable Shipping on a certain attribute, so when someone wants to buy a download only they are being charge shipping and are required to put their shipping info at checkout.

This was working before the upgrade, I double checked my settings and something is definitely wrong. Would anyone have any idea how to get this fixed asap??

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

univate’s picture

Title: Major Issues with UC 2.0 Final!! » The disable shipping option not respected on certain attributes

Try giving issues a title that have some meaning to the actual issue.

RikiB’s picture

noted, thanks. Hoping for a solution, Ive tried a lot of things to work around this but currently I just put a $0 Shipping option for now.

RikiB’s picture

I just did a lot more testing and I cant find a workaround, I hope its a simple patch. Any help would be greatly appreciated.

RikiB’s picture

I know I sound very annoying by now, but I guess I learned a good lesson about testing before upgrading.

Anyway I went through the trouble of installing a fresh copy of drupal and the problem is indeed real and confirmed on this test site.

RikiB’s picture

I have no idea why this isnt getting any attention, I mean..do I stink or something? :p

Any hope for a patch or anything?

RikiB’s picture

bumping in hopes that someone can confirm or deny this huge bug.

Island Usurper’s picture

Status: Active » Needs review
FileSize
666 bytes

Tracked down the problem to a particular change made in #574066: Object-valued by-reference parameters in Ubercart - Trouble with PHP 5.3 call_user_func_array. This patch should fix the problem so that shippable, non-shippable, and mixed orders will cause the delivery address and shipping panes to appear when they should.

rszrama’s picture

I don't believe it! That patch is the antichrist!

Definitely just kidding. :-/

Island Usurper’s picture

Wow. Took me a long time to see what you were talking about. :P

RikiB’s picture

Thank you, thank you, thank you! Seems to work perfectly.

Island Usurper’s picture

Status: Needs review » Fixed

Great. Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

RikiB’s picture

Version: 6.x-2.0 » 6.x-2.2
Status: Closed (fixed) » Active

I have to reopen this because its not completely fixed unfortunately. So I just upgraded to 2.2 to make sure and this is fixed it seems for 90% of the time. But for some crazy reason 10% of the time when someone buys download only, they are forced to put in their shipping information and also choose a shipping option (exactly the problem I state at the very top).

I know it doesnt make any sense but this is happening everyday with tons of people, but it doesnt happen everytime. Is there any hope??
For the time being Im putting a $0 shipping option until this is fixed, but it does confuse some customers.

Thanks.

Island Usurper’s picture

Priority: Critical » Normal
Status: Active » Postponed (maintainer needs more info)

At this point, I don't see how I can help if I don't know what's causing the problem. Can you duplicate the issue yourself, or is it just some of your customers? Is it possible that some of your file products don't have their shippable flags set correctly? Check both the main edit form for the product and for the file features. If that doesn't help, add some debug statements around the area of code that the patch modified. Use the dpm() function from the Devel module to the check the return values of hook_cart_item().

willowmedia’s picture

I'm not sure if this is the right place to mention this, but in case it helps someone..

I had a variation of the problem ~ I'm using UC 2.2 and buying a download-only product. I don't see the shipping information in my cart, however, when I do a checkout, I don't get the file download link emailed. The order status is set to Payment Received rather than going through to Completed.

To make it work, I made a change to uc_order.module (similar to the patch in #7) so that

    // See if any other modules have a say in the matter...
    foreach (module_list() as $module) {
      $function = $module .'_cart_item';
      if (function_exists($function) && $function('can_ship', $product)) {
        // $product must be passed by reference.
        $result[] = TRUE;
      }
    }

is changed to

  // See if any other modules have a say in the matter...
  foreach (module_list() as $module) {
    $func = $module .'_cart_item';
    if (function_exists($func)) {
      // $product must be passed by reference.
      $return = $func('can_ship', $product);

      if (!is_null($return)) {
        $result[] = $return;
      }
    }
  }

Just wondering whether if might fix the problem that RikiB is having ?

RikiB’s picture

I just wanted to reply and say this is still a real issue, but I am frustrated that I cant reproduce it. All I know is still happened to some people every single day, and if I add the same products they buy to my cart it doesnt happen to me.

So since I cant reproduce it, are there any other ideas I could try?

torgosPizza’s picture

+1. The fix in #15 worked for me.

RikiB, here's how I am able to reproduce it.

Change the last block of code in uc_order_is_shippable() to this:

    // Return TRUE by default.
    if (empty($result) || in_array(TRUE, $result)) {
      dsm('returning by default');
      $results[] = TRUE;
      continue;
    }
    dsm('results is false');
    $results[] = FALSE;
  }

(basically just adding in a couple dsm's. Make sure Devel is enabled.)

- Now create a product that is shippable. No options, attributes, etc. on it yet. Give it a weight and make sure the shippable checkbox is checked.
- Create an Attribute on that product, call it "Delivery method" or something. Create two Options, "Shippable" and "Download".
- Create Adjustments for these two options. Make one Prod-Shippable and Prod-Download, for example.
- Add a Feature that is a File download. Add it to the SKU Prod-Download, and make sure the "Shippable product" checkbox here is unchecked.
- Make a custom Conditional Action that Checks if an order can be shippable, and if so, have it Update the Status to some custom status like "Pending shipping"
- Create an order with the product. (May need to update the order status in the order Admin once to get the CA to fire, based on your Trigger settings in the CA.)

If all is set correctly, when you view the order at admin/store/orders/ORDER_ID, you should get a drupal message that says it's returning by default (unless you used the fix in #15). The reason for this is that in this code block:

 // See if any other modules have a say in the matter...
    foreach (module_list() as $module) {
      $function = $module .'_cart_item';
      if (function_exists($function) && $function('can_ship', $product)) {
        // $product must be passed by reference.
        $result[] = TRUE;
      }
    }

$function('can_ship', $product) returns with FALSE for anything that calls uc_file_cart_item('can_ship') and includes a File Download product that has the "shippable" checkbox UNCHECKED, as is the case in our example.

To refresh, the code in uc_file.module's invocation of this hook reads like so:

function uc_file_cart_item($op, &$item) {

  switch ($op) {

    case 'can_ship':      
      // Check if this model is shippable as well as a file (;/)
      $files = db_query("SELECT shippable, model FROM {uc_file_products} as fp INNER JOIN {uc_product_features} as pf ON pf.pfid = fp.pfid WHERE nid = %d", $item->nid);
      while ($file = db_fetch_object($files)) {

        // If the model is 'any' then return.
        if (empty($file->model)) {
          return $file->shippable;
        }
        else {
          // Use the adjusted SKU, or node SKU if there's none.
          $sku = empty($item->data['model']) ? $item->model : $item->data['model'];
          if ($sku == $file->model) {           
            return $file->shippable;
          }
        }
      }

      break;
  }
}

So, to break what's happening here down:

In the test examples I gave you: if you have a file download for the SKU "Prod-Download", then this function in uc_file.module will return FALSE (0) when that module's hook_uc_cart_item is invoked in uc_order_is_shippable(). Unfortunately this causes the $result[] array to not get built, because the function only cares if $function('can_ship', $product) returns TRUE, which in this example case, it will not. That results in the empty array, which in turn makes the function return the "Shipping is TRUE by default" condition, which in reality is not the case.

Hope this is thorough enough without being too verbose, but it's a pretty specific issue and one that is affecting us like crazy. We have many DVD products that are being purchased as digital files, and our person taking care of shipping has to set each of them to "Completed" manually because they all come up as "Pending shipping" even if they're not. However, the fix in #15 seems to work, and I'll be keeping an eye on it this week.

@willowmedia: Thank you for posting a solution that works :)

RikiB’s picture

Torgos, thanks for the detailed reply! It looks finally the problem is gone!!

sammys’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1003 bytes

I've stumbled onto this on a site where uc_quotes module is installed. The uc_quotes module sets products as unshippable by returning FALSE in hook_cart_item('can_ship', ...) and because the loop in uc_order_is_shippable() doesn't add a FALSE value to the $results array the default TRUE is returned as already posted about. I've rolled up and attached a patch to do what's in #15 but used original name for $function and $can_ship instead of $return.

jfirebau’s picture

So I have read this thread and patched my code. I have used drupal_set_message() lines throughout the uc_file_cart_item() function and uc_order_is_shippable() functions and they are correctly returning FALSE for my product which is a file download. However, the cart is asking for a shipping address and adding shipping fees. Where else can I look to see what is breaking? Does anyone know where the code is that uses these functions and enables/disables the panes on the cart?

Thanks.

jfirebau’s picture

Here is what I found, and maybe someone can explain why this logic is failing for me.

In /ubercart/uc_cart/uc_cart.pages.inc

// If the cart isn't shippable, remove panes with shippable == TRUE.

if (!uc_cart_is_shippable() && variable_get('uc_cart_delivery_not_shippable', TRUE) ) {
   $panes = uc_cart_filter_checkout_panes($panes, array('shippable' => TRUE));
}

variable_get('uc_cart_delivery_not_shippable', TRUE) is returning FALSE or 0 causing the panes to not be removed. If I change it to the following my store works:

// If the cart isn't shippable, remove panes with shippable == TRUE.

if (!uc_cart_is_shippable() ) {
   $panes = uc_cart_filter_checkout_panes($panes, array('shippable' => TRUE));
}

Maybe it should be || "or" instead of && "and" ? I'm not exactly sure what check is being performed there -- any suggestions?

Thanks.

sammys’s picture

Version: 6.x-2.2 » 6.x-2.3
FileSize
959 bytes

Updated patch for 2.3.

RikiB’s picture

Well I updated ubercart from 2.2 to 2.3 and the problem is back. This time neither #15 nor #22 fixes it.

Updating ubercart is so stressful for me, I knew I should have stayed with 2.2 :(

torgosPizza’s picture

Riki: What's the situation? Is it the same issue as your original post? The patch in #22 looks good to me but I haven't upgraded to 2.3 yet.

Island Usurper’s picture

Status: Needs review » Fixed

Patch makes sense, especially since the documentation expects it to work that way.

Thanks for your patience.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

RikiB’s picture

Version: 6.x-2.3 » 6.x-2.4
Status: Closed (fixed) » Active

Can we get an updated patch for 2.4? Im having the same exact problem :(

TR’s picture

Status: Active » Postponed (maintainer needs more info)

@RikiB: The patch was included in Ubercart 2.4, so it does not need to be "updated". Let me suggest that your problem might be a configuration problem rather than an Ubercart code problem. Please describe in detail the steps that you have to take to reproduce the problem.

RikiB’s picture

You could be right, but it workes perfectly in 2.2 but now in 2.3 and 2.4 the problem is back. Why does it work fine in 2.2?

All I have to do to replace the problem is to add an attribute to the cart that is download only. Since i have many of these already configured I am using the products that already exist and prove to work perfectly on my main site. However on a dev site I simply upgrade ubercart and the problem appears. The dev site is an exact copy of the main site.

RikiB’s picture

Torgos - Sorry I didnt answer you before, yes its the same exact problem as the original post.

torgosPizza’s picture

RikiB, I just upgraded my dev site to 2.4, and I am unable to reproduce this exact issue. I'm not getting a "shipping" pane during checkout, but taxes are being added to the product. I have selected, under admin/store/settings/taxes, to only tax shippable items. (This was also an issue in 2.2 which our production server is currently on; I'm only discovering this now.)

If I get time I'll try to dig into that more, but I'm fairly certain non-shippable Features are being correctly handled in Checkout. Riki, can you try testing one particular product? Just re-save that Feature (you don't need to delete it, necessarily - if it's a file it might remove the file from users who have already purchased it).. but if you re-save that Feature with the "shippable" checkbox unchecked, try purchasing it again. And if you can, please attach a screenshot of a) the feature Edit page and b) the checkout so I can better understand what's happening.

RikiB’s picture

FileSize
24.42 KB
19.7 KB
45.4 KB

Thanks torgos, I attached some images and I even created a brand new product with the minimal information to reproduce it.

torgosPizza’s picture

Riki,

Are you using a contrib module to get your checkout to look like that? Or is that all done in CSS?

RikiB’s picture

Its all done in uc-cart-checkout-form.tpl.php

However I can also see the problem on /cart, not just /cart/checkout. On /cart I have the option where they can get shipping quotes and that reveals that it thinks its a shippable product. So its a problem in ubercart in general I think. If I uncheck "this product is shippable" of course it fixes the problem but then it will never ask for shipping information or give quotes. Also /cart doesnt have a tpl.php file.

In case your interested in the code for the checkout page, here it is:

<?php 

    $output = '<div id="checkout-instructions">'. check_markup(variable_get('uc_checkout_instructions', ''), variable_get('uc_checkout_instructions_format', FILTER_FORMAT_DEFAULT), FALSE) .'</div>';

    $output .= '<div class="ccart" style=" width: 667px; float: left;">';
    $output .=  drupal_render($form['panes']['cart']);
    $output .= '</div>';
	
    $output .= '<div class="cgeotrust" style=" float: left; width: 290px; margin-top: 14px"><fieldset>';
    $output .= '<center><!-- (c) 2005, 2010. Authorize.Net is a registered trademark of CyberSource Corporation --> <div class="AuthorizeNetSeal3D"> <script type="text/javascript" language="javascript">var ANS_customer_id="";</script><script type="text/javascript" language="javascript" src="//verify.authorize.net/anetseal/seal.js" ></script></div><div class="GeotrustSeal3D"><SCRIPT LANGUAGE="JavaScript" TYPE="text/javascript" SRC="//smarticon.geotrust.com/si.js"></SCRIPT></div></center>';
    $output .= '</fieldset></div>';
	
    $output .= '<div class="cbilling" style="clear: both; float:left;  width: 480px;">';
    $output .=  drupal_render($form['panes']['billing']);
    $output .= '</div>';
	
  if (uc_cart_is_shippable() ) {
    $output .= '<div class="cdelivery" style=" float:left; width: 480px; ">';
    $output .=  drupal_render($form['panes']['delivery']);
    $output .= '</div>';
	}
	
  if (uc_cart_is_shippable() ) {$output .= '<div class="cclear" style="clear:both ">';}
  
    $output .= '<div class="cpayment" style="float:left;  width:480px; ">';
    $output .=  drupal_render($form['panes']['payment']);
    $output .= '</div>';

  if (!uc_cart_is_shippable() ) {$output .= '<div class="cclear" style="clear:both ">';}
	  
    $output .= '<div class="ccoupon" style=" float: left; width: 480px;">';
    $output .=  drupal_render($form['panes']['coupon']);
    $output .= '</div>';
	
    $output .= '<div class="cquote" style="  float: left; width: 480px;">';
    $output .=  drupal_render($form['panes']['quotes']);
    $output .= '</div>';
  
    $output .= '<div class="ccustomer" style="width: 480px; float: left">';
    $output .=  drupal_render($form['panes']['customer']);
    $output .= '</div>';
	
    $output .= '<div id="checkout-form-bottom" style="clear: both; width: 96%;  float: left;">'. drupal_render($form) .'</div>';
	
    $output .= '<br /><br />';
	
    print $output;
?>

Thanks for the help.

RikiB’s picture

FileSize
31.47 KB

Here is a screenshot of the same product in the /cart page. (notice the shipping estimate calc at the bottom)

*Edit*
I also just deleted that file and the problem continues, the styling is gone but the delivery option is still there in /cart/checkout

torgosPizza’s picture

Hey Riki,

Yeah that "shipping quotes" pane is, I think, an unrelated issue. (Unless it also didn't show up in 2.2? when it wasn't needed? I don't use it so I haven't any experience with it.)

And you said you deleted the template file and the "shipping" information pane in checkout still shows up? Hmm... that is really strange. At this point I'd have to really look at your code. I feel like there is a Contrib that is overriding something... although just because I wasn't able to reproduce it doesn't necessarily mean this issue doesn't exist. But it does sound like a potential config issue.

On your dev server can you delete the entire ubercart directory and then re-download the module? (You might want to backup your template file.) Just to make sure there aren't any stray files potentially conflicting and causing this problem..

torgosPizza’s picture

The reason why I'm going this route in the discussion is because it could be a Contrib that needs to be updated to match some code changes with 2.4, although I'm not exactly sure which changes would have an effect like this. I'm really just trying to rule things out since I have no real idea what could be the culprit! :)

RikiB’s picture

I cant thank you enough for the help. I was hoping deleting the ubercart directory and re-uploading it would help but it didnt.

Then I had another idea and may have discovered something. I am using contemplate to control my product page and I removed the code that shows the add to cart and Im using cartlinks for people to add products to their cart. If I use the default add to cart button the problem is gone! Its only when I use cartlinks (like shown below) that the cart and checkout thinks its a shippable product. Perhaps this is a bug with cartlinks, or perhaps another module is interfering. I will disable the extra modules to see if cartlinks is still broke. I still need a solution because my site depends on cartlinks heavily.

example cartlink: /cart/add/p1003604_q1_a1o1_m0?destination=cart

torgosPizza’s picture

Ah, that'll do it. I thought something was up :)

There is a patch that needs testing here: #398000-20: Cart links don't check attributes or options on their products - there isn't an actual patch, but if you're comfortable editing code, the change is a relatively small one. Let me know if the comments in #20 of that thread work for you, otherwise take a look at the changes I made in #17.

RikiB’s picture

I am very comfortable editing code :)

Sure enough #20 fixed the problem!! Thanks again torgos!!!! :D

torgosPizza’s picture

Status: Postponed (maintainer needs more info) » Fixed

Welcome! If you noticed I created a patch for that code so we can reference it later. My original issue is unaffected by it, so I may re-roll another patch for that :)

I'll mark this as Fixed for now since we realized that it was Cart Links that was the issue.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

torgosPizza’s picture

Title: uc_product_add_to_cart_data does not respect non-shippable attributes » The disable shipping option not respected on certain attributes

I'm opening this issue again. This seems to be fixed, for the most part, except other instances when an order is checked to see if it's shippable.

The symptom is that the "data" column in uc_order_products still contains a "shippable" array with a value of TRUE even when an order only contains file downloads. This doesn't seem to affect actual "shippability" of an order; in other words, CA (and the checkout pane) correctly identify the order as being shippable or not, and show/hide the "delivery information" pane, shipping quotes, etc. as necessary.

For example, in uc_shipping.module, uc_shipping_order_actions (line ~264):

$result = db_query("SELECT COUNT(nid) FROM {uc_order_products} WHERE order_id = %d AND data LIKE '%%s:9:\"shippable\";s:1:\"1\";%%'", $order->order_id);

Even if the order is not shippable, this value is still stored in the $data column, leading to a false positive. The order says it's shippable, even when it's not, and it gives you the option of packaging it. This causes the "package.gif" icon to display when it shouldn't be..

I tracked it down to the actual instance of hook_add_to_cart_data() in uc_product.module:

/**
 * Implementation of hook_add_to_cart_data().
 */
function uc_product_add_to_cart_data($form_values) {
  $node = node_load($form_values['nid']);
  return array('shippable' => $node->shippable);
}

When you add an item to your cart, this function is fired, and uc_file, at the moment, does not act on this. So you may have a product whose "main" value is shippable, but if you have an adjusted SKU / option / attribute combo that overrides this shippable status, it is ignored since uc_product_add_to_cart_data is only returning the $node->shippable value, and not letting other modules alter this value.

I'm working on a fix, but since this isn't a critical issue I'm leaving it on the back burner. If anyone feels adventurous feel free to take a look. It's not huge, just kind of annoying, especially when I realized that there are values in the cart which are "locked" but don't seem to really do anything to the order. (Why does the order have shippable:1 when it should be 0, and why are there other API methods that seem to ignore this, even if the end result is correct? Some cleanup needs to be done in that regard, IMO.)

I tried throwing a uc_cart_product_is_shippable() call alongside the db_query up above, but that didn't work as I'd hoped; it caused shippable orders to lose the package icon when they should have had it.

Any ideas or stabs at it are welcome. I'll try to get to this when I can.

torgosPizza’s picture

Title: The disable shipping option not respected on certain attributes » uc_product_add_to_cart_data does not respect non-shippable attributes
Status: Closed (fixed) » Needs work

Changing the title.

jtbayly’s picture

Title: The disable shipping option not respected on certain attributes » uc_product_add_to_cart_data does not respect non-shippable attributes

subscribe

longwave’s picture

Went way down the rabbit hole trying to figure this one out. It's complicated and ugly, and also related to the following:

#859266: Missing Delivery Information Form on Checkout
#977682: uc_order_is_shippable doesn't let see if any other modules have a say in the matter of shippability
#1019002: Bug in function uc_order_is_shippable -- Order determined to be non-shippable even when it should be shippable
#1169732: Shipping Info and Quotes Show, even when there are no shippable products

In a nutshell, calculating a product's shippability is broken in a number of subtle ways.

  • $item->data['shippable'] is only ever set to the product node's shippable flag, which is done by hook_add_to_cart_data() (assuming this is invoked, which is the responsibility of any caller to uc_cart_add_item!)
  • uc_cart_product_is_shippable() and uc_order_is_shippable() can override this with hook_cart_item('can_ship'), but $item->data['shippable'] will still be wrong if a product feature specifies something different.
  • uc_order_is_shippable() shouldn't even be calling hook_cart_item('can_ship'), it should only be reacting to stored data (as order shippability should be based on the items at the time of the order, not "now").

The hard part is how to fix this. We can't set the shippable flag for file downloads in hook_add_to_cart_data() as we don't have access to the adjusted SKU - this is only set by hook_cart_item('load') in uc_attribute. We also can't change $item->data['shippable'] any time after the cart item has been stored in uc_cart_products (for example, during hook_cart_item('load')), as this prevents the item being removed from the cart again - $item->data must match exactly when removing items.

So, this is all pretty ugly and needs fixing. Ideally, I think we need to set $item->data['shippable'] once and for all when the item is first added to the cart, then always use that to discover shippability. This will also keep $item->data consistent and avoid breaking the remove item functionality.

Another problem is what to do if two modules disagree on whether something is shippable or not. I think it should use the following rules:
- if the product node is shippable, but all 'can_ship' implementations say it isn't, it isn't shippable
- if the product node isn't shippable, but any 'can_ship' implementations say it is shippable, it becomes shippable.

Comments (or patches!) welcome.

longwave’s picture

Version: 6.x-2.4 » 6.x-2.x-dev
Status: Needs work » Needs review
FileSize
5.64 KB

Okay, so here's a first stab at a patch to fix all this.

uc_cart_get_contents() and the (unused in core) uc_cart_get_item() prepare cart items in the same way, so I abstracted this out to a new uc_cart_prepare_item() function. We can now call this during uc_cart_add_item() to discover the full SKU, and therefore discover whether the product is shippable or not. This means that $item->data['shippable'] is always stored correctly in the database. In turn this means we can simplify uc_cart_product_is_shippable() and uc_order_is_shippable() to just check $item->data['shippable'] directly. Finally, uc_product_add_to_cart_data() can now be removed as uc_cart_add_item() is now responsible for setting the shippable flag.

I tested this successfully with a product kit containing two items, neither of which are shippable, but one product has attributes and one of the attribute options is shippable. $item->data['shippable'] always appears to be correct and the shipping panes are only shown at checkout if the shippable attribute is chosen.

Also noted when debugging this; this code in uc_cart_get_contents() never does anything:

for ($i = 0; $i < count($items[$cid]); $i++) {
  if ($items[$cid][$i]->nid == $item->nid && $items[$cid][$i]->data == $item->data) {
    $items[$cid][$i]->qty += $item->qty;
    continue 2;
  }
}

$items[$cid][$i]->data is an array, $item->data is a string. It looks like this is meant to deduplicate items in the cart, but presumably it's not actually needed as the comparison is never true!

Finally, it looks like the SQL query to load items from the cart can also be simplified:

SELECT c.*, n.title, n.vid FROM {node} n INNER JOIN {uc_cart_products} c ON n.nid = c.nid WHERE c.cart_id = '...'

node_load() is called immediately, so we could just SELECT * FROM {uc_cart_products} then set vid and title from the node_load() results.

torgosPizza’s picture

A herculean effort, Longwave. Thanks. I'll check this out when I have a moment.

Status: Needs review » Needs work

The last submitted patch, 613498-shippability.patch, failed testing.

longwave’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev

Bumping to 7.x as it's likely to get fixed there first and backported.

peterx’s picture

The can_ship part of the Roles module creates a problem for us as described in http://drupal.org/node/1352204#comment-5288632. When this shippability patch is applied to 7 dev, I will test it and possible open a specific issue for roles. I include my description here because I did not find this issue when searching for delivery address and other combinations of words.

We have shippable products and unshippable products (subscriptions), and we have products that include both. Some of the subscriptions are packaged with a physical bonus product that requires a delivery address. We include the shipping cost of the physical item in the total cost so each bonus subscription has the same simple ordering as the standard subscription requiring only the delivery address, not a shipping quote.

All the subscriptions, with and without bonus products, have the same role applied by the roles module. The products with the bonus item have shippable set on. The Roles module can_ship function looks at the role and replies that the product is not shippable.The delivery address is not presented.

We could set a different role for subscriptions packaged with shippable items but this may not be practical after we finish adding all the subscription roles.

We could modify the Roles module to have a setting "Do not set shippable". We currently implement this by commenting out the can_ship code in the roles module.

Another option could be to apply "Do not set shippable" at the role level but we do not need that and would probably use the duplicate role option if we did need it.

A product could have a "Needs delivery address" or "Delivery cost included" option so we get the delivery address without going through the pain of setting up a flat rate delivery of zero dollars. Many searches were required to find the instructions for that.

A "Delivery cost included" would simplify this whole area for many local sites where the delivery cost is built in to their products. Set the product to shippable plus delivery cost included. You get the delivery address and the email fired off to the shipping department. There is no shipping quote.

j0rd’s picture

@longwave thanks for looking into this and from reading #47 it looks like you're on the right path.

I need this fix for D6 but the patch doesnt apply to -dev, so I may port it to latest when I have some time.

For my client this is kind of a large problem as its marking orders which have shippable products as "completed" instead of "paid" and then they get neglected due to how thier workflow goes.

j0rd’s picture

Version: 7.x-3.x-dev » 6.x-2.x-dev
Status: Needs work » Needs review
FileSize
4.86 KB

Sorry to move this issue back to 6.x, but I re-rolled patch #47 against latest ubercart-6.x-2.x-dev. I tested it for the one use case which was previously failing and this re-roll does not appear to cause any new problems and appears to resolve the issue.

The patch is a little scary to me, as it removes a function and plays around with a lot of code that could be used in contrib. I did grep all my current contrib modules for the removed function `function uc_product_add_to_cart_data` and nothing appears to call it. I tried to decide if I wanted to deprecate this function (have it do nothing, but report to watchdog) or completely remove it. I figure because it's a lot value internal function, that it's probably not getting called by much if any contribs, so probably best to hard fail on removal, instead of "silently fail" in watchdog.

I'm still worried about other contrib modules, which touch on the other changed functions, but my heavily contrib module'd ubercart (complete with custom add to cart form) seems to be chugging along happily, so I assume it should be fine for most.

I would like to see some tests written for this particular use case in Ubercart D7, as something like this should have been caught ideally.

Eitherway, here's the patch. Please test and see if it resolves your problems on D6 and please post in this issue queue your results, so we can get this important patch committed.

Status: Needs review » Needs work

The last submitted patch, ubercart-shippability-613498-53.patch, failed testing.

j0rd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, ubercart-shippability-613498-53.patch, failed testing.

j0rd’s picture

Not 100% sure, but I think testbot may be broken instead of the patch.
#996512: D6 contrib project tests: [MySQL] Drupal installation failed.

j0rd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, ubercart-shippability-613498-53.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
torgosPizza’s picture

@j0rd: It looks like the patch doesn't remove the hook, though, which is fine. I believe we are using hook_add_to_cart_data() for some of my custom contribs (nothing that couldn't be fixed) but it appears you have nothing to worry about, unless I've totally missed something.

But thanks for taking this on!

j0rd’s picture

Could someone outline how someone could duplicate the failing use case, so we can have people effectively test this problem. I have a failing use-case via product on my site I can easily duplicate the bug with, but others may have a harder time and I wouldn't now how to properly explain what products you need to add to cart to have this to fail.

From what I understand you need a product that runs through can_ship and fails, then another product which goes through can_ship and passes, but because the $result array in never cleared [via $result = array();] at the right place in the for loop, it continues to fail.

If someone could properly outline how to duplicate a proper test case, we could get this tested from more people with this problem (hopefully on sites which have lots of contrib modules, to see if other problems turn up) then get this committed. I have a feeling this bug has caused a lot of non-shipped orders on sites which have digital (non-shippable products) and shippable products, as those orders will be marked as completed and never looked at for shipping.

j0rd’s picture

I think I'm running into a problem because of this patch.

Happens when you attempt to get a shipping quote via USPS when creating an order in the admin interface.

Looking into it.

j0rd’s picture

There are certain function, which instead of adding line_items to carts, actually add "products" to "orders". This method seems to subvert the changes we've added to line items.

Now I would assume, we'd want to remove this functionality and add line_items to carts, and then save the carts to orders all the time for consistency. Someone like @longwave or @TR who might understand this more could chime in with the "optimal" solution.

The less optimal solution is to simply make sure then we add products to orders, the product's shipability is initialized (most likely by calling `uc_cart_prepare_item` on the products ahead of time).

Here are some functions in question which I've found ignore the add to cart changes in this patch:

uc_quote/uc_quote.js
function quoteCallback(products);

uc_quote/uc_quote.pages.inc
function uc_quote_request_quotes();

uc_order/uc_order.module
function uc_order_save($order);

uc_order/uc_order.admin.inc
function uc_order_edit_products($order);

Now the good thing is, these functions do not get called very much. I think the only real place is on the `admin/store/orders/$ORDERID/edit` type pages.

The bad news is, something will need to get done, so that these pages work as expected.

j0rd’s picture

I've added a little bit of code to `function uc_order_edit_products($order);` to make sure $product->data['shippability'] is initialized.

It might be wise, as a precaution, that is any shipping modules are enabled and in ` uc_cart_product_is_shippable($item);` if !isset($item->data['shippability']);` that we attempt to determine what it should be. This key in this function should always been initialized and in $item, we have all the data we need in that function to initialize it, should we need to.

Anyways, here's an updated patch, including one additional snippet which appears to resolve the bug in the admin interface. I would like to have this reviewed, and perhaps re-factored if there's a better place to do this.

diff --git a/uc_order/uc_order.admin.inc b/uc_order/uc_order.admin.inc
index 951aa9c..065f3e2 100644
--- a/uc_order/uc_order.admin.inc
+++ b/uc_order/uc_order.admin.inc
@@ -1315,17 +1315,27 @@ function uc_order_edit_products($order) {
       break;
     case 'add':
       $form_state['values'] = $_POST;
-      $product = node_load(intval($_POST['nid']));
+      $product = new stdClass;
+      $product->nid = intval($_POST['nid']);
       $product->qty = intval($_POST['qty']);
-      $product->price = $product->sell_price;
       $product->data = module_invoke_all('add_to_cart_data', $form_state['values']);

-      foreach (module_list() as $module) {
-        $function = $module .'_cart_item';
-        if (function_exists($function)) {
-          // $product must be passed by reference.
-          $function('load', $product);
+      uc_cart_prepare_item($product);
+
+      if(!isset($product->data['shippable'])) {
+        $node = node_load(intval($_POST['nid']));
+        $results = array();
+        foreach (module_implements('cart_item') as $module) {
+          $func = $module .'_cart_item';
+          $return = $func('can_ship', $product);
+          if (!is_null($return)) {
+            $results[] = $return;
+          }
         }
+
+        // The product is shippable if any module says that it is shippable,
+        // or if no module returned a result and the product node is shippable.
+        $product->data['shippable'] = !empty($results) ? in_array(TRUE, $results) : $node->shippable;
       }

       $price_info = array(
j0rd’s picture

Anyone manage to test this? I've been running it on my live site for a couple weeks.

j0rd’s picture

I've found a problem with this patch. Seeking guidance as to a proper solution.

As per @torgosPizza's #17 relating to uc_file and hook_cart_item($op = 'can_ship');

I've noticed that I have a node, which is set to "shippable". This node has a uc_file attachment, which is set to "shippable" = "no".

The patch in #65 causes this product to be non-shippable.

In my opinion, a product which is set to shippable, but has a uc_file attachment which is not shippable, the product should be shippable. I see a case where, there's a physical product, which also includes a downloadable file. If you wanted a purely downloadable file, you would set your product to non-shippable and your file to non-shippable.

Do I have the correct logic here? Or does the uc_file shippability override the original product shippability?

---

Also with regards to testing, the site I'm working on to test this patch, does not use attributes at all, so I'm not able to test those additional complexities.

longwave’s picture

Do I have the correct logic here? Or does the uc_file shippability override the original product shippability?

I have been eternally confused about this, I don't understand "shippability" of product features; it should be SKU based, so an SKU either is shippable or it isn't, it doesn't make sense to have it as a flag per product feature - what if two features conflict over this? Perhaps the best way is to treat it as OR, so if the base product is shippable, or any feature is shippable, the product is shippable too?

j0rd’s picture

It doesn't make sense to me. Yes they should be SKU based.

File download products should never be shippable. They are essentially electronic downloads right? You don't need to ship those.

If you need a product, which is shippable and has a downloadable file, you mark the product as shippable (root SKU is shippable) and then since file downloads are never shippable, then you have a shippable product.

If it's a pure download, then you mark the main root product as "non-shippable" and since file downloads are never shippable, then you have a non-shippable, downloadable product.

That seems to make sense to me.

What makes the most sense for me, is as you say to make it an OR. So if root product's SKU is shippable and file download is not shippable, you have a shippable product.

Problem with this is, it'll mess up products in my store with this change. It would make things more correct, but it would currently mess up my store since products are setup wrong currently.

This is something that should really be removed, unless there's a specific use case someone can bring up. Perhaps it's used with recurring billing? Where a discounted sample is shipped and a fraction of the cost is charged. Then after 30 days, the remaining balance for the item is charged? If the shippability of product features used for something like this?

rszrama’s picture

fwiw, this is exactly why we needed a new product / SKU model. In Ubercart, we don't have reliable SKUs, nor do we have a way to easily associate extra information with SKUs (such as shippability). SKUs are just determined by the random selection of product attributes, and that's that. So the reason shippability works the way it does is you don't have any way to associate random data with SKUs, but you can associate a product feature with a SKU and through that the extra data (such as shippability vs. non-shippability).

The original use case for this feature was combination file download products, e.g. buy a movie on a DVD or file download. The base product is the DVD, and it's shippable, but you would add an attribute to let the customer specify DVD or download. If you used separate SKUs and attached the file download feature to that SKU, you could then specify that SKU as non-shippable. Changing this from an override to a simple OR would break this, because in that case the file download would still be treated as shippable when it shouldn't be. One easy example of this was the Lullabot store.

Also, an example for the other way around would be a recurring billing product feature that makes a digital subscription also act as a physical description. Think of a Netflix subscription product from their meltdown last year... one product node, an attribute to select streaming (the default service) or streaming + mailed DVDs, and two recurring product features - one on each SKU for the different amounts, with the one for mailed DVDs making the subscription shippable. The root product would be non-shippable and the attribute adjustment would make the product shippable. Changing it to an OR would work fine here but break the previous example, which is probably more likely.

So summarizing, in the absence of a good product model, the product feature became a way to associate extra data with a SKU, because product features could be enabled on a per-SKU basis. I'd strongly encourage you not to change this behavior without a more comprehensive fix to the product data model. Or, you know... ; )

longwave’s picture