Landscape Client

Merge lp://staging/~tealeg/landscape-client/disable-client-from-ui into lp://staging/~landscape/landscape-client/trunk

  1. disable-client-from-ui
  2. Merge into trunk
Proposed by Geoff Teale
Status: Merged
Approved by: Mike Milner
Approved revision: 506
Merged at revision: 477
Proposed branch: lp://staging/~tealeg/landscape-client/disable-client-from-ui
Merge into: lp://staging/~landscape/landscape-client/trunk
Prerequisite: lp://staging/~tealeg/landscape-client/ui-interface-iteration3
Diff against target: 579 lines (+218/-175)
9 files modified
landscape/ui/controller/app.py (+2/-3)
landscape/ui/controller/configuration.py (+28/-12)
landscape/ui/controller/tests/test_configuration.py (+11/-2)
landscape/ui/model/registration/mechanism.py (+40/-0)
landscape/ui/model/registration/proxy.py (+66/-32)
landscape/ui/model/registration/tests/test_mechanism.py (+38/-3)
landscape/ui/model/registration/tests/test_proxy.py (+33/-20)
landscape/ui/view/configuration.py (+0/-3)
landscape/ui/view/tests/test_configuration.py (+0/-100)
To merge this branch: bzr merge lp://staging/~tealeg/landscape-client/disable-client-from-ui
Reviewer Review Type Date Requested Status
Mike Milner (community) Approve
Fernando Correa Neto (community) Approve
Review via email: mp+95588@code.staging.launchpad.net

Description of the change

Fixes bug #944652.

Note, this depends on the ui-interface-iteration3 branch.

This branch does the following:

   - Adds a call to "landscape-config --disable" into the RegistrationMechanism class.
   - Adds a DBus method "disable" to the RegistrationMechanism interface.
   - Adds a "disable" passthrough method to the RegistrationProxy.
   - Removes "persist" from the view level code
   - Makes the App controller use the ConfigController instead of the view to persist settings
   - Makes ConfigController.persist switch on the configured management type and either register the client or disable the client as appropriate.

To post a comment you must log in.
Revision history for this message
Fernando Correa Neto (fcorrea) wrote :

Looks good, +1!

review: Approve
lp://staging/~tealeg/landscape-client/disable-client-from-ui updated
506. By Geoff Teale

merged forward trunk

Revision history for this message
Mike Milner (milner) wrote :

Looks good! +1

I just have a few issues with names/docs in the tests:

[1]
303 + def make_disabling(self, suceed):
304 +
305 + def _do_disabling(this):
306 + return suceed
307 +
308 + return _do_disabling
309 +

This function (and make_registration just above it) need documentation. The variable names are also very confusing. If these are used as fake callbacks, please use "fake" in their name somewhere so it's clear. Similar issues with tests in landscape/ui/model/registration/tests/test_proxy.py

[2]
303 + def make_disabling(self, suceed):

Small typo, should be succeed.

[3]
324 + self.assertEqual(True, self.mechanism.disable())

Should use assertTrue(self.mechanism.disable()) and assertFalse() in the test below.

review: Approve
Revision history for this message
Geoff Teale (tealeg) wrote :

> Looks good! +1
>
> I just have a few issues with names/docs in the tests:
>
> [1]
> 303 + def make_disabling(self, suceed):
> 304 +
> 305 + def _do_disabling(this):
> 306 + return suceed
> 307 +
> 308 + return _do_disabling
> 309 +
>
> This function (and make_registration just above it) need documentation. The
> variable names are also very confusing. If these are used as fake callbacks,
> please use "fake" in their name somewhere so it's clear. Similar issues with
> tests in landscape/ui/model/registration/tests/test_proxy.py

Done.

>
> [2]
> 303 + def make_disabling(self, suceed):
>
> Small typo, should be succeed.

Done.

>
> [3]
> 324 + self.assertEqual(True, self.mechanism.disable())
>
> Should use assertTrue(self.mechanism.disable()) and assertFalse() in the test
> below.

Done.

lp://staging/~tealeg/landscape-client/disable-client-from-ui updated
507. By Geoff Teale

Addressed milners review points.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: