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.

Sign up for GitHub

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

Jump to bottom

[16.0][FIX] loyalty_partner_applicability: Set default value for partner domain rule #186

Merged
merged 2 commits into from
Jan 19, 2024
Merged

[16.0][FIX] loyalty_partner_applicability: Set default value for partner domain rule #186

merged 2 commits into from
Jan 19, 2024

Conversation

pilarvargas-tecnativa
Copy link
Contributor

It is necessary to set the default value, in this case "[ ]" to the rule_partners_domain field, overwriting the computed method "_program_type_default_values" that sets the default values depending on the type of program, because when creating a new program, these default rules are defined and the rule_partners_domain field, not being contemplated, its value will be "False", which cannot be interpreted as a correct domain. To avoid this error, the value is added to the rules defined by default.

The error:
image

cc @Tecnativa TT44344

@chienandalu @CarlosRoca13 please review

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-fix-loyalty_partner_applicability-default_values branch from c1e136c to f8ef2f2 Compare December 1, 2023 14:45
chienandalu
chienandalu reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thinking it twice, would it be more resilient to override the create of the program.rule and simply add the default domain in the vals if not present?

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-fix-loyalty_partner_applicability-default_values branch from f8ef2f2 to 6e4683c Compare December 4, 2023 10:16
@pilarvargas-tecnativa
Copy link
Contributor Author

Thinking it twice, would it be more resilient to override the create of the program.rule and simply add the default domain in the vals if not present?

Yes, it's more resilient. Done :)

@pilarvargas-tecnativa
[FIX] loyalty_partner_applicability: Set default value for partner do…
c66d64c 
…main rule

It is necessary to establish the default value, in this case "[ ]" to the
rule_partners_domain field, in the create method for the rules established
in the computed method "_program_type_default_values" that establishes the
default values depending on the type of program, because when creating a
new program, these rules do not have the rule_partners_domain field defined,
as it is not contemplated, its value will be "False", which cannot be
interpreted as a correct domain. To avoid this error, the check is made in
the create method and the correct value is added.

TT44344
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-fix-loyalty_partner_applicability-default_values branch from 6e4683c to c66d64c Compare December 4, 2023 11:48
chienandalu
chienandalu approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

👍

@pedrobaeza pedrobaeza added this to the 16.0 milestone Dec 4, 2023
@pedrobaeza
Copy link
Member

I don't get why the default is not applied, even if coming from that "direct creation". Anyway, the way to fix this is to override _program_type_default_values, not to put this general "fallback". One can think that having this general fallback is better, but it's not because:

  1. It's called everything you create a record, draining a bit the performance. It's not too much, but for a very corner case like this, it's not needed.
  2. It can make debugging worse if we get a non-desired result, as the value assignation is not encapsulated and spread across several parts.

Anyway, I insist, why the default value is not applied? Is this bypassing the ORM somehow?

@chienandalu
Copy link
Member

Anyway, I insist, why the default value is not applied? Is this bypassing the ORM somehow?

It's bypassed: https://github.com/odoo/odoo/blob/16.0/addons/loyalty/models/loyalty_reward.py#L20

@pilarvargas-tecnativa
Copy link
Contributor Author

ping @pedrobaeza

@pedrobaeza
[IMP] sale_loyalty_partner_applicability: Don't evaluate domain if no…
b166adb 
…t set

This way, we can avoid a crash on incorrect input data.
pedrobaeza
pedrobaeza approved these changes Jan 19, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I have just added another safeguard on the sale part.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-186-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit ee4a25c into OCA:16.0 Jan 19, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5b0e946. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 16.0-fix-loyalty_partner_applicability-default_values branch January 19, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@chienandalu chienandalu chienandalu approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pilarvargas-tecnativa @pedrobaeza @chienandalu @OCA-git-bot