Launchpad itself

Code review comment for lp:~allenap/launchpad/me-too-ajax-bug-361097-unit-tests

  1. me-too-ajax-bug-361097-unit-tests
  2. Merge into devel
Revision history for this message
Gavin Panella (allenap) wrote :

On Tue, 04 Aug 2009 16:34:43 -0000
Deryck Hodge <email address hidden> wrote:

> Review: Needs Fixing
> Hi, Gavin.
>
> This is nice work overall. ~900 lines isn't so bad to review. :)

Thanks for the review Deryck.

> Some minor things need fixing, most of which I mentioned on IRC:
>
> ** The inline CSS in test_me_too.html can be dropped in
> favor of:
>
> <link rel="stylesheet" href="../../test.css" />

Neat, didn't know about that, thanks.

...
> I will note that when I played with this, the overlay opened in weird
> locations if I had scrolled down the page some before trying to click
> the "affects me too" link. For example, if the link was near the top
> of the window, the overlay was partially off screen and I had to
> scroll back up. This isn't a huge blocking issue, IMHO, and I assume
> covered in the UI review, but maybe we want to track a bug against the
> widget if one isn't already open.

I can't replicate this. Didn't intellectronica land a fix in lazr-js
related to positioning earlier today? Perhaps pulling your lazr-js
dependency will fix this. Can you try and let me know if it still
breaks?

I've attached a diff of the changes since the review.

post-review.diff
1=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
2--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-08-04 14:57:07 +0000
3+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-08-04 16:17:45 +0000
4@@ -1331,19 +1331,8 @@
5 bugs.setup_me_too = function(user_is_affected) {
6 var me_too_content = Y.get('#affectsmetoo');
7 var me_too_edit = new MeTooChoiceSource({
8- contentBox: me_too_content,
9- value: user_is_affected,
10- title: 'Does this bug affect you?',
11- items: [
12- {name: 'Yes, it affects me', value: true,
13- source_name: 'This bug affects me too',
14- disabled: false},
15- {name: "No, it doesn't affect me", value: false,
16- source_name: "This bug doesn't affect me",
17- disabled: false}
18- ],
19- elementToFlash: me_too_content,
20- backgroundColor: '#FFFFFF'
21+ contentBox: me_too_content, value: user_is_affected,
22+ elementToFlash: me_too_content
23 });
24 me_too_edit.render();
25 };
26@@ -1369,6 +1358,33 @@
27
28 MeTooChoiceSource.ATTRS = {
29 /**
30+ * The title is always the same, so bake it in here.
31+ *
32+ * @attribute title
33+ * @type String
34+ */
35+ title: {
36+ value: 'Does this bug affect you?'
37+ },
38+
39+ /**
40+ * The items are always the same, so bake them in here.
41+ *
42+ * @attribute items
43+ * @type Array
44+ */
45+ items: {
46+ value: [
47+ { name: 'Yes, it affects me', value: true,
48+ source_name: 'This bug affects me too',
49+ disabled: false },
50+ { name: "No, it doesn't affect me", value: false,
51+ source_name: "This bug doesn't affect me",
52+ disabled: false }
53+ ]
54+ },
55+
56+ /**
57 * Y.Node containing a flame icon, displayed when the user is
58 * affected by the current bug. Should be automatically calculated
59 * by HTML_PARSER.
60
61=== modified file 'lib/canonical/launchpad/javascript/bugs/tests/test_me_too.html'
62--- lib/canonical/launchpad/javascript/bugs/tests/test_me_too.html 2009-08-03 14:38:44 +0000
63+++ lib/canonical/launchpad/javascript/bugs/tests/test_me_too.html 2009-08-04 16:47:37 +0000
64@@ -21,33 +21,8 @@
65 <!-- The test suite -->
66 <script type="text/javascript" src="test_me_too.js"></script>
67
68- <style type="text/css">
69- /* Taken and customized from testlogger.css */
70- #log .yui-console-content { width:44em }
71- #log .yui-console .yui-console-bd { height:30em }
72- #log .yui-console .yui-console-controls { display:none; }
73- #log .yui-console .yui-console-hd { display:none; }
74- #log .yui-console .yui-console-ft { position:absolute;top:0em; }
75-
76- #log .yui-console-entry-src { display:none; }
77-
78- #log .yui-console-entry-pass .yui-console-entry-cat {
79- background-color: green;
80- font-weight: bold;
81- color: white;
82- }
83- #log .yui-console-entry-fail .yui-console-entry-cat {
84- background-color: red;
85- font-weight: bold;
86- color: white;
87- }
88- #log .yui-console-entry-ignore .yui-console-entry-cat {
89- background-color: #666;
90- font-weight: bold;
91- color: white;
92- }
93- </style>
94-
95+ <!-- Test layout -->
96+ <link rel="stylesheet" href="../../test.css" />
97 <style type="text/css">
98 /* CSS classes specific to this test */
99 .unseen { display: none; }
100
101=== modified file 'lib/canonical/launchpad/javascript/bugs/tests/test_me_too.js'
102--- lib/canonical/launchpad/javascript/bugs/tests/test_me_too.js 2009-08-04 14:45:29 +0000
103+++ lib/canonical/launchpad/javascript/bugs/tests/test_me_too.js 2009-08-04 16:17:40 +0000
104@@ -11,10 +11,6 @@
105 var Assert = Y.Assert,
106 ArrayAssert = Y.ArrayAssert;
107
108-Assert.isEqual = function(a, b, msg) {
109- Assert.isTrue(a == b, msg);
110-};
111-
112 /*
113 * A wrapper for the Y.Event.simulate() function. The wrapper accepts
114 * CSS selectors and Node instances instead of raw nodes.
115@@ -46,11 +42,10 @@
116 setUp: function() {
117 // Monkeypatch LP.client to avoid network traffic and to make
118 // some things work as expected.
119- LP.client.Launchpad.prototype.named_post = (
120+ LP.client.Launchpad.prototype.named_post =
121 function(url, func, config) {
122 config.on.success();
123- }
124- );
125+ };
126 LP.client.cache.bug = {
127 self_link: "http://bugs.example.com/bugs/1234"
128 };
129@@ -76,19 +71,8 @@
130 Y.get("body").appendChild(inpage);
131 var me_too_content = Y.get('#affectsmetoo');
132 this.config = {
133- contentBox: me_too_content,
134- value: null,
135- title: 'This bug:',
136- items: [
137- {name: 'Affects me too', value: true,
138- source_name: 'This bug affects me too',
139- disabled: false},
140- {name: 'Does not affect me', value: false,
141- source_name: "This bug doesn't affect me",
142- disabled: false}
143- ],
144- elementToFlash: me_too_content,
145- backgroundColor: '#FFFFFF'
146+ contentBox: me_too_content, value: null,
147+ elementToFlash: me_too_content
148 };
149 this.choice_edit = new Y.bugs._MeTooChoiceSource(this.config);
150 this.choice_edit.render();
151@@ -109,7 +93,7 @@
152 */
153 test_is_inline: function() {
154 var display = this.choice_edit.get('boundingBox').getStyle('display');
155- Assert.isEqual(
156+ Assert.areEqual(
157 display, 'inline', "Not displayed inline, display is: " + display);
158 },
159
160@@ -206,12 +190,11 @@
161 // assertions in this method because exceptions are swallowed
162 // somewhere. Instead, we save something testable to a local
163 // var.
164- LP.client.Launchpad.prototype.named_post = (
165+ LP.client.Launchpad.prototype.named_post =
166 function(url, func, config) {
167 edit_icon_src_during_save = edit_icon.get('src');
168 config.on[callback]();
169- }
170- );
171+ };
172 simulate(this.choice_edit._choice_list.get('boundingBox'),
173 'li a[href$=true]', 'click');
174 Assert.isNotNull(
175@@ -222,33 +205,6 @@
176 Assert.isNull(
177 edit_icon.get('src').match(/\/spinner$/),
178 "The edit icon is displaying a spinner once the choice has been made.");
179- },
180-
181- /**
182- * Test that the save call back to Launchpad makes sense.
183- */
184- test_save_data: function() {
185- var url, func, config;
186- LP.client.Launchpad.prototype.named_post = (
187- function(f_url, f_func, f_config) {
188- url = f_url, func = f_func, config = f_config;
189- config.on.success();
190- }
191- );
192- // Simulate selecting something.
193- simulate(this.choice_edit.get('boundingBox'), '.value', 'mousedown');
194- simulate(this.choice_edit._choice_list.get('boundingBox'),
195- 'li a[href$=true]', 'click');
196- // See what we've got.
197- Assert.isEqual(url, "http://bugs.example.com/bugs/1234",
198- "LP client call to wrong URL.");
199- Assert.isEqual(func, "markUserAffected",
200- "LP client call to wrong method.");
201- Assert.isNotNull(config.on);
202- Assert.isNotNull(config.on.success);
203- Assert.isNotNull(config.on.failure);
204- Assert.isNotNull(config.parameters);
205- Assert.isTrue(config.parameters.affected);
206 }
207
208 }));

« Back to merge proposal