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

[14.0] Owl and Checkbox not playing well together #69726

Closed
llacroix opened this issue Apr 23, 2021 · 7 comments
Closed

[14.0] Owl and Checkbox not playing well together #69726

llacroix opened this issue Apr 23, 2021 · 7 comments
Labels
14.0 Confirmed the bug was confirmed by testers Framework General frontend/backend framework issues

Comments

@llacroix
Copy link

Impacted versions: at least 14.0

Steps to reproduce:

  1. Go in a stock picking and sign it or define an image in the field signature.
  2. Go in the tree view with the xmlid stock.vpicktree
  3. Enable the field signature as it is an optional field

Current behavior:

Traceback:
Error: Invalid Prop 'value' in component 'CustomCheckbox'
QWeb.utils.validateProps@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:1181:20
Component@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:1206:42
CustomCheckbox@https://7324612-14-0-all.runbot58.odoo.com/web/content/1760-7818cb2/web.assets_backend.js:841:172
anonymous@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js line 961 > Function:28:16
fn@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:943:173
render@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:950:20
__render@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:1267:33
__prepareAndRender@https://7324612-14-0-all.runbot58.odoo.com/web/content/1759-69b147c/web.assets_common.js:1265:29

The issue is that the field is a binary field, but the list view attempt to display a boolean value. When odoo loads the signature field, it in fact return the size in Kb of the image... And then the value is validated as invalid. :( and die.

Expected behavior:
No errors

Support ticket number submitted via odoo.com/help (optional):

@pedrobaeza pedrobaeza added 14.0 Framework General frontend/backend framework issues labels Apr 23, 2021
@pedrobaeza
Copy link
Collaborator

I don't think that's OWL anyway...

@llacroix
Copy link
Author

llacroix commented Apr 23, 2021

@pedrobaeza based on which assumption?

The error comes from here: https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3725

After this somehow set it to isValid to false https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3718

Then I guess here https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3766

It will check let say isValidProp("4Kb", Boolean) because the type is Boolean and the value is "4kb" for a 4Kb binary field.

But there is no test for when the type is Boolean

But since Boolean is a constructor when it reenter here:
https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3742

It will get to this because a string isn't an object and the String isn't a Boolean.

https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3751

So if it's an issue in Owl or in Odoo it's a matter of point of view... But either way, it will be an issue in Owl because of this:

https://github.com/odoo/odoo/blob/14.0/addons/web/static/lib/owl/owl.js#L3768

See, if the result from the previous call to isValidProp fails, then it won't even check the validate method of the property. In other words, even if you wanted to fix the issue in the CustomCheckbox it would still fail to validate because the type of the value isn't a boolean.

But if we think about it with sanity, a boolean should be either a falsy or non falsy value as it probably was before Owl. Which explains why the signature being a string worked for checkboxes / boolean field widget.

But if we think about it, why even use a Boolean widget for a binary field. Odoo should define a boolean field like "is_signed" to be used instead of displaying a boolean widget on a binary field. But at the same type, a Boolean type could be a special case where any non falsy value is considered as truthy as long as the widget is readonly in case the value isn't true,false or undefined.

Either way, Owl and Checkbox in Odoo don't play well together because it's not guaranteed that a boolean widget will be used with a boolean value and Owl doesn't handle that case.

In my opinion, both are wrong and should be fixed. Owl should be a bit more resilient and allow property validation to override the initial validation. If the validate method exist, it should be more important as it may allow handling special cases like this directly in the property...
Odoo shouldn't mix things up and have hacks here and there to make a binary field display as a boolean field. In a system I developed at work, I fixed that by having widgets per type. So instead of having a Boolean widget, I'd have a Boolean widget for a BooleanField and a Boolean widget for a BinaryField. This way the boolean widget isn't coupled to the type but the component themselves are coupled to a certain field type. Both widget may share or not share the same templates.
One example would be the selection widget that works with Selection/Many2One fields. In odoo you could define it as a widget="selection" and it will behave correctly for both of them but they're different type internally.

@pedrobaeza
Copy link
Collaborator

AFAIK, OWL is not yet fully activated in v14 in the web client, just a gateway through it for specific components (chatter and a few more), but I think tree view is not one of the activated components. Maybe it's just a problem of the compatibility layer that acts in the whole web client...

Anyways, you can see that I tagged the issue for being checked by the framework team.

@llacroix
Copy link
Author

I don't know, I've got other issues recently with the CustomCheckbox in sale orders in relation with the sale_product_configurator widget.
I'm not creating an issue yet as I can't exactly reproduce it on runbot. When I change a checkbox value during an onchange of a product that opens the configurator wizard, it fails with this.componentRef.comp is null while a getFocusable event is triggered (not the exact name).
I could patch odoo locally to handle cases when the value is null and simply ignore it.

I'm 100% that the configurator widget action triggers that because when we uninstall the module and we select the products, it does work without issues and the boolean value is updated correctly.

My guess is that the issue gets triggered when the onchange on product_id defines a value to True on a checkbox of the same sale.order.line but there's no way to test that using base modules. the only boolean fields seems to be triggered when the product_tmpl_id is changed but not when the product_id is changed.

@msh-odoo
Copy link
Contributor

@llacroix I checked and produced the issue, will have a look.

@msh-odoo
Copy link
Contributor

@llacroix Should be fixed with #72276

@Yenthe666 Yenthe666 added the Confirmed the bug was confirmed by testers label Jun 17, 2021
@msh-odoo
Copy link
Contributor

@llacroix @pedrobaeza We can close this issue as it has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
14.0 Confirmed the bug was confirmed by testers Framework General frontend/backend framework issues
Projects
None yet
Development

No branches or pull requests

4 participants