Kind of a trivial fix. Nginx forwards the origin IP address in the X-Forwarded-For HTTP header, so we need to use this one when behind it.

@twidi, care to review? :smile:

Adyen has a notification test that one can trigger from their control panel. It's useful for debugging connection problems between our servers and theirs.

This commit adds support for them, in the sense that it doesn't try to process them. The commit also moves payment notification tests from the PaymentResponseTestCase, and splits them into individual tests.

The goal of this PR is to allow altering Adyen config values based on the request. That is needed to e.g. switch the identifier if a shop serves different domains. I couldn't help myself and cleaned up a bit along the way.

This PR does mean interface changes for the Scaffold. Given that suspect this extension gets close to no outside users, I have not made any arrangements for backwards-compatibility. The changes should be easy to implement anyway and are documented.

I accidentally based my PR against master. I'm not sure if it makes much of a difference.

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!

This PR allows one to configure the name of the HTTP header in which the IP address from which a payment originated can be found.

The canonical default of 'REMOTE_ADDR' is assumed in case there is no ADYEN_IP_ADDRESS_HTTP_HEADER setting specified in the Django project — which should be the case if any component of the HTTP stack transfers this information in another HTTP header (often, but not always 'HTTP_X_FORWARDED_FOR').

This is the first iteration of the Adyen payment backend for Oscar. It still needs documentation, which I am going to improve during the day, but I figure it's worth starting the review ASAP.

As for the other backends, it follows the scaffold —> facade —> gateway pattern.

Some of it might seem overengineered right now but this is to prepare a future, fuller implementation.

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...

We ran into a bug where a payment status of ERROR was not handled by django-oscar-adyen, and we realized that a payment status of PENDING also isn't supported. To properly test for this, I went on a bit of a rampage to first clean up tests and a bit of actual code. The only "real" changes should be the two "Handle XX status notifications from Adyen" commits.

Reviewing commit by commit is recommended.

When we migrated our Adyen account to the new 'systems communications' interface, we started getting new data, which caused 500s. As all of it comes in the additionalData.KEY = VALUE format and we don't need it, we just blanket ignore it for now.

'pypip.in' remains unavailable for several months now:

• https://github.com/badges/pypipins/issues/37
• https://github.com/badges/pypipins/issues/38
• https://github.com/badges/pypipins/issues/39

Too bad… Let's use 'shields.io' instead (which has now badges like python versions and current version on PyPI).

Bumps django from 1.8.4 to 1.11.28.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

There are small typos in:

• README.rst
• adyen/signers.py
• tests/test_config.py

Fixes:

• Should read implemented rather than implementd.
• Should read config rather than confic.
• Should read carefully rather than carefuly.
• Should read backward rather than backwrd.
• Should read another rather than anoter.

Semi-automated pull request generated by https://github.com/timgates42/meticulous/blob/master/docs/NOTE.md

get_form_action() and get_form_fields() does not contain any parameter to pass when we initialise the class we have to pass the order_data in contractor

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!

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 4c28ea62685fba3cc7f2078e711508bec646fe21 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.

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!