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

Add payment profiles support #10

Merged
merged 14 commits into from Dec 1, 2015
Merged

Conversation

shioyama
Copy link
Contributor

This depends on activemerchant/active_merchant#1941, so we can't actually merge it until that PR is merged.

Chris Salzberg added 8 commits November 25, 2015 18:03
Other sources don't need to store these on Komoju for now, so just don't
do anything for them.
This seems to be expected.
We're not using these but they are expected on credit cards, so better
to have them.
This is important because in activemerchant we check if the payment is a
CreditCard.
Until activemerchant/active_merchant#1941 is merged, we'll need to just
include this here.
@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

I've included the komoju gateway from activemerchant here, which overrides anything in that gem and thus enables us to use payment profiles. Once AM merges the change we can remove this and just reference the latest version of the gem.

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

@camelmasa can you check this? I think you're in the best position to do that.

@camelmasa camelmasa self-assigned this Dec 1, 2015
@camelmasa
Copy link
Contributor

@shioyama sure, I'll do that after lunch 🍖

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

@camelmasa can I merge this?

@camelmasa
Copy link
Contributor

@shioyama I was displayed following after confirm page . Please check it 💃

action_controller__exception_caught

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

Thanks! I'll check that now.

@camelmasa
Copy link
Contributor

@shioyama But, I tried that twice. I succeed that :(

order_r356911097_-_spree_demo_site

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

Hmm I don't see that issue:

latest-screenshot

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

Oh I see: https://github.com/camelmasa/spree_komoju/blob/8b5c2bb6aa2b83319cdca507e738dae6101b25cf/app/overrides/add_instruction_to_order_show.rb#L6

This won't work for other payment methods that don't have that method.

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

@camelmasa I added the missing credit card spec, have a look.

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

@camelmasa Ah wait I'll add one for create_profile as well.

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

Ok everything is spec'ed now, have a look 😄

@camelmasa
Copy link
Contributor

@shioyama Tnank you for implementation. Please rename Spree::BankTransfer#instruction_partial_path too 🙏

@shioyama
Copy link
Contributor Author

shioyama commented Dec 1, 2015

@camelmasa done!

@camelmasa
Copy link
Contributor

@shioyama LGTM 💯

camelmasa added a commit that referenced this pull request Dec 1, 2015
@camelmasa camelmasa merged commit f00ffe3 into degica:master Dec 1, 2015
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.

None yet

2 participants