Merge lp:~sylvain-legal/openupgrade-server/three-functions-for-addons-migration into lp:openupgrade-server

[Sylvain LE GAL (GRAP)]

1/ Add a file openupgrade_70.py containing specific function for the migration of modules from xx -> 7.0.
def get_partner_id_from_partner_address_id()
def get_partner_id_from_user_id()

2/ Add a function warn_possible_dataloss() in openupgrade.py to inform the user who realize the migration that field(s) moved from a installed module to an uninstalled module. (and so there is possible data loss)

Typical use :
openupgrade-addons / product / ... / post-migration.py

# [...]
possible_dataloss_fields = [
    {'table' : 'product_template', 'field' : 'loc_case', 'new_module' : 'stock',},
    {'table' : 'product_template', 'field' : 'purchase_ok', 'new_module' : 'purchase',},
# [...]
]

@openupgrade.migrate()
def migrate(cr, version):
    # [...]
    openupgrade.warn_possible_dataloss(cr, pool, 'product', possible_dataloss_fields)
    # [...]

[Stefan Rijnhart (Opener) (stefan-opener)]

Again, the question of having copyright assigned to George Arbitbol pops up.

warn_possible_dataloss: Can you tell me the exact use case for this function? I noticed that in 7.0, a number of functionalies have been split off to separate 'glue' modules. For example, there is now sale_stock for functionality that was first in the sale and stock modules. However, this module will be installed automatically if sale and stock are both installed, which will be the case if you have the sale module installed in 6.1 as it still depended on stock.

The other methods seem very useful to me. I have three minor comments for them.

l.58: I think mentioning the year is useful for copyright assignments.

    # This module copyright (C) 2013 Georges Abitbol

l.87: you are not supposed to perform variable substitution inline when using cr.execute() (unless you are substituting table names as there is no other way). The following should work:

