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

Grab basket object from build_submission(), not from request #121

Closed
wants to merge 1 commit into from

Conversation

fghaas
Copy link
Contributor

@fghaas fghaas commented Jan 10, 2016

When the RedirectView is used in combination with another
CheckoutSessionMixin defined for a DeferredTax taxation strategy, then
submission's basket object has critical tax information, whereas
request.basket does not. Change RedirectView's basket reference to
point to build_submission()['basket'].

Fixes #98, reported by Nigel Fletton.

@maiksprenger
Copy link
Member

@fghaas thank you for your excellent PR. I would merge it as-is if the tests wouldn't fail. The failure seems unrelated to your work.
But I'm afraid I'm quite swamped with work for this month. I might not get to look at it before. But I presume you can just run against your own branch, right?

@fghaas
Copy link
Contributor Author

fghaas commented Jan 11, 2016

@maikhoepfel The PR is hardly my work at all; all credit should go to @nfletton.

Would you mind explaining what you mean by running against my own branch? Run what, exactly?

@maiksprenger
Copy link
Member

@fghaas Sorry for not being clear enough. I suppose what I was asking is how urgent merging your PR is. I assumed that in your project, instead of adding django-oscar-paypal as a requirement, you would point pip to the branch you used to make this pull request. That way, you would already run with your changes and not depend on us to merge it.
If this isn't the case, let me know and I'll see what I can do.

@fghaas
Copy link
Contributor Author

fghaas commented Jan 11, 2016

@maikhoepfel As it turns out, django-compressor removed support for Django 1.7 before their recent 2.0 release (see django-compressor/django-compressor@e27fce7). I don't know where exactly this dependency gets pulled in, but if you want the tests to complete on Django 1.7, it would appear that you would have to specify an explicit dependency on django-compressor 1.6.

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

Blocked by #122.

When the RedirectView is used in combination with another
CheckoutSessionMixin defined for a DeferredTax taxation strategy, then
submission's basket object has critical tax information, whereas
request.basket does not. Change RedirectView's basket reference to
point to build_submission()['basket'].

Fixes django-oscar#98, reported by Nigel Fletton.
@maiksprenger
Copy link
Member

Cherry-picked in be89e19. Thank you for all your work on this!

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

Ah, ok. I had just rebased my branch on master and initiated another Travis build. Never mind then. :)

@maiksprenger
Copy link
Member

Haha, you're too quick. Do you need a release?

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

You may want to look into #124 before you consider cutting a release. :)

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

OK if everything is indeed fine with the Travis build, then yes a release would be great. Thanks!

@maiksprenger
Copy link
Member

@fghaas Just pushed a release. Thank you for your work on this!

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

@maikhoepfel While this did fix the original issue (meaning taxes are applied to the total amount before sending the transaction off to PayPal), I'm still struggling with the taxes getting lost from the order after PayPal payment has been authorized. I'm currently trying to find out whether it's my CheckoutSessionMixin doing this, or whether something is still broken in the PayPal module.

@fghaas
Copy link
Contributor Author

fghaas commented Jan 12, 2016

Also, cancelling a PayPal order leaves the basket behind empty, as if the order actually went through; that's not exactly desirable either.

Edit: I should clarify: cancelling the order in PayPal works fine. Cancelling the order in preview is what leaves the basket empty. Not sure if that's desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants