Braintree and Worldpay: support overriding NTID

[aenand]

COMP-160

Adds support for the Braintree Blue and Worldpay gateways for merchants to override and bring their own NTID instead of relying on the standardized NTID framework

Test Summary
Local:
5908 tests, 79610 assertions, 0 failures, 23 errors, 0 pendings, 0 omissions, 0 notifications 99.6107% passed
Unit:
Worldpay:
119 tests, 672 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Braintree:
104 tests, 219 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote:
Worldpay:
104 tests, 447 assertions, 3 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 97.1154% passed
Braintree:
120 tests, 646 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

[travismorrison]

looks straightforward and good. I do have a code design ask that I feel sufficiently in my bones that I can't approve. however it may be misguided, lmk

[travismorrison]

I would suggest changing this signature to def add_external_vault(parameters, credential) (note loss of stored_ rather than a third argument, and choose which credential you want to send in earlier in the flow.

passing in options is a little confusing because options is from whence stored_credential[:network_transaction_id] comes from. So you got a function where argument 2 is a child of argument 3. unless I'm missing something.

Furthermore... there is this up top:

return unless (stored_credential = options[:stored_credential])

does this need to be updated now that options[:network_transaction_id] is a thing? Or is the "old style" still the sole table stakes to moving forward in this control flow?

[aenand]

AM's pattern prefers options over alternate naming if it's the full options hash. I do get your point about argument 2 is from argument 3 so I'll drop the stored_credential one.

Additionally the upper conditional does not need to be modified because we still need to make sure the stored credential mappings are in place before adding a NTID which is what the return unless (stored_credential = options[:stored_credential]) logic checks.

[travismorrison]

I would love another AM veteran's approval on this before deploy, but from my angle, it looks sensible.

[curiousepic]

Nice and simple, looks good.