-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Flex Charge: Add support for TPV store #5120
Flex Charge: Add support for TPV store #5120
Conversation
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.
@edgarv09 It's pretty good. I left a few comments to your consideration.
@@ -115,23 +135,29 @@ def add_invoice(post, money, credit_card, options) | |||
avsResultCode: options[:avs_result_code], | |||
cvvResultCode: options[:cvv_result_code], | |||
cavvResultCode: options[:cavv_result_code], | |||
cardNotPresent: credit_card.verification_value.blank? | |||
cardNotPresent: credit_card.is_a?(String) ? false : credit_card.verification_value.blank? |
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.
Just confirming: for TPV the cardNotPresent value will always be false.
But for credit cards, will it be something true if no verification value is provided?
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.
Flex charge Documentation is not clear on this topic but the error messages during the testing led me to this conclusion.
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.
Sounds reasonable 👍
7c4f059
to
4bf9913
Compare
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.
@edgarv09 looks pretty much ok, left a couple comments for your consideration
}.compact | ||
} | ||
|
||
post = { payment_method: payment_method } |
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.
💬 Comment / Style:
Is just matter of taste but, for your consideration =>
post[:payment_method] = {
credit_card: {
first_name: first_name,
last_name: last_name,
month: credit_card.month,
year: credit_card.year,
number: credit_card.number,
verification_value: credit_card.verification_value
}.compact
}
@@ -115,23 +135,29 @@ def add_invoice(post, money, credit_card, options) | |||
avsResultCode: options[:avs_result_code], | |||
cvvResultCode: options[:cvv_result_code], | |||
cavvResultCode: options[:cavv_result_code], | |||
cardNotPresent: credit_card.verification_value.blank? | |||
cardNotPresent: credit_card.is_a?(String) ? false : credit_card.verification_value.blank? |
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.
Sounds reasonable 👍
}.compact | ||
payment_method = case credit_card | ||
when String | ||
{ token: true, cardNumber: credit_card } |
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.
❓ Question:
Is weird because I assume that it works for you but, does it work with token
and Token
?
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.
It seems to work with lower/upper case.
def authorization_from(response) | ||
response[:orderSessionKey] | ||
def authorization_from(action, response) | ||
action == :store ? response[:transaction][:payment_method][:token] : response[:orderSessionKey] |
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.
💬 Comment:
Just in case of unexpected scenarios dig
is a safer way to get the token
Test summary: Local: 5898 tests, 79569 assertions, 0 failures, 17 errors, 0 pendings, 0 omissions, 0 notifications 99.7118% passed Unit: 12 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 13 tests, 34 assertions, 1 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications 91.6667% passed
4bf9913
to
5232f00
Compare
CHANGELOG
Outdated
@@ -2,6 +2,7 @@ | |||
= ActiveMerchant CHANGELOG | |||
|
|||
== HEAD | |||
* FlexCharge: Add support for TPV store [edgarv09] #5120 |
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.
💬 Comment,
This should be a the bottom of the block
* Flex Charge: Add support for TPV store Test summary: Local: 5898 tests, 79569 assertions, 0 failures, 17 errors, 0 pendings, 0 omissions, 0 notifications 99.7118% passed Unit: 12 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 13 tests, 34 assertions, 1 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications 91.6667% passed
Flex Charge: Add support for the TPV store
Test summary:
Local:
5898 tests, 79569 assertions, 0 failures, 17 errors, 0 pendings, 0 omissions, 0 notifications 99.7118% passed
Unit:
12 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote:
13 tests, 34 assertions, 1 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications 91.6667% passed