Cart links don't check attributes or options on their products

Sweet. If you can roll a D5 version of this it'd be appreciated. If not, I can take a look at yours and work on my own D5 version. Thanks, cha0s!

Subscribing.
Need this for torgosPizza's excellent upsell module.
Will get some sort of test environment up and running and see how it goes.

Alan

If invalid attributes (or none at all) are specified for a product, then I think their default options should be added to the cart item. More than one product can be specified in a cart link, so I'm not sure that redirecting to one of those products is always the right thing to do. Also, I've seen cases where cart links are used when the customer is not given view access to the product at all. Redirecting would result in a "Permission denied" error.

Also, uc_product_get_attribute() is defined in uc_attribute.module, which this module isn't dependent on. We need another module_exists() around that section.

Subscribing.

Need this for torgosPizza's excellent upsell module.
Will get some sort of test environment up and running and see how it goes.

Jordan

Tagged.

I believe the "let errors slide" in this case is a bad idea.

I would prefer to have it fail, and have the user be redirected to the product page, telling him to provide the attributes he will need. This is the route I have gone and other users are suggesting.

Letting errors slide in this case, might cause the user to buy the wrong size shoes or something like that. This will cause more issues for the seller. I think it's better to have a customer not purchase a product, than purchase the wrong product.

Tested the patch and working.

No, it can't possibly be working at this point. I know for a fact that the block of code affected by the patch has been substantially changed since the patch was posted last June. And I know the patch doesn't handle textfield or checkbox options because those types of options didn't exist in Ubercart when the patch was created. So RTBC is an inappropriate status.

@itsmahitha: When you review a patch, I would appreciate a more detailed description of exactly what you tested, how you tested, and what version of Ubercart you used for your tests so that others can know what has been confirmed and what hasn't. Also, assigning the issue to yourself means that you will be responsible for re-rolling the patch to address community input (see http://drupal.org/node/314328). If you're willing to do that, that would be great - start with re-rolling this patch so it applies to the 6.x-2.x-dev version of Ubercart and post a new version so it can be tested. If you're not going to do that, then you should take your name off the "assigned to" field.

TR: I am testing the patches on Ubercart 2.2x version. I am working with Andy and Lyle. I was given the task to test the patches and clear the issue queue. Sorry for not posting the detailed message about the testing and my observations.
Can you please give some suggestions regarding how to go about testing the old patches.

Verified the problem the current code doesn't add the attribute SKU to the product in the cart. Instead the default SKU for the product is added to the cart.

The result is the role is not assigned because the wrong SKU is added.

ReRolled the patch and did some object casting so it won't throw errors and comparing the object to the array.
I can't create the patch correctly as I use SVN not CVS at work.

Attaching the SVN patch and if someone is kind they can manually patch a local copy and post the CVS patch.

Still having problems with cart links not assigning SKUs correctly. Tracing the issue I found that in the attribute load operation for some odd reason it doesn't properly evaluate attributes when the value is an array.

When a product is added to the cart from the default "add to cart" button the attribute array is flat, meaning:

$attributes = array(
                           5 => '1'
                          )

But when I add the attribute to the cart using cart links the attribute array results in a multi-dimensional array:

$attributes = array(
                           5 => array(
                                            1 => "1"
                                          )
                          )

stepping through the functions procedurally I can't identify what is causing the weird conversion.

uc_attribute.module - Unmodified

470 $combination = array();
471       foreach ((array)$item->data['attributes'] as $aid => $value) {
472         if (is_numeric($value)) {
473           $attribute = uc_attribute_load($aid, $item->nid, 'product');
474           if ($attribute && ($attribute->display == 1 || $attribute->display == 2)) {
475             $combination[$aid] = $value;
476           }
477         }
478       }

This hack made the function correctly register the model type and pass along the correct SKU

uc_attribute.module

470
470 $combination = array();
471       foreach ((array)$item->data['attributes'] as $aid => $value) {
472         // catch for arrays
473         if (is_array($value)) {
474           $value = array_pop($value);
475         }
476         
477         if (is_numeric($value)) {
478           $attribute = uc_attribute_load($aid, $item->nid, 'product');
479           if ($attribute && ($attribute->display == 1 || $attribute->display == 2)) {
480             $combination[$aid] = $value;
481           }
482         }
483       }

Anyone have any ideas why this occurs?

I don't have a use for assigning attributes with a cart link, but I've looked at the two patches with regard to required attributes, and they worked for me, although I had to modify it a bit. In my original patch on UC.org I set it up so that if a required attribute was not selected, it would throw an error and redirect the user to the main product page. Here's my chunk of that function that handles it:

        if (module_exists('uc_attribute')) {
          $attributes = uc_product_get_attributes($p['nid']);
          foreach ($attributes as $aid => $attribute) {
            // If the attribute wasn't specified...
            if (!array_key_exists($aid, $p['attributes']) ||
                // Or it was, but the option was invalid...
                !array_key_exists($p['attributes'][$aid], $attribute->options)) {
              // Assign the default option to the specified attribute.
              $p['attributes'][$aid] = $attribute->default_option;
              unset($_REQUEST['destination']);
              drupal_set_message(t('You need to specify an option!', 'error'));
              drupal_goto('node/'. $p['nid']);
            }
          }
          // Discard any extra attributes that aren't actually on the product.
          $p['attributes'] = array_intersect_key($p['attributes'], $attributes);
        }

I realize this isn't a proper patch but I need this functionality, and I was surprised to see that it didn't make it into UC2.x. Somehow we've had this happening for users but no one has mentioned it on our site until now, so I had to find a quick solution today, and this was it. Comments welcome. (I'd like to make this a configurable option as well: 1. Assign default; 2. Stop cart action and bring user to product page.

subscribing

I still haven't gotten this issue to work with Product Kits (we don't really use them on our site) - would someone else be able to test it further using those items as a test case? Need to get this fixed.

I could be wrong here, but investigating this issue on my own, it looks like the culprit is uc_cart_links_process() placing the attribute info into the wrong location.
I appear to have just got working cart links with attributes pointing to file download products by changing:

$p['attributes'][$attribute][$option] = $option;

to this:

$p['data']['attributes'][$attribute] = $option;

at line 73 of uc_cart_links.pages.inc; inside the handling of attributes within a cart link.

Testing...

My testing shows my fix works, and I've received emails from others confirming that it works for them. I don't know the correct protocol to make my mod official... can someone else pick this up?

Just to be clear, this is the fix here: http://drupal.org/node/398000#comment-3329102 (comment #20, this same thread).

bsenftner: thank you for this fix (#20)! this issue was causing a lot of headaches on my production site! the correct SKU as definied in the options is now assigned, and roles based on those SKUs are also working correctly.

i'll test it with product kits soon and report back!

Here's a patch for #20. I'll gladly re-roll it as soon as we have Product Kits working, assuming they don't. (But if they do with this patch, then even better!)

I'll mention that one thing this patch does not appear to address is Required options/attributes being ignored. If I have a required option that a user must select before adding the product to their cart, this patch doesn't appear to do anything to address it. My changes in #17 appear to still be required in order to fix that issue (which I think was the original issue I posted here).

Thanks bsenftner!! This was causing me problems and #20 fixed them!

The problem was: when using cartlinks on a download only attribute, the cart didnt recognize that the product was download only and would require a shipping address and all that.

Just confirming, the patch in #20 / #23 doesn't affect required attributes. It looks like a solution similar to mine (or cha0s') will have to be implemented, since uc_cart_links doesn't actually invoke hook_add_to_cart(), which would allow me to stop the adding of the product in its tracks. I'll roll that into a patch shortly.

Im not sure if I should post this here but I will give it a shot. Cart links feature to empty the cart no longer seems to work for me.

This worked in 2.2 = /cart/add/e-p100_q1_a1o1_m0?destination=cart

Now it doesnt empty the cart, but everything else works as expected.

@RikiB: No, that's totally unrelated. See #881752: Cart Links - Missing code for "Empty cart" function

I've tested the patch in #20 against 2.4, and it works for the default attribute. I'm just thinking whether the default behavior should be "add the default option" or "stop the user and ask them" or maybe we should have a configured setting to allow for one or the other? Personally I think it's less confusing to stop the user and force them to choose an option, just as it would if you had the dropdown in front of you.

Erik,

I'm using this modified logic to handle much more than a product's default attribute. In my cart links, I always explicitly include a product's attribute(s), and pretty much ignore the entire 'default' fallback behavior.

I'm a bit confused by your post; are you talking about when a cart link does not specify which product attribute should be added when the user clicks the link? (Essentially, the site developer made an ambiguous cart link, and your post is about how to handle that?)

Right - if you simply have "/cart/add/p123" as your cart link, and your product (nid = 123) has a required attribute, the default attribute is automatically selected as part of the cart link. This is different from the way I was handling it in my earlier code, that is, to bring the user to node/123 with an error message saying that "This product has an option you must select first."

We have a lot of products with different attributes; it's a lot easier to make an ambiguous cart link and not link to a specific attribute, especially when it comes to putting the links in a standardized format, such as an email blast.

Granted, specifying an attribute would take care of that, but I'd rather let the user decide rather than have to provide, say, 10 cart links for each attribute/option combination. You can see how that'd get more difficult the more options a product has. (Also, unless you put something into your product template, the only way for you to find the "a" and "o" values is to edit the product and jot them all down. Again, fairly time consuming thing to do.)

I think the patch in #20 is not along the correct lines. The problem is occurring due to a fix done in last version for handling checkbox attributes which need to be stored in an array. However it wrongly assumes that textfield attributes can also be stored as arrays (as indicated by the comments in the code).

A patch is attached which does the following:

• Checks if the specified attribute is supported by the corresponding product
• Stores the checkbox attributes as array and textfield attribute as string
• In case multiple values have been specified for an attribute, in case of a checkbox attribute, all of them are stored in the array. In case of textfield attribute, previously stored valued is overwritten. So the last specified value of the attribute survives.

Also this patch does not answer the original problem being discussed. So if I should submit this patch somewhere else, please let me know.

Abhaya

The patch in the above post works for me as far as assigning attributes and options. I'll see about combining it with my patches for the original issue.

Any updates on this?

Marked #2079967: Cart links not setting alternate SKU as duplicate

This issue seems to persist in the current version of Ubercart, 7.x-3.6.

Would any of the patches posted here fix this?

When a customer uses the normal Add To Cart button and chooses a product with a required attribute, he cannot proceed without the attribute. But using Cart Link allows him to circumvent this (in my case) critical business rule.