launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/personpicker-remove-assignee-link-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #800654 in Launchpad itself: "Assign Me link on person picker visible when it should be hidden"
  https://bugs.launchpad.net/launchpad/+bug/800654

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/personpicker-remove-assignee-link-2/+merge/65487

A fix was done for bug 798775 which hid the "Remove Me" button when the field value was empty. The fix involved correcting the css class name used to hide the button. The same fix was done for the "Assign Me" button. However, a separate, unrelated logic error in the if statement used to determine if the "Assign Me" should be hidden meant that the "Assign Me" button is always visible

The tests written for the last fix were also flawed and have been fixed here also.

== Implementation ==

Use the correct check to see if the "Assign Me" button should be hidden.

== Tests ==

Fix the test_picker.js tests.
-- 
https://code.launchpad.net/~wallyworld/launchpad/personpicker-remove-assignee-link-2/+merge/65487
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/personpicker-remove-assignee-link-2 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js	2011-06-21 15:00:03 +0000
+++ lib/lp/app/javascript/picker.js	2011-06-22 12:28:30 +0000
@@ -97,7 +97,7 @@
 
         if (assign_me_button) {
             if (link !== null
-                && link.get('href').indexOf(LP.links.me + '/') !== -1) {
+                && link.get('href').match(LP.links.me+"$")==LP.links.me) {
                 assign_me_button.addClass('yui3-picker-hidden');
             } else {
                 assign_me_button.removeClass('yui3-picker-hidden');

=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js	2011-06-21 15:00:03 +0000
+++ lib/lp/app/javascript/tests/test_picker.js	2011-06-22 12:28:30 +0000
@@ -242,13 +242,13 @@
                 "description": "description-" + i,
                 "api_uri": "~/fred-" + i};
         }
+        this.ME = '/~me';
         window.LP = {
             links: {
-                me: '/~me'
+                me: this.ME
             },
             cache: {}
         };
-        this.ME_HREF = '/~me/';
 
         // We patch Launchpad client to return some fake data for the patch
         // operation.
@@ -258,7 +258,7 @@
                 // our setup assumes success, so we just do the success
                 // callback.
                 var entry_repr = {
-                  'lp_html': {'test_link': '<a href="/~me/">Content</a>'}
+                  'lp_html': {'test_link': '<a href="/~me">Content</a>'}
                 };
                 var result = new Y.lp.client.Entry(
                     null, entry_repr, "a_self_link");
@@ -335,7 +335,7 @@
     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_HREF);
+        this.create_picker(true, true, this.ME);
         this.picker.render();
         this._check_assign_me_button_state(false);
     },
@@ -364,7 +364,7 @@
     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_HREF);
+        this.create_picker(true, true, this.ME);
         this.picker.render();
         this._check_remove_button_state(true);
     },
@@ -372,7 +372,7 @@
     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_HREF);
+        this.create_picker(true, false, this.ME);
         this.picker.render();
         Assert.isNull(Y.one('.yui-picker-remove-button'));
     },
@@ -380,7 +380,7 @@
     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_HREF);
+        this.create_picker(true, true, this.ME);
         this.picker.render();
         var remove = Y.one('.yui-picker-remove-button');
         remove.simulate('click');