launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/problems-picking-with-pickers into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/problems-picking-with-pickers/+merge/72601

Summary
=======
Recently the person picker started experiencing problems with the final selection step when picking a person. The initial expectation was either improperly registered events and/or functions listening for the event, causing one click to get everything set up properly, and the second click to actually work.

Further investigation made it seem as though it was a JSON cache based problem, wherein data that the js expected to be in the cache wasn't available on the first click.

The actual problem was a bit more prosaic; the select button is actually a link; the save function didn't have a preventDefault in place, resulting in a race between page refresh on click and the js executing. This branch adds the prevent default.

Pre-implementation notes
========================
Spoke with Curtis Hovey, resulting in the first two theories of what might be going wrong, and finding multiple places that might be suffering from the problem.

Implementation details 
======================
lib/lp/app/javascript/picker/picker.js
--------------------------------------
Added a call to preventdefault when the link is clicked, so the JS executes properly.

Tests
=====
bin/test -vvc --layer=YUI

- or -

firefox lib/lp/app/javascript/tests/test_picker.html

QA
==
Confirm that clicking the select button in an expanded entry in the person picker correctly assigns that person in the change assignee widget on bugtasks.

Launchpad lint
==============

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/picker/picker.js

./lib/lp/app/javascript/picker/picker.js
      70: 'Picker' has not been fully defined yet.

This can be ignored, as it is an artifact of the prototyping.
-- 
https://code.launchpad.net/~jcsackett/launchpad/problems-picking-with-pickers/+merge/72601
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/problems-picking-with-pickers into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js	2011-08-19 06:08:49 +0000
+++ lib/lp/app/javascript/picker/picker.js	2011-08-23 16:41:25 +0000
@@ -524,6 +524,7 @@
             '<a class="sprite yes save" href="#"></a>')
                 .set('text', 'Select ' + data.title));
         links[0].on('click', function (e, value) {
+            e.preventDefault();
             this.fire(SAVE, value);
             }, this, data);
         links.push(this._text_or_link(