Change 408955 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[wikimedia/fundraising/SmashPig@master] Add mapping for Ingenico payment status codes

https://gerrit.wikimedia.org/r/408955

Change 408955 merged by jenkins-bot:
[wikimedia/fundraising/SmashPig@master] Add mapping for Ingenico payment status codes

https://gerrit.wikimedia.org/r/408955

To become more familiar with the existing implementation I ran through the failure user journey locally and documented the process with points of interest comments specific to the ticket.

https://etherpad.wikimedia.org/p/error-parsing

Change 415085 had a related patch set uploaded (by Mepps; owner: Mepps):
[wikimedia/fundraising/SmashPig@master] Tests passing, clean up

https://gerrit.wikimedia.org/r/415085

Ingenico credit card test cases in excel format: https://docs.google.com/spreadsheets/d/1JOhxAlKk4BdR-Eo75fuSfG0bUOuEgAHr0LI-XMxVjkY/edit?usp=sharing

Change 415907 had a related patch set uploaded (by Jgleeson; owner: Jgleeson):
[wikimedia/fundraising/SmashPig@master] T176502: WIP add tests for hosted payment rejections*

https://gerrit.wikimedia.org/r/415907

I've started looking into Omnipay as a source of inspiration to see if there are any strategies being used around error handling that would help us.

So far, I think the agnostic gateway failure handling (outlined here: https://github.com/thephpleague/omnipay#error-handling) is intuitive, and the API looks clean. I like the idea of checking composite response object properties in the boolean-check fashion vs searching arrays where there's a practical advantage. However, that approach requires mapping and modelling on top of what we already have and some considerable refactoring vs the existing mapping from less complex formats such as serialised data and arrays, which ultimately creates more to do in our short window!

I do think we could go wild in SmashPig, time permitting, with exceptions. And have a collection of general exceptions which could apply to all gateways (lower level for now) and then have a gateway specific bunch. We then "throw" these exceptions around Smashpig and make available for implementing libraries as needed. The cost is more work upfront and a few more try/catch/finally blocks. The benefit of that style for me vs passing arrays or serialised data is that the OOP style scales more naturally, e.g. you can quickly enhance/extend the exception classes as they become more useful adding new properties/types to/of the exception which are immediately available to areas in the code already using them. We also get bonus points S & L of SOLID.

I think there may be value in comparing an implementation in Omnipay to one of our own of the same gateway and see how they handle prickly things like extensive lists of gateway specific event codes and any other obvious areas of gateway integrations that add challenges to design agnosticism.

So it looks like we already have the general layer of exceptions in SmashPig that I described in the previous comment, I somehow missed them ... all.

Change 419842 had a related patch set uploaded (by Jgleeson; owner: Jgleeson):
[mediawiki/extensions/DonationInterface@master] T176502 WIP: Updated Ingenico error processing

https://gerrit.wikimedia.org/r/419842

UPDATED

I've made some progress on this task. We have a *working* implementation. Tests are outstanding, and I've added a few comments explaining my thoughts which can be taken out once the final approach is agreed.

Patch
SmashPig: https://gerrit.wikimedia.org/r/#/c/415907/

Summary:
It felt to me that trying to build a nice new reusable exception throwing process for gateway specific failure scenarios via some new abstract/base SmashPig API was the way to go at first, and Ingenico would benefit from it, but after sinking a lot of time trying to work out what this would look like and then trying to shoehorn a few ideas into what we already have between SmashPig and DI, I eventually gave up on that, although I think we could revisit this in the future as we expand the list of integrations within SmashPig.

So instead, I took a different route and drew a line between the SmashPig Ingenico API top-level errors and lower-level payment status errors, throwing an exception for the former and logging for the latter. We pass the response (including any lower-level errors) back to DonationInterface to be processed in line with the existing global collect processResponse()

The latest version of this restores DonationInteface back to its original state and instead now gives SmashPig sole responsibility for processing and normalising any errors encountered, before handing them back to DonationInterface.

@Ejegg, @mepps I've pushed the latest version of the Ingenico error work. I've tried to make the distinction between the two error types more clear in the code and implemented a solution for the feedback around collating all types of errors together in the SmashPig layer so that DonationInterface doesn't have to look up specific key paths.

I'm still not 100% happy with the ambiguous split between the different types of errors we get back and how we're currently handling them, exceptions for some, logging for others. It dawned on me that maybe this needs challenging, as I've assumed due to the original code having this error type split, that this needs to be the case?

I've tried introducing *another* distinction in the naming, this time its topLevelErrors & lowerLevelErrors. If you have any better naming suggestions, throw them out. I'm a little bit sad that a single API response is being processed and split across DI and SmashPig but without porting all the DI error code checking over to SmashPig, I can't see how we can get round it.

Change 419842 abandoned by Jgleeson:
T176502 WIP: Updated Ingenico error processing to detect payment status errors.

Reason:
no longer needed

https://gerrit.wikimedia.org/r/419842

Latest version here: https://gerrit.wikimedia.org/r/#/c/415907/

Things to discuss:

• Do we want to capture API Exceptions and send them to DonationInteface nicely or let it blow up as it's doing currently?
• Do we need to perform the same status error check behaviour currently happening within HostedCheckoutProvider, in IngenicoPaymentProvider ('payment.statusOutput.errors' looks like a thing)?

Change 415085 merged by jenkins-bot:
[wikimedia/fundraising/SmashPig@master] Tests passing, clean up

https://gerrit.wikimedia.org/r/415085

Change 415907 merged by jenkins-bot:
[wikimedia/fundraising/SmashPig@master] T176502: Updated Ingenico error handling

https://gerrit.wikimedia.org/r/415907

Change 423830 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/DonationInterface@master] WIP: bad code retry tests for Ingenico Connect

https://gerrit.wikimedia.org/r/423830

Change 423830 abandoned by Ejegg:

[mediawiki/extensions/DonationInterface@master] WIP: bad code retry tests for Ingenico Connect

Reason:

Getting rid of ingenico

https://gerrit.wikimedia.org/r/423830