Behaviour upon order abortion or completion

If $_SESSION['uc_checkout'][$_SESSION['cart_order']]['do_complete'] is set to true, $_SESSION['uc_checkout'][$order->order_id]['do_review'] is unset, and you redirect to cart/checkout/complete this should handle completion of sale and appropriate redirect according to Ubercart configuration.

Also, and quite important, this would appropriately handle user login upon anonymous checkout and creation of a new user account... which is missing with the way you currently implement uc_cart_complete_sale(). Many stores rely on this as an alternative to the traditional way of signing up or logging in (helps keep more customers in the checkout process, who'd otherwise associate "signing up" with "getting spam" and run away).

payment_ubercart_finish() is a Payment callback to allow the context to resume the original workflow after the payment method is done processing the payment. I'm not sure what this has to do with hook_uc_order().

Well, currently payment_ubercart_finish() contains the redirect logic that deviates from expected behaviour. I mention hook_uc_order() because through the "submit" operation of this hook (which is normally called when the review form is submitted), one can cleanly (and without reinventing the wheel) abort the checkout process and send the customer back to the checkout page with a custom message... or send the customer through to complete the sale. It's a part of Ubercart's final checks to make sure the order can be submitted. On the first error received from one of the implementing modules it'll pop the customer back to cart/checkout/review with the cart and everything intact. Typically with a message along the lines of "Payment failed (...), please try again or go back and select an alternative payment method".

I believe the way you currently implement the "redirect" method of hook_uc_payment_method() is a bit unfortunate. First of all it is meant to be used for redirects to an external payment service (like one would do with PayPal). Secondly, when you call uc_cart_complete_sale() upon submit there... the ship has sailed in terms of cleanly aborting the checkout. It will among other things completely clear out their cart.

Since the hook_uc_order() is invoked with the "submit" op from the original submit handler of the checkout review page/form, I would look into removing the "redirect" method/callback all-together. Then instead save the entity and execute the payment from the hook_uc_order() "submit" op... before immediately checking status and returning something like this if the payment is considered "failed":

return array(array(
  'pass' => FALSE,
  'message' => t('We were unable to process your payment. Please try again, or go back and choose a different payment option.'),
));

And if the payment is considered "successful" or "pending", return something like this:

return array(array(
  'pass' => TRUE,
  'message' => '[A custom message returned by the executing payment method controller]',
));

On pass uc_payment would handle the do_complete session stuff for you before redirecting the customer to the checkout complete page, so you wouldn't even have to worry about that at all. Everything in payment_ubercart_finish() could in other words be stripped out.

I will test this (to make sure it works as intended in practise, not just theory) and try to follow up a little later with a patch.

Had various other things pop up, so the time I could spend on this tonight was a little limited. Here's a patch containing what I've got so far.

In a nutshell:

- Removes the "redirect" callback stuff.
- Removes the payment_ubercart_finish() code.
- Replaces the "cart-process" implementation with a custom form submit callback on the checkout form instead (which is added on top of any existing ones). This now handles the initial save upon the user submitting the form to continue checkout, without impacting normal form processing.
- Moved payment execution and status checking to hook_uc_order(). This includes appropriate abort of checkout upon payment error, and expected checkout completion behaviour (sale completion, cart empty, redirect, user creation, ...) if pending or successful.

To-do:
- Replace the "[A custom message returned by the executing payment method controller]" strings with (if provided) a status message supplied by the payment method, or (if not provided) an appropriate default.
- Test external payment methods, and figure out how to fix it if this no longer works as expected.

I gotta hit the sack now, but I'll continue ASAP to complete the patch. Most likely tomorrow evening (busy day at work). :)

Payment method controllers do not return messages (by design). All they do is set payment statuses.

I know; my intention is to add this. As I've mentioned previously, due to the diversity of payment options out there it makes sense for the selected payment method to be able to provide better feedback to the customer. Especially for the pending state.

No "we", no "please".

Why not? You are responding to a human being, one that is about to spend money (potentially a lot of it) in your store, so why would basic curtesy be an issue?

Most of what you put in hook_uc_order('submit') MUST go in the finish callback for the reasons I stated in #1 and #3

We'll see. This is a start. Currently the flow seems to function very well for standard payment methods, and things are executed in the order they should be. I've yet to attack external ones where the user is redirected off site for payment. If things need to be adapted for them to work, that is what I will do.

Thanks for the feedback.

The current design allows the context to specify what should be displayed based on the payment status. A payment method controller should work in the background, and apart from using forms to gather user data that is absolutely required for payment execution, it should not do anything else in the UI during the execution process. This also allows payments to be executed as programmatically as possible.

I suspect you misunderstood me a little. My intention is merely to allow the payment controller to specify a set of messages in the class, which can be accessed by the context to override its defaults. Existing payment methods would not need any immediate adoption, while the authors of both new or existing modules can choose to supply custom feedback if they deem it necessary.

An example where this would be useful is an invoice based method where the payment is set to a pending state up until the order is packed and shipped with an activated invoice enclosed. If you just supplied a generic message along the lines of "Your payment is still being processed." on the order confirmation page, this could really confuse a customer. "Did the order actually go through?" "What now?"

If the payment method in this case supplied a message (again, for the context to retrieve and output) along the lines of "Payment by invoice has been successfully authorised." the average customer would feel a lot more at ease.

I struggle to see a difference in principle between collecting necessary information prior to payment execution, and providing relevant and potentially necessary feedback afterwards. Nor do I see how it would interfere with "payments to be executed as programmatically as possible" as long as it is the context itself actually outputting the messages, not the payment method controller.

See the interface text guidelines.

I see... Well, in the context of e-commerce I have to disagree with the "no Please" part. It often warrants extra friendly and polite language. Ubercart certainly do not respect the guidelines in this regard. But fair enough. I have no problems adhering to it if you wish. :)

Try not to adapt anything.

I meant in payment_ubercart, and the changes I've made in the patch. Currently the way it's done there works really well (as far as I can see) with standard payment methods that do not redirect off site. If this is not the case when I get the chance to test external ones, I'm optimistically confident (heh :P) that I can find solutions to that within the confines of the context and hooks provided by Drupal/Payment/Ubercart.

The work you've done on Payment so far is great, and I think it's a really good concept. If we can just get this flow working a little more in line with how Ubercart is supposed to behave (and the same is achieved for Drupal Commerce, if it isn't already), I think it's just a question of time before it'll truly be an established standard for integrating payment methods with Drupal projects. I'm surprised no-one's done this sooner to be honest. It's silly to maintain all these platform-specific versions.

However, your approach sounds like a partial reinvention of the status system that we have right now.

It was more intended as an extension; allowing the context to fetch a better/more relevant description of the status when it's about to output a human readable string to the customer anyway.

Today you are doing stuff like this in the context:

if ((payment_status_is_or_has_ancestor($payment->getStatus()->status, PAYMENT_STATUS_SUCCESS))) {
  drupal_set_message(t('Your payment was successfully completed.'));
  (...)
}

Instead one could do something like this (just a rough idea):

if ((payment_status_is_or_has_ancestor($payment->getStatus()->status, PAYMENT_STATUS_SUCCESS))) {
  $message = $payment->method->getMessage(PAYMENT_STATUS_SUCCESS);
  drupal_set_message((($message) ? $message : t('Your payment was successfully completed.')));
  (...)
}

Where getMessage(), if implemented by the payment method controller, had logic like this:

switch ($status) {
  case PAYMENT_STATUS_SUCCESS:
    return t('Custom feedback message for a successful payment');
  (...)
}

This would not touch the current status system at all (internally all status checking and such would work as before), but allow statuses to be associated with more specific human readable output when necessary.

Linking them to payment method controllers causes other technical difficulties, as payments can have statuses, but no payment methods associated with them.

I don't see this as an issue with the way I outlined. As long as the link is a "soft" one, where the context uses the extra information if it is available (but otherwise move about it's normal business), it wouldn't matter that the Payment can have various states where a payment method isn't associated with it. The strings in question are by their nature only relevant in a checkout/payment flow where a payment metod is involved, at which point they would be available.

It was just a lose idea though, hence why I put it on the to-do.

I'm also fine with seeing if we can add a description property to PaymentStatusInfo that contains a longer human-readable explanation of what happened to the payment. This should not break BC.

That might not be a bad idea. So in a nutshell, a payment method would implement hook_payment_status_info() and supply it's own payment statuses for the cases where it needs to give the user custom feedback:

function example_invoice_payment_status_info() {
  return array(
    new PaymentStatusInfo(array(
      'description' => t('The invoice is authorised by InvoiceProvider, but not yet been activated.'),
      'status' => PAYMENT_STATUS_INVOICE_AUTHORISED ,
      'parent' => PAYMENT_STATUS_PENDING,
      'title' => t('Invoice authorised'),
      'status_message' => t('Payment by InvoiceProvider invoice has been successfully authorised.'),
    )),
  );
}

(I assume description is for internal use?)

Then in the context do something like...

if ((payment_status_is_or_has_ancestor($payment->getStatus()->status, PAYMENT_STATUS_SUCCESS))) {
  $payment_status = payment_status_info($payment->getStatus()->status);
  $message = ((isset($payment_status->status_message)) ? $payment_status->status_message : t('Your payment was successfully completed.'));
  drupal_set_message($message);
  (...)
}

Is that along the lines of what you are thinking?

FYI: I probably do not have time to continue work on this until next week. :)

Feel free to give feedback on the payment status stuff in #12 in the meantime.

Sure - will do as soon as I get the chance. Just a bit swamped work wise at the moment. :)

What are your thoughts on the part below the second quote in #12?

Resuming work on this now, FYI. Will post a new patch with the requested changes in a bit - just going to do a bit more testing and such first.

See attached patch. It's #4 with the to-do-placerholders replaced with the original messages, and the message for unsuccessful payment refined according to guidelines. Furthermore it includes a mechanism in the finish_callback implementation which allows payment_ubercart to return control back to ubercart in cases where an external gateway is used.

I've now tested this both with standard payment methods, as well paypal_payment (PPS) to cover an external gateway. Both seem to work great - including account creation and login for anonymous checkout if this is enabled.

Now, I've put quite a bit of time into this... so I would be very grateful for proper git attribution if you use the work/patch. :)

I noticed you omitted it for the other patch I submitted (but mentioned me in the comment), so here's some information about attribution if you are just not familiar with it yet: https://drupal.org/node/1146430

In a nutshell it's as easy as committing like this (if you add anything after the last quote, don't forget a space in-between):

git commit --author="git <git@81265.no-reply.drupal.org>"

<-- Got the flu, badly, so crawling back into bed.

Excessive code comments, and comments do not follow the coding standards.

Removed one comment, and added periods.

The payment does not need to be updated if the UIDs are the same already.

Is there actually any condition under which hook_uc_order() is called with the submit operation, and the payment already has the uid associated with it? If not, having a check for it would be a waste.

Should be written in the third person, and probably merged to one sentence.

Rewritten to 3rd person (as in "Checks" and "Returns"). The two sentences are the short and extended comment, in line with standards.

object is no valid type hint.

It certainly is, but you are right in that it should be Payment in this case since it's not a stdClass.

The entire docblock should read nothing more than "Implements..." as per the coding standards.

I should have put the Implements on the summary line (first line) of the comment, and omitted the param and return documentation (added by my IDE). However - there is nothing in the coding standard which states it can /only/ contain such a reference. You can document your particular implementation below the summary, which is sensible to do here.

This will not work, as executing the payment causes the finish callback to be invoked.

See the comment on finish() below for an elaboration.

What is the array structure?

I do not believe it is required to document the array structure, nor do I see why it would be sensible to do so in this case. I have however added a documentation line for the param and return.

Don't mention the reader. Instead, use "The payment".

Don't mention the reader. Instead, use "The payment".

Don't mention the reader. Instead, use "The payment".

Well, actually, the two first are copied directly from your own code... so I merely used that, and followed the same format for a more explanatory failure message. Nonetheless... I've made the change. ;)

Again, all this docblock should say is "Implements..."

This is not actually a hook implementation, but a callback implementation... which follows a slightly different standard. Admittedly the format I've used is that of a general function, thus wrong either way, but since you've not documented the callback format in the appropriate .api.php file... I do not have anything to reference correctly in such a way. The Implements (...) line is supposed to reference the dummy name used in the callback documentation, from my understanding of it.

And again; while the documentation states "should have as its one-line documentation summary "Implements (...)" (...)" - this does not mean that the docblock should only contain one line. "its one-line documentation summary" is merely a reference to the summary line, which should be the first line of the block and kept within a single lines length. You can still add additional documentation below it should you wish to.

We can assume that if the finish callback is invoked, there is an order ID, so this line can be removed. This will also make sure we see at least *some* output if there is no order ID.

The thing with Ubercart is that for what I call normal payments (as in.. where the payment method does not require redirection to an external gateway), payment execution should happen in the submit operation of hook_uc_order() and nowhere else to ensure expected behaviour/flow. This is what I'm trying to achieve with this experiment.

hook_uc_order() is also used for the external methods here, however the user is redirected on execute() so at this point control is lost until the user hits the return menu callback. Once the payment method has done what it needs to do to process the payment, it calls finish() which ultimately handles things back over to the context.

At this point we need to do either "nothing" if it was a normal payment... or in case of an external gateway (because code after execute() did not run) we still have to return control back to Ubercart to allow it to complete the sale and resume normal flow (displaying the order completion page). (*)

Hence, within Payment::finish_callback, in this case payment_ubercart_finish(), we need to be able to determine whether or not finish() was called as a step in a normal payment execution, where Ubercart would handle everything for us, or called "manually" via the return menu callback after returning from an external redirect.

When I compared the Payment object passed to Payment::finish_callback in the two scenarios, I noticed that payment_ubercart_uc_order_id was always missing at this point for external gateways. Hence, I used this to determine whether or not we should do the session stuff and redirect the user here.

I could not see any issues with the payment/order after the completion page was displayed; everything seemed to be saved correctly to the database, including the pid to uc_order_id mapping, in both scenarios.

Admittedly using payment_ubercart_uc_order_id to separate between the two is a potentially hacky solution, because I do not know whether this is intended behaviour or a bug. I've only observed that it seems to be consistent. I felt it was better to give it a try and see what you would make of it... because something like it is needed to make this approach work. The alternative could have been to introduce some other way for the same distinction to be made.

(*) A fundamental issue that I've run into with the hook_uc_order() based approach is that external gateways such as PayPal allows the user to deviate from the payment flow to for an example go to their account overview instead of back to the merchant. The gateway still sends the appropriate data back to the site, so that the Payment registers as it should. However... if the user is not sent back to the return path... uc_cart_complete_order() is never called, the order reimains as "in checkout" , their cart is not emptied, user creation does not happen, rules are not fired, etc.

This creates a huge headache in terms of doing anything "cleanly" and without reinventing the wheel / recreating a lot of Ubercarts checkout complete stuff. It might in fact put me back to square one... because in order to catch cases like the IPN processing without a returned user, uc_cart_complete_order() would have to be done either before control is handed off to the gateway (like you've done it today), or in hook_payment_status_change() when it moves to a pending or complete state (which is perhaps more appropriate). Either way both of them sadly rules out hook_uc_order(), because we can't call uc_cart_complete_sale() more than once.

I'm not likely to have more time to work on this for another week or so, unfortunately.

I just realized that the problem with this approach is that the checkout is never completed if the Payment::finish() is not called. This can occur when the user does not return after authorizing a payment off-site, for instance. This means we'll need to make sure the checkout is completed explicitly in hook_payment_status_change().

Indeed, I mentioned that earlier. My concern with that approach was calling uc_cart_complete_order() multiple times and triggering everything more than once, but upon a closer look it seems like it has a persistent safeguard for it already... so only the user login and cart empty can run more than once for an order. That is in other words not a problem. Not sure how I missed it in the first place.

I still need a few days before I can give you a hand, FYI. Glad to see you are making progress in the meantime. :)

The current approach ensures that user login and cart empty only occur when the finish callback is called

The current approach ensures that the user login doesn't occur unless the finish() callback is called, yes, however every time you call uc_cart_complete_sale() the cart will be emptied. As I said though, this is not an issue as far as I am concerned. I don't think it is likely that a user will return to the store while the payment is being processed, and start a new order.

Just be careful that uc_cart_complete_sale() is not under any circumstances called while the user can still end up on the finish callback with a failed payment, triggering the redirect back to checkout. The cart would be nuked at that point, so there would be nothing to return to. (They'd get the cart empty page.)

Sure - I'm in the process of moving homes, but I should be able to find some time over the next couple of days for it. :)

Great - and likewise; glad we could get this sorted together. Apologies for going a bit awol on you towards the end there - the whole moving houses thing got the better of me. Been a long haul, so to speak... :P