Merge lp://staging/~tealeg/landscape-client/ui-permissions-and-panel into lp://staging/~landscape/landscape-client/trunk

[1] Please add docstrings to PolicyKitMechanism and its methods.

[2] Instead of exposing 10 methods in ConfigurationMechanism, 2 for each config value, would it be possible to have only 2 methods taking the config key as parameters? This would simplify ConfigurationProxy as well.

[3] All your tests need docstrings.

[4] Is it expected that register_fail/register_error don't do anything?

[5] I've changed trunk so that tests are skipped properly if the Gtk/Gdk imports fail, please preserve the behavior.

Thanks!

Hi Thomas.

I've addressed your points in the code and merged in the testing changes from trunk. Could you re-review please.

Specifically with regard to point 4: yes and no, or rather it is expected that those methods are essentially empty they are called for the side-effect implied by their decorators - it's quite common to just print the parameters instead of passing, but this looks messy to me. I have added comments to all four notification methods to explain why they just pass. Gotta love that DBus ;-)

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

It'd be good to reserve setUp for the real unittest setUp. Maybe "def configure_proxy" or something would be more explicit.

[7] Note that you don't need to add "pass" when a method has a docstring.

[8]
+ readables, w, x = select.select(
+ [self.process.stdout, self.process.stderr], [], [], 0)

There are so many ways this can go wrong... Let's switch to a blocking call for now, and we'll see if 1) we can improve landscape-config to handle progress better and then 2) send it there to the UI.

[9]
+ def _get_account_name(self):
+ return self._interface.get("account_name")
+
+ def _set_account_name(self, value):
+ self._interface.set("account_name", value)
+ account_name = property(_get_account_name, _set_account_name)

You should be able to reduce duplication with some functions:

def _delegate_to_interface(field):
    def get(self):
        return self._interface.get(field)
    def set(self, value):
        self._interface.set(field, value)

account_name = property(*_delegate_to_interface("account_name")
computer_title = property(*_delegate_to_interface("computer_title")

Thanks, +1!

All the above requests from Thomas have been implemented.

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):
+ ...