Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorize.net: Pass Level 2 Data Fields #2444

Closed
wants to merge 1 commit into from

Conversation

curiousepic
Copy link
Contributor

@curiousepic curiousepic commented May 29, 2017

This required some shifting of the existing request building flow due to
strange element ordering/structuring requirements from ANET.

Unit:
83 tests, 473 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
59 tests, 204 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

This required some shifting of the existing request building flow due to
strange element ordering/structuring requirements from ANET.

Unit:
83 tests, 473 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
59 tests, 204 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
Copy link
Member

@shasum shasum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousepic Quite intricate (all those fields and ordering). Looks okay to me for the most part. If I were to nitpick, the way purchase order number is handled at two different places is little off. It'd be nice to localize it to one method. Noticed ANet has a separate file for cim tests. Not sure if you had a chance to look at it for relevancy.

@curiousepic
Copy link
Contributor Author

@shasum Thanks for the look over - There is a separate CIM-specific gateway which is unused by Spreedly, that those remote tests are for; this base ANET gateway had CIM txn flow integrated with it some time ago. Re: PO Number method(s), it's difficult to unify/simplify them further, given the multiple conditional dependencies 😕

Copy link
Member

@shasum shasum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, LGTM 👍

@shasum
Copy link
Member

shasum commented May 30, 2017

@curiousepic Not sure if this fits well with the PO logic but I was imagining something like this.

xml.order do
  xml.invoiceNumber(truncate(options[:order_id], 20))
  xml.description(truncate(options[:description], 255))
  add_po_number(xml, options, transaction_type)
end

def add_po_number(xml, options, transaction_type='')
  return unless options[:po_number]
  if transaction_type.start_with?("profileTrans")
    xml.purchaseOrderNumber(options[:po_number])
  else
    xml.poNumber(options[:po_number])
  end
end

@curiousepic
Copy link
Contributor Author

curiousepic commented May 30, 2017

@shasum I tried that, but because it is also added outside the order element for normal transactions, it gets added a second time, because the transaction_type isn't actually enough for it to determine which place it should and shouldn't go for every transaction type 🤦‍♂️ ...if that makes any sense.

@shasum
Copy link
Member

shasum commented May 30, 2017

@curiousepic Gotchya. I suspected some intricacy around it that I was definitely missing. What if we throw in part of the original guard clause inside the order element. Would that eliminate duplicates?

xml.order do
  xml.invoiceNumber(truncate(options[:order_id], 20))
  xml.description(truncate(options[:description], 255))
  add_po_number(xml, options, transaction_type) if transaction_type.start_with?("profileTrans")
end

def add_po_number(xml, options, transaction_type='')
  return unless options[:po_number]
  if transaction_type.start_with?("profileTrans")
    xml.purchaseOrderNumber(options[:po_number])
  else
    xml.poNumber(options[:po_number])
  end
end

@curiousepic
Copy link
Contributor Author

Well, that will then require passing transaction_type through basically every action, even though it's not needed in most of them. It seems as un-reason-able, if not more, than the split, isolated logic of the original... but not sure which is the lesser of evils. Anyhow this feels like an awful lot of time invested in a simple feature so I think I'm going to call it here, but totally open to revisiting it if I see other opinions - thanks for the effort!

@curiousepic
Copy link
Contributor Author

Closed by a7bf55c

@curiousepic curiousepic closed this Jun 1, 2017
@curiousepic curiousepic deleted the 2091 branch July 13, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants