Implement Worldpay store #3232

bayprogrammer · 2019-05-24T17:08:38Z

We are only supporting shopper-scoped tokens right now, and not
implementing delete/unstore, saving a token along with an authorize or
purchase call, overriding token details on use, or setting the token
lifetime.

The design of the opaque token authorization string we return from
store has been intentionally designed to be flexible enough to extend
in a backwards-compatible way should any of the above features need to
be implemented.

Unit:
59 tests, 329 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
50 tests, 219 assertions, 2 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
96% passed

The two failing remote tests are concerning 3DS parameter pass through
which the test credentials I'm using does not have permission to use
presently.

ECS-347

bayprogrammer · 2019-05-24T17:11:49Z

lib/active_merchant/billing/gateways/worldpay.rb

+          (pair ? pair.last : nil)
+        end
+      end
+


If, in the future, anyone needs to support merchant-scoped tokens, it will be easy to add without breaking compatibility with the current format. Likewise should this form of authorization need to embed the original order id (not something we need to do now, but would be needed to be able to return an authorization from a authorize/purchase call where the caller also wants to store the token in a single call (Worldpay does support this use case)).

So I think I like this. But I do want to bring up a consideration. Obviously the way AM handles authorizations is not ideal, but to the extent that there is a standard, that standard seems to be for newly supported elements to be concatenated on the end of whatever is already present, for any action type. So what I would have expected here is that the data for a stored card would start with the second element of a concatenated authorization, regardless of whether that first element actually has a value.

Does that make sense? And if so, does it sound like something we should actually care about?

I think the assumed advantage of this is that, if we're assuming it needs to remain a string, the ordering allows it to be parsed both by the adapter and by anything else looking at the string (as long as the order is known and maintained), independent of transaction type (some adapters add the action type for further explicit parsing if all/most response elements are present for multiple action types).

I think we could retain backward compatibility with existing pay_as_order actions by checking the length of a .split auth.

Yah, I think it's a good idea (and easy to implement) to ensure that the order_id, if ever it should be present in a compound auth like this one, would take the first ordinal position (and just be empty for now in store where it doesn't apply). That way any future extensions where both the order_id and these token-specific bits are necessary (such as if anyone needs to implement authorize+store in a single call) can rely on the first element (including if there are no "|" characters) always being the order_id.

Not completely sure though if I follow about checking the length of split for pay-as-order though. Are you referring to how we distinguish between pay-as-order payment method strings and stored token id strings?

Cool, I think we should go with that.

Not completely sure though if I follow about checking the length of split for pay-as-order though. Are you referring to how we distinguish between pay-as-order payment method strings and stored token id strings?

Yes, though I guess it depends on how the current methods change, we might not even need that. I'll leave it up to you and wait to see the changes.

@curiousepic Ok, I've pushed up some updates that implement this.

Since we want to ensure there is a slot in our compound authorization strings that can hold the order_id value (if and when it's used), I went ahead and ensured that these complex authorization strings could be used in place of a bare order_id authorization.

Was actually a bit uncertain whether to proceed that far, but it would have felt awkward to me to have the ability to store the order_ids in an authorization that would have been invalid to use for capture/etc. Especially welcome feedback on this point.

bayprogrammer · 2019-05-24T17:14:09Z

lib/active_merchant/billing/gateways/worldpay.rb

@@ -460,17 +507,35 @@ def handle_response(response)
      #   - a string or an array of strings (if one of many responses)
      #   - An array of strings if one of many responses could be considered a
      #     success.


We need the ability to return success based on responses materially different than looking for :ok in the parsed response. The :last_event stuff also wouldn't work for store because the response for store doesn't include last event information. I broke things down into simpler methods for each type of success-check, and left things more easily extensible for any future types of action-specific success criteria we may need (rather than continue to pile on the existing conditional logic in the original method).

I'd vote to just axe success_and_message_from too.

bayprogrammer · 2019-05-24T17:17:43Z

lib/active_merchant/billing/gateways/worldpay.rb

@@ -144,6 +151,10 @@ def credit_request(money, payment_method, options)
        commit('credit', build_authorization_request(money, payment_method, options), :ok, 'SENT_FOR_REFUND', options)
      end

+      def store_request(credit_card, options)
+        commit('store', build_store_request(credit_card, options), options)


Note that we are intentionally not passing the *success_requirements argument. It is optional, and not applicable to store. See comments RE: success_and_message_from.

bayprogrammer · 2019-05-24T17:19:10Z

lib/active_merchant/billing/gateways/worldpay.rb

          node.elements.each { |e| parse_element(raw, e) }
        else
-          raw[node.name.underscore.to_sym] = node.text unless node.text.nil?


This allows us to include the error message when there are newlines. I discovered this issue when trying to make assertions about the returned error message from existing canned responses in the unit tests. Now we can return the error message whether the CDATA information is preceded and/or followed by newlines or not.

curiousepic · 2019-05-28T20:49:13Z

lib/active_merchant/billing/gateways/worldpay.rb

@@ -241,7 +268,7 @@ def add_amount(xml, money, options)
      end

      def add_payment_method(xml, amount, payment_method, options)
-        if payment_method.is_a?(String)
+        if options[:payment_type] == :pay_as_order


I have yet to fully grok your additions, but I'm seeing that for users of this feature will need to start sending a new payment_type option? Would it be best to retain the string check for backward compatibility?

Or was this made to distinguish it from stored PMs, since they end up as strings?

I see now that this is generated internally by a method that parses the PM, neat. Disregard for now.

Yep, you got it. The problem was that both the TPV tokens and pay-by-reference/pay-as-order string payment methods needed to be distinguished early on. Not a user-facing option, should be fully backwards-compatible.

bayprogrammer · 2019-05-29T18:15:53Z

lib/active_merchant/billing/gateways/worldpay.rb

+
+      def payment_details_from(payment_method)
+        payment_details = {}
+        if payment_method.is_a?(CreditCard)


@curiousepic So I updated this to rely on our parsing the complex auth and checking that the token-specific field is in the resultant hash to determine whether it's a pay-by-order auth string or token auth string.

As part of this I flipped the logic around to rely on detecting whether it's a credit card payment method using is_a? (I generally try really hard to avoid is_a?). One thing I'm uncertain of is if there is the potential for any other kind of payment_method we need to worry about at this stage which would fail this check but also not be a String (and thus cause the later code to break which assume either a CreditCard or String instance being used).

Is there a common method that all card-like objects can respond to which could be used to differentiate them from String instances without having to resort to is_a? here?

Looks good! We do often use respond_to?(:number).

Ah, I like that better. Updated. Thanks!

curiousepic

Cool, should be good to go. Nice work on this!

We are only supporting shopper-scoped tokens right now, and not implementing delete/unstore, saving a token along with an authorize or purchase call, overriding token details on use, or setting the token lifetime. The design of the opaque token authorization string we return from `store` has been intentionally designed to be flexible enough to extend in a backwards-compatible way should any of the above features need to be implemented. Unit: 66 tests, 392 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 50 tests, 219 assertions, 2 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 96% passed The two failing remote tests are concerning 3DS parameter pass through which the test credentials I'm using does not have permission to use presently. ECS-347 Closes #3232

We are only supporting shopper-scoped tokens right now, and not implementing delete/unstore, saving a token along with an authorize or purchase call, overriding token details on use, or setting the token lifetime. The design of the opaque token authorization string we return from `store` has been intentionally designed to be flexible enough to extend in a backwards-compatible way should any of the above features need to be implemented. Unit: 66 tests, 392 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 50 tests, 219 assertions, 2 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 96% passed The two failing remote tests are concerning 3DS parameter pass through which the test credentials I'm using does not have permission to use presently. ECS-347 Closes activemerchant#3232

bayprogrammer self-assigned this May 24, 2019

bayprogrammer commented May 24, 2019

View reviewed changes

bayprogrammer requested a review from a team May 24, 2019 17:56

curiousepic reviewed May 28, 2019

View reviewed changes

bayprogrammer commented May 29, 2019

View reviewed changes

curiousepic approved these changes May 29, 2019

View reviewed changes

Zeb DeOs added 2 commits May 30, 2019 12:21

Fix CHANGELOG for additions since v1.95.0

8c9ff13

bayprogrammer merged commit 1243289 into activemerchant:master May 30, 2019

bayprogrammer deleted the ECS-347_worldpay-store branch May 30, 2019 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Worldpay store #3232

Implement Worldpay store #3232

bayprogrammer commented May 24, 2019

bayprogrammer May 24, 2019

curiousepic May 28, 2019

curiousepic May 29, 2019 •

edited

Loading

bayprogrammer May 29, 2019

curiousepic May 29, 2019

bayprogrammer May 29, 2019

bayprogrammer May 24, 2019

curiousepic May 28, 2019 •

edited

Loading

bayprogrammer May 24, 2019

bayprogrammer May 24, 2019

curiousepic May 28, 2019 •

edited

Loading

curiousepic May 28, 2019

curiousepic May 28, 2019 •

edited

Loading

bayprogrammer May 28, 2019 •

edited

Loading

bayprogrammer May 29, 2019 •

edited

Loading

curiousepic May 29, 2019

bayprogrammer May 29, 2019

curiousepic left a comment

Implement Worldpay store #3232

Implement Worldpay store #3232

Conversation

bayprogrammer commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic May 29, 2019 • edited Loading

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

curiousepic May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic May 28, 2019 • edited Loading

Choose a reason for hiding this comment

bayprogrammer May 28, 2019 • edited Loading

Choose a reason for hiding this comment

bayprogrammer May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic left a comment

Choose a reason for hiding this comment

curiousepic May 29, 2019 •

edited

Loading

curiousepic May 28, 2019 •

edited

Loading

curiousepic May 28, 2019 •

edited

Loading

curiousepic May 28, 2019 •

edited

Loading

bayprogrammer May 28, 2019 •

edited

Loading

bayprogrammer May 29, 2019 •

edited

Loading