Merge lp:~salgado/launchpad/bug-482235 into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

Change our make_picker() function to use the new
Y.lazr.TextFieldPickerPlugin plugin so that the picker's default value
is taken from the input field associated with it.

Also updates to a newer lazr-js, containing just one extra revision with
the fix for bug 482235.

== Tests ==

./bin/test test_person_picker

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  versions.cfg
  lib/canonical/widgets/templates/vocabulary-picker.js
  lib/canonical/launchpad/javascript/lp/picker.js
  lib/canonical/launchpad/javascript/bugs/bugtask-index.js
  lib/canonical/launchpad/javascript/code/codereview.js
  lib/lp/registry/windmill/tests/test_person_picker.py

== JSLint notices ==
jslint: No problem found in '/home/salgado/devel/launchpad/bug-482235/lib/canonical/launchpad/javascript/bugs/bugtask-index.js'.

jslint: No problem found in '/home/salgado/devel/launchpad/bug-482235/lib/canonical/launchpad/javascript/code/codereview.js'.

jslint: No problem found in '/home/salgado/devel/launchpad/bug-482235/lib/canonical/launchpad/javascript/lp/picker.js'.

jslint: Lint found in '/home/salgado/devel/launchpad/bug-482235/lib/canonical/widgets/templates/vocabulary-picker.js':
Line 8 character 16: Expected an identifier and instead saw '%'.
    var args = %s;

Line 8 character 16: Stopping, unable to continue. (14% scanned).

The lint error above is expected because that file is actually a
template used from within python code.

[Māris Fogels (mars)]

Hi Salgado,

Your changes look good, and thanks for updating lazr-js.

I see one coding convention that needs fixing, and had some small questions about implementation details.

• line 62: By convention, the optional 'save' function should be a key in the config dictionary. Only the simplest of functions use optional positional args in JavaScript, as getting around them is a pain (unlike in Python).
• line 78: Might be better to use Y.Lang.isFunction()
• line 116: Does YUI understand field IDs with dots in them yet? If it does, then the input_element string construction can be greatly simplified using a '#'
• line 163: What is the 'shadow' element? Some invisible element in the DOM? Just asking to make sure that 'shadow' is a behavioural object, not a presentation object :)
• line 170: Why do you have a hard sleep here?
• Regarding the JSLint error: should we rename that file to .js.pt or .js.template, as it isn't a valid JavaScript file?

Maris

review: Needs Fixing

[Guilherme Salgado (salgado)]

On Mon, 2010-01-18 at 20:22 +0000, Māris Fogels wrote:
> Review: Needs Fixing
> Hi Salgado,
>
> Your changes look good, and thanks for updating lazr-js.

Thanks for the review. In the future, would you mind including the diff
in your review and adding your comments around the code you're referring
to? That makes the conversation a lot smoother as we don't have to keep
going back to the diff for context.

>
> I see one coding convention that needs fixing, and had some small
> questions about implementation details.
>
> • line 62: By convention, the optional 'save' function should be a key
> in the config dictionary. Only the simplest of functions use optional
> positional args in JavaScript, as getting around them is a pain
> (unlike in Python).

While I was doing this change I noticed that both Y.lp.picker.create and
Y.lp.picker.addPickerPatcher treat the config argument as optional
(although it's not documented). I don't see a reason for that and all
their callsites pass in a config dictionary, as expected. Would it be
OK for me to change both methods to make the config required?

> • line 78: Might be better to use Y.Lang.isFunction()

Sure, I can do that.

> • line 116: Does YUI understand field IDs with dots in them yet? If
> it does, then the input_element string construction can be greatly
> simplified using a '#'

It doesn't yet, I tried. :/

> • line 163: What is the 'shadow' element? Some invisible element in
> the DOM? Just asking to make sure that 'shadow' is a behavioural
> object, not a presentation object :)

I *think* it's the shadow around the overlay; I've used it because it
was one of the few elements in the overlay that had an ID. But maybe I
didn't look carefully for other elements with an ID there.

> • line 170: Why do you have a hard sleep here?

