Paymill: Add currency and amount to store requests #3289

jasonxp · 2019-07-31T05:21:55Z

Paymill now requires a currency and amount to be specified for tokenization. These are used
for an authorization. We now set this authorization amount to $1 USD -- the minimum allowed -- in our store requests.

The currency and money options are translated to presentation.amount3D and presentation.currency3D in the request made to Paymill.

curiousepic · 2019-07-31T13:57:24Z

Can/should the amount be voided afterward? If so, we can use a MultiResponse to perform a void after the store.

molbrown · 2019-07-31T14:03:18Z

Was about to ask the same question ☝️

leila-alderman

Just a bunch of questions on things I don't understand! 😄

leila-alderman · 2019-07-31T15:02:22Z

test/unit/gateways/paymill_test.rb

+  def test_store_includes_currency
+    expected_currency = 'USD'
+
+    @gateway.expects(:raw_ssl_request).with(


Looking through the other unit tests in this file, none of them use .with. Why is it needed for this unit test?

The purpose of the test is to ensure that the currency gets included in the request sent to Paymill. So, the expectation is that when we call the method to make an SSL request, it's done with the currency parameter set to the right value.

Okay, that makes sense. Why was this check added as a unit test but not as a remote test?

Good question. When Paymill responds to a store request, it doesn't return anything different in the case of a currency and amount being sent vs. not being sent. So, there's nothing that we can check in a remote test. The options are being set in the code that implements the request, but a user of that code cannot see those details. The remote test is performing its testing as if it were a user of the code, only calling the public methods (e.g., gateway.store) and checking the return values. The more general theme here is unit testing vs. integration testing.

I remember that there was a similar issue when trying to test the new tx_source GSF for the MiGS gateway. Would replicating the approach I took for those remote make sense here?
4493b55#diff-a1a05cf6a8356f8c6539f34382a90bf6R110

That is indeed a similar case. I don't think a test like that is possible in this case, though, because the options being passed are hardcoded in the store implementation. There isn't a way for the caller (i.e. our remote test) to change them to something invalid. FWIW, I did manually test that.

leila-alderman · 2019-07-31T15:02:31Z

test/unit/gateways/paymill_test.rb

+    expected_currency = 'USD'
+
+    @gateway.expects(:raw_ssl_request).with(
+      :get,


What does this line do?

store_endpoint is just a method that returns a string. That string is the second expected argument in the raw_ssl_request call.

Oooh, I think I see now. So the .with()method requires all of the parameters that are passed to the given function, meaning that you needed to pass in everything here:

def raw_ssl_request(method, endpoint, data, headers = {})

https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/posts_data.rb#L47

That's where I had been confused. I didn't know what the four different parameters listed out here were doing.

leila-alderman · 2019-07-31T15:04:28Z

test/unit/gateways/paymill_test.rb

+      store_endpoint(@credit_card, expected_currency, 100),
+      nil,
+      {}
+    ).returns(successful_store_response, successful_purchase_response)


Since it's been determined that the store call no longer works without passing both currency and amount, what is the reason for creating two separate tests ensuring that these values are passed as opposed to just testing that the store calls are successful?

leila-alderman · 2019-07-31T15:06:13Z

test/unit/gateways/paymill_test.rb

+    @gateway.store(@credit_card)
+  end
+
+  def test_store_includes_amount


Why have these set up as two separate tests? It looks like both of these tests are passing in both currency and amount, so I'm not seeing the difference between these tests.

test/unit/gateways/paymill_test.rb

leila-alderman · 2019-07-31T15:10:16Z

lib/active_merchant/billing/gateways/paymill.rb

@@ -48,6 +48,11 @@ def void(authorization, options={})
      end

      def store(credit_card, options={})
+        # The store request requires a currency and amount of at least $1 USD.
+        # This is used for an authorization.


I don't understand why this comment is talking about authorization within the store method. Is the issue that a store transaction for PayMill now runs an authorization first before storing the card? Do all store methods work that way?

Yep, that is the issue. The store requests for Paymill started resulting in errors when Paymill added currency and amount as required fields for the store operation. I can tweak the comment to be more explicit in saying that the authorization is something Paymill does internally. I'm not aware of any other gateway stores that work this way.

jasonxp · 2019-07-31T15:13:19Z

@curiousepic and @molbrown My understanding is that the "store" authorization is something that Paymill does internally. It doesn't appear to be possible to void it from our end, since we don't receive an authorization ID back in response. This is what the store response looks like: https://github.com/activemerchant/active_merchant/blob/master/test/unit/gateways/paymill_test.rb#L229.

curiousepic · 2019-07-31T19:12:09Z

Oh duh, it likely doesn't actually capture, and they just let the auth timeout. 👍

molbrown · 2019-07-31T19:27:06Z

I do wonder why they ask for an amount though, if that is the case. You would think they could just do an internal $1 auth on any store request. I am skeptical that this could be for a purchase+store action, and I find their docs really challenging to navigate.

Do we have confirmation from Paymill/ other confidence that the store amount does what we think it does?

Paymill now requires a currency and amount to be specified for tokenization. These are used for an authorization. We now set this to $1 USD, which is the minimum required. Suite test/remote/gateways/remote_paymill_test 18 tests, 74 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed All unit tests 4188 tests, 70111 assertions, 0 failures, 0 errors, 0 pendings, 2 omissions, 0 notifications 100% passed Rename store_endpoint -> store_endpoint_url Combine tests for store currency and amount Tweak comment Update Changelog for Paymill store change

jasonxp requested review from a team July 31, 2019 05:22

leila-alderman reviewed Jul 31, 2019

View reviewed changes

curiousepic approved these changes Jul 31, 2019

View reviewed changes

jasonxp force-pushed the ce-63-paymill-add-amount-and-currency-to-store branch from f434ce5 to a2059bb Compare August 1, 2019 17:29

jasonxp merged commit a2059bb into activemerchant:master Aug 1, 2019

jasonxp deleted the ce-63-paymill-add-amount-and-currency-to-store branch August 1, 2019 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paymill: Add currency and amount to store requests #3289

Paymill: Add currency and amount to store requests #3289

jasonxp commented Jul 31, 2019

curiousepic commented Jul 31, 2019

molbrown commented Jul 31, 2019

leila-alderman left a comment

leila-alderman Jul 31, 2019

jasonxp Jul 31, 2019

leila-alderman Jul 31, 2019

jasonxp Aug 1, 2019

leila-alderman Aug 1, 2019

jasonxp Aug 1, 2019

leila-alderman Jul 31, 2019

jasonxp Jul 31, 2019

leila-alderman Jul 31, 2019

leila-alderman Jul 31, 2019

leila-alderman Jul 31, 2019

leila-alderman Jul 31, 2019

jasonxp Jul 31, 2019

jasonxp commented Jul 31, 2019

curiousepic commented Jul 31, 2019

molbrown commented Jul 31, 2019

Paymill: Add currency and amount to store requests #3289

Paymill: Add currency and amount to store requests #3289

Conversation

jasonxp commented Jul 31, 2019

curiousepic commented Jul 31, 2019

molbrown commented Jul 31, 2019

leila-alderman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonxp commented Jul 31, 2019

curiousepic commented Jul 31, 2019

molbrown commented Jul 31, 2019