Allow client code to use the Adyen merchantReturnData field.

[willharris]

The merchantReturnData field was blocked for use in client code, as the module used it for its own purpose in storing the transaction amount value (see the TODO at facade.py:85). This PR allows client code to also use this field by providing a merchant_return_data value in the order details, while preserving the internal use.

If any client data is provided for this field, it is appended to the amount value (now converted to a string) and separated by a tilde (~) character, which is valid in a URL without encoding, and highly unlikely to appear in any client data (e.g. it normally wouldn't appear in a Django session ID, which is what I need it for, and also what Adyen says is the typical usage for the field). Any provided data is first encoded as UTF-8 and then urlencoded for safe transmission as a URL param.

During the postback detail unpacking, the merchantReturnData field is split on the tilde character (if it is present), and the decoded data is returned to the client code as details['merchant_return_data'].

As with the other PR, this one is based on the current master, and will require a minor fix to work with my other Python 2 PR - which I also hope you merge! Happy to rebase after...

[Exirel]

Hi @willharris and first, thank you for your interest and your work on that. I apologise for the lack of follow-up from us. We struggle to maintain this project, and I didn't have time to get into this project before this week.

I think I see your concern about the merchantReturnData. I had a look at the current code, and I saw that we, indeed, try to record an AdyenTransaction using this amount. I think it's acceptable to use this field for whatever reasons developers want.

That is to say, we need to address:

1. This plugin wants to get an amount from merchantReturnData,
2. Developers want to add more than the amount in merchantReturnData.

Instead of adding ad-hoc code inside the plugin, I would rather keep the current behavior, but allow a hook that would:

1. Allow one to set the value of merchantReturnData (say, a method to do that),
2. Allow one to extract the value of merchantReturnData (say, another method to do that).

It would work with a simple contract: the plugin gives input data to the method, the method must return the submission data (ie. the form fields that will be sent to Adyen). Some fields are required (say, the amount, the order number, etc.). On the other end, a method will get the Adyen response with all the fields, and it must return a python dict with values. Some of these values must be "amount", and for example "merchant_return_data".

I know, you did a lot of work with this PR, and you may not appreciate that I don't merge it "like that". However, I would like to find a trade-off between:

1. the time I have to work on this project myself,
2. the flexibility of the plugin,
3. your needs, and any other developer's needs,
4. a bit of backward compatibility.

Do not hesitate to get in touch with me at [email protected] which is my professional email, or here on github, through issues and PR.

Again, thanks for the PR and your interest.

[willharris]

Hey @Exirel, no worries about the delay!

It's not clear to me whether you are interested in a larger refactoring of the handling of order_data fields in the get_form_fields call, and would therefore prefer to delay (or reject) my implementation until such time as you can do such a refactoring. Is that the case?

A quick statement about terminology: for me, "the plugin" refers to django-oscar-adyen, i.e. a plugin for django-oscar. Am I understanding you correctly?

In any case, I would argue the following: currently, there is simply no way for a plugin user (i.e. someone using django-oscar-adyen in their code) to provide any kind of value for merchantReturnData, as this is simply blocked/ignored by the plugin. I think the fact that the plugin has kind of "hijacked" this field, which is meant to be used by external developers, as you note, is quite a shortcoming in the plugin, and is commented as such in _unpack_details.

What I have implemented provides that functionality for the plugin user while:

• maintaining backward compatibility. There is no change needed in anyone's code as a result of this PR, and the plugin can continue to store and retrieve the amount value in this field.
• maintaining the existing paradigm of calling get_form_fields with a simple Python dict.
• leaving the way open for a later refactoring by not introducing any additional interfaces that might then have to be broken.

In this way, the actual implementation details of how the plugin is using this field for its own purposes remain shielded from the developer, and supplying a value for merchant_return_data in the dictionary "just works" as a developer might expect from reading the Adyen documentation.

What do you think?

[Exirel]

A quick statement about terminology: for me, "the plugin" refers to django-oscar-adyen, i.e. a plugin for django-oscar. Am I understanding you correctly?

Yes! I'm sorry I didn't make it clear, because I knew it could be confusing - that's something I would like to write in the doc!

to delay (or reject) my implementation until such time as you can do such a refactoring. Is that the case?

It is yes. It makes sense that you tried to only modify as few code as possible of django-oscar-adyen with your PR. But I invite you to have a look at my recent commit in the master branch:

• 6b901b9 where I use get_class to allow one to extend only one or two components (ie. classes),
• 5328eea where I split get_form_fields, in particular adding get_field_merchant_return_data method
• be24b2e where I expose unpack_details, with other methods

I make sure it's backward compatible, and I think it'll help you implement your own version of both get_field_merchant_return_data and unpack_details.

Of course, since there is no documentation, it's not clear how one can do that safely. I know that point and I want to write documentation to clarify all these points.

I think the fact that the plugin has kind of "hijacked" this field, which is meant to be used by external developers, as you note, is quite a shortcoming in the plugin

Exactly yes! And I don't like that too. After reading carefully, it looks like we wanted to always record the result of the transaction from Adyen:

• When customers come back from Adyen HPP through a GET request ("Payment Return URL"),
• When the server receive the Adyen Notification through a POST request ("Payment Notification")

But that's another story. Please, have a look at my latest commits on master, and tell me if it can help you.

[willharris]

Hi @Exirel,

Thanks for continuing the dialog! A few comments about the commits you mentioned:

• 6b901b9 - I like this one, it does indeed make it easy to override some of the default behaviour of the plugin in a very Oscar-like way.
• be24b2e - I think it's a good idea to make unpack_details an officially public method. Together with get_class this would allow plugin developers to cleanly override the whole unpacking functionality should they so desire.
• 5328eea - I disagree with what you've done here. Having the amount value in the unpacked details is very valuable, but the way you have written it, I will have to write my own code to handle the amount cleanly should I also wish to use merchantReturnData for my own purposes. This adds more code to my own code base for no gain.

In general, I think using both amount and merchantReturnData would be a relatively common use-case for plugin users, whereas wholesale overriding of the unpack_details method is a bit of an edge case. I understand that the original authors needed to use merchantReturnData to handle the amount value, and I think that's legitimate, but I think splitting out individual fields for special handling in their own methods is over-engineering the problem and will lead to code bloat for most users.

I also think it's important that whatever the implementation does, it should not require plugin users to write code in two different places to achieve their effect. As it is now written, I will have to override both Scaffold and Facade just to use my own merchantReturnData, and ensure that I am doing the same thing in both places. This just makes the plugin harder to use.

I think to goal of the plugin should be to provide the best compatibility with the existing Adyen interface for the largest part of the userbase, while making it possible for those with special needs to do what they want with minimal fuss.

[Exirel]

I also think it's important that whatever the implementation does, it should not require plugin users to write code in two different places to achieve their effect. As it is now written, I will have to override both Scaffold and Facade just to use my own merchantReturnData, and ensure that I am doing the same thing in both places. This just makes the plugin harder to use.

Sure it is. That's not an easy refactoring indeed, and I think it is only the first step. That being said, the merchantReturnData is not an easy topic, and I would like to write an issue about it before further changes.

[willharris]

I'm not sure why you think it's a difficult topic - it is simply a user-provided string that needs to safely handled in a URL context. There are no semantics to it.