From 44a19384fcf4f866cddeb008cd7337e9453f4e2e Mon Sep 17 00:00:00 2001 From: qsm-odoo Date: Fri, 27 May 2022 15:23:51 +0000 Subject: [PATCH] [FIX] tools, base: restore branding on siblings of a replaced root node Since [1] which tried to fix the data-oe-xpath branding on nodes in some cases, the branding actually became potentially incorrect on siblings of a node which is replaced multiple times. E.g. Parent view: ```xml ``` Child view 1: ```xml ``` Child view 2: ```xml ``` No problem, two distincts elements are replaced, the system understands that the `data-oe-xpath` of the third world of the parent view should be `/hello[1]/world[3]`. But in this other case: Parent view: ```xml ``` Child view: ```xml ``` Child view of the child view: ```xml ``` The `data-oe-xpath` of the third world of the parent view (in the resulting view) was wrong: `/hello[1]/world[4]` -> because the system saw two replacements + the unreplaced second ``, so the index "4" was computed. Now the system will understand that the double replacement in fact acts as a single replacement. Note: this was also the same with "cross inheriting" (if the "new_a" `` of the child view was replaced by another child view of the parent view). At last, another 4th case was found and worth mentioning because it is in fact the root cause of the problem. The problem is not actually the double replacement as mentioned above but simply the replacement of a root level element of a child view (which is what is basically done in the last two mentioned cases). In that case, the root level nodes added by the first child view have already their `data-oe-xpath` branding computed before they are potentially replaced. Indicating the location of the replacement in that case was thus only leading to bugs. E.g. Parent view: ```xml ``` Child view: ```xml ``` Child view of the child view: ```xml ``` Before this commit, before the branding is distributed, the result is: ```xml ``` => Hence the `data-oe-xpath` of the last `` was computed to `/hello[1]/world[3]` instead of `/hello[1]/world[2]` after branding distribution because the ProcessingInstruction marking the node removal location should not have been added: it could only be useful to following siblings which are not branded, which is not possible as the branding added on the second `` of the child view (`/data/xpath/world[2]`) was computed before any removal. Tests are added in this commit for the 3 last mentioned cases. As explained, the last case is actually the same of the 2nd and 3rd ones but it was decided to keep the 3 tests as it helps to understand the problems better and, if the code evolves, it could become different cases (= this is 3 cases which are currently technically equivalent but these are different functionnal use cases). A test was written for the first case then removed as it is basically a pure copy of other existing tests written in [2] (trying to be improved by [1]). [1]: https://github.com/odoo/odoo/commit/f67832a3ae0d9a3b5b53129132762e6bc1aed874 [2]: https://github.com/odoo/odoo/commit/c077ef05575d9677bce284195683f96c68386788 closes odoo/odoo#92589 X-original-commit: d6e0b3d570a4b27f72852eb261660ad09de12eeb Signed-off-by: Romain Derie (rde) Signed-off-by: Quentin Smetz (qsm) --- odoo/addons/base/tests/test_views.py | 176 +++++++++++++++++++++++++++ odoo/tools/template_inheritance.py | 8 +- 2 files changed, 183 insertions(+), 1 deletion(-) diff --git a/odoo/addons/base/tests/test_views.py b/odoo/addons/base/tests/test_views.py index 71fbb699e13..9118e3ce48e 100644 --- a/odoo/addons/base/tests/test_views.py +++ b/odoo/addons/base/tests/test_views.py @@ -950,6 +950,182 @@ class TestTemplating(ViewCase): initial.get('data-oe-xpath'), "The node's xpath position should be correct") + def test_branding_inherit_multi_replace_node(self): + view1 = self.View.create({ + 'name': "Base view", + 'type': 'qweb', + 'arch': """ + + + + + + """ + }) + view2 = self.View.create({ + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view1.id, + 'arch': """ + + + + + + + """ + }) + self.View.create({ # Inherit from the child view and target the added element + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view2.id, + 'arch': """ + + + + + + """ + }) + + arch_string = view1.with_context(inherit_branding=True).get_combined_arch() + arch = etree.fromstring(arch_string) + self.View.distribute_branding(arch) + + # Check if the replacement inside the child view did not mess up the + # branding of elements in that child view + [initial] = arch.xpath('//world[hasclass("z")]') + self.assertEqual( + '/data/xpath/world[2]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + + # Check if the replacement of the first worlds did not mess up the + # branding of the last world. + [initial] = arch.xpath('//world[hasclass("c")]') + self.assertEqual( + '/hello[1]/world[3]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + + def test_branding_inherit_multi_replace_node2(self): + view1 = self.View.create({ + 'name': "Base view", + 'type': 'qweb', + 'arch': """ + + + + + + """ + }) + self.View.create({ + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view1.id, + 'arch': """ + + + + + + + """ + }) + self.View.create({ # Inherit from the parent view but actually target + # the element added by the first child view + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view1.id, + 'arch': """ + + + + + + """ + }) + + arch_string = view1.with_context(inherit_branding=True).get_combined_arch() + arch = etree.fromstring(arch_string) + self.View.distribute_branding(arch) + + # Check if the replacement inside the child view did not mess up the + # branding of elements in that child view + [initial] = arch.xpath('//world[hasclass("z")]') + self.assertEqual( + '/data/xpath/world[2]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + + # Check if the replacement of the first worlds did not mess up the + # branding of the last world. + [initial] = arch.xpath('//world[hasclass("c")]') + self.assertEqual( + '/hello[1]/world[3]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + + def test_branding_inherit_remove_added_from_inheritance(self): + view1 = self.View.create({ + 'name': "Base view", + 'type': 'qweb', + 'arch': """ + + + + + """ + }) + view2 = self.View.create({ + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view1.id, + # Note: class="x" instead of t-field="x" in this arch, should lead + # to the same result that this test is ensuring but was actually + # a different case in old stable versions. + 'arch': """ + + + + + + + """ + }) + self.View.create({ # Inherit from the child view and target the added element + 'name': "Extension", + 'type': 'qweb', + 'inherit_id': view2.id, + 'arch': """ + + + + """ + }) + + arch_string = view1.with_context(inherit_branding=True).get_combined_arch() + arch = etree.fromstring(arch_string) + self.View.distribute_branding(arch) + + # Check if the replacement inside the child view did not mess up the + # branding of elements in that child view, should not be the case as + # that root level branding is not distributed. + [initial] = arch.xpath('//world[hasclass("y")]') + self.assertEqual( + '/data/xpath/world[2]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + + # Check if the child view replacement of added nodes did not mess up + # the branding of last world in the parent view. + [initial] = arch.xpath('//world[hasclass("b")]') + self.assertEqual( + '/hello[1]/world[2]', + initial.get('data-oe-xpath'), + "The node's xpath position should be correct") + def test_branding_inherit_remove_node_processing_instruction(self): view1 = self.View.create({ 'name': "Base view", diff --git a/odoo/tools/template_inheritance.py b/odoo/tools/template_inheritance.py index 45e7d7f96f5..77f0057cd40 100644 --- a/odoo/tools/template_inheritance.py +++ b/odoo/tools/template_inheritance.py @@ -183,7 +183,13 @@ def apply_inheritance_specs(source, specs_tree, inherit_branding=False, pre_loca # fix, this solution was chosen: the location is marked # with a "ProcessingInstruction" which will not impact # the "Element" structure of the resulting tree. - if inherit_branding: + # Exception: if we happen to replace a node that already + # has xpath branding (root level nodes), do not mark the + # location of the removal as it will mess up the branding + # of siblings elements coming from other views, after the + # branding is distributed (and those processing instructions + # removed). + if inherit_branding and not node.get('data-oe-xpath'): node.addprevious(etree.ProcessingInstruction('apply-inheritance-specs-node-removal', node.tag)) for child in spec: