Merge lp:~diegosarmentero/ubuntuone-control-panel/limit-bandwidth into lp:ubuntuone-control-panel

Proposed by Diego Sarmentero
Status: Merged
Approved by: dobey
Approved revision: 304
Merged at revision: 301
Proposed branch: lp:~diegosarmentero/ubuntuone-control-panel/limit-bandwidth
Merge into: lp:ubuntuone-control-panel
Diff against target: 83 lines (+42/-2)
3 files modified
ubuntuone/controlpanel/backend.py (+7/-2)
ubuntuone/controlpanel/gui/qt/tests/test_preferences.py (+15/-0)
ubuntuone/controlpanel/tests/test_backend.py (+20/-0)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-control-panel/limit-bandwidth
Reviewer Review Type Date Requested Status
dobey (community) Approve
Natalia Bidart Approve
Review via email: mp+99575@code.launchpad.net

Commit message

- Adding limit_bandwidth attribute to the info dict processed by preferences (LP: #944256).

To post a comment you must log in.
Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Thanks for working on this!

We should not add a new entry LIMIT_BW_KEY in the dictionary since that will require a FFe (since the API changes), and also because is not necessary.

The bug is clearly where you detected it (the missing call to limits_enabled = yield self.sd_client.bandwidth_throttling_enabled()), so we should indeed add that call, and then make both result[DOWNLOAD_KEY] = limits['download'] and result[UPLOAD_KEY] be -1, since the agreement between the UI and the backend is:

- if limits are -1, then it means that throttling is disabled.
- if any limit is not -1, then throttling is enabled for that limit.

Does the above make sense?

review: Needs Fixing
302. By Diego Sarmentero

Improves in limit detection.

303. By Diego Sarmentero

Adding test for backend

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

I really like the code!

One tiny note: the test added should have something like this:

    @inlineCallbacks
    def test_file_sync_settings_info_with_limit(self):
        """The settings_info method exercises its callback."""
        self.patch(self.be.sd_client, "throttling", False)
        self.be.sd_client.limits = {"download": 987456, "upload": 125698}

        # ...

            backend.DOWNLOAD_KEY: -1,
            backend.UPLOAD_KEY: -1,
        }

so we fully test that even if SD limits are not -1, the limits returned to the UI are -1 when throttling is disabled.

review: Needs Fixing
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

> I really like the code!
>
> One tiny note: the test added should have something like this:
>
> @inlineCallbacks
> def test_file_sync_settings_info_with_limit(self):
> """The settings_info method exercises its callback."""
> self.patch(self.be.sd_client, "throttling", False)
> self.be.sd_client.limits = {"download": 987456, "upload": 125698}
>
> # ...
>
> backend.DOWNLOAD_KEY: -1,
> backend.UPLOAD_KEY: -1,
> }
>
> so we fully test that even if SD limits are not -1, the limits returned to the
> UI are -1 when throttling is disabled.

Fixed

304. By Diego Sarmentero

Improving test.

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Looks and works great!

review: Approve
Revision history for this message
dobey (dobey) :
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-01 22:05:51 +0000
3+++ ubuntuone/controlpanel/backend.py 2012-03-28 17:13:17 +0000
4@@ -787,8 +787,13 @@
5 result[name] = bool(value)
6
7 limits = yield self.sd_client.get_throttling_limits()
8- result[DOWNLOAD_KEY] = limits['download']
9- result[UPLOAD_KEY] = limits['upload']
10+ limits_enabled = yield self.sd_client.bandwidth_throttling_enabled()
11+ if limits_enabled:
12+ result[DOWNLOAD_KEY] = limits['download']
13+ result[UPLOAD_KEY] = limits['upload']
14+ else:
15+ result[DOWNLOAD_KEY] = -1
16+ result[UPLOAD_KEY] = -1
17
18 returnValue(result)
19
20
21=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_preferences.py'
22--- ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-03-01 22:05:51 +0000
23+++ ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-03-28 17:13:17 +0000
24@@ -76,6 +76,21 @@
25
26 self.assertFalse(self.ui.is_processing)
27
28+ def test_process_info_limit_bandwidth(self):
29+ """The ui is not processing when contents are load."""
30+ self.ui.process_info(SAMPLE_SETTINGS)
31+
32+ self.assertTrue(self.ui.ui.limit_uploads_checkbox.isChecked())
33+ self.assertTrue(self.ui.ui.limit_downloads_checkbox.isChecked())
34+
35+ settings = SAMPLE_SETTINGS.copy()
36+ settings[gui.backend.DOWNLOAD_KEY] = -1
37+ settings[gui.backend.UPLOAD_KEY] = -1
38+ self.ui.process_info(settings)
39+
40+ self.assertFalse(self.ui.ui.limit_uploads_checkbox.isChecked())
41+ self.assertFalse(self.ui.ui.limit_downloads_checkbox.isChecked())
42+
43 def test_info_is_requested_on_load(self):
44 """The info is requested to the backend."""
45 self.assert_backend_called('file_sync_settings_info')
46
47=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
48--- ubuntuone/controlpanel/tests/test_backend.py 2012-02-06 15:23:27 +0000
49+++ ubuntuone/controlpanel/tests/test_backend.py 2012-03-28 17:13:17 +0000
50@@ -1598,6 +1598,7 @@
51 @inlineCallbacks
52 def test_file_sync_settings_info(self):
53 """The settings_info method exercises its callback."""
54+ self.patch(self.be.sd_client, "throttling", True)
55 self.be.sd_client.limits = {"download": 1000, "upload": 100}
56 expected = {
57 backend.AUTOCONNECT_KEY: self.be.sd_client.autoconnect,
58@@ -1614,6 +1615,25 @@
59 self.assertEqual(expected, result)
60
61 @inlineCallbacks
62+ def test_file_sync_settings_info_with_limit(self):
63+ """The settings_info method exercises its callback."""
64+ self.patch(self.be.sd_client, "throttling", False)
65+ self.be.sd_client.limits = {"download": 987456, "upload": 125698}
66+ expected = {
67+ backend.AUTOCONNECT_KEY: self.be.sd_client.autoconnect,
68+ backend.SHOW_ALL_NOTIFICATIONS_KEY:
69+ self.be.sd_client.show_all_notifications,
70+ backend.SHARE_AUTOSUBSCRIBE_KEY:
71+ self.be.sd_client.share_autosubscribe,
72+ backend.UDF_AUTOSUBSCRIBE_KEY:
73+ self.be.sd_client.udf_autosubscribe,
74+ backend.DOWNLOAD_KEY: -1,
75+ backend.UPLOAD_KEY: -1,
76+ }
77+ result = yield self.be.file_sync_settings_info()
78+ self.assertEqual(expected, result)
79+
80+ @inlineCallbacks
81 def test_change_file_sync_setting_autoconnect(self):
82 """The settings can be changed for autoconnect."""
83 yield self.assert_boolean_setting_is_correct(backend.AUTOCONNECT_KEY)

Subscribers

People subscribed via source and target branches