Merge lp:~anybox/aeroo/fix-upgrade into lp:~anybox/aeroo/7.0-anybox

[Georges Racinet]

To test, take any customer project using an earlier version of aeroo (no restart_ooo_cmd column in oo_config table), and perform an upgrade with the upgrade script provided by the recipe.

A quicker test is to check if the server starts *without -u report_aeroo_ooo*

Note that I am also tempted by fixing the bare excepts once the fix itself is more mature.

I'm asking ccomb, but any comments are welcome, and especially further tests by the Anybox team on customer projects.

[Christophe Combelles (ccomb)]

+1 to replace the bare exception and add uncatched exceptions as they appear

review: Needs Fixing (read only, no try)

[lp:~anybox/aeroo/fix-upgrade]

38. By Georges Racinet <email address hidden> on 2014-04-17

Removed some bare excepts

This did not change the behaviour for me. But exception treatment is so
messy in aeroo that I could not see the warning in the except: block

I could see parts of the module that systematically wrap and reraise everything
as Exception

[Georges Racinet (gracinet)]

@ccomb: removed the two bare excepts I highlited earlier. Are you satisfied ?
I'll work on the other aspects of exception handling in other branches, it's a different topic in my eyes.

[Christophe Combelles (ccomb)]

review: Approve (read only, no try)

[lp:~anybox/aeroo/fix-upgrade]

39. By Pierre Verkest on 2014-04-24

use exception message

40. By Pierre Verkest on 2014-04-24

[FIX]use Message instead message from uno exception

41. By Georges Racinet on 2014-04-25

[FIX] (temp) graceful instantiation of OOo service depended on bare except

put a big comment in check_deps indicating how a proper fix of this terrible
entanglement should look

=== modified file 'report_aeroo/report_xml.py'
--- report_aeroo/report_xml.py	2013-12-03 14:15:00 +0000
+++ report_aeroo/report_xml.py	2014-04-25 09:16:09 +0000
@@ -225,23 +225,28 @@
         super(report_xml, self).register_all(cr)
         ########### Run OpenOffice service ###########
         try:
-            from report_aeroo_ooo.report import OpenOffice_service
-        except Exception, e:
-            OpenOffice_service = False
+            from report_aeroo_ooo.report import OpenOffice_service, DocumentConversionException
+        except (ImportError, orm.except_orm):
+            # TODO GR temporary catching the improper except_orm,
+            # see comment in aeroo_ooo.check_deps
+            OpenOffice_service = None
 
         if OpenOffice_service:
             cr.execute("SELECT id, state FROM ir_module_module WHERE name='report_aeroo_ooo'")
             helper_module = cr.dictfetchone()
-            helper_installed = helper_module and helper_module['state']=='installed'
+            helper_installed = helper_module and helper_module['state'] == 'installed'
 
         if OpenOffice_service and helper_installed:
+            # GR internal API discrepancy, see docstring of OpenOffice_service.__init__()
             cr.execute("SELECT host, port FROM oo_config")
             host, port = cr.fetchone()
             try:
                 OpenOffice_service(cr, host, port)
                 self._logger.info("OpenOffice.org connection successfully established")
-            except Exception, e:
-                self._logger.warning(str(e))
+            except DocumentConversionException:
+                self._logger.warning("OpenOffice conversion failed (host=%r, port=%r)", host, port,
+                                     exc_info=True)
+
         ##############################################
 
         cr.execute("SELECT * FROM ir_act_report_xml WHERE report_type = 'aeroo' and active = true ORDER BY id") # change for OpenERP 6.0

=== modified file 'report_aeroo_ooo/DocumentConverter.py'
--- report_aeroo_ooo/DocumentConverter.py	2014-03-05 15:11:00 +0000
+++ report_aeroo_ooo/DocumentConverter.py	2014-04-25 09:16:09 +0000
@@ -96,19 +96,19 @@
         try:
             self._context = self._resolver.resolve("uno:socket,host=%s,port=%s;urp;StarOffice.ComponentContext" % (host, port))
         except IllegalArgumentException, exception:
-            raise DocumentConversionException("The url is invalid (%s)" % exception)
+            raise DocumentConversionException("The url is invalid (%s)" % exception.Message)
         except NoConnectException, exception:
             if self._restart_ooo():
                 # We try again once
                 try:
                     self._context = self._resolver.resolve("uno:socket,host=%s,port=%s;urp;StarOffice.ComponentContext" % (host, port))
                 except NoConnectException, exception:
-                    raise DocumentConversionException("Failed to connect to OpenOffice.org on host %s, port %s. %s" % (host, port, exception))
+                    raise DocumentConversionException("Failed to connect to OpenOffice.org on host %s, port %s. %s" % (host, port, exception.Message))
 
             else:
-                raise DocumentConversionException("Failed to connect to OpenOffice.org on host %s, port %s. %s" % (host, port, exception))
+                raise DocumentConversionException("Failed to connect to OpenOffice.org on host %s, port %s. %s" % (host, port, exception.Message))
         except ConnectionSetupException, exception:
-            raise DocumentConversionException("Not possible to accept on a local resource (%s)" % exception)
+            raise DocumentConversionException("Not possible to accept on a local resource (%s)" % exception.Message)
 
     def putDocument(self, data):
         try:
@@ -162,7 +162,7 @@
             try:
                 found.insertDocumentFromURL('private:stream', self._toProperties(InputStream = subStream, FilterName = "writer8"))
             except Exception, ex:
-                print (_("Error inserting file %s on the OpenOffice document: %s") % (subreport, ex))
+                print (_("Error inserting file %s on the OpenOffice document: %s") % (subreport, ex.Message))
             #found = self.document.findNext(found, search)
 
             os.unlink(subreport)
@@ -174,7 +174,7 @@
             try:
                 self.document.Text.getEnd().insertDocumentFromURL('private:stream', self._toProperties(InputStream = subStream, FilterName = "writer8"))
             except Exception, ex:
-                print (_("Error inserting file %s on the OpenOffice document: %s") % (docs, ex))
+                print (_("Error inserting file %s on the OpenOffice document: %s") % (docs, ex.Message))
 
     def convertByPath(self, inputFile, outputFile):
         inputUrl = self._toFileUrl(inputFile)

=== modified file 'report_aeroo_ooo/check_deps.py'
--- report_aeroo_ooo/check_deps.py	2014-03-05 13:52:34 +0000
+++ report_aeroo_ooo/check_deps.py	2014-04-25 09:16:09 +0000
@@ -47,6 +47,14 @@
             error = True
             import_errors.append(str(e))
     if error:
+        # TODO GR: this is ridiculous. Missing dependencies that make conversions unusable are
+        # definitely not user errors. Either we want aeroo reports to stay functional in pure ODF
+        # mode, or not (note that this is imported in all cases at startup).
+        # the dependency checking should definitely not rely on such. It worked because caller code
+        # (report_aeroo.report_xml) used to catch everything
+        # (fixing this could be in another branch, I can't chase all
+        # consequencies right now). Probably we should replace all this cruft with OpenERP's native
+        # way of checking dependencies (through manifest file).
         raise orm.except_orm(_('Warning!')+' '+_('Unmet python dependencies!'), '\n'.join(import_errors))
 
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:

=== modified file 'report_aeroo_ooo/report.py'
--- report_aeroo_ooo/report.py	2014-03-31 13:08:07 +0000
+++ report_aeroo_ooo/report.py	2014-04-25 09:16:09 +0000
@@ -33,11 +33,38 @@
 from openerp.osv import orm, fields
 from openerp import netsvc
 from .DocumentConverter import DocumentConverter
+from .DocumentConverter import DocumentConversionException  # noqa (just forwarding)
+
 
 class OpenOffice_service (DocumentConverter, netsvc.Service):
 
     def __init__(self, cr, host, port):
-        cr.execute("SELECT host, port, ooo_restart_cmd FROM oo_config")
+        """Init the service, reading properties from database.
+
+        Care is taken not to break if the ``ooo_restart_cmd``` column does
+        not exist in config table.
+
+        This is useful for transitional situations
+        (upgrade/migrate scripts that need to load the database for inspection
+        before launching the upgrade itself.
+
+        If the column is missing, the restart command is set to ``False``, the
+        OpenERP standard missing marker. This is equivalent to the situation
+        once the upgrade is complete.
+
+        .. note:: parameters ``host`` and ``port`` are discarded by the current
+                  implementation, being replaced by values read from the config table.
+
+                  The standard instantiation caller, :mod:`openerp.addons.report_aeroo.report_xml`,
+                  performs the same lookup for these parameters, and passes them. This
+                  is an internal API discrepancy, probably introduced precisely with the
+                  ``ooo_restard_cmd`` parameter, and that we may want to take care of in a second
+                  move.
+        """
+        cr.execute("SELECT id FROM ir_model_fields "
+                   "WHERE name='ooo_restart_cmd' AND model='oo.config'")
+        restart_col = 'ooo_restart_cmd' if cr.fetchone() else 'FALSE AS ooo_restart_cmd'
+        cr.execute("SELECT host, port, " + restart_col + " FROM oo_config")
         host, port, ooo_restart_cmd = cr.fetchone()
         DocumentConverter.__init__(self, host, port, ooo_restart_cmd)
         netsvc.Service.__init__(self, 'openoffice')

=== added directory 'report_aeroo_ooo/tests'
=== added file 'report_aeroo_ooo/tests/__init__.py'
--- report_aeroo_ooo/tests/__init__.py	1970-01-01 00:00:00 +0000
+++ report_aeroo_ooo/tests/__init__.py	2014-04-25 09:16:09 +0000
@@ -0,0 +1,4 @@
+from . import test_service
+
+fast_suite = [test_service,
+              ]

=== added file 'report_aeroo_ooo/tests/test_service.py'
--- report_aeroo_ooo/tests/test_service.py	1970-01-01 00:00:00 +0000
+++ report_aeroo_ooo/tests/test_service.py	2014-04-25 09:16:09 +0000
@@ -0,0 +1,34 @@
+from contextlib import contextmanager
+from openerp.tests.common import TransactionCase
+from ..report import OpenOffice_service
+from ..DocumentConverter import DocumentConverter
+
+
+def mock_doc_converter_init(self, host='localhost', port=8100, ooo_restart_cmd=None):
+    self._host = host
+    self._port = port
+    self._ooo_restart_cmd = ooo_restart_cmd
+
+
+@contextmanager
+def mock_doc_converter():
+    saved_init = DocumentConverter.__init__
+    DocumentConverter.__init__ = mock_doc_converter_init
+    yield
+    DocumentConverter.__init__ = saved_init
+
+
+class TestOooService(TransactionCase):
+
+    def test_bare_instantiation(self):
+        with mock_doc_converter():
+            OpenOffice_service(self.cr, 'the_host', 8101)
+        # no assertion because of internal API discrepancy
+
+    def test_instantiation_missing_column(self):
+        self.cr.execute("ALTER TABLE oo_config DROP COLUMN ooo_restart_cmd")
+        self.cr.execute("DELETE FROM ir_model_fields "
+                        "WHERE model='oo.config' AND name='ooo_restart_cmd'")
+        with mock_doc_converter():
+            service = OpenOffice_service(self.cr, 'the_host', 8101)
+        self.assertFalse(service._ooo_restart_cmd)