NMI: Add tax, shipping, and ponumber level 3 fields #3239

jasonxp · 2019-06-06T14:35:59Z

No description provided.

jknipp

Looks good, 👍on refactoring common code.

jknipp · 2019-06-07T17:25:21Z

lib/active_merchant/billing/gateways/nmi.rb

@@ -102,6 +106,7 @@ def store(payment_method, options = {})
        add_customer_data(post, options)
        add_vendor_data(post, options)
        add_merchant_defined_fields(post, options)
+        add_level3_fields(post, options)


Seems odd that we would pass this info for store 🤷‍♂

I agree I don't think this is necessary.

👍Thanks -- I removed the fields from store.

jknipp · 2019-06-07T17:27:40Z

lib/active_merchant/billing/gateways/nmi.rb

@@ -131,6 +136,16 @@ def supports_network_tokenization?

      private

+      def add_field_if_present(post, options, field)
+        post[field] = options[field] if options[field]
+      end


👍I feel like a great portion of Active Merchant is testing for the presence of a field and adding it if present. Seems like there should be a base method like this that also has an option default to use if the field is not present.

FYI Benjamin Pollock did some exploratory work along these lines shortly before he left, but we never revisited it: #3067

In general though, it's much more common for adapters to check for presence in the line which adds the element. I think this is generally because there could be some mapping to different field names and seeing it all in one place is a bit more readable (from a certain perspective)

That said, I have nothing against this particular implementation in theory, it just feels a little weird for it to be only used for a small subset of field additions.

I streamlined the helper further and moved it to the base class with tests. That way, we can employ it in the future in any gateway implementation, and extend with other options if desired. Grepping the code, I saw this pattern hundreds of times, so I tend to think it's worthwhile to have a helper to start DRYing things out, even if exceptions like mapping to a different field name can't use it. Let me know if there are any strong preferences to the contrary though.

I think I like this route. It might be best to keep it simple, forcing the exceptions to be explicit (though it might be best to add some common exceptions, sparingly). The challenge will be remembering to use it 😄
I do have a couple small suggestions for the base methods.

👍 Should the base methods be named in such a way to indicate it will only work for request params/JSON API's rather than XML?

Small nitpick on the method arguments, I'd prefer more descriptive names for the method args or at least comments to indicate what they represent.

Good point. It might be worth making it handle both JSON and XML.

Good point on the naming. Since the context is the gateway class instead of somewhere like Hash, naming according to the intended use does work better. Let me know if anything is still not clear. I'd prefer to add the functionality for XML (which might be in a different helper altogether) later at some point when it'd be used, for the sake of scope and testing.

Agreed. We have already expanded scope on the original change so this makes sense.

curiousepic · 2019-06-10T14:58:46Z

lib/active_merchant/billing/gateway.rb

@@ -200,6 +200,16 @@ def supports_network_tokenization?
        false
      end

+      def add_present_fields(to, from, fields)


I think the field and fields methods should have a more similar name; I suggest changing this one to add_fields_if_present. I think that formulation is more explicit.

Should the singular method also be public? It would be a little weird for an adapter to use a plural method for a single field, but maybe it's NBD. Or perhaps this should be private or protected? I'm actually not sure I understand the impact of either in this context...

Sure - I like the idea of having singular/plural public methods, especially since both versions already exist.

curiousepic · 2019-06-10T15:26:13Z

lib/active_merchant/billing/gateways/nmi.rb

@@ -4,6 +4,7 @@ class NmiGateway < Gateway
      include Empty

      DUP_WINDOW_DEPRECATION_MESSAGE = 'The class-level duplicate_window variable is deprecated. Please use the :dup_seconds transaction option instead.'
+      LEVEL3_FIELDS = [:tax, :shipping, :ponumber]


This would also be kind of a new pattern; I'm not sure we should prefer it over setting the fields directly in the method call. What do you think @jknipp ?

To be consistent with what exists in Active Merchant, I would prefer that we set the fields in the method call. AM leans toward explicitness and since the constant is used only once, it's a slight pre-mature optimization.

@jasonxp That said, I don't want to discourage you from this type of improvement. I think it's great that you are thinking of simplification, reuse, and long-term maintainability.

Sure. Since the fields are already in the add_level3_fields one-liner, it's already clear that they're level 3 fields, so I agree there isn't much value in the constant.

curiousepic · 2019-06-10T20:51:45Z

lib/active_merchant/billing/gateway.rb

+
+      def add_option_to_post_if_present(post, options, option)
+        post[option] = options[option] if options[option]
+      end


🤔 Hrmmmmmnamingishard. I can't tell if these reservations are worth worrying about but: the term option is more general than a field with a value that would be added to a request, and this also sounds a bit like it takes the entire options hash in the context of the transaction, when it really only handles a passed subset. I think they're fine as argument names though, but field or similar might be better for the method name...

Looks great!

curiousepic

Cool, I think we're good. Thanks for working through our suggestions! 🚢

All unit tests and remote NMI tests passed. Closes activemerchant#3239

jasonxp requested review from a team and removed request for a team June 6, 2019 14:35

jknipp requested a review from a team June 7, 2019 17:13

jknipp approved these changes Jun 7, 2019

View reviewed changes

curiousepic reviewed Jun 10, 2019

View reviewed changes

jasonxp force-pushed the ecs-257-add-nmi-tax-fields branch from 4dff39c to 2b9e064 Compare June 10, 2019 20:14

curiousepic reviewed Jun 10, 2019

View reviewed changes

jasonxp force-pushed the ecs-257-add-nmi-tax-fields branch 2 times, most recently from d822d48 to 04936db Compare June 11, 2019 05:43

curiousepic approved these changes Jun 11, 2019

View reviewed changes

jasonxp force-pushed the ecs-257-add-nmi-tax-fields branch from 04936db to f6801bf Compare June 11, 2019 18:52

NMI: Add level 3 fields tax, shipping, and ponumber.

a1845c3

All unit tests and remote NMI tests passed. Closes activemerchant#3239

jasonxp force-pushed the ecs-257-add-nmi-tax-fields branch from f6801bf to a1845c3 Compare June 11, 2019 18:56

jasonxp merged commit a1845c3 into activemerchant:master Jun 11, 2019

jasonxp deleted the ecs-257-add-nmi-tax-fields branch June 11, 2019 19:16

whitby3001 pushed a commit to whitby3001/active_merchant that referenced this pull request Sep 3, 2019

NMI: Add level 3 fields tax, shipping, and ponumber.

79fe1db

All unit tests and remote NMI tests passed. Closes activemerchant#3239

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NMI: Add tax, shipping, and ponumber level 3 fields #3239

NMI: Add tax, shipping, and ponumber level 3 fields #3239

jasonxp commented Jun 6, 2019

jknipp left a comment

jknipp Jun 7, 2019

curiousepic Jun 7, 2019

jasonxp Jun 9, 2019

jknipp Jun 7, 2019

curiousepic Jun 7, 2019 •

edited

Loading

curiousepic Jun 7, 2019

curiousepic Jun 7, 2019 •

edited

Loading

jasonxp Jun 9, 2019

curiousepic Jun 10, 2019

jknipp Jun 10, 2019

curiousepic Jun 10, 2019

jasonxp Jun 10, 2019

jknipp Jun 10, 2019

curiousepic Jun 10, 2019

curiousepic Jun 10, 2019 •

edited

Loading

jasonxp Jun 10, 2019

curiousepic Jun 10, 2019 •

edited

Loading

jknipp Jun 10, 2019

jasonxp Jun 10, 2019

curiousepic Jun 10, 2019

jasonxp Jun 11, 2019

curiousepic Jun 11, 2019

curiousepic left a comment

NMI: Add tax, shipping, and ponumber level 3 fields #3239

NMI: Add tax, shipping, and ponumber level 3 fields #3239

Conversation

jasonxp commented Jun 6, 2019

jknipp 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

curiousepic Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic Jun 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic Jun 10, 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

Choose a reason for hiding this comment

curiousepic left a comment

Choose a reason for hiding this comment

curiousepic Jun 7, 2019 •

edited

Loading

curiousepic Jun 7, 2019 •

edited

Loading

curiousepic Jun 10, 2019 •

edited

Loading

curiousepic Jun 10, 2019 •

edited

Loading