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
=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py 2012-03-01 22:05:51 +0000
+++ ubuntuone/controlpanel/backend.py 2012-03-28 17:13:17 +0000
@@ -787,8 +787,13 @@
787 result[name] = bool(value)787 result[name] = bool(value)
788788
789 limits = yield self.sd_client.get_throttling_limits()789 limits = yield self.sd_client.get_throttling_limits()
790 result[DOWNLOAD_KEY] = limits['download']790 limits_enabled = yield self.sd_client.bandwidth_throttling_enabled()
791 result[UPLOAD_KEY] = limits['upload']791 if limits_enabled:
792 result[DOWNLOAD_KEY] = limits['download']
793 result[UPLOAD_KEY] = limits['upload']
794 else:
795 result[DOWNLOAD_KEY] = -1
796 result[UPLOAD_KEY] = -1
792797
793 returnValue(result)798 returnValue(result)
794799
795800
=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_preferences.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-03-01 22:05:51 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-03-28 17:13:17 +0000
@@ -76,6 +76,21 @@
7676
77 self.assertFalse(self.ui.is_processing)77 self.assertFalse(self.ui.is_processing)
7878
79 def test_process_info_limit_bandwidth(self):
80 """The ui is not processing when contents are load."""
81 self.ui.process_info(SAMPLE_SETTINGS)
82
83 self.assertTrue(self.ui.ui.limit_uploads_checkbox.isChecked())
84 self.assertTrue(self.ui.ui.limit_downloads_checkbox.isChecked())
85
86 settings = SAMPLE_SETTINGS.copy()
87 settings[gui.backend.DOWNLOAD_KEY] = -1
88 settings[gui.backend.UPLOAD_KEY] = -1
89 self.ui.process_info(settings)
90
91 self.assertFalse(self.ui.ui.limit_uploads_checkbox.isChecked())
92 self.assertFalse(self.ui.ui.limit_downloads_checkbox.isChecked())
93
79 def test_info_is_requested_on_load(self):94 def test_info_is_requested_on_load(self):
80 """The info is requested to the backend."""95 """The info is requested to the backend."""
81 self.assert_backend_called('file_sync_settings_info')96 self.assert_backend_called('file_sync_settings_info')
8297
=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py 2012-02-06 15:23:27 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py 2012-03-28 17:13:17 +0000
@@ -1598,6 +1598,7 @@
1598 @inlineCallbacks1598 @inlineCallbacks
1599 def test_file_sync_settings_info(self):1599 def test_file_sync_settings_info(self):
1600 """The settings_info method exercises its callback."""1600 """The settings_info method exercises its callback."""
1601 self.patch(self.be.sd_client, "throttling", True)
1601 self.be.sd_client.limits = {"download": 1000, "upload": 100}1602 self.be.sd_client.limits = {"download": 1000, "upload": 100}
1602 expected = {1603 expected = {
1603 backend.AUTOCONNECT_KEY: self.be.sd_client.autoconnect,1604 backend.AUTOCONNECT_KEY: self.be.sd_client.autoconnect,
@@ -1614,6 +1615,25 @@
1614 self.assertEqual(expected, result)1615 self.assertEqual(expected, result)
16151616
1616 @inlineCallbacks1617 @inlineCallbacks
1618 def test_file_sync_settings_info_with_limit(self):
1619 """The settings_info method exercises its callback."""
1620 self.patch(self.be.sd_client, "throttling", False)
1621 self.be.sd_client.limits = {"download": 987456, "upload": 125698}
1622 expected = {
1623 backend.AUTOCONNECT_KEY: self.be.sd_client.autoconnect,
1624 backend.SHOW_ALL_NOTIFICATIONS_KEY:
1625 self.be.sd_client.show_all_notifications,
1626 backend.SHARE_AUTOSUBSCRIBE_KEY:
1627 self.be.sd_client.share_autosubscribe,
1628 backend.UDF_AUTOSUBSCRIBE_KEY:
1629 self.be.sd_client.udf_autosubscribe,
1630 backend.DOWNLOAD_KEY: -1,
1631 backend.UPLOAD_KEY: -1,
1632 }
1633 result = yield self.be.file_sync_settings_info()
1634 self.assertEqual(expected, result)
1635
1636 @inlineCallbacks
1617 def test_change_file_sync_setting_autoconnect(self):1637 def test_change_file_sync_setting_autoconnect(self):
1618 """The settings can be changed for autoconnect."""1638 """The settings can be changed for autoconnect."""
1619 yield self.assert_boolean_setting_is_correct(backend.AUTOCONNECT_KEY)1639 yield self.assert_boolean_setting_is_correct(backend.AUTOCONNECT_KEY)

Subscribers

People subscribed via source and target branches