VisaNet Peru: Always include DSC_COD_ACCION

[bayprogrammer]

@nfarve could you check my thinking on this draft pull request? I'm working on a ticket (ECS-173) where I need to modify this adapter to add extra information to messages, but the existing message extraction logic is a bit tricky and I can't quite tell if it was doing what we expected or not.

---

VisaNet Peru: Ensure msg kept on bad cancelDeposit

Looking at:

#2772 (comment)

It seems our intent is to stash the errorMessage in options so it can be added
to a follow up request if the follow up request fails (this would only happen
when the user has opted to force a full refund for an unsettled refund, if I'm
understanding the logic correctly).

However, when I went to add a test ensuring my understanding is correct it
seems we may have been checking for the wrong action for this "keep error
message before retrying full refund" logic, and it wasn't getting saved in the
option hash as I expected.

I've added a test that works the way I expect it to, then modified the
message_from implementation to make the test pass. I don't know for sure
whether this is correct, but I need to make sure the logic is correct here
before I can proceed with the fix for ECS-173 (where I'm going to add extra
information to the message).

ECS-173

Units:
14 tests, 74 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remotes:
17 tests, 25 assertions, 15 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
11.7647% passed

(Most of the remote tests are failing because our test credentials are bad and
we've not been able to get new ones yet).

[nfarve]

Sorry for taking so long to get to this!

It seems our intent is to stash the errorMessage in options so it can be added
to a follow up request if the follow up request fails (this would only happen
when the user has opted to force a full refund for an unsettled refund, if I'm
understanding the logic correctly).

I think you have it correct. I am trying to follow my logic and it looks like your change makes sense.

[bayprogrammer]

Updated commit (squashed from initial commit used in draft):

---

VisaNet Peru: Always include DSC_COD_ACCION

Always include the action code description (DSC_COD_ACCION), when
available.

This also adds a test[1] to ensure the logic[2] is correct for our
stashing the message in the options hash for retrying a full refund if a
partial refund fails.

[1] #3174 (comment)

[2] #2772 (comment)

ECS-173

Units:
14 tests, 78 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remotes:
17 tests, 25 assertions, 15 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
11.7647% passed

(Please note that most of the remote tests are failing because our test
credentials are invalid and we've not been able to get new/valid
credentials as of the time of this commit).

[jknipp]

Can we add a remote test and also a unit test for the full message that would be returned if all values are present?

[jknipp]

Does this change the error message format from commas to pipe? I'm not sure if this might cause a problem for customers if they are parsing on commas. Also, if it changes the order, we might think about reworking this.

[bayprogrammer]

I don't think the joining was working in the first place: because the cancelDeposit case comes first, the original elsif action == 'refund' condition would not have been true such that the stashed message would have been returned to the user (it may have been stashed, but we would never have used it when the action was refund). This is what prompted my initial draft PR with the additional test to verify what the intended behavior was (properly distinguishing between a refund and cancelDeposit and how that affected the message construction.

If that is correct, my thinking was that the risk of end users relying on parsing the field is minimal, and the advantage of the pipe over the comma moving forward is that if sometimes the messages we're joining have punctuation already, I wanted to avoid something like "Error message., Error code explanation". I don't know whether that's a real concern, just noticed the punctuation in some of the canned responses.

Since there was only this one (probably not working) case where we were doing the joining of any kind, I don't think there is an order to preserve for the expanded case. In the specific case where we were joining only if the first cancelDeposit refund attempt failed, this does change the order. However, if people are parsing this string, then previously they would be getting the response['errorMessage'] in practice, so putting that first seemed logical (even though it changes our original intended ordering in the special case).

Edit: I made a mistake there, when the errorMessage was empty then the DSC_COD_ACCTION would have been used instead for the broad case; still the two would only have been joined as I previously described.

I'm not opposed to changing any of this though, just laying out my thought process so far.

[jknipp]

Ok, that makes sense and if it wasn't working before there is a lower risk.

[bayprogrammer]

Can we add a remote test and also a unit test for the full message that would be returned if all values are present?

@jknipp Yep, I'll add those. For the remote test I won't be able to run the test successfully (we don't have valid credentials for VisaNet Peru), but I think I can write something reasonable and perhaps leave a note that we should verify it's correct once credentials are obtained in the future.

[bayprogrammer]

Hey @jknipp, just a heads up that I've pushed up my revisions (for whenever you have a free moment, no rush). Thanks again for your guidance and insights on this one so far!

[jknipp]

@bayprogrammer This looks good to me.

@curiousepic is there precedence for joining error messages like this that we should be aware of for consistency?

[bayprogrammer]

@curiousepic is there precedence for joining error messages like this that we should be aware of for consistency?

@curiousepic Just wanted to bump this in case it fell out of view. Please let me know if you have any questions about my implementation or ideas on better string-joining characters for such cases. :)

[curiousepic]

Could use .blank?

[bayprogrammer]

empty? was already in use here (I just extracted to a method to clarify the conditionals elsewhere) so I retained it for lack of a reason to change it. Is .blank? to be preferred to empty? here?

[curiousepic]

Not a big deal; in fact there's a vague pressure to avoid dependencies on ActiveSupport.

[curiousepic]

Apologies for missing that earlier - I don't think we have particularly consistent formatting for this. Most often there are two values and we do something like more general error message or code: more specific error message, but I think just concatenating them with pipes is fine in this case since it could be one or more of three different things coming through. It might make sense to prepend them with the field names, though I don't think we've done that before.

[bayprogrammer]

@curiousepic

It might make sense to prepend them with the field names, though I don't think we've done that before.

Do you have a strong preference to prepend field names in this case? I'm leaning against, but will defer to your judgement here.

[curiousepic]

I don't have a strong opinion, but since this is for the merchant's benefit, it would be nice for them to have the extra context, since they may not be able to tell which system the message is referring to.

And I just realized, adapters more often have a hierarchy of ||s for different error message fields, so this is already theoretically more information than usual, but I think being explicit would be nice.

[bayprogrammer]

@curiousepic @jknipp Thank you both for your review and feedback. Here's where I'm at:

One potential wrinkle here is in the case we retry a full refund when a partial fails (and the user arranged for the retry via the options) we include both the original message from the failed attempt then any that are generated for the full refund. Figuring out how to structure this extra information in a key-value format could get pretty messy. Do we come up with a bespoke format here, or use embedded escaped JSON as the returned message (and do we always do that, or only when there's this concatenation going on)?

Also given VisaNet Peru is already returning compound messaging in a format like "['message 1', 'message 2'], our own additions could make that even muddier. If we JSONified our concatenated messages, do we further escape these multi-message cases coming from VisaNet Peru, or incorporate them as a component in the JSON as a first class array literal?

Though I very much sympathize with giving as much context as possible back to the merchant, if it's up to my judgement, I am strongly inclined to keep the simplest approach (especially given this message extraction logic is already pretty tricky). Maybe a unified compound message format would be desirable in the future, but I don't feel like this would be the right place to take that on (though am happy to follow existing established patterns if we already have a way of dealing with cases like this).

Please let me know if my rationale here makes sense to move forward, or if I need to step back and rethink this issue.

[jknipp]

@bayprogrammer I think your logic makes sense. Let's do our best to provide as much information in the simplest approach. It's likely that whatever complex format we come up with won't provide the user what they need or in a helpful format. Let's take the simple approach and have customers/users drive future iterations.

[curiousepic]

@bayprogrammer 👍 on that last paragraph.

Re: the first two, I'm not sure I 100% follow the adapter logic here, but is there any chance those flows can and/or should be handled with MultiResponse? Or is the "retry" entirely gateway-side?

[bayprogrammer]

@curiousepic I don't think I understand for certain what a "gateway-side" retry would mean in this context. It looks like what we're doing is attempting a full refund based on a combination of whether the initial (non-full?) refund via the cancelDeposit request fails AND the user opted to force a full refund in those cases. So it's controlled via a combination of what the gateway does and what the user asked the adapter to do.

With the existing logic we always return a single response, either the original one (if success, and failure unless otherwise told otherwise by caller), or the attempted full-refund response.

I've looked at some other uses of MultiResponse in other adapters, and very often it seems to be used to capture each response in a multi-request flow to implement the action. But I've also found a couple instances where we also use MultiResponse with internal conditional logic. I also see instances where we use a multi-response but then explicitly only return the last (MultiResponse.run { |r| ... }.last). I don't understand what the point of MultiResponse would be if we were only going to return the last one.

The advantage I see of switching to MultiResponse in this case is that we can eliminate the message concatenation that is related to the retry logic (returning both responses in the MultiResponse, and let the user inspect which messages went with which response). The disadvantage is the existing logic was already in place and vetted, and changing it does not seem to be necessary to complete the task at hand (always returning the DSC_COD_ACCION as part of the message).

Do you think there is a compelling reason to switch to MultiResponse now as part of this ticket, or could/should that be filed as a backlog maintenance issue? (I ask genuinely here, not meaning it in an adversarial sense at all; I lean heavily on and appreciate your insight here!)

[curiousepic]

@bayprogrammer Your summary is accurate, and the message handling is the primary reason I suggested MultiResponse, though the flow overall seems to match the MultiResponse pattern as well (as in, I probably would have implemented it that way originally, though I could be missing some nuance that would contraindicate it). But I definitely don't think it's necessary for the task at hand, and whether we should move to it eventually can certainly be a separate discussion 👍

[bayprogrammer]

Apparently the design of MultiResponse is that follow-up requests are ignored unless prior responses are a success (unless you tell it to ignore the result, which I cannot do here without breaking the duck-type of the result). Or I am not understanding how to use MultiResponse.

Here's what I want (VisanetPeruGateway#refund):

      def refund(amount, authorization, options={})
        MultiResponse.run do |r|
          params = {}
          params[:amount] = amount(amount) if amount
          add_auth_order_id(params, authorization, options)
          r.process { commit('cancelDeposit', params, options) }
          if try_full_refund?(r, authorization, options)
            prepare_refund_data(params, authorization, options)
            r.process_unconditionally { commit('refund', params, options) }
          end
        end
      end

Note how I need the second process to happen without regard to the success of the first one (for our logic is predicated on trying the second request on whether the first failed, not restricted to if the first succeeded).

Here's how I had to update MultiResponse to accomodate:

class MultiResponse < Response
  # ...

  def process(ignore_result=false, &block)
    return unless success?
    process_unconditionally(ignore_result, &block)
  end

  def process_unconditionally(ignore_result=false, &block)
    response = block.call
    self << response

    unless ignore_result
      if(@use_first_response && response.success?)
        @primary_response ||= response
      else
        @primary_response = response
      end
    end
  end

  # ...
end

So I think I now understand why we didn't use MultiResponse originally: its logic is incompatible with our requirements. Here are our options as I see them:

1. Go with the original solution, and live with the lack of MultiReponse for this method
2. Modify MultiResponse to accommodate unconditional processing (what my examples above do)
3. Make some other kind of response which has the same behavior as MultiResponse save for removing the predicate for follow-up processing depending on success of previous requests.

My change to MultiResponse seems to work (didn't cause regressions in the unit test suite as a whole), but now I'm really beginning to think I'm straying too far outside the bounds of the original ticket.

[curiousepic]

Aaah, so the nuance was there.

I like the idea of making MultiResponse more versatile. Though I wonder if it should instead be an argument similar to ignore_result rather than another method?

Thoughts @jknipp?

[bayprogrammer]

If I may offer this for consideration, I am opposed generally to adding arguments to a method just to control its internal logic via a series of internal conditionals. I think smaller methods with clear method names is a preferable way to add specialization over adding options to existing methods to achieve the same. I submit that the former are easier to test, easier to maintain, and easier to reason about. They allow the caller to control the logic rather than rely on knowing the positional arguments and how they are meant to control that logic internally.

If the process method signature became something like this:

def process(ignore_result=false, ignore_previous_response_success_status=false)
  # ...
end

And I want to not ignore the result but do want to ignore previous success statuses (as I would need to do for this ticket), then to make the call would look something like this:

r.process(false, true) { ... }

That is much less clear to me at the call site than a clearly named method would be.

[jknipp]

Going back to @bayprogrammer's solution suggestions:

Do we feel like an update toMultiResponse will be useful for adapters and requests other than this specific use case? This is starting to feel like premature optimization.

If we do decide that the logic should live in MultiResponse: In general I do like Zeb's sentiment about breaking code into separate functions, BUT in this case, the smallest amount of change is introduced if we add an additional argument.

Could we work around the concern of arguments to process using a mix of default args and keyword args?

def process(ignore_result=false, process_unconditionally: false)
  return unless success? || process_unconditionally
end

# Allows you to stick with the default for ignore_result, but intentionally specify the other argument
r.process(process_unconditionally: true) 

[bayprogrammer]

While I definitely would not insist on the new method approach, I think the diff in terms of lines of functional code changed would be nearly the same as just adding a new method, and adding a new method leaves the existing method call signature completely unchanged (no new arguments, exact same external-facing behavior). But if we were really set on avoiding a new method, then I would agree that a new optional keyword argument is better than an additional optional positional argument. (I would not be strongly attached to method over keyword here.)

However, I think this is becoming a case of "perfect is the enemy of done". I really hate leaving behind technical debt, but I am not sure this PR is the right place to burn down the debt of the potential shortcomings of MultiResponse. If MultiResponse could have been used as is, it would have been a more obvious win to use it here. I am reluctant to rush to make changes to the API of MultiResponse just for this use case.

If we really can't move forward with this PR because of the existing design of MultiResponse, then I believe we should treat changing MultiResponse as a separate issue that blocks this PR until addressed. I'm in favor of moving forward with this PR as it is currently coded.

[curiousepic]

I am not too worried about pushing current code as a bandaid that shouldn't be too painful to rip off later if we deem appropriate.

[jknipp]

@bayprogrammer I agree. I think we have spent more time than we would have liked on this issue already. I don't like the idea of making changes we aren't comfortable with for a single use case.

I'm recommending we move forward as is and come back to this in the future.

[bayprogrammer]

@curiousepic @jknipp thank you both for your review & thoughtful analysis and feedback. I really appreciate it!