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

Braintree: return global_id in response #4884

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

rachelkirk
Copy link
Contributor

@rachelkirk rachelkirk commented Sep 12, 2023

CER-868

This field needs to be added to the transaction_hash method to be returned in the response. The Braintree gem required an update to make the payment_receipt object available

I did not include a unit test because I'm trying to get something returned in the response and according to the Braintree docs, it does not appear that anything should be passed in the request to trigger this coming back.

Remote
107 tests, 212 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
31.7757% passed

Unit
95 tests, 110 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
41.0526% passed

Local
5599 tests, 77770 assertions, 0 failures, 56 errors, 0 pendings, 0 omissions, 0 notifications
98.9998% passed

@rachelkirk rachelkirk requested review from yunnydang and a team September 12, 2023 14:35
@rachelkirk rachelkirk force-pushed the CER-868_braintree_global_id branch 2 times, most recently from f01746c to 6de1589 Compare September 22, 2023 20:50
Copy link
Contributor

@yunnydang yunnydang left a comment

Choose a reason for hiding this comment

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

All looks good! However i noticed there wasn't any unit tests? Maybe we can write a simple one similar to the risk_data object on line 248 in the unit spec file?

@rachelkirk
Copy link
Contributor Author

All looks good! However i noticed there wasn't any unit tests? Maybe we can write a simple one similar to the risk_data object on line 248 in the unit spec file?

The unit tests are testing what is going into the requests, since there's nothing about global_id in the request and it's only returned in the response I can't really write a unit test for it. Those risk data tests have things that are included in the request headers

@yunnydang
Copy link
Contributor

All looks good! However i noticed there wasn't any unit tests? Maybe we can write a simple one similar to the risk_data object on line 248 in the unit spec file?

The unit tests are testing what is going into the requests, since there's nothing about global_id in the request and it's only returned in the response I can't really write a unit test for it. Those risk data tests have things that are included in the request headers

All looks good! However i noticed there wasn't any unit tests? Maybe we can write a simple one similar to the risk_data object on line 248 in the unit spec file?

The unit tests are testing what is going into the requests, since there's nothing about global_id in the request and it's only returned in the response I can't really write a unit test for it. Those risk data tests have things that are included in the request headers

Nice! Thanks for clarifying I just peeked super quick at the unit tests, awesome work!

CER-868

This field needs to be added to the transaction_hash method to
be returned in the response. This also updates the Braintree gem, which was need to make these fields available

Remote
107 tests, 212 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
31.7757% passed

Unit
95 tests, 110 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
41.0526% passed

Local
5599 tests, 77770 assertions, 0 failures, 56 errors, 0 pendings, 0 omissions, 0 notifications
98.9998% passed
@rachelkirk rachelkirk merged commit 257075c into master Sep 26, 2023
5 checks passed
This pull request was closed.
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.

3 participants