Deprecate base_contact

[yajo]

This fixes #119.

The new modules:

• partner_contact_personal_information_page: The new base.
• partner_contact_birthdate
• partner_contact_in_several_companies
• partner_contact_nationality

The old base_contact warns deprecation everywhere.

Old translations preserved where possible with credits.

After merging we can start again with #118.

[bwrsandman]

Could you use my savoirfairelinux email? sandy.carter@savoirfairelinux.com while you're there

[dreispt]

This is a case study for me.
I'm curious: how will this behave in existing installations, when doing an upgrade? Is there risk of losing data?

[coveralls]

Coverage increased (+0.83%) to 52.31% when pulling 6b9b31f on grupoesoc:deprecate-base_contact into 2b27e6c on OCA:8.0.

[yajo]

I'll do some checks and let you know.

[yajo]

I did this:

• Create an empty demo database.
• Install base_contact.
• Allow technical features for admin.
• Create a contact.
• Put there birthdate, nationality and some job positions.
• Shutdown the server.
• git checkout deprecate-base_contact.
• Start the server.
• Configuration > Update modules list.
• Manually upgrade module base_contact.
• Checked that all data from that contact is OK (and it was).
• Change birthdate, nationality and job positions.
• Save.
• Record updated OK.

The bold lines are the necessary steps for sysadmins to follow, and nothing was broken.

[hbrunn]

@yajo great initiative, but you definitely need migration scripts for that. At the very least, you need to migrate the xmlids of the fields's ir.model.fields record to the appropriate new module (see OCA/OpenUpgrade#303 for discussion, I just stumpled upon this one yesterday). Otherwise, odoo will drop the columns if you deinstall the then obsolete base_contact.

[yajo]

Ignore last update, it's just a rebase.

@hbrunn Thanks but I don't understand you very well... Can you explain better please? And if you have any docs (or at least examples) for that, they'll be very appreciated.

[hbrunn]

@yajo when you deinstall a module, odoo unlinks all records that have an xml id (look at the table ir_model_data) that were created by this module. This happens automatically on module installation. When a module adds a field, an entry in ir_model_fields is created and a record in ir_model_data pointing to this record.

For documentation, use the source: https://github.com/odoo/odoo/blob/8.0/openerp/models.py#L356

Now on module deinstallation, odoo unlinks the ir.model.fields entry, but this results in dropping the column: https://github.com/odoo/odoo/blob/8.0/openerp/addons/base/ir/ir_model.py#L355 - that's a good thing, because uninstalled modules don't leave stuff behind this way.

But in this case, the xmlid belongs to the module base_contact, so if you uninstall that, you lose the columns now living in the other modules.

To fix that, create a file base_contact/migrations/8.0.2.0/pre-migrate.py saying

def migrate(cr, version):
  cr.execute("update ir_model_data set module='the new module' where module='base_contact' and name in ('the generated xmlid name(s) of the field(s) in question')")
  #repeat for all moved fields

This will run if you update base_contact from a version lower than 8.0.2.0 to this version. (For testing purposes, you can modify latest_version in ir_module_module to trigger this)

Please don't hesitate to ask if something is still unclear

[yajo]

Thanks, it's clear now. I also found https://github.com/odoo/odoo/blob/8ff7230e2d6d49e5b880fbee9589de1e8a78fb95/openerp/modules/migration.py#L38 useful.

I'm on it.

[coveralls]

Coverage increased (+0.63%) to 49.88% when pulling 0b56819 on grupoesoc:deprecate-base_contact into 302da89 on OCA:8.0.

[yajo]

@hbrunn Well spotted. After uninstalling, fields were removed. Last update fixes that. Thank you!

[dreispt]

@hbrunn Many thanks for the advice!

[yajo]

Rebased to see if that fixes the runbot builds.

[yajo]

Can anyone help me fixing the runbot error please? It seems to me like it's treating the deprecation warning as an error, but it's the expected behavior.

[bwrsandman]

Runbot is sending fails to github when it has a warning status

[yajo]

Warnings from runbot come from here, but it's a normal deprecation warning. Nothing to fix.

[bwrsandman]

That this will flash every time a server worker is launched even if the module isn't installed.

[yajo]

I'm not sure if that can be avoided...

[gurneyalex]

@yajo: can you replace your logging statement with the following code:

