odoo
[IMP] remove and uninstall modules and themes recursively
Current implementation won't uninstall/remove dependencies. This can easily lead to a broken database, and is an unexpected and important divergence compared to the normal ORM uninstallation or removal process.
@moduon MT-7110
@aj-fuentes would you please review this one? I think it is a very handy enhancement.
This option is not as useful as it looks --I'd say it is dangerous. In many cases when we remove a module all modules that depend on it need to be adapted, instead of removed. Some modules are even custom for which we do not have the code readily available. I think the best approach is to deal with this on a case by case basis, a loop over pre-computed modules --as you propose here-- in client code is better IMO.
In this case the deviation of the ORM behavior is intentional: we want to remove a module without removing the modules that depend on it.
For the ultimate opinion we need to summon @KangOl
I would also prefer not to change those methods. It's too dangerous. It's always better to be explicit about which modules to uninstall/remove. Especially for removes, not all downstream dependencies should also be removed. Some may simply lose that dependency or even be merged into one of their parent.
For such cases, it is always better to be explicit.
I understand how dangerous it would be for your migration process, where you get many databases with extra modules as inputs, and you have to return them respecting all data that doesn't belong to Odoo CE/EE modules.
For that reason, all methods keep the old behavior with the default value with_dependencies=False. So you should notice no difference after merging (apart from the fact that these methods now return something, but that's not much likely to cause any big issues).
Downstream, instead, our use case is different. Different databases for different customers might have different modules, and listing all of them explicitly is not practical. We upload to you a database that has a set of addons and we download a new database that is 99% of the times broken (because you didn't touch community or private addons (and thanks for that!)). So, we have to reupgrade that database with after-upgrade scripts.
In those scripts, we never want a broken database as a final output. Therefore, listing dependencies is unpractical: if a module disappears, its dependencies should too, always. Otherwise, Odoo will break somewhere.
This PR makes the library not only useful for you, but also for downstream partners like us.
If you want, I can enhance the docstrings explaining the dangers of removing dependencies recursively, depending on the use case. Would that work for you?
I believe this library is very useful for you without the proposed patch
Of course! Sorry, I meant to make it more useful. It is very useful indeed.
We don't have a use case for it. Supporting this in such an important and used function is not something we'd want to do.
I understand. At least this little new function should help for those that need it downstream.
Please review again.
Do you have a compelling case for other states than what we consider "installed states" during the upgrade?
Yes. I want to remove (not only uninstall) a module and its dependencies (installed or not).
Yes. I want to remove (not only uninstall) a module and its dependencies (installed or not).
In principle I'm OK with it. I guess you need this to pickup all modules that were depending on a possibly removed module. In this case your original query without any state check is better (otherwise the caller would need to provide a list of all states). You could OTOH return pairs (module_name, module_state) --instead of just module_name-- as the query result. Since we don't have a need for this new tool I cannot form a strong opinion :/ It seems useful (perhaps even important) for the caller to know the module state --before calling remove_module for example.
Well, as the scope of the change keeps getting smaller, maybe its utility too... Let me close this one and I'll try to solve the need by other means.
Thanks anyway!