Merge lp:~savoirfairelinux-openerp/openupgrade-server/base_contact into lp:openupgrade-server

[Sandy Carter (http://www.savoirfairelinux.com)]

Marks base_contact module to be removed.
Moves entries from res_partner_contact to res_partner.

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

Thanks! Looks alright to me, in principle. Two questions:

l.51 and l.78: why do you need to check if the fields exist? Seems to me this could mask potential problems in the current or future version of the code.
l.12: would it not be safer if you checked the status of the entry for 'base_contact' in the ir_module_module table? The table could theoretically belong to another module, or contain deprecated information after a deinstallation of the base_contact module.

review: Needs Information

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

> l.51 and l.78: why do you need to check if the fields exist? Seems to me this
> could mask potential problems in the current or future version of the code.

For l.78, I put that in place to avoid an error I had while testing relating to the 'mobile' column, which is not present on a bare system with only base_contact installed. I duplicated that safeguard in l.51 to avoid future similar errors.

Steps to reproduce:
OpenERP 6.1 with new database, install base_contact
Run OpenUpgrade

2013-11-14 16:33:28,336 9796 ERROR upgrade openerp: Failed to initialize database `upgrade`.
Traceback (most recent call last):
...
  File "base/migrations/7.0.1.3/post-migration.py", line 282, in migrate
  File "base/migrations/7.0.1.3/post-migration.py", line 211, in migrate_partner_address
  File "base/migrations/7.0.1.3/post-migration.py", line 170, in process_address_type
  File "parts/openerp/openerp/sql_db.py", line 161, in wrapper
    return f(self, *args, **kwargs)
  File "parts/openerp/openerp/sql_db.py", line 226, in execute
    res = self._obj.execute(query, params)
ProgrammingError: column "mobile" does not exist
LIGNE 1 : ...irthdate, city, country_id, email, fax, function, mobile, ph...
                                                               ^

> l.12: would it not be safer if you checked the status of the entry for
> 'base_contact' in the ir_module_module table? The table could theoretically
> belong to another module, or contain deprecated information after a
> deinstallation of the base_contact module.
It would. It's fixed now.

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

Thanks for adapting the check for base_contact!

I see now how the mobile field can go AWOL. Base_contact changes the field type to an unstored related field, upon which the ORM drops the column (I never liked it doing that). So your code makes good sense.

I'd like it though, if you could report on the missing fields in a logged warning. Would you mind doing so? Apart from that, this looks really good.

review: Needs Fixing

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

Stefan,
Thank you for your review.
This last commit should log as a warning any fields missing.

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

Thanks! Is there a special reason that you log the lost fields in l.194, which is called inside a loop? I'd prefer to log in line 82, right after determining the value of lost_fields. And maybe make the message a little less alarming and a more explaining, like:

'The following columns are not present in the table of %s: %s. This can be the case if an additional module installed on your database changes the type of a regular column to a non-stored function or related field.'

Given the nature of the message, I expect this one to pop up on the mailing list and having a more verbose message saves me the trouble of having to think ;-)

review: Needs Fixing

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

FYI, I found a merge proposal to bring base_contact into v7.0
This could affect this MP, even make it obsolete.

https://code.launchpad.net/~openerp-dev/openobject-addons/7.0-base-contact-xal/+merge/192129

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

@sandy Yes I was aware of this branch but I did not know it was submitted for review. Let's see where that goes.

I'll approve this proposal as per your latest changes (thanks!) but set it to 'Work in progress' until you decide which way you want to go.

review: Approve

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

@Stefan, I feel like the 7.0 base_contact will not be merged for a long time.
If we offer this intermediary step for users upgrading it will be more useful than stalling.

Once the 7.0 base_contact merge is accepted, we just have to take the scripts out.

I think we should merge it.

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Pedro Manuel Baeza (pedro.baeza)]

I see this OK, so as Sandy suggested, we should procede with the merge.

review: Approve (code review)

=== modified file 'openerp/addons/base/migrations/7.0.1.3/post-migration.py'
--- openerp/addons/base/migrations/7.0.1.3/post-migration.py	2013-10-21 08:59:09 +0000
+++ openerp/addons/base/migrations/7.0.1.3/post-migration.py	2013-11-18 15:34:37 +0000
@@ -56,11 +56,69 @@
             SET rml_footer = rml_footer1
         """)
 
+def migrate_base_contact(cr):
+    """
+    Move entries of res_partner_contact into res_partner
+    """
+    cr.execute("SELECT * FROM ir_module_module WHERE name = 'base_contact' and state = 'to remove';")
+    if not cr.fetchall():
+        return
+    fields = [
+        'create_date',
+        'name',
+        'website',
+        'image',
+        'active',
+        'comment',
+        'title',
+        'phone',
+        'country_id',
+        'email',
+        'birthdate',
+        'lang',
+        'parent_id',
+    ]
+    # Add lang from lang_id
+    openupgrade.logged_query(cr, "ALTER TABLE res_partner_contact ADD COLUMN lang character varying(5);")
+    openupgrade.logged_query(cr, """
+        UPDATE res_partner_contact
+        SET lang = (SELECT code
+                    FROM res_lang l
+                    WHERE lang_id = l.id
+                    LIMIT 1);""")
+    # Add parent_id
+    openupgrade.logged_query(cr, "ALTER TABLE res_partner_contact ADD COLUMN parent_id integer;")
+    openupgrade.logged_query(cr, """
+        UPDATE res_partner_contact
+        SET parent_id = (SELECT openupgrade_7_migrated_to_partner_id
+                         FROM res_partner_address a
+                         WHERE id = a.contact_id
+                         LIMIT 1);""")
+    # Make sure fields exist
+    cr.execute(
+        "SELECT column_name "
+        "FROM information_schema.columns "
+        "WHERE table_name = 'res_partner_contact';")
+    available_fields = set(i[0] for i in cr.fetchall())
+    lost_fields = set(fields) - available_fields
+    if lost_fields:
+        openupgrade.logger.warning("""\
+The following columns are not present in the table of %s: %s.
+
+This can be the case if an additional module installed on your database changes the type of a 
+regular column to a non-stored function or related field.
+""", 'res_partner_contact', ", ".join(lost_fields))
+    fields = list(available_fields.intersection(fields))
+    # Move data
+    openupgrade.logged_query(cr, """
+        INSERT INTO res_partner (%s)
+        SELECT %s
+        FROM res_partner_contact;""" % (", ".join(fields + ['customer', 'is_company']),
+                                        ", ".join(fields + ['TRUE', 'FALSE'])))
+
 def migrate_partner_address(cr, pool):
     """ res.partner.address is obsolete. Move existing data to
     partner
-
-    TODO: break hard when base_contact is installed
     """
     partner_obj = pool.get('res.partner')
     cr.execute(
@@ -82,6 +140,22 @@
     partner_found = []
     processed_ids = []
 
+    # Make sure fields exist
+    cr.execute(
+        "SELECT column_name "
+        "FROM information_schema.columns "
+        "WHERE table_name = 'res_partner_address';")
+    available_fields = set(i[0] for i in cr.fetchall())
+    lost_fields = set(fields) - available_fields
+    if lost_fields:
+        openupgrade.logger.warning("""\
+The following columns are not present in the table of %s: %s.
+
+This can be the case if an additional module installed on your database changes the type of a 
+regular column to a non-stored function or related field.
+""", 'res_partner_address', ", ".join(lost_fields))
+    fields = available_fields.intersection(fields)
+
     def set_address_partner(address_id, partner_id):
         cr.execute(
             "UPDATE res_partner_address "
@@ -107,8 +181,9 @@
         """
         Migrate addresses to partners, based on sql WHERE clause
         """
-        cr.execute(
-            "SELECT " + ', '.join(fields) + " FROM res_partner_address "
+        openupgrade.logged_query(cr, "\n"
+            "SELECT " + ', '.join(fields) + "\n"
+            "FROM res_partner_address\n"
             "WHERE " + whereclause, args or ())
         for row in cr.fetchall():
             row_cleaned = [val or False for val in row]
@@ -222,6 +297,7 @@
     migrate_ir_translation(cr)
     migrate_company(cr)
     migrate_partner_address(cr, pool)
+    migrate_base_contact(cr)
     update_users_partner(cr, pool)
     reset_currency_companies(cr, pool)
     migrate_res_company_logo(cr, pool)

=== modified file 'openerp/addons/base/migrations/7.0.1.3/pre-migration.py'
--- openerp/addons/base/migrations/7.0.1.3/pre-migration.py	2013-10-25 19:54:27 +0000
+++ openerp/addons/base/migrations/7.0.1.3/pre-migration.py	2013-11-18 15:34:37 +0000
@@ -63,6 +63,19 @@
         cr,
         "UPDATE ir_module_module SET demo = false")
 
+def rename_base_contact_columns(cr):
+    """
+    Rename columns only if res_partner_contact is installed
+    """
+    cr.execute("SELECT * FROM information_schema.tables WHERE table_name = 'res_partner_contact';")
+    if cr.fetchall():
+        openupgrade.rename_columns(cr, {
+            'res_partner_contact': [
+                ('photo', 'image'),
+                ('mobile', 'phone'),
+            ]
+        })
+
 def migrate_ir_attachment(cr):
     # Data is now stored in db_datas column and datas is a function field
     # like in the document module in 6.1. If the db_datas column already
@@ -175,12 +188,13 @@
 def remove_obsolete_modules(cr):
     obsolete_modules = (
         'base_tools',
+        'base_contact',
         )
     cr.execute(
         """
         UPDATE ir_module_module
         SET state = 'to remove'
-        WHERE name in %s
+        WHERE name in %s AND state <> 'uninstalled'
         """, (obsolete_modules,))
 
 @openupgrade.migrate()
@@ -192,6 +206,7 @@
         )
     openupgrade.drop_columns(cr, [('ir_actions_todo', 'action_id')])
     openupgrade.rename_columns(cr, column_renames)
+    rename_base_contact_columns(cr)
     openupgrade.rename_xmlids(cr, xmlid_renames)
     openupgrade.rename_models(cr, model_renames)
     migrate_ir_attachment(cr)