django-oscar / django-oscar-adyen Goto Github PK

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!

I was reading the documentation for this and I'm confused on how to use this with Oscar. I've completed the configuration steps, but no idea where to go from there. Are there any examples out there on how to integrate this with a new Oscar project?

Also it this still actively maintained or should I be using something else?

We recently chatted about an Oscar-friendly way to expose the payment method that a user selected on Adyen. It's not pressing for us to implement, so I'm recording our findings in this issue.

Looking at payment.Source and payment.SourceType, it looks entirely reasonable to me that django-oscar-adyen is allowed to get_or_create an Adyen source, and create a SourceType instance for each payment when recording successful payments.

As we already record the Adyen transaction ID in PaymentEvent, I would feel comfortable
recording the payment method in payment.Source.reference. Sounds sane, and it's not unlikely that was the intention of the (rather vaguely defined) field.

I was looking for a nice Oscar-Adyen integration, and now I'm deciding between this one and https://github.com/machtfit/adyen Sadly I'm not able to see through this code where the integration from handle_payment() should start, and where it actually calls the Adyen webservices. Could you clarify that for me? Thanks in advance!

I was wondering if this pacakges supports recurring payments via Adyen?

The issuer_id is not always required by adyen when brand_code is used.
Therefore the issuer id shouldn't be required in scaffold.py when a brand code is used.

Hey all -- not sure if this is the right spot for this.

I'm looking into django-oscar, and getting set up with adyen. Both look very promising, but I'm spinning my wheels a bit. I have followed the installation steps for django-oscar (I think), as well as django-oscar-adyen.
Currently getting the following error when I try to run manage.py makemigrations (or any manage.py command, I suppose):

Feb 12 03:40:21 myproject gunicorn[12456]:     "Couldn't find an Oscar app to import %s from" % module_label)
Feb 12 03:40:21 myproject gunicorn[12456]: oscar.core.exceptions.AppNotFoundError: Couldn't find an Oscar app to import adyen.gateway from

There's a strong possibility that I"m doing things wrong, but thought I'd reach out. Thanks!

I'm trying to implement notification handling using the Adyen server-to-server communication (i.e. not the redirect feedback from the browser at the time of payment - that's working fine). handle_payment_notification is throwing an exception about a missing currency field for every test notification I post from the Adyen CA.

Here're some examples of the notification data being POSTed from Adyen:

{"live":"false","notificationItems":[{"NotificationRequestItem":{"amount":{"currency":"GBP","value":35323},"eventCode":"MANUAL_REVIEW_REJECT","eventDate":"2015-12-12T07:51:56+01:00","merchantAccountCode":"XYZ","merchantReference":"itIDu6_-U0J6xTaK","pspReference":"jJAGY9CPs01Pl","success":"false"}}]}

{"live":"false","notificationItems":[{"NotificationRequestItem":{"additionalData":{"cardSummary":"7777","shopperIP":"127.0.0.1","totalFraudScore":"10","expiryDate":"12\/2012","billingAddress.street":"Nieuwezijds Voorburgwal","cardBin":"976543","extraCostsValue":"101","billingAddress.city":"Amsterdam","threeDAuthenticated":"false","alias":"H934380689410347","shopperInteraction":null,"paymentMethodVariant":"visa","billingAddress.country":"NL","fraudCheck-6-ShopperIpUsage":"10"," NAME1 ":"VALUE1","authCode":"1234","cardHolderName":"J. De Tester","threeDOffered":"false","billingAddress.houseNumberOrName":"21 - 5","threeDOfferedResponse":"N\/A","NAME2":"  VALUE2  ","billingAddress.postalCode":"1012RC","issuerCountry":"unknown","threeDAuthenticatedResponse":"N\/A","aliasType":"Default","extraCostsCurrency":"EUR"},"amount":{"currency":"EUR","value":10100},"eventCode":"AUTHORISATION","eventDate":"2015-12-15T08:48:56+01:00","merchantAccountCode":"XYZ","merchantReference":"8313842560770001","operations":["CANCEL","CAPTURE","REFUND"],"paymentMethod":"visa","pspReference":"test_AUTHORISATION_1","reason":"1234:7777:12\/2012","success":"true"}}]}

{"live":"false","notificationItems":[{"NotificationRequestItem":{"amount":{"currency":"EUR","value":1100},"eventCode":"PENDING","eventDate":"2015-12-15T09:18:00+01:00","merchantAccountCode":"XYZ","merchantReference":"testMerchantRef1","paymentMethod":"paypal","pspReference":"8313842560770001","success":"true"}}]}

Am I missing something, or is django-oscar-adyen not meant to be handling these notifications? The code mentions something about client code having to handle exceptions about missing fields and so on, but I'm not certain what I'm expected to do at this point...

After using get_class a bit into the plugin, I think something doesn't work well: get_class, from Django Oscar, look at oscar.apps.* package first. If plugin users try to extend adyen in their project (say in myproject.apps.adyen), it does not work well:

• Creating only myproject.apps.adyen.scaffold is not enough, because calls to get_class will raise import error because there is no such thing as oscar.apps.adyen,
• Plugin users will need to also create myproject.apps.adyen.facade and so on. If new sub-module are added to adyen, plugin user's project will be broken with each new update.

Solutions

Plan A: write custom get_class

The first option would be to write a custom get_class, say in adyen.utils.get_class. I'm sure we can reuse parts of oscar.core.loadings functions, but that still a lot of works (more tests, more possible bugs...).

On the other hand, that would be quite convenient since get_class is a widely used in Django Oscar. This might be more convenient for Django Oscar users, since they would be used to it already.

Plan B: write build_FOO_BAR methods on Scaffold, Facade and Gateway

Something I already tried to do in recent commit such as 4c28ea6 to allow one to build the PaymentReturn and PaymentNotification handlers without overriding more than 1 or 2 methods.

It would mean to add at least Scaffold.build_facade and Facade.build_gateway, to make sure one can simply override few classes. That might be enough for most case.

What to do now?

I can not take a decision right now, I need to think a bit more about that first. Both solution are interesting, but both come with costs.