orm: xml field location method for inheritance is flawed

[Nicolas JEUDY]

description:

In orm.py (2074):

       def locate(source, spec):
       ...
            elif spec.tag == 'field':
                # Only compare the field name: a field can be only once in a given view
                # at a given level (and for multilevel expressions, we should use xpath
                # inheritance spec anyway).
                for node in source.getiterator('field'):
                    if node.get('name') == spec.get('name'):
                        return node
                return None
        ...

when doing applying inheritance mechanism,

  eg. :

                <field name="state" position="after">
                    <field name="sale_line_id" string="Order Line"/>
                </field>

Only the first node matching the field name is returned. getiterator() parses the xml depth first, so the first field matching the field name will be the top-most in the file. If you have defined subviews (form,tree on one2many field for example) before the targetted field, and one of their field name matches the searched parent field name, *the replacement will miss its original target* and this will produce unexpected bugs. These are very difficult to trace, especially if subviews came from inheritance mechanism.

Version: OpenERP 7.0 runbot (and probably others)
How to demonstrate this thanks to a direct bug in runbot:

- launch runbot
- in setting>project, check "allow task delegation"
- In task view, sale_order_id should appear after state located in Extra info page

 code: (project_mrp/project_mrp_view.xml)

       <record id="view_project_mrp_inherit_form2" model="ir.ui.view">
            <field name="name">project.mrp.form.view.inherit</field>
            <field name="model">project.task</field>
            <field name="inherit_id" ref="project.view_task_form2"/>
            <field name="arch" type="xml">
                <field name="state" position="after">
                    <field name="sale_line_id" string="Order Line"/>
                </field>
            </field>
        </record>

but due to the presence of 2 occurrences of "state" labeled fields in the project.task form, inheritance apply only to the first one, which happens to be the wrong. With chance (or bad luck), this bug won't cause exceptions, because the subview happens to support the addition of "sale_line_id". Nevertheless, this causes a silent bug in the replacement of the field. This is what is shown in the video.

project.project_view.xml:

        <field name="arch" type="xml">
                <form string="Project" version="7.0">
                    ....
                    <notebook>
                        ...
                        <page string="Delegation" groups="project.group_delegate_task">
                            ...
                            <field name="child_ids">
                                <tree string="Delegated tasks">
                                    ...
                                    <field name="stage_id"/>
                                    <field name="state" invisible="1"/>
                                    <field name="effective_hours" widget="float_time"/>
                                    ...
                                </tree>
                            </field>
                        </page>
                        <page string="Extra Info" attrs="{'readonly':[('state','=','done')]}">
                            <group col="4">
                                ...
                                <field name="state" invisible="1"/>
                            </group>
                        </page>
                    </notebook>
                    </sheet>
                     ...
                </form>
            </field>

[Nicolas JEUDY (njeudy)]

See this https://bugs.launchpad.net/openobject-addons/+bug/1097808 to see consequence

[Jignesh Rathod(OpenERP) (jir-openerp)]

Hello Nicolas JEUDY ,

I agree with you , When we used <fields name="" position =""> it always
search for first level .it should work for multilevel expressions,
currently for multi-level you can use xpath. But its good improvement, So
I make this issue as wishlist.

Thanks.