Ubuntu One Client

Merge lp:~dobey/ubuntuone-client/fix-570721-stable into lp:ubuntuone-client/stable-1-2

  1. fix-570721-stable
  2. Merge into stable-1-2
Proposed by dobey
Status: Merged
Approved by: Natalia Bidart
Approved revision: 513
Merged at revision: 515
Proposed branch: lp:~dobey/ubuntuone-client/fix-570721-stable
Merge into: lp:ubuntuone-client/stable-1-2
Diff against target: 37 lines (+3/-2)
3 files modified
bin/ubuntuone-preferences (+1/-1)
ubuntuone/syncdaemon/dbus_interface.py (+1/-1)
ubuntuone/syncdaemon/tools.py (+1/-0)
To merge this branch: bzr merge lp:~dobey/ubuntuone-client/fix-570721-stable
Reviewer Review Type Date Requested Status
Natalia Bidart (community) Approve
Vincenzo Di Somma (community) Approve
Review via email: mp+26245@code.launchpad.net

Commit message

Fix typo in music_check_toggled call (removed preceding __)
Convert the DBusBool '0' to int then Python bool for storing
Update the internal config object with the new value after calling DBus

To post a comment you must log in.
Revision history for this message
Vincenzo Di Somma (vds) wrote :

Looks fine.

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

There should be tests for the change in ubuntuone/syncdaemon/dbus_interface.py and ubuntuone/syncdaemon/tools.py.

review: Needs Fixing
Revision history for this message
dobey (dobey) wrote :

> There should be tests for the change in ubuntuone/syncdaemon/dbus_interface.py
> and ubuntuone/syncdaemon/tools.py.

This change was already approved/landed as-is in trunk, and this is just a backport of that revision to the stable-1-2 branch. I don't think we should be writing tests for stable-1-2 and then trying to forward port them to trunk if necessary. If tests are a must-have for tools.py (which is largely lacking tests outside the scope of this change anyway), these tests should be done in a branch for trunk and then backported to stable-1-2 if necessary. Although I don't think tests are as valuable for most of tools.py as they are simply calling dbus methods which are already tested, inside a Deferred.

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

> > There should be tests for the change in
> ubuntuone/syncdaemon/dbus_interface.py
> > and ubuntuone/syncdaemon/tools.py.
>
> This change was already approved/landed as-is in trunk, and this is just a
> backport of that revision to the stable-1-2 branch. I don't think we should be
> writing tests for stable-1-2 and then trying to forward port them to trunk if
> necessary. If tests are a must-have for tools.py (which is largely lacking
> tests outside the scope of this change anyway), these tests should be done in
> a branch for trunk and then backported to stable-1-2 if necessary. Although I
> don't think tests are as valuable for most of tools.py as they are simply
> calling dbus methods which are already tested, inside a Deferred.

The change in the file tools.py is modifying the logic of the method, so is not just a wrapper to a tested function. I'm not suggesting a whole suite covering the whole module, but a unit test covering the change you just added (config.set_files_sync_enabled(False)). Same for the change in dbus_interface.

The mentioned tests should be very quick to build. If you need some hints, feel free to ask me.

I agree that tests should be back-ported from trunk. Could you please add the tests in trunk and link that branch here? I'll be happy to review both.

Revision history for this message
dobey (dobey) wrote :

Bug for needing tests: https://bugs.edge.launchpad.net/ubuntuone-client/+bug/589894

Revision history for this message
Natalia Bidart (nataliabidart) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/ubuntuone-preferences'
2--- bin/ubuntuone-preferences 2010-04-21 20:33:11 +0000
3+++ bin/ubuntuone-preferences 2010-05-27 20:48:28 +0000
4@@ -802,7 +802,7 @@
5 if self.ums_id:
6 self.music_check.set_sensitive(True)
7 if self.music_check.get_active():
8- self.__music_check_toggled(self.music_check)
9+ self.music_check_toggled(self.music_check)
10 else:
11 self.sdtool.quit().addCallback(
12 lambda _: self.devices.list_devices())
13
14=== modified file 'ubuntuone/syncdaemon/dbus_interface.py'
15--- ubuntuone/syncdaemon/dbus_interface.py 2010-05-17 22:23:56 +0000
16+++ ubuntuone/syncdaemon/dbus_interface.py 2010-05-27 20:48:28 +0000
17@@ -1214,7 +1214,7 @@
18 """Enable UDF autosubscribe."""
19 logger.debug('called set_files_sync_enabled %d', enabled)
20 user_config = config.get_user_config()
21- user_config.set_files_sync_enabled(enabled)
22+ user_config.set_files_sync_enabled(bool(int(enabled)))
23 user_config.save()
24
25 @dbus.service.method(DBUS_IFACE_CONFIG_NAME,
26
27=== modified file 'ubuntuone/syncdaemon/tools.py'
28--- ubuntuone/syncdaemon/tools.py 2010-04-14 19:01:12 +0000
29+++ ubuntuone/syncdaemon/tools.py 2010-05-27 20:48:28 +0000
30@@ -719,6 +719,7 @@
31 enabled,
32 reply_handler=d.callback,
33 error_handler=d.errback)
34+ config.set_files_sync_enabled(False)
35 return d
36 else:
37 if enabled:

Subscribers

People subscribed via source and target branches

to all changes: