Cleanup tests, add support for error and pending payment statuses #20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As I like to keep things simple, I removed one layer of inheritance. The test case was pretty useless, as it mostly served to set settings that we could set like any other Django setting already.
Conceptually, that's a totally different phase of the Adyen flow. As they're a bit noisy, I've moved them to their own module.
The response tests deserved their own module. And with that, the remaining tests were only testing payment notifications, so the previously catch-all 'test_adyen' module was renamed.
It was easy to make those tests a bit easier to read, so I did. Also removed a switch based on the Python version, because we only support Python 3 anyway.
Woops, I called responses what are really redirects; the class is called PaymentRedirection after all.
It only added an unneeded level of redirection and was only used when being inherited from again by FormRequest. It might make sense to factor it out later when adding another request class, but as it stands, it just makes things harder to understand.
This commit moves some duplicate attributes to the common BaseInteraction and removes one level of inheritance between PaymentFormRequest and BaseInteraction.
This commit * switches to using a MockRequest class instead of passing around weird state on self * moves one-time-only data into the individual tests * creates a dedicated module for unit tests * uses py.test syntax and Django foo to make tests more readable
I was bothered by params.get(self.HASH_FIELD) because I'd expect the hash field to always be present. But I realized it's better to allow it not being present, but loudly raising an InvalidTransactionException, irrespective whehter self.hash() returns anything or not (which it might not do accidentally). This is probably minor nitpicking, but better be safe than sorry!
The point of this PR really is to address a bug with ERROR notifications from Adyen. Oscaro employees can check Sentry: http://sentry-prod.oscaro.com/oshop/oshop-prod/group/1310/events/3457809/ The bug is two-fold. Error messages were forgotten in the status map. Note that PAYMENT_RESULT_ERROR is already present in the codebase. The other issue was that the lookup in ADYEN_TO_COMMON_STATUSES was implemented to be forgiving (using get()). Hence the traceback above is somewhat subtle; it lead to status being returned as None.
As documented, Adyen may return a payment status of pending. This commit adds support of it. As the job of django-oscar-adyen is merely to bubble that data up to the Scaffold, the change is pretty simple.
This PR improved upon the tests, but there's always work left to do. This commits adds a few of my ramblings about how we could improve further.
maiksprenger
force-pushed
the
feature/error-notifications
branch
from
f08e7d8
to
76a1b31
Compare
|
I just verified locally that with this code, |
|
Looks good. Just one comment. |
maiksprenger
added a commit
that referenced
this pull request
Oct 7, 2015
Cleanup tests, add support for error and pending payment statuses
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We ran into a bug where a payment status of
ERRORwas not handled bydjango-oscar-adyen, and we realized that a payment status ofPENDINGalso 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.