[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatturapa #3595

SirTakobi · 2022-10-20T08:33:06Z

From version 14.0, module l10n_it_fatturapa conflicts with l10n_it_edi: https://github.com/OCA/l10n-italy/blob/bb780459361ad45900ccfb74aea22ca99b958d75/l10n_it_fatturapa/__manifest__.py#L16.

We assume that if l10n_it_fatturapa is installed, then l10n_it_edi is not to be installed: this is the accepted solution when this same error happens while installing the modules manually, see https://github.com/OCA/l10n-italy/blame/a5bb1352e155dd1e5d81feea5e631ad76543559e/l10n_it_fatturapa/README.rst#L70-L75.

Without this fix, when upgrading from 13.0 to 14.0, the following is raised during the migration of base:

odoo.exceptions.UserError: I moduli "ITA - Fattura elettronica - Base" e "Italy - E-invoicing" non sono compatibili.

(It means that mentioned modules are incompatible)

OCA-git-bot · 2022-10-20T08:33:19Z

Hi @pedrobaeza, @MiquelRForgeFlow, @StefanRijnhart,
some modules you are maintaining are being modified, check this out!

legalsylvain

hi. I don't understand why you don't simply write :

if openupgrade.is_module_installed(cr, "l10n_it_fatturapa"):
        openupgrade.logged_query(
            cr,
            """
            UPDATE ir_module_module
            SET state='to remove'
            WHERE name = 'l10n_it_edi'""",
        )

(eventually with an extra where clause where state in ['to install', 'installed', to upgrade'] ...

I mean what you want to ensure is : if l10n_it_fatturapa is installed, then l10n_it_edi is not. Right ?

SirTakobi · 2022-10-20T08:54:46Z

hi. I don't understand why you don't simply write :
if openupgrade.is_module_installed(cr, "l10n_it_fatturapa"):
        openupgrade.logged_query(
            cr,
            """
            UPDATE ir_module_module
            SET state='to remove'
            WHERE name = 'l10n_it_edi'""",
        )
(eventually with an extra where clause where state in ['to install', 'installed', to upgrade'] ...

I mean what you want to ensure is : if l10n_it_fatturapa is installed, then l10n_it_edi is not. Right ?

Thanks for having a look!
Almost right: I want to ensure that if l10n_it_fatturapa is installed and l10n_it_edi would auto_install, then l10n_it_edi is not installed.
That is why in this PR, the code only edits the state of l10n_it_edi when it would be installed automatically (i.e. its dependencies are installed).

Your proposal would work too, but in my opinion it would work in too many cases: it would edit the state of l10n_it_edi even when it will not be auto installed.

Maybe it is worth mentioning that l10n_it_fatturapa does not depend on l10n_it.

legalsylvain · 2022-10-20T09:31:03Z

Your proposal would work too, but in my opinion it would work in too many cases: it would edit the state of l10n_it_edi even when it will not be auto installed.

I don't think so. here, we are the base pre-migration step. so the update of the module to install / update / removed has been done. 2 cases :

the state of l10n_it_edi is uninstalled -> nothing to do. (certainly because dependencies are not all installed).
the state of l10n_it_edi is 'to install', 'to upgrade' and so we have to set 'to remove'.

don't you think ? (I'm not sure what I say). ;-)

SirTakobi · 2022-10-20T10:41:07Z

don't you think ? (I'm not sure what I say). ;-)

As far as I can see in #3595 (review), your proposal is to uninstall l10n_it_edi whenever l10n_it_fatturapa is installed, so reviewing the two cases:
First case

the state of l10n_it_edi is uninstalled -> nothing to do. (certainly because dependencies are not all installed).

Right, there is nothing to do here, but:

your proposal would set l10n_it_edi as 'to_remove' anyway, if l10n_it_fatturapa is installed
this PR's code does nothing.

So I think this PR's code doesn't have to be changed.

Second case

the state of l10n_it_edi is 'to install', 'to upgrade' and so we have to set 'to remove'.

This happens both with or without your proposal.

Maybe you are suggesting to check if both l10n_it_edi and l10n_it_fatturapa are installed? Since you say that

the update of the module to install / update / removed has been done

legalsylvain · 2022-10-20T11:01:11Z

yes that was the sense of the text "(eventually with an extra where clause where state in ['to install', 'installed', to upgrade'] ..."

but in facts, I wonder if it's a problem of Open Upgrade.

I mean, if in version 14, you make an update all of the database with l10n_it_fatturapa installed, it will try to install also l10n_it_edi if it is auto_install to True. Right ?

SirTakobi · 2022-10-20T13:00:06Z

yes that was the sense of the text "(eventually with an extra where clause where state in ['to install', 'installed', to upgrade'] ..."

Oh I thought that was for l10n_it_fatturapa! Now I have changed the code, can you please check if that's better?

but in facts, I wonder if it's a problem of Open Upgrade.

I mean, if in version 14, you make an update all of the database with l10n_it_fatturapa installed, it will try to install also l10n_it_edi if it is auto_install to True. Right ?

The l10n_it_edi was installed anyway despite the pre-migration, so it is right that an update all raised the error again.
Now I have added an explicit uninstall in end-migration so it effectively uninstalled in the end, and an update all does not raise the error again.

l10n_it_edi is tricky because it not auto_install as I thought it was, but installs anyway since odoo/odoo#74305.

legalsylvain

LGTM.

legalsylvain · 2022-10-20T17:56:19Z

Note, you can use the module module_change_auto_install on your production (and even in openupgrade process) to switch off / on auto_install for a list of modules.

See : OCA/server-tools#2091

openupgrade_scripts/scripts/base/14.0.1.3/pre-migrate.py

…urapa

pedrobaeza · 2022-10-24T18:31:02Z

openupgrade_scripts/scripts/base/14.0.1.3/end-migration.py

+            ],
+            limit=1,
+        )
+        it_edi_module.button_uninstall()


Why doing 2 times the same?

Thanks for having a look!

Mark as 'to_remove' to avoid raising a conflict; it will be installed anyway,
but we will uninstall it for good in end-migration.

I wrote it in a comment in the code in https://github.com/OCA/OpenUpgrade/pull/3595/files#diff-aec3724a38e5c01c0ea5b62012c57a9145be5ab7058f222be86607cb7c50301fR93-R94.

Yeah, but why it will be installed? You are saying in other comment that is not auto-install. And if it's installed by another module, uninstalling this one will uninstall the other module as well.

Just summarizing a bit and adding a few more details:
In 13.0 DB I have:

name state

l10n_it_edi installed

l10n_it_fatturapa to upgrade

When I upgrade to 14.0 without this PR, the error in the description is raised:

odoo.exceptions.UserError: I moduli "ITA - Fattura elettronica - Base" e "Italy - E-invoicing" non sono compatibili.

If I include only the pre-migrate part of this PR, the migration concludes successfully BUT l10n_it_edi and l10n_it_fatturapa are both installed; that is why I also added the end-migration to uninstall the module.

Now back to your question:

Yeah, but why it will be installed?

l10n_it_edi is added to the graph of modules to be loaded in https://github.com/odoo/odoo/blob/a46d20c89c24a4e60faef09cf8ad7721bd29d6cc/odoo/modules/loading.py#L341-L345:

cr.execute("SELECT name from ir_module_module WHERE state IN %s" ,(tuple(states),)) module_list = [name for (name,) in cr.fetchall() if name not in graph] if not module_list: break graph.add_modules(cr, module_list, force)

because it is called including the state to_remove in https://github.com/odoo/odoo/blob/a46d20c89c24a4e60faef09cf8ad7721bd29d6cc/odoo/modules/loading.py#L455-L457:

processed_modules += load_marked_modules(cr, graph, ['installed', 'to upgrade', 'to remove'], force, status, report, loaded_modules, update_module, models_to_check)

Odoo thinks that l10n_it_edi has to be updated because the following condition is True:

needs_update = ( hasattr(package, "init") or hasattr(package, "update") or package.state in ("to install", "to upgrade") )

