Default renew (uc_file, uc_role) should contain conditionon on "username != Anonymous". It will prevent renew triggers locking after checkout. Here's my explanation from Ubercart's forum:
I almost cracked my head by debugging this locks today. But I finally got the reason. And I should say that we don't need to remove locking functionality to avoid locks on files.
Here's the usual after-checkout routine:
(customer payed at pay page, and are being redirected to complete page)
Under the hood:
- Added full payment, which triggering Full Payment Action, which sets Order status set to Completed.
- ... which is triggering Renew predicates (1). But in our $order object, uid == 0, so Renew just exits and locks are being set.
- Right after it, order_complete_sale() are being called, which sets order to Pending.
- Complete Checkout Trigger than are being called, which again sets Order status to Completed.
- ...which is trying to call Renew predicates that are already locked.
So, to avoid locking, we just need to add condition on UID != 0 for all Renew predicates.
Adding this checks by hands solved whole problem for me, but I'm sure it should include this conditions by default.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 426914.renew_conditions.2.x.patch | 2.54 KB | cha0s |
| #4 | 426914.renew_conditions.2.x.patch | 2.65 KB | cha0s |
Comments
Comment #1
cha0s commentedIs ther an existing CA condition that checks an order user's UID that I'm missing? If not, it will be easy to create, I'm just wondering if you were using a preexisting one, 'cause I don't see one...
Comment #2
neochief commentedI made it at my site using Username check (there's possibility to check Anonymous users there).
Comment #3
cha0s commentedd.o can be weird sometimes ;p disregard this post O.o
Comment #4
cha0s commentedWell, the solution is great, and a nice way to work around the predicate locking. I'm still of the opinion that we should probably remove those locks... however this at least *works*.
I also found that global $user wasn't getting imported in the file renew function.
This patch working properly also depends on the patch at #431140: uc_order_condition_user_name() often fails anonymous user check....
Comment #5
neochief commentedNice patch, thanks! Glad this issue will be fixed.
Comment #6
Island Usurper commentedPredicate locking has been removed. Is this patch still needed?
Comment #7
cha0s commentedIt isn't necessary, per se. It's more efficient to do things this way, though.
Comment #8
Island Usurper commentedI don't think it's a good idea to take out the checks on the $order_user->uid. Even with the conditions added to the predicates, it just feels like asking for trouble.
Comment #9
cha0s commentedCurious, is the 'not an anonymous user' Conditional Action condition broken, then?
Comment #10
Island Usurper commentedNo, I'm just worried that someone will try to remove that condition and then there could be some inconsistent data with the files and orders.
Comment #11
rszrama commentedTagging.
Comment #12
cha0s commentedBefore we solve this, I'll ask what your thoughts are on making some conditions/actions unable to be removed? I can't see why anyone would want to assign a role to UID 0 based on an arbitrary purchase by one customer.
Can I get some current thoughts on this, I've brought it up in the past...
Comment #13
vitis commentedsubscribing
Comment #14
Island Usurper commentedWell, no one should want to assign files or roles to the anonymous user, since it wouldn't work. That's why the predicates now wait until the "customer completes checkout" trigger to change the order status to completed, instead of right when the payment is made.
All I'm saying is that I think of predicates as user input, and therefore not to be trusted. There's also nothing stopping someone from calling the action's function on their own. It would be better to display an error message in the watchdog than to go through the process of trying to grant the file or role to nobody.
Putting the condition on the predicate makes it clearer to the user what's happening, but leaving the check in the action is safer.
Comment #15
bkosborneLooks like all this was fixed up as of UC 2.4
However, I offer downloadable and shippable products together in my store. It is possible for someone to complete an order with both types...
According to existing conditional actions, if a user were to purchase just a downloadable product, there is a trigger/action that will move the order status from payment_received to completed. Doing this then triggers the predicate & conditions to renew file downloads and email the user.
If someone purchased a shippable product as well, the order will just go to payment received and sit there. This is as it should be, because it is assumed that the product needs to be prepared and then shipped, before moving it to "complete".
The problem is that I can't change the condition for renewing files/sending files email from "complete" to "payment received" (as I would like it to be), because at that point the UID in the $order object is still 0.
Does anybody know of how I can fix this? I guess Ubercart doesn't create an anonymous users account until after that trigger is pulled....
Comment #16
longwave@bkosborne: the patch in #644538-230: Duplicate order notification e-mail, and duplicate stock decrement may help with this; it ensures the account is created first, then orders always move from in_checkout to payment_received and then to completed only if they were not shippable. This means you could change the file/role predicates to trigger on "payment received" and it should work with both shippable and non-shippable items.
Comment #17
j0rd commentedI've explained some issues related to Ubercart, Anonymous Checkout & UC File purchases here:
http://drupal.org/node/1227076
Comment #18
longwaveThis won't be fixed properly by adding new conditions; instead the order of events should be fixed so anonymous users are always given accounts before firing any of the related actions, as explained in the linked issues above.