Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix origin IP address retrieval #4

Merged
merged 6 commits into from Sep 23, 2014

Conversation

metamatik
Copy link
Contributor

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

therefore configurable via the ADYEN_IP_ADDRESS_HTTP_HEADER Django
setting. We fallback on the basic `REMOTE_ADDR`one which is the
standard, unproxied, HTTP one.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring is cool, as you explained why you did that (as you said "we need"). Yet, it does not tell what the function actually did, like "return an IP address from header".

Also, maybe something more concise on the setting available, like "Header's key can be configured by the ADYEN_IP_ADDRESS_HTTP_HEADER setting, and REMOTE_ADDR is used as default.".

Don't get me wrong: I like your style and tone, but I like when I've just two lines to read to fully know what the function does, instead of a (still well written) full paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, there should be a one-line summary of what the method actually does.
Adding it.

@metamatik
Copy link
Contributor Author

Dear Creature of the Night, I have implemented thy requested changes.
May I therefore have thy Seal of Approval? 😈

@metamatik
Copy link
Contributor Author

Same question. 😄

@Exirel
Copy link
Contributor

Exirel commented Sep 23, 2014

By the great power of the Eternal Shadow, I hereby grant you the LGTM.

@metamatik metamatik merged commit 9032eaf into develop Sep 23, 2014
@wo0dyn wo0dyn deleted the feature/fix-ip-address-retrieval branch April 29, 2015 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants