launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #787595 in Launchpad itself: "person picker could have a link to choose myself when I am a valid choice"
  https://bugs.launchpad.net/launchpad/+bug/787595

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/oh-oh-pick-me-pick-me/+merge/63047

Summary
=======
As part of the work for disclosure, a better personpicker is needed that users can trust. The current personpicker is actually just the regular picker, passed a person-specific vocabulary. Because the personpicker will have it's own UI attributes (e.g. "Assign me" and "Remove assignee" buttons like the bug assign pickerPatcher), we need a PersonPicker YUI widget.  

Preimplementation
=================
Spoke with Deryck Hodge about creating a new widget, and Curtis Hovey.

Implementation
==============
lib/lp/app/widgets/popup.py
---------------------------
Drive by whitespace fixes.

lib/lp/registry/javascript/personpicker.js
------------------------------------------
Created PersonPicker, which extends Y.lazr.Picker. It creates an assign_me and a remove_asignee button, as those are UI elements we want when using the person picker, and wires them up to provide the users name or to clear the entry as appropriate.

lib/lp/registry/javascript/tests/test_personpicker.html
lib/lp/registry/javascript/tests/test_personpicker.js
-----------------------------------------------------
Tests.

Tests
=====
firefox lib/lp/registry/javascript/tests/test_personpicker.html

QA
==
None. This is a new widget not yet wired into any of the UI.

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/popup.py
  lib/lp/registry/javascript/personpicker.js
  lib/lp/registry/javascript/tests/test_personpicker.html
  lib/lp/registry/javascript/tests/test_personpicker.js

-- 
https://code.launchpad.net/~jcsackett/launchpad/oh-oh-pick-me-pick-me/+merge/63047
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me into lp:launchpad.

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-05-29 23:34:26 +0000
+++ lib/lp/app/widgets/popup.py	2011-05-31 22:01:19 +0000
@@ -156,6 +156,7 @@
 
 
 class PersonPickerWidget(VocabularyPickerWidget):
+
     include_create_team_link = False
 
     def chooseLink(self):
@@ -169,6 +170,8 @@
     def nonajax_uri(self):
         return '/people/'
 
+        
+
 
 class BugTrackerPickerWidget(VocabularyPickerWidget):
 

=== added file 'lib/lp/registry/javascript/personpicker.js'
--- lib/lp/registry/javascript/personpicker.js	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/personpicker.js	2011-05-31 22:01:19 +0000
@@ -0,0 +1,74 @@
+/* Copyright 2011 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * @namespace Y.lp.registry.personpicker
+ * @requires lazr.picker
+ */
+YUI.add('lp.registry.personpicker', function(Y) {
+var namespace = Y.namespace('lp.registry.personpicker');
+
+var footer_label = ".yui3-picker-footer-slot"
+
+var PersonPicker = function() {
+    PersonPicker.superclass.constructor.apply(this, arguments);
+
+    this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
+};
+
+Y.extend(PersonPicker, Y.lazr.Picker, {
+    hide: function() {
+        this.get('boundingBox').setStyle('display', 'none');
+    },
+
+    show: function() {
+        this.get('boundingBox').setStyle('display', 'block');
+    },
+
+    remove: function () {
+        this.fire('save', {value: ''});
+    },
+
+    assign_me: function () {
+        name = LP.links.me.replace('/~', '');
+        this.fire('save', {value: name});
+    },
+
+    renderUI: function() {
+        this.constructor.superclass.renderUI.call(this);
+        var remove_button, assign_me_button;
+        //# TODO config should set extrabuttons
+        var show_remove_button = true;
+        var show_assign_me_button = true;
+        var remove_button_text = "Remove assignee";
+        var assign_me_button_text = "Assign me";
+        if (show_remove_button) {
+            remove_button = Y.Node.create(
+                '<a class="yui-picker-remove-button bg-image" ' +
+                'href="javascript:void(0)" ' +
+                'style="background-image: url(/@@/remove); padding-right: 1em">' +
+                remove_button_text + '</a>');
+            remove_button.on('click', this.remove, this);
+            this._extra_buttons.appendChild(remove_button);
+        }
+        if (show_assign_me_button) {
+            assign_me_button = Y.Node.create(
+                '<a class="yui-picker-assign-me-button bg-image" ' +
+                'href="javascript:void(0)" ' +
+                'style="background-image: url(/@@/person)">' +
+                assign_me_button_text + '</a>');
+            assign_me_button.on('click', this.assign_me, this);
+            this._extra_buttons.appendChild(assign_me_button);
+        }
+    },
+
+    syncUI: function() {
+        // call Picker's sync
+        this.constructor.superclass.syncUI.call(this);
+        footer_slot = Y.one(footer_label);
+        footer_slot.appendChild(this._extra_buttons);
+    }
+});
+PersonPicker.NAME = 'person-picker';
+namespace.PersonPicker = PersonPicker
+
+}, "0.1", {"requires": ['lazr.picker']});

=== added file 'lib/lp/registry/javascript/tests/test_personpicker.html'
--- lib/lp/registry/javascript/tests/test_personpicker.html	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/tests/test_personpicker.html	2011-05-31 22:01:19 +0000
@@ -0,0 +1,40 @@
+<html>
+  <head>
+    <title>Launchpad PersonPicker</title>
+    <!-- YUI 3.0 Setup -->
+    <script type="text/javascript" src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+    <script type="text/javascript"
+      src="../../../../canonical/launchpad/icing/lazr/build/lazr.js"></script>
+    <link rel="stylesheet"
+      href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+    <link rel="stylesheet"
+      href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+    <link rel="stylesheet"
+      href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+    <link rel="stylesheet"
+      href="../../../../canonical/launchpad/javascript/test.css" />
+    <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+    <script type="text/javascript" src="../../../app/javascript/lp.js"></script>
+
+    <!-- The module under test -->
+    <script type="text/javascript" src="../personpicker.js"></script>
+
+    <!-- The test suite -->
+    <script type="text/javascript" src="test_personpicker.js"></script>
+  </head>
+  <body class="yui-skin-sam">
+
+    <!-- The example markup required by the script to run -->
+    <input type="text" value="" id="choose"
+                         name="choose" size="20"
+                         maxlength=""
+                         onKeyPress="" style=""
+                         class="" /> 
+    <span class="">
+        (<a id="show-widget" href="/people/">Find&hellip;</a>)
+    </span>
+
+    <!-- The test output -->
+    <div id="log"></div>
+  </body>
+</html>

=== added file 'lib/lp/registry/javascript/tests/test_personpicker.js'
--- lib/lp/registry/javascript/tests/test_personpicker.js	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/tests/test_personpicker.js	2011-05-31 22:01:19 +0000
@@ -0,0 +1,66 @@
+/* Copyright 2011 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw', combine: false,
+    fetchCSS: false
+    }).use('test', 'console', 'plugin', 'lp.registry.personpicker',
+           'node-event-simulate', function(Y) {
+
+    var suite = new Y.Test.Suite("lp.registry.personpicker Tests");
+
+    suite.add(new Y.Test.Case({
+        name: 'personpicker',
+
+        setUp: function() {
+            window.LP = {
+                links: {me: '/~no-one'},
+                cache: {}
+            }
+        },
+
+        test_render: function () {
+            var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+            personpicker.render();
+            personpicker.show();
+            
+            // The extra buttons section exists
+            Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
+            Y.Assert.isNotNull(Y.one('.yui-picker-remove-button'));
+            Y.Assert.isNotNull(Y.one('.yui-picker-assign-me-button'));
+        },
+        
+        test_buttons: function () {
+            var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+            personpicker.render();
+            personpicker.show();
+           
+            // Patch the picker so the assign_me and remove methods can be 
+            // tested.
+            var data = null;
+            personpicker.on('save', function (result) {
+                data = result.value;
+            });
+            var remove = Y.one('.yui-picker-remove-button');
+            remove.simulate('click');
+            Y.Assert.areEqual('', data);
+
+            var assign_me = Y.one('.yui-picker-assign-me-button');
+            assign_me.simulate('click');
+            Y.Assert.areEqual('no-one', data);
+        }
+    }));
+
+    // Lock, stock, and two smoking barrels.
+    Y.Test.Runner.add(suite);
+
+    var console = new Y.Console({newestOnTop: false});
+    console.render('#log');
+
+    Y.on('domready', function() {
+        Y.Test.Runner.run();
+    });
+});
+