launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/picker-unassign-link-803996 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #803996 in Launchpad itself: "person picker widget doesn't show link to unassign myself"
  https://bugs.launchpad.net/launchpad/+bug/803996

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/picker-unassign-link-803996/+merge/67710

After using the person picker to perform a search and assign someone to a bug, the "Remove assignee" and "Assign Me" links would disappear. These links are supposed to disappear when a search is done and the results displayed. But they should reappear when the picker is closed after a result is selected.

== Implementation ==

Turned out to be a trivial fix. The picker _clear() function was doing this:
this.set(RESULTS, [{}]);

The results were "cleared" but the array still had a length of 1 . Hence the logic to show/hide the links thought the picker still was displaying results. The logic was changed to
this.set(RESULTS, []);

Also, there were a bunch of duplicate tests in test_personpicker, left over from a previous rollback. These were deleted since they live in test_pickerpatcher.js

== Tests ==

All picker related yui tests pass. A new test was added also for the specific bug: test_buttons_reappear
The picker was tested also by running lp locally.

== Lint ==

jslint: 2 files to lint.
jslint: No problem found in '/home/ian/projects/lp-branches/devel-sandbox/lib/lp/app/javascript/picker/picker.js'.

jslint: No problem found in '/home/ian/projects/lp-branches/devel-sandbox/lib/lp/app/javascript/picker/tests/test_personpicker.js'.

