[16.0][FIX] base: keep translations for null src while upgrade

[chienandalu]

The method update_translatable_fields is inspired in the core _get_translation_upgrade_queries method, which had a bug that has been recently addressed in odoo/odoo#168038

Also, we're avoiding removing any original ir_translation record so we can address any further issue easly without needing to redump the table from the former DB.

Let's use that core method with openupgradelib so we can be up to date with any further bugfix.

Depends on

• [FIX] migrate_translations_to_jsonb: avoid ir_translation terms cleanup openupgradelib#374

cc @Tecnativa TT49615

please review @pedrobaeza

fyi @hbrunn

[pedrobaeza]

Why this is not using existing Odoo method for building the query? There's https://github.com/OCA/openupgradelib/blob/5646c07e598c411ae77b630e19122efc410415ae/openupgradelib/openupgrade_160.py#L29 to do it in theory.

[chienandalu]

Mmm, yes, that's a good question 😅 I'll check it tomorrow

[pedrobaeza]

Did you see why the Odoo core method can't be used?

[hbrunn]

we can't use Odoo function directly because it needs a registry, while this function might be called before some field exists there

[pedrobaeza]

And why not doing it in post-migration?

[chienandalu]

I will make a performance comparison and see

[pedrobaeza]

 psycopg2.errors.DatatypeMismatch: column "title" is of type jsonb but expression is of type character varying
LINE 4:                 UPDATE utm_campaign SET title=name;
                                                      ^
HINT:  You will need to rewrite or cast the expression.

[chienandalu]

Yes, looks like it will be needed to fix some scripts

[pedrobaeza]

Why do it at end-migration? That way, if the translatable fields are involved in any computed operation, this will fail.

[chienandalu]

Yes I see. The issue here is that only the base models are populated in the environment env at the moment of post-migrate... so a lower level approach will be needed...

[pedrobaeza]

Yeah, that's true... so the core method is requiring that the model is loaded? It seems a bit restricting... Then we should turn back to the pure SQL approach that it was before. I think we have the answer to why this was done this way originally. Please document it for not going again this way in the future.

[chienandalu]

I think we can work with just the field, that we can access in post but adapt openupgradelib to avoid reliying on env

[pedrobaeza]

Seeing the core method, it relies on the registry model to get the table, so that's a non-go one...

[chienandalu]

Ok, but this would leave behind html translatable fields, which weren't covered by OU: https://github.com/odoo/odoo/blob/3f9eab47809da86eacf0e5a89914aea7531ab44f/odoo/tools/translate.py#L1593

My propose would be:

• Let's do in pre an initial pass
• And then in end do an additional pass on html translatable records to let them right

[pedrobaeza]

I still think that this is not correct, as if any computed field requires a translatable field, it will do the computation with empty strings.

[chienandalu]

Only translatable fields with a callable (html_translate and xml_translate) are left for end-migration. Anyway, are you thinking in some case in particular?

In the other hand, Odoo themselves must use this in end-migration if they rely in the registry...

[pedrobaeza]

OK, I see

@hbrunn can you give your final blessing?

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[hbrunn]

looks good to me. What do you mean in your comment?

[pedrobaeza]

Nevermind, they were just wrong thoughts: first of all, a computed stored field depending on translations is incorrect by definition. Second, the end-migration script is only for callable translations (translate=html_translate), so they don't have also computed fields depending on them. Merging then.