Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADD] new module module_change_auto_install to configure auto installable modules by configuration #2091

Merged

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented May 25, 2021

DESCRIPTION

In odoo, by default some modules are marked as auto installable by the auto_install key present in the manifest.

  • This feature is very useful for "glue" modules that allow two modules to work together. (A typical example is sale_stock which allows sale and stock modules to work together).

  • However, Odoo SA also marks some modules as auto installable, even though this is not technically required. This can happen for modules the company wants to promote like iap, modules with a big wow effect like partner_autocomplete, or some modules they consider useful by default like account_edi. See the discussion: [] account_edi: Why account_edi is autoinstall? odoo/odoo#71190

This module allows to change by configuration, the list of auto installable modules, adding or removing some modules to
auto install.

CONFIGURE

  • Edit your odoo.cfg configuration file:

  • Add the module module_change_auto_install in the server_wide_modules list.

  • (optional) Add a new entry modules_auto_install_disabled to mark a list of modules as NOT auto installable.

  • (optional) Add a new entry modules_auto_install_enabled to mark a list of modules as auto installable. This
    feature can be usefull for companies that are hosting a lot of Odoo instances for many customers, and want some modules to be always installed.

Typical settings

server_wide_modules = web,module_change_auto_install
modules_auto_install_disabled = partner_autocomplete,iap,mail_bot,account_edi,account_edi_facturx,account_edi_ubl
modules_auto_install_enabled = web_responsive,web_no_bubble,base_technical_features,disable_odoo_online,account_menu

Run your instance and check logs. Modules that has been altered should be present in your log, at the load of your instance:

INFO db_name odoo.addons.module_change_auto_install.patch: Module 'iap' has been marked as not auto installable.
INFO db_name odoo.addons.module_change_auto_install.patch: Module 'mail_bot' has been marked as not auto installable.
INFO db_name odoo.addons.module_change_auto_install.patch: Module 'partner_autocomplete' has been marked as not auto installable.
INFO db_name odoo.addons.module_change_auto_install.patch: Module 'account_edi' has been marked as not auto installable.
INFO db_name odoo.addons.module_change_auto_install.patch: Module 'account_edi_facturx' has been marked as not auto installable.
INFO db_name odoo.addons.module_change_auto_install.patch: Module 'account_edi_ubl' has been marked as not auto installable.
INFO db_name odoo.modules.loading: 42 modules loaded in 0.32s, 0 queries (+0 extra)

INSTALL

You don't have to install this module. Just make it available in your addons path.

@pedrobaeza
Copy link
Member

I prefer to set auto_install=False in OCB instead of this, as this module should be installed/present, and it can be too late for avoiding the auto-installing.

@legalsylvain
Copy link
Contributor Author

legalsylvain commented May 25, 2021

Hi @pedrobaeza. Thanks for your quick review !

I prefer to set auto_install=False in OCB instead of this

Your proposal presents some limitations IMO :

  • it is static. Maybe some peoples consider that account_edi should NOT be auto-installable, and others think the opposite. Somtimes it depends on localization, or it can be matter of taste.
  • it is not working with EE auto installable modules. [] account_edi: Why account_edi is autoinstall? odoo/odoo#71190 is mentionning account_ponto that is not present in OCB
  • it is not working with non OCB instance, so I see this module as complementary
  • I see it less visible : with that module, we can see explicitly what are the differences between the official Odoo software, and the deployed instance

and it can be too late for avoiding the auto-installing.

Didn't understood that point. Could you elaborate ?

@legalsylvain legalsylvain force-pushed the 14.0-ADD-module_change_auto_install branch from 2d8d24e to 87bd7ed Compare May 25, 2021 10:50
@pedrobaeza
Copy link
Member

I mean that when you setup the module with those that you don't want, they are already populated in your DB.

@alexis-via
Copy link
Contributor

Maybe we should set this module as auto_install=True :) :) :)
I like the idea.

@legalsylvain legalsylvain force-pushed the 14.0-ADD-module_change_auto_install branch from 87bd7ed to df8f7ad Compare May 25, 2021 11:45
@legalsylvain
Copy link
Contributor Author

I mean that when you setup the module with those that you don't want, they are already populated in your DB.

Well, if you use this module on a existing database, it will not uninstall undesired modules (that is quite safe). If you create a database with the correct setting in your odoo.cfg it will work and prevent to install undesired modules. So it is exactly as using OCB branch AFAIK.

You don't have to install this module. it just has to be present in your addons path, and mentionned in the server_wide_modules value. The technic is similar to new the design present in OpenUpgrade modules. see OCA/OpenUpgrade#2440 (comment) for deeper information.
I added a install.rst file to explicit that the module does'nt have to be installed.
You don't have to install this module. Just make it available in your addons path.

Is it OK for you, regarding this point ?

@pedrobaeza
Copy link
Member

Yeah, thanks for the extra explanations.

@pedrobaeza pedrobaeza added this to the 14.0 milestone May 25, 2021
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Code review

@legalsylvain legalsylvain force-pushed the 14.0-ADD-module_change_auto_install branch 2 times, most recently from 00b5dc0 to 002a03e Compare May 25, 2021 12:16
Copy link
Contributor

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

Thank you @legalsylvain

I think the current way will apply the patch even if this module is not installed.

What about using a post_load way?

So, only if you have installed this module the patch will be apply

@legalsylvain
Copy link
Contributor Author

legalsylvain commented May 25, 2021

Hi @moylop260. Thanks for your review.

I think the current way will apply the patch even if this module is not installed.

  1. if the module is "active" only if it is installed, it will not work until it is installed, so during the installation, some undesired modules will be installed. The feature should be enabled very early.

So, only if you have installed this module the patch will be apply

  1. The patch will be applied only if it is mentioned in the server_wide_modules. Otherwise, nothing is patched, so it is quite safe and controllable.

  2. furthermore, If the module has to be installed, it is less powerfull. With the current implementation, you can change the odoo.cfg of your receipe to deploy your Odoo instances. And all the new databases will have the correct settings, without any actions. (any installations). It is valid for modules NOT to be installed, but also for module that we want to allways install. typical exemple : web_responsive, account_menu, base_technical_features. With the current implementation, you create an environment that will auto install correctly what you want.

So I don't think your proposal is a valid alternative and I don't think it will work. Feel free to make a test if you're not agree. I'll test your proposal with pleasure.

kind regards.

@moylop260
Copy link
Contributor

@legalsylvain

You are right!

I just realized that Odoo~14.0 is not pre-loading all the modules early even if it has a static folder (like previous versions)
So, you are right it will be enabled only if it is installed or using the --load parameter.

BTW I tested the hook and it is working in the same way (using load parameter or installed)

Check the following patch I just applied:

diff --git a/module_change_auto_install/__init__.py b/module_change_auto_install/__init__.py
index b946c62..2e653d6 100644
--- a/module_change_auto_install/__init__.py
+++ b/module_change_auto_install/__init__.py
@@ -1 +1 @@
-from . import patch
+from .patch import post_load
diff --git a/module_change_auto_install/__manifest__.py b/module_change_auto_install/__manifest__.py
index 7408083..268bb92 100644
--- a/module_change_auto_install/__manifest__.py
+++ b/module_change_auto_install/__manifest__.py
@@ -13,4 +13,5 @@
     "installable": True,
     "depends": ["base"],
     "license": "AGPL-3",
+    "post_load": "post_load",
 }
diff --git a/module_change_auto_install/patch.py b/module_change_auto_install/patch.py
index 861a12d..ce7962f 100644
--- a/module_change_auto_install/patch.py
+++ b/module_change_auto_install/patch.py
@@ -30,10 +30,11 @@ def _overload_load_information_from_description_file(module, mod_path=None):

     return res

-
+def post_load():
+    _logger.warning("modules.module.load_information_from_description_file patched apply")
-modules.module.load_information_from_description_file = (
-    _overload_load_information_from_description_file
-)
+    )
-modules.load_information_from_description_file = (
-    _overload_load_information_from_description_file
-)
+    )
+    modules.module.load_information_from_description_file = (
+        _overload_load_information_from_description_file
+    modules.load_information_from_description_file = (
+        _overload_load_information_from_description_file

And the following tests:

  • image

It is because the hook is loaded only after to import the module:

There is not real advantages using post_load hook for 14.0 but if someone backport this module to previous odoo versions we will need to be care about this since that Odoo is pre-loading the modules even if they are not installed for some cases.
And using post_load hook is not changing the current behaviour in 14.0

Do you think we should let it ready to be compatible for previous versions with minimal changes for backport?

@pedrobaeza
Copy link
Member

AFAIK, since v12 the static thing is not happening.

@legalsylvain
Copy link
Contributor Author

Hi @moylop260. Thanks for your answer.

BTW I tested the hook and it is working in the same way (using load parameter or installed)

Nice ! I didn't know how post_load was working exactly, and when it was executed. Thanks !

So,

  • both implementation seems to work on 14.0 (and should work on 12.0 and 13.0)
  • Both syntax are quite identical and has similar complexity
  • For V<12, the current implementation will monkey patch even if the module is not in the --load option, but will do nothing.(harmless)
  • For V<12, I don't know if the post_load works in a similar way

I will 2 / 3 extra tests, but if all is ok, I think that we can implement the post_load syntax, as it seems to be more "in the spirit of Odoo". I will keep you informed.

@StefanRijnhart : maybe that thread could interest you, with your "ample experience with dirty monkeypatching in Odoo" ;-)
(Ref : OCA/OpenUpgrade#2440 (comment))

kind regards.

@legalsylvain legalsylvain force-pushed the 14.0-ADD-module_change_auto_install branch from 002a03e to 5bf1227 Compare May 25, 2021 20:44
@legalsylvain
Copy link
Contributor Author

Hi @moylop260. After extra tests both implementation are working correctly. (also tested both on V12).
I so added the post_load function.
Is it ok for you ?

@legalsylvain legalsylvain force-pushed the 14.0-ADD-module_change_auto_install branch from 5bf1227 to 3224468 Compare May 25, 2021 20:57
Copy link
Contributor

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍
Thank you again

@moylop260
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-2091-by-moylop260-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4c2a1d8 into OCA:14.0 May 25, 2021
@OCA-git-bot
Copy link
Contributor

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

@legalsylvain
Copy link
Contributor Author

Hum... seing some recent PR in Odoo core like this one, odoo/odoo#75178 I think that this module will be ported in V15+ ;-)

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.

7 participants