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

Fix validation for tokenized ECv1 #29

Merged
merged 13 commits into from
Feb 12, 2024
Merged

Conversation

brorbw
Copy link
Contributor

@brorbw brorbw commented Jan 18, 2024

Start supporting validation of tokenized ECv1 tokens. Previously they have not been supported.

@brorbw brorbw requested a review from a team January 18, 2024 21:28
@Lassejoe
Copy link

Lassejoe commented Jan 26, 2024

I have gone through the PR and compared it with https://developers.google.com/pay/api/processors/guides/test-and-validation/sample-tokens#ecv2_4
I have not been able to spot any mistakes.
Also this is my first time working with dry I also utilized https://rubydoc.info/gems/dry-validation for reviewing the code.

@mt-clearhaus mt-clearhaus requested a review from a team January 26, 2024 12:25
lib/aliquot/payment.rb Outdated Show resolved Hide resolved
lib/aliquot/validator.rb Outdated Show resolved Hide resolved
lib/aliquot/validator.rb Outdated Show resolved Hide resolved
@thejspr
Copy link

thejspr commented Jan 29, 2024

A few small suggestions from me, otherwise it looks solid.

thejspr
thejspr previously approved these changes Feb 2, 2024
Gemfile Outdated
@@ -1,3 +1,5 @@
source 'https://rubygems.org'

gem 'aliquot-pay', :git => 'https://github.com/clearhaus/aliquot-pay', :branch => 'make-tokens-consistent-with-googles-sample-tokens'
Copy link
Contributor

@cbobach cbobach Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed before merging, right? And re introduced in aliquot.gemspec with version requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed it should but with the mutual dependency the automated tests will fail without.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just to highlight that this was still hanging around even though you got a ✔️

Copy link
Contributor

@mt-clearhaus mt-clearhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few things I stumbled upon. Only syntax stuff.

Comment on lines +69 to +71
@message['paymentMethodDetails'].merge!(
'threedsCryptogram' => @message['paymentMethodDetails']
.delete('3dsCryptogram')) if @message['paymentMethodDetails']['3dsCryptogram']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is IMO not easily readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@message['paymentMethodDetails'].merge!(
'threedsCryptogram' => @message['paymentMethodDetails']
.delete('3dsCryptogram')) if @message['paymentMethodDetails']['3dsCryptogram']
details = @message['paymentMethodDetails']
if details['3dsCryptogram']
details['threedsCryptogram'] = details.delete('3dsCryptogram')
end

Copy link
Contributor

@mt-clearhaus mt-clearhaus Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@message['paymentMethodDetails'].merge!(
'threedsCryptogram' => @message['paymentMethodDetails']
.delete('3dsCryptogram')) if @message['paymentMethodDetails']['3dsCryptogram']
d = @message['paymentMethodDetails']
d['threedsCryptogram'] = d.delete('3dsCryptogram') if d['3dsCryptogram']

rule(:paymentMethod) do
key.failure('must be equal to CARD') unless 'CARD'.eql?(value)
if values[:paymentMethodDetails].is_a?(Hash)
if '3DS'.eql?(values[:paymentMethodDetails]['authMethod']) # Tokenized ECv1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if '3DS'.eql?(values[:paymentMethodDetails]['authMethod']) # Tokenized ECv1
if values[:paymentMethodDetails]['authMethod'] == '3DS' # Tokenized ECv1

Comment on lines +330 to +342
if version == 'ECv1'
if tokenized
Aliquot::Validator::ECv1_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv1_PaymentMethodDetailsContract.new
end
else
if tokenized
Aliquot::Validator::ECv2_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv2_PaymentMethodDetailsContract.new
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if version == 'ECv1'
if tokenized
Aliquot::Validator::ECv1_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv1_PaymentMethodDetailsContract.new
end
else
if tokenized
Aliquot::Validator::ECv2_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv2_PaymentMethodDetailsContract.new
end
end
if version == 'ECv1'
if tokenized
Aliquot::Validator::ECv1_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv1_PaymentMethodDetailsContract.new
end
else
if tokenized
Aliquot::Validator::ECv2_TokenizedPaymentMethodDetailsContract.new
else
Aliquot::Validator::ECv2_PaymentMethodDetailsContract.new
end
end

@brorbw brorbw merged commit 5ef568e into master Feb 12, 2024
0 of 4 checks passed
@brorbw brorbw deleted the support-ecv1-tokenized-tokens branch February 12, 2024 11:53
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.

5 participants