Support extra data for Barclay Smartcard credits

[bpollack]

Barclays Smartcard will be requiring credits that lack a PSP to include four additional pieces of information: the date-of-birth, entity type, nationality, and full name of the person requesting the credit. This change enables including them.

[bpollack]

I couldn't find this pattern elsewhere, but it seemed like a really clean way of handling this client-side. Other reasonable options would be to do this, but without the requires! line; or just to unconditionally pass through these attributes, if they exist.

[dtykocki]

I've typically seen the pattern of conditionally adding each option individually vs the use of requires!. Example:

post[:dateOfBirth] = if options[:date_of_birth]
post[:entityType] = if options[:entity_type]
post[:nationality] =  if options[:nationality]

I don't have the full scope of changes in my head so I'm assuming that each item can be sent to the gateway individually instead of requiring the entire group of options.

[bpollack]

I don't have the full scope of changes in my head so I'm assuming that each item can be sent to the gateway individually instead of requiring the entire group of options.

In this particular case, the absence of an order ID requires all four options at once; none are optional, and none are required for other transaction types—hence this style of requires!. (More details in the JIRA ticket this branch is named after, if you're bored.) Whether it should be ActiveMerchant's job to do this, and whether shoving a requires! halfway through the method, are still points I'm not clear on. It definitely works, but it's a pattern I didn't see in other adapters I glanced at.

[curiousepic]

What's the purpose of the order_id check?

[curiousepic]

And yeah it's more common to check the fields individually than with a requires! block. We usually let users send whatever they have and leave out what they don't, and get a error message from the gateway. Depends on how the gateway responds to missing or blank fields.

[bpollack]

The order_id check is because those fields aren't required if it's present. The alternative to doing it this way would be to say that it's not ActiveMerchant's problem to tell you you're missing fields, at which point this method would instead become:

def credit(money, creditcard, options = {})
  post = payment_request(money, options)
  post[:amount] = amount_hash(money, options[:cgurrency])
  post[:card] = credit_card_hash(creditcard)
  post[:dateOfBirth] = options[:date_of_birth] if options[:date_of_birth]
  post[:entityType]  = options[:entity_type] if options[:entity_type]
  post[:nationality] = options[:nationality] if options[:nationality]
  post[:shopperName] = options[:shopper_name] if options[:shopper_name]
  commit('refundWithData', post)
end

This seemed harsher to the user and doesn't save us much work, but it permits greater flexibility on the part of the gateway.

[bpollack]

This can be pulled out of the credit card, but I was a bit ambivalent on whether that actually made sense. Nationality, entity type, and date of birth obviously cannot be pulled out of that field, so having this consistently be its own set of data seemed saner to me.

[curiousepic]

I think it does as well. Typically I add a || creditcard.name as a fallback in these situations.

[curiousepic]

Though I see with the requires! it would never trigger...

[curiousepic]

Typo in shopper_name

[bpollack]

Dangit. Fixing.

[bpollack]

Okay, all changes we talked about @curiousepic are in here. Code does much less and doesn't confuse :order_id for a pspReference anymore.

[curiousepic]

Now without the requires! we can at least be helpful with a fallback like || creditcard.name

[bpollack]

I wrote this, but as I started updating the tests to handle it, I realized I disagree with the feature: for us to do this, you'd have to 1) issue a credit, 2) successfully supply the date of birth, entity type, and nationality, but 3) forget shopper name. That's already an edge case of an edge case, but yoinking data from the credit card fields has a fun side-effect that the user may get the credit rejected for having the wrong shopper name, and then will be trying to figure out how that's even possible/what the shopper name was in the first place.

I thus think this is probably the best path forward as-is.

[curiousepic]

I think that's fair 👍

[curiousepic]

Otherwise looks good, this is probably the best we can do for now.