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

Deprecate install_in_connector() #74

Merged
merged 15 commits into from Jun 3, 2015

Conversation

Projects
None yet
6 participants
@jssuzanne
Contributor

jssuzanne commented Jun 1, 2015

See if we can remove install_in_connector() by checking in ir.module.module with caching of the result

jssuzanne added some commits Jun 1, 2015

Show outdated Hide outdated connector/module.py
def state_update(self, *args, **kwargs):
res = super(IrModuleModule, self).state_update(*args, **kwargs)
self.clear_caches()
return res

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

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.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

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.

This comment has been minimized.

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

You are right, I will fix this now, thank

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

You are right, I will fix this now, thank

@StefanRijnhart

This comment has been minimized.

Show comment
Hide comment
@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

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?

Member

StefanRijnhart commented Jun 1, 2015

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?

Show outdated Hide outdated connector/producer.py
@@ -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:

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

'if not is_module_installed()' is preferred.

Curious to see you passing self.env instead of self. Am I missing something?

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

'if not is_module_installed()' is preferred.

Curious to see you passing self.env instead of self. Am I missing something?

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

Oh, probably something to do with this being a static method you are importing.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

Oh, probably something to do with this being a static method you are importing.

This comment has been minimized.

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

No, I just replace pool by env in the goal to get the cursor, If you prefer I can replace self.env by self

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

No, I just replace pool by env in the goal to get the cursor, If you prefer I can replace self.env by self

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

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.

@StefanRijnhart

StefanRijnhart Jun 1, 2015

Member

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.

This comment has been minimized.

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

You, not see the good file.
At the top of the file you can read the import::

     from .connector import is_module_installed

this function is an entry point, and this entry point is tested.

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

You, not see the good file.
At the top of the file you can read the import::

     from .connector import is_module_installed

this function is an entry point, and this entry point is tested.

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Jun 1, 2015

Member

Great! Thanks
👍

Member

guewen commented Jun 1, 2015

Great! Thanks
👍

Show outdated Hide outdated connector/module.py
"""
_inherit = 'ir.module.module'
@ormcache(skiparg=1)

This comment has been minimized.

@guewen

guewen Jun 1, 2015

Member

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.

@guewen

guewen Jun 1, 2015

Member

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.

This comment has been minimized.

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

Unfortunately, your solution is not working, because if you add both decorator, the method doesn't appear in the registry

@jssuzanne

jssuzanne Jun 1, 2015

Contributor

Unfortunately, your solution is not working, because if you add both decorator, the method doesn't appear in the registry

This comment has been minimized.

@guewen

guewen Jun 1, 2015

Member

@api.model would be more correct but does not work with @ormcache. So okay to keep it like that.

@guewen

guewen Jun 1, 2015

Member

@api.model would be more correct but does not work with @ormcache. So okay to keep it like that.

jssuzanne added some commits Jun 2, 2015

Fix is_module_installed function, check if one model of the module co…
…nnector exist before call the method define by module connector
@StefanRijnhart

This comment has been minimized.

Show comment
Hide comment
@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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?

Member

StefanRijnhart commented Jun 2, 2015

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?

Show outdated Hide outdated connector/connector.py
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:

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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')?

@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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')?

This comment has been minimized.

@jssuzanne

jssuzanne Jun 2, 2015

Contributor

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

@jssuzanne

jssuzanne Jun 2, 2015

Contributor

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

This comment has been minimized.

@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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.

@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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.

This comment has been minimized.

@jssuzanne

jssuzanne Jun 2, 2015

Contributor

The first test is only to check that the connector module is installed, but we want make this test with another model too

@jssuzanne

jssuzanne Jun 2, 2015

Contributor

The first test is only to check that the connector module is installed, but we want make this test with another model too

@StefanRijnhart

This comment has been minimized.

Show comment
Hide comment
@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

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.

Member

StefanRijnhart commented Jun 2, 2015

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.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 2, 2015

Coverage Status

Coverage decreased (-0.0%) to 75.14% when pulling 378edf9 on jssuzanne:8.0 into 9e66233 on OCA:8.0.

coveralls commented Jun 2, 2015

Coverage Status

Coverage decreased (-0.0%) to 75.14% when pulling 378edf9 on jssuzanne:8.0 into 9e66233 on OCA:8.0.

@bguillot

This comment has been minimized.

Show comment
Hide comment
@bguillot

bguillot Jun 2, 2015

Contributor

looks good, thanks
👍

Contributor

bguillot commented Jun 2, 2015

looks good, thanks
👍

@StefanRijnhart

This comment has been minimized.

Show comment
Hide comment
@StefanRijnhart

StefanRijnhart Jun 2, 2015

Member

Works perfect now, thanks! 👍

Member

StefanRijnhart commented Jun 2, 2015

Works perfect now, thanks! 👍

guewen added some commits Jun 3, 2015

Test the cache invalidation on 'base' instead of 'connector'
Because the check uses a fast-path for the connector so it does not reach the
cache.
Remove useless trick in is_module_installed
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
@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Jun 3, 2015

Member

@jssuzanne a test is failing. I corrected it on jssuzanne#1, could you merge it?

Member

guewen commented Jun 3, 2015

@jssuzanne a test is failing. I corrected it on jssuzanne#1, could you merge it?

Merge pull request #1 from guewen/is_installed-fixes
Fix tests, Great thx for the fix
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 3, 2015

Coverage Status

Coverage decreased (-0.01%) to 75.12% when pulling 8406a62 on jssuzanne:8.0 into 9e66233 on OCA:8.0.

coveralls commented Jun 3, 2015

Coverage Status

Coverage decreased (-0.01%) to 75.12% when pulling 8406a62 on jssuzanne:8.0 into 9e66233 on OCA:8.0.

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Jun 3, 2015

Member

Green!
Thanks!

Member

guewen commented Jun 3, 2015

Green!
Thanks!

guewen added a commit that referenced this pull request Jun 3, 2015

Merge pull request #74 from jssuzanne/8.0
Deprecate install_in_connector()

@guewen guewen merged commit 6e3ab38 into OCA:8.0 Jun 3, 2015

2 checks passed

ci/runbot runbot build 2948199-74-8406a6 (runtime 34s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gurneyalex added a commit to gurneyalex/connector-magento that referenced this pull request Aug 12, 2015

remove deprecated call to install_in_connector
the sole presence of connector.py is all that is needed for the addon to be
considered.

See OCA/connector#74

guewen added a commit to guewen/connector-magento that referenced this pull request Aug 17, 2015

remove deprecated call to install_in_connector
the sole presence of connector.py is all that is needed for the addon to be
considered.

See OCA/connector#74

guewen added a commit to guewen/connector-magento that referenced this pull request Aug 17, 2015

@oca-clabot

This comment has been minimized.

Show comment
Hide comment
@oca-clabot

oca-clabot Feb 24, 2016

Hey @jssuzanne,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

oca-clabot commented Feb 24, 2016

Hey @jssuzanne,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

blaggacao pushed a commit to xoes-oca/magentoerpconnect that referenced this pull request Dec 7, 2017

blaggacao pushed a commit to xoes-oca/customize_example that referenced this pull request Dec 7, 2017

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