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

Regression: price_excl_tax is necessary for availability #2664

Closed
maiksprenger opened this issue Apr 3, 2018 · 3 comments
Closed

Regression: price_excl_tax is necessary for availability #2664

maiksprenger opened this issue Apr 3, 2018 · 3 comments
Labels

Comments

@maiksprenger
Copy link
Member

I get to upgrade an old Oscar project, and that went really well. Thanks for keeping Oscar so backwards-compatible! I noticed just a small regression for a corner case:

I work on a project where it was essential that Oscar stores the tax-inclusive price and the price without tax is calculated. The reasoning is that the tax-inclusive prices are legally binding, and doing it this way ensures that a bug in the price calculation logic has less of an impact.

This was easily done by storing the price_incl_tax on the stock record, and writing a short pricing policy for it. But two recent changes here and here make this harder, because they look at a price_excl_tax and assume because that is not set, the product is not available to buy.

I'd like to argue that pricing and availability are concepts that should be kept separate, and the two commits linked above essentially bypass Oscar's pricing logic.

My workaround was to backport the StockRequired policy from Oscar 1.4.

@solarissmoke solarissmoke changed the title Regression: price_excl_tax is necessary for Regression: price_excl_tax is necessary for availability Apr 4, 2018
@solarissmoke
Copy link
Member

@maikhoepfel I think you're right that pricing and availability should be kept separate.

The issue that those commits were trying to fix was an exception if you try to add a product without price_excl_tax to the basket. How do you handle that issue in your case?

@maiksprenger
Copy link
Member Author

maiksprenger commented Apr 5, 2018 via email

@solarissmoke
Copy link
Member

Proposed fix here: #2679.

I have only reverted the changes in #1913 and offered a different way to handle the issue that it was trying to fix.

#2294 seems to be correct to me in the sense that that change was to pricing policy rather than availability policy - and for FixedRateTax it makes sense to return UnavailablePricing if price_excl_tax is not set - if you don't know it then you need to use a different strategy.

MrVoltz pushed a commit to EndevelCZ/django-oscar that referenced this issue Aug 21, 2018
Revert changes in django-oscar#1913 and django-oscar#2294 that required a stock record to have price_excl_tax set in order to report the product as available.

Add separate checks to the basket form/add logic that checks at the time of
adding a product to the basket whether a price exists, and report an error if it doesn't.

Fixes django-oscar#2664.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants