juju-gui

Merge lp:~benji/juju-gui/no-foot-shooting into lp:juju-gui/experimental

  1. no-foot-shooting
  2. Merge into trunk
Proposed by Benji York
Status: Merged
Merged at revision: 327
Proposed branch: lp:~benji/juju-gui/no-foot-shooting
Merge into: lp:juju-gui/experimental
Diff against target: 1140 lines (+684/-172)
15 files modified
app/templates/overview.handlebars (+1/-1)
app/templates/service-config.handlebars (+12/-6)
app/templates/service-footer-common-controls.partial (+30/-27)
app/templates/service-footer-destroy-service.partial (+7/-8)
app/templates/service.handlebars (+7/-8)
app/views/service.js (+142/-100)
app/views/topology/service.js (+24/-9)
app/views/utils.js (+62/-0)
lib/views/stylesheet.less (+16/-7)
test/index.html (+1/-0)
test/test_application_notifications.js (+2/-0)
test/test_service_view.js (+113/-2)
test/test_templates.js (+128/-0)
test/test_topology.js (+72/-4)
test/test_utils.js (+67/-0)
To merge this branch: bzr merge lp:~benji/juju-gui/no-foot-shooting
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+143715@code.launchpad.net

Description of the change

Prevent destruction of the Juju GUI service

https://codereview.appspot.com/7129049/

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :
Download full text (9.1 KiB)

Good stuff, Benji, thank you.

Land with changes. What I'm bringing is usually trivial, and should be
fairly easy to address, I hope, even when I think it is important.

Gary

https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-common-controls.partial
File app/templates/service-footer-common-controls.partial (right):

https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-common-controls.partial#newcode1
app/templates/service-footer-common-controls.partial:1: {{#unless
serviceIsJujuGUI}}
My inclination is to let the GUI increase the unit count, but not be
exposed--IOW, move serviceIsJujuGUI into only be around
"control-expose," below. Given the basic approach that I think we have
now of "innocent until proven guilty" for disabling GUI service
functionality, do you have a concrete reason to exclude unit count--a
reason to say that this is a problem? Note that I am pretty sure that
unit count may not be reduced beneath one, thanks to a pre-existing
constraint in the GUI.

https://codereview.appspot.com/7129049/diff/1/app/templates/service.handlebars
File app/templates/service.handlebars (right):

