Merge lp:~blake-rouse/maas/networks-edit-ui into lp:maas/trunk

Proposed by Blake Rouse on 2016-04-29
Status: Merged
Approved by: Blake Rouse on 2016-05-04
Approved revision: 4977
Merged at revision: 4992
Proposed branch: lp:~blake-rouse/maas/networks-edit-ui
Merge into: lp:maas/trunk
Diff against target: 81 lines (+7/-14)
3 files modified
src/maasserver/static/partials/networks-list.html (+3/-10)
src/maasserver/static/partials/nodes-list.html (+3/-3)
src/maasserver/websockets/handlers/tests/test_subnet.py (+1/-1)
To merge this branch: bzr merge lp:~blake-rouse/maas/networks-edit-ui
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2016-04-29 Approve on 2016-05-04
Review via email: mp+293445@code.launchpad.net

Commit message

Add the maasObjForm directive and use it to allow editing fabrics, vlans, spaces, and subnets.

The maasObjForm directive gives the ability to modify any object that is provided over the websocket. Provide the directive with the object you want to edit and the manager for that object. Define the fields in you want to be able to edit and profit! This follows the design that losing focus on the field causes it to save.

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

Want to go ahead and get a review. I need to still add tests for allowing the "update" method on the websocket handlers. I am also waiting on Rich to get back to me with the CSS fix.

Mike Pontillo (mpontillo) wrote :

This looks really good so far. Thanks for making this a directive; I think the code looks really clean.

I branched it and tested it out. Some obvious things need fixing, which I think you already know about for the most part, but I thought it might be helpful to make a list:

General comments:
(1) Labels appear to the right of edit widgets
(2) When pressing <enter> to save a field, it would be nice if it was consistent with the "clicking away" behavior (pressing enter should gray out the field to give the user an indication that saving is in progress, and they should no longer try to edit.)
(3) Pressing <esc> should cancel editing and revert the field to its previous value.
(4) Large text areas such as description fields should start out grayed out to be consistent with the smaller text fields; right now they always appear white.

Subnet page:
(5) We decided with the design team that the VLAN selection widget should not be a group by; rather it should change the acceptable VLANs based on the fabric selection. (I actually like it better as a group by, for the record, but I've got to pick my battles!) ;-)

VLAN page:
(6) Shouldn't the fabric be editable here? (only for the non-default VLAN...)

I think the only thing that blocks landing this branch is (1). (4) would be nice to have. The others could happen in follow-on branches.

review: Needs Fixing
Mike Pontillo (mpontillo) wrote :

Also, I'd be okay with landing this temporarily if it's easier for Rich to fix the CSS once it lands in trunk, but we'd need to time it so that it occurs after the next beta release if you want to do it that way.

Blake Rouse (blake-rouse) wrote :

> This looks really good so far. Thanks for making this a directive; I think the
> code looks really clean.
>
> I branched it and tested it out. Some obvious things need fixing, which I
> think you already know about for the most part, but I thought it might be
> helpful to make a list:
>
> General comments:
> (1) Labels appear to the right of edit widgets

Fixed.

> (2) When pressing <enter> to save a field, it would be nice if it was
> consistent with the "clicking away" behavior (pressing enter should gray out
> the field to give the user an indication that saving is in progress, and they
> should no longer try to edit.)

This will happen unless you are hovering the field. When your hovering it gives the same look except it has no cursor. That would need to be something we would need to raise with design to get changed.

> (3) Pressing <esc> should cancel editing and revert the field to its previous
> value.

Done. Good idea!

> (4) Large text areas such as description fields should start out grayed out to
> be consistent with the smaller text fields; right now they always appear
> white.

Fixed.

>
> Subnet page:
> (5) We decided with the design team that the VLAN selection widget should not
> be a group by; rather it should change the acceptable VLANs based on the
> fabric selection. (I actually like it better as a group by, for the record,
> but I've got to pick my battles!) ;-)
>

Fixed.

> VLAN page:
> (6) Shouldn't the fabric be editable here? (only for the non-default VLAN...)

We actually do not allow even over the API to move a VLAN to a different fabric. I think that might be wierd to allow. Nothing stops them from creating that VLAN and moving the subnet to that VLAN.

>
> I think the only thing that blocks landing this branch is (1). (4) would be
> nice to have. The others could happen in follow-on branches.

I fixed any real blockers in this branch nothing should need to be landed after. I also sat down with Carla and fixed so other issues with the "Add" sections both for machines, chassis, and all the networking add functions.

Mike Pontillo (mpontillo) wrote :

Sounds great. I'll give it another round of testing.

Mike Pontillo (mpontillo) wrote :

I tested this again; it looks great now.

Only found one quirk: when I edit a field and get an error, if I click on the error message (as in, to try to select and copy it) it disappears momentarily and the screen flickers, and my selection doesn't work.

review: Approve
Blake Rouse (blake-rouse) wrote :

Yeah the error requires you to stay focus on the input field until you fix the error.

MAAS Lander (maas-lander) wrote :
Download full text (1.1 MiB)

The attempt to merge lp:~blake-rouse/maas/networks-edit-ui into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [93.3 kB]
Get:3 http://security.ubuntu.com/ubuntu xenial-security InRelease [92.2 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Fetched 185 kB in 0s (402 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
libpq-dev is already the newest version (9.5.2-1).
make is already the newest version (4.1-6).
postgresql is already the newest versio...

4977. By Blake Rouse on 2016-05-04

Fix tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/static/partials/networks-list.html'
2--- src/maasserver/static/partials/networks-list.html 2016-05-04 04:28:52 +0000
3+++ src/maasserver/static/partials/networks-list.html 2016-05-04 23:35:56 +0000
4@@ -24,11 +24,8 @@
5 <!-- Forms -->
6 <div data-ng-repeat="option in actionOptions">
7 <div class="clearfix margin-bottom" data-ng-if="actionOption.name === option.name">
8-
9-
10-
11- <div class="page-header__dropdown no-padding-bottom">
12- <h3>{$ actionOption.selectedTitle $}</h3>
13+ <div class="page-header__dropdown border">
14+ <h3 class="padding-top--ten">{$ actionOption.selectedTitle $}</h3>
15 <form class="twelve-col padding-top--ten margin-bottom">
16 <div class="inline six-col {$ $odd ? 'last-col' : '' $}"
17 data-maas-model-field="item"
18@@ -37,8 +34,7 @@
19 data-maas-input-text-class="three-col {$ $odd ? 'last-col' : '' $}"
20 data-ng-repeat="item in option.form.items"></div>
21 </form>
22-
23- <div class="page-header__feedback twelve-col no-margin-bottom">
24+ <div class="page-header__feedback twelve-col no-margin-bottom no-padding-bottom">
25 <div data-ng-if="!option.error">
26 <div class="right">
27 <button class="cta-ubuntu text-button"
28@@ -65,9 +61,6 @@
29 </div>
30 </div>
31 </div>
32-
33-
34-
35 </div>
36 </div>
37 </div>
38
39=== modified file 'src/maasserver/static/partials/nodes-list.html'
40--- src/maasserver/static/partials/nodes-list.html 2016-05-04 21:28:50 +0000
41+++ src/maasserver/static/partials/nodes-list.html 2016-05-04 23:35:56 +0000
42@@ -124,7 +124,7 @@
43 </p>
44 </div>
45 </div>
46- <div class="page-header__dropdown ng-hide" data-ng-show="addHardwareScope.viewable" data-ng-controller="AddHardwareController">
47+ <div class="page-header__dropdown border ng-hide" data-ng-show="addHardwareScope.viewable" data-ng-controller="AddHardwareController">
48 <h3 class="padding-top--ten" data-ng-if="showMachine()">Add machine</h3>
49 <h3 class="padding-top--ten" data-ng-if="showChassis()">Add chassis</h3>
50 <div class="page-header__feedback" data-ng-hide="architectures.length">
51@@ -153,7 +153,7 @@
52 <select name="architecture" id="architecture" class="three-col"
53 data-ng-model="machine.architecture"
54 data-ng-options="arch for arch in architectures">
55- <option value="" disabled>Choose a architecture</option>
56+ <option value="" disabled>Choose an architecture</option>
57 </select>
58 </div>
59 <div class="inline">
60@@ -304,7 +304,7 @@
61 </p>
62 </div>
63 </div>
64- <div class="page-header__dropdown ng-hide" data-ng-show="addDeviceScope.viewable" data-ng-controller="AddDeviceController">
65+ <div class="page-header__dropdown border ng-hide" data-ng-show="addDeviceScope.viewable" data-ng-controller="AddDeviceController">
66 <h3>Add device</h3>
67 <form class="twelve-col">
68 <fieldset class="twelve-col last-col no-padding">
69
70=== modified file 'src/maasserver/websockets/handlers/tests/test_subnet.py'
71--- src/maasserver/websockets/handlers/tests/test_subnet.py 2016-05-04 04:28:52 +0000
72+++ src/maasserver/websockets/handlers/tests/test_subnet.py 2016-05-04 23:35:56 +0000
73@@ -26,7 +26,7 @@
74 "created": dehydrate_datetime(subnet.created),
75 "name": subnet.name,
76 "description": subnet.description,
77- "dns_servers": " ".join(subnet.dns_servers),
78+ "dns_servers": " ".join(sorted(subnet.dns_servers)),
79 "vlan": subnet.vlan_id,
80 "space": subnet.space_id,
81 "rdns_mode": subnet.rdns_mode,