StripePI: Skip add_network_token_cryptogram_and_eci method to accept ApplePay recurring payments #5212
Conversation
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
3 times, most recently
from
455ab09
to
fad0e58
Compare
Good catch, done! |
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
fad0e58
to
49d354c
Compare
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
49d354c
to
1ee70c2
Compare
| @@ -38,7 +38,7 @@ def create_intent(money, payment_method, options = {}) | |||
| return result if result.is_a?(ActiveMerchant::Billing::Response) | |||
| end | |||
|
|
|||
| add_network_token_cryptogram_and_eci(post, payment_method) | |||
| add_network_token_cryptogram_and_eci(post, payment_method, options) | |||
There was a problem hiding this comment.
Does Apple Pay and Google Pay also add these values in add_digital_wallet? It seems a bit confusing if the new AP route hits this method twice.
There was a problem hiding this comment.
Good catch! Fixed!
| def adding_network_token_card_data?(payment_method, options = {}) | ||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && | ||
| payment_method.source == :network_token && | ||
| payment_method.source != :apple_pay && | ||
| options.dig(:stored_credential, :initiator) != 'merchant' |
There was a problem hiding this comment.
| def adding_network_token_card_data?(payment_method, options = {}) | |
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && | |
| payment_method.source == :network_token && | |
| payment_method.source != :apple_pay && | |
| options.dig(:stored_credential, :initiator) != 'merchant' | |
| def add_network_token_card_data?(payment_method, options = {}) | |
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && | |
| payment_method.source == :network_token && | |
| options.dig(:stored_credential, :initiator) != 'merchant' |
There was a problem hiding this comment.
I don't think we need two source checks
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
1ee70c2
to
78f4a97
Compare
| def adding_network_token_card_data?(payment_method) | ||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && payment_method.source == :network_token | ||
| def adding_network_token_card_data?(payment_method, options = {}) | ||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && |
There was a problem hiding this comment.
Can we just leave this method as it was before? I rather remove it from add_network_token_cryptogram_and_eci. adding_network_token_card_data? is used in other methods and I wouldn't want us to break anything else.
| def add_network_token_cryptogram_and_eci(post, payment_method) | ||
| return unless adding_network_token_card_data?(payment_method) | ||
| def add_network_token_cryptogram_and_eci(post, payment_method, options) | ||
| return unless !options[:wallet_type] || adding_network_token_card_data?(payment_method, options) |
There was a problem hiding this comment.
I think it would be better to update this to be something like
return unless payment_method.is_a(NetworkTokenizationCreditCard)
return if options[:stored_credential] && options.dig(:stored_credential, :initiator) == 'merchant'
There was a problem hiding this comment.
We only pass options[:wallet_type] when the payment method is a non tokenized GooglePay and in AM the payment method model for this would be CreditCard
|
|
||
| post[:payment_method_options] ||= {} | ||
| post[:payment_method_options][:card] ||= {} | ||
| post[:payment_method_options][:card][:network_token] ||= {} | ||
| post[:payment_method_options][:card][:network_token][:cryptogram] = payment_method.payment_cryptogram if payment_method.payment_cryptogram | ||
| post[:payment_method_options][:card][:network_token][:electronic_commerce_indicator] = payment_method.eci if payment_method.eci | ||
| post[:payment_method_options][:card][:network_token] = { |
There was a problem hiding this comment.
Also if post[:payment_method_options][:card][:network_token] already exist we would have to use merge! because here we are resetting the object.
There was a problem hiding this comment.
So you could do something like
post[:payment_method_options][:card][:network_token].merge!({
cryptogram: payment_method.respond_to?(:payment_cryptogram) ? payment_method.payment_cryptogram : options[:cryptogram],
electronic_commerce_indicator: format_eci(payment_method, options)
}.compact)
78f4a97
to
9649f56
Compare
| @@ -317,7 +317,8 @@ def digital_wallet_payment_method?(payment_method) | |||
| end | |||
|
|
|||
| def adding_network_token_card_data?(payment_method) | |||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && payment_method.source == :network_token | |||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && | |||
| payment_method.source == :network_token | |||
There was a problem hiding this comment.
Do we need this check for source?
There was a problem hiding this comment.
I checked and it is not necessary
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
9649f56
to
fed72da
Compare
| def adding_network_token_card_data?(payment_method) | ||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && payment_method.source == :network_token | ||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) | ||
|
|
||
| false | ||
| end |
There was a problem hiding this comment.
What do you think about updating this method to have the check for options.dig(:stored_credential, :initiator) != 'merchant' as well so that the add_network_token_cryptogram_and_eci method can just rely on this one?
sinourain
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
fed72da
to
98afaef
Compare
Please make sure all of Alma's feedback is addressed!
| @@ -316,8 +316,8 @@ def digital_wallet_payment_method?(payment_method) | |||
| payment_method.source == :google_pay || payment_method.source == :apple_pay | |||
| end | |||
|
|
|||
| def adding_network_token_card_data?(payment_method) | |||
| return true if payment_method.is_a?(ActiveMerchant::Billing::NetworkTokenizationCreditCard) && payment_method.source == :network_token | |||
| def adding_network_token_card_data?(payment_method, options = {}) | |||
There was a problem hiding this comment.
I would prefer this stay as it is in master. adding_network_token_card_data? is used in 4 other places and I do not want to alter how that is working at the moment.
There was a problem hiding this comment.
Sure, no problem :)
| def add_network_token_cryptogram_and_eci(post, payment_method) | ||
| return unless adding_network_token_card_data?(payment_method) | ||
| def add_network_token_cryptogram_and_eci(post, payment_method, options) | ||
| return unless adding_network_token_card_data?(payment_method, options) |
There was a problem hiding this comment.
This should be updated to return unless payment_method.is_a?(NetworkTokenizationCreditCard) && options.dig(:stored_credential, :initiator) != 'merchant'
There was a problem hiding this comment.
Only AP/GP transactions on the new flow would reach here since if it was on the old flow the payment_method would be a token
There was a problem hiding this comment.
If we want to just be safe that it's only AP/GP new flow on this we could do something like
return unless payment_method.is_a?(NetworkTokenizationCreditCard) && options.dig(:stored_credential, :initiator) != 'merchant'
return if digital_wallet_payment_method?(payment_method) && options[:new_ap_gp_route] != true
98afaef
to
d005d06
Compare
sinourain
changed the title
almalee24
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
d005d06
to
23a3389
Compare
Summary: ------------------------------ StripePI: Skip add_network_token_cryptogram_and_eci method to accept ApplePay recurring payments and add unit/remote tests [SER-3618](https://spreedly.atlassian.net/browse/ECS-3618) Remote Test: ------------------------------ Loaded suite test/remote/gateways/remote_stripe_payment_intents_test Started Finished in 255.69074 seconds. 97 tests, 460 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 0.38 tests/s, 1.80 assertions/s Unit Tests: ------------------------------ Finished in 29.539052 seconds. 5993 tests, 80199 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 202.88 tests/s, 2715.02 assertions/s RuboCop: ------------------------------ 798 files inspected, no offenses detected
almalee24
force-pushed
the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
from
23a3389
to
3d28e30
Compare
almalee24
deleted the
ECS-3618_Stripe_PI_Stored_Credentials_for_ApplePay
branch
Summary:
StripePI: Skip add_network_token_cryptogram_and_eci method to accept ApplePay recurring payments and add unit/remote tests
SER-3618
Remote Test:
Loaded suite test/remote/gateways/remote_stripe_payment_intents_test
Started
Finished in 255.69074 seconds.
97 tests, 460 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
0.38 tests/s, 1.80 assertions/s
Unit Tests:
Finished in 29.539052 seconds.
5993 tests, 80199 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
202.88 tests/s, 2715.02 assertions/s
RuboCop:
798 files inspected, no offenses detected