Merge lp:~nataliabidart/ubuntuone-control-panel/decouple-devices into lp:ubuntuone-control-panel

[Natalia Bidart]

- Decoupled device list retrieved from the web from the local settings retrieved from syncdaemon (LP: #720704).

This change decouples the retrieval of the device info list from the retrieval of local settings from syncdaemon.

To test this, you should:

killall ubuntuone-control-panel-backend

In two terminals, run:

DEBUG=True PYTHONPATH=. bin/ubuntuone-control-panel-backend
DEBUG=True PYTHONPATH=. bin/ubuntuone-control-panel-gtk

The first test would be disconnecting your network connection and going to the devices tab, you should see a single device (your local device).

The second test is more complex, since it requires that syncdaemon is disabled. Due to bug bug #744980, you need to:

* u1sdtool -q
* edit ~/.config/ubuntuone/syncdaemon.conf and set files_sync_enabled = False
* re-run the control panel backend and frontend
* go to the devices tab and confirm that the device list is retrieved, but no local settings are offered

[lp:~nataliabidart/ubuntuone-control-panel/decouple-devices]

119. By Natalia Bidart on 2011-03-29

Fixed typo.

[Alejandro J. Cura (alecu)]

Nice branch!

review: Approve

[Manuel de la Peña (mandel)]

Work fine with me to small remarks:

1. 'more the devices will be configurable.' - I guess is either 'more devices' or 'more of the devices'

2. I'm confused about this:
118 + show_notifs = bool_str(show_notifs)
119 + local_device["show_all_notifications"] = show_notifs

Why reassigning show_notifs and then storing it in the local_devices dict. What about just assigning it to the dict directly?

Since I don't consider the two above things to be lowering the quality of the code I approve and will trust the committer to the right thing.

review: Approve

[Natalia Bidart (nataliabidart)]

> Work fine with me to small remarks:
>
> 1. 'more the devices will be configurable.' - I guess is either 'more devices'
> or 'more of the devices'

Fixed, thanks for the attention to detail!

> 2. I'm confused about this:
> 118 + show_notifs = bool_str(show_notifs)
> 119 + local_device["show_all_notifications"] = show_notifs
>
> Why reassigning show_notifs and then storing it in the local_devices dict.
> What about just assigning it to the dict directly?

Because otherwise it fells over the 80-columns width. I personally prefer assigning to a local variable that chopping off the line with = \.

[lp:~nataliabidart/ubuntuone-control-panel/decouple-devices]

120. By Natalia Bidart on 2011-03-29

Another typo, thanks mandel!

=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py	2011-03-28 17:25:21 +0000
+++ ubuntuone/controlpanel/backend.py	2011-03-29 17:12:42 +0000
@@ -26,7 +26,7 @@
 from ubuntuone.controlpanel import dbus_client
 from ubuntuone.controlpanel import replication_client
 from ubuntuone.controlpanel.logger import setup_logging, log_call
-from ubuntuone.controlpanel.webclient import WebClient
+from ubuntuone.controlpanel.webclient import WebClient, WebClientError
 
 
 logger = setup_logging('backend')
@@ -143,6 +143,49 @@
                                       _set_status_changed_handler)
 
     @inlineCallbacks
+    def _process_device_web_info(self, devices, enabled, limit_bw, limits,
+                                 show_notifs):
+        """Return a lis of processed devices."""
+        result = []
+        for d in devices:
+            di = {}
+            di["type"] = d["kind"]
+            di["name"] = d["description"]
+            di["configurable"] = ''
+            if di["type"] == DEVICE_TYPE_COMPUTER:
+                di["device_id"] = di["type"] + d["token"]
+            if di["type"] == DEVICE_TYPE_PHONE:
+                di["device_id"] = di["type"] + str(d["id"])
+
+            is_local = yield self.device_is_local(di["device_id"])
+            di["is_local"] = bool_str(is_local)
+            # currently, only local devices are configurable.
+            # eventually, more devices will be configurable.
+            di["configurable"] = bool_str(is_local and enabled)
+
+            if bool(di["configurable"]):
+                di["limit_bandwidth"] = bool_str(limit_bw)
+                di["show_all_notifications"] = bool_str(show_notifs)
+                upload = limits["upload"]
+                download = limits["download"]
+                di[UPLOAD_KEY] = str(upload)
+                di[DOWNLOAD_KEY] = str(download)
+
+            # date_added is not in the webservice yet (LP: #673668)
+            # di["date_added"] = ""
+
+            # missing values (LP: #673668)
+            # di["available_services"] = ""
+            # di["enabled_services"] = ""
+
+            if is_local:  # prepend the local device!
+                result.insert(0, di)
+            else:
+                result.append(di)
+
+        returnValue(result)
+
+    @inlineCallbacks
     def get_token(self):
         """Return the token from the credentials."""
         credentials = yield dbus_client.get_credentials()
