Merge lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 into lp:launchpad
- confirm-reviewer-subscription-330290
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Ian Booth |
Approved revision: | no longer in the source branch. |
Merged at revision: | 14744 |
Proposed branch: | lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 |
Merge into: | lp:launchpad |
Prerequisite: | lp:~wallyworld/launchpad/person-getBranchVisibilityInfo-api |
Diff against target: |
670 lines (+470/-65) 10 files modified
lib/lp/app/javascript/picker/picker_patcher.js (+69/-1) lib/lp/app/javascript/picker/tests/test_picker_patcher.js (+1/-1) lib/lp/app/widgets/templates/form-picker-macros.pt (+2/-28) lib/lp/app/widgets/tests/test_popup.py (+1/-1) lib/lp/bugs/javascript/bugtask_index.js (+5/-6) lib/lp/bugs/javascript/tests/test_bugtask_delete.js (+11/-5) lib/lp/code/javascript/branchmergeproposal.nominate.js (+133/-0) lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html (+51/-0) lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js (+191/-0) lib/lp/code/templates/branch-register-merge.pt (+6/-23) |
To merge this branch: | bzr merge lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
j.c.sackett (community) | Approve | ||
Richard Harding (community) | code* | Approve | |
Review via email: mp+90070@code.launchpad.net |
Commit message
[r=jcsackett,
Description of the change
== Implementation ==
Basically just a bunch of new javascript in module 'lp.code.
The javascript provides a new picker validation callback - when a reviewer is chosen, the validator makes an API call to see if the target branch and source branch are visible to the nominated reviewer. If not, the user is prompted to confirm the selected reviewer.
== Demo and QA ==
The screenshot shows what the confirmation prompt in the picker looks like:
http://
== Tests ==
Add some new yui tests for the new javascript module:
test_branchmerg
test_branchmerg
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
lib/lp/
j.c.sackett (jcsackett) wrote : | # |
Ian--
Thanks for this branch, I know it's been a real pain.
I think Rick's suggestion for mustache would clean up that html generation block a good bit, but I wouldn't spend more than an hour trying to get it to work--just make an XXX after that amount of time if not, because we do need to get this part of disclosure over with.
I like Rick's mock, but I can see that you're dealing with the existing issues of the picker in this code. At a minimum, I would ask that if you not take his suggestions at this time, you file a bug and link in his mock as an approach in a bug comment, so that someone on purple can do more cleaning of the pickers when we're on maintenance.
I do wonder at the need for the global lp_client variable; I'm guessing it's so you can pass the mock io in the conf in testing? It looks like a possible problem waiting to happen. Perhaps you could create a function factory that sets up the necessary lp.client for check_reviewer_
I'm marking this as approved, b/c again I don't see this as a blocker or as something that more than an hour of your day should be spent in trying to fix--there's nothing actually wrong with it per se, but I'm leery of it.
Ian Booth (wallyworld) wrote : | # |
Hi Rick
Thanks for a very thoughtful review.
>
> In your tests you setup a LP = {}, but the test write out a window.LP. Then the code uses just LP. If you reference things as window. I'd find it clearer for others to see all occurances using that so that it's explicit vs trusting that LP is found up the scope chain.
>
Sure, fixed. The construct used originally is how we do it everywhere else.
> In the tests you use var reviewer = Y.one("
>
Yes, YUI doesn't like ids with "." in them.
> The validation namespace is ENV., but you're using YUI.namespace which the docs seem to indicate is global already? I know it has to match what's already there, so this is more a question at this point than a request for change.
>
The validation namespace was changed in a dependent branch and no longer
uses ENV.
> There's enough html generation in there that I think it'd be nice to clean it up using Mustache. You could use it to build the content, place the person name, and loop through the branches.
>
Done as per you example. However, thus mustache template can't live in
the TAL since it also needs to be available for yui tests, so I've used
the same technique as elsewhere, eg ['foo', 'bar'].join('')
> Now onto the optional stuff. I tend to try to set this up in a YUI object to make it a bit easier to follow and track the various functions defined and used. They're all placed upon the namespace, and I understand that for testing, but then it makes them all potential calling/entry points for code elsewhere.
>
> I've mocked up (completely not run through/tested/etc) a YUI object with a normal YUI initialize and a setup() method to mimic what it appears you've got there. I also added the Mustache templating to kind of show how confirm_reviewer simplifies a little bit. I've tried to indicate "private" methods with _ vs the ones that appear to be externally callable for real use.
>
> https:/
>
The approach I used matches our current codebase. I'd like consensus on
a design etc before we introduce yet another variation into the
codebase. I've included an XXX and raised bug 925818 for this work.
> Consider this just a transfer of technique and take from it what you will. Let me know if you have any questions or concerns. I understand this is hooking into existing picker code.
Preview Diff
1 | === modified file 'lib/lp/app/javascript/picker/picker_patcher.js' |
2 | --- lib/lp/app/javascript/picker/picker_patcher.js 2012-02-03 06:31:31 +0000 |
3 | +++ lib/lp/app/javascript/picker/picker_patcher.js 2012-02-03 06:31:31 +0000 |
4 | @@ -9,6 +9,74 @@ |
5 | var BATCH_SIZE = 6; |
6 | var MAX_BATCHES = 20; |
7 | |
8 | +// An internal method to add a picker to a text field with a Choose... link. |
9 | +var _addPicker = function(config, show_widget_id) { |
10 | + var picker = null; |
11 | + var vocabulary = config.vocabulary_name; |
12 | + var vocabulary_filters = config.vocabulary_filters; |
13 | + var input_element = config.input_element; |
14 | + // Sort out the Choose... link. |
15 | + var show_widget_node = Y.one('#' + show_widget_id); |
16 | + if (!Y.Lang.isValue(show_widget_node) ) { |
17 | + return; |
18 | + } |
19 | + // The node may already have been processed. |
20 | + if (show_widget_node.hasClass('js-action')) { |
21 | + return; |
22 | + } |
23 | + show_widget_node.set('innerHTML', 'Choose…'); |
24 | + show_widget_node.addClass('js-action'); |
25 | + show_widget_node.get('parentNode').removeClass('unseen'); |
26 | + show_widget_node.on('click', function (e) { |
27 | + if (picker === null) { |
28 | + picker = namespace.create( |
29 | + vocabulary, config, input_element, |
30 | + vocabulary_filters); |
31 | + } |
32 | + picker.show(); |
33 | + e.preventDefault(); |
34 | + }); |
35 | +}; |
36 | + |
37 | + |
38 | +/** |
39 | + * Adds a picker to a text field with a Choose... link. |
40 | + * @param {Object} config Object literal of config name/value pairs. The |
41 | + * values listed below are common for all picker types. |
42 | + * config.vocabulary_name: the named vocabulary to select from. |
43 | + * config.vocabulary_filters: any vocaulary filters to use. |
44 | + * config.input_element: the id of the text field to update with the |
45 | + * selected value. |
46 | + * config.picker_type: the type of picker to create (default or person). |
47 | + * config.header: a line of text at the top of the widget. |
48 | + * config.step_title: overrides the subtitle. |
49 | + * config.null_display_value: Override the default 'None' text. |
50 | + * config.show_search_box: Should the search box be shown. |
51 | + * Vocabularies that are not huge should not have a search box. |
52 | + * @param show_widget_id the id of the Choose... link |
53 | + */ |
54 | +namespace.addPicker = function(config, show_widget_id) { |
55 | + if (Y.UA.ie) { |
56 | + return; |
57 | + } |
58 | + _addPicker(config, show_widget_id); |
59 | + Y.on('lp.widget.connect', function(e) { |
60 | + _addPicker(config, show_widget_id); |
61 | + }); |
62 | +}; |
63 | + |
64 | + |
65 | +/** |
66 | + * Re-wires the picker with the given widget id. Essentially the same as |
67 | + * calling addPicker(config, show_widget_id) with the same parameters as |
68 | + * used originally. |
69 | + * @param show_widget_id the id of the Choose... link |
70 | + */ |
71 | +namespace.reconnectPicker = function(show_widget_id) { |
72 | + Y.fire('lp.widget.connect'); |
73 | +}; |
74 | + |
75 | + |
76 | /* Add a picker widget which will PATCH a given attribute on |
77 | * a given resource. |
78 | * |
79 | @@ -425,7 +493,7 @@ |
80 | } else if (Y.Lang.isFunction(config_validate_callbacks)) { |
81 | validation_callbacks.push(config_validate_callbacks); |
82 | } |
83 | - var val_ns = YUI.namespace('Env.lp.app.picker.validation'); |
84 | + var val_ns = Y.namespace('lp.app.picker.validation'); |
85 | var other_validate_callbacks = val_ns[config.show_widget_id]; |
86 | if (Y.Lang.isArray(other_validate_callbacks)) { |
87 | other_validate_callbacks.forEach(function(callback) { |
88 | |
89 | === modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js' |
90 | --- lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2012-02-03 06:31:31 +0000 |
91 | +++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2012-02-03 06:31:31 +0000 |
92 | @@ -56,7 +56,7 @@ |
93 | this.select_menu = null; |
94 | this.saved_picker_value = null; |
95 | this.validation_namespace = |
96 | - YUI.namespace('Env.lp.app.picker.validation'); |
97 | + Y.namespace('lp.app.picker.validation'); |
98 | }, |
99 | |
100 | tearDown: function() { |
101 | |
102 | === modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt' |
103 | --- lib/lp/app/widgets/templates/form-picker-macros.pt 2012-02-03 06:31:31 +0000 |
104 | +++ lib/lp/app/widgets/templates/form-picker-macros.pt 2012-02-03 06:31:31 +0000 |
105 | @@ -31,36 +31,10 @@ |
106 | <tal:chooseLink replace="structure view/chooseLink" /> |
107 | <script tal:content="structure string: |
108 | LPJS.use('node', 'lp.app.picker', function(Y) { |
109 | - if (Y.UA.ie) { |
110 | - return; |
111 | - } |
112 | - |
113 | - var picker = null; |
114 | var config = ${view/json_config}; |
115 | - var vocabulary = config.vocabulary_name; |
116 | - var vocabulary_filters = config.vocabulary_filters; |
117 | - var input_element = config.input_element; |
118 | - var show_widget_id = config.show_widget_id; |
119 | - var namespace = Y.namespace('lp.app.picker.connect'); |
120 | - namespace[show_widget_id] = function() { |
121 | - // Sort out the Choose... link. |
122 | - var show_widget_node = Y.one('#'+show_widget_id); |
123 | - |
124 | - show_widget_node.set('innerHTML', 'Choose…'); |
125 | - show_widget_node.addClass('js-action'); |
126 | - show_widget_node.get('parentNode').removeClass('unseen'); |
127 | - show_widget_node.on('click', function (e) { |
128 | - if (picker === null) { |
129 | - picker = Y.lp.app.picker.create( |
130 | - vocabulary, config, input_element, |
131 | - vocabulary_filters); |
132 | - } |
133 | - picker.show(); |
134 | - e.preventDefault(); |
135 | - }); |
136 | - }; |
137 | + var show_widget_id = '${view/show_widget_id}'; |
138 | Y.on('domready', function(e) { |
139 | - namespace[show_widget_id](); |
140 | + Y.lp.app.picker.addPicker(config, show_widget_id); |
141 | }); |
142 | }); |
143 | "/> |
144 | |
145 | === modified file 'lib/lp/app/widgets/tests/test_popup.py' |
146 | --- lib/lp/app/widgets/tests/test_popup.py 2012-01-01 02:58:52 +0000 |
147 | +++ lib/lp/app/widgets/tests/test_popup.py 2012-02-03 06:31:31 +0000 |
148 | @@ -83,7 +83,7 @@ |
149 | 'field.test_valid.item', picker_widget.input_id) |
150 | self.assertIsNone(picker_widget.extra_no_results_message) |
151 | markup = picker_widget() |
152 | - self.assertIn("Y.lp.app.picker.create", markup) |
153 | + self.assertIn("Y.lp.app.picker.addPicker", markup) |
154 | self.assertIn('ValidTeamOwner', markup) |
155 | |
156 | def test_widget_filtered_vocabulary(self): |
157 | |
158 | === modified file 'lib/lp/bugs/javascript/bugtask_index.js' |
159 | --- lib/lp/bugs/javascript/bugtask_index.js 2012-02-03 06:31:31 +0000 |
160 | +++ lib/lp/bugs/javascript/bugtask_index.js 2012-02-03 06:31:31 +0000 |
161 | @@ -147,7 +147,8 @@ |
162 | // XXX Abel Deuring 2009-04-23, bug 365462 |
163 | // Y.one('#field.private') returns null. |
164 | // Seems that YUI does not like IDs containing a '.' |
165 | - var field_private = document.getElementById('field.private'); |
166 | + var field_private |
167 | + = document.getElementById('field.private'); |
168 | if (field_private !== null) { |
169 | field_private.focus(); |
170 | } |
171 | @@ -658,16 +659,14 @@ |
172 | if (!Y.Lang.isValue(bugtask_data)) { |
173 | return; |
174 | } |
175 | - var picker_connect = Y.namespace('lp.app.picker.connect'); |
176 | var process_link = function(link) { |
177 | // The link may already have been processed. |
178 | if (link.hasClass('js-action')) { |
179 | return; |
180 | } |
181 | - var func_name = link.get('id'); |
182 | - var connect_func = picker_connect[func_name]; |
183 | - if (Y.Lang.isFunction(connect_func)) { |
184 | - connect_func(); |
185 | + var widget_id = link.get('id'); |
186 | + if (widget_id !== '') { |
187 | + Y.lp.app.picker.reconnectPicker(widget_id); |
188 | } |
189 | }; |
190 | var id; |
191 | |
192 | === modified file 'lib/lp/bugs/javascript/tests/test_bugtask_delete.js' |
193 | --- lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2011-12-19 09:53:25 +0000 |
194 | +++ lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2012-02-03 06:31:31 +0000 |
195 | @@ -1,5 +1,6 @@ |
196 | YUI().use('lp.testing.runner', 'lp.testing.mockio', 'base', 'test', 'console', |
197 | 'node', 'node-event-simulate', 'lp.bugs.bugtask_index', |
198 | + 'lp.app.picker', |
199 | function(Y) { |
200 | |
201 | var suite = new Y.Test.Suite("Bugtask deletion Tests"); |
202 | @@ -117,10 +118,11 @@ |
203 | test_setup_bugtask_table: function() { |
204 | // Test that the bugtask table is wired up, the pickers and the |
205 | // delete links etc. |
206 | - var namespace = Y.namespace('lp.app.picker.connect'); |
207 | var connect_picker_called = false; |
208 | - namespace['show-widget-product'] = function() { |
209 | - connect_picker_called = true; |
210 | + var orig_reconnectPicker = Y.lp.app.picker.reconnectPicker; |
211 | + Y.lp.app.picker.reconnectPicker = function(show_widget_id) { |
212 | + connect_picker_called = show_widget_id === |
213 | + 'show-widget-product'; |
214 | }; |
215 | var orig_confirm_bugtask_delete = module._confirm_bugtask_delete; |
216 | var self = this; |
217 | @@ -136,8 +138,11 @@ |
218 | |
219 | // Test wiring of delete link for row with an associated form. |
220 | this.delete_link.simulate('click'); |
221 | - Y.Assert.isTrue(connect_picker_called); |
222 | - Y.Assert.isTrue(confirm_delete_called); |
223 | + this.wait(function() { |
224 | + // Wait for the events to fire. |
225 | + Y.Assert.isTrue(connect_picker_called); |
226 | + Y.Assert.isTrue(confirm_delete_called); |
227 | + }, 20); |
228 | |
229 | // Test wiring of delete link for row without an associated form. |
230 | confirm_delete_called = false; |
231 | @@ -148,6 +153,7 @@ |
232 | Y.Assert.isFalse(connect_picker_called); |
233 | Y.Assert.isTrue(confirm_delete_called); |
234 | |
235 | + Y.lp.app.picker.reconnectPicker = orig_reconnectPicker; |
236 | module._confirm_bugtask_delete = orig_confirm_bugtask_delete; |
237 | }, |
238 | |
239 | |
240 | === added file 'lib/lp/code/javascript/branchmergeproposal.nominate.js' |
241 | --- lib/lp/code/javascript/branchmergeproposal.nominate.js 1970-01-01 00:00:00 +0000 |
242 | +++ lib/lp/code/javascript/branchmergeproposal.nominate.js 2012-02-03 06:31:31 +0000 |
243 | @@ -0,0 +1,133 @@ |
244 | +/** Copyright (c) 2012, Canonical Ltd. All rights reserved. |
245 | + * |
246 | + * Code for handling the update of the branch merge proposals. |
247 | + * |
248 | + * @module lp.code.branchmergeproposal.nominate |
249 | + */ |
250 | + |
251 | +YUI.add('lp.code.branchmergeproposal.nominate', function(Y) { |
252 | + |
253 | +var namespace = Y.namespace('lp.code.branchmergeproposal.nominate'); |
254 | + |
255 | +var lp_client; |
256 | + |
257 | +/** |
258 | + * Picker validation callback which confirms that the nominated reviewer can |
259 | + * be given visibility to the specified branches. |
260 | + * @param branches_to_check |
261 | + * @param branch_info |
262 | + * @param picker |
263 | + * @param save_fn |
264 | + * @param cancel_fn |
265 | + */ |
266 | +var confirm_reviewer = function(branches_to_check, branch_info, picker, |
267 | + save_fn, cancel_fn) { |
268 | + var visible_branches = branch_info.visible_branches; |
269 | + if (Y.Lang.isArray(visible_branches) |
270 | + && visible_branches.length !== branches_to_check.length) { |
271 | + var invisible_branches = branches_to_check.filter(function(i) { |
272 | + return visible_branches.indexOf(i) < 0; |
273 | + }); |
274 | + var yn_content = Y.lp.mustache.to_html([ |
275 | + "<p class='large-warning' style='padding:12px 2px 0 36px;'>", |
276 | + "{{person_name}} does not currently have permission to ", |
277 | + "view branches:", |
278 | + "<ul style='margin-left: 50px'>", |
279 | + " {{#invisible_branches}}", |
280 | + " <li>{{.}}</li>", |
281 | + " {{/invisible_branches}}", |
282 | + "</ul>", |
283 | + "</p>", |
284 | + "<p>If you proceed, {{person_name}} will be subscribed to the " + |
285 | + "branches.</p>", |
286 | + "<p>Do you really want to nominate them as a reviewer?</p>" |
287 | + ].join(''), { |
288 | + invisible_branches: invisible_branches, |
289 | + person_name: branch_info.person_name |
290 | + }); |
291 | + Y.lp.app.picker.yesno_save_confirmation( |
292 | + picker, yn_content, "Nominate", "Choose Again", |
293 | + save_fn, cancel_fn); |
294 | + } else { |
295 | + if (Y.Lang.isFunction(save_fn)) { |
296 | + save_fn(); |
297 | + } |
298 | + } |
299 | +}; |
300 | + |
301 | +var check_reviewer_can_see_branches = function(picker, value, save_fn, |
302 | + cancel_fn) { |
303 | + if (value === null || !Y.Lang.isValue(value.api_uri)) { |
304 | + if (Y.Lang.isFunction(save_fn)) { |
305 | + save_fn(); |
306 | + return; |
307 | + } |
308 | + } |
309 | + |
310 | + var reviewer_uri = Y.lp.client.normalize_uri(value.api_uri); |
311 | + reviewer_uri = Y.lp.client.get_absolute_uri(reviewer_uri); |
312 | + var error_handler = new Y.lp.client.ErrorHandler(); |
313 | + error_handler.showError = function(error_msg) { |
314 | + picker.set('error', error_msg); |
315 | + }; |
316 | + |
317 | + var branches_to_check = [LP.cache.context.unique_name]; |
318 | + var target_name = Y.DOM.byId('field.target_branch.target_branch').value; |
319 | + if (Y.Lang.trim(target_name) !== '') { |
320 | + branches_to_check.push(target_name); |
321 | + } |
322 | + var confirm = function(branch_info) { |
323 | + namespace.confirm_reviewer( |
324 | + branches_to_check, branch_info, picker, save_fn, cancel_fn); |
325 | + }; |
326 | + var y_config = { |
327 | + on: { |
328 | + success: confirm, |
329 | + failure: error_handler.getFailureHandler() |
330 | + }, |
331 | + parameters: { |
332 | + person: reviewer_uri, |
333 | + branch_names: branches_to_check |
334 | + } |
335 | + }; |
336 | + lp_client.named_get("/branches", "getBranchVisibilityInfo", y_config); |
337 | +}; |
338 | + |
339 | +var setup_reviewer_confirmation = function() { |
340 | + var validation_namespace = Y.namespace('lp.app.picker.validation'); |
341 | + var widget_id = 'show-widget-field-reviewer'; |
342 | + validation_namespace[widget_id]= check_reviewer_can_see_branches; |
343 | +}; |
344 | + |
345 | +// XXX wallyworld 2012-02-03 bug=925818 |
346 | +// We should construct YUI objects and widgets as required and not just |
347 | +// attach stuff to the namespace. |
348 | +// For testing |
349 | +namespace.setup_reviewer_confirmation = setup_reviewer_confirmation; |
350 | +namespace.check_reviewer_can_see_branches = check_reviewer_can_see_branches; |
351 | +namespace.confirm_reviewer = confirm_reviewer; |
352 | + |
353 | +// We want to disable the review_type field if no reviewer is |
354 | +// specified. In such cases, the reviewer will be set by the back end |
355 | +// to be the default for the target branch and the review type will be None. |
356 | +var reviewer_changed = function(value) { |
357 | + var reviewer = Y.Lang.trim(value); |
358 | + var review_type = Y.DOM.byId('field.review_type'); |
359 | + review_type.disabled = (reviewer === ''); |
360 | +}; |
361 | + |
362 | +namespace.setup = function(conf) { |
363 | + lp_client = new Y.lp.client.Launchpad(conf); |
364 | + Y.on('blur', |
365 | + function(e) { |
366 | + reviewer_changed(e.target.get('value')); |
367 | + }, |
368 | + Y.DOM.byId('field.reviewer')); |
369 | + var f = Y.DOM.byId('field.reviewer'); |
370 | + reviewer_changed(f.value); |
371 | + |
372 | + setup_reviewer_confirmation(); |
373 | +}; |
374 | + |
375 | +}, "0.1", {"requires": ["io", "substitute", "dom", "node", |
376 | + "event", "lp.client", "lp.mustache", "lp.app.picker"]}); |
377 | |
378 | === added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html' |
379 | --- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html 1970-01-01 00:00:00 +0000 |
380 | +++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html 2012-02-03 06:31:31 +0000 |
381 | @@ -0,0 +1,51 @@ |
382 | +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> |
383 | +<html> |
384 | + <head> |
385 | + <title>Branch Merge Proposal Nominate</title> |
386 | + |
387 | + <!-- YUI and test setup --> |
388 | + <script type="text/javascript" |
389 | + src="../../../../canonical/launchpad/icing/yui/yui/yui.js"> |
390 | + </script> |
391 | + <link rel="stylesheet" href="../../../app/javascript/testing/test.css" /> |
392 | + <script type="text/javascript" |
393 | + src="../../../app/javascript/testing/testrunner.js"></script> |
394 | + <script type="text/javascript" |
395 | + src="../../../app/javascript/testing/mockio.js"></script> |
396 | + <script type="text/javascript" src="../../../contrib/javascript/lp.mustache.js"></script> |
397 | + <script type="text/javascript" src="../../../app/javascript/client.js"></script> |
398 | + <script type="text/javascript" src="../../../app/javascript/lp.js"></script> |
399 | + <script type="text/javascript" src="../../../app/javascript/activator/activator.js"></script> |
400 | + <script type="text/javascript" src="../../../app/javascript/anim/anim.js"></script> |
401 | + <script type="text/javascript" src="../../../app/javascript/confirmationoverlay/confirmationoverlay.js"></script> |
402 | + <script type="text/javascript" src="../../../app/javascript/choiceedit/choiceedit.js"></script> |
403 | + <script type="text/javascript" src="../../../app/javascript/effects/effects.js"></script> |
404 | + <script type="text/javascript" src="../../../app/javascript/expander.js"></script> |
405 | + <script type="text/javascript" src="../../../app/javascript/extras/extras.js"></script> |
406 | + <script type="text/javascript" src="../../../app/javascript/formoverlay/formoverlay.js"></script> |
407 | + <script type="text/javascript" src="../../../app/javascript/inlineedit/editor.js"></script> |
408 | + <script type="text/javascript" src="../../../app/javascript/lazr/lazr.js"></script> |
409 | + <script type="text/javascript" src="../../../app/javascript/overlay/overlay.js"></script> |
410 | + <script type="text/javascript" src="../../../app/javascript/picker/picker.js"></script> |
411 | + <script type="text/javascript" src="../../../app/javascript/picker/picker_patcher.js"></script> |
412 | + <script type="text/javascript" src="../../../app/javascript/picker/person_picker.js"></script> |
413 | + |
414 | + |
415 | + <!-- The module under test --> |
416 | + <script type="text/javascript" src="../branchmergeproposal.nominate.js"></script> |
417 | + |
418 | + <!-- The test suite --> |
419 | + <script type="text/javascript" src="test_branchmergeproposal.nominate.js"></script> |
420 | + |
421 | +</head> |
422 | +<body class="yui3-skin-sam"> |
423 | + <div id="fixture"></div> |
424 | + <script type="text/x-template" id="form-template"> |
425 | + <form> |
426 | + <input type="text" name="field.target_branch.target_branch" id="field.target_branch.target_branch" value=""/> |
427 | + <input type="text" name="field.reviewer" id="field.reviewer" value=""/> |
428 | + <input type="text" name="field.review_type" id="field.review_type" value="" disabled="disabled"/> |
429 | + </form> |
430 | + </script> |
431 | +</body> |
432 | +</html> |
433 | |
434 | === added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js' |
435 | --- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 1970-01-01 00:00:00 +0000 |
436 | +++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2012-02-03 06:31:31 +0000 |
437 | @@ -0,0 +1,191 @@ |
438 | +/* Copyright 2012 Canonical Ltd. This software is licensed under the |
439 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
440 | + * |
441 | + * Tests for lp.code.branchmergeproposal.nominate. |
442 | + * |
443 | + */ |
444 | + |
445 | +YUI().use('lp.testing.runner', 'test', 'dump', 'console', 'node', |
446 | + 'lp.testing.mockio', 'lp.mustache', 'event', |
447 | + 'node-event-simulate', |
448 | + 'lp.code.branchmergeproposal.nominate', function(Y) { |
449 | + |
450 | +var suite = new Y.Test.Suite("BranchMergeProposal Nominate Tests"); |
451 | +var module = Y.lp.code.branchmergeproposal.nominate; |
452 | + |
453 | +/* |
454 | + * Tests for when a reviewer is nominated for a mp and we check that they |
455 | + * can see the source and target branches. |
456 | + * |
457 | + */ |
458 | + |
459 | +suite.add(new Y.Test.Case({ |
460 | + |
461 | + name: 'branchmergeproposal-nominate-tests', |
462 | + |
463 | + setUp: function() { |
464 | + this.fixture = Y.one('#fixture'); |
465 | + var form = Y.Node.create(Y.one('#form-template').getContent()); |
466 | + this.fixture.appendChild(form); |
467 | + this.mockio = new Y.lp.testing.mockio.MockIo(); |
468 | + window.LP = { |
469 | + links: {me : "/~user"}, |
470 | + cache: { |
471 | + context: { |
472 | + unique_name: 'b2' |
473 | + } |
474 | + } |
475 | + }; |
476 | + module.setup({io_provider: this.mockio}); |
477 | + }, |
478 | + |
479 | + tearDown: function() { |
480 | + if (this.fixture !== null) { |
481 | + this.fixture.empty(); |
482 | + } |
483 | + delete this.fixture; |
484 | + delete this.mockio; |
485 | + delete window.LP; |
486 | + }, |
487 | + |
488 | + // The module setup function works as expected. |
489 | + test_setup: function() { |
490 | + var validation_namespace = |
491 | + Y.namespace('lp.app.picker.validation'); |
492 | + var widget_id = 'show-widget-field-reviewer'; |
493 | + Y.Assert.areEqual( |
494 | + module.check_reviewer_can_see_branches, |
495 | + validation_namespace[widget_id]); |
496 | + var review_type = Y.DOM.byId('field.review_type'); |
497 | + Y.Assert.isTrue(review_type.disabled); |
498 | + }, |
499 | + |
500 | + // The review type field is correctly disabled if there is no reviewer. |
501 | + test_review_type_enable: function() { |
502 | + var reviewer = Y.one("[name='field.reviewer']"); |
503 | + var review_type = Y.one("[name='field.review_type']"); |
504 | + Y.Assert.isTrue( |
505 | + review_type.get('disabled'), |
506 | + 'review type should be disabled'); |
507 | + reviewer.set('value', 'someone'); |
508 | + reviewer.simulate('blur'); |
509 | + Y.Assert.isFalse( |
510 | + review_type.get('disabled'), |
511 | + 'review type should be enabled'); |
512 | + reviewer.set('value', ''); |
513 | + reviewer.simulate('blur'); |
514 | + Y.Assert.isTrue( |
515 | + review_type.get('disabled'), |
516 | + 'review type should now be disabled'); |
517 | + }, |
518 | + |
519 | + // The check_reviewer function works as expected. |
520 | + test_check_reviewer_can_see_branches: function() { |
521 | + var orig_confirm_reviewer = module.confirm_reviewer; |
522 | + var dummy_picker = {}; |
523 | + var confirm_reviewer_called = false; |
524 | + module.confirm_reviewer = function( |
525 | + branches_to_check, branch_info, picker, save_fn, cancel_fn) { |
526 | + Y.Assert.areEqual(dummy_picker, picker); |
527 | + Y.Assert.areEqual('Fred', branch_info.person_name); |
528 | + Y.Assert.areEqual('b2', branch_info.visible_branches[0]); |
529 | + Y.Assert.areEqual('b2', branches_to_check[0]); |
530 | + Y.Assert.areEqual('b1', branches_to_check[1]); |
531 | + confirm_reviewer_called = true; |
532 | + }; |
533 | + var selected_value = { |
534 | + api_uri: '~fred' |
535 | + }; |
536 | + var target_branch_field = |
537 | + Y.DOM.byId('field.target_branch.target_branch'); |
538 | + target_branch_field.value = 'b1'; |
539 | + var save_fn = function() { |
540 | + |
541 | + }; |
542 | + module.check_reviewer_can_see_branches( |
543 | + dummy_picker, selected_value, save_fn); |
544 | + this.mockio.success({ |
545 | + responseText: |
546 | + '{"person_name": "Fred", "visible_branches": ["b2"]}', |
547 | + responseHeaders: {'Content-Type': 'application/json'}}); |
548 | + module.confirm_reviewer = orig_confirm_reviewer; |
549 | + // Check the parameters passed to the io call. |
550 | + Y.Assert.areEqual( |
551 | + '/api/devel/branches', this.mockio.last_request.url); |
552 | + var reviewer_uri = Y.lp.client.normalize_uri( |
553 | + selected_value.api_uri); |
554 | + reviewer_uri = encodeURIComponent( |
555 | + Y.lp.client.get_absolute_uri(reviewer_uri)); |
556 | + Y.Assert.areEqual( |
557 | + 'ws.op=getBranchVisibilityInfo&person=' + |
558 | + reviewer_uri + '&branch_names=b2&branch_names=b1', |
559 | + this.mockio.last_request.config.data); |
560 | + Y.Assert.isTrue(confirm_reviewer_called); |
561 | + }, |
562 | + |
563 | + // Invoke the validation callback with the specified visible branches. |
564 | + // The branches to check is always ['b1', 'b2'] and the person name is |
565 | + // always 'Fred'. We are checking the correct behaviour depending on what |
566 | + // visible branches are passed in. |
567 | + _invoke_confirm_reviewer: function(visible_branches) { |
568 | + var orig_yesyno = Y.lp.app.picker.yesno_save_confirmation; |
569 | + var dummy_picker = {}; |
570 | + var yesno_called = false; |
571 | + Y.lp.app.picker.yesno_save_confirmation = function( |
572 | + picker, content, yes_label, no_label, yes_fn, no_fn) { |
573 | + Y.Assert.areEqual('Nominate', yes_label); |
574 | + Y.Assert.areEqual('Choose Again', no_label); |
575 | + Y.Assert.areEqual(dummy_picker, picker); |
576 | + var message = Y.Node.create(content).get('text'); |
577 | + Y.Assert.isTrue(message.indexOf('Fred') >= 0); |
578 | + var invisible_branches = ['b1', 'b2'].filter(function(i) { |
579 | + return visible_branches.indexOf(i) < 0; |
580 | + }); |
581 | + invisible_branches.forEach(function(branch_name) { |
582 | + Y.Assert.isTrue(message.indexOf(branch_name) > 0); |
583 | + }); |
584 | + yesno_called = true; |
585 | + }; |
586 | + var branch_info = { |
587 | + person_name: 'Fred', |
588 | + visible_branches: visible_branches |
589 | + }; |
590 | + var save_fn_called = false; |
591 | + var save_fn = function() { |
592 | + save_fn_called = true; |
593 | + }; |
594 | + module.confirm_reviewer( |
595 | + ['b1', 'b2'], branch_info, dummy_picker, save_fn); |
596 | + Y.lp.app.picker.yesno_save_confirmation = orig_yesyno; |
597 | + return { |
598 | + save_called: save_fn_called, |
599 | + yesno_called: yesno_called |
600 | + }; |
601 | + }, |
602 | + |
603 | + // Test the validation callback with all branches being visible. |
604 | + test_confirm_reviewer_all_branches_visible: function() { |
605 | + var result = this._invoke_confirm_reviewer(['b1', 'b2']); |
606 | + Y.Assert.isTrue(result.save_called); |
607 | + Y.Assert.isFalse(result.yesno_called); |
608 | + }, |
609 | + |
610 | + // Test the validation callback with no branches being visible. |
611 | + test_confirm_reviewer_no_branches_visible: function() { |
612 | + var result = this._invoke_confirm_reviewer([]); |
613 | + Y.Assert.isFalse(result.save_called); |
614 | + Y.Assert.isTrue(result.yesno_called); |
615 | + }, |
616 | + |
617 | + // Test the validation callback with some branches being visible. |
618 | + test_confirm_reviewer_some_branches_visible: function() { |
619 | + var result = this._invoke_confirm_reviewer(['b1']); |
620 | + Y.Assert.isFalse(result.save_called); |
621 | + Y.Assert.isTrue(result.yesno_called); |
622 | + } |
623 | +})); |
624 | + |
625 | + |
626 | +Y.lp.testing.Runner.run(suite); |
627 | + |
628 | +}); |
629 | |
630 | === modified file 'lib/lp/code/templates/branch-register-merge.pt' |
631 | --- lib/lp/code/templates/branch-register-merge.pt 2012-02-01 15:31:32 +0000 |
632 | +++ lib/lp/code/templates/branch-register-merge.pt 2012-02-03 06:31:31 +0000 |
633 | @@ -58,32 +58,15 @@ |
634 | </metal:formbody> |
635 | </div> |
636 | |
637 | - <!-- We want to disable the review_type field if no reviewer is |
638 | - specified. In such cases, the reviewer will be set by the back end |
639 | - to be the default for the target branch and the review type will be |
640 | - None. |
641 | - --> |
642 | <tal:script> |
643 | - <script type="text/javascript" tal:content="string: |
644 | - LPJS.use('node', 'event', function(Y) { |
645 | - function reviewer_changed(value) { |
646 | - reviewer = Y.Lang.trim(value); |
647 | - review_type = document.getElementById('field.review_type'); |
648 | - review_type.disabled = (reviewer == ''); |
649 | - }; |
650 | - Y.on('blur', |
651 | - function(e) { |
652 | - reviewer_changed(e.target.get('value')) |
653 | - }, |
654 | - document.getElementById('field.reviewer')); |
655 | - Y.on('load', |
656 | - function(e) { |
657 | - f = document.getElementById('field.reviewer') |
658 | - reviewer_changed(f.value); |
659 | + <script type="text/javascript"> |
660 | + LPJS.use('lp.code.branchmergeproposal.nominate', function(Y) { |
661 | + Y.on('domready', |
662 | + function(e) { |
663 | + Y.lp.code.branchmergeproposal.nominate.setup(); |
664 | }, |
665 | window); |
666 | - });" |
667 | - > |
668 | + }); |
669 | </script> |
670 | </tal:script> |
671 |
Ian, thanks for this. I had some trouble initially going through it and so I've got some feedback, much is optional, but I wanted to share and see if any of it is useful to you.
First, just a couple of points. Everything here seems like it should work out ok, so I'm marking approved with some minor notes:
In your tests you setup a LP = {}, but the test write out a window.LP. Then the code uses just LP. If you reference things as window. I'd find it clearer for others to see all occurances using that so that it's explicit vs trusting that LP is found up the scope chain.
In the tests you use var reviewer = Y.one(" [name=' field.reviewer' ]"); where as the rest of the code is all referring to things by the id. Is there a reason you went with an attribute search?
The validation namespace is ENV., but you're using YUI.namespace which the docs seem to indicate is global already? I know it has to match what's already there, so this is more a question at this point than a request for change.
There's enough html generation in there that I think it'd be nice to clean it up using Mustache. You could use it to build the content, place the person name, and loop through the branches.
Now onto the optional stuff. I tend to try to set this up in a YUI object to make it a bit easier to follow and track the various functions defined and used. They're all placed upon the namespace, and I understand that for testing, but then it makes them all potential calling/entry points for code elsewhere.
I've mocked up (completely not run through/tested/etc) a YUI object with a normal YUI initialize and a setup() method to mimic what it appears you've got there. I also added the Mustache templating to kind of show how confirm_reviewer simplifies a little bit. I've tried to indicate "private" methods with _ vs the ones that appear to be externally callable for real use.
https:/ /pastebin. canonical. com/59299/
Consider this just a transfer of technique and take from it what you will. Let me know if you have any questions or concerns. I understand this is hooking into existing picker code.