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

[17.0][MIG] sale_coupon_criteria_order_based #228

Open
wants to merge 17 commits into
base: 17.0
Choose a base branch
from

Conversation

mmequignon
Copy link
Member

No description provided.

diga and others added 16 commits October 21, 2024 09:58
Currently translated at 100.0% (10 of 10 strings)

Translation: sale-promotion-14.0/sale-promotion-14.0-sale_coupon_criteria_order_based
Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-14-0/sale-promotion-14-0-sale_coupon_criteria_order_based/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: sale-promotion-15.0/sale-promotion-15.0-sale_coupon_criteria_order_based
Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-15-0/sale-promotion-15-0-sale_coupon_criteria_order_based/
Currently translated at 85.7% (6 of 7 strings)

Translation: sale-promotion-15.0/sale-promotion-15.0-sale_coupon_criteria_order_based
Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-15-0/sale-promotion-15-0-sale_coupon_criteria_order_based/it/
Currently translated at 100.0% (7 of 7 strings)

Translation: sale-promotion-15.0/sale-promotion-15.0-sale_coupon_criteria_order_based
Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-15-0/sale-promotion-15-0-sale_coupon_criteria_order_based/es/
…tests

This test requires a CoA to be installed, it should be tagged "post_install"

TT46134
@mmequignon
Copy link
Member Author

ping @francesco-ooops @Ivorra78 @pilarvargas-tecnativa
could you please drop a review ?
Thanks!

Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please check whether documentation is still accurate (eg: there's no "Sales/Products/Promotion Programs menu" in v17)

  • Domain button is not visible:

image

  • Also, is it correct that "Based on order" domain is both visible in promotion form and "conditional rules" popup?

@mmequignon
Copy link
Member Author

  • Please check whether documentation is still accurate (eg: there's no "Sales/Products/Promotion Programs menu" in v17)

You're right, I totally forgot to update the doc

* Domain button is not visible:

Yes, adding it right now

image

* Also, is it correct that "Based on order" domain is both visible in promotion form and "conditional rules" popup?

Yes, and they are evaluated sequencially, which allows to have more complex rules.
1 domain which is common to all rules set on the program.
1 domain on the rule itself, which is specific to it.
With that we can imagine for a given program, the order should always be draft, 1 rule requires 10$ spent, the other a specific product to trigger the access to the reward.

@francesco-ooops
Copy link
Contributor

@mmequignon that's good, what about adding to the doc also this explanation about the presence of the field in both the promo and the condition rule to make more explicit how it's supposed to work?

@mmequignon mmequignon force-pushed the 17.0-mig-sale_coupon_criteria_order_based branch from ba0087c to 2726966 Compare October 28, 2024 13:47
@mmequignon
Copy link
Member Author

@francesco-ooops I updated the PR, can you please make another pass ?
Thanks !

@francesco-ooops
Copy link
Contributor

@mmequignon ping me after tests are fixed :)

@mmequignon mmequignon force-pushed the 17.0-mig-sale_coupon_criteria_order_based branch from 2726966 to a5fc044 Compare October 28, 2024 14:15
@mmequignon
Copy link
Member Author

@mmequignon ping me after tests are fixed :)

@francesco-ooops done

@francesco-ooops
Copy link
Contributor

@mmequignon mmm I don't see the field

image

@mmequignon
Copy link
Member Author

mmequignon commented Oct 28, 2024

@mmequignon mmm I don't see the field

image

That is because I added base.group_no_one (as it is done on product_domain) on the field.
you have to be have debug with assets enabled to see it

@francesco-ooops
Copy link
Contributor

Well,

  1. why?

  2. that's the kind of stuff that makes functionals go to devs and ask "why don't I see this field?" and after their reply, "why isn't that in the documentation?"

I don't want to be rude, but our time is valuable too :)

@mmequignon
Copy link
Member Author

Well,

1. why?

2. that's the kind of stuff that makes functionals go to devs and ask "why don't I see this field?" and after their reply, "why isn't that in the documentation?"

I don't want to be rude, but our time is valuable too :)

I see your point, I just thought it would be better to stick to what was done for product_domain.
I'll drop this groups attribute.

@mmequignon mmequignon force-pushed the 17.0-mig-sale_coupon_criteria_order_based branch from a5fc044 to 1a29aa7 Compare October 28, 2024 14:44
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional ok!

@francesco-ooops
Copy link
Contributor

just a suggestion: UX-wise, it would be useful if domain condition would be displayed in the "condition" card, in order to make all the rules explicit (as it happens for other fields)

image

@mmequignon
Copy link
Member Author

just a suggestion: UX-wise, it would be useful if domain condition would be displayed in the "condition" card, in order to make all the rules explicit (as it happens for other fields)

image

I'll add this to the ROADMAP

Copy link

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The criteria cannot be applied at rule level and at program level. You have to decide at which level it should be applied and follow the odoo flow to apply a promotion and respect the changes that have been made since version 16.0 to adapt the promotion modules. You can check the 16.0 branch and see how other modules have been migrated for reference.

class LoyaltyProgram(models.Model):
_inherit = "loyalty.program"

rule_order_domain = fields.Char(string="Based on Order", default="[]")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are you going to apply the criteria? At the rule level or at the program level?

class LoyaltyRule(models.Model):
_inherit = "loyalty.rule"

rule_order_domain = fields.Char(string="Based on Order", default="[]")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are you going to apply the criteria? At the rule level or at the program level?

class LoyaltyProgram(models.Model):
_inherit = "loyalty.program"

rule_order_domain = fields.Char(string="Based on Order", default="[]")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to use a M2o on ir.filter in order to make it reusable between different programs?

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.

7 participants