@@ -188,47 +231,39 @@
     @inlineCallbacks
     def devices_info(self):
         """Get the user devices info."""
-        result = []
-        limit_bw = yield dbus_client.bandwidth_throttling_enabled()
-        show_all_notif = yield dbus_client.show_all_notifications_enabled()
-        limits = yield dbus_client.get_throttling_limits()
-
-        devices = yield self.wc.call_api(DEVICES_API)
-        for d in devices:
-            di = {}
-            di["type"] = d["kind"]
-            di["name"] = d["description"]
-            di["configurable"] = ''
-            if di["type"] == DEVICE_TYPE_COMPUTER:
-                di["device_id"] = di["type"] + d["token"]
-            if di["type"] == DEVICE_TYPE_PHONE:
-                di["device_id"] = di["type"] + str(d["id"])
-
-            is_local = yield self.device_is_local(di["device_id"])
-            di["is_local"] = bool_str(is_local)
-            # currently, only local devices are configurable.
-            # eventually, more the devices will be configurable.
-            di["configurable"] = bool_str(is_local)
-
-            if bool(di["configurable"]):
-                di["limit_bandwidth"] = bool_str(limit_bw)
-                di["show_all_notifications"] = bool_str(show_all_notif)
+        result = limit_bw = limits = show_notifs = None
+        enabled = yield dbus_client.files_sync_enabled()
+        if enabled:
+            limit_bw = yield dbus_client.bandwidth_throttling_enabled()
+            show_notifs = yield dbus_client.show_all_notifications_enabled()
+            limits = yield dbus_client.get_throttling_limits()
+
+        try:
+            devices = yield self.wc.call_api(DEVICES_API)
+        except WebClientError:
+            logger.exception('devices_info: web client failure:')
+        else:
+            result = yield self._process_device_web_info(devices, enabled,
+                                                         limit_bw, limits,
+                                                         show_notifs)
+        if result is None:
+            credentials = yield dbus_client.get_credentials()
+            local_device = {}
+            local_device["type"] = DEVICE_TYPE_COMPUTER
+            local_device["name"] = credentials['name']
+            device_id = local_device["type"] + credentials["token"]
+            local_device["device_id"] = device_id
+            local_device["is_local"] = bool_str(True)
+            local_device["configurable"] = bool_str(enabled)
+            if bool(local_device["configurable"]):
+                local_device["limit_bandwidth"] = bool_str(limit_bw)
+                show_notifs = bool_str(show_notifs)
+                local_device["show_all_notifications"] = show_notifs
                 upload = limits["upload"]
                 download = limits["download"]
-                di[UPLOAD_KEY] = str(upload)
-                di[DOWNLOAD_KEY] = str(download)
-
-            # date_added is not in the webservice yet (LP: #673668)
-            # di["date_added"] = ""
-
-            # missing values (LP: #673668)
-            # di["available_services"] = ""
-            # di["enabled_services"] = ""
-
-            if is_local:  # prepend the local device!
-                result.insert(0, di)
-            else:
-                result.append(di)
+                local_device[UPLOAD_KEY] = str(upload)
+                local_device[DOWNLOAD_KEY] = str(download)
+            result = [local_device]
 
         returnValue(result)
 

