Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2 3 stable fix #12

Open
wants to merge 12 commits into
base: 2-3-stable
Choose a base branch
from
Open

2 3 stable fix #12

wants to merge 12 commits into from

Conversation

paulo-surfdome
Copy link

No description provided.

@simmsy
Copy link

simmsy commented Aug 18, 2014

@huoxito Hey Washington. Here is the updated version we have had to hack to get everything working more inline with how Spree expects payments to be handled. If you have take a look at what we have done then we should have a chat to discuss. Basically, I've take a much different approach on the HPP payments. Rather than redirecting the user from the payment view, the view now sends an alternative_payment_source source back with the select hpp payment method. We attempt to process payments in the payment state and therefore this enables a payment record to be created in the DB in the processing state. We then raise an exception when attempting to authorise using the Gateway, which is rescued and then the user is redirected. This not only falls much more inline with the Spree standard flow, it also keeps a record of the payments. Once the user is returned from the HPP the payment record status is simply updated and checkout processed s normal.

@huoxito
Copy link
Contributor

huoxito commented Aug 19, 2014

hey @joe,

so I just run your approach here for processing via encrypted gateway and works pretty great, (ps. didn't need to add any decorator considering theres no confirm step because spree won't actually load the payments from database I believe order.unprocessed_payments will give us the in memory payment so source is still available in that request). That said the only reason I'm hesitating in pushing that approach to master here is that it won't work for stores with a checkout confirm step while the current approach would still work on that case.

I'm thinking about calling payment.process! inside AdyenCommon#create_profile_on_card, we would still be processing payments the standard spree way and create the profile right after that and wouldn't risk losing all cc info along the way if spree for some reason reload payment later on the stack also no decorators needed, just not sure if that would work right now but will give a try.

couldn't review the hpp payment stuff yet, see e95e8f8929cd^...3b19427 and wvanbergen/adyen#86 for some progress on this

@huoxito
Copy link
Contributor

huoxito commented Aug 20, 2014

hey guys,

I've pushed the changes included related to grabbing the card last digits on authorise response to both master and 2-3-stable please see 06685dc...b9d4c44 (note it points to a new branch of my adyen fork)

I took another look at the Payment::Processing api and I couldn't find a way around to use that here when setting up the profile. So far I still feel that the current approach is easier to debug and support because we clearly see all calls in the same method (we authorise, get the last digits, get the recurring token and mark the payment as processing). Splitting the profile set up across the normal spree flow, Order#process_payments!, gives the false idea that those actions are independent when they really are not. Totally agree with you that gateways usually should not change payment state like that but for this scenario I think the trade off is worth it.

About the HPP stuff sorry but I'm not sure I understand the motivation to make it go through the usual spree payments flow. I see the appeal here for also letting things to be triggered only on payment.process but in the end it seems to me that it ends up doing the same manual state change here, e.g. payment.pend!? Overall I can imagine it might be helpful for some reason having a record that a customer choose HPP and was redirected to Adyen but except for that is that really worth it the trouble it can cause? This approach takes more code and logic to look after while the current implementation simply create a payment from scratch based on the adyen redirect response. What you think?

Keep in mind that the current 3D implementation takes a very similar approach, if 3D card is detected on checkout the payment is not created at all at that point. User will be redirected to Adyen servers and once back to your store a payment will be created from scratch. (now that I put more thought about it I'm all for adding some more log calls across this process for the sake of keeping record of whats going on, here for example before raising payment.log_entries.create!(:details => response.to_yaml).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants