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

Proposed by Diego Sarmentero on 2012-05-10
Status: Merged
Approved by: Diego Sarmentero on 2012-05-14
Approved revision: 320
Merged at revision: 318
Proposed branch: lp:~diegosarmentero/ubuntuone-control-panel/sync-status
Merge into: lp:ubuntuone-control-panel
Diff against target: 84 lines (+31/-1)
4 files modified
ubuntuone/controlpanel/backend.py (+6/-1)
ubuntuone/controlpanel/gui/qt/filesyncstatus.py (+2/-0)
ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py (+15/-0)
ubuntuone/controlpanel/tests/test_backend.py (+8/-0)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-control-panel/sync-status
Reviewer Review Type Date Requested Status
Mike McCracken (community) Approve on 2012-05-11
Roberto Alsina (community) 2012-05-10 Approve on 2012-05-11
Review via email: mp+105394@code.launchpad.net

Commit message

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

To post a comment you must log in.
Roberto Alsina (ralsina) wrote :

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) wrote :

> 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.

319. By Diego Sarmentero on 2012-05-11

Checking first for Mapping

Roberto Alsina (ralsina) :
review: Approve
Mike McCracken (mikemc) wrote :

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
320. By Diego Sarmentero on 2012-05-11

Fixing docstrings.

Mike McCracken (mikemc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/backend.py'
2--- ubuntuone/controlpanel/backend.py 2012-03-29 21:41:54 +0000
3+++ ubuntuone/controlpanel/backend.py 2012-05-11 13:33:18 +0000
4@@ -16,6 +16,7 @@
5
6 """A backend for the Ubuntu One Control Panel."""
7
8+import collections
9 import operator
10 import os
11
12@@ -152,9 +153,13 @@
13 FILE_SYNC_UNKNOWN
14 * MSG_KEY: a non translatable but human readable string of the status.
15
16+ Return None if we receive and invalid type in status.
17+
18 """
19 logger.debug('sync status: %r', status)
20- if not status:
21+ if not isinstance(status, collections.Mapping):
22+ return None
23+ elif not status:
24 self.file_sync_disabled = True
25 return self.STATUS_DISABLED
26
27
28=== modified file 'ubuntuone/controlpanel/gui/qt/filesyncstatus.py'
29--- ubuntuone/controlpanel/gui/qt/filesyncstatus.py 2012-03-02 13:53:24 +0000
30+++ ubuntuone/controlpanel/gui/qt/filesyncstatus.py 2012-05-11 13:33:18 +0000
31@@ -139,6 +139,8 @@
32 @log_call(logger.debug)
33 def process_info(self, status):
34 """Match status with signals."""
35+ if status is None:
36+ return
37 try:
38 status_key = status[backend.STATUS_KEY]
39 data = FILE_SYNC_STATUS[status_key]
40
41=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py'
42--- ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py 2012-03-12 16:36:06 +0000
43+++ ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py 2012-05-11 13:33:18 +0000
44@@ -93,6 +93,21 @@
45 self.assertIsInstance(self.ui.backend,
46 backend.ControlBackend)
47
48+ def test_process_info_invalid_status(self):
49+ """File sync status invalid, ignore event."""
50+ status = {backend.STATUS_KEY: backend.FILE_SYNC_STARTING,
51+ backend.MSG_KEY: gui.FILE_SYNC_STARTING,
52+ 'icon': backend.FILE_SYNC_STARTING}
53+ self.ui.process_info(status)
54+ self.ui.process_info(3)
55+
56+ actual_icon = self.ui.ui.sync_status_icon.pixmap()
57+ pixmap_name = gui.icon_name_from_status(backend.FILE_SYNC_STARTING)
58+ expected_icon = gui.pixmap_from_name(pixmap_name)
59+ self.assertEqualPixmaps(expected_icon, actual_icon)
60+ actual_text = unicode(self.ui.ui.sync_status_label.text())
61+ self.assertEqual(gui.FILE_SYNC_STARTING, actual_text)
62+
63 def test_process_info_changed(self):
64 """Backend's file sync status changed callback is connected."""
65 self.assertEqual(self.ui.backend.status_changed_handler,
66
67=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
68--- ubuntuone/controlpanel/tests/test_backend.py 2012-03-29 21:37:22 +0000
69+++ ubuntuone/controlpanel/tests/test_backend.py 2012-05-11 13:33:18 +0000
70@@ -1370,6 +1370,14 @@
71 expected_status = self.be._process_file_sync_status(status)
72 self.assertEqual(self._called, ((expected_status,), {}))
73
74+ def test_invalid_status_type(self):
75+ """Check that the method return None with invalid types."""
76+ self.be.status_changed_handler = self._set_called
77+ status = 3
78+ self.be.sd_client.status_changed_handler(status)
79+
80+ self.assertEqual(self._called, ((None,), {}))
81+
82
83 class BackendSyncStatusIfDisabledTestCase(BackendSyncStatusTestCase):
84 """Syncdaemon state for the backend when file sync is disabled."""

Subscribers

People subscribed via source and target branches