https://codereview.appspot.com/7129049/diff/1/app/templates/service.handlebars#newcode25
app/templates/service.handlebars:25: {{#unless serviceIsJujuGUI}}
If you agree that we should reinstate unit count, then IIUC you would
also remove this conditional.

https://codereview.appspot.com/7129049/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode170
app/views/service.js:170: // it would break the application they are
currently using.
Suggest you add another comment similar to the following:

// Note that the destroy button is not shown for the Juju GUI, so this
is a fail-safe.

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode381
app/views/service.js:381: * Aside from a nice seperation of concerns,
this method also
typo: separation

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode382
app/views/service.js:382: * facilitates testing.
Interesting approach. I like it so far.

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode79
app/views/topology/service.js:79: // it would break the application they
are currently using.
I'm not sure yet from the code, but I'm hoping that this is again a
fail-safe because the "destroy" option is not given to the user, in
which case a comment to that effect would again be my preference.

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode740
app/views/topology/service.js:740: var menu =
view.get('container').one('#service-menu'),
nicer variable name, thank you :-)

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode755
app/views/topology/service.js:755:
menu.one('.destroy-service').addClass('disabled');
Per my previous comment, yep, I see you disable it visually as well.
Cool.

https://codereview.appspot.com/7129049/diff/1/app/vie...

Read more...

Revision history for this message
Gary Poster (gary) wrote :
Download full text (5.7 KiB)

Sounds good benji. Thanks!

Gary

https://codereview.appspot.com/7129049/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode170
app/views/service.js:170: // it would break the application they are
currently using.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > Suggest you add another comment similar to the following:
> >
> > // Note that the destroy button is not shown for the Juju GUI, so
 this is a
> > fail-safe.

> I would be inclined to remove the code altogether. I accidentally
duplicated
> this between the menu's destroy handler and the destroy button at the
bottom of
> the service view. It is needed for the menu because the menu item is
only
> disabled in as far as its visual styling is different, the click event
is still
> fired.

> I have removed the code but if you like the fail-safe, I will be glad
to put it
> back (with commentary).

Cool, that's fine. I could go either way.

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode79
app/views/topology/service.js:79: // it would break the application they
are currently using.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > I'm not sure yet from the code, but I'm hoping that this is again a
 fail-safe
> > because the "destroy" option is not given to the user, in which case
 a comment
> > to that effect would again be my preference.

> Nope, this is really the code that keeps the destroy from happening.
The menu
> item is only "disabled" visually. The click event is still fired.
This is the
> pre-existing pattern in the code (for the always-dead "Units" menu
item.

Ah, OK, cool.

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js#newcode963
app/views/utils.js:963: var charmUrl = candidate.charm ||
candidate.get('charm');
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > In the previous branch, Nicola and I both found this to be too
 fragile. See
> the
> > version in
 https://codereview.appspot.com/7069068/diff/1/app/views/utils.js .
> > You don't need to use that approach, and *maybe* it is not necessary
 now for
> > some reason, but it was necessary in the previous branch for tests
 to pass.

> All the tests pass with this version, so I am inclined to avoid the
"sloppier"
> version. The things passed in really should have a "charm" attribute
one way or
> the other.

> That being said, I don't feel strongly about it, so one more nudge
will get me
> to use the other branch's version.

Naah, let's stick with what you have, thanks.

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less#newcode1507
lib/views/stylesheet.less:1507: .warning-message {
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:3...

Read more...

lp:~benji/juju-gui/no-foot-shooting updated
320. By Benji York

merge from trunk

321. By Benji York

checkpoint

322. By Benji York

more review changes

323. By Benji York

fix lint

Revision history for this message
Benjamin Saller (bcsaller) wrote :

LGTM, a minor suggestion or two included.

https://codereview.appspot.com/7129049/diff/15001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/7129049/diff/15001/app/views/service.js#newcode566
app/views/service.js:566: *
Agreed, each of these changes wrt gatherRenderData is nice.

https://codereview.appspot.com/7129049/diff/15001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7129049/diff/15001/app/views/topology/service.js#newcode769
app/views/topology/service.js:769:
menu.one('.destroy-service').addClass('disabled');
Does this actually work? .destroy-service still has the click handler
bound even if the class greys out the link or something, no?

Might we remove the link from the menu all together?

https://codereview.appspot.com/7129049/diff/15001/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/7129049/diff/15001/test/test_topology.js#newcode118
test/test_topology.js:118:
topo.events.ServiceModule.scene['.destroy-service'].click.callback(
a simulate('click') test might be better here if you leave this in the
menu.

https://codereview.appspot.com/7129049/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/templates/overview.handlebars'
2--- app/templates/overview.handlebars 2012-12-24 03:17:11 +0000
3+++ app/templates/overview.handlebars 2013-01-17 20:33:22 +0000
4@@ -5,7 +5,7 @@
5 <ul>
6 <li class="view-service">View</li>
7 <li class="add-relation">Build Relation</li>
8- <li class="add-remove-units">Units</li>
9+ <li class="add-remove-units disabled">Units</li>
10 <li class="destroy-service">Destroy</li>
11 </ul>
12 </div>
13
14=== modified file 'app/templates/service-config.handlebars'
15--- app/templates/service-config.handlebars 2012-10-12 18:27:36 +0000
16+++ app/templates/service-config.handlebars 2013-01-17 20:33:22 +0000
17@@ -2,12 +2,18 @@
18 <div class="view-container">
19 <form id="service-config" class="form-horizontal">
20 <legend>{{service.id}} settings</legend>
21- {{> service-config}}
22- <div class="control-group">
23- <div class="controls">
24- <button type="button" id="save-service-config">Update</button>
25- </div>
26+ {{> service-config}}
27+ <div class="control-group">
28+ <div class="controls">
29+ <button type="button" id="save-service-config">Update</button>
30+ {{#if serviceIsJujuGUI}}
31+ <span class="warning-message">
32+ Warning: Changing the settings on this page may adversely affect
33+ your ability to use the Juju GUI.
34+ </span>
35+ {{/if}}
36 </div>
37+ </div>
38 </form>
39 </div>
40-{{> service-footer}}
41+{{> service-footer}}
42
43=== modified file 'app/templates/service-footer-common-controls.partial'
44--- app/templates/service-footer-common-controls.partial 2012-10-25 07:57:11 +0000
45+++ app/templates/service-footer-common-controls.partial 2013-01-17 20:33:22 +0000
46@@ -1,28 +1,31 @@
47 <div class="inline service-common-controls">
48- {{#unless charm.is_subordinate}}
49- <div class="control-unit-count">
50- <div class="inline"><span>Unit count</span></div>
51- <div class="inline">
52- <input type="text" id="num-service-units" value="{{service.unit_count}}">
53- <img class="divider"
54- src="/juju-ui/assets/images/bottom_bar_small_div.png" />
55- </div>
56- </div>
57- {{/unless}}
58- <div class="control-expose">
59- <div class="inline"><span>Expose</span></div>
60- <div class="inline">
61- {{#if service.exposed}}
62- <img class="unexposeService"
63- alt="Exposed"
64- src="/juju-ui/assets/images/slider_on.png" />
65- <span class="on">On</span>
66- {{else}}
67- <img class="exposeService"
68- alt="Not exposed"
69- src="/juju-ui/assets/images/slider_off.png" />
70- <span class="off">Off</span>
71- {{/if}}
72- </div>
73- </div>
74-</div>
75+{{#unless charm.is_subordinate}}
76+ <div class="control-unit-count">
77+ <div class="inline"><span>Unit count</span></div>
78+ <div class="inline">
79+ <input type="text" id="num-service-units"
80+ value="{{service.unit_count}}">
81+ <img class="divider"
82+ src="/juju-ui/assets/images/bottom_bar_small_div.png" />
83+ </div>
84+ </div>
85+{{/unless}}
86+{{#unless serviceIsJujuGUI}}
87+ <div class="control-expose">
88+ <div class="inline"><span>Expose</span></div>
89+ <div class="inline">
90+ {{#if service.exposed}}
91+ <img class="unexposeService"
92+ alt="Exposed"
93+ src="/juju-ui/assets/images/slider_on.png" />
94+ <span class="on">On</span>
95+ {{else}}
96+ <img class="exposeService"
97+ alt="Not exposed"
98+ src="/juju-ui/assets/images/slider_off.png" />
99+ <span class="off">Off</span>
100+ {{/if}}
101+ </div>
102+ </div>
103+ </div>
104+{{/unless}}
105
106=== modified file 'app/templates/service-footer-destroy-service.partial'
107--- app/templates/service-footer-destroy-service.partial 2012-10-22 11:57:15 +0000
108+++ app/templates/service-footer-destroy-service.partial 2013-01-17 20:33:22 +0000
109@@ -1,8 +1,7 @@
110-<div class="destroy-control">
111- <a id="destroy-service" data-toggle="modal"
112- href="#destroy-service-modal">
113- <img src="/juju-ui/assets/images/destroy_icon.png" />Destroy
114- </a>
115- <img class="divider"
116- src="/juju-ui/assets/images/bottom_bar_big_div.png" />
117-</div>
118\ No newline at end of file
119+{{#unless serviceIsJujuGUI}}
120+ <div class="destroy-control">
121+ <a id="destroy-service" data-toggle="modal" href="#destroy-service-modal"
122+ ><img src="/juju-ui/assets/images/destroy_icon.png" />Destroy</a>
123+ <img class="divider" src="/juju-ui/assets/images/bottom_bar_big_div.png" />
124+ </div>
125+{{/unless}}
126
127=== modified file 'app/templates/service.handlebars'
128--- app/templates/service.handlebars 2012-11-05 23:10:01 +0000
129+++ app/templates/service.handlebars 2013-01-17 20:33:22 +0000
130@@ -14,17 +14,16 @@
131 <div class="inline filter-label"><span>Filter</span></div>
132 <div class="state-buttons">
133 {{#states}}
134- <div
135- class="inline {{#if active}}active{{/if}}">
136- <a href="{{link}}">
137- <span class="state-title {{#if active}}active{{/if}}">{{title}}
138- </span>
139- </a>
140- </div>
141+ <div class="inline {{#if active}}active{{/if}}">
142+ <a href="{{link}}">
143+ <span class="state-title {{#if active}}active{{/if}}">{{title}}
144+ </span>
145+ </a>
146+ </div>
147 {{/states}}
148 </div>
149 <div class="inline control-separator">
150- <img class="divider"
151+ <img class="divider"
152 src="/juju-ui/assets/images/bottom_bar_big_div.png" />
153 </div>
154 </div>
155
156=== modified file 'app/views/service.js'
157--- app/views/service.js 2012-11-08 16:23:16 +0000
158+++ app/views/service.js 2013-01-17 20:33:22 +0000
159@@ -164,9 +164,9 @@
160 },
161
162 confirmDestroy: function(ev) {
163+ ev.preventDefault();
164 // We wait to make the panel until now, because in the render method
165 // the container is not yet part of the document.
166- ev.preventDefault();
167 if (Y.Lang.isUndefined(this.panel)) {
168 this.panel = views.createModalPanel(
169 'Are you sure you want to destroy the service? ' +
170@@ -360,7 +360,7 @@
171 });
172 views.serviceBase = ServiceViewBase;
173
174- var ServiceRelations = Y.Base.create(
175+ views.service_relations = Y.Base.create(
176 'ServiceRelationsView', ServiceViewBase, [views.JujuBaseView], {
177
178 template: Templates['service-relations'],
179@@ -369,17 +369,20 @@
180 '#service-relations .btn': {click: 'confirmRemoved'}
181 },
182
183- render: function() {
184+ /**
185+ * Gather up all of the data required for the template.
186+ *
187+ * Aside from a nice separation of concerns, this method also
188+ * facilitates testing.
189+ *
190+ * @method gatherRenderData
191+ * @return {Object} The data the template will render.
192+ */
193+ gatherRenderData: function() {
194 var container = this.get('container'),
195- getModelURL = this.get('getModelURL'),
196 service = this.get('model'),
197 db = this.get('db'),
198 querystring = this.get('querystring');
199- if (!service || !service.get('loaded')) {
200- container.setHTML('<div class="alert">Loading...</div>');
201- console.log('waiting on service data');
202- return this;
203- }
204 var relation_data = utils.getRelationDataForService(db, service);
205 Y.each(relation_data, function(rel) {
206 if (rel.relation_id === querystring.rel_id) {
207@@ -389,14 +392,27 @@
208 var charm_id = service.get('charm'),
209 charm = db.charms.getById(charm_id),
210 charm_attrs = charm ? charm.getAttrs() : undefined;
211- container.setHTML(this.template(
212- { viewName: 'relations',
213- tabs: this.getServiceTabs('relations'),
214- service: service.getAttrs(),
215- relations: relation_data,
216- charm: charm_attrs,
217- charm_id: charm_id}));
218+ return {
219+ viewName: 'relations',
220+ tabs: this.getServiceTabs('relations'),
221+ service: service.getAttrs(),
222+ relations: relation_data,
223+ charm: charm_attrs,
224+ charm_id: charm_id,
225+ serviceIsJujuGUI: utils.isGuiCharmUrl(charm_id)
226+ };
227+ },
228
229+ render: function() {
230+ var container = this.get('container');
231+ var service = this.get('model');
232+ if (!service || !service.get('loaded')) {
233+ container.setHTML('<div class="alert">Loading...</div>');
234+ console.log('waiting on service data');
235+ } else {
236+ container.setHTML(this.template(this.gatherRenderData()));
237+ }
238+ return this;
239 },
240
241 confirmRemoved: function(ev) {
242@@ -474,9 +490,7 @@
243 }
244 });
245
246- views.service_relations = ServiceRelations;
247-
248- var ServiceConstraints = Y.Base.create(
249+ views.service_constraints = Y.Base.create(
250 'ServiceConstraintsView', ServiceViewBase, [views.JujuBaseView], {
251
252 template: Templates['service-constraints'],
253@@ -544,11 +558,17 @@
254 }
255 },
256
257- render: function() {
258- var container = this.get('container'),
259- service = this.get('model'),
260- getModelURL = this.get('getModelURL'),
261- db = this.get('db'),
262+ /**
263+ * Gather up all of the data required for the template.
264+ *
265+ * Aside from a nice separation of concerns, this method also
266+ * facilitates testing.
267+ *
268+ * @method gatherRenderData
269+ * @return {Object} The data the template will render.
270+ */
271+ gatherRenderData: function() {
272+ var service = this.get('model'),
273 constraints = service.get('constraints'),
274 display_constraints = [];
275
276@@ -574,7 +594,8 @@
277 });
278
279 console.log('service constraints', display_constraints);
280- container.setHTML(this.template({
281+ var charm_id = service.get('charm');
282+ return {
283 viewName: 'constraints',
284 tabs: this.getServiceTabs('constraints'),
285 service: service.getAttrs(),
286@@ -586,17 +607,20 @@
287 });
288 return arr;
289 })(),
290- charm_id: service.get('charm')}
291- ));
292+ charm_id: charm_id,
293+ serviceIsJujuGUI: utils.isGuiCharmUrl(charm_id)
294+ };
295+ },
296
297+ render: function() {
298+ var container = this.get('container');
299+ container.setHTML(this.template(this.gatherRenderData()));
300 return this;
301 }
302
303 });
304
305- views.service_constraints = ServiceConstraints;
306-
307- var ServiceConfigView = Y.Base.create(
308+ views.service_config = Y.Base.create(
309 'ServiceConfigView', ServiceViewBase, [views.JujuBaseView], {
310
311 template: Templates['service-config'],
312@@ -605,27 +629,27 @@
313 '#save-service-config': {click: 'saveConfig'}
314 },
315
316- render: function() {
317- var container = this.get('container'),
318- db = this.get('db'),
319- service = this.get('model');
320-
321- if (!service || !service.get('loaded')) {
322- container.setHTML('<div class="alert">Loading...</div>');
323- console.log('waiting on service data');
324- return this;
325- }
326-
327- console.log('config', service.get('config'));
328-
329- // combine the charm schema and the service values for display.
330- var charm = db.charms.getById(service.get('charm')),
331- config = service.get('config'),
332- getModelURL = this.get('getModelURL'),
333- charm_config = charm.get('config'),
334- schema = charm_config && charm_config.options,
335- settings = [],
336- field_def;
337+ /**
338+ * Gather up all of the data required for the template.
339+ *
340+ * Aside from a nice separation of concerns, this method also
341+ * facilitates testing.
342+ *
343+ * @method gatherRenderData
344+ * @return {Object} The data the template will render.
345+ */
346+ gatherRenderData: function() {
347+ var container = this.get('container');
348+ var db = this.get('db');
349+ var service = this.get('model');
350+ var charm = db.charms.getById(service.get('charm'));
351+ var config = service.get('config');
352+ var getModelURL = this.get('getModelURL');
353+ var charm_config = charm.get('config');
354+ var schema = charm_config && charm_config.options;
355+ var settings = [];
356+ var charm_id = service.get('charm');
357+ var field_def;
358
359 Y.Object.each(schema, function(field_def, field_name) {
360 var entry = {
361@@ -651,16 +675,25 @@
362 settings.push(Y.mix(entry, field_def));
363 });
364
365- console.log('render view svc config', service.getAttrs(), settings);
366-
367- container.setHTML(this.template(
368- { viewName: 'config',
369- tabs: this.getServiceTabs('config'),
370- service: service.getAttrs(),
371- settings: settings,
372- charm_id: service.get('charm')}
373- ));
374-
375+ return {
376+ viewName: 'config',
377+ tabs: this.getServiceTabs('config'),
378+ service: service.getAttrs(),
379+ settings: settings,
380+ charm_id: charm_id,
381+ serviceIsJujuGUI: utils.isGuiCharmUrl(charm_id)
382+ };
383+ },
384+
385+ render: function() {
386+ var container = this.get('container');
387+ var service = this.get('model');
388+ if (!service || !service.get('loaded')) {
389+ container.setHTML('<div class="alert">Loading...</div>');
390+ console.log('waiting on service data');
391+ } else {
392+ container.setHTML(this.template(this.gatherRenderData()));
393+ }
394 return this;
395 },
396
397@@ -719,8 +752,8 @@
398 container.one('#save-service-config').set('disabled', 'disabled');
399
400 var new_values = utils.getElementsValuesMapping(
401- container, '.config-field'),
402- errors = utils.validate(new_values, schema);
403+ container, '.config-field');
404+ var errors = utils.validate(new_values, schema);
405
406 if (Y.Object.isEmpty(errors)) {
407 env.set_config(
408@@ -767,8 +800,6 @@
409 }
410 });
411
412- views.service_config = ServiceConfigView;
413-
414 // Display a unit grid based on the total number of units.
415 Y.Handlebars.registerHelper('show_units', function(units) {
416 var template;
417@@ -797,29 +828,29 @@
418
419 template: Templates.service,
420
421- render: function() {
422- console.log('service view render');
423-
424- var container = this.get('container'),
425- getModelURL = this.get('getModelURL'),
426- db = this.get('db'),
427- service = this.get('model'),
428- filter_state = this.get('querystring').state;
429-
430- if (!service) {
431- container.setHTML('<div class="alert">Loading...</div>');
432- console.log('waiting on service data');
433- return this;
434- }
435-
436- var units = db.units.get_units_for_service(service),
437- state_data = [{
438- title: 'All',
439- link: '.',
440- active: !filter_state,
441- count: this.filterUnits(null, units).length
442- }];
443-
444+ /**
445+ * Gather up all of the data required for the template.
446+ *
447+ * Aside from a nice separation of concerns, this method also
448+ * facilitates testing.
449+ *
450+ * @method gatherRenderData
451+ * @return {Object} The data the template will render.
452+ */
453+ gatherRenderData: function() {
454+ var db = this.get('db');
455+ var service = this.get('model');
456+ var filter_state = this.get('querystring').state;
457+ var units = db.units.get_units_for_service(service);
458+ var charm_id = service.get('charm');
459+ var charm = db.charms.getById(charm_id);
460+ var charm_attrs = charm ? charm.getAttrs() : undefined;
461+ var state_data = [{
462+ title: 'All',
463+ link: '.',
464+ active: !filter_state,
465+ count: this.filterUnits(null, units).length
466+ }];
467 Y.each(['Running', 'Pending', 'Error'], function(title) {
468 var lower = title.toLowerCase();
469 state_data.push({
470@@ -828,18 +859,29 @@
471 count: this.filterUnits(lower, units).length,
472 link: '?state=' + lower});
473 }, this);
474- var charm_id = service.get('charm'),
475- charm = db.charms.getById(charm_id),
476- charm_attrs = charm ? charm.getAttrs() : undefined;
477- container.setHTML(this.template(
478- { viewName: 'units',
479- tabs: this.getServiceTabs('.'),
480- service: service.getAttrs(),
481- charm_id: charm_id,
482- charm: charm_attrs,
483- state: filter_state,
484- units: this.filterUnits(filter_state, units),
485- states: state_data}));
486+ return {
487+ viewName: 'units',
488+ tabs: this.getServiceTabs('.'),
489+ service: service.getAttrs(),
490+ charm_id: charm_id,
491+ charm: charm_attrs,
492+ serviceIsJujuGUI: utils.isGuiCharmUrl(charm_id),
493+ state: filter_state,
494+ units: this.filterUnits(filter_state, units),
495+ states: state_data
496+ };
497+ },
498+
499+ render: function() {
500+ console.log('service view render');
501+ var container = this.get('container');
502+ var service = this.get('model');
503+ if (!service || !service.get('loaded')) {
504+ container.setHTML('<div class="alert">Loading...</div>');
505+ console.log('waiting on service data');
506+ } else {
507+ container.setHTML(this.template(this.gatherRenderData()));
508+ }
509 return this;
510 },
511
512
513=== modified file 'app/views/topology/service.js'
514--- app/views/topology/service.js 2013-01-15 22:49:05 +0000
515+++ app/views/topology/service.js 2013-01-17 20:33:22 +0000
516@@ -75,6 +75,11 @@
517 var topo = context.get('component');
518 var box = topo.get('active_service');
519 var service = topo.serviceForBox(box);
520+ // The user is not allowed to destroy the Juju GUI service because
521+ // it would break the application they are currently using.
522+ if (utils.isGuiService(service)) {
523+ return;
524+ }
525 context.service_click_actions
526 .toggleControlPanel(box, context);
527 context.service_click_actions
528@@ -738,21 +743,31 @@
529 */
530 service_click_actions: {
531 /*
532- * Default action: show or hide control panel.
533+ * Default action: show or hide control panel (service menu).
534+ *
535+ * @method toggleControlPanel
536+ * @param {Object} service The service in question (if any).
537+ * @param {Object} view The current view.
538+ * @param {object} context The active context.
539 */
540- toggleControlPanel: function(m, view, context) {
541- var container = view.get('container'),
542- topo = view.get('component'),
543- cp = container.one('#service-menu');
544+ toggleControlPanel: function(service, view, context) {
545+ var menu = view.get('container').one('#service-menu'),
546+ topo = view.get('component');
547
548- if (cp.hasClass('active') || !m) {
549- cp.removeClass('active');
550+ if (menu.hasClass('active') || !service) {
551+ menu.removeClass('active');
552 topo.set('active_service', null);
553 topo.set('active_context', null);
554+ // Most services can be destroyed via the GUI.
555+ menu.one('.destroy-service').removeClass('disabled');
556 } else {
557- topo.set('active_service', m);
558+ topo.set('active_service', service);
559 topo.set('active_context', context);
560- cp.addClass('active');
561+ menu.addClass('active');
562+ // We do not want the user destroying the Juju GUI service.
563+ if (utils.isGuiService(service)) {
564+ menu.one('.destroy-service').addClass('disabled');
565+ }
566 view.updateServiceMenuLocation();
567 }
568 },
569
570=== modified file 'app/views/utils.js'
571--- app/views/utils.js 2013-01-15 09:43:29 +0000
572+++ app/views/utils.js 2013-01-17 20:33:22 +0000
573@@ -906,6 +906,68 @@
574 return result;
575 };
576
577+ /**
578+ * Determine if a service is the Juju GUI by inspecting the charm URL.
579+ *
580+ * @method isGuiCharmUrl
581+ * @param {String} charmUrl The service to inspect.
582+ * @return {Boolean} True if the charm URL is that of the Juju GUI.
583+ */
584+ utils.isGuiCharmUrl = function(charmUrl) {
585+ // Regular expression to match charm URLs. Explanation generated by
586+ // http://rick.measham.id.au/paste/explain.pl and then touched up a bit to
587+ // fit in our maximum line width. Note that the bit about an optional
588+ // newline matched by $ is a Perlism that JS does not share.
589+ //
590+ // NODE EXPLANATION
591+ // ------------------------------------------------------------------------
592+ // ^ the beginning of the string
593+ // ------------------------------------------------------------------------
594+ // (?: group, but do not capture:
595+ // ------------------------------------------------------------------------
596+ // [^:]+ any character except: ':' (1 or more
597+ // times (matching the most amount
598+ // possible))
599+ // ------------------------------------------------------------------------
600+ // : ':'
601+ // ------------------------------------------------------------------------
602+ // ) end of grouping
603+ // ------------------------------------------------------------------------
604+ // (?: group, but do not capture (0 or more times
605+ // (matching the most amount possible)):
606+ // ------------------------------------------------------------------------
607+ // [^\/]+ any character except: '\/' (1 or more
608+ // times (matching the most amount
609+ // possible))
610+ // ------------------------------------------------------------------------
611+ // \/ '/'
612+ // ------------------------------------------------------------------------
613+ // )* end of grouping
614+ // ------------------------------------------------------------------------
615+ // juju-gui- 'juju-gui-'
616+ // ------------------------------------------------------------------------
617+ // \d+ digits (0-9) (1 or more times (matching
618+ // the most amount possible))
619+ // ------------------------------------------------------------------------
620+ // $ before an optional \n, and the end of the
621+ // string
622+ return (/^(?:[^:]+:)(?:[^\/]+\/)*juju-gui-\d+$/).test(charmUrl);
623+ };
624+
625+ /**
626+ * Determine if a service is the Juju GUI by inspecting the charm URL.
627+ *
628+ * @method isGuiService
629+ * @param {Object} candidate The service to inspect.
630+ * @return {Boolean} True if the service is the Juju GUI.
631+ */
632+ utils.isGuiService = function(candidate) {
633+ // Some candidates have the charm URL as an attribute, others require a
634+ // "get".
635+ var charmUrl = candidate.charm || candidate.get('charm');
636+ return utils.isGuiCharmUrl(charmUrl);
637+ };
638+
639 Y.Handlebars.registerHelper('unitState', function(relation_errors,
640 agent_state) {
641 if ('started' === agent_state && relation_errors &&
642
643=== modified file 'lib/views/stylesheet.less'
644--- lib/views/stylesheet.less 2013-01-10 10:25:55 +0000
645+++ lib/views/stylesheet.less 2013-01-17 20:33:22 +0000
646@@ -96,8 +96,15 @@
647 }
648
649 .view-container {
650- overflow: auto;
651- padding: 10px;
652+ overflow: auto;
653+ padding: 10px;
654+
655+ .control-group {
656+ .warning-message {
657+ color: red;
658+ padding: 1em;
659+ }
660+ }
661 }
662
663 .navbar {
664@@ -291,11 +298,6 @@
665 }
666 &.add-remove-units {
667 background-image: url(/juju-ui/assets/images/icons/icon_noshadow_relation.png);
668- color: gray;
669- cursor: default;
670- &:hover {
671- background-color: inherit;
672- }
673 }
674 &.destroy-service {
675 background-image: url(/juju-ui/assets/images/icons/icon_noshadow_destroy.png);
676@@ -307,6 +309,13 @@
677 &:hover {
678 background-color: lighten(@background_color, 10%);
679 }
680+ &.disabled {
681+ color: gray;
682+ cursor: default;
683+ &:hover {
684+ background-color: inherit;
685+ }
686+ }
687 }
688
689 }
690
691=== modified file 'test/index.html'
692--- test/index.html 2013-01-16 09:06:43 +0000
693+++ test/index.html 2013-01-17 20:33:22 +0000
694@@ -54,6 +54,7 @@
695 <script src="test_unit_view.js"></script>
696 <script src="test_utils.js"></script>
697 <script src="test_viewport_module.js"></script>
698+ <script src="test_templates.js"></script>
699
700
701 <script>
702
703=== modified file 'test/test_application_notifications.js'
704--- test/test_application_notifications.js 2013-01-15 10:56:19 +0000
705+++ test/test_application_notifications.js 2013-01-17 20:33:22 +0000
706@@ -104,6 +104,8 @@
707 if ('unit_count' === key) {
708 // We simulate a model with 2 units
709 return 2;
710+ } else if ('loaded' === key) {
711+ return true;
712 }
713 return null;
714 }
715
716=== modified file 'test/test_service_view.js'
717--- test/test_service_view.js 2012-12-21 20:28:25 +0000
718+++ test/test_service_view.js 2013-01-17 20:33:22 +0000
719@@ -1,6 +1,10 @@
720 'use strict';
721
722 (function() {
723+ // XXX This "describe" is a lie. There are tests here for both the service
724+ // view (views.service) and the relations view (views.service_relations).
725+ // This test suite needs to be broken out into two that just contain tests
726+ // for one or the other.
727 describe('juju service view', function() {
728 var models, Y, container, service, db, conn, env, charm, ENTER, ESC,
729 makeServiceView, makeServiceRelationsView, views, unit;
730@@ -670,5 +674,112 @@
731 assert.equal('error2', filtered[1].testKey);
732 });
733
734- });
735-}) ();
736+ it('tells the template if the service is the GUI (service)', function() {
737+ var view = makeServiceView();
738+ var renderData = view.gatherRenderData();
739+ assert.equal(renderData.serviceIsJujuGUI, false);
740+ });
741+
742+ it('tells the template if the service is the GUI (relations)', function() {
743+ // This would ideally be in its own suite; see the XXX at top of this
744+ // file.
745+ var view = makeServiceRelationsView();
746+ var renderData = view.gatherRenderData();
747+ assert.equal(renderData.serviceIsJujuGUI, false);
748+ });
749+
750+ it('loading message if the service is not loaded (service)', function() {
751+ var view = makeServiceView();
752+ view.get('model').set('loaded', false);
753+ view.render();
754+ var html = container.getHTML();
755+ assert.match(html, /Loading\.\.\./);
756+ });
757+
758+ it('loading message if the service is not loaded (relations)', function() {
759+ // This would ideally be in its own suite; see the XXX at top of this
760+ // file.
761+ var view = makeServiceRelationsView();
762+ view.get('model').set('loaded', false);
763+ view.render();
764+ var html = container.getHTML();
765+ assert.match(html, /Loading\.\.\./);
766+ });
767+
768+ });
769+})();
770+
771+(function() {
772+ describe('Service config view (views.service_config)', function() {
773+ var models, Y, container, service, db, conn, env, charm, views, view;
774+
775+ before(function(done) {
776+ Y = YUI(GlobalConfig).use(
777+ 'juju-views', 'juju-models', 'base', 'node', 'json-parse',
778+ 'juju-env', 'node-event-simulate', 'juju-tests-utils', 'event-key',
779+ function(Y) {
780+ models = Y.namespace('juju.models');
781+ views = Y.namespace('juju.views');
782+ done();
783+ });
784+ });
785+
786+ beforeEach(function(done) {
787+ conn = new (Y.namespace('juju-tests.utils')).SocketStub(),
788+ env = new (Y.namespace('juju')).Environment({conn: conn});
789+ env.connect();
790+ conn.open();
791+ container = Y.Node.create('<div/>').hide();
792+ Y.one('#main').append(container);
793+ db = new models.Database();
794+ charm = new models.Charm({id: 'cs:precise/mysql-7', description: 'A DB'});
795+ db.charms.add([charm]);
796+ service = new models.Service(
797+ { id: 'mysql',
798+ charm: 'cs:precise/mysql-7',
799+ unit_count: db.units.size(),
800+ loaded: true,
801+ exposed: false});
802+
803+ db.services.add([service]);
804+ view = new views.service_config({
805+ db: db,
806+ env: env,
807+ getModelURL: function(model, intent) {
808+ return model.get('name');
809+ },
810+ model: service,
811+ container: container
812+ });
813+ done();
814+ });
815+
816+ afterEach(function(done) {
817+ container.remove(true);
818+ service.destroy();
819+ db.destroy();
820+ env.destroy();
821+ done();
822+ });
823+
824+ it('displays a loading message if the service is not loaded', function() {
825+ view.get('model').set('loaded', false);
826+ view.render();
827+ var html = container.getHTML();
828+ assert.match(html, /Loading\.\.\./);
829+ });
830+
831+ it('displays no loading message if the service is loaded', function() {
832+ view.get('model').set('loaded', true);
833+ view.render();
834+ var html = container.getHTML();
835+ assert.notMatch(html, /Loading\.\.\./);
836+ });
837+
838+ it('informs the template if the service is the GUI', function() {
839+ var renderData = view.gatherRenderData();
840+ assert.equal(renderData.serviceIsJujuGUI, false);
841+ });
842+
843+ });
844+})();
845
846=== added file 'test/test_templates.js'
847--- test/test_templates.js 1970-01-01 00:00:00 +0000
848+++ test/test_templates.js 2013-01-17 20:33:22 +0000
849@@ -0,0 +1,128 @@
850+'use strict';
851+
852+(function() {
853+
854+ describe('Template: service-footer-destroy-service.partial', function() {
855+ var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
856+ var Y, conn, env, partial;
857+
858+ before(function(done) {
859+ Y = YUI(GlobalConfig).use(requires, function(Y) {
860+ var partials = Y.Handlebars.partials;
861+ partial = partials['service-footer-destroy-service'];
862+ done();
863+ });
864+ });
865+
866+ it('does not display a "Destroy" button for the Juju GUI', function() {
867+ // Disallow foot-shooting.
868+ var html = partial({serviceIsJujuGUI: true});
869+ assert.notMatch(html, /Destroy/);
870+ });
871+
872+ it('does display a "Destroy" button for other services', function() {
873+ var html = partial({serviceIsJujuGUI: false});
874+ assert.match(html, /Destroy/);
875+ });
876+
877+ });
878+})();
879+
880+(function() {
881+
882+ describe('Template: service-footer-common-controls.partial', function() {
883+ var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
884+ var Y, conn, env, partial;
885+
886+ before(function(done) {
887+ Y = YUI(GlobalConfig).use(requires, function(Y) {
888+ var partials = Y.Handlebars.partials;
889+ partial = partials['service-footer-common-controls'];
890+ done();
891+ });
892+ });
893+
894+ it('includes unit count UI for the Juju GUI service', function() {
895+ var html = partial({serviceIsJujuGUI: true});
896+ assert.match(html, /Unit count/);
897+ });
898+
899+ it('does not include (un)expose for the Juju GUI service', function() {
900+ var html = partial({serviceIsJujuGUI: true});
901+ assert.notMatch(html, /Expose/);
902+ });
903+
904+ it('includes unit count UI for non-Juju-GUI services', function() {
905+ var html = partial({serviceIsJujuGUI: false});
906+ assert.match(html, /Unit count/);
907+ });
908+
909+ it('includes (un)expose for non-Juju-GUI services', function() {
910+ var html = partial({serviceIsJujuGUI: false});
911+ assert.match(html, /Expose/);
912+ });
913+
914+
915+ });
916+})();
917+
918+(function() {
919+
920+ describe('Template: service-config.handlebars', function() {
921+ var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
922+ var warningMessage = /Warning: Changing the settings/;
923+ var Y, conn, env, template;
924+
925+ before(function(done) {
926+ Y = YUI(GlobalConfig).use(requires, function(Y) {
927+ var templates = Y.namespace('juju.views').Templates;
928+ template = templates['service-config'];
929+ done();
930+ });
931+ });
932+
933+ it('includes a warning about reconfiguring the Juju GUI', function() {
934+ // Reconfiguring the Juju GUI service could break it, causing the user to
935+ // lose access to the app they are in the process of using.
936+ var html = template({serviceIsJujuGUI: true});
937+ assert.match(html, warningMessage);
938+ });
939+
940+ it('does not warn about about reconfiguring other services', function() {
941+ var html = template({serviceIsJujuGUI: false});
942+ assert.notMatch(html, warningMessage);
943+ });
944+
945+ });
946+})();
947+
948+(function() {
949+
950+ describe('Template: service.handlebars', function() {
951+ var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
952+ var warningMessage = 'Warning: Changing the settings';
953+ var divider = '<img class="divider"';
954+ var Y, conn, env, template;
955+
956+ before(function(done) {
957+ Y = YUI(GlobalConfig).use(requires, function(Y) {
958+ var templates = Y.namespace('juju.views').Templates;
959+ template = templates.service;
960+ done();
961+ });
962+ });
963+
964+ it('does not show the destroy or expose UI for Juju GUI', function() {
965+ var html = template({serviceIsJujuGUI: true, units: []});
966+ assert.notMatch(html, /Destroy/);
967+ assert.notMatch(html, /Expose/);
968+ });
969+
970+ it('shows the destroy or expose UI for non-Juju-GUI services', function() {
971+ var html = template({serviceIsJujuGUI: false, units: []});
972+ assert.match(html, /Destroy/);
973+ assert.match(html, /Expose/);
974+ });
975+
976+ });
977+})();
978
979=== modified file 'test/test_topology.js'
980--- test/test_topology.js 2013-01-09 17:17:46 +0000
981+++ test/test_topology.js 2013-01-17 20:33:22 +0000
982@@ -1,4 +1,3 @@
983-
984 'use strict';
985
986 describe('topology', function() {
987@@ -94,6 +93,75 @@
988 Y.Lang.isValue(topo.vis).should.equal(true);
989 });
990
991-});
992-
993-
994+ it('should prevent the Juju GUI service from being destroyed', function() {
995+ var service = {
996+ charm: 'cs:precise/juju-gui-7'
997+ };
998+ var fauxTopo = {
999+ get: function() {
1000+ return null;
1001+ },
1002+ serviceForBox: function() {
1003+ return service;
1004+ }
1005+ };
1006+ // The context is used to do the destroying, so if it does not have the
1007+ // destroy method, an exception will be raised if the service would be
1008+ // destroyed.
1009+ var context = {
1010+ get: function(name) {
1011+ if (name === 'component') {
1012+ return fauxTopo;
1013+ }
1014+ }
1015+ };
1016+ topo.events.ServiceModule.scene['.destroy-service'].click.callback(
1017+ undefined, context);
1018+ });
1019+});
1020+
1021+describe('service menu', function() {
1022+ var Y, views;
1023+
1024+ before(function(done) {
1025+ Y = YUI(GlobalConfig).use(['juju-topology'], function(Y) {
1026+ views = Y.namespace('juju.views');
1027+ done();
1028+ });
1029+ });
1030+
1031+ it('should disable the "Destroy" menu for the Juju GUI service', function() {
1032+ var service = {
1033+ charm: 'cs:precise/juju-gui-7'
1034+ };
1035+ var addedClassName;
1036+ var menu = {
1037+ hasClass: function() {
1038+ return false;
1039+ },
1040+ addClass: function() {},
1041+ one: function() {
1042+ return {
1043+ addClass: function(className) {
1044+ addedClassName = className;
1045+ }
1046+ };
1047+ }
1048+ };
1049+ var fauxView = {
1050+ get: function(name) {
1051+ if (name === 'container') {
1052+ return {one: function() { return menu; }};
1053+ } else if (name === 'component') {
1054+ return {set: function() {}};
1055+ }
1056+ },
1057+ updateServiceMenuLocation: function() {}
1058+ };
1059+ var view = new views.ServiceModule();
1060+ view.service_click_actions.toggleControlPanel(
1061+ service, fauxView, undefined);
1062+ assert.equal(addedClassName, 'disabled');
1063+ });
1064+
1065+});
1066
1067=== modified file 'test/test_utils.js'
1068--- test/test_utils.js 2012-10-12 23:52:48 +0000
1069+++ test/test_utils.js 2013-01-17 20:33:22 +0000
1070@@ -339,3 +339,70 @@
1071
1072 });
1073 })();
1074+
1075+(function() {
1076+ describe('utils.isGuiService', function() {
1077+
1078+ var utils, Y;
1079+
1080+ before(function(done) {
1081+ Y = YUI(GlobalConfig).use('juju-views', function(Y) {
1082+ utils = Y.namespace('juju.views.utils');
1083+ done();
1084+ });
1085+ });
1086+
1087+ it('should extract values from "charm" attribute', function() {
1088+ var candidate = {charm: 'cs:precise/juju-gui-7'};
1089+ assert.isTrue(utils.isGuiService(candidate));
1090+ });
1091+
1092+ it('should extract values from .get("charm")', function() {
1093+ var candidate = {
1094+ get: function(name) {
1095+ if (name === 'charm') {
1096+ return 'cs:precise/juju-gui-7';
1097+ }
1098+ }
1099+ };
1100+ assert.isTrue(utils.isGuiService(candidate));
1101+ });
1102+
1103+ });
1104+})();
1105+
1106+(function() {
1107+ describe('utils.isGuiCharmUrl', function() {
1108+
1109+ var utils, Y;
1110+
1111+ before(function(done) {
1112+ Y = YUI(GlobalConfig).use('juju-views', function(Y) {
1113+ utils = Y.namespace('juju.views.utils');
1114+ done();
1115+ });
1116+ });
1117+
1118+ it('should recognize charm store URLs', function() {
1119+ assert.isTrue(utils.isGuiCharmUrl('cs:precise/juju-gui-7'));
1120+ });
1121+
1122+ it('should recognize unofficial charm store URLs', function() {
1123+ assert.isTrue(utils.isGuiCharmUrl('cs:~foobar/precise/juju-gui-7'));
1124+ });
1125+
1126+ it('should ignore owners of unofficial charm store URLs', function() {
1127+ assert.isFalse(utils.isGuiCharmUrl('cs:~juju-gui/precise/foobar-7'));
1128+ });
1129+
1130+ it('should recognize local charm URLs', function() {
1131+ assert.isTrue(utils.isGuiCharmUrl('local:juju-gui-3'));
1132+ });
1133+
1134+ it('should not allow junk on the end of the URL', function() {
1135+ assert.isFalse(utils.isGuiCharmUrl('local:juju-gui-3 ROFLCOPTR!'));
1136+ });
1137+
1138+ });
1139+})();
1140+

Subscribers

People subscribed via source and target branches