Merge lp://staging/~tealeg/landscape-client/ui-interface-iteration3 into lp://staging/~landscape/landscape-client/trunk

Looks great!

[1]

427 + except StateError:
428 + # We probably don't care.
429 + pass

Even if we don't care, maybe we should log something instead of not handling it?

[2]

975 + self._state._proxy.account_name = \
976 + self._state.get(first_key, ACCOUNT_NAME)
977 + self._state._proxy.registration_password = \
978 + self._state.get(first_key, PASSWORD)

I've noticed in previous reviews that we should avoid breaking likes using backslash. Maybe the following could make it consistent with the rest of our codebase.

            self._state._proxy.account_name = self._state.get(
                first_key, ACCOUNT_NAME)
            self._state._proxy.registration_password = self._state.get(
                first_key, PASSWORD)

(Didn't check if that goes beyond the 79char limit)

Also, do you think it would make sense to move l.ui.tests.helpers to a l.ui.testing module so it's more consistent with what we have in the server?
No biggie on this one, just trying to keep consistency at all costs :)

+1!

Tealeg, I like this iteration of the UI configurator as I feel the layout and prompting makes the dialog much more clear. The ability to understand that you will be setting up the client to be managed by landscape or not in a simple drop down seems to best illustrate the consequences of the available config actions.

[1] getting an error on ./scripts/landscape-client-settings-ui exit when I choose any of the options:
"Do not manage with this computer landscape"
"Use Canonical's hosted landscape service"
"Use dedicated landscape system"

./scripts/landscape-client-settings-ui
Traceback (most recent call last):
  File "./landscape/ui/controller/app.py", line 60, in setup_ui
    self.settings_dialog.persist()
  File "./landscape/ui/view/configuration.py", line 158, in persist
    self.controller.persist()
  File "./landscape/ui/model/configuration/state.py", line 437, in persist
    self._current_state = self._current_state.persist()
  File "./landscape/ui/model/configuration/state.py", line 367, in persist
    return self._unpersistable_helper.persist()
  File "./landscape/ui/model/configuration/state.py", line 285, in persist
    " cannot be transitioned via persist().")
landscape.ui.model.configuration.state.StateError: A ConfiguratonModel in UnpersistableHelper cannot be transitioned via persist().

[2] a number of lint failures are surfaced via "make lint"
bulk of the lint failures seem to in tests that are missing imports for constants from landscape/ui/model/configuration/state.py or landscape.ui.constants

[3] +ping_url = http://localhost:8080/ping
./dev/racercar by default throws the ping server at port 8081. Any reason for the shift to 8080 in landscape-client.conf?

still have a bit more to review on this. abstaining until I apply the related MP that might handle some of the persist errors.

If [1] is an easy fix, the settings UI appears to write the appropriate values to the /etc/landscape/client.conf appropriately. Just need to handle the error.

> Looks great!
>
> [1]
>
> 427 + except StateError:
> 428 + # We probably don't care.
> 429 + pass
>
> Even if we don't care, maybe we should log something instead of not handling
> it?

Actually this error is just telling us that there are no changes to save, originally I had the idea that we would disable the OK button until changes were made, but this is generally a bad thing from a UX point of view, so instead I just catch the error and push on. In the later branch this results in a registration attempt.

Now logging as suggested.

>
>
> [2]
>
> 975 + self._state._proxy.account_name = \
> 976 + self._state.get(first_key, ACCOUNT_NAME)
> 977 + self._state._proxy.registration_password = \
> 978 + self._state.get(first_key, PASSWORD)
>
> I've noticed in previous reviews that we should avoid breaking likes using
> backslash. Maybe the following could make it consistent with the rest of our
> codebase.
>
> self._state._proxy.account_name = self._state.get(
> first_key, ACCOUNT_NAME)
> self._state._proxy.registration_password = self._state.get(
> first_key, PASSWORD)
>
> (Didn't check if that goes beyond the 79char limit)

Done!

>
> Also, do you think it would make sense to move l.ui.tests.helpers to a
> l.ui.testing module so it's more consistent with what we have in the server?
> No biggie on this one, just trying to keep consistency at all costs :)

I'd rather be consistent within the client for now and maybe we can convert all cases as a separate task.

> +1!

> Tealeg, I like this iteration of the UI configurator as I feel the layout and
> prompting makes the dialog much more clear. The ability to understand that
> you will be setting up the client to be managed by landscape or not in a
> simple drop down seems to best illustrate the consequences of the available
> config actions.
>

Thanks, it's not my idea, it was just my interpretation of what was discussed. Glad you like it though.

> [1] getting an error on ./scripts/landscape-client-settings-ui exit when I
> choose any of the options:
> "Do not manage with this computer landscape"
> "Use Canonical's hosted landscape service"
> "Use dedicated landscape system"
>
>
> ./scripts/landscape-client-settings-ui
> Traceback (most recent call last):
> File "./landscape/ui/controller/app.py", line 60, in setup_ui
> self.settings_dialog.persist()
> File "./landscape/ui/view/configuration.py", line 158, in persist
> self.controller.persist()
> File "./landscape/ui/model/configuration/state.py", line 437, in persist
> self._current_state = self._current_state.persist()
> File "./landscape/ui/model/configuration/state.py", line 367, in persist
> return self._unpersistable_helper.persist()
> File "./landscape/ui/model/configuration/state.py", line 285, in persist
> " cannot be transitioned via persist().")
> landscape.ui.model.configuration.state.StateError: A ConfiguratonModel in
> UnpersistableHelper cannot be transitioned via persist().
>

Ah, yes - the call is transparently passing through the controller to the model, which is wrong. I have addressed this, although it actually goes away in a later branch anyhow. Easier to think about the branches in isolation I think.

> [2] a number of lint failures are surfaced via "make lint"
> bulk of the lint failures seem to in tests that are missing imports for
> constants from landscape/ui/model/configuration/state.py or
> landscape.ui.constants
>

Hmm, any chance you can provide output? My lint shows no errors.

>
> [3] +ping_url = http://localhost:8080/ping
> ./dev/racercar by default throws the ping server at port 8081. Any reason for
> the shift to 8080 in landscape-client.conf?

No.. it just got checked in accidentally! 2nd time that has happened, good catch!

>
>
> still have a bit more to review on this. abstaining until I apply the related
> MP that might handle some of the persist errors.
>

OK - I'll check in with you later today.

>
> If [1] is an easy fix, the settings UI appears to write the appropriate values
> to the /etc/landscape/client.conf appropriately. Just need to handle the
> error.

See above, we should be good.

[1] pyflakes

landscape/ui/controller/tests/test_app.py:14: redefinition of unused 'SettingsApplicationController' from line 9
landscape/ui/controller/tests/test_configuration.py:6: 'ConfigControllerLockError' imported but unused
landscape/ui/controller/tests/test_configuration.py:12: redefinition of unused 'ConfigController' from line 6
landscape/ui/model/configuration/tests/test_state.py:12: redefinition of unused 'landscape' from line 6
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'ConfigurationModel' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'StateError' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'VirginState' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'InitialisedState' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'ModifiedState' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'HOSTED' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'LOCAL' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'HOSTED_LANDSCAPE_HOST' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'LANDSCAPE_HOST' from line 7
landscape/ui/model/configuration/tests/test_state.py:13: redefinition of unused 'COMPUTER_TITLE' from line 7

[2]
+ try:
+ setattr(self._configuration, name, value)
+ self._configuration.modify()
+ except AttributeError:
+ return object.__setattr__(self, name, value)

I think the modify call should be in the else clause.

[3] Can you use triple quotes for docstring everywhere, in particular ConfigController?

[4]
+ self.default_data = {"management-type": "not",
+ "computer-title": "",
+ "hosted-landscape-host": "",
+ "hosted-account-name": "",
+ "hosted-password": "",
+ "local-landscape-host": "",
+ "local-account-name": "",
+ "local-password": ""
+ }

Nitpicking again, but the last "}" should be on the line before.

[5]
+from landscape.ui.constants import (
+ CANONICAL_MANAGED, NOT_MANAGED)

This can be on one line.

[6] It'd be nice to have some more docstrings on ClientSettingsDialog.

Great work, +1!

> [1] pyflakes
>
> landscape/ui/controller/tests/test_app.py:14: redefinition of unused
> 'SettingsApplicationController' from line 9
> landscape/ui/controller/tests/test_configuration.py:6:
> 'ConfigControllerLockError' imported but unused
> landscape/ui/controller/tests/test_configuration.py:12: redefinition of unused
> 'ConfigController' from line 6
> landscape/ui/model/configuration/tests/test_state.py:12: redefinition of
> unused 'landscape' from line 6
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'ConfigurationModel' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'StateError' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'VirginState' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'InitialisedState' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'ModifiedState' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'HOSTED' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'LOCAL' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'HOSTED_LANDSCAPE_HOST' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'LANDSCAPE_HOST' from line 7
> landscape/ui/model/configuration/tests/test_state.py:13: redefinition of
> unused 'COMPUTER_TITLE' from line 7
>

Addressed, apart from one which sadly has to be there.

> [2]
> + try:
> + setattr(self._configuration, name, value)
> + self._configuration.modify()
> + except AttributeError:
> + return object.__setattr__(self, name, value)
>
> I think the modify call should be in the else clause.

Makes sense. Done.

> [3] Can you use triple quotes for docstring everywhere, in particular
> ConfigController?

Done.

>
> [4]
> + self.default_data = {"management-type": "not",
> + "computer-title": "",
> + "hosted-landscape-host": "",
> + "hosted-account-name": "",
> + "hosted-password": "",
> + "local-landscape-host": "",
> + "local-account-name": "",
> + "local-password": ""
> + }
>
> Nitpicking again, but the last "}" should be on the line before.
>

Done.

> [5]
> +from landscape.ui.constants import (
> + CANONICAL_MANAGED, NOT_MANAGED)
>
> This can be on one line.

Done

>
> [6] It'd be nice to have some more docstrings on ClientSettingsDialog.

Done.

> Great work, +1!

Thanks.

I also added some minor UI tweaks from a review by mpt.