-- 
https://code.launchpad.net/~wallyworld/launchpad/picker-unassign-link-803996/+merge/67710
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/picker-unassign-link-803996 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js	2011-07-06 14:18:59 +0000
+++ lib/lp/app/javascript/picker/picker.js	2011-07-12 14:37:48 +0000
@@ -684,7 +684,7 @@
     _clear: function() {
         this.set(CURRENT_SEARCH_STRING, '');
         this.set(ERROR, '');
-        this.set(RESULTS, [{}]);
+        this.set(RESULTS, []);
         this.set(BATCHES, null);
         this.set(BATCH_COUNT, null);
         this._search_input.set('value', '');

=== modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.js'
--- lib/lp/app/javascript/picker/tests/test_personpicker.js	2011-07-08 06:29:36 +0000
+++ lib/lp/app/javascript/picker/tests/test_personpicker.js	2011-07-12 14:37:48 +0000
@@ -2,11 +2,7 @@
  * 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',
+YUI().use('test', 'console', 'plugin',
            'lazr.picker', 'lazr.person-picker', 'lp.app.picker',
            'node-event-simulate', function(Y) {
 
@@ -123,149 +119,6 @@
                 config);
     },
 
-    _check_button_state: function(btn_class, is_visible) {
-        var assign_me_button = Y.one(btn_class);
-        Assert.isNotNull(assign_me_button);
-        if (is_visible) {
-            Assert.isFalse(
-                assign_me_button.hasClass('yui3-picker-hidden'),
-                btn_class + " should be visible but is hidden");
-        } else {
-            Assert.isTrue(
-                assign_me_button.hasClass('yui3-picker-hidden'),
-                btn_class + " should be hidden but is visible");
-        }
-    },
-
-    _check_assign_me_button_state: function(is_visible) {
-        this._check_button_state('.yui-picker-assign-me-button', is_visible);
-    },
-
-    _check_remove_button_state: function(is_visible) {
-        this._check_button_state('.yui-picker-remove-button', is_visible);
-    },
-
-    test_render_with_links: function () {
-        // The extra buttons section exists when there are assign/remove links
-        this.create_picker(true, true);
-        this.picker.render();
-        this.picker.show();
-
-        Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
-        Y.Assert.isNotUndefined(this.picker.assign_me_button);
-        Y.Assert.isNotUndefined(this.picker.remove_button);
-    },
-
-    test_render_without_links: function () {
-        // The extra buttons section doesn't exists when there no
-        // assign/remove links.
-        this.create_picker(false, false);
-        this.picker.render();
-        this.picker.show();
-
-        Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
-        Y.Assert.isUndefined(this.picker.remove_button);
-        Y.Assert.isUndefined(this.picker.assign_me_button);
-    },
-
-    test_picker_assign_me_button_text: function() {
-        // The assign me button text is correct.
-        this.create_picker(true, true);
-        this.picker.render();
-        var assign_me_button = Y.one('.yui-picker-assign-me-button');
-        Assert.areEqual('Assign Moi', assign_me_button.get('innerHTML'));
-    },
-
-    test_picker_remove_button_text: function() {
-        // The remove button text is correct.
-        this.create_picker(true, true);
-        this.picker.render();
-        var remove_button = Y.one('.yui-picker-remove-button');
-        Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
-    },
-
-    test_picker_has_assign_me_button: function() {
-        // The assign me button is shown.
-        this.create_picker(true, true);
-        this.picker.render();
-        this._check_assign_me_button_state(true);
-    },
-
-    test_picker_no_assign_me_button_unless_configured: function() {
-        // The assign me button is only rendered if show_assign_me_button
-        // config setting is true.
-        this.create_picker(false, true);
-        this.picker.render();
-        Assert.isNull(Y.one('.yui-picker-assign-me-button'));
-    },
-
-    test_picker_no_assign_me_button_if_value_is_me: function() {
-        // The assign me button is not shown if the picker is created for a
-        // field where the value is "me".
-        this.create_picker(true, true, this.ME);
-        this.picker.render();
-        this._check_assign_me_button_state(false);
-    },
-
-    test_picker_assign_me_button_hide_on_save: function() {
-        // The assign me button is shown initially but hidden if the picker
-        // saves a value equal to 'me'.
-        this.create_picker(true, true);
-        this._check_assign_me_button_state(true);
-        this.picker.set('results', this.vocabulary);
-        this.picker.render();
-        simulate(
-            this.picker.get('boundingBox').one('.yui3-picker-results'),
-                'li:nth-child(1)', 'click');
-        this._check_assign_me_button_state(false);
-    },
-
-    test_picker_no_remove_button_if_null_value: function() {
-        // The remove button is not shown if the picker is created for a field
-        // which has a null value.
-        this.create_picker(true, true);
-        this.picker.render();
-        this._check_remove_button_state(false);
-    },
-
-    test_picker_has_remove_button_if_value: function() {
-        // The remove button is shown if the picker is created for a field
-        // which has a value.
-        this.create_picker(true, true, this.ME);
-        this.picker.render();
-        this._check_remove_button_state(true);
-    },
-
-    test_picker_no_remove_button_unless_configured: function() {
-        // The remove button is only rendered if show_remove_button setting is
-        // true.
-        this.create_picker(true, false, this.ME);
-        this.picker.render();
-        Assert.isNull(Y.one('.yui-picker-remove-button'));
-    },
-
-    test_picker_remove_button_clicked: function() {
-        // The remove button is hidden once a picker value has been removed.
-        // And the assign me button is shown.
-        this.create_picker(true, true, this.ME);
-        this.picker.render();
-        var remove = Y.one('.yui-picker-remove-button');
-        remove.simulate('click');
-        this._check_remove_button_state(false);
-        this._check_assign_me_button_state(true);
-    },
-
-    test_picker_assign_me_button_clicked: function() {
-        // The assign me button is hidden once it is clicked.
-        // And the remove button is shown.
-        this.create_picker(true, true);
-        this.picker.render();
-        var remove = Y.one('.yui-picker-assign-me-button');
-        remove.simulate('click');
-        this._check_remove_button_state(true);
-        this._check_assign_me_button_state(false);
-    },
-
     test_search_field_focus: function () {
         // The search field has focus when the picker is shown.
         this.create_picker();
@@ -320,6 +173,19 @@
 
         this._change_results(this.picker, true);
         Y.Assert.isFalse(this.picker._extra_buttons.hasClass('unseen'));
+    },
+
+    test_buttons_reappear: function () {
+        // The assign/remove links are shown again after doing a search and
+        // selecting a result. Doing a search hides the links so we need to
+        // ensure they are made visible again.
+        this.create_picker(true, true);
+        this.picker.render();
+        this._change_results(this.picker, false);
+        Y.Assert.isTrue(this.picker._extra_buttons.hasClass('unseen'));
+        simulate(
+            this.picker.get('boundingBox'), '.yui3-picker-results li', 'click');
+        Y.Assert.isFalse(this.picker._extra_buttons.hasClass('unseen'));
     }
 }));