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 Paypal making requests using POST instead of GET. #56

Closed
wants to merge 44 commits into from
Closed

Fix Paypal making requests using POST instead of GET. #56

wants to merge 44 commits into from

Conversation

dhontecillas
Copy link
Contributor

While I was testing using the Paypal sandbox I found I was getting POST request instead for GET for its notifications. I modified the code in a way that existing code won't break, but it will also handle the POST method.
Hope it gets merged.
Thanks.

@spookylukey
Copy link
Owner

This needs a unit test before it can be merged.

@dhontecillas
Copy link
Contributor Author

I will try to put a test this weekend.

@dhontecillas
Copy link
Contributor Author

I've enabled the PDT tests again in my fork. One of them is failing, and I guess should be fixed before I put tests for the POST method.
I've put the PAYPAL_IDENTITY_TOKEN in a separate file, to be sourced from bash (and on the runtests.py it reads it from the environment).

@spookylukey
Copy link
Owner

I had already re-enabled PDT tests in master, and done some fixes so that they all run and pass. From what I can see, the existing tests didn't need PAYPAL_IDENTITY_TOKEN to be a correct token.

It's not going to be practical for the test suite to depend on tokens that are not stored in the repo - it will severely hamper development if we have to depend on one particular person running the tests and reporting back to us.

@dhontecillas
Copy link
Contributor Author

I've made a new pull request, with the fixes. Hope it is all ok.

@dhontecillas
Copy link
Contributor Author

After messing it for a while yesterday night, I finally made the fix, and i've put the tests. It is in a new pull request. Shall I close this one?

@spookylukey
Copy link
Owner

Closed in favour of #68

@spookylukey spookylukey closed this Feb 1, 2017
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

2 participants