Comments

cslevy created an issue. See original summary.

cslevy’s picture

Assigned: Unassigned » cslevy
cslevy’s picture

StatusFileSize
new45.88 KB

Implemented commerce wishlist for anonymous users.

cslevy’s picture

Assigned: cslevy » Unassigned
Status: Active » Needs review
nvahalik’s picture

Status: Needs review » Needs work

I know this patch is only 12 days old, but -dev saw a lot of changes in just the past couple of days. It'll need a re-roll.

In general, though, I tend to like what I see in the patch, but can't really do a review until the patch gets updated.

patching file commerce_wishlist.admin.inc
patching file commerce_wishlist.module
Hunk #1 FAILED at 11.
Hunk #2 succeeded at 77 with fuzz 2 (offset 33 lines).
Hunk #3 succeeded at 251 (offset 131 lines).
Hunk #4 succeeded at 277 (offset 141 lines).
Hunk #5 succeeded at 302 (offset 147 lines).
Hunk #6 FAILED at 322.
Hunk #7 FAILED at 467.
Hunk #8 succeeded at 572 with fuzz 2 (offset 242 lines).
Hunk #9 succeeded at 608 (offset 242 lines).
Hunk #10 FAILED at 653.
Hunk #11 FAILED at 713.
Hunk #12 succeeded at 738 (offset 250 lines).
Hunk #13 FAILED at 748.
Hunk #14 succeeded at 816 (offset 277 lines).
Hunk #15 FAILED at 867.
Hunk #16 FAILED at 915.
Hunk #17 succeeded at 948 (offset 286 lines).
Hunk #18 succeeded at 1009 (offset 286 lines).
Hunk #19 FAILED at 1051.
Hunk #20 succeeded at 1060 (offset 287 lines).
Hunk #21 succeeded at 1104 (offset 287 lines).
Hunk #22 FAILED at 1136.
Hunk #23 succeeded at 1183 with fuzz 2 (offset 287 lines).
Hunk #24 succeeded at 1222 with fuzz 2 (offset 287 lines).
Hunk #25 succeeded at 1362 with fuzz 2 (offset 404 lines).
10 out of 25 hunks FAILED -- saving rejects to file commerce_wishlist.module.rej
patching file includes/views/commerce_wishlist.views_default.inc
Hunk #1 FAILED at 45.
1 out of 1 hunk FAILED -- saving rejects to file includes/views/commerce_wishlist.views_default.inc.rej
patching file includes/views/handlers/commerce_wishlist_handler_field_remove.inc
Hunk #1 FAILED at 27.
1 out of 1 hunk FAILED -- saving rejects to file includes/views/handlers/commerce_wishlist_handler_field_remove.inc.rej

jmarr’s picture

I'm going to say this as calmly and politely as I can......It stuns me that someone would build a commerce wishlist module that does absolutely nothing for 99.0999999% of visitors to an e-commerce site. My suggestion (and I'm, only a full time e-commerce merchant so what do I know?) is that whoever is working on this module DROP what whatever you are doing and address the glaring problem!

Or failing that put a bold face disclaimer on your web page saying that anonymous users of sites using this will click a button that does nothing...and then call the merchant in fuming anger to say they will never shop on that site again since it is obviously run by incompetent clowns!

I understand that Drupal is a volunteer effort but please...if you don't know e-commerce enough to see a major failing in your module you're probably better off not contributing to it.

nvahalik’s picture

Hi jmarr, will gladly consider any patches you have to contribute to this issue!

andyg5000’s picture

Hey @jmarr. You sound like a nice guy. Thanks for all your contributions to Drupal!

jmarr’s picture

nvahalik, I'm not a developer but I really wanted to get the message through that things like this - so glaringly obvious and so harmful to real world users - are sinking Drupal. Most of my peers have moved to other platforms simply because the number of incidents of things like this is really intolerable. Don't shoot the messenger.

andyg5000’s picture

Dear @jmarr. We don't shoot messengers only ass holes

"I understand that Drupal is a volunteer effort but please...if you don't know e-commerce enough to see a major failing in your module you're probably better off not contributing to it."

Have fun with woo commerce.

cslevy’s picture

@jmarr

I don't really understand what is your problem. This is an extra functionality. If you use it or not is your decision. But maybe someone need this functionality.

nvahalik’s picture

@jmarr, if you'd like to fund the development of this module, I'd be happy to help point you in the right direction. These modules grow organically from the needs of our clients and this work rarely gets done for free.

So if you're interested in contributing: code or cash is cool, but comments about how you can't believe this doesn't work the way you want it to are not helpful.

jmarr’s picture

My apologies to anyone who saw my input as anything but what it was. I guess I should have kept my criticism to myself.

kfitz’s picture

StatusFileSize
new84.07 KB

I'm not entirely sure why the patch fails to apply cleanly, but I've attempted to clean it up as much as possible. Applied patch against commerce_kickstart-7.x-2.36. @cslevy, hopefully you can go through this and make the necessary changes.

kfitz’s picture

Adding patch referenced in #14

thejacer87’s picture

Issue summary: View changes
StatusFileSize
new47.54 KB

kfitz's patch in #14 gives me a white screen when applied. cslevy's patch in #3 doesn't apply properly and has quite a few errors. I've cleaned it up so my patch applies and fixes some errors from the original patch.

I can add things to my wish list (as a user and anonymous) but if you try to view your wishlist the site crashes: Call to a member function set_display() on null in /home/.../modules/contrib/commerce/commerce.module on line 358>.

The original patch tried to change the user wishlist view and things broke there. You cannot open the view from the admin menu.

Anyway here it is. Like I said, it should apply. I think the commerce_wishlist.views_default.inc file will need to get fixed up before any more work can be done.

thejacer87’s picture

StatusFileSize
new102.58 KB

also here are some errors I got after I tried adding an item to wishlist

cslevy’s picture

What I discovered when I exported my first patch, and try to apply on the original code, it was only applying partially. The modifications on the vies handler wasn't applied. Maybe if you apply manually the changes on views handler then will work. Sorry i didn't have time to investigate further.

thejacer87’s picture

@cslevy, can you explain what/where to make the changes in the views handler? I plan on working on this issue very soon.

cslevy’s picture

@thejacer87
The changes that need to be done are in the patch file, but they doesn't apply for some reason. You can check the difference in the patch file.

thejacer87’s picture

Holy smokes that was crazy. Here is a BRAND NEW patch. Pretty much went line by line with cslevy's patch. Then made adjustments for the view and other stuff. At the end of the day, the anon user can add stuff to a wishlist that is stored in the session. And when you login, those items get appended to a wishlish that you may have already had. The functionality is all there.

I believe that there are some access argument issues for viewing/updating the wishlist. If anyone wants to look at those or help me sort it out, let me know.

Also the wishlist table got messed up somehow (see attached png). If anyone can point me in the right direction for that, or fix it up, please do.

Please test and review!!

thejacer87’s picture

Status: Needs work » Needs review
joshmiller’s picture

Status: Needs review » Needs work

git apply -v commerce_wishlist-anonymous-wishlist-2677086-21.patch

produces ...

Checking patch commerce_wishlist.admin.inc...
Checking patch commerce_wishlist.module...
Hunk #6 succeeded at 315 (offset 2 lines).
Hunk #7 succeeded at 334 (offset 2 lines).
Hunk #8 succeeded at 421 (offset 2 lines).
Hunk #9 succeeded at 444 (offset 2 lines).
Hunk #10 succeeded at 595 (offset 2 lines).
Hunk #11 succeeded at 603 (offset 2 lines).
Hunk #12 succeeded at 618 (offset 2 lines).
Hunk #13 succeeded at 696 (offset 2 lines).
Hunk #14 succeeded at 722 (offset 2 lines).
Hunk #15 succeeded at 740 (offset 2 lines).
error: while searching for:
  // back the changes.
  $line_item = commerce_line_item_load($form_state['line_item']->line_item_id);
  $line_item->data['wishlist_products'][] = array(
    'node_id'    => $form_state['values']['wishlist_nid'],
    'user_id'    => $form_state['values']['wishlist_uid'],
    'product_id' => $form_state['values']['product_id'],
  );


error: patch failed: commerce_wishlist.module:667
error: commerce_wishlist.module: patch does not apply
Checking patch includes/views/commerce_wishlist.views_default.inc...
Checking patch includes/views/handlers/commerce_wishlist_handler_field_remove.inc...

So we need to base your patch off the latest dev release before additional testing can commence.

thejacer87’s picture

Status: Needs work » Needs review
StatusFileSize
new53.69 KB

Here it is. Applies cleanly to 7.x-3.x!!

1 thing to look at:

when you click "allow anons to have a wishlist" in the module settings form, you still have to grant the "manage own wishlist" permission to anon users (the patch notes this in the form element description). Should the module not automatically grant that permission when that checkbox in the settings form is selected? Is that even possible (i've never granted permission programmatically before)

thejacer87’s picture

Here, use this patch. There is something funky going on with the "remove from wishlist" rules action that automatically removes teh item from the wishlist on events such as completing a purchase...

thejacer87’s picture

liamanderson’s picture

After applying the patch, during testing I came across the following issues:

  • As logged in user:
    1. Add a product to wishlist
    2. Click on the "your wish list" link in the success message
    3. Error is thrown:
      Notice: Undefined property: stdClass::$commerce_product in commerce_wishlist_handler_field_add_to_cart_form->render() (line 37 of /home/landerson/sites/patch_2677086/wwwroot/sites/all/modules/commerce_wishlist/includes/views/handlers/commerce_wishlist_handler_field_add_to_cart_form.inc).
      Notice: Trying to get property of non-object in commerce_wishlist_handler_field_add_to_cart_form->render() (line 40 of /home/landerson/sites/patch_2677086/wwwroot/sites/all/modules/commerce_wishlist/includes/views/handlers/commerce_wishlist_handler_field_add_to_cart_form.inc).
      Notice: Trying to get property of non-object in commerce_product_line_item_populate() (line 1376 of /home/landerson/sites/patch_2677086/wwwroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/product_reference/commerce_product_reference.module).
      Notice: Trying to get property of non-object in commerce_product_line_item_populate() (line 1382 of /home/landerson/sites/patch_2677086/wwwroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/product_reference/commerce_product_reference.module).
      EntityMetadataWrapperException: Invalid data value given. Be sure it matches the required data type and format. in EntityDrupalWrapper->set() (line 737 of /home/landerson/sites/patch_2677086/wwwroot/profiles/commerce_kickstart/modules/contrib/entity/includes/entity.wrapper.inc).
  • As an anonymous user:
    1. Click "Add to Wishlist" button on a product page
    2. Warnings as displayed:
      Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 175 of /home/landerson/sites/patch_2677086/wwwroot/includes/entity.inc).
      Please log in or register to add this product to your wish list.
      Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 175 of /home/landerson/sites/patch_2677086/wwwroot/includes/entity.inc).

Notes:
Tested with kickstart 2.x. 'Display "Add to Wishlist" for anonymous users' and 'Show "Remove from Wishlist"' both enabled.

Let me know if you have any questions or cannot reproduce.

joshmiller’s picture

Status: Needs review » Needs work
thejacer87’s picture

ok, so here is the patch. this one takes car of the issue i mentioned in #24 where checking the allow anon wishlists will also change the permission.

i dont know why or how the array_flip() error is happening stack overflow says its probably from an older version of a xxx_load() function. there are a few user_load() calls in the module, also some commerce_product_load() calls. maybe those are the issues. i am having trouble tracing the call to anything in the wishlist module though.

thejacer87’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

zdenkov’s picture

I think I found a cause for error described under #27 (wishlist page).
It seems that in the file "includes/views/handlers/commerce_wishlist_handler_field_add_to_cart_form.inc", you have to fetch product ID from the commerce line item before fetching a product.
So, in function render($values), the line
$product = commerce_product_load($wish_list_line_item->commerce_line_items[LANGUAGE_NONE][0]['line_item_id']);
should be changed to:

$line_item = commerce_line_item_load($wish_list_line_item->commerce_line_items[LANGUAGE_NONE][0]['line_item_id']);
$product = commerce_product_load($line_item->commerce_product[LANGUAGE_NONE][0]['product_id']);

Well, at least it works for me. I don't know how to prepare a patch so I leave it to you guys.

liamanderson’s picture

Status: Needs work » Reviewed & tested by the community

Successfully tested patch #29 on Kickstart 2.
Good job everyone!

The following was tested:

  1. Add to wishlist as anonymous user
  2. Logging in merges wishlist
  3. Add to wishlist as logged in user
  4. New browser instance/session shows merged wishlist is stored correctly when logged in
  5. Removing from wishlist when logged in

Note:
I don't think this is an issue I just wanted to make note of the behaviour when following the steps below:

  1. Add to wishlist as anonymous user
  2. login to merge wishlist
  3. logout
    • results: Wishlist is now empty because it is attached the user not the session
liamanderson’s picture

liamanderson’s picture

lubwn’s picture

Applied patch successfully, thanks a lot for this! But I do not seem to be able to get it working. I checked wishlist configuration and checked anonymous access but clicking on "add to wishlist" link as anonymous user still redirects to login page. Everything is working super good when logged in (as admin) but not as anonymous user. Anything I missed? Maybe some configuration. Also checked permissions but still can not get it to work. Thanks a lot!

lubwn’s picture

I actually got it working. I was applying patch to release version instead of dev version. Now it works nicely. Thanks.

AusJohn’s picture

I'm getting array_flip errors as well. Trying to get to the bottom of it.

I applied patch 27 to core includes/common.inc from here to try backtrace this error some more.

So far I got this. 'false' is being passed as an ID, this causes the error.
I'll try dig deeper but, I think I'm in way over my head with this one.

Only local images are allowed.

AusJohn’s picture

StatusFileSize
new179.14 KB

Image of xdebug

AusJohn’s picture

I could fix the array_flip() issue.

The issue is this will only accept an int or string.
The wishlist id doesn't exist for Anon, as anon has no order in the system. If they have something in Cart, then it might work.

I fixed this by checking if $wishlist is FALSE, then change it to an INT value of 0.

function commerce_wishlist_user_has_product_in_wishlist($product_id, $wishlist = NULL) {
  global $user;
  $uid = $user->uid;

  if (empty($wishlist)) {
    $wishlist = commerce_wishlist_order_id($uid);
  }

  // Drupal expects value to be an int or string.
  // When a user has no order in DB, $wishlist is set to FALSE.
  // If wishlist ID is false, set it to 0. When the user makes an order by
  // either adding to cart or adding to wishlist, a wishlist order ID starts to exist.
  if($wishlist == FALSE){$wishlist = 0;}

  $wishlist = array($wishlist);

Hope this helps anyone suffering from the same issues.