Skip to content

[17.0][OU-FIX] account: Use migrate_translations_to_jsonb to migrate translations from the field - #5355

Merged
OCA-git-bot merged 1 commit into
OCA:17.0from
Tecnativa:17.0-ou-fix-account-TT57778
Sep 8, 2025
Merged

[17.0][OU-FIX] account: Use migrate_translations_to_jsonb to migrate translations from the field#5355
OCA-git-bot merged 1 commit into
OCA:17.0from
Tecnativa:17.0-ou-fix-account-TT57778

Conversation

@victoralmau

Copy link
Copy Markdown
Member

Use migrate_translations_to_jsonb to migrate translations from the field

Related to 3d5ebac

Similar to

openupgrade_160.migrate_translations_to_jsonb(env, fields_spec)

Please @pedrobaeza can you review it?

@Tecnativa TT57778

@pedrobaeza pedrobaeza added this to the 17.0 milestone Aug 28, 2025
@pedrobaeza

Copy link
Copy Markdown
Member

Isn't this covered by the base module and the fix should be to not perform anything here?

@victoralmau

Copy link
Copy Markdown
Member Author

I don't quite understand what you mean by the base module.
Let me try to explain what is happening:

  • In v16, there was a non-translatable description field.
  • In v17, the invoice_label field was added as translatable.

Simply renaming it would not be enough, because the database column will not be in json (with the appropriate translations) and Odoo will auto-set the empty json column (without translations).

IMO, using the migrate_translations_to_jsonb method after renaming would be the right solution, although I think this needs to be thoroughly tested and confirmed before merging.

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, you have answered me indirectly. The key is that there's a renaming in between: description > invoice_label. That can be the reason why the method in the base module doesn't handle the transition to JSONB.

I wait for @hbrunn to confirm the change as well.

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this change? migrate_translations_to_jsonb calls convert_column_translatable internally, and only adds running the translation queries afterwards. But as you rightly pointed out the field isn't translatable in v16, so I wouldn't expect this to make any difference?

Which problem are you solving here?

@victoralmau

Copy link
Copy Markdown
Member Author

Oops, I think I forgot to clarify something important: the l10n_multilang module (no longer available in v17).
Anyone who has the l10n_multilang module installed will have this problem. I'm going to improve this code so that only those who have defined that field as translate need to apply this (this should apply to other fields in other models).

@hbrunn

hbrunn commented Aug 29, 2025

Copy link
Copy Markdown
Member

aaah yes, now that makes sense. No, just keep the code as it is, as the translation queries shouldn't do anything for the case where the field isn't translated.

Just add a comment that you deliberately call migrate_translations_to_jsonb instead of convert_column_translatable to support the case you have translate=True on the field in v16

@victoralmau
victoralmau force-pushed the 17.0-ou-fix-account-TT57778 branch from 060ca6c to 1cb34cc Compare August 29, 2025 11:27

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert to your previous version, because the way it's now, it's wrong and only doesn't fail tests because the multilang addon is installed in the CI database.

As written above, I think it's perfectly safe to just call migrate_translations_to_jsonb unconditionally

@victoralmau
victoralmau force-pushed the 17.0-ou-fix-account-TT57778 branch from 1cb34cc to b097c5d Compare September 8, 2025 07:04
@victoralmau
victoralmau requested a review from hbrunn September 8, 2025 07:22

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-5355-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

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). 🤖

@OCA-git-bot
OCA-git-bot merged commit 2a8b5d2 into OCA:17.0 Sep 8, 2025
4 checks passed
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 2a8b5d2. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants