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

Proposed by Benji York on 2013-01-17
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 2013-01-17 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.
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...

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 on 2013-01-17
320. By Benji York on 2013-01-17

merge from trunk

321. By Benji York on 2013-01-17

checkpoint

322. By Benji York on 2013-01-17

more review changes

323. By Benji York on 2013-01-17

fix lint

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