Merge lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 into lp:launchpad

Proposed by Ian Booth
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
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,rharding][bug=330290] If a nominated reviewer cannot see a mp source or target branch, warn the user the reviewer will be subscribed to the invisible branches.

Description of the change

== Implementation ==

Basically just a bunch of new javascript in module 'lp.code.branchmergeproposal.nominate'. I also moved some javascript from the branch-register-merge.pt TAL to the new module.

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://people.canonical.com/~ianb/reviewer-confirm.png

== Tests ==

Add some new yui tests for the new javascript module:

test_branchmergeproposal_nominate.js
test_branchmergeproposal_nominate.html

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/javascript/branchmergeproposal.nominate.js
  lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html
  lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js
  lib/lp/code/templates/branch-register-merge.pt

To post a comment you must log in.
Revision history for this message
Richard Harding (rharding) wrote :

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.

review: Approve (code*)
Revision history for this message
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_can_see_branches?

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.

review: Approve
Revision history for this message
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("[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?
>

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://pastebin.canonical.com/59299/
>

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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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&hellip;');
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