Cecabank: Add 3DS Global to Cecabank REST JSON gateway #4940
Conversation
There was a problem hiding this comment.
@sinourain done with the review, left some comments and a big warning for your consideration.
| @@ -140,8 +140,23 @@ def add_stored_credentials(post, creditcard, options) | |||
|
|
|||
| def add_three_d_secure(post, options) | |||
| params = post[:parametros] ||= {} | |||
|
|
|||
| params[:ThreeDsResponse] = options[:three_d_secure] if options[:three_d_secure] | |||
| three_d_secure = options[:three_d_secure] | |||
There was a problem hiding this comment.
💬 Style/comment:
Maybe the shorter version?
return unless three_d_secure = options[:three_d_secure]| three_d_secure = options[:three_d_secure] | ||
| return unless three_d_secure.present? | ||
|
|
||
| three_d_response = {} |
There was a problem hiding this comment.
💬 Style/Comment:
maybe a more declarative approach
three_d_response = {
exemption_type: three_d_secure[:exemption_type],
three_ds_version: three_d_secure[:version],
authentication_value: three_d_secure[:cavv],
directory_server_transaction_id: three_d_secure[:ds_transaction_id],
acs_transaction_id: three_d_secure[:acs_transaction_id],
authentication_response_status: three_d_secure[:authentication_response_status],
three_ds_server_trans_id: three_d_secure[:three_ds_server_trans_id],
ecommerce_indicator: three_d_secure[:eci]
}| three_d_response[:three_ds_server_trans_id] = three_d_secure[:three_ds_server_trans_id] | ||
| three_d_response[:ecommerce_indicator] = three_d_secure[:eci] | ||
|
|
||
| extra_params = options[:extra_options_for_three_d_secure].with_indifferent_access |
There was a problem hiding this comment.
🔴 ❓ Question 🔴
So the idea is that Cecabank is going to need a ton of values from the 3DS Response and you want to add a GSF to allow the merchant to provide the extra data, right?
If that is the idea, is not going to work, on 3DS Global the merchant doesn't have access to the actual 3DS response, the response is saved on CORE DB after the customer has finished the 3DS.
We need to understand what are the required fields on that ThreeDsResponse.
There was a problem hiding this comment.
I am going to test which fields are necessary, which fields we can obtain from the options, and which fields we can ask if they can be excluded in the Cecabank API
There was a problem hiding this comment.
I removed extra_options_for_three_d_secure because the only field that we need is amount and this field we have in the options
| three_d_response[:three_ds_server_trans_id] = three_d_secure[:three_ds_server_trans_id] | ||
| three_d_response[:ecommerce_indicator] = three_d_secure[:eci] | ||
|
|
||
| extra_params = options[:extra_options_for_three_d_secure].with_indifferent_access |
There was a problem hiding this comment.
💬 Comment:
Not super bad, but with_indifferent_access doesn't have any impact here.
| extra_params = options[:extra_options_for_three_d_secure].with_indifferent_access | ||
| three_d_response.merge!(extra_params) | ||
|
|
||
| params[:ThreeDsResponse] = three_d_response.to_json.to_s |
There was a problem hiding this comment.
💬 Comment:
Again, not super bad, but to_json returns an string
| return unless three_d_secure.present? | ||
|
|
||
| three_d_response = {} | ||
| three_d_response[:exemption_type] = three_d_secure[:exemption_type] |
There was a problem hiding this comment.
exemption_type is a field on the request to core but is not part of the values that core passes down to AM, check this params
| three_d_response[:acs_transaction_id] = three_d_secure[:acs_transaction_id] | ||
| three_d_response[:authentication_response_status] = three_d_secure[:authentication_response_status] | ||
| three_d_response[:three_ds_server_trans_id] = three_d_secure[:three_ds_server_trans_id] | ||
| three_d_response[:ecommerce_indicator] = three_d_secure[:eci] |
There was a problem hiding this comment.
💬 Comment:
You can add the enrolled attributed from three_d_secure[:enrolled]
| } | ||
| end | ||
|
|
||
| def extra_options_for_three_d_secure |
There was a problem hiding this comment.
💬 Comment
Two things here:
-
there are some values that are available as part of the request like
email|amount|ip| etc. -
As a consequence of the comment related to available params this value is not going to represent the actual result of the 3DS authentication by Seglan.
sinourain
force-pushed
the
Cecabank_rest_json_3DS_global_implementation
branch
4 times, most recently
from
a9a3608
to
6d7a9e3
Compare
naashton
force-pushed
the
Cecabank_rest_json_implementation
branch
from
fe96fdb
to
3cdb744
Compare
sinourain
changed the base branch from
Cecabank_rest_json_implementation
to
master
sinourain
force-pushed
the
Cecabank_rest_json_3DS_global_implementation
branch
from
6d7a9e3
to
e19b23e
Compare
sinourain
force-pushed
the
Cecabank_rest_json_3DS_global_implementation
branch
from
e19b23e
to
c8ba0d5
Compare
Description ------------------------- Include 3DS2 Global For Spreedly reference: SER-817 Unit test ------------------------- Finished in 21.808339 seconds. 5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 259.53 tests/s, 3590.42 assertions/s Rubocop ------------------------- Inspecting 778 files 778 files inspected, no offenses detected
naashton
force-pushed
the
Cecabank_rest_json_3DS_global_implementation
branch
from
c8ba0d5
to
93d0cd6
Compare
Description ------------------------- Include 3DS2 Global For Spreedly reference: SER-817 Unit test ------------------------- Finished in 21.808339 seconds. 5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 259.53 tests/s, 3590.42 assertions/s Rubocop ------------------------- Inspecting 778 files 778 files inspected, no offenses detected Co-authored-by: Luis Urrea <[email protected]>
Description ------------------------- Include 3DS2 Global For Spreedly reference: SER-817 Unit test ------------------------- Finished in 21.808339 seconds. 5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 259.53 tests/s, 3590.42 assertions/s Rubocop ------------------------- Inspecting 778 files 778 files inspected, no offenses detected Co-authored-by: Luis Urrea <[email protected]>
Description ------------------------- Include 3DS2 Global For Spreedly reference: SER-817 Unit test ------------------------- Finished in 21.808339 seconds. 5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 259.53 tests/s, 3590.42 assertions/s Rubocop ------------------------- Inspecting 778 files 778 files inspected, no offenses detected Co-authored-by: Luis Urrea <[email protected]>
Description ------------------------- Include 3DS2 Global For Spreedly reference: SER-817 Unit test ------------------------- Finished in 21.808339 seconds. 5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 259.53 tests/s, 3590.42 assertions/s Rubocop ------------------------- Inspecting 778 files 778 files inspected, no offenses detected Co-authored-by: Luis Urrea <[email protected]>
Description
Include 3DS2 Global
For Spreedly reference:
SER-817
Unit test
Finished in 21.808339 seconds.
5660 tests, 78301 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
Remote test
259.53 tests/s, 3590.42 assertions/s
Rubocop
Inspecting 778 files
778 files inspected, no offenses detected