Merge lp:~benji/launchpad/bug-809511 into lp:launchpad

[Benji York]

[r=gary][bug=809511] use a heuristic to guess the likely bug ID from the branch name when associating

This branch addressed bug 809511 which is about adding a heuristic to
the "Link a bug report" functionality on branch pages. The heuristic
looks for potential bug numbers in the branch name and if found
pre-fills the bug number field with it and then selects the field
contents so the user can easily remove the number if the guess was
wrong.

Huw and I talked about this in Dublin so I guess that counts as both a
pre-implementation discussion and UI approval.

Tests are in the new test_bugspeclinks.js file and can be run by loading
lib/lp/code/javascript/tests/test_bugspeclinks.html in a browser.

QA should be to visit a branch with a bug number in the name and click
"Link a bug report" and verify that:

- the bug number is provided
- it is selected
- the field has focus
- typing something replaces the guessed ID
- not typing anything and clicking the OK button works

Also, a branch with no ID in its name should be visited and it should be
verified that:

- the field is empty
- the field has focus
- typing something works
- entering a bug ID and clicking the OK button works

After fixing some pre-existing lint the make lint report is clean.

[Gary Poster (gary)]

This is cool, Benji! Thank you for thinking of it and doing it.

Feel free to ignore this bikeshed color, but I suggest that you make the digit limit for a bug number 5, not 6. We do have branches to fix older bugs. Launchpad has three five digit bugs that are still open (https://bugs.launchpad.net/launchpad/+bugs). Ubuntu has one (https://bugs.launchpad.net/ubuntu/+bugs). It's not a big deal, but my suggestion.

Did you consider updating "Y.DOM.byId('field.bug')" to "Y.one('#field.bug")"?

Other than that, looks good. Cool to see the JS tests gradually growing.

review: Approve

[Benji York (benji)]

Thanks for the review.

> I suggest that you make the digit limit for a bug number 5, not 6

Sounds good to me, done.

> Did you consider updating "Y.DOM.byId('field.bug')" to "Y.one('#field.bug")"?

I did, but it didn't work for some reason, so I left it as-is.

=== modified file 'lib/lp/code/javascript/branch.bugspeclinks.js'
--- lib/lp/code/javascript/branch.bugspeclinks.js	2011-06-30 15:20:59 +0000
+++ lib/lp/code/javascript/branch.bugspeclinks.js	2011-08-02 21:14:44 +0000
@@ -18,6 +18,34 @@
 var error_handler;
 
 /*
+ * Extract the best candidate for a bug number from the branch name.
+ */
+function extract_candidate_bug_id(branch_name) {
+    // Extract all the runs of numbers in the branch name and sort by
+    // descending length.
+    var chunks = branch_name.split(/\D/g).sort(function (a, b) {
+        return b.length - a.length;
+    });
+    var chunk, i;
+    for (i=0; i<chunks.length; i++) {
+        chunk = chunks[i];
+        // Bugs with fewer than six digits aren't being created any more (by
+        // Canonical's LP at least), but there are lots of open five digit
+        // bugs so ignore runs of fewer than five digits in the branch name.
+        if (chunk.length < 5) {
+            break;
+        }
+        // Bug IDs don't start with a zero.
+        if (chunk[0] !== '0') {
+            return chunk;
+        }
+    }
+    return null;
+}
+// Expose in the namespace so we can test it.
+namespace._extract_candidate_bug_id = extract_candidate_bug_id;
+
+/*
  * Connect the links to the javascript events.
  */
 namespace.connect_branchlinks = function() {
@@ -50,10 +78,19 @@
     linkbug_handle.on('click', function(e) {
         e.preventDefault();
         link_bug_overlay.show();
-        Y.DOM.byId('field.bug').focus();
+        var field = Y.DOM.byId('field.bug');
+        field.focus();
+        var guessed_bug_id = extract_candidate_bug_id(LP.cache.context.name);
+        if (Y.Lang.isValue(guessed_bug_id)) {
+            field.value = guessed_bug_id;
+            // Select the pre-filled bug number (if any) so that it will be
+            // replaced by anything the user types (getting the guessed bug
+            // number out of the way quickly if we guessed incorrectly).
+            field.selectionStart = 0;
+            field.selectionEnd = 999;
+        }
     });
     connect_remove_links();
-
 };
 
 /*
@@ -123,9 +160,10 @@
         on: {
             success: function(id, response) {
                 destroy_temporary_spinner();
-                Y.one('#linkbug').set(
-                    'innerHTML', 'Link to another bug report');
-                Y.one('#buglink-list').set('innerHTML', response.responseText);
+                Y.one('#linkbug')
+                    .set('innerHTML', 'Link to another bug report');
+                Y.one('#buglink-list')
+                    .set('innerHTML', response.responseText);
                 var new_buglink = Y.one('#buglink-' + bug.get('id'));
                 var anim = Y.lazr.anim.green_flash({node: new_buglink});
                 anim.on('end', connect_remove_links);

=== added file 'lib/lp/code/javascript/tests/test_bugspeclinks.html'
--- lib/lp/code/javascript/tests/test_bugspeclinks.html	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_bugspeclinks.html	2011-08-02 21:14:44 +0000
@@ -0,0 +1,28 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+
+<html>
+  <head>
+  <title>Test page for bug link functionality.</title>
+
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/testrunner.js"></script>
+
+  <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+  <script type="text/javascript"
+    src="../../../app/javascript/overlay/overlay.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../branch.bugspeclinks.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_bugspeclinks.js"></script>
+  </head>
+
+<body class="yui3-skin-sam">
+</body>
+</html>

=== added file 'lib/lp/code/javascript/tests/test_bugspeclinks.js'
--- lib/lp/code/javascript/tests/test_bugspeclinks.js	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_bugspeclinks.js	2011-08-02 21:14:44 +0000
@@ -0,0 +1,84 @@
+/* Copyright 2011 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for lp.code.branch.bugspeclinks.
+ *
+ */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw', combine: false
+    }).use('test', 'console', 'node-event-simulate',
+        'lp.code.branch.bugspeclinks', function(Y) {
+
+    var module = Y.lp.code.branch.bugspeclinks;
+    var extract_candidate_bug_id = module._extract_candidate_bug_id;
+    var suite = new Y.Test.Suite("lp.code.branch.bugspeclinks Tests");
+
+    suite.add(new Y.Test.Case({
+        name: 'Test bug ID guessing',
+
+        test_no_bug_id_present: function() {
+            // If nothing that looks like a bug ID is present, null is
+            // returned.
+            Y.Assert.isNull(extract_candidate_bug_id('no-id-here'));
+        },
+
+        test_short_digit_rund_ignored: function() {
+            Y.Assert.isNull(extract_candidate_bug_id('foo-1234-bar'));
+        },
+
+        test_leading_zeros_disqualify_potential_ids: function() {
+            // Since bug IDs can't start with zeros, any string of numbers
+            // with a leading zero are not considered as a potential ID.
+            Y.Assert.isNull(extract_candidate_bug_id('foo-0123456-bar'));
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('foo-0123456-999999-bar'), '999999');
+        },
+
+        test_five_digit_bug_ids_are_extracted: function() {
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('foo-12345-bar'), '12345');
+        },
+
+        test_six_digit_bug_ids_are_extracted: function() {
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('foo-123456-bar'), '123456');
+        },
+
+        test_seven_digit_bug_ids_are_extracted: function() {
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('foo-1234567-bar'), '1234567');
+        },
+
+        test_eight_digit_bug_ids_are_extracted: function() {
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('foo-12345678-bar'), '12345678');
+        },
+
+        test_longest_potential_id_is_extracted: function() {
+            // Since there may be numbers other than a bug ID in a branch
+            // name, we want to extract the longest string of digits.
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('bug-123456-take-2'), '123456');
+            Y.Assert.areEqual(
+                extract_candidate_bug_id('123456-1234567'), '1234567');
+        }
+
+        }));
+
+    var handle_complete = function(data) {
+        window.status = '::::' + JSON.stringify(data);
+        };
+    Y.Test.Runner.on('complete', handle_complete);
+    Y.Test.Runner.add(suite);
+
+    var console = new Y.Console({newestOnTop: false});
+    console.render('#log');
+
+    // Start the test runner on Y.after to ensure all setup has had a
+    // chance to complete.
+    Y.after('domready', function() {
+        Y.Test.Runner.run();
+    });
+});