Posted by wodenx on October 4, 2011 at 12:10am
7 followers
| Project: | Ubercart |
| Version: | 7.x-3.x-dev |
| Component: | Orders |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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).
Comments
#1
Patch attached.
#2
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.
#3
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.
#4
I am not really sure what that uid check is meant to achieve in the first place, or what would trigger it to fail.
#5
The last submitted patch, 1298550-abandoned-3.patch, failed testing.
#6
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?
#7
Looks like that was introduced by commit #1645d08e8d293b335e35cbe8600ed90f00582fd4, right around the time I submitted the last patch. Anyway, here's revised version.
#8
#9
Updated to apply to current HEAD. Giving this a bump bc it would be helpful for #649306: Make coupon usage counting more flexible
#10
+ 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.
#11
Incorporated suggestions from #10.
#12
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.
#13
@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.
#14
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.
#15
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.
#16
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).
#17
The last submitted patch, 1298550-abandoned-16.patch, failed testing.
#18
.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.
#19
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.
#20
Committed #14.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.
#22
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:
It looks like 600 is still hard coded into uc_cart.pages.inc in the latest branch.
Patch attached
#23
See #18 and #19. Test your patch with the relevant modules disabled and see if it still works.
#24
I disabled uc_cart -- ran update.php, no problems
I disabled uc_order -- ran update.php, no problems
#25
although i didn't have any updates to perform.. does that matter?
#26
Committed, thanks. This is valid; the only place the constant should not be used is in the update hook in uc_order.install.
#27
Automatically closed -- issue fixed for 2 weeks with no activity.