launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-809511 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #809511 in Launchpad itself: "Pre-populate guessed bug number in link-bug-to-branch dialog."
  https://bugs.launchpad.net/launchpad/+bug/809511

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-809511/+merge/70219

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.

-- 
https://code.launchpad.net/~benji/launchpad/bug-809511/+merge/70219
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-809511 into lp:launchpad.

=== 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 20:36:41 +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 6 digits aren't being created any more (by
+        // Canonical's LP at least), so ignore runs of fewer than 6 digits in
+        // the branch name.
+        if (chunk.length < 6) {
+            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 20:36:41 +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 20:36:41 +0000
@@ -0,0 +1,82 @@
+/* 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_bug_ids_ignored: function() {
+            // Since (the Canonical instance of) Launchpad is generating six
+            // digit bug IDs at the time of this writing, strings of numbers
+            // shorter than that are ignored.
+            Y.Assert.isNull(extract_candidate_bug_id('foo-12345-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_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();
+    });
+});