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

[IMP] web: throwing an exception when comparison wrong datatypes #153595

Closed

Conversation

mega-odoo
Copy link
Contributor

Before this commit

When comparing values of different data types in an XML expression,The expression would return a value instead of throwing an exception.

After this commit

If a developer mistakenly compares values of different data types in an XML expression, An exception will be thrown.

Task - 3600595


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Feb 12, 2024

@C3POdoo C3POdoo added the RD research & development, internal work label Feb 12, 2024
@mega-odoo mega-odoo force-pushed the master-compare-wrong-datatypes-mega branch 2 times, most recently from dc8d540 to 028dacf Compare February 19, 2024 13:12
@mega-odoo mega-odoo force-pushed the master-compare-wrong-datatypes-mega branch 6 times, most recently from 4671ca0 to 2d0be61 Compare February 20, 2024 12:49
expect(evaluateExpr("'foo' < 4444")).toBe(false);
expect(evaluateExpr("{} < []")).toBe(true);
expect(evaluateExpr('0 < "2024-02-09"')).toBe(true);
expect(evaluateExpr('"2024-02-09" > 0')).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I see, the purpose of the pr is to make an error when the types are different.
In this case, why make a new special case with the dates?

@@ -96,7 +102,14 @@ function isLess(left, right) {
if (leftIndex === rightIndex) {
return left < right;
}
return leftIndex < rightIndex;
if ((left == 0 && rightIndex == 4) || (leftIndex == 4 && right == 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make a new special case with the dates and 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In certain scenarios, when comparing two date fields and one of them is empty, At that point system treats it as a "false" value. Consequently, there arises a need to handle comparisons between a "false" value and a valid date value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Gorash I think we should cancel this task. I didn't anticipate the date cornercase, but now that I'm aware of it, I think it's better to keep the behavior as it is right now. 0 < "2022-10-10" isn't a valid python expression, but we must support it. So those aren't really python expressions, I rather see them as a derived DSL. We should keep it as simple as possible, and this PR adds complexity.

@mega-odoo mega-odoo force-pushed the master-compare-wrong-datatypes-mega branch from 2d0be61 to b40fa67 Compare March 13, 2024 13:39
Before this commit

When comparing values of different data types in an XML
expression,The expression would return a value instead
of throwing an exception.

After this commit

If a developer mistakenly compares values of different data
types in an XML expression, An exception will be thrown.

Task-3600595
@mega-odoo mega-odoo force-pushed the master-compare-wrong-datatypes-mega branch from b40fa67 to f63d221 Compare March 14, 2024 05:58
@aab-odoo aab-odoo marked this pull request as ready for review May 17, 2024 05:40
@C3POdoo C3POdoo requested review from a team, aab-odoo and BastienFafchamps and removed request for a team May 17, 2024 05:41
* @param {any} val
* @returns {boolean} boolean type
*/
function is_date(val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isDate, not is_date

@aab-odoo
Copy link
Contributor

Cancelling the task/closing the PR. See #153595 (comment) for reason.

@aab-odoo aab-odoo closed this May 29, 2024
@aab-odoo
Copy link
Contributor

Sorry for the inconvenience @mega-odoo @maan-odoo @bit-odoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants