-
Notifications
You must be signed in to change notification settings - Fork 0
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 logic for ApplePay #2
Conversation
This commit is not complete but it follows the same path as Google Pay. We need to see what token Apple returns in order to fill up the stub object we created for specs.
@kcpavan I put together this draft. once we get the token from Apple we need to update the stub. |
<payment_data><![CDATA[#{payment_method_token}]]></payment_data> | ||
#{'<test_card_number>4111111111111111</test_card_number>' if options[:test_mode]} | ||
</apple_pay> | ||
XML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably do something fancier here in which we use the value of the options[:payment_method] as the value for <apple_pay> or <google_pay>
Example:
doc << <<~XML
<#{options[:paymet_method]}
....
</#{options[:paymet_method]}
I had placed the apple_pay payment_method file in the incorrect location. And I was missing a apple pay token sample. This commit fixes both issues.
|
||
class ApplePay < PaymentMethod | ||
|
||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the extra space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I feel we need a plan (epic in JIRA) to make sure this gem keeps up with the original or pretty much assume that we have a risk that Spreedly decides to break their API or something and we deal with it in the future 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I feel we need a plan (epic in JIRA) to make sure this gem keeps up with the original or pretty much assume that we have a risk that Spreedly decides to break their API or something and we deal with it in the future 🤷
@joseluistorres I agree. We should at least issue a PR against the spreedly/spreedly-gem |
@joseluistorres @hoenth yeah the plan has always been to try to get this accepted on the main gem. although I accidentally opened two PR already on the main gem... so they know its coming :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
@@ -28,6 +28,8 @@ def self.new_from(xml_doc) | |||
return ThirdPartyToken.new(xml_doc) | |||
when 'google_pay' | |||
return GooglePay.new(xml_doc) | |||
when 'apple_pay' | |||
return ApplePay.new(xml_doc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an issue, just a Ruby newbie question about performance. When ordering a "case" statement, is any thought given to ordering the "when"s so that the most likely hits are at the top, and the least likely are at the bottom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably up to the individual engineer, and whether there is a real world difference between the options. So yes, in some cases, but there is nothing dogmatic that we follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never really thought about it, I guess is you have a very large case statement it could have an effect but in general i would say the gains would be minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, in this case the gain would be marginal since each option is a simple string.
Heads up, I posted this on their gem a few weeks ago, I'm merging this for now 🙏 |
Partially for https://networkforgood.atlassian.net/browse/AIO-1422
In order for us to be able to use apple pay through spreedly, we had to fork the gem and update it just like we did for google pay. this is what we are doing on this PR.
making it apple pay good.
The main magic happens on lib/spreedly/environment.rb in which we check for the payment_type and build the XML as needed by spreedly.