Support custom fields for TrustCommerce

[jasonxp]

Also fix some tests that previously failed.

Unit:
4236 tests, 70441 assertions, 0 failures, 0 errors, 0 pendings, 2 omissions, 0 notifications
100% passed

Remote:
17 tests, 67 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

[jasonxp]

This is just a null safety check that fixed an error that occurred when the authorization was nil.

[molbrown]

LGTM 👍

[leila-alderman]

I made several suggestions and added a few general questions throughout. Enjoyed the opportunity to learn more about implementing custom fields!

[jknipp]

💙the clarifying comments!

[jasonxp]

@leila-alderman All feedback has been addressed with the latest commit.

[leila-alderman]

The standard variable name for this seems to be @declined_card.
(Within the Active Merchant code base, there are 33 instances of bad_credit_card and over 400 instances of declined_card.)

[jasonxp]

I think the variable name here indicates its use in the tests, and can't quite see impact/value in making additional changes to match a majority of other, similar variable names in different tests. Let me know if you think otherwise.

[leila-alderman]

Do you feel that declined_card isn't as descriptive of its use in the tests?

Having generally seen declined_card used as the variable name in Active Merchant tests, I just noticed the different name and had to do a quick check to see whether it was trying to accomplish the same thing. I think it's cleaner, simpler, and clearer to stick to the same variable name that's widely used throughout. I'm generally a big fan of consistency. 😀

[jasonxp]

Yep, in this case I think bad_credit_card is more descriptive. "Declined" suggests to me that there was or at least will be an attempted transaction by the gateway. This card is used in a test that generates a format error, without the gateway attempting to transact.

I agree consistency is generally good, but I do think there is a tradeoff when it comes to making revisions to address minutia. Time and focus is extremely valuable as well, and I think that we could be sacrificing too much of them if we get in the habit of worrying about this level of detail (matching variable names in other test files).

[leila-alderman]

I would tend to disagree. I noticed that this variable name was different than what I was accustomed to seeing and had to spend more time tracking it down and clarifying whether it was the same as declined_card or if it was indicating something else. I feel that maintaining consistency saves time long term, and I also feel that accurate and effective code lies in the details and that this would be a pretty quick and easy change to make.

All that said, those are just my opinions, and this is your PR. Thanks for being open to discussion on these issues!

[jasonxp]

Thanks for that perspective. I would have understood the use from the variable name and definition, so it's interesting to learn that wouldn't be the case for everyone. My general thought is mainly that I wouldn't expect people to spend time searching for stats on which similar local/instance variable names are more commonly used in other files in the codebase, as I wouldn't see that as an effective use of time relative to the value it would provide. It could be a slippery slope of time consumption if we held variable name popularity across files as a standard. I'm happy to change the name here since you've already done that legwork while working to understand the code.