-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
product should not be priceless #11863
product should not be priceless #11863
Conversation
267f34b
to
103621b
Compare
@abdellani I know this is still work in progress but I need to ask one question: Will this PR prevent having any products/variants with price = 0? @openfoodfoundation/train-drivers-product-owners I think there are use cases for free products, like some giveaways or whatever. |
that's what I understood from the issue description. I don't think it'll make sense to prevent creating a product with a price=0, without preventing the variant's price from being 0. |
@abdellani I've just reproduced it and the problem in shopfronts happens when price is empty, not when price is equal to 0. Can we prevent empty prices and keep prices = to 0 ? Basically it's about having prices as mandatory fields. |
with redirect_to, the validation errors will be lost at the rendering time.
103621b
to
a13e087
Compare
813f3de
to
da90032
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good solution. I guess, as alternative, we could have added a presence validation on prices. But this should work for now as well.
Enables/disables localization to make the scenario from openfoodfoundation#11863 fail/pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one 👍
Hey @abdellani , Thanks for this fix 💪 I've tested in staging-UK with the localization option enabled (as described in this comment): It introduces a validation on
Other scenarios were also verified to allow for zero priced products. As a quick double check disabled the localization option, which seems to not trigger the bug: all good as expected. Merging! |
Enables/disables localization to make the scenario from openfoodfoundation#11863 fail/pass
I see that 'technical changes only' was ticked, but the label hasn't been applied. I think this is a user-facing bug so will leave it unlabelled (therefore "user-facing changes"). |
What? Why?
What should we test?
4 things:
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates