Merge lp:~gary/juju-gui/bug1090716 into lp:juju-gui/experimental

Proposed by Gary Poster on 2013-01-10
Status: Rejected
Rejected by: Brad Crittenden on 2013-01-18
Proposed branch: lp:~gary/juju-gui/bug1090716
Merge into: lp:juju-gui/experimental
Diff against target: 394 lines (+286/-15)
5 files modified
app/views/topology/relation.js (+10/-0)
app/views/topology/service.js (+23/-14)
app/views/utils.js (+79/-1)
test/test_environment_view.js (+70/-0)
test/test_utils.js (+104/-0)
To merge this branch: bzr merge lp:~gary/juju-gui/bug1090716
Reviewer Review Type Date Requested Status
Juju GUI Hackers 2013-01-10 Pending
Review via email: mp+142794@code.launchpad.net

Description of the change

Make GUI charm not shown in GUI

This only hides juju-gui charms in the environment. Relationships with the gui will be shown on inner pages, and you can actually click through to see a page for the juju-gui charm. This is regarded as a valuable incremental step--and possibly where we will leave things for now until we discover a need for greater hiding.

Branch by Benji, with tweaks from Nicola and myself.

https://codereview.appspot.com/7069068/

To post a comment you must log in.
Gary Poster (gary) wrote :

Reviewers: mp+142794_code.launchpad.net,

Message:
Please take a look.

Description:
Make GUI charm not shown in GUI

This only hides juju-gui charms in the environment. Relationships with
the gui will be shown on inner pages, and you can actually click through
to see a page for the juju-gui charm. This is regarded as a valuable
incremental step--and possibly where we will leave things for now until
we discover a need for greater hiding.

Branch by Benji, with tweaks from Nicola and myself.

https://code.launchpad.net/~gary/juju-gui/bug1090716/+merge/142794

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7069068/

Affected files:
   A [revision details]
   M app/views/topology/relation.js
   M app/views/topology/service.js
   M app/views/utils.js
   M test/test_environment_view.js
   M test/test_utils.js

Madison Scott-Clary (makyo) wrote :

This looks good and works well. Land as is, or land with minor below.

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

https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js#newcode101
app/views/topology/relation.js:101: }).length;
Given the function in utils.js, a lighter weight solution might be
http://pastebin.ubuntu.com/1518726/ - feel free to ignore, though.

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

https://codereview.appspot.com/7069068/diff/1/app/views/utils.js#newcode652
app/views/utils.js:652: };
Thanks for the comment!

https://codereview.appspot.com/7069068/

Kapil Thangavelu (hazmat) wrote :

On 2013/01/10 22:08:46, gary.poster wrote:
> Please take a look.

this seems like a lot more code then just filtering at the model / delta
stream level.

https://codereview.appspot.com/7069068/

Nicola Larosa (teknico) wrote :

Land as is.

Very nice, thanks for completing this.

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

https://codereview.appspot.com/7069068/diff/1/app/views/utils.js#newcode666
app/views/utils.js:666: return charmUrl &&
utils.isGuiCharmUrl(charmUrl);
So it's ok that this can happen, good. Yesterday I was not sure about
it.

https://codereview.appspot.com/7069068/

Kapil Thangavelu (hazmat) wrote :

On 2013/01/11 05:29:36, hazmat wrote:
> On 2013/01/10 22:08:46, gary.poster wrote:
> > Please take a look.

> this seems like a lot more code then just filtering at the model /
delta stream
> level.

more specifically trying to keep every known view/render location
guarded with a service filter is much more fragile and more code then
simply removing/preventing in one place at ingest into the app models of
the gui service.

the utility functions here feel like they should be in model.utils
instead of view.utils.

needs fixing, imo.

https://codereview.appspot.com/7069068/

Gary Poster (gary) wrote :

On 2013/01/11 00:59:15, matthew.scott wrote:
> This looks good and works well. Land as is, or land with minor below.

https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js
> File app/views/topology/relation.js (right):

https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js#newcode101
> app/views/topology/relation.js:101: }).length;
> Given the function in utils.js, a lighter weight solution might be
> http://pastebin.ubuntu.com/1518726/ - feel free to ignore, though.

Nice change. I like it, and will use it. Thank you.

Thanks for the review!

Gary

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

https://codereview.appspot.com/7069068/diff/1/app/views/utils.js#newcode652
> app/views/utils.js:652: };
> Thanks for the comment!

https://codereview.appspot.com/7069068/

Gary Poster (gary) wrote :

On 2013/01/11 12:51:03, hazmat wrote:
> On 2013/01/11 05:29:36, hazmat wrote:
> > On 2013/01/10 22:08:46, gary.poster wrote:
> > > Please take a look.
> >
> > this seems like a lot more code then just filtering at the model /
delta
> stream
> > level.

> more specifically trying to keep every known view/render location
guarded with a
> service filter is much more fragile and more code then simply
> removing/preventing in one place at ingest into the app models of the
gui
> service.

Benji and I discussed the two options in a pre-implementation call. We
preferred filtering on the view level for a few reasons.

- We were not convinced that it would be any easier in the long run to
make the change at the db/ingestion level rather than the view level.
If we had felt that this ease was the primary driver for an important
choice, we would have considered commissioning competing
implementations. However, we had other reasons to not pursue the
db/ingestion approach, given below.

- This is a view-level feature: we don't want to confuse users by
presenting certain charms. The db has nothing to do with the desired
outcome. In both of our experiences, implementing view-level behavior
in view code not only makes more sense intuitively but also is a better
long-term design decision.

- As a concrete example of why keeping the db truthful might be an
advantage, we discussed a possible future feature in which hidden
services could be revealed. For instance, let's say we want a proxy in
front of the gui or some other future hidden service. If we can reveal
hidden services with a gesture, we can make this happen, and we can also
see why a proxy that appears to be disconnected is actually connected
and performing a desired role.

> the utility functions here feel like they should be in model.utils
instead of
> view.utils.

Given our perspective that this is a view-level feature, and that these
utils are used in view-level code, the utilities make sense where they
are. Certainly if the code is refactored to filter at the ingestion/db
level then the utils belong somewhere else.

> needs fixing, imo.

I think this is an "implementer's choice" disagreement, where both sides
have reasonable arguments and the resolution should be in the hands of
the person who did the implementation. That's your call.

Gary

https://codereview.appspot.com/7069068/

Kapil Thangavelu (hazmat) wrote :
Download full text (3.5 KiB)

crappy network ate my first response, redo with brevity.

On 2013/01/11 13:44:38, gary.poster wrote:
> On 2013/01/11 12:51:03, hazmat wrote:
> > On 2013/01/11 05:29:36, hazmat wrote:
> > > On 2013/01/10 22:08:46, gary.poster wrote:
> > > > Please take a look.
> > >
> > > this seems like a lot more code then just filtering at the model /
delta
> > stream
> > > level.
> >
> >
> > more specifically trying to keep every known view/render location
guarded with
> a
> > service filter is much more fragile and more code then simply
> > removing/preventing in one place at ingest into the app models of
the gui
> > service.

> Benji and I discussed the two options in a pre-implementation call.
We
> preferred filtering on the view level for a few reasons.

> - We were not convinced that it would be any easier in the long run to
make the
> change at the db/ingestion level rather than the view level. If we
had felt
> that this ease was the primary driver for an important choice, we
would have
> considered commissioning competing implementations. However, we had
other
> reasons to not pursue the db/ingestion approach, given below.

> - This is a view-level feature: we don't want to confuse users by
presenting
> certain charms. The db has nothing to do with the desired outcome.
In both of
> our experiences, implementing view-level behavior in view code not
only makes
> more sense intuitively but also is a better long-term design decision.

the purpose of content in the db is for presentation to the user, else
it has no purpose being in the client side db.

the problem with this is a view level feature is that it has to be
implemented everywhere we render a service. If we grow new views we have
to duplicate the same filtering code there as well. From a maintenance
and efficiency perspective it seems a clear win to me, to implement once
vs. implement n.

> - As a concrete example of why keeping the db truthful might be an
advantage, we
> discussed a possible future feature in which hidden services could be
revealed.
> For instance, let's say we want a proxy in front of the gui or some
other future
> hidden service. If we can reveal hidden services with a gesture, we
can make
> this happen, and we can also see why a proxy that appears to be
disconnected is
> actually connected and performing a desired role.

that future use case isn't well defined or even clear that its valid. in
future with jaas, the typical environment won't have these services.

> >
> > the utility functions here feel like they should be in model.utils
instead of
> > view.utils.

> Given our perspective that this is a view-level feature, and that
these utils
> are used in view-level code, the utilities make sense where they are.
Certainly
> if the code is refactored to filter at the ingestion/db level then the
utils
> belong somewhere else.

regardless of usage location, the fact that these only operated on model
attributes (or in future app state) was my differentor to definition
location. its a minor though, implementor's choice on that one.

> >
> > needs fixing, imo.

> I think this is an "implementer's choice" disagreement, where both
sides have
> reasonable arguments and the res...

Read more...

Gary Poster (gary) wrote :
Download full text (8.1 KiB)

On 2013/01/11 17:22:53, hazmat wrote:
> crappy network ate my first response, redo with brevity.

> On 2013/01/11 13:44:38, gary.poster wrote:
> > On 2013/01/11 12:51:03, hazmat wrote:
> > > On 2013/01/11 05:29:36, hazmat wrote:
> > > > On 2013/01/10 22:08:46, gary.poster wrote:
> > > > > Please take a look.
> > > >
> > > > this seems like a lot more code then just filtering at the model
/ delta
> > > stream
> > > > level.
> > >
> > >
> > > more specifically trying to keep every known view/render location
guarded
> with
> > a
> > > service filter is much more fragile and more code then simply
> > > removing/preventing in one place at ingest into the app models of
the gui
> > > service.
> >
> > Benji and I discussed the two options in a pre-implementation call.
We
> > preferred filtering on the view level for a few reasons.
> >
> > - We were not convinced that it would be any easier in the long run
to make
> the
> > change at the db/ingestion level rather than the view level. If we
had felt
> > that this ease was the primary driver for an important choice, we
would have
> > considered commissioning competing implementations. However, we had
other
> > reasons to not pursue the db/ingestion approach, given below.
> >
> > - This is a view-level feature: we don't want to confuse users by
presenting
> > certain charms. The db has nothing to do with the desired outcome.
In both
> of
> > our experiences, implementing view-level behavior in view code not
only makes
> > more sense intuitively but also is a better long-term design
decision.

> the purpose of content in the db is for presentation to the user, else
it has no
> purpose being in the client side db.

Certainly. However, we have a clear model/view distinction in our code.
  The db/delta/model's job is a simple one at the moment: synchronize the
client db with the server db. Any one-off customizations from that
mission are a maintenance burden. Neither of us have proof that one
approach's maintenance burden is greater over time. As I said, IME
moving these kinds of very-high-level UI decisions down into the db
level is a mistake. You have a different perspective.

> the problem with this is a view level feature is that it has to be
implemented
> everywhere we render a service. If we grow new views we have to
duplicate the
> same filtering code there as well.

With trivial and already-implemented code reuse. Moreover, your "grow
new views" story is a less concrete example than the one I gave, with no
user story associated with it.

> From a maintenance and efficiency perspective
> it seems a clear win to me, to implement once vs. implement n.

In terms of code efficiency, it is true that simply not rendering the
GUI service rather than hiding it would reduce some computation.
However, that appears to be a negligible increase as I understand the
code paths.

In terms of developer efficiency, I think it is a matter of opinion,
given the current state of the app and future plans.

> >
> > - As a concrete example of why keeping the db truthful might be an
advantage,
> we
> > discussed a possible future feature in which hidden services could
be
> revealed.
> > For instance, let's say we w...

Read more...

Kapil Thangavelu (hazmat) wrote :
Download full text (7.6 KiB)

On Fri, Jan 11, 2013 at 12:54 PM, <email address hidden> wrote:

> On 2013/01/11 17:22:53, hazmat wrote:
>
>> crappy network ate my first response, redo with brevity.
>>
>
> On 2013/01/11 13:44:38, gary.poster wrote:
>> > On 2013/01/11 12:51:03, hazmat wrote:
>> > > On 2013/01/11 05:29:36, hazmat wrote:
>> > > > On 2013/01/10 22:08:46, gary.poster wrote:
>> > > > > Please take a look.
>> > > >
>> > > > this seems like a lot more code then just filtering at the model
>>
> / delta
>
>> > > stream
>> > > > level.
>> > >
>> > >
>> > > more specifically trying to keep every known view/render location
>>
> guarded
>
>> with
>> > a
>> > > service filter is much more fragile and more code then simply
>> > > removing/preventing in one place at ingest into the app models of
>>
> the gui
>
>> > > service.
>> >
>> > Benji and I discussed the two options in a pre-implementation call.
>>
> We
>
>> > preferred filtering on the view level for a few reasons.
>> >
>> > - We were not convinced that it would be any easier in the long run
>>
> to make
>
>> the
>> > change at the db/ingestion level rather than the view level. If we
>>
> had felt
>
>> > that this ease was the primary driver for an important choice, we
>>
> would have
>
>> > considered commissioning competing implementations. However, we had
>>
> other
>
>> > reasons to not pursue the db/ingestion approach, given below.
>> >
>> > - This is a view-level feature: we don't want to confuse users by
>>
> presenting
>
>> > certain charms. The db has nothing to do with the desired outcome.
>>
> In both
>
>> of
>> > our experiences, implementing view-level behavior in view code not
>>
> only makes
>
>> > more sense intuitively but also is a better long-term design
>>
> decision.
>
> the purpose of content in the db is for presentation to the user, else
>>
> it has no
>
>> purpose being in the client side db.
>>
>
> Certainly. However, we have a clear model/view distinction in our code.
> The db/delta/model's job is a simple one at the moment: synchronize the
> client db with the server db. Any one-off customizations from that
> mission are a maintenance burden.

all code is a maintenance burden, that's why its generally better to do
something once then repeat yourself in code (DRY).

> Neither of us have proof that one
> approach's maintenance burden is greater over time.

The number of touchpoints with the current implementation is clearly
greater than the alternative i've posited, and will continue to grow as we
introduce additional views. Moreover it introduces valid paths to data that
we want to hide that we're not exercising or testing, for example direct
navigation to the gui url service, etc, that are not being accounted for.
All of those issues would disappear with the alternative implementation
path.

As I said, IME
> moving these kinds of very-high-level UI decisions down into the db
> level is a mistake. You have a different perspective.

> the problem with this is a view level feature is that it has to be
>>
> implemented
>
>> everywhere we render a service. If we grow new views we have to
>>
> duplicate the
>
>> same filtering code there as well.
>>
>
> With trivial and alre...

Read more...

Kapil Thangavelu (hazmat) wrote :
Download full text (3.7 KiB)

The root of the issues here come out to the choice and the reason for it made at the preimplementation given two paths forward. The analogy to me is filtering database content at the app level in each view rendering the given data types instead of abstracting it via db query at either the orm or db level. The principle i'm trying to express is if there is a clear place to implement something once it should be used in preference to implementing it multiple times. The consequence of not doing it is increased costs and fragility because of multiple implementations that need to be kept in sync and duplicated for further display of this data. As an example of that fragility, this implementation leaves out an additional place the filtering logic needs to be cargo copied for correctness in the route for the service. The reason for the pre-implementation choice is specious imo as well given the logic for filtering is behind the utility filtering function, even if it where at the db level, the functionality for the additonal use case presented would be implementable simply via additional parameterization/logic in that utility function.

regarding rework. code is not set in stone, its living and breathing, and rework and refactoring are critical to achieving high quality code. Without it, code decays as the code and feature set evolve without wholistic evaluation.

its unreasonable to expect that reviews only focus on micro concerns, macro concerns are have to be considered fair game for review to achieve the goal of high quality software. Its cheaper in the software development lifecycle to catch issues before they land.

its also unreasonable to expect that a bug/feature request can be so fully specified that any implementation path that leads to working code and tests is considered good. The principle of implementing something once vs multiple times is generally considered obvious to a software engineer, people who implemented this branch have remarked on the same principle in their reviews of other people's work. iotw. its not something that i think is valuable noted in a separate principles page.

Robert's perspective and principles where guided by the situtation and problem he was asked to solve. Namely approaching a multi-million line code base with hundreds of man years spread across dozens of machine and tasked to make it cheaper, faster, and more agile. It also provided a great basis to analyze, measure and derive such principles. Those principles where high-level archiecture oriented though not implementation focused.

The problem domain we're dealing with is much different. Its a small code base and small team and we're evolving practices and knowledge as our feature set and targets grow. The topology rework is being done is a good example of us attempting to incorporate best practices after the fact. Its healthy but time-consuming compared to being able to implement it at the get go.

As a set of principles beyond KISS, i've highlighted this before.
http://gorban.org/post/32873465932/software-architecture-cheat-sheet

its the orthogonal and dry principles that this branch has issues w/ currently.

I'm happy to look over previous rework ...

Read more...

Gary Poster (gary) wrote :
Download full text (5.7 KiB)

Thanks for the reply, Kapil. Brief comments/updates inline below.

> The root of the issues here come out to the choice and the reason for it made
> at the preimplementation given two paths forward.
...

We have different perspectives. In trying to determine the way forward from your comments, Benji and I developed an analysis of next steps. I introduce it below.

> regarding rework.
...

Unfortunately we have a number of disagreements. As I said before, I agree that rework is sometimes necessary in a review. We disagree on its acceptable frequency, its implications about process, and other topics. I'll pursue these further in a different time and forum.

> all that said, i'm okay with this proceeding without rework inspite of my
> concerns, given the service route is also updated (ie 3 conditionals), in the
> hope that's its easy to refactor/remove if its a problem in the future.

Benji and I talked about this. You have a different goal than we had in mind, and we think there might be an even better goal than what either of us had thought.

Our goal was to keep the service from cluttering the environment--to keep the GUI away from the user in the common case. In this approach, nothing is ever broken even if a link to a hidden service is inadvertently revealed, because routes still work. You can also reveal and hide services on demand. We identified two downsides. First, there would be an opportunity for some surprise if a user found notifications or relations referring to a hidden service. This shouldn't happen, and if it did, nothing would break. Second, when you try to create a service, messages about existing names for a hidden service might be confusing. However, the client app would actually know about the hidden service, and we could write a custom error message if this proved to be a real problem.

To do this, Benji had two conceptual tasks:
- Hide services in the environment view.
- Do not draw relations to hidden services in the environment view.

That's what this branch does, along with various tests.

You've asked for a different goal, to completely hide the service. That goal has one user advantage within the GUI, in that it reduces the possibility of surprise. However, it sets up expectations that are viral: any application that links to the GUI, such as Landscape, but also not link to the GUI for services that are hidden. Other alternatives start to look a lot like the "don't clutter the environment goal," above. It also involves more changes. I'll talk about the changes associated with this goal, given two possible implementation approaches, in a moment.

Before I do so, I wanted to mention a third, alternative goal: keep the user from destroying the GUI unintentionally. Nothing is ever broken, and nothing is ever surprising, because we show the truth. While the GUI charm might somewhat clutter up the environment, users can move it to the side, and this placement will even be persistent soon. To accomplish this, we only need on change from trunk, conceptually: disable the service "destroy" button, with or without a message. This accomplishes the critical user goal with the absolute least code and the lea...

Read more...

Benji York (benji) wrote :

We had a short video conference discussion of this and settled on choosing the goal of "keeping the user from shooting themselves in the foot" as superior to all others at this point in time. As such I will create a new branch that:

1) disables the "destroy" UI for the Juju GUI service
2) disables the "unexpose" UI for the Juju GUI service
3) places a warning on the Juju GUI service configuration page that alerts the user that changing the configuration may disrupt their ability to use the GUI.

Unmerged revisions

315. By Gary Poster on 2013-01-10

Mark branch as fixing bug 1090716

314. By Gary Poster on 2013-01-10

lint

313. By Gary Poster on 2013-01-10

make test work, and make hiding unofficial juju-gui charms work.

312. By Gary Poster on 2013-01-10

merge trunk

311. By Gary Poster on 2013-01-10

fix tests

310. By Nicola Larosa on 2013-01-10

Merge from trunk.

309. By Nicola Larosa on 2013-01-10

Some cleanup, added commented code for interface tests.

308. By Nicola Larosa on 2013-01-10

Merge from trunk, one conflict resolved.

307. By Benji York on 2013-01-09

checkpoint; working and tested; still needs functional tests

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/views/topology/relation.js'
2--- app/views/topology/relation.js 2013-01-03 20:38:59 +0000
3+++ app/views/topology/relation.js 2013-01-10 22:09:22 +0000
4@@ -89,9 +89,19 @@
5
6 processRelations: function(rels) {
7 var self = this;
8+ var db = this.get('component').get('db');
9 var pairs = [];
10 Y.each(rels, function(rel) {
11 var pair = self.processRelation(rel);
12+ // If one end of the relation is with the Juju GUI service,
13+ // do not draw the relation.
14+ var relationWithGUI = !!rel.get('endpoints').filter(function(details) {
15+ var endpoint = db.services.getById(details[0]);
16+ return utils.isGuiService(endpoint);
17+ }).length;
18+ if (relationWithGUI) {
19+ return;
20+ }
21
22 // skip peer for now
23 if (pair.length === 2) {
24
25=== modified file 'app/views/topology/service.js'
26--- app/views/topology/service.js 2013-01-10 20:01:12 +0000
27+++ app/views/topology/service.js 2013-01-10 22:09:22 +0000
28@@ -361,7 +361,9 @@
29 .call(function() {
30 // Create new nodes.
31 self.createServiceNode(this);
32- });
33+ })
34+ .filter(utils.isGuiService) // Hide the Juju GUI.
35+ .style('display', 'none');
36
37 // Update all nodes.
38 self.updateServiceNodes(node);
39@@ -608,6 +610,10 @@
40 .text(function(d) {
41 return utils.humanizeNumber(d.unit_count);
42 });
43+
44+ // The Juju GUI service is hidden.
45+ node.filter(utils.isGuiService)
46+ .style('display', 'none');
47 },
48
49
50@@ -615,23 +621,23 @@
51 * Show/hide/fade selection.
52 */
53 show: function(evt) {
54- var selection = evt.selection;
55- selection.attr('opacity', '1.0')
56- .style('display', 'block');
57+ evt.selection.filter(utils.negate(utils.isGuiService))
58+ .attr('opacity', '1.0')
59+ .style('display', 'block');
60 },
61
62 hide: function(evt) {
63- var selection = evt.selection;
64- selection.attr('opacity', '0')
65- .style('display', 'none');
66+ evt.selection
67+ .attr('opacity', '0')
68+ .style('display', 'none');
69 },
70
71 fade: function(evt) {
72- var selection = evt.selection,
73- alpha = evt.alpha;
74- selection.transition()
75- .duration(400)
76- .attr('opacity', alpha !== undefined && alpha || '0.2');
77+ var alpha = evt.alpha;
78+ evt.selection
79+ .transition()
80+ .duration(400)
81+ .attr('opacity', alpha !== undefined && alpha || '0.2');
82 },
83
84 /*
85@@ -676,8 +682,8 @@
86 },
87
88 /*
89- * Event handler to show the graph-list picker
90- */
91+ * Event handler to show the graph-list picker
92+ */
93 showGraphListPicker: function(evt) {
94 var container = this.get('container'),
95 picker = container.one('.graph-list-picker');
96@@ -695,6 +701,9 @@
97 picker.one('.picker-expanded').removeClass('active');
98 },
99
100+ /*
101+ * Update the location of the active service panel.
102+ */
103 updateServiceMenuLocation: function() {
104 var topo = this.get('component'),
105 container = this.get('container'),
106
107=== modified file 'app/views/utils.js'
108--- app/views/utils.js 2013-01-07 13:59:45 +0000
109+++ app/views/utils.js 2013-01-10 22:09:22 +0000
110@@ -603,7 +603,85 @@
111 return errors;
112 };
113
114-
115+ /**
116+ * Determine if a service is the Juju GUI by inspecting the charm URL.
117+ *
118+ * @method isGuiCharmUrl
119+ * @param {String} charmUrl The service charm URL to inspect.
120+ * @return {Boolean} True if the charm URL is that of the Juju GUI.
121+ */
122+ utils.isGuiCharmUrl = function(charmUrl) {
123+ // Regular expression to match charm URLs. Explanation generated by
124+ // http://rick.measham.id.au/paste/explain.pl and then touched up a bit to
125+ // fit in our maximum line width. Note that the bit about an optional
126+ // newline matched by $ is a Perlism that JS does not share.
127+ //
128+ // NODE EXPLANATION
129+ // ------------------------------------------------------------------------
130+ // ^ the beginning of the string
131+ // ------------------------------------------------------------------------
132+ // (?: group, but do not capture:
133+ // ------------------------------------------------------------------------
134+ // [^:]+ any character except: ':' (1 or more
135+ // times (matching the most amount
136+ // possible))
137+ // ------------------------------------------------------------------------
138+ // : ':'
139+ // ------------------------------------------------------------------------
140+ // ) end of grouping
141+ // ------------------------------------------------------------------------
142+ // (?: group, but do not capture (0 or more times
143+ // (matching the most amount possible)):
144+ // ------------------------------------------------------------------------
145+ // [^\/]+ any character except: '\/' (1 or more
146+ // times (matching the most amount
147+ // possible))
148+ // ------------------------------------------------------------------------
149+ // \/ '/'
150+ // ------------------------------------------------------------------------
151+ // )* end of grouping
152+ // ------------------------------------------------------------------------
153+ // juju-gui- 'juju-gui-'
154+ // ------------------------------------------------------------------------
155+ // \d+ digits (0-9) (1 or more times (matching
156+ // the most amount possible))
157+ // ------------------------------------------------------------------------
158+ // $ before an optional \n, and the end of the
159+ // string
160+ return (/^(?:[^:]+:)(?:[^\/]+\/)*juju-gui-\d+$/).test(charmUrl);
161+ };
162+
163+ /**
164+ * Determine if a service is the Juju GUI by inspecting the charm URL.
165+ *
166+ * @method isGuiService
167+ * @param {Object} candidate The service to inspect.
168+ * @return {Boolean} True if the service is the Juju GUI.
169+ */
170+ utils.isGuiService = function(candidate) {
171+ // Some candidates have the charm URL as an attribute, others require a
172+ // "get", and others do not actually have either.
173+ var charmUrl = (
174+ candidate.charm || candidate.get && candidate.get('charm'));
175+ return charmUrl && utils.isGuiCharmUrl(charmUrl);
176+ };
177+
178+ /**
179+ * Returns a function is the logical complement of the given function.
180+ *
181+ * This is used in contexts that call for the inversion of a predicate, e.g.
182+ * selection.filter(utils.negate(utils.isGuiService)).
183+ *
184+ * @method negate
185+ * @param {Boolean} f The function we want to negate.
186+ * @return {Function} A function that returns the boolean inverse of f given
187+ * the same arguments.
188+ */
189+ utils.negate = function(f) {
190+ return function() {
191+ return !f.apply(this, arguments);
192+ };
193+ };
194
195 /*
196 * Utility class that encapsulates Y.Models and keeps their positional
197
198=== modified file 'test/test_environment_view.js'
199--- test/test_environment_view.js 2013-01-09 17:17:46 +0000
200+++ test/test_environment_view.js 2013-01-10 22:09:22 +0000
201@@ -731,4 +731,74 @@
202
203 });
204
205+ describe('Hiding Juju GUI service and relations', function() {
206+ var Y, views;
207+ var test = it; // No need to pretend we are doing BDD.
208+
209+ var makeService = function(charmUrl) {
210+ return {
211+ charm: charmUrl,
212+ attrCalledWith: {},
213+ styleCalledWith: {},
214+ attr: function() {
215+ this.attrCalledWith = arguments;
216+ return this;
217+ },
218+ style: function() {
219+ this.styleCalledWith = arguments;
220+ return this;
221+ }
222+ };
223+ };
224+
225+ before(function(done) {
226+ Y = YUI(GlobalConfig).use(['juju-views', 'juju-models'],
227+ function(Y) {
228+ views = Y.namespace('juju.views');
229+ done();
230+ });
231+ });
232+
233+ test('Juju GUI services should not be shown', function() {
234+ var services = [
235+ makeService('cs:precise/haproxy'),
236+ makeService('cs:precise/juju-gui-7'),
237+ makeService('cs:precise/mysql')
238+ ];
239+ function StubSelection(services) {
240+ this.services = services;
241+ }
242+ StubSelection.prototype.filter = function(predicate) {
243+ return new StubSelection(this.services.filter(predicate));
244+ };
245+ StubSelection.prototype.attr = function() {
246+ var args = arguments;
247+ Y.each(this.services, function(s) {s.attr.apply(s, args);});
248+ return this;
249+ };
250+ StubSelection.prototype.style = function() {
251+ var args = arguments;
252+ Y.each(this.services, function(s) {s.style.apply(s, args);});
253+ return this;
254+ };
255+ var evt = {selection: new StubSelection(services)};
256+
257+ (new views.ServiceModule()).show(evt);
258+
259+ // This is haproxy.
260+ assert.deepEqual(
261+ services[0].attrCalledWith, {'0': 'opacity', '1': '1.0'});
262+ assert.deepEqual(
263+ services[0].styleCalledWith, {'0': 'display', '1': 'block'});
264+ // This is juju-gui.
265+ assert.deepEqual(services[1].attrCalledWith, {});
266+ assert.deepEqual(services[1].styleCalledWith, {});
267+ // This is mysql.
268+ assert.deepEqual(
269+ services[2].attrCalledWith, {'0': 'opacity', '1': '1.0'});
270+ assert.deepEqual(
271+ services[2].styleCalledWith, {'0': 'display', '1': 'block'});
272+ });
273+ });
274+
275 })();
276
277=== modified file 'test/test_utils.js'
278--- test/test_utils.js 2012-10-12 23:52:48 +0000
279+++ test/test_utils.js 2013-01-10 22:09:22 +0000
280@@ -3,6 +3,7 @@
281 (function() {
282 describe('juju-views-utils', function() {
283 var views, Y;
284+
285 before(function(done) {
286 Y = YUI(GlobalConfig).use(
287 'juju-view-utils', 'node-event-simulate',
288@@ -339,3 +340,106 @@
289
290 });
291 })();
292+
293+(function() {
294+ describe('utils.negate', function() {
295+
296+ var utils, Y;
297+
298+ before(function(done) {
299+ Y = YUI(GlobalConfig).use('juju-views', function(Y) {
300+ utils = Y.namespace('juju.views.utils');
301+ done();
302+ });
303+ });
304+
305+ it('should return false if the function returns true', function() {
306+ var f = function() { return true; };
307+ var n = utils.negate(f);
308+ assert.isFalse(n());
309+ });
310+
311+ it('should return true if the function returns false', function() {
312+ var f = function() { return false; };
313+ var n = utils.negate(f);
314+ assert.isTrue(n());
315+ });
316+
317+ it('should forward all arguments to the underlying function', function() {
318+ var passed_arguments;
319+ var f = function() {
320+ passed_arguments = arguments;
321+ };
322+ var n = utils.negate(f);
323+ n(1, 2, 3);
324+ assert.deepEqual(passed_arguments, {'0': 1, '1': 2, '2': 3});
325+ });
326+
327+ });
328+})();
329+
330+(function() {
331+ describe('utils.isGuiService', function() {
332+
333+ var utils, Y;
334+
335+ before(function(done) {
336+ Y = YUI(GlobalConfig).use('juju-views', function(Y) {
337+ utils = Y.namespace('juju.views.utils');
338+ done();
339+ });
340+ });
341+
342+ it('should extract values from "charm" attribute', function() {
343+ var candidate = {charm: 'cs:precise/juju-gui-7'};
344+ assert.isTrue(utils.isGuiService(candidate));
345+ });
346+
347+ it('should extract values from .get("charm")', function() {
348+ var candidate = {
349+ get: function(name) {
350+ if (name === 'charm') {
351+ return 'cs:precise/juju-gui-7';
352+ }
353+ }
354+ };
355+ assert.isTrue(utils.isGuiService(candidate));
356+ });
357+
358+ });
359+})();
360+
361+(function() {
362+ describe('utils.isGuiCharmUrl', function() {
363+
364+ var utils, Y;
365+
366+ before(function(done) {
367+ Y = YUI(GlobalConfig).use('juju-views', function(Y) {
368+ utils = Y.namespace('juju.views.utils');
369+ done();
370+ });
371+ });
372+
373+ it('should recognize charm store URLs', function() {
374+ assert.isTrue(utils.isGuiCharmUrl('cs:precise/juju-gui-7'));
375+ });
376+
377+ it('should recognize unofficial charm store URLs', function() {
378+ assert.isTrue(utils.isGuiCharmUrl('cs:~foobar/precise/juju-gui-7'));
379+ });
380+
381+ it('should ignore owners of unofficial charm store URLs', function() {
382+ assert.isFalse(utils.isGuiCharmUrl('cs:~juju-gui/precise/foobar-7'));
383+ });
384+
385+ it('should recognize local charm URLs', function() {
386+ assert.isTrue(utils.isGuiCharmUrl('local:juju-gui-3'));
387+ });
388+
389+ it('should not allow junk on the end of the URL', function() {
390+ assert.isFalse(utils.isGuiCharmUrl('local:juju-gui-3 ROFLCOPTR!'));
391+ });
392+
393+ });
394+})();

Subscribers

People subscribed via source and target branches