Code review comment for lp://staging/~tealeg/landscape-client/ui-permissions-and-panel

Hi Geoff, nice work, I have a few comments but mostly stylistic ones. The changeset is rather large and takes a while to read, please break it down in smaller branches next time (the diff should be ~500 lines).

+1!

[1]

+it's calls via DBus to the L{ConfigurationMechanism}.

s/it's/its/

[2]

+class RegistrationProxy(object):

It'd be good to add a class docstring for this class.

[3]

+class ConfigurationProxyBaseTest(LandscapeTest):

It'd be good to use composition instead of inheritance. The LandscapeTest class has support for "helpers" (sort of plugins/fixtures), you can find some examples in landscape.tests.helpers (see HelperTestCase) and landscape.broker.tests.helpers. We could have landscape.ui.tests.helpers.ConfigurationProxyHelper.

[4]

+ class MyLandscapeSetupConfiguration(LandscapeSetupConfiguration):
+ default_config_filenames = [self.config_filename]
+
+ self.config = MyLandscapeSetupConfiguration()

I *think* you don't need a full subclass for this, just:

        self.config = LandscapeSetupConfiguration()
        self.config.default_config_filenames = [self.config_filename]

[5]

+ # from dbus.service.Object which throws a fit it notices you using it

s/it notices/if it notices/ ?

[6]

+ ConfigurationProxy._setup_interface = setup_interface
+ self.proxy = ConfigurationProxy()

This seems to modify the actual class (without cleaning it in tearDown). Also, you could achieve the same without patching the original class by adding the interface object as parameter:

    def __init__(self, interface=None):
        self._interface = interface
        if self._interface is None:
            self._bus = dbus.SystemBus()
            self._remote_object = self._bus.get_object(SERVICE_NAME, OBJECT_PATH)
            self._interface = dbus.Interface(self._remote_object, INTERFACE_NAME)

[7]

+ implemented via PolicyKit policy. The use of DBus results from the use of
+ PolicyKit, not the other way around, and is done that way because that is
+ considered to be the Right Thing for Ubuntu Desktop circa January 2012.

Nice design defense :)

[8]

+ def __init__(self, config, bus_name, bypass=False, conn=None):
+ super(ConfigurationMechanism, self).__init__(
+ OBJECT_PATH, bus_name, PermissionDeniedByPolicy,
+ bypass=bypass, conn=conn)
+ self.config = config

I presume self.config could be private (self._config).

[9]

+ Test that we can get and set and account name via the configuration

s/and account/an account/

[10]

+ self.assertEqual(self.proxy.account_name, "foo")
+ self.proxy.account_name = "bar"
+ self.assertEqual(self.proxy.account_name, "bar")

It's probably not strictly needed, but it'd be nice to also assert that the "remote" config object gets actually modified:

self.assertEqual(self.config.account_name, "bar")

[11]

All the assertions should be inverted, the pattern we adopt is the one recommended by unittest and (xUnit in general afaict):

self.assertEqual(expected_value, actual_value)

It's been counter-intuitive for me at start, but I eventually adjusted.

[12]

+ def setUp(self, config):
+ super(ConfigurationProxyBaseTest, self).setUp()
+ self.config_filename = self.makeFile(config)

Do we really need to pass the config content around? I believe it would suffice to let this class build an empty self.config object and let tests get/set the values they need directly on it. That would reduce the boiler-plate I think.