hook_uc_checkout_complete() is called outside of checkout process (when payment is entered)

#1579970: Email sent when staff enter Payment Received only if they use the appears to describe the same issue.

Splitting "user must have an account" out of uc_cart_complete_sale() seems to make sense.

Marked #1340782: In 6.x-2.7 entering payment now causes email to be sent out as duplicate

Marked #1915564: Payment Entered Sends "New Order" Email as duplicate

I'm confused at how to resolve the issue. Is there a patch to fix this? How can I "split the 'user must have an account' out of uc_cart_complete_sale()" ?

There is no patch to fix this yet; that is just a hint for any developer that wants to try and tackle this.

There is no patch to fix this yet; that is just a hint for any developer that wants to try and tackle this.

Might as well see what happens when the tests run..

I don't think that uc_payment_enter() should execute uc_cart_complete_sale(). It breaks the work flow of hook_order() submit op. It also breaks anonymous checkout password display on the order completion page.

function hook_order
submit: When a sale is being completed and the customer has clicked the Submit order button from the checkout screen, the hook is invoked with this op. This gives modules a chance to determine whether or not the order should be allowed.

The example below, using uc_authorizenet, shows the problem. The order is completed before hook_order() submit op is run.

0.99092500 1371422209 uc_cart_checkout_review_form_submit begin
0.99158600 1371422209 uc_authorizenet_charge begin
0.97482000 1371422234 uc_cart_complete_sale begin **** should not happen here
0.08070500 1371422235 hook_uc_checkout_complete begin 
0.08131900 1371422235 hook_uc_checkout_complete end
0.34281300 1371422235 hook_order (op = submit) begin **** should complete before hook_uc_checkout_complete
0.36137600 1371422235 uc_cart_checkout_review_form_submit end
0.36129000 1371422235 hook_order (op = submit) end 
0.57265200 1371422235 uc_cart_checkout_complete begin
0.60414000 1371422235 uc_cart_complete_sale begin **** modules not invoked for uc_checkout_complete again

I don't understand why we have to "Ensure user has an account before payment is made." Ubercart has entered payments with uid 0 before. The docs say "The user ID of the person logging the payment, or 0 if the payment was processed automatically".

It is not meant to be the uid of the customer account. The payment entered hook and CA should not be used to perform actions on the customer account because the order has not successfully completed.

This breaks any module that uses hook_order('submit') to cancel order processing and would allow orders to complete when they should not so I think it is a major issue.

Other post of interest:
#1371760: Doesn't seem to work with ubercart 2.7
#898050: hook_order('submit', ...) invoked by paypal module before order actually is submitted
#1423482: Anonymous checkout for Recurring Product not listed properly under Subscription Manager and My Account
#1558410: Account created - password is "your password"

We have to ensure the user has an account before payment is made, because uc_roles (and possibly some contrib modules) are triggered when payment is received, but the account needs to exist for the role to be attached to it. We have an automated test to ensure this works, which fails when your patch is applied.

@longwave, The uc_role conditional action is wrong. It should not assign roles when a payment is entered because there is no guarantee that a user exists. Forcing premature checkout completion is not a solution. This action should process on trigger 'customer completes checkout' with condition 'order balance <= 0'. This could alternatively work with condition: 'check order status' is 'payment_received' or 'completed' but I don't like using order status trigger, since order status can be customized, and just add complexity.

I have attached another patch which:
- Removes uc_cart_complete_sale() from uc_payment_enter()
- Modifies the roles renewal CA to trigger on 'customer completes checkout' with condition order balance <= 0.
- Change order comment user to 0 since the customer is not granting the roles; the system is doing it.

As for other contrib modules: Breaking hook_order() op 'submit' is far worse then breaking some contrib CAs. Other modules should use 'customer completes checkout' if they want to modify the customer account. I tried to find modules that use trigger 'uc_payment_entered' and modify the customer account but could not find one. Do you know of any?

Oops, looks like I did not search very well. Seems uc_file has to be updated too.

OK the simpletest failed because it did not add payment then complete checkout so the new CA condition was not satisfied. Hopefully this patch will solve that.

I would prefer it if we could fix this without modifying any existing tests; feel free to add new ones, but the point of the tests is to prove we aren't breaking existing behaviour.

I also would like to avoid modifying the default CA rules if possible, because there are thousands of existing stores that may have already modified these rules. If this code is committed and they upgrade, they will have to manually upgrade their rules as well, or their store may not work as it did before. This does make fixing this issue much more difficult than it should be.

Modifies the roles renewal CA to trigger on 'customer completes checkout' with condition order balance <= 0.

The problem with doing this is that many payment methods complete checkout before payment is received, including PayPal where the IPN notification can be delayed many minutes or hours after the user has reached the "thank you for your order" page, or for example stores where users can mail checks that are not received for several days. Basically, we cannot guarantee the order in which users will complete checkout and full payment will be received, but we do not want roles or file downloads to be granted until payment is actually received, so that is the only safe trigger to use.

@longwave,

Just to reiterate: this is the current faulty work flow:
hook_order() op submit - modules start validating
-> uc_credit_order() executes - this is where the problem begins
-> uc_payment_process() - if successful other modules cannot stop the order anymore because
-> uc_payment_enter() -> uc_cart_complete_sale() - executes before other modules complete hook_order op submit
-> hook_order op submit continues - finishes processing using old data that causes sync issues.
-> uc_cart_complete_sale() - runs again but this time does not invoke uc_checkout_complete. Modules that counted on this to complete the order executed prematurely and may not have access to data that should have been created during hook_order() op submit.

Any module that comes before uc_credit in the module_list() can process and stop the order processing. All other modules cannot stop processing and get messed up. The only reason this is not causing huge problems is that uc_credit is typically close to the end of module_list().

Are we in agreement that hook_order() op submit is currently broken?
If so how can a module stop an order from completing?
If there is no way then don't we have to fix this issue?

I have gone through the code of the most of the payment modules included in ubercart (and tested some). This includes: uc_2checkout, uc_authorizenet, uc_cybersource, uc_google_checkout, uc_payment_pack, and uc_paypal

uc_2checkout, uc_cybersource, uc_google_checkout, uc_paypal bypass the 'cart/checkout/complete/' which contains uc_cart_complete_sale() and execute it later on custom checkout pages. All of these modules call uc_payment_enter() and uc_cart_complete_sale() so obviously they do not expect it to have uc_cart_complete_sale() in uc_payment_enter() and will not be affected if it is removed. In fact having it fire twice is a bug for them. The only reason it's not a visible problem is that they have uc_cart_complete_sale() and uc_payment_enter() within a few lines of code.

uc_authorizenet uses uc_credit and the standard checkout which again would not be affected by removing uc_cart_complete_sale() from uc_payment_enter(). Any module that uses uc_credit currently breaks hook_order() op submit with the current bug.

So I think it is not only safe to remove uc_cart_complete_sale() from uc_payment_enter() but I think it will be beneficial.

I decided to take a different tack and modified uc_cart_complete_sale() and uc_cart_complete_sale_account(). I moved around some code to cleanly split out what is required to load/create an account during checkout and what is needed to complete the sale. I renamed uc_cart_complete_sale_account() to uc_cart_user_load() which will load/create the user account. I have replaced the call to uc_cart_checkout_complete() in uc_payment_enter() to use uc_cart_user_load to ensure and account exists.

No changes to CAs or simpletest.

The exceptions in the previous test were my fault but the fails are due to faulty simpletest. Since the patch removes uc_cart_complete_checkout() from uc_payment enter() then it has to be added later if orders are created using functions rather than going through checkout. The rest of the errors were caused by a faulty simpletest filter of emails looking for $email['param']['id'] which never exists because it's supposed to be $email['id'].

Well I'm sort of glad #22 failed to test because I worked out a more elegant method that allows any module to use uc_cart_user_load() without having to check if an order is in checkout. It also maintains $order->data['complete_sale'] for backwards compatibility.

OK so now that it passed simpletests I will explain what happens to the lay man.

New function uc_cart_user_load($order) returns user account:

• Can be used as a replacement for user_load() in any module that executes during checkout if:
  • An account is required
  • It is not known that a user account exists
  • Needs to retain what type of checkout is executed.
• If checkout has completed then it simply returns the same as user_load() using $order. Otherwise:
  • Follows the existing conditions used to set $order->data['complete_sale'] but temporarily save it as $order->data['checkout_sale'].
  • If new_user then save password to $order->data['new_user']['password'] so that it can be used when checkout completes. Pass the password to $account->password and $order->password so that emails and on screen messages have the info.
  • Modify order to keep track of info so that we know we are in checkout and what type of checkout.

When uc_cart_complete_sale() executes:

• Retains the safety check to make sure it is only run once using $order->data['complete_sale']
• Executes uc_cart_user_load($order) to load or create the account and linking account to checkout.
• Set $order->data['complete_sale'] = $order->data['checkout_sale']. This ensures that checkout is run once, knows what type of checkout, and reverts uc_cart_user_load() to user_load() when the order is completed.
• Clear temp data so no sensitive info is saved in database.
• Executes using the same conditions as pre-patch.

Before this gets committed:

• Test with all payment methods and make sure that they:
  • Bypass url 'cart/checkout/complete' and execute uc_cart_checkout_complete() when payment received.
  • OR use the default checkout page.
• Test conditional actions based on order status change when status is pre-checkout. This should have no problem due to uc_cart_user_login() ensuring user account.
• Update simpletests to make sure that order completes checkout by either executing uc_cart_complete_sale() or submitting the checkout review form.
• This essentially creates a user account when needed. It makes sense when a payment is entered but may create unneeded user accounts that are never used if hook_order() op submit returns FALSE. There may be a need to delete accounts. This should follow suit with clearing 'in_checkout' order if there is an action.

I need some time to review this but as the tests are passing this is looking quite good now, thank you for working on it!

However, can we rename uc_cart_user_load() to something else? There isn't a problem in 6.x but when this is forward ported to 7.x this function name will clash with hook_user_load().

Yes of course it can be renamed! I would be very happy if this made it in since its causing me problems with uc_recurring and would cause problems on a new module I am working on. Of the top of my head it could be 'uc_cart_user_check' or 'uc_cart_user_ensure' -- if you want to maintain some history ;)

Hate to sound impetuous but how can this issue be moved ahead? Since discovering it I have realized it is the cause of many Conditional Actions, Rules, Recurring and delayed Payment processing, and other errors in modules that execute based on the Ubercart checkout process when customers are not logged in. Maybe other users do not realize this is the source of many problems.

If there is anything I can do to help move this along please let me know.

Apologies, this fell off my radar, and I haven't been looking at many 6.x-2.x issues recently. This needs fixing in 7.x-3.x as well, however.

The patch no longer applies since #1983358: Blocked user gets active account email in anonymous checkout but I have rerolled it and made a few minor changes. uc_cart_user_load() is now uc_cart_get_order_user(), but it feels like this should just belong to uc_order.module instead? Also, if there is a bug in one of the tests, we should fix that at the same time; I've removed the email ID to see what happens.

The major problem I can see with this approach: we are not allowed to store plaintext passwords in the database, which is done here via $order->data['new_user']['password']. If the user does not specify a password, the patch stores the password for later use, as this is needed when sending the account creation email later. I am not sure what to do about this. Maybe the only solution is to create the account as early as possible during checkout?

Needs work, based on the password issue described above.

@tinker: Any suggestions or comments you have here that may help move this forward would be gratefully received!

@longwave, The user account is created, if it does not exist, on the first call to uc_cart_user_load() the same way as it did before on the first call to uc_cart_complete_sale(). The password is only stored so that it can be displayed to the user on the order completion page. Looking at the original code I see the password does not get passed if uc_cart_complete_sale() was executed before checkout so I guess this should happen the same way.

Is it OK to have the unencrypted password in a session variable between page loads?

Here's direct re-roll of #29 for Ubercart 7.3.

Second patch is my attempt to solve password problem. Since we show the password only in case of newly created, not logged in account, we could just generate a brand new password right before serving the page with password to user.

32: ubercart-7.3-1669968-secure-password.patch queued for re-testing.

Finally got around to looking at this again. Had to pick up from patch #24 as subsequent ones all had issues. This essentially follows work flow outlined in #25 but removes the password issue indicated by @longwave in #29. The password functionality remains the same as current ubercart implementation. If the raw password is lost between page loads, or user entered a password, then they will see 'Password : Your password' displayed. Store emails no longer send out passwords so that is one less problem.

Some additional important points:

1. I have renamed the cart user loading function to uc_cart_user_ensure(). This function now takes $order by reference, updating the current order object and saving only $order->data and $order->uid to database if it changes. It returns the loaded customer account.
2. uc_payment_enter() loads $account using variable $uid passed to it. In patch #24 I incorrectly loaded the order customer account. Looking at hook_uc_payment_entered() I see that $account should be the user who entered the payment. uc_cart_user_ensure() is there to make sure $order->uid is not 0.
3. The new customer email will be sent out when uc_cart_user_ensure() is first called. If the order is not completed the user account will still be available for use.
4. The is a block of code commented out with comment 'Generate a new password...' This code came from the idea by @neochief but unfortunately failed testing. Since this was a luxury feature that improves on current operation I have left it commented out as I want this patch to get accepted ASAP.
5. testCheckoutRoleAssignment() still has to be patched adding uc_cart_complete_sale() because it has been removed from uc_payment_enter(). testCheckoutComplete() uses uc_cart_complete_sale() the same way so this not fudging the test.
6. I have run all the ubercart simple tests locally and did not get any failures. (1 exception in Attribute Checkout which should be in no way related to patch)

One question is a new user who is logged in supposed to get the drupal new user email. If so then a bit of code needs to be re-ordered.

As I mentioned 2 years ago in #10 I think that current ubercart implementation of uc_payment_enter() is broken, does not match docs, and causes many issues for any module that uses hook_uc_order($op == 'submit'). This definitely affects uc_recurring if a CIM profile fails to be created allowing the order to complete when it should not. I also suspect that Paypal IPN has problems with it too. I really want this patch to be accepted so that ubercart can return to proper functionality. I will have time to work on this further for the next month so please let me know if there is anything else I can do.

Hi again tinker and thanks again for all the work that you have done on this issue. I am going to apply the patch to our test site and run some orders through the PayPal sandbox. Once that is complete, we'll move to apply the patch to our live sites. Perhaps this will solve our duplicate stock decrement/email problem we have been experiencing intermittently for the last year or so. Will report back on our progress but based upon your very logical outline of the order flow, I would think that this patch should be committed.

One very interesting observation is a note that the PayPal IPNs are coming back very quickly now. Clearly PayPal has improved their server infrastructure. My rather simple mind wonders if this might relate to the fact that we are seeing this problem more often now.

Al

Related issues:

• #2490446: hook_uc_checkout_complete being called before hook_uc_order ($op='submit') Closed as duplicate of this issue
• #2404913: Stock being reduced twice for one order Appears to be fixed by patch #37

I also suspect that since Ubercart 6.x-2.7 any workflows being executed twice (resulting in doubling) based on the following are related:

• Rules/CAs based on order status 'completed'
• Rules/CAs based on event 'uc_checkout_complete'
• Modules using hook_uc_checkout_complete()
• Modules using hook_uc_order($op = 'submit')

This probably includes the following issues:

• #2321633: Wrong Expiration Date Calculated for Expiring Roles
• #2143347: PayPal stuck in Pending due to IPN received twice from PayPal
• #2129243: rules condition for admin created orders
• #1736846: Duplicate Customer and Admin Emails Sent on Checkout
• #1356310: Stock decreases 2x instead of 1x for any item after checkout?

I am sorry to say that the patch in 37 did not work for me. I am still receiving duplicate IPNs for new users only. Existing users do not seem to encounter this error. I am using the uc_roles module so the new members accounts are stuck in Pending and they do not yet have the "member" role applied to gain access to member content. I would like to help test and will try to add as much relevant detail as possible. I'm not sure if it's worth noting but my web host is using PHP 5.5.x

This thread seems long dead at this point, but I went hunting in D8 to see whatever became of this.

It looks like the D8 version does still essentially call uc_cart_checkout_complete() (again seemingly the whole checkout complete - but just to ensure an account??) but it does it in a different way:

Basically now uc_payment_enter() just creates a PaymentReceipt entity.

Then uc_cart picks up and runs complete sale like this:

/**
 * Implements hook_uc_payment_receipt_insert().
 *
 * Ensure the user has an account when payment is made.
 */
function uc_cart_uc_payment_receipt_insert(PaymentReceiptInterface $payment) {
  \Drupal::service('uc_cart.manager')->completeSale($payment->getOrder(), FALSE);
}

I don't have time to dig any deeper right now, but I thought this might be helpful to anyone else who lands here.

I should have mentioned - here is the commit that converts uc_payment to use entities, maintaining this relic of running the complete sale logic.

Issue #2681831 by longwave: Convert uc_payment to use entities

I also noticed that the D8 uc_paypal module no longer directly runs the D8 equivalent of uc_cart_checkout_complete_sale() as referenced in #20.

Instead it now only calls uc_payment_enter().

This is the commit that removes that call. I'm not sure if it was intentional, or an oversight in the shuffle of refactoring things.
https://git.drupalcode.org/project/ubercart/-/commit/d48c8bbf382bb397842...

If it was intentional, it might be because currently uc_payment_enter() would be running that logic ... but of course in this thread we're discussing that maybe uc_payment_enter should NOT do that. In that case that call probably should NOT have been removed from uc_paypal.