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

Versapay: Store Unstore #5315

Draft
wants to merge 1 commit into
base: SER-1334-Versapay_Void_Refund_Credit
Choose a base branch
from

Conversation

gasb150
Copy link
Collaborator

@gasb150 gasb150 commented Oct 22, 2024

This include the Store, Unstore methods,
and transactions related with third_party_token

Unit Tests

Finished in 0.070294 seconds.
27 tests, 159 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

Remote Tests

Finished in 72.628972 seconds.
25 tests, 125 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

Rubocop

804 files inspected, no offenses detected

This include the Store, Unstore methods,
and transactions related with third_party_token

Unit Tests
----------------
Finished in 0.070294 seconds.
27 tests, 159 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote Tests
----------------
Finished in 72.628972 seconds.
25 tests, 125 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Rubocop
----------------
804 files inspected, no offenses detected
@gasb150 gasb150 marked this pull request as draft October 22, 2024 22:05
Copy link
Collaborator

@Heavyblade Heavyblade left a comment

Choose a reason for hiding this comment

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

Looks pretty good

@@ -141,6 +155,9 @@ def add_payment_method(post, payment_method, options)
cvv: payment_method.verification_value
}
add_address(post[:credit_card], options, 'billing', 'payment_method')
elsif payment_method.is_a?(String)
_, _, fund_token = payment_method.split('|')
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 Comment/Style:

You can achieve basically the same result, by:

post[:fund_token] = payment_method.split('|').last

end

def unstore(authorization, options = {})
_, wallet_token, fund_token = authorization.split('|')
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 Comment / Style:

Same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I'm getting two values, not only the last one.

def success_from(response)
response['success'] || false
def success_from(response, action)
case action
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 Comment:

Is going o work anyway but is not consistent, see action can be 'purchase,capture,void' but you are mixing those actions with the endpoint 'wallets'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cause the actions are the same as the endpoint, I'm not mapping it, they are just the same name.


first_transaction = response['transactions']&.first
gateway_response_errors = gateway_errors_message(response)

response_message = {
errors: response['errors']&.join(', ').presence,
errors: response.dig('errors')&.join(', ').presence,
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 Comment:

Whats the point of dig vs [ ] ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was because of a different kind of error, but it was solved differently, it can be removed.

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.

2 participants