WHERE id=%s""",
(partner_address_id,))

Same for l. 99.

review: Needs Fixing

[lp:~sylvain-legal/openupgrade-server/three-functions-for-addons-migration]

4619. By Sylvain LE GAL (GRAP) on 2013-07-15

[REF] change the call params of cr.execute() function.

[Sylvain LE GAL (GRAP) (sylvain-legal)]

Hi Stefan,

1. First sorry for the copyright. I just didn't have idea what to write because for me there is no one Copyright but many. for exemple I write something, you review it and fix something else. My point of view is that is a community copyright. But ok.
--> updated. (rev 4619)

2. For the warn_possible_dataloss. For me the use case is the following :
- I provide a 6.1 database with 'product' module installed but without 'purchase'.
- The users fill the field 'purchase_ok' because it's available. (some product values to True, other to False)
- I realize the migration to 7.0.
- I clean up the DB after. (deleting unused field, like you said in your comment https://answers.launchpad.net/openupgrade-server/+question/230898)
- There is a Data Loss.

The warn_possible_dataloss is usefull for that. After the migration, reading log, (it's just warning), maybe i'll install 'purchase' before cleaning up the DB. (It's a choice)

I agree to say that it's a kind of theorical problem because using 'purchase_ok' without 'purchase' module installed is not very usefull but it's possible problem.

3. about cr.execute() call, thank you.
--> updated. (rev 4619)

[Stefan Rijnhart (Opener) (stefan-opener)]

Thank you for your explanation (and your technical changes which look good). I personally still think that it should suffice for the administrator to review the list of columns to be removed once we have the cleanup module ready. In a sense, removing each of those columns constitutes a potential data loss, but then again, if a model column is dangling after the upgrade, it is by definition not used in the (ORM) system.

I actually traced your use case to the invoicing part of OpenERP, as in 6.1 there still is a filter on purchasable products in the case of a supplier invoice even if the purchase module is not installed. That has now been dropped, see lp:1171794. So for this particular use case, when only invoicing is used, the data loss is indeed theoretic. Even if one decides to install the purchase module for the sake of managing this field, it is still not applied when using invoicing without purchase orders.

We could consider putting up hints in the cleanup module if we have information available that a column has moved to a different module, but even that could lead to a false sense of security in case of columns that moved between modules in a way that is not yet detected by the analysis process. But with a sensible disclaimer, that I would find useful.

So far for my two cents, what do other people think?

[Holger Brunn (Therp) (hbrunn)]

Lacking the cleanup module, I think this function is very useful. But shouldn't it be able to figure out by itself where the field come from now?

I'd much prefer actually to have the cleanup module and a function that allows us to add messages to the administrator with a severity on a per module, model and field level.

How about the compromise: Write this function first, what it currently does is write into the log, and some time in the future to the cleanup-module's messages table?

[Stefan Rijnhart (Opener) (stefan-opener)]

Holger, thanks. Let's then keep the warn_possible_dataloss method and canalize the interesting logs that it generates.

I think we should work this one out one day but until then, I have a simplistic starting point that will at least allow us to put messages in the database when we are ready for that.

https://code.launchpad.net/~therp-nl/openupgrade-server/7.0-API-logging/+merge/175132

Holger, if you can live with this for now, we can go ahead with this proposal, only requiring a change in l.41 of the diff (I think l.35 is fine in an INFO log but I would not bother OpenERP administrators with such messages in the polished upgrade report, by which I am ironically demonstrating the need for levelled logging like you suggest).

[Stefan Rijnhart (Opener) (stefan-opener)]

Sylvain, as you have indicated that you are on holidays now, I took the liberty to refactor your contribution here: https://code.launchpad.net/~therp-nl/openupgrade-server/three-functions-for-addons-migration/+merge/175790

[Holger Brunn (Therp) (hbrunn)]

I'll set this one to superseded and discuss the code in Stefan's MP

Unmerged revisions

=== modified file 'openerp/openupgrade/openupgrade.py'
--- openerp/openupgrade/openupgrade.py	2013-06-11 08:09:44 +0000
+++ openerp/openupgrade/openupgrade.py	2013-07-15 10:42:33 +0000
@@ -202,6 +202,44 @@
         cr,
         "DELETE FROM wkf WHERE osv = %s", (model,))
 
+def warn_possible_dataloss(cr, pool, old_module, fields):
+    """
+    Use that function in the following case : 
+    if a field of a model was moved from a 'A' module to a 'B' module. 
+    ('B' depend on 'A'), 
+    This function will test if 'B' is installed. 
+    If not, count the number of different value and possibly warn the user.
+    Use orm, so call from the post script.
+    
+    :param old_module: name of the old module
+    :param fields: list of dictionnary with the following keys :
+        'table' : name of the table where the field is.
+        'field' : name of the field that are moving.
+        'new_module' : name of the new module
+    """
+    module_obj = pool.get('ir.module.module')
+    for field in fields: 
+        module_ids = module_obj.search(cr, SUPERUSER_ID, [
+                ('name', '=', field['new_module']),
+                ('state', 'in', ['installed', 'to upgrade', 'to install'])
+            ])
+        if not module_ids: 
+            cr.execute("SELECT count(*) FROM (SELECT %s from %s group by %s) as tmp" \
+                %(field['field'], field['table'], field['field']))
+            row = cr.fetchone()
+            if row[0] == 1: 
+                # not a problem, that field was'nt used. Just a loss of fonctionnality
+                logger.info("'%s' in module '%s' has moved in module " \
+                    "'%s' that is not installed : " \
+                    "Users'll loose fonctionnalities" \
+                    %(field['field'], old_module, field['new_module']))
+            else: 
+                # there is data loss after the migration.
+                logger.warning("'%s' in module '%s' has moved in module " \
+                    "'%s' that is not installed : " \
+                    "There was %s differentes values in this field." \
+                    %(field['field'], old_module, field['new_module'], row[0]))
+
 def set_defaults(cr, pool, default_spec, force=False):
     """
     Set default value. Useful for fields that are newly required. Uses orm, so

=== added file 'openerp/openupgrade/openupgrade_70.py'
--- openerp/openupgrade/openupgrade_70.py	1970-01-01 00:00:00 +0000
+++ openerp/openupgrade/openupgrade_70.py	2013-07-15 10:42:33 +0000
@@ -0,0 +1,47 @@
+# -*- coding: utf-8 -*-
+##############################################################################
+#
+# OpenERP, Open Source Management Solution
+# This migration script copyright (C) 2013-today Sylvain LE GAL
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################
+
+# This module provides simple tools for openupgrade migration, specific for the
+# 6.1 -> 7.0.
+
+def get_partner_id_from_partner_address_id(cr, partner_address_id):
+    """
+        Get the new partner_id from old partner_address_id.
+        :param partner_address_id : res_partner_address previously used.
+    """
+    cr.execute("""
+        SELECT openupgrade_7_migrated_to_partner_id 
+        FROM res_partner_address
+        WHERE id=%s""",
+        (partner_address_id,))
+    return cr.fetchone()[0]
+    
+def get_partner_id_from_user_id(cr, user_id):
+    """
+        Get the new partner_id from user_id.
+        :param user_id : user previously used.
+    """
+    cr.execute("""
+        SELECT partner_id 
+        FROM res_users 
+        WHERE id=%s""",
+        (user_id,))
+    return cr.fetchone()[0]