Because that triggers a search, which might take a while to complete.

I think I read somewhere that the assertProperty() call will also wait
for the element to show up, in which case the sleep() would be
unnecessary. Is that why you asked?

> • Regarding the JSLint error: should we rename that file to .js.pt
> or .js.template, as it isn't a valid JavaScript file?
>

Yeah, .js.template sounds good to me.

--
Guilherme Salgado <email address hidden>

[Māris Fogels (mars)]

On Tue, Jan 19, 2010 at 7:15 AM, Guilherme Salgado <email address hidden> wrote:
> On Mon, 2010-01-18 at 20:22 +0000, Māris Fogels wrote:
>> Review: Needs Fixing
>> Hi Salgado,
>>
>> Your changes look good, and thanks for updating lazr-js.
>
> Thanks for the review.  In the future, would you mind including the diff
> in your review and adding your comments around the code you're referring
> to?  That makes the conversation a lot smoother as we don't have to keep
> going back to the diff for context.
>

Ok. For JS reviews I find in-context diffs don't work, as I often spot problems in multiple places, with multiple problems per line. As a result, I note them like this:

  line 23,45,67: foo is missing
  line 23: bar should be baz

>>
>> I see one coding convention that needs fixing, and had some small
>> questions about implementation details.
>>
>> • line 62: By convention, the optional 'save' function should be a key
>> in the config dictionary.  Only the simplest of functions use optional
>> positional args in JavaScript, as getting around them is a pain
>> (unlike in Python).
>
> While I was doing this change I noticed that both Y.lp.picker.create and
> Y.lp.picker.addPickerPatcher treat the config argument as optional
> (although it's not documented).  I don't see a reason for that and all
> their callsites pass in a config dictionary, as expected.  Would it be
> OK for me to change both methods to make the config required?

Sometimes the config only contains optional settings, so you could obviously mark the entire config as an optional argument. However, for some objects, like the Y.Anim() object, the config contains required settings, and is *not* optional.

In this case, I leave the judgement up to you.

>> • line 163: What is the 'shadow' element?  Some invisible element in
>> the DOM?  Just asking to make sure that 'shadow' is a behavioural
>> object, not a presentation object :)
>
> I *think* it's the shadow around the overlay; I've used it because it
> was one of the few elements in the overlay that had an ID. But maybe I
> didn't look carefully for other elements with an ID there.

You might want to ask Tom how the bugs team tests for overlay presence and absence.

There are CSS classes associated with overlay state you may be able to use instead: yui-mywidgetname-hidden and yui-mywidget-disabled (also yui-mywidget-focused)

http://developer.yahoo.com/yui/3/widget/#CSS

>
>> • line 170: Why do you have a hard sleep here?
>
> Because that triggers a search, which might take a while to complete.
>
> I think I read somewhere that the assertProperty() call will also wait
> for the element to show up, in which case the sleep() would be
> unnecessary. Is that why you asked?
>

Nope, it is just habit to question a hard sleep like that. Usually you wait for an element in the DOM to show up to signal that the search is done.

Ok, this code looks good to me. Change what you desire, and r=mars.

Thanks,
Maris

[Guilherme Salgado (salgado)]

On Tue, 2010-01-19 at 15:12 +0000, Māris Fogels wrote:
>
> On Tue, Jan 19, 2010 at 7:15 AM, Guilherme Salgado <email address hidden> wrote:
> > On Mon, 2010-01-18 at 20:22 +0000, Māris Fogels wrote:
> >> Review: Needs Fixing
> >> Hi Salgado,
> >>
> >> Your changes look good, and thanks for updating lazr-js.
> >
> > Thanks for the review. In the future, would you mind including the diff
> > in your review and adding your comments around the code you're referring
> > to? That makes the conversation a lot smoother as we don't have to keep
> > going back to the diff for context.
> >
>
> Ok. For JS reviews I find in-context diffs don't work, as I often

I don't think JS reviews are intrinsically different from, say, python
reviews.

> spot problems in multiple places, with multiple problems per line.
> As a result, I note them like this:
>
> line 23,45,67: foo is missing
> line 23: bar should be baz

When doing reviews I often spot problems that apply to multiple lines as
well, and in those cases I just copy-n-paste the comment or make a
reference to it. But it'd also be fine to have it at the top of the
message, I think, as long as the diff is included.

The fact that the diff is not included here means we have to switch back
and forth between the browser and the email client, which is a bit
disturbing.

>
> >>
> >> I see one coding convention that needs fixing, and had some small
> >> questions about implementation details.
> >>
> >> • line 62: By convention, the optional 'save' function should be a key
> >> in the config dictionary. Only the simplest of functions use optional
> >> positional args in JavaScript, as getting around them is a pain
> >> (unlike in Python).
> >
> > While I was doing this change I noticed that both Y.lp.picker.create and
> > Y.lp.picker.addPickerPatcher treat the config argument as optional
> > (although it's not documented). I don't see a reason for that and all
> > their callsites pass in a config dictionary, as expected. Would it be
> > OK for me to change both methods to make the config required?
>
> Sometimes the config only contains optional settings, so you could obviously mark the entire config as an optional argument. However, for some objects, like the Y.Anim() object, the config contains required settings, and is *not* optional.
>
> In this case, I leave the judgement up to you.

I changed the docs to state that the config is optional.

>
>
> >> • line 163: What is the 'shadow' element? Some invisible element in
> >> the DOM? Just asking to make sure that 'shadow' is a behavioural
> >> object, not a presentation object :)
> >
> > I *think* it's the shadow around the overlay; I've used it because it
> > was one of the few elements in the overlay that had an ID. But maybe I
> > didn't look carefully for other elements with an ID there.
>
> You might want to ask Tom how the bugs team tests for overlay presence
> and absence.
>
> There are CSS classes associated with overlay state you may be able to
> use instead: yui-mywidgetname-hidden and yui-mywidget-disabled (also
> yui-mywidget-focused)

They use that on huge conditional xpaths, which look rather ugly and
hard to maintain -- that...

[Māris Fogels (mars)]

Hi Salgado,

Your changes look good. The optional argument looks much cleaner. Thanks! r=mars

Maris

review: Approve

=== modified file 'Makefile'
--- Makefile	2010-01-22 06:24:01 +0000
+++ Makefile	2010-02-01 14:05:26 +0000
@@ -357,9 +357,6 @@
 reload-apache: enable-apache-launchpad
 	/etc/init.d/apache2 restart
 
-static:
-	$(PY) scripts/make-static.py
-
 TAGS: compile
 	# emacs tags
 	bin/tags -e

=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2010-01-21 21:54:12 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2010-02-01 14:05:26 +0000
@@ -251,10 +251,10 @@
         picker_activator: '.menu-link-addsubscriber'
     };
 
-    var picker = Y.lp.picker.create(
-        'ValidPersonOrTeam',
-        function(result) { subscribe_someone_else(result, subscription); },
-        config);
+    config.save = function(result) {
+        subscribe_someone_else(result, subscription);
+    };
+    var picker = Y.lp.picker.create('ValidPersonOrTeam', config);
 }
 
 /*
@@ -606,8 +606,8 @@
             picker_activator: '.menu-link-addbranch'
         };
 
-        var picker = Y.lp.picker.create(
-            'Branch', get_branch_and_link_to_bug, config);
+        config.save = get_branch_and_link_to_bug;
+        var picker = Y.lp.picker.create('Branch', config);
     }
 }
 

=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js	2010-01-15 04:34:57 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js	2010-02-01 14:05:26 +0000
@@ -139,13 +139,11 @@
         step_title: 'Search'
     };
 
-    reviewer_picker = Y.lp.picker.create(
-        'ValidPersonOrTeam',
-        function(result) {
-            var review_type = Y.one("[id=field.review_type]").get('value');
-            request_reviewer(result, review_type);
-        },
-        config);
+    config.save = function(result) {
+        var review_type = Y.one("[id=field.review_type]").get('value');
+        request_reviewer(result, review_type);
+    };
+    reviewer_picker = Y.lp.picker.create('ValidPersonOrTeam', config);
     reviewer_picker.set('footer_slot', Y.Node.create([
         '<div>',
         '<div style="float: left; padding-right: 9px;">',

=== modified file 'lib/canonical/launchpad/javascript/lp/picker.js'
--- lib/canonical/launchpad/javascript/lp/picker.js	2009-12-09 15:07:39 +0000
+++ lib/canonical/launchpad/javascript/lp/picker.js	2010-02-01 14:05:26 +0000
@@ -150,7 +150,8 @@
         });
     };
 
-    var picker = Y.lp.picker.create(vocabulary, save, config);
+    config.save = save;
+    var picker = Y.lp.picker.create(vocabulary, config);
     picker._resource_uri = resource_uri;
     var extra_buttons = Y.Node.create(
         '<div style="text-align: center; height: 3em; ' +
@@ -192,17 +193,18 @@
   * @requires dom, dump, lazr.overlay, lazr.picker
   * @method create
   * @param {String} vocabulary Name of the vocabulary to query.
-  * @param {Function} save Function which takes a single string argument.
-  * @param {Object} config Object literal of config name/value pairs.
+  * @param {Object} config Optional Object literal of config name/value pairs.
   *                        config.header is a line of text at the top of
   *                        the widget.
   *                        config.step_title overrides the subtitle.
-  *                        description strings.
+  *                        config.save is a Function (optional) which takes
+  *                        a single string argument.
   */
-Y.lp.picker.create = function (vocabulary, save, config) {
+Y.lp.picker.create = function (vocabulary, config) {
     if (Y.UA.ie) {
         return;
     }
+
     if (config !== undefined) {
         var header = 'Choose an item.';
         if (config.header !== undefined) {
@@ -233,13 +235,13 @@
         zIndex: 1000,
         visible: false
         });
-    var picker = new Y.Picker(new_config);
+    var picker = new Y.lazr.Picker(new_config);
 
     picker.subscribe('save', function (e) {
         Y.log('Got save event.');
-        // Y.one() uses CSS3 selectors which don't work with ids containing
-        // a period, so we have to use Y.DOM.byId().
-        save(e.details[Y.Picker.SAVE_RESULT]);
+        if (Y.Lang.isFunction(config.save)) {
+            config.save(e.details[Y.lazr.Picker.SAVE_RESULT]);
+        }
     });
 
     picker.subscribe('cancel', function (e) {

=== modified file 'lib/canonical/launchpad/javascript/registry/team.js'
--- lib/canonical/launchpad/javascript/registry/team.js	2009-12-22 20:30:39 +0000
+++ lib/canonical/launchpad/javascript/registry/team.js	2010-02-01 14:05:26 +0000
@@ -25,7 +25,8 @@
         picker_activator: '.menu-link-add_member'
     };
 
-    Y.lp.picker.create('ValidTeamMember', _add_member, config);
+    config.save = _add_member;
+    Y.lp.picker.create('ValidTeamMember', config);
 };
 
 var _add_member = function(selected_person) {

=== modified file 'lib/canonical/launchpad/windmill/testing/lpuser.py'
--- lib/canonical/launchpad/windmill/testing/lpuser.py	2009-10-27 13:01:29 +0000
+++ lib/canonical/launchpad/windmill/testing/lpuser.py	2010-02-01 14:05:26 +0000
@@ -19,6 +19,8 @@
 
     def ensure_login(self, client):
         """Ensure that this user is logged on the page under windmill."""
+        # Refresh the page to make sure we're not fooled by a stale one.
+        client.refresh()
         client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
         result = client.asserts.assertNode(
             name=u'loginpage_submit_login', assertion=False)

=== modified file 'lib/canonical/widgets/popup.py'
--- lib/canonical/widgets/popup.py	2009-07-30 17:58:16 +0000
+++ lib/canonical/widgets/popup.py	2010-02-01 14:05:26 +0000
@@ -127,7 +127,7 @@
 
     def chooseLink(self):
         js_file = os.path.join(os.path.dirname(__file__),
-                               'templates/vocabulary-picker.js')
+                               'templates/vocabulary-picker.js.template')
         js_template = open(js_file).read()
 
         if self.header is None:

=== renamed file 'lib/canonical/widgets/templates/vocabulary-picker.js' => 'lib/canonical/widgets/templates/vocabulary-picker.js.template'
--- lib/canonical/widgets/templates/vocabulary-picker.js	2009-09-21 12:27:14 +0000
+++ lib/canonical/widgets/templates/vocabulary-picker.js.template	2010-02-01 14:05:26 +0000
@@ -1,4 +1,4 @@
-YUI().use('node', 'lp.picker', function(Y) {
+YUI().use('node', 'lp.picker', 'plugin', function(Y) {
     if (Y.UA.ie) {
         return;
     }
@@ -9,15 +9,12 @@
     // The vocabulary picker, created when used for the first time.
     var picker = null;
     function make_picker() {
-        var save = function (result) {
-            Y.DOM.byId(args.input_id).value = result.value;
-        };
         var config = {
             header: args.header,
             step_title: args.step_title,
             extra_no_results_message: args.extra_no_results_message
         };
-        var picker = Y.lp.picker.create(args.vocabulary, save, config);
+        var picker = Y.lp.picker.create(args.vocabulary, config);
         if (config.extra_no_results_message !== null) {
             picker.before('resultsChange', function (e) {
                 var new_results = e.details[0].newVal;
@@ -30,6 +27,8 @@
                 }
             });
         }
+        picker.plug(Y.lazr.TextFieldPickerPlugin,
+                    {input_element: '[id="' + args.input_id + '"]'});
         return picker;
     }
 

=== added file 'lib/lp/registry/windmill/tests/test_person_picker.py'
--- lib/lp/registry/windmill/tests/test_person_picker.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/windmill/tests/test_person_picker.py	2010-02-01 14:05:26 +0000
@@ -0,0 +1,55 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for using a person picker widget."""
+
+__metaclass__ = type
+__all__ = []
+
+import unittest
+
+from windmill.authoring import WindmillTestClient
+
+from canonical.launchpad.windmill.testing import constants, lpuser
+
+from lp.registry.windmill.testing import RegistryWindmillLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TesPersonPickerWidget(TestCaseWithFactory):
+
+    layer = RegistryWindmillLayer
+
+    def setUp(self):
+        super(TesPersonPickerWidget, self).setUp()
+        self.client = WindmillTestClient('PersonPickerWidget')
+
+    def test_person_picker_widget(self):
+        client = self.client
+        lpuser.SAMPLE_PERSON.ensure_login(client)
+
+        client.open(url=u'http://launchpad.dev:8085/people/+requestmerge')
+        client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        client.waits.forElement(id=u'show-widget-field-dupe_person',
+                                timeout=constants.FOR_ELEMENT)
+
+        client.type(text=u'guilherme', name=u'field.dupe_person')
+
+        client.click(id=u'show-widget-field-dupe_person')
+        client.waits.forElement(id=u'shadow', timeout=constants.FOR_ELEMENT)
+
+        client.asserts.assertProperty(
+            xpath=u'//div[@class="yui-picker-search-box"]/input',
+            validator=u'value|guilherme')
+
+        client.click(xpath=u'//div[@class="yui-picker-search-box"]/button')
+        client.waits.sleep(milliseconds=constants.SLEEP)
+
+        client.click(xpath=u'//ul[@class="yui-picker-results"]/li[1]')
+        client.asserts.assertProperty(
+            xpath=u'//input[@name="field.dupe_person"]',
+            validator='value|salgado')
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'versions.cfg'
--- versions.cfg	2010-01-22 04:01:17 +0000
+++ versions.cfg	2010-02-01 14:05:26 +0000
@@ -37,7 +37,7 @@
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1
 lazr.uri = 1.0.2
-lazr-js = 0.9.2DEV
+lazr-js = 0.9.2
 martian = 0.11
 mechanize = 0.1.11
 mercurial = 1.3.1