Spin-off from #1285938: Create order on initial checkout. As suggested, creates a new order status 'abandoned' and assigns it to orders which have timed out, either when the checkout page is revisited, or via cron. I opted not to add a new rules event because it seemed it would be easy enough for admins to implement the order status updated event to do something else when this happened (e.g. delete the order).

Files: 
CommentFileSizeAuthor
#22 1298550-abandoned-22.patch1.26 KBtmsimont
PASSED: [[SimpleTest]]: [MySQL] 2,457 pass(es).
[ View ]
#16 1298550-abandoned-16.patch11.73 KBwodenx
FAILED: [[SimpleTest]]: [MySQL] 1,723 pass(es), 3 fail(s), and 1 exception(es).
[ View ]
#14 1298550-abandoned-14.patch8.42 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 1,728 pass(es).
[ View ]
#11 1298550-abandoned-11.patch7.14 KBwodenx
PASSED: [[SimpleTest]]: [MySQL] 1,692 pass(es).
[ View ]
#9 1298550-abandoned-9.patch2.33 KBwodenx
PASSED: [[SimpleTest]]: [MySQL] 1,643 pass(es).
[ View ]
#7 1298550-abandoned-7.patch2.28 KBwodenx
PASSED: [[SimpleTest]]: [MySQL] 1,328 pass(es).
[ View ]
#3 1298550-abandoned-3.patch2.28 KBwodenx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ubercart/uc_order/uc_order.install.
[ View ]
#1 1298550-abandoned.patch2.16 KBwodenx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1298550-abandoned.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

wodenx’s picture

Status:Active» Needs review
StatusFileSize
new2.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1298550-abandoned.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Patch attached.

longwave’s picture

Status:Needs review» Needs work
   if (!$order || ...) {
+    uc_order_update_status($order->order_id, 'abandoned');

If $order is FALSE for some reason, trying to update its status will result in an error.

I also don't think we should touch the order status if it's not in_checkout for some reason, or if the uid changes. Perhaps the customer called in to complete the order, and the admin manually changed the status while it's still in the customer's session.

wodenx’s picture

StatusFileSize
new2.28 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ubercart/uc_order/uc_order.install.
[ View ]

Good point about the order status. Not sure I understand about the uid - when would an admin change the uid of an order and leave it in checkout? I think as long as the order has timed-out while it's status is in_checkout, it should be marked as abandoned.

longwave’s picture

Status:Needs work» Needs review

I am not really sure what that uid check is meant to achieve in the first place, or what would trigger it to fail.

Status:Needs review» Needs work

The last submitted patch, 1298550-abandoned-3.patch, failed testing.

TR’s picture

Patch failure is "Cannot redeclare uc_order_update_7004()"

This is because there's already a uc_order_update_7004() defined in uc_order.install. The hook_update_N() added by the patch needs to be renamed uc_order_update_7005().

@wodenx: Can you post an updated patch?

wodenx’s picture

StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 1,328 pass(es).
[ View ]

Looks like that was introduced by commit #1645d08e8d293b335e35cbe8600ed90f00582fd4, right around the time I submitted the last patch. Anyway, here's revised version.

wodenx’s picture

Status:Needs work» Needs review
wodenx’s picture

StatusFileSize
new2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 1,643 pass(es).
[ View ]

Updated to apply to current HEAD. Giving this a bump bc it would be helpful for #649306: Make coupon usage counting more flexible

longwave’s picture

+    if ($order && $order->status == 'in_checkout' && $order->modified < REQUEST_TIME - 600) {

Shouldn't that be $order->order_status? Let's also get that "600" moved to a constant rather than using magic numbers.

Would be nice to get a test added for this, it would have to update $order->modified manually to avoid the test having to wait ten minutes, but otherwise should be fairly easy to write.

wodenx’s picture

StatusFileSize
new7.14 KB
PASSED: [[SimpleTest]]: [MySQL] 1,692 pass(es).
[ View ]

Incorporated suggestions from #10.

longwave’s picture

This looks good and works well in testing.

I wonder if we should have a hook_update_N() to do the initial update here, setting the order_status directly; on sites with many "in checkout" orders (perhaps upgrades from D6) the first cron run might take a long time.

TR’s picture

@wodenx: Thanks for adding the tests - that more than anything helps me see how this is supposed to work and convinces me that it works properly.

I agree with @longwave in #12 that the initial update of the status should be done as a batch in a hook_update_N() rather than leaving it to the first run of hook_cron(). Otherwise, I think this is ready to go in.

longwave’s picture

StatusFileSize
new8.42 KB
PASSED: [[SimpleTest]]: [MySQL] 1,728 pass(es).
[ View ]

As this is a new status, nothing should be expecting to react on it, so I think we can safely abandon old orders in a single query.

TR’s picture

Status:Needs review» Reviewed & tested by the community

Tested and works for me on a site which had ~500 In Checkout orders. Update successfully moved all In Checkout to Abandoned. New In Checkout orders were moved to Abandoned via cron after 10 minutes.

wodenx’s picture

StatusFileSize
new11.73 KB
FAILED: [[SimpleTest]]: [MySQL] 1,723 pass(es), 3 fail(s), and 1 exception(es).
[ View ]

Ha - I was just testing it - works well here, too. My only thought was, should we move the definition of the timeout into uc_order so we don't have to hardcode the number in the update hook (see attached patch).

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1298550-abandoned-16.patch, failed testing.

longwave’s picture

Status:Needs work» Reviewed & tested by the community

.module files are not guaranteed to be available during hook_update_N() unless loaded explicitly, which is why I hardcoded the constant. I think the timeout should belong to uc_cart rather than uc_order; the "in checkout" and "abandoned" statuses should probably technically be owned by uc_cart as well, but it's not worth fixing that now.

Back to RTBC for #14, will commit later.

wodenx’s picture

Oh, I see - if update is run when the module is disabled. I guess that makes sense - you want the update_N to work even if that constant is removed in the future.

longwave’s picture

Status:Reviewed & tested by the community» Fixed

Committed #14.

Status:Fixed» Closed (fixed)

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

tmsimont’s picture

Status:Closed (fixed)» Needs review
StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 2,457 pass(es).
[ View ]

Hey not sure if I should re-open or open my own issue, but it looks like something was missed in all of this:

#10:

Let's also get that "600" moved to a constant rather than using magic numbers.

It looks like 600 is still hard coded into uc_cart.pages.inc in the latest branch.

Patch attached

Dan Z’s picture

See #18 and #19. Test your patch with the relevant modules disabled and see if it still works.

tmsimont’s picture

I disabled uc_cart -- ran update.php, no problems
I disabled uc_order -- ran update.php, no problems

tmsimont’s picture

although i didn't have any updates to perform.. does that matter?

longwave’s picture

Status:Needs review» Fixed

Committed, thanks. This is valid; the only place the constant should not be used is in the update hook in uc_order.install.

Status:Fixed» Closed (fixed)

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