from warnings import warn
warn('base_contact: this module is deprecated. See README', DeprecationWarning)

[yajo]

@gurneyalex Doing that in my tests logs nothing. Are you sure you want me to push that?

Another solution is to downgrade the message to info, but IMHO it should be a warning.

[yajo]

I opted for transforming the warn in info to fix the checks.

[gurneyalex]

@yajo yes DeprecationWarnings are ignored by default (https://docs.python.org/2.7/library/warnings.html?highlight=warnings#warning-categories)

If you prefer having a WARNING log level (and I agree this is better than Info), you could check the hostname of the server. If it matches runbot.*.odoo-community.org then don't log. I can make sure that the runbot has an environment variable set (e.g. OCA_RUNBOT) and you can use this rather than the fqdn for the test. And the best way of doing this is to use code such as the following:

in the init.py of your module:

import logging
import os
logger = logging.getLogger(__name__ + ".deprecated")
if os.environ.get("OCA_RUNBOT"):
    logger.setLevel(logging.ERROR)
logger.warning("Module %r is DEPRECATED. See %s/README.", 
                         __name__, 
                         os.path.dirname(__file__))

(caution, I did not test this, there may by typos).

Addind '.deprecated' is important so that the loggers in the other files of the modules are not impacted.

[yajo]

I removed the name part because anyway the logger already logs that automatically. Let's see if this time it passes the checks.

[coveralls]

Coverage increased (+0.63%) to 49.88% when pulling 602ca74 on grupoesoc:deprecate-base_contact into f99b1d1 on OCA:8.0.

[gurneyalex]

@yajo I did not have time to update the runbot, so it will fail.

[yajo]

Can you explain yourself better, please? Sorry, I did not understand.

[gurneyalex]

fixed typo in my comment.

[yajo]

Then what should I do now?

[yajo]

@gurneyalex Have you updated it already?

[pedrobaeza]

OK, finally merging

[lepistone]

Hi,

sorry I arrive late in the discussion and this is now merged, but I think the warning should be moved somewhere that is executed only if the module is installed. I'm unsure where though. Maybe auto_init in a model?

Thanks!

[dreispt]

@hbrunn Just checking:

This will run if you update base_contact from a version lower than 8.0.2.0 to this version. (For testing purposes, you can modify latest_version in ir_module_module to trigger this)

The module version was bumped to 2.0.0, which is equivalent to 8.0.2.0.0.
This is the version the migration refers to. And the extra .0 is OK.
Correct?

[hbrunn]

Good you asked, I'm not so sure: https://github.com/OCA/OCB/blob/8.0/openerp/modules/migration.py#L96 ff

Did anyone try if the migration script runs when upgrading from base_contact after this version number change? If not, that's a serious problem when people uninstall base_contact.

@lepistone I have the same problem. init_hook seems the good choice to me.

[hbrunn]

ping @dreispt (just to avoid you missed my previous non-attributed post)

[dreispt]

@hbrunn I didn't test. I guess the relevant pieces of code are:

• https://github.com/OCA/OCB/blob/8.0/openerp/modules/migration.py#L140
• https://github.com/odoo/odoo/blob/8.0/openerp/tools/parse_version.py#L30

From the code, I'm guessing it should work...

[hbrunn]

@dreispt yes I just debugged thourgh it. There are some curious transformations of the version string, and it works out in the end. So nothing to do and sorry for the noise

[dreispt]

@hbrunn Thanks for your time checking that.

[lepistone]

@hbrunn about the init hook: I'm not sure myself, as that would be shown only on module installation. Where would you put them, and with which log level? Thanks!

[hbrunn]

@lepistone I was thinking about raising in the init hook, but that indeed doesn't do anything for existing installations. Another possibility might be setting 'installable': False in the manifest, but I never tried our how this behaves if the module in question is already installed.

For new users, I'd declare an abstract model that gives a warning in its _register_hook. We might even set the module's state to 'to uninstall' there, then it will be gone with the next database update.

But on the other hand: What's the problem with keeping this module as sort of metapackage?

[yajo]

But on the other hand: What's the problem with keeping this module as sort of metapackage?

The only problem is maintaining deprecated code, but it should work as it is.

Indeed the warning raises even when it is not installed, but no idea about how to avoid that. It should be a separate issue anyway.