Skip to content

[20.0][IMP] openupgrade_scripts: fill full apriori - #6077

Open
MiquelRForgeFlow wants to merge 1 commit into
OCA:20.0from
ForgeFlow:20.0-imp-fill-apriori
Open

MiquelRForgeFlow wants to merge 1 commit into
OCA:20.0from
ForgeFlow:20.0-imp-fill-apriori

Conversation

@MiquelRForgeFlow

Copy link
Copy Markdown
Contributor

.

@MiquelRForgeFlow MiquelRForgeFlow added this to the 20.0 milestone Nov 3, 2026
@OCA-git-bot OCA-git-bot added mod:openupgrade_scripts Module openupgrade_scripts series:20.0 labels Nov 3, 2026
Comment thread openupgrade_scripts/apriori.py Resolved
"crm.lead2opportunity.partner": "crm.lead2opportunity.partner.mass",
"hr.leave.type": "hr.work.entry.type",
"ir.model.access": "ir.access",
"ir.rule": "ir.access",

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.

well, I think this will cause messy analysis files. Without this, we get all ir.rule records in one block where we can write NOTHING TO DO and are done with it. With this merge, we get them spread out between all the other ir.access records, and have to deal with them line by line.

that's different from ir.model.access, where v20 usually reuses the xmlid for the ir.access entry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A priori, I disagree. Why do you have to handle them line by line? What do you mean? I think we only should care if they have changed from noupdate = 1 to 0, and vice versa, which the analysis already provides. And also, if the xmlid name changes, which then the analysis already provides. We should not care in that part if the "ir.access" xmlid name comes from "ir.model.access" or "ir.rule", right? The migration scripts to handle the migration of "ir.model.access" and "ir.rule" don't care about the explicit listed xmlids in the analysis file.

With this merge, we get them spread out between all the other ir.access record

Have you tested how it looks merged? Maybe with an example to compare, a visual thing, we could check which option is better.

@FerDeNetlab FerDeNetlab left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Drafted with AI assistance (Claude Opus 5.5), reviewed by me.

"account_add_gln": "account",
"account_peppol_response": "account_peppol",
"base_iban": "account",
"base_vat": "base",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

base_vat was split: the format checks went to base, but VIES (vies_valid, vat_check_vies, the cron) went to the new l10n_eu_account_vies. Merging into base drops VIES on databases that use it; renaming it (what we did locally) installs VIES on every database that had base_vat, also outside the EU. Maybe merge into base and install l10n_eu_account_vies in a script when a company has vat_check_vies? What would you prefer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe merge into base and install l10n_eu_account_vies in a script when a company has vat_check_vies?

Exactly.

Comment thread openupgrade_scripts/apriori.py Resolved
Comment thread openupgrade_scripts/apriori.py Resolved
@MiquelRForgeFlow
MiquelRForgeFlow force-pushed the 20.0-imp-fill-apriori branch 2 times, most recently from b0d8189 to 8e0a891 Compare November 4, 2026 12:48
@mauro2306

Copy link
Copy Markdown

+1 for installing l10n_eu_account_vies from a script when a company has vat_check_vies: all three of our production databases (Belgium) have it enabled.

The same kind of split exists for mail, and it is not covered yet. In 20.0, mail.tracking.value moved from mail to the new core module mail_tracking, which is neither auto_install nor a dependency of account. On a database where nothing pulls it in, the model is left without a module, so as far as I can tell the table, i.e. the field history shown in the chatter, would be dropped with the orphaned ir.model at the end of the update. On our side that's 6,624 rows on one database, invoices included.

apriori can't express a split, so I guess this belongs in the mail migration: mark mail_tracking for install whenever mail is installed, and move the model's xmlids with update_module_moved_models().

One interaction with #6071, which renames OCA's mail_tracking to mail_recipient_tracking: the rename has to happen before the core module is marked for install. Otherwise, a database that had the OCA module will look as if it already had the core mail_tracking.

@mauro2306

Copy link
Copy Markdown

Follow-up with a tested fix, and a correction. I tried this on a local bench (#6068 at 1faabaf, which includes #6046, plus the change of OCA/openupgradelib#468), migrating mail and its dependencies.

Correction: the table is not dropped, since openupgrade_framework never drops tables or columns. But without a fix, mail_tracking stays uninstalled and the end of the update deletes 24 records (the model's fields, the action, the menu, the views). mail.message has no tracking_value_ids any more, so the history no longer shows anywhere.

What works, in mail's pre-migration:

  • openupgrade.update_module_moved_models(env.cr, "mail.tracking.value", "mail", "mail_tracking"), plus rename_xmlids for the action, the menu and both views;
  • button_install() on mail_tracking when it is uninstalled. The loading loop of the same update picks it up before _process_end, and the auto_install bridge mail_tracking_sms follows.

With that, the tracking values are kept, the action, menu and views keep their ids, and the only field deleted is currency_id, as in the analysis. Tests have to live in scripts/mail_tracking/tests, since mail's own tests run before mail_tracking is loaded.

mail also needed one more thing to load at all: v20 adds mail.channel_member_channel_admin_partner_admin (noupdate), which fails on discuss_channel_member_partner_unique because the admin is already a member of that channel in v19, through the group auto-subscription, without an xmlid. Attaching the xmlid to the existing membership in the pre-migration fixes it.

I can open this as a PR for mail (pre-migration plus tests), or hand it to whoever takes mail, whichever you prefer.

@FerDeNetlab

Copy link
Copy Markdown

@mauro2306 thanks, I can confirm it on our bench: mail_tracking stays uninstalled and the action, menu, views and fields of mail.tracking.value are deleted, the 332 tracking values are left without any UI.

Please go ahead with the mail PR, I'll review it. Our branch has the same fix for the admin's membership of #admin, and one more thing you may need: precreating mail.activity.phone and role_id, whose computes crashed while loading mail on databases with activities on account.move (FerDeNetlab@9c57225). Take whatever is useful, I won't open a PR for mail.

Drafted with AI assistance (Claude Opus 5.5), reviewed by me.

This branch has not been deployed

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

Labels

mod:openupgrade_scripts Module openupgrade_scripts series:20.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants