Skip to content

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

Closed
fghaas wants to merge 1 commit into
django-oscar:masterfrom
fghaas:issue-98
Closed

fghaas wants to merge 1 commit into
django-oscar:masterfrom
fghaas:issue-98

Conversation

@fghaas

@fghaas fghaas commented Jan 10, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

fghaas commented Jan 11, 2016

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

fghaas commented Jan 11, 2016

Copy link
Copy Markdown
Contributor Author

@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

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@fghaas

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

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

@maiksprenger

Copy link
Copy Markdown
Member

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

@fghaas

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

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

@fghaas

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

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

@maiksprenger

Copy link
Copy Markdown
Member

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

@fghaas

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

@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

fghaas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

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