Merge into devel : picker-wiring-812255 : Code : Launchpad itself

Status:

Merged

Approved by:

Curtis Hovey on 2011-07-26

Approved revision:

no longer in the source branch.

Merged at revision:

13532

Proposed branch:

lp:~wallyworld/launchpad/picker-wiring-812255

Merge into:

lp:launchpad

Diff against target:

0 lines

To merge this branch:

bzr merge lp:~wallyworld/launchpad/picker-wiring-812255

Related bugs:

Bug #812255: Person picker can show "Pick me" when it shouldn't	High	Fix Released
Bug #812257: Person picker sometimes shows "Remove person" instead of "Remove team"	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	code	2011-07-21	Approve on 2011-07-26
Review via email: mp+68629@code.launchpad.net

Commit message

[r=sinzui][bug=812255,812257] Fix person picker "Assign me" and "Remove" link behaviour and add test coverage.

Description of the change

This branch fixes a few picker bugs as well as performing as significant refactor and adding extra yui test coverage for person pickers.
Bugs 812255 and 812257 were the main focus but work was also done to implement a better fix for bug 798778 (which was previously closed).
A previously implemented hack to control the creation of the new PersonPicker widget was also removed.

== Implementation ==

There are 2 ways of instantiating a picker:

Y.lp.app.picker.addPickerPatcher(...)
Y.lp.app.picker.create(...)

The first way is used to create a picker invoked from the little yellow edit icon; the second way creates a picker from the Choose... link.

The root cause of the bugs was that code existed in the addPickerPatcher() method to wire up the "Assign me" and "Remove" links. This code was different from that used by the create() method and so if a PersonPicker was instantiated using create(), things weren't wired up properly. Besides all that, the code in question implements functionality which really does belong in the PersonPicker implementation.

The main work involved:
- moving the code for the "Assign Me" and "Remove" links to the PersonPicker
- add a picker_type property to the picker view classes to allow the Javascript hack to be removed
- add a new "selected_value" attribute to the core Picker widget
- control the rendering of the "Assign me" and "Remove" links based on this new selected_value attribute
- use an onChange event handler for the new "selected_value" attribute to update the links when the user makes a new selection using the picker
- delete unnecessary and improperly placed code by removing config for the "Assign me" and "Remove" links from the core picker implementation

The use of the selected_value attribute removes the need to "screen scrape" to read the picker value from the associated form widget and is a much cleaner implementation.

Bug 798778 was previously fixed by hiding the "Assign me" and "Remove" links when a search was done. This was not the best approach, in part because once a search was done, the "Remove" link was made invisible and could *never* be accessed again without a page refresh. The solution in this branch is to move the "Assign me" and "Remove" links above the search box - this separates them from the search results as requested in the bug and avoids the issues with the current solution.

When the picker is created using the Choose... link, the "Remove" link is hidden. It adds little value in this case since all that needs to be done is to clear the associated text field, and having the "Remove" link is not clear as to exactly what it does in this case.

== Tests ==

Relevant tests for the "Assign me" and "Remove links" were moved from test_picker_patcher.js to test_personpicker.js. The test coverage inside test_personpicker.js was extended to cover both types of person picker instantiation. This was done by defining common tests and using Y.merge() to mix in the relevant picker setup. This approach ensures consistent test coverage between the picker types without cut and paste code.

YUI tests run:
  test_picker.js
  test_personpicker.js
  test_pickerpatcher.js

In the absence of windmill tests, bug task assigning, branch reviewer editing, and recipe owner editing were tested by hand - these use cases cover the different application scenarios where a person picker is used.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/app/browser/lazrjs.py
  lib/lp/app/javascript/picker/person_picker.js
  lib/lp/app/javascript/picker/picker.js
  lib/lp/app/javascript/picker/picker_patcher.js
  lib/lp/app/javascript/picker/tests/test_personpicker.js
  lib/lp/app/javascript/picker/tests/test_picker.js
  lib/lp/app/javascript/picker/tests/test_picker_patcher.js
  lib/lp/app/widgets/popup.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/javascript/bugtask_index.js

./lib/lp/app/javascript/picker/person_picker.js
      13: 'PersonPicker' has not been fully defined yet.
      81: Expected '!==' and instead saw '!='.
      83: Expected '===' and instead saw '=='.
      84: Expected '===' and instead saw '=='.
./lib/lp/app/javascript/picker/picker.js
      67: 'Picker' has not been fully defined yet.
     258: Move 'var' declarations to the top of the function.
     258: Stopping. (24% scanned).
      -1: JSLINT had a fatal error.
     598: Line exceeds 78 characters.
./lib/lp/app/javascript/picker/picker_patcher.js
      70: Expected '!==' and instead saw '!='.
     355: Expected '!==' and instead saw '!='.
./lib/lp/app/javascript/picker/tests/test_personpicker.js
     344: Expected '!==' and instead saw '!='.
./lib/lp/app/javascript/picker/tests/test_picker.js
     196: Move 'var' declarations to the top of the function.
     196: Stopping. (20% scanned).
      -1: JSLINT had a fatal error.
      49: Line exceeds 78 characters.
     465: Line exceeds 78 characters.
     516: Line exceeds 78 characters.
     960: Line exceeds 78 characters.
./lib/lp/bugs/javascript/bugtask_index.js
     839: Expected '===' and instead saw '=='.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2011-07-21:

#

Download full text (6.0 KiB)

I take lint issues seriously. I fixed a few null-undefined issues that were masked by !=/==. Lint also failed to complete the check of some files because it hates the definition of a var in a for-loop.

> === modified file 'lib/lp/app/browser/lazrjs.py'
> --- lib/lp/app/browser/lazrjs.py 2011-07-18 00:45:28 +0000
> +++ lib/lp/app/browser/lazrjs.py 2011-07-21 05:12:40 +0000
...
> @@ -283,20 +275,35 @@
...
> @property
> + def selected_value(self):
> + """ String representation of field value associated with the picker.
> +
> + Default implementation is to return the 'name' attribute.
> + """
> + val = getattr(self.context, self.exported_field.__name__)
> + if val is not None and hasattr(val, 'name'):

Use safe_hasattr which will honour keyboard and system interrupts.
from canonical.lazr.utils import safe_hasattr

=== modified file 'lib/lp/app/javascript/picker/person_picker.js'
--- lib/lp/app/javascript/picker/person_picker.js 2011-07-19 03:27:34 +0000
+++ lib/lp/app/javascript/picker/person_picker.js 2011-07-21 05:12:40 +0000

> @@ -59,13 +58,50 @@
> this.constructor.superclass.show.call(this);
> },
>
> + _update_button_text: function() {
> + var link_text = this._remove_person_text;
> + if (this.get('selected_value_metadata') === 'team') {
> + link_text = this._remove_team_text;
> + }
> + this.remove_button.set('text', link_text);
> + },
> +
> + _show_hide_buttons: function () {
> + var selected_value = this.get('selected_value');
> + if (this.remove_button) {
> + if (selected_value === null) {
> + this.remove_button.addClass('yui3-picker-hidden');
> + } else {
> + this.remove_button.removeClass('yui3-picker-hidden');
> + this._update_button_text();
> + }
> + }
> +
> + if (this.assign_me_button) {
> + if ((selected_value != null

This looks like one of the lint issue reported. We do not use coercive
operators. You many want to write this as
if ((selected_value

> + && LP.links.me.match('~'+selected_value + "$")
> + == '~'+selected_value)

use space between the operators.

> === modified file 'lib/lp/app/javascript/picker/picker.js'
> --- lib/lp/app/javascript/picker/picker.js 2011-07-19 15:16:37 +0000
> +++ lib/lp/app/javascript/picker/picker.js 2011-07-21 05:12:40 +0000
...
> @@ -1016,16 +1041,24 @@
> this.doAfter('save', function (e) {
> var result = e.details[Picker.SAVE_RESULT];
> this.get('host').set(SELECTED_VALUE_METADATA, result.metadata);
> - input.set("value", result.value);
> + this.get('host').set(SELECTED_VALUE, result.value);
> + if (result.value != null) {

Do you mean null? Can this be undefined? I think link reported this issue.
since I think we want a value below, may Y.Lang.isValue is better?

> + input.set("value", result.value);
> + } else {
> + input.set("value", '');
> + }

We do not see this form very often in Lp code, but it avoids ...

I take lint issues seriously. I fixed a few null-undefined issues that were masked by !=/==. Lint also failed to complete the check of some files because it hates the definition of a var in a for-loop.

> === modified file 'lib/lp/app/browser/lazrjs.py'
> --- lib/lp/app/browser/lazrjs.py	2011-07-18 00:45:28 +0000
> +++ lib/lp/app/browser/lazrjs.py	2011-07-21 05:12:40 +0000
...
> @@ -283,20 +275,35 @@
...
>      @property
> +    def selected_value(self):
> +        """ String representation of field value associated with the picker.
> +
> +        Default implementation is to return the 'name' attribute.
> +        """
> +        val = getattr(self.context, self.exported_field.__name__)
> +        if val is not None and hasattr(val, 'name'):

Use safe_hasattr which will honour keyboard and system interrupts.
from canonical.lazr.utils import safe_hasattr

=== modified file 'lib/lp/app/javascript/picker/person_picker.js'
--- lib/lp/app/javascript/picker/person_picker.js	2011-07-19 03:27:34 +0000
+++ lib/lp/app/javascript/picker/person_picker.js	2011-07-21 05:12:40 +0000

> @@ -59,13 +58,50 @@
>          this.constructor.superclass.show.call(this);
>      },
>  
> +    _update_button_text: function() {
> +        var link_text = this._remove_person_text;
> +        if (this.get('selected_value_metadata') === 'team') {
> +            link_text = this._remove_team_text;
> +        }
> +        this.remove_button.set('text', link_text);
> +    },
> +
> +    _show_hide_buttons: function () {
> +        var selected_value = this.get('selected_value');
> +        if (this.remove_button) {
> +            if (selected_value === null) {
> +                this.remove_button.addClass('yui3-picker-hidden');
> +            } else {
> +                this.remove_button.removeClass('yui3-picker-hidden');
> +                this._update_button_text();
> +            }
> +        }
> +
> +        if (this.assign_me_button) {
> +            if ((selected_value != null

This looks like one of the lint issue reported. We do not use coercive
operators. You many want to write this as
            if ((selected_value

> +                && LP.links.me.match('~'+selected_value + "$")
> +                    == '~'+selected_value)

use space between the operators.

> === modified file 'lib/lp/app/javascript/picker/picker.js'
> --- lib/lp/app/javascript/picker/picker.js	2011-07-19 15:16:37 +0000
> +++ lib/lp/app/javascript/picker/picker.js	2011-07-21 05:12:40 +0000
...
> @@ -1016,16 +1041,24 @@
>          this.doAfter('save', function (e) {
>              var result = e.details[Picker.SAVE_RESULT];
>              this.get('host').set(SELECTED_VALUE_METADATA, result.metadata);
> -            input.set("value",  result.value);
> +            this.get('host').set(SELECTED_VALUE, result.value);
> +            if (result.value != null) {

Do you mean null? Can this be undefined? I think link reported this issue.
since I think we want a value below, may Y.Lang.isValue is better?

> +                input.set("value",  result.value);
> +            } else {
> +                input.set("value", '');
> +            }

We do not see this form very often in Lp code, but it avoids the if-else
issue above.
            input.set("value", result.value || '');

> === modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
> --- lib/lp/app/javascript/picker/picker_patcher.js	2011-07-19 03:27:34 +0000
> +++ lib/lp/app/javascript/picker/picker_patcher.js	2011-07-21 05:12:40 +0000
...
@@ -79,82 +62,31 @@
...
>      var save = function (picker_result) {
>          activator.renderProcessing();
>          var success_handler = function (entry) {
> +          var to_render = null_display_value;
> +          var selected_value = null;
> +          if (entry.get(attribute_name) != null) {

I think lint expected !==

> === modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.js'
> --- lib/lp/app/javascript/picker/tests/test_personpicker.js	2011-07-15 02:20:36 +0000
> +++ lib/lp/app/javascript/picker/tests/test_personpicker.js	2011-07-21 05:12:40 +0000
...
> @@ -73,7 +75,10 @@
>                  // our setup assumes success, so we just do the success
>                  // callback.
>                  var entry_repr = {
> -                  'lp_html': {'test_link': '<a href="/~me">Content</a>'}
> +                  'test_link': representation.test_link,
> +                  'lp_html': {
> +                      'test_link': '<a href="'
> +                                    +representation.test_link+'">Content</a>'}

I misread this at first. I was confused by the missing spaces between the
operator and the identifier.

> === modified file 'lib/lp/app/widgets/popup.py'
> --- lib/lp/app/widgets/popup.py	2011-07-19 03:27:34 +0000
> +++ lib/lp/app/widgets/popup.py	2011-07-21 05:12:40 +0000
> @@ -105,6 +105,17 @@
>                           class="%(cssClass)s" />""" % d
>  
>      @property
> +    def selected_value(self):
> +        """ String representation of field value associated with the picker.
> +
> +        Default implementation is to return the 'name' attribute.
> +        """
> +        val = self._getFormValue()
> +        if val is not None and hasattr(val, 'name'):

Use safe_hasattr which will honour keyboard and system interrupts.
from canonical.lazr.utils import safe_hasattr

> === modified file 'lib/lp/bugs/javascript/bugtask_index.js'
> --- lib/lp/bugs/javascript/bugtask_index.js	2011-07-19 14:42:52 +0000
> +++ lib/lp/bugs/javascript/bugtask_index.js	2011-07-21 05:12:40 +0000
> @@ -862,8 +862,11 @@
>          // an assignee. We check to see if an assignee is a contributor and if
>          // they are not, the user is asked to confirm their selection.
>          var validate_assignee = function(picker, value, save_fn, cancel_fn) {
> -            if (value === null) {
> -                return true;
> +            if (value === null || value.api_uri == null) {

Lint did not like this. maybe
            if (value === null || value.api_uri) {
or
            if (value === null || Y.Lang.isValue(value.api_uri) {

review: Needs Fixing (code)

Revision history for this message

Ian Booth (wallyworld) wrote on 2011-07-22:

#

Thanks for the detailed review. I've addressed the lint issues and other suggestions.

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/app/browser/lazrjs.py
  lib/lp/app/javascript/picker/person_picker.js
  lib/lp/app/javascript/picker/picker.js
  lib/lp/app/javascript/picker/picker_patcher.js
  lib/lp/app/javascript/picker/tests/test_personpicker.js
  lib/lp/app/javascript/picker/tests/test_picker.js
  lib/lp/app/javascript/picker/tests/test_picker_patcher.js
  lib/lp/app/widgets/popup.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/widgets/bugtask.py
  lib/lp/bugs/javascript/bugtask_index.js

./lib/lp/app/javascript/picker/person_picker.js
13: 'PersonPicker' has not been fully defined yet.
./lib/lp/app/javascript/picker/picker.js
67: 'Picker' has not been fully defined yet.

I tried the /*jslint widget: true */ option and it did not remove the above lint "issues".

Revision history for this message

Curtis Hovey (sinzui) wrote on 2011-07-26:

#

Thank you for taking the time to clean up the work.

review: Approve (code)

Launchpad itself

Merge lp:~wallyworld/launchpad/picker-wiring-812255 into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers