[FIX] validate field names in attributes of views #20893

rco-odoo · 2017-11-13T11:12:16Z

OPW 779494

rco-odoo · 2017-11-13T11:13:25Z

@sle-odoo I had to fix some views in mrp and stock.
Can you please have a look? Thanks

mgeubelle

Not confident enough with my Python knowledge to approve but that's a great improvement ! Thanks.

mgeubelle · 2017-11-13T16:08:58Z

addons/mrp/views/mrp_production_views.xml

@@ -121,7 +121,7 @@
                                    <field name="needs_lots" readonly="1" groups="stock.group_production_lot"/>
                                    <field name="is_done" invisible="1"/>
                                    <field name="sequence" invisible="1"/>
-                                    <field name="location_id" domain="[('id', 'child_of', parent.location_id)]" invisible="1"/>
+                                    <field name="location_id" invisible="1"/>


Are you sure you can just delete the domain? Maybe the field has been renamed? I see production_location_id on the parent, could it be it?

There are 3 fields referring to locations in the parent model...

We don't care since it's invisible

mgeubelle · 2017-11-13T16:10:23Z

addons/mrp/views/stock_move_views.xml

@@ -79,7 +79,7 @@
                                <span><field name="product_uom_qty" readonly="1" nolabel="1"/></span>
                                <span><field name="product_uom_id" attrs="{'readonly': [('id', '!=', False)]}" nolabel="1"/></span>
                            </div>
-                            <field name="lot_id" string="Lot/Serial number" groups="stock.group_production_lot" readonly="1" attrs="{'invisible': [('lots_visible', '=', False)]}"/>
+                            <field name="lot_id" string="Lot/Serial number" groups="stock.group_production_lot" readonly="1"/>


lots_visible still exists on the model ; invisible isn't needed anymore?

Add the field lots_visible as invisible instead.

Removing the attrs keeps the behavior as is. We can also add the field lots_visible in the view...

mgeubelle · 2017-11-13T16:28:47Z

odoo/addons/base/tests/test_views.py

+            })
+
+    @mute_logger('odoo.addons.base.ir.ir_ui_view')
+    def test_context_in_subview(self):


I would have added a test if model is not in the subview but in the main view.

mgeubelle · 2017-11-13T16:30:05Z

odoo/addons/base/tests/test_views.py

+                </form>
+            """,
+        })
+        with self.assertRaises(ValidationError):


Idem (error if model in main view but not in subview).

pimodoo · 2017-11-14T13:19:00Z

@sle-odoo

amoyaux · 2017-11-14T13:26:36Z

addons/stock/views/stock_move_views.xml

@@ -9,7 +9,7 @@
                    <field name="product_id"/>
                    <field name="product_uom_qty"/>
                    <field name="product_uom" options="{'no_open': True, 'no_create': True}" string="Unit of Measure" groups="product.group_uom"/>
-                    <field name="product_packaging" domain="[('product_tmpl_id','=',product_tmpl_id)]" groups="product.group_stock_packaging"/>


You should replace it by domain="[('product_id','=',product_id)]" instead of remove it

amoyaux · 2017-11-14T13:30:17Z

addons/stock/views/stock_move_views.xml

@@ -183,7 +183,7 @@
                <tree editable="bottom" decoration-muted="state == 'done' and is_locked == True" decoration-success="product_uom_qty==qty_done" decoration-danger="qty_done &gt; product_uom_qty and state != 'done'">
                    <field name="picking_id" invisible="1"/>
                    <field name="product_id" invisible="1"/>
-                    <field name="location_id" invisible="not context.get('show_source_location')" domain="[('id', 'child_of', parent.location_id)])" groups="stock.group_stock_multi_locations"/>
+                    <field name="location_id" invisible="not context.get('show_source_location')" groups="stock.group_stock_multi_locations"/>


What should we do? Do we need to duplicate twice this view and add them in two inline views instead of using tree_view_ref? Or create a related field parent_location_id on stock_move_line?

@amoyaux We can also add the missing field location_id in the parent view(s).

Actually this domain had a typo (extra closing parenthesis).
So I just fixed the typo 😄

nhomar · 2017-11-15T09:25:01Z

please my friends, do not even think put this in master PLEASE ;-)

rco-odoo · 2017-11-15T09:29:36Z

@nhomar why? Can you elaborate on this?

The main purpose of this validation is to prevent people from screwing up views with Studio. We recently got examples of users removing fields from views, and making domains impossible to evaluate. The issue was totally invisible while in Studio, but the view kept on crashing when used for real ☹️

nhomar · 2017-11-15T15:55:39Z

@rco-odoo I mean I love it in 11.0 as it is, but the point is that the stable policy does not allow fix this kind of big things in stable and this PR is very nice, this was the reason of my comments.

xmo-odoo · 2017-11-15T16:03:46Z

odoo/addons/base/ir/ir_ui_view.py

+                # retrieve subfields of 'parent'
+                get = partial(get_parent, get)
+            for child in node:
+                yield from node_names(child, get)


That doesn't seem like a hill worth removing Python 2 support on

Not funny. I wanted to break some stuff ;-)

xmo-odoo · 2017-11-15T16:07:01Z

odoo/addons/base/ir/ir_ui_view.py

+        return ['self', 'parent', 'id', 'uid', 'context',
+                'active_id', 'active_ids', 'active_model',
+                'time', 'datetime', 'relativedelta',
+                'context_today', 'current_date']


Could return a set directly.

xmo-odoo · 2017-11-15T16:08:32Z

odoo/addons/base/ir/ir_ui_view.py

+            return (name
+                    for node in ast.walk(ast.parse(expr.strip(), mode='eval'))
+                    for name in [get(node)]
+                    if name)


Why make these inner functions? That seems unnecessary.

Readability. It took me a while to get the algorithm right.

I understand having them as functions (that seems fair), just not inner ones, why not toplevel?

xmo-odoo · 2017-11-15T16:14:38Z

odoo/addons/base/ir/ir_ui_view.py

+                attrs = node.get('attrs')
+                # collect field names in left-hand-side of conditions
+                for domain in safe_eval(attrs).values():
+                    if isinstance(domain, list):


Is there any case where the domain could be a non-list?

It once crashed with a boolean domain. Maybe we should fix the domain...

rco-odoo · 2017-11-16T10:46:49Z

@nhomar I understand and I always keep this aspect in mind.
On the other hand, we need something to validate the views modified by Studio. We have recently encountered support tickets where people screw up views with Studio without noticing...

I was thinking about using this when actually validating the view, but not when building the view for the client (via fields_view_get). This would ensure validation when you modify views, but not when you simply use them.

wtaferner · 2017-11-16T10:59:13Z

@rco-odoo
I am not surprised that Odoo Studio is screwing up a lot even though I think you guys did a great job that this kind of marketing tool can be helpful, even for devs at some point (now I am trying to keep my customers in bounce that they do not screw up on their new freedom too fast).
A lot of work still to do and ahead as we talked on Odoo experience to have all different data sources reasonably under control, aware to performance through recommendations and hints and most of all flexible and endurable over versions (still ask myself how Odoo will handle studio customization in migrations...deactivating breaking views and removing fields is probably not the smartest move when you sold it as a killer feature), but keep on rockin :-)

rco-odoo · 2017-11-24T11:03:48Z

@mart-e I add a new translated term here (error message).
Should I regenerate the .pot file?

rco-odoo · 2017-11-24T11:11:03Z

@odony this adds some extra validation in views, to answer a request from the Studio team.
We recently got several support tickets for users that screw up views with Studio: they remove a field that is used in a domain in another field, for instance. The idea is, in the view validation, to check that all fields appearing in domains, contexts, etc. are effectively present in the view.

I would very much appreciate your review. Implementing the validation brought a few surprises: some views in Odoo 11 are not valid. Hopefully they do not seem to break anything so far, because of domains never being evaluated or so.

rco-odoo · 2017-12-01T10:00:38Z

ping @odony

odony

Looks very good and elegant, considering the complexity of the verification, thanks!

It will be excellent with the small improvement to the error message, as discussed.

OPW 779494

rco-odoo requested review from xmo-odoo and mgeubelle November 13, 2017 11:12

xmo-odoo requested a review from sle-odoo November 13, 2017 11:17

rco-odoo force-pushed the 11.0-opw-779494-rco branch from bc52496 to 65d6f9e Compare November 13, 2017 13:58

C3POdoo added the OE the report is linked to a support ticket (opw-...) label Nov 13, 2017

mgeubelle reviewed Nov 13, 2017

View reviewed changes

rco-odoo force-pushed the 11.0-opw-779494-rco branch from 65d6f9e to 56cbaaa Compare November 14, 2017 13:13

rco-odoo requested review from pimodoo and removed request for sle-odoo November 14, 2017 13:15

rco-odoo force-pushed the 11.0-opw-779494-rco branch from 56cbaaa to b2ddd22 Compare November 14, 2017 13:17

amoyaux reviewed Nov 14, 2017

View reviewed changes

rco-odoo force-pushed the 11.0-opw-779494-rco branch 3 times, most recently from 76a2c9c to 6d7a408 Compare November 15, 2017 09:22

rco-odoo force-pushed the 11.0-opw-779494-rco branch from 6d7a408 to 73bab27 Compare November 15, 2017 13:24

xmo-odoo approved these changes Nov 15, 2017

View reviewed changes

rco-odoo force-pushed the 11.0-opw-779494-rco branch 2 times, most recently from 50f5217 to c4566f4 Compare November 21, 2017 09:16

rco-odoo force-pushed the 11.0-opw-779494-rco branch 2 times, most recently from 50537cc to 3ac561a Compare November 23, 2017 16:22

rco-odoo requested a review from odony November 24, 2017 11:11

odony approved these changes Dec 6, 2017

View reviewed changes

rco-odoo force-pushed the 11.0-opw-779494-rco branch from 3ac561a to a3f3d9c Compare December 7, 2017 10:05

rco-odoo added 2 commits December 7, 2017 14:21

[FIX] ir_ui_view: validate contexts, domains and attrs in views

7b3796d

OPW 779494

[FIX] mrp, stock: bad context/domain/attrs in views

0109c9e

rco-odoo force-pushed the 11.0-opw-779494-rco branch from a3f3d9c to 0109c9e Compare December 7, 2017 13:21

rco-odoo merged commit 0109c9e into odoo:11.0 Dec 7, 2017

rco-odoo deleted the 11.0-opw-779494-rco branch December 7, 2017 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] validate field names in attributes of views #20893

[FIX] validate field names in attributes of views #20893

rco-odoo commented Nov 13, 2017

rco-odoo commented Nov 13, 2017

mgeubelle left a comment

mgeubelle Nov 13, 2017

rco-odoo Nov 14, 2017

amoyaux Nov 14, 2017

mgeubelle Nov 13, 2017

amoyaux Nov 14, 2017 •

edited

Loading

rco-odoo Nov 14, 2017

mgeubelle Nov 13, 2017

mgeubelle Nov 13, 2017

pimodoo commented Nov 14, 2017

amoyaux Nov 14, 2017

amoyaux Nov 14, 2017

rco-odoo Nov 14, 2017

rco-odoo Nov 15, 2017

nhomar commented Nov 15, 2017

rco-odoo commented Nov 15, 2017

nhomar commented Nov 15, 2017

xmo-odoo Nov 15, 2017

rco-odoo Nov 16, 2017

xmo-odoo Nov 15, 2017

xmo-odoo Nov 15, 2017

rco-odoo Nov 16, 2017

xmo-odoo Nov 16, 2017

xmo-odoo Nov 15, 2017

rco-odoo Nov 16, 2017

rco-odoo commented Nov 16, 2017

wtaferner commented Nov 16, 2017 •

edited

Loading

rco-odoo commented Nov 24, 2017

rco-odoo commented Nov 24, 2017

rco-odoo commented Dec 1, 2017

odony left a comment

[FIX] validate field names in attributes of views #20893

[FIX] validate field names in attributes of views #20893

Conversation

rco-odoo commented Nov 13, 2017

rco-odoo commented Nov 13, 2017

mgeubelle 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

amoyaux Nov 14, 2017 • edited Loading

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

pimodoo commented Nov 14, 2017

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

nhomar commented Nov 15, 2017

rco-odoo commented Nov 15, 2017

nhomar commented Nov 15, 2017

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

rco-odoo commented Nov 16, 2017

wtaferner commented Nov 16, 2017 • edited Loading

rco-odoo commented Nov 24, 2017

rco-odoo commented Nov 24, 2017

rco-odoo commented Dec 1, 2017

odony left a comment

Choose a reason for hiding this comment

amoyaux Nov 14, 2017 •

edited

Loading

wtaferner commented Nov 16, 2017 •

edited

Loading