(from https://github.com/odoo/odoo/blob/a46d20c89c24a4e60faef09cf8ad7721bd29d6cc/odoo/modules/loading.py#L161-L165)
more precisely, hasattr(package, "update") is True because it is assigned in https://github.com/odoo/odoo/blob/cbab786eb2e153eb7eefbd994cb261fa96e56800/odoo/modules/graph.py#L85-L87:

for kind in ('init', 'demo', 'update'): if package in tools.config[kind] or 'all' in tools.config[kind] or kind in force: setattr(node, kind, True)

More precisely, package in tools.config[kind] is True.

Thank you for the detailed explanation. Wow, such a difficult thing to handle...

pedrobaeza · 2022-10-24T18:31:29Z

Isn't better to switch the auto-install to False in OCB and use that source for the migrations?

SirTakobi · 2022-10-25T07:14:46Z

Isn't better to switch the auto-install to False in OCB and use that source for the migrations?

As I wrote earlier (#3595 (comment)), l10n_it_edi is not auto_install:

l10n_it_edi is tricky because it not auto_install as I thought it was, but installs anyway since odoo/odoo#74305.

OCA-git-bot · 2022-10-25T09:00:02Z

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

SirTakobi force-pushed the 14.0-conflicting_edi branch from 581d165 to 140c7b3 Compare October 20, 2022 08:35

legalsylvain reviewed Oct 20, 2022

View reviewed changes

SirTakobi force-pushed the 14.0-conflicting_edi branch 2 times, most recently from 75be281 to 3357e81 Compare October 20, 2022 12:55

SirTakobi marked this pull request as ready for review October 20, 2022 13:45

SirTakobi requested a review from legalsylvain October 20, 2022 13:45

legalsylvain approved these changes Oct 20, 2022

View reviewed changes

MiquelRForgeFlow reviewed Oct 21, 2022

View reviewed changes

openupgrade_scripts/scripts/base/14.0.1.3/pre-migrate.py Show resolved Hide resolved

[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatt…

d09e091

…urapa

SirTakobi force-pushed the 14.0-conflicting_edi branch from 3357e81 to d09e091 Compare October 21, 2022 09:17

SirTakobi requested a review from MiquelRForgeFlow October 21, 2022 09:17

MiquelRForgeFlow approved these changes Oct 21, 2022

View reviewed changes

OCA-git-bot added the approved label Oct 21, 2022

MiquelRForgeFlow requested a review from pedrobaeza October 21, 2022 09:30

pedrobaeza added this to the 14.0 milestone Oct 24, 2022

pedrobaeza reviewed Oct 24, 2022

View reviewed changes

OCA-git-bot added the ready to merge label Oct 25, 2022

pedrobaeza merged commit 0d5efe7 into OCA:14.0 Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatturapa #3595

[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatturapa #3595

SirTakobi commented Oct 20, 2022

OCA-git-bot commented Oct 20, 2022

legalsylvain left a comment

SirTakobi commented Oct 20, 2022

legalsylvain commented Oct 20, 2022

SirTakobi commented Oct 20, 2022

legalsylvain commented Oct 20, 2022

SirTakobi commented Oct 20, 2022

legalsylvain left a comment

legalsylvain commented Oct 20, 2022

pedrobaeza Oct 24, 2022

SirTakobi Oct 25, 2022

pedrobaeza Oct 25, 2022

SirTakobi Oct 25, 2022

pedrobaeza Oct 31, 2022

pedrobaeza commented Oct 24, 2022

SirTakobi commented Oct 25, 2022

OCA-git-bot commented Oct 25, 2022

[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatturapa #3595

[FIX] base: Uninstall l10n_it_edi to avoid conflict with l10n_it_fatturapa #3595

Conversation

SirTakobi commented Oct 20, 2022

OCA-git-bot commented Oct 20, 2022

legalsylvain left a comment

Choose a reason for hiding this comment

SirTakobi commented Oct 20, 2022

legalsylvain commented Oct 20, 2022

SirTakobi commented Oct 20, 2022

legalsylvain commented Oct 20, 2022

SirTakobi commented Oct 20, 2022

legalsylvain left a comment

Choose a reason for hiding this comment

legalsylvain commented Oct 20, 2022

pedrobaeza Oct 24, 2022

Choose a reason for hiding this comment

SirTakobi Oct 25, 2022

Choose a reason for hiding this comment

pedrobaeza Oct 25, 2022

Choose a reason for hiding this comment

SirTakobi Oct 25, 2022

Choose a reason for hiding this comment

pedrobaeza Oct 31, 2022

Choose a reason for hiding this comment

pedrobaeza commented Oct 24, 2022

SirTakobi commented Oct 25, 2022

OCA-git-bot commented Oct 25, 2022