When a new shopping cart order is created, we initialize the e-mail address on the order to the customer's current e-mail address if they're logged in. While fixing #1242156: illegal username character I realized that users can feasibly change their e-mail addresses prior to checkout, but they'd have no recourse to update the e-mail address on their current cart orders. Since we're initializing the e-mail address on cart orders like this, we should also hook into user account updates and update the default e-mail address on shopping cart orders for the users. This functionality should be implemented by the Cart module, but it shouldn't just be based on the Shopping Cart status; it should go for any of the user's orders in a valid cart status determined by:

  // If the order is in a shopping cart order status, assume it is a cart.
  $is_cart = in_array($order->status, array_keys(commerce_order_statuses(array('cart' => TRUE))));

Obviously we'll need to use the array_keys() return value in an EntityFieldQuery; this code was just copied from commerce_order_is_cart().

CommentFileSizeAuthor
#9 update_cart_order_email_when_user_email_changed-1444680-9.patch1.2 KBJulienD
#7 update_cart_order_email_when_user_email_changed-1444680-7.patch1.19 KBfoopang
#5 update_cart_order_email_when_user_email_changed-1444680-5.patch899 bytesfoopang
#2 update_cart_order_email_when_user_email_changed-1444680-2.patch1.31 KBfoopang

Comments

rszrama’s picture

rszrama commented
Issue tags: +Usability, +low-hanging fruit

Tagging!

foopang’s picture

foopang commented
Status: Active » Needs review
StatusFileSize
new update_cart_order_email_when_user_email_changed-1444680-2.patch1.31 KB

Hi rszrama, please review my patch here, thanks!

simon georges’s picture

simon georges commented

I can confirm the patch applies cleanly (thus, bumping up the issue to bring it to rszrama's eyes again ;-)).

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

The comments in the patch should probably be edited down to stay relevant and inform developers of what the code actually does, rather than repeating the issue summary. E.g., no need to include editorial comments like "While fixing [issue number], etc etc etc"

Perhaps:

 /**
+ * Implements hook_user_update().
+ *
+ * When a new shopping cart order is created, we initialize the e-mail address
+ * on the order to the customer's current e-mail address if they're logged in.
+ * While fixing #1242156: illegal username character I realized that users can
+ * feasibly change their e-mail addresses prior to checkout, but they'd have no
+ * recourse to update the e-mail address on their current cart orders.
+ * Since we're initializing the e-mail address on cart orders like this, we
+ * should also hook into user account updates and update the default e-mail
+ * address on shopping cart orders for the users.
+ */

could just be instead:

 /**
+ * Implements hook_user_update().
+ *
+ * When a user updates their account e-mail address, update their default e-mail
+ *  address for orders currently in the "cart" status.
+ *
+ */

Also a question: does it matter if all of those function calls are part of the condition? Or should they be nested, so we only call commerce_order_statuses() after we've checked for any shopping cart orders (via commerce_cart_order_load($account->uid))? I generally feel it's better to avoid making unnecessary function calls, but I'm not a performance expert. :)

foopang’s picture

foopang commented
StatusFileSize
new update_cart_order_email_when_user_email_changed-1444680-5.patch899 bytes

Yes, torgosPizza, I think your are right. Patch updated, thanks!

rszrama’s picture

rszrama commented
Status: Needs review » Needs work

One more quick change to the patch - right now it will update the current cart order for the user, but it is possible that the user has multiple cart orders (Commerce supports this). So instead of using the commerce_cart_order_load() function, we should use EntityFieldQuery to find any order owned by the user with a shopping cart status.

foopang’s picture

foopang commented
StatusFileSize
new update_cart_order_email_when_user_email_changed-1444680-7.patch1.19 KB

Hi rszrama, thanks for your review. I've updated the patch.

foopang’s picture

foopang commented
Status: Needs work » Needs review
JulienD’s picture

JulienD commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new update_cart_order_email_when_user_email_changed-1444680-9.patch1.2 KB

Hi,

I've tested the patch and it solves the problem. I've tested with a customer having one order and three orders and every orders have been updated.

I've rolled the patch to follow Drupal's coding standards.

Thx foopang for the patch.

foopang’s picture

foopang commented

Thanks JulienD for reviewing the patch and fixing the coding standards of the patch.

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Fixed

Looking in the $edit variable, I saw that it preserves the original user account. This means we can optimize this even further by only running the EFQ if we know the user account e-mail address has been updated. I did that and committed.

Commit:http://drupalcode.org/project/commerce.git/commitdiff/f4b0b4c

foopang’s picture

foopang commented

Thanks rszrama!

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