-
-
Notifications
You must be signed in to change notification settings - Fork 497
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
[14.0][IMP][FIX] website_sale_product_assortment: handle multiple assortments #838
Conversation
Hi @CarlosRoca13, |
b84e956
to
c77c157
Compare
Failing CI is due to another issue that I'm fixing here: #839 |
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.
Functional ok!
@CarlosRoca13 what do you think of this one? |
c77c157
to
9c85741
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.
Code and functional review 👍
Thanks!
assortment = assortments[0] | ||
res["message_unavailable"] = assortment.message_unavailable | ||
res["assortment_information"] = assortment.assortment_information | ||
|
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.
Please don't add empty lines inside a method.
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.
done
8c88f00
to
4213656
Compare
This commit changes the way multiple assortments are handled and fixes a couple of bugs in the process. The current version of the module is not able to handle multiple assortments, especially ones that are exclusive (e.g. one assortment with all consumable products and one with all services) and the module seemingly breaks, ignoring the selected rule (e.g. no_show) With these changes, that behaviour is fixed, and additionally assortments for the same website and partner and merged in a logical OR.
4213656
to
ddbdd42
Compare
Thanks for the fix and the regression tests: /ocabot merge patch |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 49f9443. Thanks a lot for contributing to OCA. ❤️ |
This commit changes the way multiple assortments are handled and fixes a couple of bugs in the process.
The current version of the module is not able to handle multiple assortments, especially ones that are exclusive (e.g. one assortment with all consumable products and one with all services) and the module seemingly breaks, ignoring the selected rule (e.g. no_show)
With these changes, that behaviour is fixed, and additionally assortments for the same website and partner are merged in a logical OR.