How to handle merchantReturnData and how to refactor the plugin

[Exirel]

Working on this plugin leads me to realize that we may not be using this field properly. As pointed out by @willharris in #25 the plugin initially use this field to store the order's amount, as a way to get this amount back at the Payment Return URL (after customers paid for their order on the HPP).

What Adyen say about this field

This field value is appended as-is to the return URL when the shopper completes, or abandons, the payment process and is redirected to your web shop.

Typically, this field is used to hold and transmit a session ID.

Maximum allowed character length: 128 characters.

And, the warning note added for this field is rather important:

If by including merchantReturnData in a request causes it to exceed the allowed maximum size, the payment can fail.

(emphasis is mine)

What we do in the code

• The amount is needed to record the transaction as an AdyenTransaction object. It has no other purpose inside the plugin.
• Outside the plugin, ie. inside the plugin user's project, the amount is easy to get by looking at the Order object.
• The amount is not given by the Payment Return URL,
• The amount is given by the Payment Notification.
• To handle merchantReturnData both the Scaffold and the Facade need to be modified. This is not ideal since it requires one to override both to add anything new.

Something that plugin users can not be aware of is that originally, this project is used by Oscaro internally, so some decision are related to Oscaro own projects. It does not mean it is a good idea to force Oscaro needs into the plugin this way. More importantly, Oscaro projects do not need the amount in the merchantReturnData any more.

These lead me to the conclusion that:

• Using the amount in the merchantReturnData was an arbitrary decision,
• This decision seems a bit outdated now,
• From this decision we have a backward compatibility issue if we want to make any modification,
• Today, I don't see why we need to record AdyenTransaction outside a Payment Notification.
• Until now, the plugin did not expose a way to handle separately Payment Return and Payment Notification. This is modified by recent commit for 0.6.0, so this might be a way to handle things more easily.

What to do now?

First, I would like to address the "where to handle merchantReturnData" problem: can the AdyenConfig object handle that? Or provide an handler class for it? Like, a MerchantReturnDataParser or something? I'm not sure about that yet. Maybe just CustomDataWriter and CustomerDataReader.

In #25 @willharris suggest that the plugin should handle a plugin-specific format. I'm not really happy with that, because I don't see why one would need to pass more than an identifier here (like a session, or a tracking ID, or else). The amount stored here sound like a bad idea to me.

I'm a bit worried that one may not be aware of the max of 128 characters length, and add arbitrary data without the plugin complaining. Or even worse, if, say, plugin users provide exactly 128 characters, and because we still keep the amount, this explode the limit and make payment failed.

Second, I want to keep the backward compatibility at least for 0.6.0, then maybe in 0.7.0, and drop it eventually in 0.8.0. I think there is more to do with how the plugin handle Payment Return and Payment Notification.

Another idea: set a flag on the AdyenConfig to say "let me handle merchantReturnData myself", or something like that. Somehow, this would be the same as using a settings to get a custom handler, with a default handler provided directly by the plugin.

Right now, I can not take a decision, but writing this down help me a bit, and I'm sure other developers will have ideas/comments to add. Feel free to do so!