Merge lp:~wallyworld/launchpad/fix-enable-review-type into lp:launchpad

[Ian Booth]

= Summary =

As per bug 671665, when the popup selection widget is used to choose the reviewer for a mp, the review type field does not become enabled.

= Implementation =

There are 2 parts to the fix.
1. Enhance lazr-js so that focus is returned to the input field when the user finishes selecting the value from the popup
2. Change the mechanism to trigger the review_type field enable update from onchange to onblur

The enabled processing will occur when the user tabs out of the reviewer field, and will work in the same way whether the reviewer value is typed or entered using the popup.

This branch should be merged only once lazr-js is upgraded. The relevant branch there is lp:~wallyworld/lazr-js/popup-selection-focus-fix

= Tests =

The code.windmill.test_branchmergeproposal_review.py test was enhanced to the that the correct behaviour occurred when using the popup selector.

bin/test -vvt test_branchmergeproposal_review

= Lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/templates/branch-register-merge.pt
  lib/lp/code/windmill/tests/test_branchmergeproposal_review.py

[Ian Booth (wallyworld)]

The merge proposal for lazr-js branch lp:~wallyworld/lazr-js/popup-selection-focus-fix has now been approved

[Māris Fogels (mars)]

Hi Ian,

This change looks good.

I noticed one spot that may be an issue: is a JavaScript execution step triggered by an event after client.keyPress(), but before client.assertProperty()? (Is this where the blur event occurs?) If so, you may need to insert a programmatic wait into the test at that point to give time for the JS to execute, and then for the browser to update the DOM (assertProperty() might already wait for you; the documentation would say for sure).

If the test does not wait, then you accidentally introduce a race condition between the browser and the testrunner, similar to the ones we have been seeing in buildbot. It is best to try and catch such races now. They are hard to track down after landing.

With that consideration, r=mars

review: Approve

[Ian Booth (wallyworld)]

Hi Maris

Thanks for the pointer about the possible race condition. You are right
in that the keyPress() call is to send a tab key press to cause the blur
event. I've added a small 500ms wait.

On 09/11/10 23:42, Māris Fogels wrote:
> Review: Approve
> Hi Ian,
>
> This change looks good.
>
> I noticed one spot that may be an issue: is a JavaScript execution step triggered by an event after client.keyPress(), but before client.assertProperty()? (Is this where the blur event occurs?) If so, you may need to insert a programmatic wait into the test at that point to give time for the JS to execute, and then for the browser to update the DOM (assertProperty() might already wait for you; the documentation would say for sure).
>
> If the test does not wait, then you accidentally introduce a race condition between the browser and the testrunner, similar to the ones we have been seeing in buildbot. It is best to try and catch such races now. They are hard to track down after landing.
>
> With that consideration, r=mars

[Ian Booth (wallyworld)]

I resubmitted the mp so that it is targeted to land in devel rather than db-devel

=== modified file 'lib/lp/code/templates/branch-register-merge.pt'
--- lib/lp/code/templates/branch-register-merge.pt	2010-10-01 05:34:25 +0000
+++ lib/lp/code/templates/branch-register-merge.pt	2010-11-18 01:30:34 +0000
@@ -69,7 +69,7 @@
                 review_type = document.getElementById('field.review_type');
                 review_type.disabled = (reviewer == '');
               };
-              Y.on('change',
+              Y.on('blur',
                   function(e) {
                     reviewer_changed(e.target.get('value'))
                   },

=== modified file 'lib/lp/code/windmill/tests/test_branchmergeproposal_review.py'
--- lib/lp/code/windmill/tests/test_branchmergeproposal_review.py	2010-11-04 19:59:02 +0000
+++ lib/lp/code/windmill/tests/test_branchmergeproposal_review.py	2010-11-18 01:30:34 +0000
@@ -57,15 +57,26 @@
         # reviewer field is empty works.
         client.asserts.assertProperty(
             id=u"field.review_type", validator='disabled|true')
+        # User types into reviewer field manually.
         client.type(text=u'mark', id=u'field.reviewer')
         client.asserts.assertProperty(
             id=u"field.review_type", validator='disabled|false')
         client.type(text=u'', id=u'field.reviewer')
         client.asserts.assertProperty(
             id=u"field.review_type", validator='disabled|true')
+        # User selects reviewer using popup selector widget.
+        client.click(id=u'show-widget-field-reviewer')
+        search_and_select_picker_widget(client, u'name12', 1)
+        # Tab out of the field.
+        client.keyPress(
+            options='\\9,true,false,false,false,false',
+            id=u'field.reviewer')
+        # Give javascript event handler time to run
+        client.waits.sleep(milliseconds=unicode(500))
+        client.asserts.assertProperty(
+            id=u"field.review_type", validator='disabled|false')
 
         client.click(id=u'field.actions.register')
-
         client.waits.forPageLoad(timeout=u'10000')
         client.click(id=u'request-review')