Deprecate install_in_connector() #74
Conversation
…module with caching of the result
| def state_update(self, *args, **kwargs): | ||
| res = super(IrModuleModule, self).state_update(*args, **kwargs) | ||
| self.clear_caches() | ||
| return res |
There was a problem hiding this comment.
In module_uninstall(), in addons/base/module/module.py the state is set to 'uninstalled' without calling this method. Do you want to cover this scenario? I'm guessing the previous implementation didn't so it would not be a regression.
There was a problem hiding this comment.
You are right, I will fix this now, thank
|
CLA false alarm. Github handle is registered in the OCA instance and CLA's signed. Maybe someone added the Github handle to the OCA partner record just now? |
| @@ -45,7 +46,7 @@ | |||
| @openerp.api.returns('self', lambda value: value.id) | |||
| def create(self, vals): | |||
| record_id = create_original(self, vals) | |||
| if self.pool.get('connector.installed') is not None: | |||
| if is_module_installed(self.env, 'connector') is not None: | |||
There was a problem hiding this comment.
'if not is_module_installed()' is preferred.
Curious to see you passing self.env instead of self. Am I missing something?
There was a problem hiding this comment.
Oh, probably something to do with this being a static method you are importing.
There was a problem hiding this comment.
No, I just replace pool by env in the goal to get the cursor, If you prefer I can replace self.env by self
There was a problem hiding this comment.
If that works, it would be clearer. You are passing self.env as the first argument of https://github.com/OCA/connector/pull/74/files#diff-2700ffb2a55d3a936589a6fab4428330R31 called self, then access self.env. Again, I am wondering why this works at all.
There was a problem hiding this comment.
You, not see the good file.
At the top of the file you can read the import::
from .connector import is_module_installedthis function is an entry point, and this entry point is tested.
|
Great! Thanks |
| """ | ||
| _inherit = 'ir.module.module' | ||
|
|
||
| @ormcache(skiparg=1) |
There was a problem hiding this comment.
Wondering if it shouldn't be:
@api.model
@ormcache()
def is_module_installed(self, module_name):
....
Because the default decorator is @api.multi, that's why it you need to set skiparg=1 where the arg is ids, always empty in this case.
There was a problem hiding this comment.
Unfortunately, your solution is not working, because if you add both decorator, the method doesn't appear in the registry
There was a problem hiding this comment.
@api.model would be more correct but does not work with @ormcache. So okay to keep it like that.
…nnector exist before call the method define by module connector
…state 'installed' but 'to install'
|
I have now tested it, and I am afraid this is not working as expected. After installation, the cache still returns False. Only after a restart of Odoo, the method returns True. After deinstallation, any create results in an Exception 'ir.module.module' object has no attribute 'is_module_installed'. The same is true when I try to use other databases on the instance that don't have the module installed, so these are in fact unusable. Maybe it is better to test for the presence of an attribute on one of the modules that the connector module adds, like I suggested before? |
| name is the name of the module with a ``.intalled`` suffix: | ||
| ``{name_of_the_openerp_module_to_install}.installed``. | ||
| def is_module_installed(env, module_name): | ||
| if env.registry.get('connector.backend') is None: |
There was a problem hiding this comment.
Do you still need the other test? What happens if you just return (a boolean cast of) the value of env.registry.get('connector.backend')?
There was a problem hiding this comment.
the verification of 'connector.backend' is use only to test if connector is installed:
if not return False
if True, call the [ir.module.module].is_module_installed method, come from connector, to check if the module_name is installed
There was a problem hiding this comment.
So, the generic use case does not work otherwise we would not need to check the existence of the model. And the second test is made redundant by the first test for this specific use case.
There was a problem hiding this comment.
The first test is only to check that the connector module is installed, but we want make this test with another model too
|
Much better now, thanks! No more errors after deinstallation or on other databases. Only the installation of the module is still not registered properly. Odoo needs a restart to clear the cache. |
|
looks good, thanks |
|
Works perfect now, thanks! 👍 |
Because the check uses a fast-path for the connector so it does not reach the cache.
This code is never reached when we check the installation state with the high-level function is_module_installed for the module 'connector' because it directly looks for a model in the registry to check the installation state
|
@jssuzanne a test is failing. I corrected it on jssuzanne#1, could you merge it? |
Fix tests, Great thx for the fix
|
Green! |
Deprecate install_in_connector()
the sole presence of connector.py is all that is needed for the addon to be considered. See OCA/connector#74
the sole presence of connector.py is all that is needed for the addon to be considered. See OCA/connector#74
|
Hey @jssuzanne, Appreciation of efforts, |
See if we can remove install_in_connector() by checking in ir.module.module with caching of the result