Allow altering config values based on request #14
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
We will need to be able to alter settings based on e.g. the request, so as a first step, I'm introducing a layer between Django settings and the Adyen code. We introduce the concept of a config class, that can be set from the Django settings. It defaults to a supplied config class that just fetches the needed values from the Django settings as before. But now we have the possibility of switiching out the config class to fetch those values from elsewhere.
This commit adds a few basic tests to check things work as expected.
The idea is that we want to change certain configuration values based on the request. To get this working, we need to change the interface for the config class to pass the request to most of it's methods. To get the request object, we need to also alter the facade's and scaffold's interface to more often include the request. For this commit, I had a look at how data gets passed from e.g. Oscaro to the scaffold. Unfortunately I discovered an anti-pattern, where a lot of required data is passed in via an undocumented order_data dictionary. That needs to be cleaned up separately.
This commit improves upon the situation by making the dictionary explicitly required.
get_form_fields concatenated the form fields together to produce some HTML. Arguably, that's out of scope for a payment gateway. Furthermore, it's not currently used. Hence, I removed it and renamed the more useful get_form_fields_list. This allows dropping a requirement.
|
I pushed two commits to incorporate your feedback. Will rebase into semantic commits at the end. |
|
Looks good, thanks. |
This was feedback given in the review.
In the feedback round, we spotted that the tests could be improved.
maiksprenger
force-pushed
the
feature/variable-config
branch
from
7fbc18f
to
458d4c0
Compare
maiksprenger
added a commit
that referenced
this pull request
Jul 8, 2015
Allow altering config values based on request
|
Thanks for the interesting discussion! |
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.
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.