Merge ~mpontillo/maas:default-domain-ui--bug-1756745 into maas:master

Proposed by Mike Pontillo on 2018-09-28
Status: Merged
Approved by: Mike Pontillo on 2018-10-03
Approved revision: 55574626b4b4c830577f61335ac79e0d16183730
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:default-domain-ui--bug-1756745
Merge into: maas:master
Diff against target: 334 lines (+173/-14)
7 files modified
src/maasserver/static/js/angular/controllers/domains_list.js (+14/-0)
src/maasserver/static/js/angular/controllers/tests/test_domains_list.js (+41/-0)
src/maasserver/static/js/angular/factories/domains.js (+15/-2)
src/maasserver/static/js/angular/factories/tests/test_domains.js (+29/-1)
src/maasserver/static/partials/domains-list.html (+41/-11)
src/maasserver/websockets/handlers/domain.py (+11/-0)
src/maasserver/websockets/handlers/tests/test_domain.py (+22/-0)
Reviewer Review Type Date Requested Status
Lee Trager 2018-09-28 Approve on 2018-10-03
Kit Randel Approve on 2018-10-02
Review via email: mp+355810@code.launchpad.net

Commit message

LP: #1756745 - Allow setting the default domain from the UI.

To post a comment you must log in.
Anthony Dillon (ya-bo-ng) wrote :

For medium and small screens simply replace `p-table--mobile-card` class on the table with `p-table`. The mobile card is not required for this table.

Also, seems like you should not be able to set the current default domain as default. Could you conditionally display the action menu?

Kit Randel (blr) wrote :

lgtm. As discussed we can add an item to our backlog to modernise the table css at some point.

review: Approve
Mike Pontillo (mpontillo) wrote :

Thanks for the help and feedback.

I've moved forward using the col-N classes, since I see regressions with regard to column alignment (and sometimes placement of the confirm message) in this table every time I try to use the new CSS-based method.

I've also hard-coded a min-height style for now, to make all the rows the same height (regardless of whether or not they have an actions menu)

5557462... by Mike Pontillo on 2018-10-03

Hide domain actions menu for non-admins.

Lee Trager (ltrager) wrote :

I focused on the websocket side of things but this looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/static/js/angular/controllers/domains_list.js b/src/maasserver/static/js/angular/controllers/domains_list.js
2index ee71c21..da2b3ee 100644
3--- a/src/maasserver/static/js/angular/controllers/domains_list.js
4+++ b/src/maasserver/static/js/angular/controllers/domains_list.js
5@@ -22,6 +22,7 @@ angular.module('MAAS').controller('DomainsListController', [
6 $scope.predicate = "name";
7 $scope.reverse = false;
8 $scope.loading = true;
9+ $scope.confirmSetDefaultRow = null;
10
11 // This will hold the AddDomainController once it's initialized. The
12 // controller will set this variable as it's always a child of this
13@@ -43,6 +44,19 @@ angular.module('MAAS').controller('DomainsListController', [
14 return UsersManager.isSuperUser();
15 };
16
17+ $scope.confirmSetDefault = function(domain) {
18+ $scope.confirmSetDefaultRow = domain;
19+ };
20+
21+ $scope.cancelSetDefault = function() {
22+ $scope.confirmSetDefaultRow = null;
23+ };
24+
25+ $scope.setDefault = function(domain) {
26+ DomainsManager.setDefault(domain);
27+ $scope.confirmSetDefaultRow = null;
28+ };
29+
30 ManagerHelperService.loadManagers(
31 $scope, [DomainsManager, UsersManager]).then(
32 function() {
33diff --git a/src/maasserver/static/js/angular/controllers/tests/test_domains_list.js b/src/maasserver/static/js/angular/controllers/tests/test_domains_list.js
34index 02576bd..86eb06f 100644
35--- a/src/maasserver/static/js/angular/controllers/tests/test_domains_list.js
36+++ b/src/maasserver/static/js/angular/controllers/tests/test_domains_list.js
37@@ -101,6 +101,47 @@ describe("DomainsListController", function() {
38 });
39 });
40
41+ describe("confirmSetDefault", function() {
42+
43+ it("sets confirmSetDefaultRow to the specified row", function() {
44+ var controller = makeController();
45+ var obj = {
46+ id: makeInteger(0, 100)
47+ };
48+ $scope.confirmSetDefault(obj);
49+ expect($scope.confirmSetDefaultRow).toBe(obj);
50+ });
51+ });
52+
53+ describe("cancelSetDefault", function() {
54+
55+ it("sets confirmSetDefaultRow to the specified row", function() {
56+ var controller = makeController();
57+ var obj = {
58+ id: makeInteger(0, 100)
59+ };
60+ $scope.confirmSetDefaultRow = obj;
61+ $scope.cancelSetDefault();
62+ expect($scope.confirmSetDefaultRow).toBe(null);
63+ });
64+ });
65+
66+ describe("setDefault", function() {
67+
68+ it("calls DomainsManager.setDefault and clears selection", function() {
69+ var controller = makeController();
70+ spyOn(DomainsManager, "setDefault");
71+ var obj = {
72+ id: makeInteger(0, 100)
73+ };
74+ $scope.confirmSetDefaultRow = obj;
75+ $scope.setDefault(obj);
76+ expect(DomainsManager.setDefault).toHaveBeenCalledWith(obj);
77+ expect($scope.confirmSetDefaultRow).toBe(null);
78+
79+ });
80+ });
81+
82 setupController = function(domains) {
83 var defer = $q.defer();
84 var controller = makeController(defer);
85diff --git a/src/maasserver/static/js/angular/factories/domains.js b/src/maasserver/static/js/angular/factories/domains.js
86index ebc1561..5a9537f 100644
87--- a/src/maasserver/static/js/angular/factories/domains.js
88+++ b/src/maasserver/static/js/angular/factories/domains.js
89@@ -78,13 +78,25 @@ angular.module('MAAS').factory(
90 }
91 };
92
93+ // Set the specified domain as the default.
94+ DomainsManager.prototype.setDefault = function(domain) {
95+ var promise = RegionConnection.callMethod(
96+ "domain.set_default", {'domain': domain.id});
97+ promise.then(function() {
98+ // Reload the domains, so that the new default will be
99+ // reflected everywhere, and record counts will be properly
100+ // updated.
101+ self.reloadItems();
102+ });
103+ };
104+
105 DomainsManager.prototype.getDefaultDomain = function() {
106 if(this._items.length === 0) {
107 return null;
108 } else {
109 var i;
110 for(i=0;i<this._items.length;i++) {
111- if(this._items[i].id === 0) {
112+ if(this._items[i].is_default === true) {
113 return this._items[i];
114 }
115 }
116@@ -104,5 +116,6 @@ angular.module('MAAS').factory(
117 return null;
118 };
119
120- return new DomainsManager();
121+ var self = new DomainsManager();
122+ return self;
123 }]);
124diff --git a/src/maasserver/static/js/angular/factories/tests/test_domains.js b/src/maasserver/static/js/angular/factories/tests/test_domains.js
125index 0beb002..920c12c 100644
126--- a/src/maasserver/static/js/angular/factories/tests/test_domains.js
127+++ b/src/maasserver/static/js/angular/factories/tests/test_domains.js
128@@ -12,9 +12,12 @@ describe("DomainsManager", function() {
129
130 // Load the DomainsManager.
131 var DomainsManager;
132+ var $q, $rootScope;
133 beforeEach(inject(function($injector) {
134 DomainsManager = $injector.get("DomainsManager");
135 RegionConnection = $injector.get("RegionConnection");
136+ $q = $injector.get("$q");
137+ $rootScope = $injector.get("$rootScope");
138 }));
139
140 // Make a random domain.
141@@ -31,6 +34,11 @@ describe("DomainsManager", function() {
142 if(angular.isDefined(selected)) {
143 domain.$selected = selected;
144 }
145+ if(domain.id === 0) {
146+ domain.is_default = true;
147+ } else {
148+ domain.is_default = false;
149+ }
150 return domain;
151 }
152
153@@ -44,7 +52,7 @@ describe("DomainsManager", function() {
154 expect(DomainsManager.getDefaultDomain()).toBe(null);
155 });
156
157- it("getDefaultDomain returns domain with id = 0", function() {
158+ it("getDefaultDomain returns domain with is_default", function() {
159 var zero = makeDomain(0);
160 DomainsManager._items.push(makeDomain());
161 DomainsManager._items.push(zero);
162@@ -61,6 +69,26 @@ describe("DomainsManager", function() {
163 });
164 });
165
166+ describe("setDefault", function() {
167+ it("calls set_default for domain", function () {
168+ var scope = $rootScope.$new();
169+ var defer = $q.defer();
170+ var promise = defer.promise;
171+ spyOn(RegionConnection, "callMethod").and.returnValue(promise);
172+ spyOn(DomainsManager, "reloadItems");
173+ var domain_id = makeInteger(0, 100);
174+ var record = {
175+ id: domain_id
176+ };
177+ DomainsManager.setDefault(record);
178+ expect(RegionConnection.callMethod).toHaveBeenCalledWith(
179+ "domain.set_default", {'domain': domain_id});
180+ defer.resolve(record);
181+ scope.$digest();
182+ expect(DomainsManager.reloadItems).toHaveBeenCalled();
183+ });
184+ });
185+
186 describe("createDNSRecord", function() {
187 it("calls create_address_record for A record", function() {
188 spyOn(RegionConnection, "callMethod");
189diff --git a/src/maasserver/static/partials/domains-list.html b/src/maasserver/static/partials/domains-list.html
190index db6fa03..db7cced 100644
191--- a/src/maasserver/static/partials/domains-list.html
192+++ b/src/maasserver/static/partials/domains-list.html
193@@ -63,21 +63,51 @@
194 </div>
195 <div class="p-strip" data-ng-show="!loading">
196 <section class="row">
197- <table class="p-table--mobile-card">
198+ <table class="p-table-expanding" role="grid">
199 <thead>
200- <tr>
201- <th class="table-col--25" data-ng-click="predicate='name'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'name', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Domain">Domain</th>
202- <th class="table-col--25" data-ng-click="predicate='authoritative'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'authoritative', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Authoritative">Authoritative</th>
203- <th class="table-col--25 u-align--right" data-ng-click="predicate='hosts'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'hosts', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Hosts">Hosts</th>
204- <th class="table-col--25 u-align--right" data-ng-click="predicate='resource_count'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'resource_count', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Total Records">Total Records</th>
205+ <tr role="row">
206+ <th class="col-3" data-ng-click="predicate='name'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'name', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Domain">Domain</th>
207+ <th class="col-2 u-align--right" data-ng-click="predicate='authoritative'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'authoritative', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Authoritative">Authoritative</th>
208+ <th class="col-2 u-align--right" data-ng-click="predicate='hosts'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'hosts', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Hosts">Hosts</th>
209+ <th class="col-2 u-align--right" data-ng-click="predicate='resource_count'; reverse = !reverse" data-ng-class="{'is-sorted': predicate === 'resource_count', 'sort-asc': reverse === false, 'sort-desc': reverse === true}" title="Total Records">Total Records</th>
210+ <th class="col-3 u-align--right" title="Actions" data-ng-if="isSuperUser()"><span class="u-float--right">Actions</span></th>
211 </tr>
212 </thead>
213 <tbody>
214- <tr data-ng-repeat="row in domains | orderBy:predicate:reverse track by row.id">
215- <td class="table-col--25" aria-label="Domain"><a href="#/domain/{$ row.id $}" title="{$ row.displayname $}">{$ row.displayname $}</a></td>
216- <td class="table-col--25" aria-label="Authorative" title="{$ row.authoritative ? 'Yes' : 'No' $}">{$ row.authoritative ? "Yes" : "No" $}</td>
217- <td class="table-col--25 u-align--right" aria-label="Hosts" title="{$ row.hosts $}">{$ row.hosts $}</td>
218- <td class="table-col--25 u-align--right" aria-label="Total records" title="{$ row.resource_count $}">{$ row.resource_count $}</td>
219+ <tr role="row" data-ng-repeat="row in domains | orderBy:predicate:reverse track by row.id"
220+ data-ng-class="{ 'is-active': confirmSetDefaultRow === row }">
221+ <td class="col-3" aria-label="Domain"><a href="#/domain/{$ row.id $}" title="{$ row.displayname $}">{$ row.displayname $}</a></td>
222+ <td class="col-2 u-align--right" aria-label="Authoritative" title="{$ row.authoritative ? 'Yes' : 'No' $}">{$ row.authoritative ? "Yes" : "No" $}</td>
223+ <td class="col-2 u-align--right" aria-label="Hosts" title="{$ row.hosts $}">{$ row.hosts $}</td>
224+ <td class="col-2 u-align--right" aria-label="Total records" title="{$ row.resource_count $}">{$ row.resource_count $}</td>
225+ <td class="col-3 u-align--right p-table--action-cell" style="min-height: 43px;" aria-label="Actions" data-ng-if="isSuperUser()">
226+ <div class="p-contextual-menu" data-ng-class="{ 'u-hide': row.is_default === true || confirmSetDefaultRow === row }" toggle-ctrl>
227+ <button class="p-button--base is-small p-contextual-menu__toggle" data-ng-click="toggleMenu()">
228+ <i class="p-icon--contextual-menu u-no-margin--right">Actions</i></button>
229+ <div class="p-contextual-menu__dropdown" role="menu" data-ng-show="isToggled">
230+ <button class="p-contextual-menu__link"
231+ aria-label="Set default"
232+ data-ng-click="toggleMenu(); confirmSetDefault(row)">Set default...</button>
233+ </div>
234+ </div>
235+ </td>
236+ <td class="is-active p-table-expanding__panel p-table--action-cell col-12 u-align--right" data-ng-if="confirmSetDefaultRow === row">
237+ <div class="row u-no-margin--top">
238+ <hr />
239+ </div>
240+ <div class="row u-no-margin--top">
241+ <div class="col-6">
242+ Setting this domain as the default will update all existing machines
243+ (in Ready state) with the new default domain. Are you sure?
244+ </div>
245+ <div class="col-6">
246+ <span class="u-float--right">
247+ <button class="p-button--base" type="button" data-ng-click="cancelSetDefault()">Cancel</button>
248+ <button class="p-button--positive u-no-margin--top" data-ng-click="setDefault(row)">Save</button>
249+ </span>
250+ </div>
251+ </div>
252+ </td>
253 </tr>
254 </tbody>
255 </div>
256diff --git a/src/maasserver/websockets/handlers/domain.py b/src/maasserver/websockets/handlers/domain.py
257index a9a7c30..6a06ba6 100644
258--- a/src/maasserver/websockets/handlers/domain.py
259+++ b/src/maasserver/websockets/handlers/domain.py
260@@ -15,6 +15,7 @@ from maasserver.forms.domain import DomainForm
261 from maasserver.models import (
262 DNSData,
263 DNSResource,
264+ GlobalDefault,
265 )
266 from maasserver.models.domain import Domain
267 from maasserver.websockets.base import (
268@@ -41,6 +42,7 @@ class DomainHandler(TimestampedModelHandler, AdminOnlyMixin):
269 'update',
270 'delete',
271 'set_active',
272+ 'set_default',
273 'create_dnsresource',
274 'update_dnsresource',
275 'delete_dnsresource',
276@@ -65,8 +67,10 @@ class DomainHandler(TimestampedModelHandler, AdminOnlyMixin):
277 data["resource_count"] = len(rrsets)
278 if domain.is_default():
279 data["displayname"] = "%s (default)" % data["name"]
280+ data["is_default"] = True
281 else:
282 data["displayname"] = data["name"]
283+ data["is_default"] = False
284 return data
285
286 def _get_domain_or_permission_error(self, params):
287@@ -199,3 +203,10 @@ class DomainHandler(TimestampedModelHandler, AdminOnlyMixin):
288 self._get_domain_or_permission_error(params)
289 dnsdata = DNSData.objects.get(id=params['dnsdata_id'])
290 dnsdata.delete()
291+
292+ def set_default(self, params):
293+ domain = self._get_domain_or_permission_error(params)
294+ global_defaults = GlobalDefault.objects.instance()
295+ global_defaults.domain = domain
296+ global_defaults.save()
297+ return self.full_dehydrate(domain)
298diff --git a/src/maasserver/websockets/handlers/tests/test_domain.py b/src/maasserver/websockets/handlers/tests/test_domain.py
299index b8d11b6..e917ecb 100644
300--- a/src/maasserver/websockets/handlers/tests/test_domain.py
301+++ b/src/maasserver/websockets/handlers/tests/test_domain.py
302@@ -51,6 +51,7 @@ class TestDomainHandler(MAASServerTestCase):
303 "ttl": domain.ttl,
304 "updated": dehydrate_datetime(domain.updated),
305 "created": dehydrate_datetime(domain.created),
306+ "is_default": domain.is_default(),
307 }
308 ip_map = StaticIPAddress.objects.get_hostname_ip_mapping(
309 domain, raw_ttl=True)
310@@ -617,3 +618,24 @@ class TestDomainHandlerAddressRecords(MAASServerTestCase):
311 'domain': domain.id,
312 'dnsresource': resource.id,
313 })
314+
315+ def test__set_default_sets_default(self):
316+ user = factory.make_admin()
317+ handler = DomainHandler(user, {}, None)
318+ factory.make_Domain()
319+ domain2 = factory.make_Domain()
320+ self.assertThat(domain2.is_default(), Equals(False))
321+ handler.set_default({
322+ 'domain': domain2.id,
323+ })
324+ domain2 = reload_object(domain2)
325+ self.assertThat(domain2.is_default(), Equals(True))
326+
327+ def test__set_default_as_non_admin_fails(self):
328+ user = factory.make_User()
329+ handler = DomainHandler(user, {}, None)
330+ domain = factory.make_Domain()
331+ with ExpectedException(HandlerPermissionError):
332+ handler.set_default({
333+ 'domain': domain.id,
334+ })

Subscribers

People subscribed via source and target branches

to all changes: