[FIX] core: propagate update of modifiers attributes from base terms

[aj-fuentes]

When we update an inline translated element (like <span>) for the base
language en_US we should update the modifiers attributes for all
languages.

For example when updating from (16.0)
https://github.com/odoo/odoo/blob/7ecd9413/odoo/addons/base/views/ir_ui_view_views.xml#L127-L129
https://github.com/odoo/odoo/blob/7ecd9413/odoo/addons/base/i18n/fr.po#L7233-L7235
to (17.0)
https://github.com/odoo/odoo/blob/b1461d28/odoo/addons/base/views/ir_ui_view_views.xml#L126-L128
https://github.com/odoo/odoo/blob/b1461d28/odoo/addons/base/i18n/fr.po#L12087-L12089

The base term, en_US, is updated since the text matches but the
attributes of the translated terms, fr_FR for example, are not
updated. Later when the PO file is loaded in non-overwrite mode the
translated terms for fr_FR is not updated. This causes all sort of
issues during an upgrade for inline-translated terms -- like <span>.
More so since the recent change that converts domain-based attributes
into inline Python expressions.

In this patch we propagate modifiers attributes from inline-translated
items in the new base term into all translated terms when the base term
is updated. In that way we ensure the attributes are correct in all
languages even if later the loading of their corresponding PO file
doesn't update the term.

For a detailed example, let's see what happens when loading the view
above during an upgrade 16->17, right at the first load of the XML file
at https://github.com/odoo/odoo/blob/b1461d28/odoo/fields.py#L1864

(Pdb) p old_term
'<span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'soft\')]}">This view has no previous version.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'hard\')]}">This view is not coming from a file.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'other_view\')]}">You need two views to compare.</span>'
(Pdb) p closest_term
'<span invisible="reset_mode != \'soft\'">This view has no previous version.</span>\n                        <span invisible="reset_mode != \'hard\'">This view is not coming from a file.</span>\n                        <span invisible="reset_mode != \'other_view\'">You need two views to compare.</span>'
(Pdb) p translation_dictionary[old_term]
defaultdict(<class 'dict'>, {'fr_FR': '<span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'soft\')]}">Cette vue n\'a pas de version antérieure.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'hard\')]}">Cette vue ne provient pas d\'un fichier.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'other_view\')]}">Vous avez besoin de deux vues pour comparer.</span>'})

As we can see the new term will get an updated value for its invisible
attribute, while also removing attrs. The translated terms will be
still keep the old modifier though.
Now later when the fr_FR.po file is loaded we reach this point
https://github.com/odoo/odoo/blob/b1461d28/odoo/tools/translate.py#L1442

(Pdb) p term_en
'<span invisible="reset_mode != \'soft\'">This view has no previous version.</span>\n                        <span invisible="reset_mode != \'hard\'">This view is not coming from a file.</span>\n                        <span invisible="reset_mode != \'other_view\'">You need two views to compare.</span>'
(Pdb) p translation_dictionary[term_en]
defaultdict(<function DeepDefaultDict at 0x7fc9640cdfc0>, {'fr_FR': '<span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'soft\')]}">Cette vue n\'a pas de version antérieure.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'hard\')]}">Cette vue ne provient pas d\'un fichier.</span>\n                        <span attrs="{\'invisible\': [(\'reset_mode\', \'!=\', \'other_view\')]}">Vous avez besoin de deux vues pour comparer.</span>'})

Thus the translated values are NOT updated, keeping the wrong
modifiers. This is later fixed during the upgrade in a clumsy way.
C.f. the warnings like this one in runbot:

Incomplete conversion for view(id=77, lang=fr_FR) at
<span attrs="{'invisible': [('reset_mode', '!=', 'soft')]}">Cette vue n'a pas de version ant&https://github.com/odoo-dev/odoo/pull/233;rieure.</span>

Note that such warnings are gone in current PR CI.
The root issue here is that when the fr_FR translation is loaded the
terms are not updated due to a combination of factors:

1. The text content of the term didn't change
2. There is no override flag set for translations

Option 2 is not a valid option during upgrades because we want to keep
custom translations. We could instead of the current patch tweak how
option 1 works and perhaps make the closest term more restricted. This
would lead to the update of the whole translation though while the
actual issue here is just the modifiers. Moreover if the translations
are out of sync the translated terms will still keep the wrong values
that could still be essential for the correct functioning of the record
they belong too (view archs -- for example).

Finally this is a more extreme case (16.0):
https://github.com/odoo/enterprise/blob/1e63b4a8/sale_subscription/views/sale_order_views.xml#L142
In this case during the upgrade we fix the modifier value (refer to
runbot warning above -- it's the same script that fixes it) and set

invisible="(subscription_management == 'upsell') or (recurrence_id == False)"

for translations, which is wrong. The correct value is (17.0):

invisible="not plan_id or subscription_state == '7_upsell'"

https://github.com/odoo/enterprise/blob/530ba3ad/sale_subscription/views/sale_order_views.xml#L136

[robodoo]

.

[HydrionBurst]

It seems a very interesting feature. But

1. The transform cannot handle multi layer xml/html
   e.g. <small><span ...
2. I don't know if it would be too complicated to explain the expected result after translation sync. For your problem, can we just diasble the close matching feature when env.lang is None / a special context / upgrade?

[aj-fuentes]

It seems a very interesting feature. But

1. The transform cannot handle multi layer xml/html
   e.g. <span ...
2. I don't know if it would be too complicated to explain the expected result after translation sync. For your problem, can we just diasble the close matching feature when env.lang is None / a special context / upgrade?

1. iter will do a DFS, still to be 100% correct the transformer should check the full structure like parents matching etc. (as you suggested the full length is part of it). This was a first draft to see if the idea is workable ;) The premise in the current implementation is that if the nodes match in DFS fashion then the structure is OK (it misses the case where one of the iterators stops earlier -- can be fixed with strict=True for python3.10 -- but to ensure a perfectly matching structure needs a dedicated algorithm)

>>> from lxml import etree as et
>>> x=et.fromstring("<a><b/><c><d/></c></a>")
>>> [y.tag for y in x.iter()]
['a', 'b', 'c', 'd']

2. I can add details about the expected result in the commit message. The gist of it is that if we update the attribute in the base term it doesn't matter what translations have, they will be updated. Regarding the idea of using a context flag for upgrades, I think that's also OK for us. In theory we could patch this part
   https://github.com/odoo/odoo/blob/73bf5fbfc4b9e07667727cafcb9a314187c3e3c0/odoo/tools/translate.py#L1482-L1485
   to take the attributes from the base (en_US) term. I experimented with a similar patch there. I opted to move it to the current place because it seemed more "logical" to propagate the change when the base term was changed instead of checking each term at translations load time. With the current way if the attributes are instrumental for the correct functioning of the DB they will be propagated already when the base term is updated, even if the translations are out of sync.

[aj-fuentes]

Hello @HydrionBurst

I pushed a more detailed description with a full example. I also made the structural matching more robust and put the use of the new transformation behind install_mode=True context flag.

[rco-odoo]

That patch feels a bit invasive for a module upgrade issue. But it looks kind of okay to me. Did you benchmark it? Is the result worth the added overhead?

[aj-fuentes]

For info I'm working in running this in runbot DB with all modules installed + some improvements. I'll take into account the suggestions ;)

[aj-fuentes]

OK, so I pushed all your suggestions with some small details.

• Better name for the adapter
• Check if install mode and en_US
• No try/except for terms xml parser

With the previous version the run I did on a runbot db with all modules installed in an upgrade 16->17 with these languages installed en_US,ar_001,zh_CN,zh_HK,zh_TW,nl_BE,nl_NL,en_AU,en_CA,en_GB,en_IN,fr_BE,fr_CA,fr_CH,fr_FR,de_DE,de_CH,ja_JP,es_AR,es_CL,es_MX,es_VE,es_ES

• total calls to the adapter: 8471
• of which 1871 were via the ident adapter (i.e. do nothing -> fast)

I'm re-running the upgrade with the latest version of the code. I'll post the total time spent in the adapters. Total time of the upgrade is around 40min.

EDIT: on my last run, 31min total upgrade time of which just 0.3 seconds were spent in the adapters for the roughly 7.5K calls to the non-identity adapter.

[HydrionBurst]

Except the small question, others LGTM
Thanks for your great work!

[aj-fuentes]

@odoo/rd-security could you please approve this? The getattr calls are used in a standard way to get the bound methods from the current translate method.

[xmo-odoo]

could you please approve this? The getattr calls are used in a standard way to get the bound methods from the current translate method.

They're both completely unnecessary, you can use hasattr and a regular attribute lookup.

[rco-odoo]

@robodoo override=ci/security

Both calls to getattr are fine, since the attribute is static. They are used to access attributes field.translate.is_text and field.translate.term_adapter, which are optional on field.translate.

[rco-odoo]

@robodoo rebase-ff r+

[robodoo]

Merge method set to rebase and fast-forward.