Merge lp:~sinzui/launchpad/bug-tag-completions into lp:launchpad

[Curtis Hovey]

Enable bug tag completions on +filebug and advanced bug search.

Recent refactorings of the bug tag entry code make it possible to
reuse the autocompletion part on +filebug and advanced bug search.

--------------------------------------------------------------------

RULES

    Pre-implementation: jcsackett
    * Extract the autocompletion from setup_tag_entry() so that
      other use cases can reuse it when the tag input field already
      exists.
    * Update +filebug to provide available_official_tags.
    * setup autocomplete in the template.
    * Update advanced search to provide available_official_tags.
    * setup autocomplete in the template.

    Addendum
    * I tried to extend the Autocomplete widget, but I discovered that
      I would also need to write a lot of duplicate css rules. The
      subclass effectively renames all the presentation rules :(
    * I tried a plugin, but it did not generate the markup before
      the widget was initialised.
    * I settled on a simple setup function.

QA

    * Visit https://bugs.qastaging.launchpad.net/gdp/+bugs?advanced
    * Verify the bug tags input will complete tags.
    * Choose Report a bug.
    * Enter "testing", Continue, expand Extra options.
    * Verify the bug tags input will complete tags.

LINT

    lib/lp/app/javascript/autocomplete/autocomplete.js
    lib/lp/app/javascript/autocomplete/tests/test_autocomplete.js
    lib/lp/bugs/browser/widgets/bug.py
    lib/lp/bugs/browser/widgets/tests/test_bug.py
    lib/lp/bugs/javascript/bug_tags_entry.js
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.html
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.js
    lib/lp/bugs/templates/bugtask-macros-tableview.pt

TEST

    ./bin/test -vvc lp.bugs.browser.widgets.tests.test_bug
    ./bin/test -vvc --layer=YUITest

IMPLEMENTATION

I turn of the browser's autocomplete when the widget is run. The bug tag
completions were hidden under Chromium's completions. The browser must
be turned off for every input we add completions to.
    lib/lp/app/javascript/autocomplete/autocomplete.js
    lib/lp/app/javascript/autocomplete/tests/test_autocomplete.js

I extracted the setup the autocomplete widget to a separate function
that could be called without the editor. I removed the "on('queryChange'"
because I discovered it did nothing when I wrote a test for it...it
was probably used to debug key input when the widget was created.
    lib/lp/bugs/javascript/bug_tags_entry.js
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.html
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.js

I Updated the BugTagsWidget to include the script to progressively
enhance any bug tag input to support autocomplete. This widget is used
both on advanced search and on +filebug. I moved the help link into the
widget to ensure it is always rendered and that it is rendered
consistently. I discovered that both +filebug and advanced bug had
duplicate ids in their crafted forms. A simple selector was not enough;
the selector must find the bug tag input field that is type="text".
    lib/lp/bugs/browser/widgets/bug.py
    lib/lp/bugs/browser/widgets/tests/test_bug.py
    lib/lp/bugs/templates/bugtask-macros-tableview.pt

[j.c.sackett (jcsackett)]

Curtis--

This looks good. Just one style note.

In a few places (line 226 or 311, for example): It looks like you're doing
pythonic brace/paren closure in places. I think for javascript we have the
closing brace at the same tab level as the opening call, rather than at the
tab level of the enclosed lines.

In other words, I believe we prefer:

  some_call(function () {
    stuff;
  });

Rather than:

  some_call(function () {
    stuff;
    });

review: Approve

[Richard Harding (rharding)]

Looks good. In addition to the alignment of the closing braces JC brought up, I've got a couple other suggestions.

As we discussed in IRC, widgets have a CSS_PREFIX value you can set to manually override the generated CSS prefix based on NAME. This might let you continue the idea of extending the Autocomplete widget.

http://yuilibrary.com/yui/docs/api/files/widget_js_Widget.js.html#l95

Small naming question/suggestion. Now that we're starting to properly build and namespace our JS, I'm trying to see if I can start to simplify naming. So could the module just be Y.lp.bugs.tags_entry [sans bug_]?

#197 is the autocomplete declared var?

#294/300/306/307 please surround attribute selectors in quotes: id="field.tag"

review: Approve

[Curtis Hovey (sinzui)]

Hi. Jon, Rick.

I outdented the close of the methods calls that contain anonymous functions

I removed the redundant "bug_" from the namespace.

autocomplete is a private namespace var declared on line 24 of the module. The module has lots of hidden, untestible parts because of the structure of the entire module.

I quoted the attribute value in the selectors. I was inconsistent.

I will play with the widget and CSS in another branch.

=== modified file 'lib/lp/app/javascript/autocomplete/autocomplete.js'
--- lib/lp/app/javascript/autocomplete/autocomplete.js	2012-05-18 22:56:13 +0000
+++ lib/lp/app/javascript/autocomplete/autocomplete.js	2012-06-29 04:38:24 +0000
@@ -198,6 +198,8 @@
             'left': iregion.left   + 'px',
             'top':  iregion.bottom + 'px'
         });
+        // Disable the browser autocomplete so that it does not conflict.
+        input.setAttribute('autocomplete', 'off');
     },
 
     /**

=== modified file 'lib/lp/app/javascript/autocomplete/tests/test_autocomplete.js'
--- lib/lp/app/javascript/autocomplete/tests/test_autocomplete.js	2012-04-06 17:28:25 +0000
+++ lib/lp/app/javascript/autocomplete/tests/test_autocomplete.js	2012-06-29 04:38:24 +0000
@@ -329,13 +329,13 @@
 
         /* A helper function to determine if two match result items are equal */
         matches_are_equal: function(a, b) {
-            if (typeof a == 'undefined') {
+            if (Y.Lang.isUndefined(a)) {
                 Assert.fail("Match set 'a' is of type 'undefined'!");
             }
-            if (typeof b == 'undefined') {
+            if (Y.Lang.isUndefined(b)) {
                 Assert.fail("Match set 'b' is of type 'undefined'!");
             }
-            return (a.text == b.text) && (a.offset == b.offset);
+            return (a.text === b.text) && (a.offset === b.offset);
         },
 
         test_no_matches_returns_an_empty_array: function() {

=== modified file 'lib/lp/bugs/browser/widgets/bug.py'
--- lib/lp/bugs/browser/widgets/bug.py	2011-02-24 15:30:54 +0000
+++ lib/lp/bugs/browser/widgets/bug.py	2012-06-29 04:38:24 +0000
@@ -10,7 +10,7 @@
     ]
 
 import re
-
+from simplejson import dumps
 from zope.app.form.browser.textwidgets import (
     IntWidget,
     TextAreaWidget,
@@ -26,6 +26,8 @@
 from lp.app.errors import NotFoundError
 from lp.app.validators import LaunchpadValidationError
 from lp.bugs.interfaces.bug import IBugSet
+from lp.registry.interfaces.distribution import IDistribution
+from lp.registry.interfaces.product import IProduct
 
 
 class BugWidget(IntWidget):
@@ -135,6 +137,37 @@
     def _getInputValue(self):
         return TextWidget.getInputValue(self)
 
+    def __call__(self):
+        """Return the input with a script."""
+        input_markup = super(BugTagsWidget, self).__call__()
+        script_markup = """
+            <a href="/+help-bugs/tag-search.html"
+               class="sprite maybe action-icon"
+               target="help">Tag search help</a>
+            <script type="text/javascript">
+            LPJS.use('event', 'lp.bugs.tags_entry', function(Y) {
+                %s
+                Y.on('domready', function(e) {
+                     Y.lp.bugs.tags_entry.setup_tag_complete(
+                         'input[id="field.%s"][type="text"]', official_tags);
+                     });
+                });
+            </script>
+            """ % (self.official_tags_js, self.context.__name__)
+        return input_markup + script_markup
+
+    @property
+    def official_tags_js(self):
+        """Return the JavaScript of bug tags used by the bug tag completer."""
+        bug_target = self.context.context
+        pillar_target = (
+            IProduct(bug_target, None) or IDistribution(bug_target, None))
+        if pillar_target is not None:
+            official_tags = list(pillar_target.official_bug_tags)
+        else:
+            official_tags = []
+        return 'var official_tags = %s;' % dumps(official_tags)
+
 
 class BugTagsFrozenSetWidget(BugTagsWidget):
     """A widget for editing bug tags in a `FrozenSet` field."""

=== added file 'lib/lp/bugs/browser/widgets/tests/test_bug.py'
--- lib/lp/bugs/browser/widgets/tests/test_bug.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/widgets/tests/test_bug.py	2012-06-29 04:38:24 +0000
@@ -0,0 +1,81 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from lp.bugs.browser.widgets.bug import BugTagsWidget
+from lp.bugs.interfaces.bugtarget import IHasOfficialBugTags
+from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class BugTagsWidgetTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def get_widget(self, bug_target):
+        field = IHasOfficialBugTags['official_bug_tags']
+        bound_field = field.bind(bug_target)
+        request = LaunchpadTestRequest()
+        return BugTagsWidget(bound_field, None, request)
+
+    def test_official_tags_js_not_adaptable_to_product_or_distro(self):
+        # project groups are not full bug targets so they have no tags.
+        project_group = self.factory.makeProject()
+        widget = self.get_widget(project_group)
+        js = widget.official_tags_js
+        self.assertEqual('var official_tags = [];', js)
+
+    def test_official_tags_js_product_without_tags(self):
+        # Products without tags have an empty list.
+        product = self.factory.makeProduct()
+        widget = self.get_widget(product)
+        js = widget.official_tags_js
+        self.assertEqual('var official_tags = [];', js)
+
+    def test_official_tags_js_product_with_tags(self):
+        # Products with tags have a list of tags.
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            product.official_bug_tags = [u'cows', u'pigs', u'sheep']
+        widget = self.get_widget(product)
+        js = widget.official_tags_js
+        self.assertEqual('var official_tags = ["cows", "pigs", "sheep"];', js)
+
+    def test_official_tags_js_distribution_without_tags(self):
+        # Distributions without tags have an empty list.
+        distribution = self.factory.makeDistribution()
+        widget = self.get_widget(distribution)
+        js = widget.official_tags_js
+        self.assertEqual('var official_tags = [];', js)
+
+    def test_official_tags_js_distribution_with_tags(self):
+        # Distributions with tags have a list of tags.
+        distribution = self.factory.makeDistribution()
+        with person_logged_in(distribution.owner):
+            distribution.official_bug_tags = [u'cows', u'pigs', u'sheep']
+        widget = self.get_widget(distribution)
+        js = widget.official_tags_js
+        self.assertEqual('var official_tags = ["cows", "pigs", "sheep"];', js)
+
+    def test_call(self):
+        # __call__ renders the input, help link, and script with official tags.
+        # Products with tags have a list of tags.
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            product.official_bug_tags = [u'cows', u'pigs', u'sheep']
+        widget = self.get_widget(product)
+        markup = widget()
+        self.assertIn(
+            '<input class="textType" id="field.official_bug_tags"', markup)
+        self.assertIn('<a href="/+help-bugs/tag-search.html"', markup)
+        self.assertIn('var official_tags = ["cows", "pigs", "sheep"];', markup)
+        self.assertIn(
+            'Y.lp.bugs.tags_entry.setup_tag_complete(', markup)
+        self.assertIn(
+            """'input[id="field.official_bug_tags"][type="text"]',""", markup)
+        self.assertIn("official_tags)", markup)

=== modified file 'lib/lp/bugs/javascript/bug_tags_entry.js'
--- lib/lp/bugs/javascript/bug_tags_entry.js	2012-06-22 19:49:29 +0000
+++ lib/lp/bugs/javascript/bug_tags_entry.js	2012-06-29 04:38:24 +0000
@@ -7,9 +7,9 @@
  * @submodule bug_tags_entry
  */
 
-YUI.add('lp.bugs.bug_tags_entry', function(Y) {
+YUI.add('lp.bugs.tags_entry', function(Y) {
 
-var namespace = Y.namespace('lp.bugs.bug_tags_entry');
+var namespace = Y.namespace('lp.bugs.tags_entry');
 
 var bug_tags_div;
 var tags_heading;
@@ -251,21 +251,30 @@
     });
     tags_trigger.addClass('js-action');
 
-    bug_tags_div.append(
+    autocomplete = namespace.setup_tag_complete(
+        '#tag-input', available_official_tags);
+};
+
+
+/**
+ * Set up bug tag autocompletion on a text input.
+ *
+ * @method setup_tag_complete
+ */
+namespace.setup_tag_complete = function(input, official_tags) {
+    Y.one('body').appendChild(
         '<div id="tags-autocomplete"><div id="tags-autocomplete-content">' +
         '</div></div>');
-    autocomplete = new Y.lazr.AutoComplete({
-        input: '#tag-input',
-        data: available_official_tags,
+    var autocomplete = new Y.lazr.AutoComplete({
+        input: input,
+        data: official_tags,
         boundingBox: '#tags-autocomplete',
         contentBox: '#tags-autocomplete-content'
     });
-    autocomplete.on('queryChange', function(e) {
-        var val = "null";
-        if (e.newVal !== null) {
-            val = "'" + e.newVal.text + "', " + e.newVal.offset;
-        }
+    autocomplete.get('input').on('focus', function(e) {
+        autocomplete.render();
     });
+    return autocomplete;
 };
 }, "0.1", {
     "requires": [

=== modified file 'lib/lp/bugs/javascript/tests/test_bug_tags_entry.html'
--- lib/lp/bugs/javascript/tests/test_bug_tags_entry.html	2012-06-22 14:58:26 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_tags_entry.html	2012-06-29 04:38:24 +0000
@@ -82,5 +82,14 @@
               class="sprite maybe action-icon">Tag help</a>
           </div>
         </script>
+
+        <script type="text/x-template" id="form-with-bug-tags">
+          <form>
+            <div>
+              <input type="text" name="field.tag" id="field.tag" />
+              <a href="/help/tags.html" target="help" class="help">(?)</a>
+            </div>
+          </form>
+        </script>
     </body>
 </html>

=== modified file 'lib/lp/bugs/javascript/tests/test_bug_tags_entry.js'
--- lib/lp/bugs/javascript/tests/test_bug_tags_entry.js	2012-06-22 19:49:29 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_tags_entry.js	2012-06-29 04:38:24 +0000
@@ -1,8 +1,8 @@
 YUI().use('lp.testing.runner', 'lp.testing.mockio', 'test', 'console',
-          'lp.client', 'node-event-simulate', 'lp.bugs.bug_tags_entry',
+          'lp.client', 'node-event-simulate', 'lp.bugs.tags_entry',
     function(Y) {
 
-    var module = Y.lp.bugs.bug_tags_entry;
+    var module = Y.lp.bugs.tags_entry;
     var suite = new Y.Test.Suite("Bug tags entry Tests");
 
     suite.add(new Y.Test.Case({
@@ -87,12 +87,7 @@
             Y.Assert.isInstanceOf(Y.Node, form_node.one('#tags-edit-spinner'));
             Y.Assert.isInstanceOf(Y.Node, form_node.one('#edit-tags-cancel'));
             Y.Assert.isInstanceOf(Y.Node, form_node.one('#tag-input'));
-            // The Autocompleter nodes are provided.
-            var completer_node = this.bug_tags_div.one('#tags-autocomplete');
-            Y.Assert.isInstanceOf(Y.Node, completer_node);
-            var completer_content = completer_node.one(
-                '#tags-autocomplete-content');
-            Y.Assert.isInstanceOf(Y.Node, completer_content);
+            Y.Assert.isInstanceOf(Y.Node, Y.one('#tags-autocomplete'));
         },
 
         test_show_activity: function() {
@@ -206,5 +201,47 @@
         }
     }));
 
+    suite.add(new Y.Test.Case({
+        name: 'Completer',
+
+        setUp: function() {
+            this.fixture = Y.one("#fixture");
+            var template = Y.one('#form-with-bug-tags').getContent();
+            this.fixture.append(template);
+        },
+
+        tearDown: function() {
+            if (this.fixture !== null) {
+                this.fixture.empty();
+            }
+            delete this.fixture;
+        },
+
+        test_setup_tag_complete: function() {
+            // The Autocompleter nodes are provided.
+            module.setup_tag_complete(
+                'input[id="field.tag"]',['project-tag']);
+            var completer_node = Y.one('#tags-autocomplete');
+            Y.Assert.isInstanceOf(Y.Node, completer_node);
+            var completer_content = completer_node.one(
+                '#tags-autocomplete-content');
+            Y.Assert.isInstanceOf(Y.Node, completer_content);
+            var input = Y.one('input[id="field.tag"]');
+        },
+
+        test_render_on_focus: function() {
+            // The Autocompleter nodes are provided.
+            var completer = module.setup_tag_complete(
+                'input[id="field.tag"]',['project-tag']);
+            var input = Y.one('input[id="field.tag"]');
+            var called = false;
+            completer.render = function() {
+                called = true;
+            };
+            input.simulate('focus');
+            Y.Assert.isTrue(called);
+        }
+    }));
+
     Y.lp.testing.Runner.run(suite);
 });

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2012-06-22 20:57:42 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2012-06-29 04:38:24 +0000
@@ -153,10 +153,10 @@
         </div>
 
         <script type="text/javascript">
-          LPJS.use('event', 'node', 'lp.bugs.bug_tags_entry', function(Y) {
+          LPJS.use('event', 'node', 'lp.bugs.tags_entry', function(Y) {
               Y.on('domready',
                    function(e) {
-                       Y.lp.bugs.bug_tags_entry.setup_tag_entry(
+                       Y.lp.bugs.tags_entry.setup_tag_entry(
                            available_official_tags);
                    },
                    window);

=== modified file 'lib/lp/bugs/templates/bugtask-macros-tableview.pt'
--- lib/lp/bugs/templates/bugtask-macros-tableview.pt	2012-03-01 19:12:34 +0000
+++ lib/lp/bugs/templates/bugtask-macros-tableview.pt	2012-06-29 04:38:24 +0000
@@ -516,8 +516,6 @@
                    tal:content="structure view/widgets/tag/label">
               Tag
             </label>: <input tal:replace="structure view/widgets/tag" />
-            <a href="/+help-bugs/tag-search.html"
-               target="help">Tag search help</a>
             <div
               tal:condition="error"
               class="message"

=== modified file 'lib/lp/registry/javascript/milestoneoverlay.js'
--- lib/lp/registry/javascript/milestoneoverlay.js	2012-05-12 01:58:54 +0000
+++ lib/lp/registry/javascript/milestoneoverlay.js	2012-06-29 04:38:24 +0000
@@ -78,7 +78,7 @@
 
         var set_milestone_tags = function(milestone) {
             var tagstring = data['field.tags'][0].toLowerCase();
-            var tags = Y.lp.bugs.bug_tags_entry.parse_tags(tagstring);
+            var tags = Y.lp.bugs.tags_entry.parse_tags(tagstring);
 
             // Since we are creating a new milestone it cannot already
             // have pre-existing tags, so if none are given we can
@@ -201,5 +201,5 @@
                         "lazr.formoverlay",
                         "lp.app.calendar",
                         "lp.client",
-                        "lp.bugs.bug_tags_entry"
+                        "lp.bugs.tags_entry"
                         ]});