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

[FIX] expression: properly handle {TRUE,FALSE}_LEAF #31202

Closed
wants to merge 1 commit into
base: 10.0
from

Conversation

Projects
None yet
5 participants
@Elkasitu
Copy link
Contributor

Elkasitu commented Feb 18, 2019

Before this commit, doing expression.OR() with only FALSE_LEAF would
yield [] which is equivalent to TRUE_LEAF and is therefore not correct.

The same happened (to a lesser extent) with expression.AND() within an
expression.OR(), since the former would return a [] which would be
ignored by expression.OR().

See tests for a clearer view of the use cases.

Fixes #30113, #26540

@Elkasitu Elkasitu requested a review from rco-odoo Feb 18, 2019

@robodoo robodoo added the seen 🙂 label Feb 18, 2019

@C3POdoo C3POdoo added the RD label Feb 18, 2019

@robodoo robodoo added the CI 🤖 label Feb 18, 2019

@pedrobaeza
Copy link
Contributor

pedrobaeza left a comment

Thanks for handling this.

Maybe you can add a test for the expected OR combination with a false leaf plus other regular leaf:

        expr = expression.OR([fd, [('foo', '=', 'bar')]])
        self.assertEqual(expr, [('foo', '=', 'bar')])
@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

Elkasitu commented Feb 20, 2019

@pedrobaeza I thought that case would already be covered, but I guess it's not... goes to show how well expression.py is tested :^)

Thanks, I'll add it

@Elkasitu Elkasitu force-pushed the odoo-dev:10.0-fix-expression-combine-adt branch Feb 20, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 20, 2019

[FIX] expression: properly handle {TRUE,FALSE}_LEAF
Before this commit, doing expression.OR() with only FALSE_LEAF would
yield [] which is equivalent to TRUE_LEAF and is therefore not correct.

The same happened (to a lesser extent) with expression.AND() within an
expression.OR(), since the former would return a [] which would be
ignored by expression.OR().

See tests for a clearer view of the use cases.

Fixes #30113, #26540

@Elkasitu Elkasitu force-pushed the odoo-dev:10.0-fix-expression-combine-adt branch to 289f8a8 Feb 20, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 20, 2019

@rco-odoo
Copy link
Member

rco-odoo left a comment

@robodoo robodoo added the r+ 👌 label Feb 26, 2019

robodoo pushed a commit that referenced this pull request Feb 26, 2019

[FIX] expression: properly handle {TRUE,FALSE}_LEAF
Before this commit, doing expression.OR() with only FALSE_LEAF would
yield [] which is equivalent to TRUE_LEAF and is therefore not correct.

The same happened (to a lesser extent) with expression.AND() within an
expression.OR(), since the former would return a [] which would be
ignored by expression.OR().

See tests for a clearer view of the use cases.

Fixes #30113, #26540

closes #31202
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 26, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.