Merge lp:~diegosarmentero/ubuntuone-control-panel/sync-status into lp:ubuntuone-control-panel

[Diego Sarmentero]

- Checking that we are receiving the right argument type, or ignore it if it's not valid (LP: 995146).

[Roberto Alsina (ralsina)]

Isn't the problem that we are switching to DISABLED in line 15 of the diff because we are getting a 0?

If status is meant to be a mapping, shouldn't we check for that BEFORE changing state? I have not looked carefully at the code and I am sleepy, so I may be saying nonsense ;-)

review: Needs Information

[Diego Sarmentero (diegosarmentero)]

> Isn't the problem that we are switching to DISABLED in line 15 of the diff
> because we are getting a 0?
>
> If status is meant to be a mapping, shouldn't we check for that BEFORE
> changing state? I have not looked carefully at the code and I am sleepy, so I
> may be saying nonsense ;-)

Actually, we are receiving a dbus.UInt32(3), but yes, it makes more sense to check for Mapping before.

[lp:~diegosarmentero/ubuntuone-control-panel/sync-status]

319. By Diego Sarmentero on 2012-05-11

Checking first for Mapping

[Roberto Alsina (ralsina)]

review: Approve

[Mike McCracken (mikemc)]

Code looks good. In general I'm curious why we're getting sent invalid status arguments in the first place...

I'd like to see two minor docstring changes:

controlpanel/backend.py:

for _process_file_sync_status, I'd like to see an updated docstring, because it doesn't always return a dictionary like the current doc string says it does.
It would be nice for future maintenance to say what it might return and what that means.

tests/test_backend.py:

didn't understand docstring of new test.
the commit comment seems like it'd be a better doc string
"Checking that the method receive the proper argument type"

review: Needs Fixing

[lp:~diegosarmentero/ubuntuone-control-panel/sync-status]

320. By Diego Sarmentero on 2012-05-11

Fixing docstrings.

[Mike McCracken (mikemc)]

review: Approve

=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py	2012-03-29 21:41:54 +0000
+++ ubuntuone/controlpanel/backend.py	2012-05-11 13:33:18 +0000
@@ -16,6 +16,7 @@
 
 """A backend for the Ubuntu One Control Panel."""
 
+import collections
 import operator
 import os
 
@@ -152,9 +153,13 @@
             FILE_SYNC_UNKNOWN
         * MSG_KEY: a non translatable but human readable string of the status.
 
+        Return None if we receive and invalid type in status.
+
         """
         logger.debug('sync status: %r', status)
-        if not status:
+        if not isinstance(status, collections.Mapping):
+            return None
+        elif not status:
             self.file_sync_disabled = True
             return self.STATUS_DISABLED
 

=== modified file 'ubuntuone/controlpanel/gui/qt/filesyncstatus.py'
--- ubuntuone/controlpanel/gui/qt/filesyncstatus.py	2012-03-02 13:53:24 +0000
+++ ubuntuone/controlpanel/gui/qt/filesyncstatus.py	2012-05-11 13:33:18 +0000
@@ -139,6 +139,8 @@
     @log_call(logger.debug)
     def process_info(self, status):
         """Match status with signals."""
+        if status is None:
+            return
         try:
             status_key = status[backend.STATUS_KEY]
             data = FILE_SYNC_STATUS[status_key]

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py	2012-03-12 16:36:06 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py	2012-05-11 13:33:18 +0000
@@ -93,6 +93,21 @@
         self.assertIsInstance(self.ui.backend,
                               backend.ControlBackend)
 
+    def test_process_info_invalid_status(self):
+        """File sync status invalid, ignore event."""
+        status = {backend.STATUS_KEY: backend.FILE_SYNC_STARTING,
+                  backend.MSG_KEY: gui.FILE_SYNC_STARTING,
+                  'icon': backend.FILE_SYNC_STARTING}
+        self.ui.process_info(status)
+        self.ui.process_info(3)
+
+        actual_icon = self.ui.ui.sync_status_icon.pixmap()
+        pixmap_name = gui.icon_name_from_status(backend.FILE_SYNC_STARTING)
+        expected_icon = gui.pixmap_from_name(pixmap_name)
+        self.assertEqualPixmaps(expected_icon, actual_icon)
+        actual_text = unicode(self.ui.ui.sync_status_label.text())
+        self.assertEqual(gui.FILE_SYNC_STARTING, actual_text)
+
     def test_process_info_changed(self):
         """Backend's file sync status changed callback is connected."""
         self.assertEqual(self.ui.backend.status_changed_handler,

=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py	2012-03-29 21:37:22 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py	2012-05-11 13:33:18 +0000
@@ -1370,6 +1370,14 @@
         expected_status = self.be._process_file_sync_status(status)
         self.assertEqual(self._called, ((expected_status,), {}))
 
+    def test_invalid_status_type(self):
+        """Check that the method return None with invalid types."""
+        self.be.status_changed_handler = self._set_called
+        status = 3
+        self.be.sd_client.status_changed_handler(status)
+
+        self.assertEqual(self._called, ((None,), {}))
+
 
 class BackendSyncStatusIfDisabledTestCase(BackendSyncStatusTestCase):
     """Syncdaemon state for the backend when file sync is disabled."""