[FIX] web: fix the traceback #106503

ppr-odoo · 2022-11-25T07:31:25Z

fix the trackback when we add larger value in integer field

Why trackback occur:
     Currently,  the range of integer is not checked, so when we entering a large value, an
     out of range exception is thrown.
Steps to reproduce:
   - Installed Contacts
   - Open the  Abigail Peterson
   - Add the Level Weight  ex-999999999999999999999
   - Save the record

Fix the trackback in progressbar field

Why trackback occur:
      When entering larger value in integer progressbar, an out of range exception is thrown
       because the check parseFloat.

Steps to reproduce:

 -Installed project  app
 -Open Project > Go to project update
 -Add value in Progress field(Ex-999999999999999)
 -Save the record

task-3070721

robodoo · 2022-11-25T07:32:09Z

Pull request status dashboard.

MissingNoShiny

Hello, thanks for your work. 🙂
I left a few comments

MissingNoShiny · 2023-01-26T10:00:48Z

addons/web/static/src/views/fields/parsers.js

@@ -133,6 +133,9 @@ export function parseInteger(value) {
            throw new InvalidNumberError(`"${value}" is not a correct number`);
        }
    }
+    if (parsed % 1 || parsed < -2147483648 || parsed > 2147483647) {


All three checks could be combined into if (parsed | 0 !== parsed) thanks to the fact that bitwise operations return 32 bits integers in javascript. Not sure if that would be considered too magical though

All three checks could be combined into if (parsed | 0 !== parsed) thanks to the fact that bitwise operations return 32 bits integers in javascript. Not sure if that would be considered too magical though

Hello ,
Already used in 15.0 so I used it 15.0. I checked your suggestion but doesn't' working.

You are right, it won't work because !== has a higher priority than |.
This should work however: if ((parsed | 0) !== parsed).

But as you said, it might be better to keep it as it was in 15.0 then.

MissingNoShiny · 2023-01-26T10:01:17Z

addons/web/static/tests/views/fields/progress_bar_field_tests.js

@@ -594,7 +594,7 @@ QUnit.module("Fields", (hooks) => {
            const input = target.querySelector(".o_progressbar_value.o_input");
            assert.strictEqual(input.value, "99", "Initial value in input is correct");

-            await editInput(target, ".o_field_widget input", "1#037:9");
+            await editInput(target, ".o_field_widget input", "1037");


This change defeats the point of the test. You should change the test to use float_field instead.

This change defeats the point of the test. You should change the test to use float_field instead.

this test write float instead of int meaning is changed if we use float_field field instead float_integer. what do you think ?

This test is checking that we can use both floats and integers, and most importantly floats with a custom locale. With this PR, we can no longer enter floats if the field is an integer, so the type of the field should be changed rather than using an integer instead of a float in the test.

Now. I'v updated the test title and code. You can check it. thank you

MissingNoShiny · 2023-01-26T10:20:31Z

addons/web/static/tests/views/fields/progress_bar_field_tests.js

@@ -650,4 +650,27 @@ QUnit.module("Fields", (hooks) => {
            assert.verifySteps(["Show error message"], "The error message was shown correctly");
        }
    );
+
+    QUnit.test(
+        "ProgressBarField: check the input value is valid or not in progressbar field",


This message doesn't make the purpose of the test very clear. What do you think of something like :
"ProgressBarField: writing overflowing int throws warning"?

MissingNoShiny · 2023-01-26T10:26:21Z

addons/web/static/src/views/fields/parsers.js

@@ -133,6 +133,9 @@ export function parseInteger(value) {
            throw new InvalidNumberError(`"${value}" is not a correct number`);
        }
    }
+    if (parsed % 1 || parsed < -2147483648 || parsed > 2147483647) {
+        throw new InvalidNumberError(`"${value}" is not a correct number`);


I feel like this message is not very clear as it does not specify that values are limited to integers, although I'm not sure how we could change it without getting too technical. Maybe "${value}" is not a valid integer? And the other message above at line 133 has the same issue, maybe it could be changed too?

MissingNoShiny · 2023-01-26T10:36:19Z

addons/web/static/src/views/fields/progress_bar/progress_bar_field.js

+            if (this.props.type === 'integer') {
+                parsedValue = parseInteger(ev.target.value);
+            } else {
+                parsedValue = parseFloat(ev.target.value);
+            }


Has this been discussed with the framework team? It's a slight change in behavior

Has this been discussed with the framework team? It's a slight change in behavior

no discussion with framework team about this. you can do

Why trackback occur: Currently, the range of integer is not checked, so when we entering a large value, an out of range exception is thrown. Old version check the range-https://github.com/odoo/odoo/blob/15.0/addons/web/static/src/legacy/js/fields/field_utils.js#L700 Steps to reproduce: - Installed Contacts - Open the Abigail Peterson - Add the Level Weight ex-999999999999999999999 - Save the record task-3070721

Why trackback occur: When entering larger value in integer progressbar, an out of range exception is thrown because the check parseFloat. Steps to reproduce: -Installed project app -Open Project > Go to project update -Add value in Progress field(Ex-999999999999999) -Save the record task-3070721

sdegueldre · 2023-01-27T09:54:16Z

I don't think this should be fixed in JS at all. It is possible to modify the backing field in postgres to bigint to support larger numbers if you need to, and at the moment this works fine and Odoo doesn't care. This breaks that use case. I think a better solution would be to catch the psycopg error and raise a ValidationError from the python which will prevent the save.

C3POdoo added the RD research & development, internal work label Nov 25, 2022

ppr-odoo changed the title ~~16.0 web number field ppr~~ [FIX] web: fix the traceback Nov 25, 2022

MissingNoShiny reviewed Jan 26, 2023

View reviewed changes

ppr-odoo force-pushed the 16.0-web-number-field-ppr branch from 759f31d to 278fc9e Compare January 27, 2023 06:16

MissingNoShiny marked this pull request as ready for review January 27, 2023 08:51

C3POdoo requested review from a team, caburj and sdegueldre and removed request for a team January 27, 2023 08:52

ppr-odoo added 2 commits January 27, 2023 15:12

ppr-odoo force-pushed the 16.0-web-number-field-ppr branch from 278fc9e to 5e14ce4 Compare January 27, 2023 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] web: fix the traceback #106503

[FIX] web: fix the traceback #106503

ppr-odoo commented Nov 25, 2022 •

edited

robodoo commented Nov 25, 2022

MissingNoShiny left a comment

MissingNoShiny Jan 26, 2023

ppr-odoo Jan 27, 2023

MissingNoShiny Jan 27, 2023

MissingNoShiny Jan 26, 2023

ppr-odoo Jan 27, 2023

MissingNoShiny Jan 27, 2023

ppr-odoo Jan 27, 2023

MissingNoShiny Jan 26, 2023

MissingNoShiny Jan 26, 2023

MissingNoShiny Jan 26, 2023

ppr-odoo Jan 27, 2023

sdegueldre commented Jan 27, 2023

[FIX] web: fix the traceback #106503

Are you sure you want to change the base?

[FIX] web: fix the traceback #106503

Conversation

ppr-odoo commented Nov 25, 2022 • edited

robodoo commented Nov 25, 2022

MissingNoShiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdegueldre commented Jan 27, 2023

ppr-odoo commented Nov 25, 2022 •

edited