=== modified file 'ubuntuone/controlpanel/tests/__init__.py'
--- ubuntuone/controlpanel/tests/__init__.py	2011-03-02 19:54:09 +0000
+++ ubuntuone/controlpanel/tests/__init__.py	2011-03-29 17:12:42 +0000
@@ -23,7 +23,7 @@
 
 TOKEN = {u'consumer_key': u'xQ7xDAz',
          u'consumer_secret': u'KzCJWCTNbbntwfyCKKjomJDzlgqxLy',
-         u'token_name': u'test',
+         u'name': u'Ubuntu One @ localhost',
          u'token': u'ABCDEF01234-localtoken',
          u'token_secret': u'qFYImEtlczPbsCnYyuwLoPDlPEnvNcIktZphPQklAWrvyfFMV'}
 
@@ -110,19 +110,21 @@
 ]
 """
 
+LOCAL_DEVICE = {
+    'is_local': 'True',
+    'configurable': 'True',
+    'device_id': 'ComputerABCDEF01234-localtoken',
+    'limit_bandwidth': '',
+    'show_all_notifications': 'True',
+    'max_download_speed': '-1',
+    'max_upload_speed': '-1',
+    'name': 'Ubuntu One @ localhost',
+    'type': 'Computer',
+}
+
 # note that local computer should be first, do not change!
 EXPECTED_DEVICES_INFO = [
-    {
-        'is_local': 'True',
-        'configurable': 'True',
-        'device_id': 'ComputerABCDEF01234-localtoken',
-        'limit_bandwidth': '',
-        'show_all_notifications': 'True',
-        'max_download_speed': '-1',
-        'max_upload_speed': '-1',
-        'name': 'Ubuntu One @ localhost',
-        'type': 'Computer',
-    },
+    LOCAL_DEVICE,
     {
         "device_id": "ComputerABCDEF01234token",
         "name": "Ubuntu One @ darkstar",

=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py	2011-03-28 16:23:59 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py	2011-03-29 17:12:42 +0000
@@ -44,6 +44,7 @@
     EXPECTED_ACCOUNT_INFO,
     EXPECTED_ACCOUNT_INFO_WITH_CURRENT_PLAN,
     EXPECTED_DEVICES_INFO,
+    LOCAL_DEVICE,
     ROOT_PATH,
     SAMPLE_ACCOUNT_NO_CURRENT_PLAN,
     SAMPLE_ACCOUNT_WITH_CURRENT_PLAN,
@@ -340,9 +341,43 @@
     @inlineCallbacks
     def test_devices_info_fails(self):
         """The devices_info method exercises its errback."""
+        def fail(*args, **kwargs):
+            """Raise any error other than WebClientError."""
+            raise ValueError(args)
+
+        self.patch(self.be.wc, 'call_api', fail)
+        yield self.assertFailure(self.be.devices_info(), ValueError)
+
+    @inlineCallbacks
+    def test_devices_info_with_webclient_error(self):
+        """The devices_info returns local info if webclient error."""
         # pylint: disable=E1101
         self.be.wc.failure = 404
-        yield self.assertFailure(self.be.devices_info(), WebClientError)
+        result = yield self.be.devices_info()
+
+        self.assertEqual(result, [LOCAL_DEVICE])
+        self.assertTrue(self.memento.check_error('devices_info',
+                                                 'web client failure'))
+
+    @inlineCallbacks
+    def test_devices_info_if_files_disable(self):
+        """The devices_info returns device only info if files is disabled."""
+        yield self.be.disable_files()
+        status = yield self.be.file_sync_status()
+        assert status['status'] == backend.FILE_SYNC_DISABLED, status
+
+        # pylint: disable=E1101
+        self.be.wc.results[DEVICES_API] = SAMPLE_DEVICES_JSON
+        result = yield self.be.devices_info()
+
+        expected = EXPECTED_DEVICES_INFO[:]
+        for device in expected:
+            device.pop('limit_bandwidth', None)
+            device.pop('max_download_speed', None)
+            device.pop('max_upload_speed', None)
+            device.pop('show_all_notifications', None)
+            device['configurable'] = ''
+        self.assertEqual(result, expected)
 
     @inlineCallbacks
     def test_remove_device(self):
@@ -372,7 +407,8 @@
         """The remove_device method fails as expected."""
         # pylint: disable=E1101
         self.be.wc.failure = 404
-        yield self.assertFailure(self.be.devices_info(), WebClientError)
+        yield self.assertFailure(self.be.remove_device(self.local_token),
+                                 WebClientError)
 
     @inlineCallbacks
     def test_change_show_all_notifications(self):