spookylukey / django-paypal

A pluggable Django application for integrating PayPal Payments Standard or Payments Pro
MIT License
729 stars 208 forks source link

Fix encrypted buttons under Python 3 #199

Closed JonathanRoach closed 6 years ago

JonathanRoach commented 6 years ago

Avoid the b'' decoration under Python 3 when mixing binary (encrypted button contents) and normal strings. Fix the examples in the documentation for encrypted buttons to use the encrypted form.

spookylukey commented 6 years ago

Thanks so much for the PR! This code needs tests (tests that fail without the patch) in order to be merged.

JonathanRoach commented 6 years ago

Hi Luke,

Sorry, I’m not ignoring you - I’m just prioritising finishing some other things off first.

Jon

On 14 Feb 2018, at 16:53, Luke Plant notifications@github.com wrote:

Thanks so much for the PR! This code needs tests (tests that fail without the patch) in order to be merged.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spookylukey/django-paypal/pull/199#issuecomment-365671360, or mute the thread https://github.com/notifications/unsubscribe-auth/AWPEgsXqkpQEq4ZrhRtzvhBZ0FxHo4DXks5tUw9zgaJpZM4SFDDH.

spookylukey commented 6 years ago

BTW, there is no need to bump version numbers, I'll do that when I release.

JonathanRoach commented 6 years ago

Test written (bug fixed - oops), doc updated (M2Crypto is now Python3 capable), and .DS_Store added to .gitignore (for Mac users).

spookylukey commented 6 years ago

Would you be able to fix the merge conflicts? It looks the main problem is the version number bumps - just drop all your changes in that regard, and put docs the "under development" heading. Thanks!

mmic commented 6 years ago

I attest the issue. PayPal checkouts would have stopped working after migrating our codebase to python 3. How can we contribute tests to have this patch merged?

Regarding the conflict, notice that there's no conflicts relevant to the patch. At the core, the patch is a one-liner: adding ".decode()" (not .encode() !) to paypal/standard/forms.py 's return value of the _encrypt() function.

JonathanRoach commented 6 years ago

I think I've adjusted the notes & versioning to merge correctly now.

mmic commented 6 years ago

Careful, the original patch is incorrect. ".decode()" should be appended, not ".encode()".

JonathanRoach commented 6 years ago

The revision 'Add test for encrypted buttons' includes the correction to .decode() - tests are a good thing!

JonathanRoach commented 6 years ago

The automated tests aren’t working - m2crypto needs installing. Is the correct fix to add:

Thanks, Jon

spookylukey commented 6 years ago

@JonathanRoach - you should instead modify tox.ini to add the dependency under the deps = section. You can test locally by doing something like:

$ pip install tox
$ tox -e py27-django111-tests-usetztrue -e py35-django200-tests-usetztrue

(i.e. one Python 2 and one Python 3 environment).

I normally use exact versions in tox.ini (e.g. m2crypto==0.29 rather than m2crypto>=0.29), so that we don't get unexpected failures on Travis caused by some new incompatibility in a recent version of something.

spookylukey commented 6 years ago

Looks good - thanks so much!