Merge lp:~mandel/ubuntuone-windows-installer/uninstall-old-app into lp:ubuntuone-windows-installer

[Manuel de la Peña]

Provides a function that can be used to uninstall the old beta of ubuntu one.

Provides a function that can be used to uninstall the old beta of ubuntu one.

[lp:~mandel/ubuntuone-windows-installer/uninstall-old-app]

76. By Manuel de la Peña on 2011-09-28

Merged with parent.

77. By Manuel de la Peña on 2011-09-28

Re-merged with parent.

78. By Manuel de la Peña on 2011-09-28

Remerged with parent.

79. By Manuel de la Peña on 2011-09-29

Merged with parent.

80. By Manuel de la Peña on 2011-10-03

Merged with parent.

81. By Manuel de la Peña on 2011-10-03

Remerged with parent for lint issues.

82. By Manuel de la Peña on 2011-10-03

Fixed lint issues.

83. By Manuel de la Peña on 2011-10-03

Re-merged with parent and fixed conflicts.

84. By Manuel de la Peña on 2011-10-03

Fixed some lint issues.

85. By Manuel de la Peña on 2011-10-04

Merged with parent.

86. By Manuel de la Peña on 2011-10-04

Fixed lint issues and pep8 ones.

[Natalia Bidart (nataliabidart)]

The code in the existent is_old_beta_installed should reuse the newly added get_property_for_product, so we do not add code duplication. Likewise, tests should be refactored so there is no duplication there (I'm talking about _assert_get_product_property and _assert_is_old_beta_installed).

review: Needs Fixing

[lp:~mandel/ubuntuone-windows-installer/uninstall-old-app]

87. By Manuel de la Peña on 2011-10-06

Reuse the method used to get a property.

[Natalia Bidart (nataliabidart)]

(11:51:59 AM) nessita: mandel: I'm re-reviewing uninstall-old-app. I guess that all the comments inside is_old_beta_installed should be moved to get_property_for_product. And, for clarity sake, I would advice defining get_property_for_product before the is_old_beta_installed function...
(11:52:09 AM) nessita: mandel: can you please change that?

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

Also, this docstring:

    """Retruns the value of a fiven property from a product."""

should be written as a command (Returm the value...). Same for:

"""Will remove the old beta from the users machine."""

should be """Remove the old beta from the user's machine.""".

[lp:~mandel/ubuntuone-windows-installer/uninstall-old-app]

88. By Manuel de la Peña on 2011-10-10

Fixed docstrings and move the funtion so that is more clear for other developers.

89. By Manuel de la Peña on 2011-10-10

Fixed pep8 issues.

[Natalia Bidart (nataliabidart)]

Looks good now!

review: Approve

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

=== modified file 'ubuntuone_installer/gui/qt/utils/__init__.py'
--- ubuntuone_installer/gui/qt/utils/__init__.py	2011-09-29 13:26:44 +0000
+++ ubuntuone_installer/gui/qt/utils/__init__.py	2011-10-10 15:22:19 +0000
@@ -31,6 +31,7 @@
     is_old_beta_installed = windows.is_old_beta_installed
     are_updates_present = windows.are_updates_present
     perform_update = windows.perform_update
+    uninstall_old_beta = windows.uninstall_old_beta
 else:
     from ubuntuone_installer.gui.qt.utils import linux
     uninstall_application = linux.uninstall_application
@@ -40,3 +41,4 @@
     is_old_beta_installed = lambda: False
     are_updates_present = lambda *args, **kwargs: False
     perform_update = lambda *args, **kwargs: None
+    uninstall_old_beta = lambda *args, **kwargs: None

=== modified file 'ubuntuone_installer/gui/qt/utils/tests/test_windows.py'
--- ubuntuone_installer/gui/qt/utils/tests/test_windows.py	2011-10-04 10:43:26 +0000
+++ ubuntuone_installer/gui/qt/utils/tests/test_windows.py	2011-10-10 15:22:19 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 # Authors: Diego Sarmentero <[e-mail]>
+#          Manuel de la Pena <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -237,41 +238,153 @@
 
     """Test the update from the old beta."""
 
-    def _assert_is_old_beta_installed(self, return_code, expected_result):
+    def _assert_is_old_beta_installed(self, return_value, expected_result):
         """Helper to test the is_old_beta_installed."""
         # vars used to know that we did indeed call the msi lib correctly
         called = {}
 
-        def fake_get_product_info(uid, property_name, property_buffer,
-                                  size):
+        def fake_get_product_property(uid, property_name):
             """Fake call to the msi lib."""
             called['uid'] = uid
-            called['property'] = property_name
-            called['property_buffer'] = property_buffer
-            called['size'] = size
-            return return_code
+            called['property_name'] = property_name
+            return return_value
 
-        self.patch(utils.windows.ctypes.windll.msi, 'MsiGetProductInfoW',
-                   fake_get_product_info)
+        self.patch(utils.windows, 'get_property_for_product',
+                   fake_get_product_property)
         self.assertEqual(utils.is_old_beta_installed(), expected_result)
         # assert that we indeed called the ctypes with the correct data
         self.assertEqual(utils.windows.OLD_BETA_UID_KEY,
-                         called['uid'].value)
+                         called['uid'])
         self.assertEqual(utils.windows.VERSION_STRING_PROPERTY,
-                         called['property'])
+                         called['property_name'])
 
     def test_is_old_beta_installed_true(self):
         """Test the function used to know if it is installed."""
-        self._assert_is_old_beta_installed(utils.windows.ERROR_SUCCESS, True)
-
-    def test_is_old_beta_installed_more_data(self):
-        """Test the function used to know if it is installed."""
-        self._assert_is_old_beta_installed(utils.windows.ERROR_MORE_DATA, True)
+        self._assert_is_old_beta_installed('SomeString', True)
 
     def test_is_old_beta_installed_false(self):
         """Test the function used to know if it is installed."""
-        self._assert_is_old_beta_installed(utils.windows.ERROR_UNKNOWN_PRODUCT,
-                False)
+        self._assert_is_old_beta_installed(None, False)
+
+    def _assert_get_product_property(self, uid, property_name, return_code,
+                                     expected_result):
+        """Generic test for getting the property."""
+        # vars used to know that we did indeed call the msi lib correctly
+        called = {}
+
+        def fake_get_product_info(uid, property_name, property_buffer, size):
+            """Fake call to the msi lib."""
+            called['called_uid_buffer'] = uid
+            called['called_property_name'] = property_name
+            if expected_result:
+                property_buffer.value = expected_result
+                called['called_property_buffer'] = expected_result
+            called['called_size'] = size
+            return return_code
+
+        self.patch(utils.windows.ctypes.windll.msi, 'MsiGetProductInfoW',
+                   fake_get_product_info)
+        self.assertEqual(expected_result,
+                         utils.windows.get_property_for_product(uid,
+                                                                property_name))
+        self.assertEqual(uid, called['called_uid_buffer'], uid)
+        self.assertEqual(property_name, called['called_property_name'])
+
+    def test_get_product_property_present(self):
+        """Test that we do indeed get the correct value."""
+        uid = u'{som-uid}'
+        property_name = u'VersionString'
+        return_code = 0
+        self._assert_get_product_property(uid, property_name, return_code,
+                                          property_name)
+
+    def test_get_product_property_not_present(self):
+        """Test that we do get None."""
+        uid = u'{som-uid}'
+        property_name = u'VersionString'
+        return_code = 1212
+        self._assert_get_product_property(uid, property_name, return_code,
+                                          None)
+
+    def test_uninstall_present(self):
+        """Test uninstalling when the msi is present."""
+        called = {}
+        path = 'path/to/msi'
+
+        def fake_get_property_for_product(uid, property_name, buf_size=None):
+            """A fake method."""
+            called['property_uid'] = uid
+            called['property_name'] = property_name
+            return path
+
+        def fake_msi_install_product(callable_fn, path, cmd):
+            """A fake install function."""
+            called['callable_fn'] = callable_fn
+            called['install_path'] = path
+            called['cmd'] = cmd
+            return defer.succeed(0)
+
+        self.patch(utils.windows, 'get_property_for_product',
+                   fake_get_property_for_product)
+        self.patch(utils.windows, 'deferToThread',
+                   fake_msi_install_product)
+        utils.uninstall_old_beta()
+        self.assertEqual(called['property_uid'],
+                         utils.windows.OLD_BETA_UID_KEY)
+        self.assertEqual(called['property_name'], u'LocalPackage')
+        self.assertEqual(utils.windows.ctypes.windll.msi.MsiInstallProductW,
+                         called['callable_fn'])
+        self.assertEqual(called['install_path'], path)
+        self.assertEqual(called['cmd'], u'REMOVE=ALL')
+
+    def test_uninstall_error(self):
+        """Test uninstalling with an error."""
+        called = {}
+        path = 'path/to/somewhere'
+
+        def fake_get_property_for_product(uid, property_name, buf_size=None):
+            """A fake method."""
+            called['property_uid'] = uid
+            called['property_name'] = property_name
+            return path
+
+        def fake_msi_install_product(callable_fn, path, cmd):
+            """A fake install function."""
+            called['callable_fn'] = callable_fn
+            called['install_path'] = path
+            called['cmd'] = cmd
+            return defer.succeed(1)
+
+        self.patch(utils.windows, 'get_property_for_product',
+                   fake_get_property_for_product)
+        self.patch(utils.windows, 'deferToThread',
+                   fake_msi_install_product)
+        self.assertFailure(utils.uninstall_old_beta(),
+                           utils.windows.MsiException)
+        self.assertEqual(called['property_uid'],
+                         utils.windows.OLD_BETA_UID_KEY)
+        self.assertEqual(called['property_name'], u'LocalPackage')
+        self.assertEqual(utils.windows.ctypes.windll.msi.MsiInstallProductW,
+                         called['callable_fn'])
+        self.assertEqual(called['install_path'], path)
+        self.assertEqual(called['cmd'], u'REMOVE=ALL')
+
+    def test_uninstall_not_present(self):
+        """Test uninstalling when the msi is missing."""
+        called = {}
+
+        def fake_get_property_for_product(uid, property_name):
+            """A fake method."""
+            called['uid'] = uid
+            called['property_name'] = property_name
+            return None
+
+        self.patch(utils.windows, 'get_property_for_product',
+                   fake_get_property_for_product)
+        self.assertFailure(utils.uninstall_old_beta(),
+                           utils.windows.MsiException)
+        self.assertEqual(called['uid'], utils.windows.OLD_BETA_UID_KEY)
+        self.assertEqual(called['property_name'], u'LocalPackage')
 
 
 class FakeLogger(object):

=== modified file 'ubuntuone_installer/gui/qt/utils/windows.py'
--- ubuntuone_installer/gui/qt/utils/windows.py	2011-10-03 14:21:48 +0000
+++ ubuntuone_installer/gui/qt/utils/windows.py	2011-10-10 15:22:19 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 # Authors: Diego Sarmentero <[e-mail]>
+#          Manuel de la Pena <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -34,6 +35,7 @@
 
 from twisted.internet import defer
 from twisted.internet.utils import getProcessValue
+from twisted.internet.threads import deferToThread
 
 AUTORUN_KEY = r"Software\Microsoft\Windows\CurrentVersion\Run"
 
@@ -41,6 +43,7 @@
 # beta is indeed present in the system, can be found in
 # lp:ubuntuone-windows-installed/beta/install/UbuntuOne7.wxs
 OLD_BETA_UID_KEY = '{13b7ee30-849c-11df-8395-0800200c9a66}'
+PROPERTY_BUFFER_SIZE = 256
 # error code used by ctypes.windll.msi
 ERROR_MORE_DATA = 234
 ERROR_SUCCESS = 0
@@ -49,6 +52,10 @@
 VERSION_STRING_PROPERTY = u'VersionString'
 
 
+class MsiException(Exception):
+    """Raised when there are msi issues."""
+
+
 def uninstall_application():
     """Uninstall Ubuntu One."""
     if hasattr(sys, "frozen"):
@@ -156,32 +163,61 @@
     return [docs, music, pictures]
 
 
+def get_property_for_product(product, property_name,
+                             buf_size=PROPERTY_BUFFER_SIZE):
+    """Return the value of a given property from a product."""
+    property_buffer = ctypes.create_unicode_buffer(buf_size)
+    size = wintypes.DWORD(buf_size)
+    result = ctypes.windll.msi.MsiGetProductInfoW(product, property_name,
+                                                  property_buffer,
+                                                  ctypes.byref(size))
+    if result == ERROR_MORE_DATA:
+        return get_property_for_product(product, property_name,
+                2 * buf_size)
+    elif result == ERROR_SUCCESS:
+        return property_buffer.value
+    else:
+        # The logic of the following statement is the following. The above code
+        # queried the msi system for a property regarding the msi uid. This
+        # uid is the code that uniquely identifies an app in the system. When
+        # working with COM properties there are different errors to take into
+        # account:
+        # ERROR_SUCCESS: We managed to get the data from the property.
+        # ERROR_MORE_DATA: The property does exist but the buffer allocated for
+        #                  the result is too small.
+        #
+        # ERROR_UNKNOWN_PROPERTY: The property does not exist.
+        # To the above errors we need to add ERROR_UNKNOWN_PRODUCT which is
+        # particular to msi and states that the product does not exist.
+        #
+        # knowing the above we can deduce that if we do not get any of the
+        # above error it means that such a property does not exists and
+        # therefore we return None.
+        return None
+
+
 def is_old_beta_installed():
     """Return whether the old beta is installed in the system."""
     # we try to get the VersisonString for the uid, if we get an error it means
     # that the product is not installed in the system.
-    buf_size = 256
-    uid_buffer = ctypes.create_unicode_buffer(OLD_BETA_UID_KEY)
-    property_buffer = ctypes.create_unicode_buffer(buf_size)
-    size = wintypes.DWORD(buf_size)
-    result = ctypes.windll.msi.MsiGetProductInfoW(uid_buffer,
-                                                  VERSION_STRING_PROPERTY,
-                                                  property_buffer,
-                                                  ctypes.byref(size))
-    # The logic of the following statement is the following. The above code
-    # queried the msi system for a property regarding the old beta uid. This
-    # uid is the code that uniquely identifies an app in the system. When
-    # working with COM properties there are different errors to take into
-    # account:
-    # ERROR_SUCCESS: We managed to get the data from the property.
-    # ERROR_MORE_DATA: The property does exist but the buffer allocated for the
-    #                  result is too small.
-    # ERROR_UNKNOWN_PROPERTY: The property does not exist.
-    # To the above errors we need to add ERROR_UNKNOWN_PRODUCT which is
-    # particular to msi and states that the product does not exist.
-    #
-    # knowing the above we can deduce the existence of the application by
-    # checking if the ERROR_SUCCESS or ERROR_MORE_DATA were returned. This
-    # is because the fact that those errors are given state that the uid of the
-    # msi is present and that we can get the info.
-    return result in (ERROR_SUCCESS, ERROR_MORE_DATA)
+    version_string = get_property_for_product(OLD_BETA_UID_KEY,
+                                              VERSION_STRING_PROPERTY)
+    return version_string is not None
+
+
[e-mail]
+def uninstall_old_beta():
+    """Remove the old beta from the user's machine."""
+    # we use the msi lib to be able to uninstall apps
+    property_name = u'LocalPackage'
+    uninstall_path = get_property_for_product(OLD_BETA_UID_KEY, property_name)
+    if uninstall_path is not None:
+        # lets remove the package.
+        command_line = u'REMOVE=ALL'
+        result = yield deferToThread(ctypes.windll.msi.MsiInstallProductW,
+                                     uninstall_path, command_line)
+        if result != ERROR_SUCCESS:
+            raise MsiException('Could not remove old beta.')
+    else:
+        # the local file is missing so we cannot un install it :(
+        raise MsiException('Could not remove old beta.')