-
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
CheckoutV2: Add Amount Allocations object #5186
base: master
Are you sure you want to change the base?
CheckoutV2: Add Amount Allocations object #5186
Conversation
Add Amount Allocations object for Sub-Entity payments as Marketplace object is deprecated. LOCAL: rake test:local TEST=test/unit/gateways/checkout_v2_test.rb Finished in 0.020203 seconds. ----------------------------------------------------------------------------------------------------------------------------------- 68 tests, 431 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed ----------------------------------------------------------------------------------------------------------------------------------- 3365.84 tests/s, 21333.47 assertions/s REMOTE: rake test:remote TEST=test/remote/gateways/remote_checkout_v2_test.rb - Update fixtures (Checkout Keys), three (3) processing_channel_id and entity_id. Finished in 169.233266 seconds. ----------------------------------------------------------------------------------------------------------------------------------- 113 tests, 267 assertions, 15 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 86.7257% passed ----------------------------------------------------------------------------------------------------------------------------------- 0.67 tests/s, 1.58 assertions/s Test Failed: - test_expired_card_returns_error_code - test_failed_purchase - test_invalid_shipping_address - test_money_transfer_payout_handles_blank_destination_address - test_money_transfer_payout_reverts_to_credit_if_payout_sent_as_nil - test_successful_authorize_includes_avs_result - test_successful_credit - test_successful_money_transfer_payout_via_credit_corporate_account_holder_type - test_successful_money_transfer_payout_via_credit_individual_account_holder_type - test_successful_purchase_includes_avs_result - test_successful_purchase_with_ip - test_successful_purchase_with_minimal_options - test_successful_purchase_with_processing_data - test_successful_purchase_with_shipping_address - test_successful_purchase_without_phone_number
8dc512f
to
ba43711
Compare
To provide a cleaner slate for the maintenance of the library, this PR/Issue is being labeled stale after 60 days without activity. It will be closed in 14 days unless you comment with an update regarding its applicability to the current build. Thank you! |
Not stale. |
Hi @jduff, sorry if I'm breaking contributor protocol by tagging you directly, but I couldn't see any guidelines for this kind of etiquette in the wiki, and the Google group linked off the project page (https://activemerchant.org (Discussion)-> https://groups.google.com/g/activemerchant) looks like it hasn't been used in 6 years. Is it possible we can ping someone to review this? Over here at Pin Payments, we need the |
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.
@damienjbyrne unfortunately I don't have commit access to this project anymore so I can't really help with getting it merged. I did leave a code review in case that helps (keep in mind I haven't worked on or used this project in ~10 years).
transaction_indicator: 2, | ||
previous_charge_id: 'pay_123', | ||
processing_channel_id: 'pc_123', | ||
amount_allocations: [ |
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.
You might want tests for when these are missing the id
or amount
. I'm assuming there are tests already where this isn't included.
@@ -1130,21 +1138,21 @@ def test_failed_void_via_oauth | |||
end | |||
|
|||
def test_successful_verify | |||
response = @gateway.verify(@credit_card, @options) | |||
response = @gateway.verify(@credit_card, @options_for_verify) |
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.
I would leave some tests where the allocations options are not included.
@@ -503,6 +504,14 @@ def add_marketplace_data(post, options) | |||
end | |||
end | |||
|
|||
def add_amount_allocations_data(post, options) | |||
return unless options[:amount_allocations]&.is_a?(Array) |
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.
nit: it doesn't hurt but you don't really need the null safe here, nil.is_a?(Array)
should work just fine.
Description
Add Amount Allocations object for Sub-Entity payments as Marketplace object is deprecated.
Referrence
Test on Master