-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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][MIG] sale promotion rule #1583
Conversation
@hparfr IMHO, either a Roadmap, either a modification before merge should integrate coupon module that has been split by Odoo from 13 => 14 as it could be interesting to base our flow on coupon.coupon model https:/odoo/odoo/blob/14.0/addons/coupon/models/coupon.py#L10 Not too much logic there, no mandatory fields. What do you think ? |
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.
Thanks for the PR. There are minor improvements not required to merge this.
<group name="condition" string="Condition" col="3"> | ||
<label for="date_from" string="Date début/fin" /> | ||
<field name="date_from" nolabel="1" /> | ||
<field name="date_to" nolabel="1" /> | ||
<field name="minimal_amount" colspan="3" /> | ||
<field name="is_minimal_amount_tax_incl" colspan="3" /> | ||
<field name="only_newsletter" colspan="3" /> | ||
<field | ||
name="restrict_pricelist_ids" | ||
widget="many2many_tags" | ||
colspan="3" | ||
/> | ||
<label for="restrict_partner_ids" colspan="3" /> | ||
<field | ||
name="restrict_partner_ids" | ||
nolabel="1" | ||
colspan="3" | ||
/> | ||
</group> | ||
</group> |
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.
</group>
<group name="condition" string="Condition" col="6">
<label for="date_from" string="Date début/fin"/>
<field name="date_from" nolabel="1"/>
<field name="date_to" nolabel="1"/>
<field name="minimal_amount" colspan="3"/>
<field name="is_minimal_amount_tax_incl" colspan="3"/>
<field name="only_newsletter" colspan="3"/>
<field name="restrict_pricelist_ids" widget="many2many_tags" colspan="3"/>
<label for="restrict_partner_ids" colspan="6"/>
<field name="restrict_partner_ids" nolabel="1" colspan="6"/>
</group>
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.
@hailangvn This is reformated through pre-commit.
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.
Thanks @rousseldenis. I was new at the time I commented. :)
<?xml version="1.0" encoding="UTF-8" ?> | ||
<odoo> | ||
<record id="coupon" model="product.product"> | ||
<field name="name">Coupon</field> | ||
<field name="type">service</field> | ||
</record> | ||
</odoo> |
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.
Is this needed with current coupon logic?
Why not. I was not aware of this coupon module. |
My comment was about demo coupon product, which IMHO helps new user to understand this module. I was wondering what to do next after having a coupon product while we already added coupon code to promotions page. There is another demo file as well. |
<field name="multi_rule_strategy" /> | ||
</group> | ||
<group name="condition" string="Condition" col="3"> | ||
<label for="date_from" string="Date début/fin" /> |
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.
Could you change this to English version ?
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.
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.
ping @xavier-bouquiaux
8b6b6d1
to
4c95642
Compare
@xavier-bouquiaux @rousseldenis wouldn't it make more sense to move this module to sale-promotion repo? let us know if you need us to work on it |
or rather: what are the differences between this module's promotions and odoo standard promotions? should they coexist? |
After few months of implementation with coupons in v14, it's missing key features for our e-commerce case (like apply more than 1 promotion at time). Coupon is not designed to allow behavior changes. So it's a bit hacky to add features on top of it. We spent a lot of effort to make it work. Sale promotion rule, is an old module date here from 2017 and I think it came from a time when we had to synchronize with Magento / Prestashop and cover main features. It's designed and implemented to allow behavior changes if needed. I think a good refactoring will not hurt. Like always, it depends of your needs and your budget. |
@hparfr But from 13 version, it is possible to use coupon for its model only and build on top of it as we did here : https:/acsone/acsone-addons/tree/14.0/account_wallet_coupon |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
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.
LGTM (code review), we should merge it even odoo have an alternative implementation (coupon do not solve all case). In a long terme we should see if we can use coupon or improve this module.
For now some customer need such feature so let's merge it
<field name="multi_rule_strategy" /> | ||
</group> | ||
<group name="condition" string="Condition" col="3"> | ||
<label for="date_from" string="Date début/fin" /> |
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.
ping @xavier-bouquiaux
The module now supports multiple promotion rules. When a coupon is used, it always take precedence on automatic promotion rule. The computation of promotions has been refactored to update lines from the sale.order object. Sine lines are updated from the SO, the recompute of SO fields that depends of lines is only tiggered at the end of the update on the lines. (In place of each time a line is updated). You can ask to recompute the promotion of a sale.order. If a coupon is already specified it's conserved but all the automatic rules are applied again.
If the coupon rule apply a discount in fixed price, the rule should not be applied on each sale order line
Sequence order was never applied du of missing _order [UPD] Update sale_promotion_rule.pot
Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-workflow-10.0/sale-workflow-10.0-sale_promotion_rule Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-10-0/sale-workflow-10-0-sale_promotion_rule/ [UPD] Update sale_promotion_rule.pot Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-workflow-10.0/sale-workflow-10.0-sale_promotion_rule Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-10-0/sale-workflow-10-0-sale_promotion_rule/ Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-workflow-10.0/sale-workflow-10.0-sale_promotion_rule Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-10-0/sale-workflow-10-0-sale_promotion_rule/
If no rule is percentage (i.e. edit discount field) we should not clear the discount when removing promotions.
Fix tests using demo data modified by other modules
4c95642
to
9390ffd
Compare
Changed to english version and rebased regards xbo |
/ocabot merge patch |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at c9dc6ab. Thanks a lot for contributing to OCA. ❤️ |
No description provided.