Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Version: 7.x-1.3 » 7.x-1.x-dev
Status: Active » Needs review
FileSize
1.05 KB

Yep, honestly I have no clue what this code is doing in here. It must have been accidentally committed from an in progress patch along with other payment module changes. Unfortunate, but I suppose the best we could do is just delete the code. I'll just need someone to test what happens in the UI if a Rule has this action on it and then the action disappears in code (patch attached).

druroot’s picture

I think this is a worthwile feature to have, it's something I need for a site that I'm working on that does recurring billing with a free trial. Here's a patch that implements this rule by reading the base property of the payment_method and looking for a _capture callback implemented by the payment module. I also have a sample implementation patch for the authnet module that I'll post in another comment.

druroot’s picture

Here's a patch to the authnet module that implements the _capture callback. Most of the code for capturing from a prior authorization was taken from the form submission handler for commerce_authnet_aim_capture_form. To reduce code duplication, I've collapsed the code from that submission handler to utilize this new callback.

This serves as an example implementation accompanying the code in #2, and if that code is accepted, then I can post this patch into a new issue in the commerce_authnet queue.

Status: Needs review » Needs work

The last submitted patch, commerce_authnet_capture_callback-1820356-3.patch, failed testing.

tmsimont’s picture

tmsimont’s picture

Component: Rules integration » Payment

OK so I'm confused now... I can't tell if this is a duplicate of my other issue or not.

In any case, the rule is a required step for Paypal WPP to work. It looks like druroot's first patch might have fixed the problem, but I'm not sure if that should be in Commerce core of if it should be added to the WPP module

tmsimont’s picture

Status: Needs work » Needs review
FileSize
3.09 KB

OK this is a 2 part problem with WPP and this module. I think that the rule action that used to be here without code was a necessary rule for the commerce payment process.

The rule wouldn't do anything on its own, but it allows other modules to define a [module_name]_capture callback to "hook" into this rule action.

If we put this rule back, it will provide a nice and clean method for other payment modules to use an authorize, then capture approach. I think this belongs here in commerce and not in other modules because this rule would be necessary for any payment module that would want to utilize such an approach.

I've got a patch for WPP, too, that I've tested along with the attached patch here.

tmsimont’s picture

I've attached the referenced WPP patch here #1851340: WPP: Payment to paypal sent before Review is submitted

Status: Needs review » Needs work
rszrama’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
tmsimont’s picture

that's odd -- i don't see how any fatal errors could come of this.. i will have to try to run it on a clean install i guess

rszrama’s picture

Yeah, me neither. I think something might've been screwy in test bot.

rszrama’s picture

Status: Needs work » Needs review
tmsimont’s picture

yay

tmsimont’s picture

Last patch had trailing whitespace.. this one should do

dwkitchen’s picture

Status: Needs review » Needs work

I think - maybe from the originally stray code - this has gone down slightly the wrong route.

The permitter being passed to the action is the order, but it should be the authorise transaction as this is what is being captured.

xenophyle’s picture

Status: Needs work » Needs review

I tested the patch from #16 and it worked (of course I had to add a hook_capture function to my payment module). The transaction didn't seem to have to come in as a parameter since it was looked up using the order.

tmsimont’s picture

I've kind of given up on the need for this in light of rszrama's comments in this other issue: #1851340: WPP: Payment to paypal sent before Review is submitted

If anyone else wants to pick up from where I've left it and get the rule to work that's cool but I no longer need this so I probably won't be able to make time for it.

timdiacon’s picture

I'm trying to implement a flow where once a product is sold out the authorised payments from WPP are then captured as outlined in this post - http://drupal.org/node/1964390

This patch in #2 looks like just what I'm after. I've applied the patch and then successfully used Rules to check when the stock level reaches 0 and then load all the related orders into Rules via VBO

I'm then looping through the orders and performing the "Capture from a prior authorization" that your patch provides on each order. Additionally I'm displaying a message with each order number to check everything is getting through correctly.

Everything is working except that the "Capture from a prior authorization" isn't performing the capture. Is this because the WPP module doesn't have the capture call back required or am I doing something else wrong?

edit - I've now also applied the patch for WPP in this post - http://drupal.org/node/1851340

When I test it is throwing an error that arguments 3 and 4 are missing from commerce_paypal_wpp_capture()

Any ideas why that might be? I'm so close to getting this working :-)

edit2

Ignore me I'm an idiot. I'm now using both the WPP and the Payment patches from tmsimont and everything is working perfectly

webavant’s picture

mglaman’s picture

Issue summary: View changes
Status: Needs review » Needs work

Does not apply and "capture" should be added to API docs for payment methods.

torgosPizza’s picture

I'm going to try to work on this, as it would be handy for #2600690: Allow for authorization only transactions in Commerce Stripe.

torgosPizza’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

Here's a re-roll against latest dev that handles the @todo in previous patch (logs in Watchdog if there was an error capturing a charge), as well as provides an example to commerce_payment.api.php.

rszrama’s picture

Status: Needs review » Fixed

I really regret that we never wrapped this one up, but as 2.x progresses and we move 1.x more toward maintenance mode, I don't think pushing this one to completion will be a good investment of limited time. At some point in time I actually did commit the patch from comment #1 (no clue why I didn't note that in here), so the empty code is at least not present any more.

I don't see anything wrong with the patch from torgosPizza in #24, but what makes me think it may not be worth a generic core solution is our inability to resolve edge cases here. For example, the current approach makes a generally safe assumption that the last payment transaction is the one that should be captured. I can't think of a scenario where you'd expect to capture an older authorization, but what might the presence of multiple valid transactions indicate? Should a merchant not also void older authorizations?

There's also a question of who's responsible for determining how much to charge. We got in this pickle elsewhere where we inconsistently used the order total amount vs. the order balance. This current action directs the gateway to charge the transaction amount, but will that always be the legitimate amount? e.g. an authorization could have pre-dated tax calculation / shipping application (in the case of PayPal EC w/ the shortcut flow), and we'd actually want to capture the full balance; or perhaps store credit applied and reduced the order balance post-authorization and we'd have to accommodate that.

I don't think any of these questions are that hard to resolve on their own, but I'd worry we'd create a lot of work for ourselves trying to include this in core vs. focusing those sorts of new developments on 2.x. My apologies to the folks who've invested time here over the years, but if you do have a solution using the patch in #24 with patches to other gateways, there's no reason not to leave those patches as part of your builds while you're still on 1.x.

I'm going to mark this issue as fixed based on the patch in comment #1 so everyone here gets credit. Had I been maintaining this better in the past I would've resolved this issue and opened a new feature request for "Add Rules support to capture the last payment authorization." C'est la vie.

Status: Fixed » Closed (fixed)

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