Merge lp:~wallyworld/launchpad/filebug-choice-widgets-1003748 into lp:launchpad

[Ian Booth]

Add progressive enhancement for status and importance popups on file bug page.

== Implementation ==

The branch provides functionality to enhance the +filebug form when javascript is enabled. The standard form uses simple drop down combo controls for bug status and importance. These controls are replaced with choice source popups the same as used on the bugtask form itself. So the user gets nice colours and descriptions etc.

The way it works is that the drop down combo is hidden and the choice source popup is rendered in its place. The the popup choice is made, it is saved to the hidden combo widget so that when the form is saved, the correct value is passed to the back end.

I have made an addition change for the popups. Not all of the available bug status choices are valid when filing a bug. Specifically, these choices have been excluded from selection:
- expired
- invalid
- opinion
- won't fix
- incomplete

So the user gets to choose from:
- new
- confirmed
- triaged
- in progress
- fix committed
- fix released

The latter 2 choices allow a bug to be file retrospectively for a fix that has already been completed.

== Tests ==

Add new yui tests for the additional file bug functionality.
Update the TestFileBugRequestCache test case to check the additional json request cache attributes for status and importance.
Update TestVocabularyToChoiceEditItems to check that vocabulary_to_choice_edit_items() handles the excluded_items parameter.

= Lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/browser/lazrjs.py
  lib/lp/app/javascript/choice.js
  lib/lp/app/widgets/tests/test_itemswidgets.py
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/browser/tests/test_bugtarget_filebug.py
  lib/lp/bugs/javascript/filebug.js
  lib/lp/bugs/javascript/tests/test_filebug.html
  lib/lp/bugs/javascript/tests/test_filebug.js

[Curtis Hovey (sinzui)]

Wow. Thank you.

I do not think "Confirmed" should be in the list given your reasoning. Confirmed means someone other than the bug reporter is affected by the bug, so the UI should not let me cheat. If I am the triager, I can set "Triaged" to mean someone affiliated with the project acknowledges the bug.

review: Approve (code)

=== modified file 'lib/lp/app/browser/lazrjs.py'
--- lib/lp/app/browser/lazrjs.py	2012-03-05 09:17:17 +0000
+++ lib/lp/app/browser/lazrjs.py	2012-05-28 22:54:19 +0000
@@ -513,8 +513,8 @@
 
 def vocabulary_to_choice_edit_items(
     vocab, include_description=False, css_class_prefix=None,
-    disabled_items=None, as_json=False, name_fn=None, value_fn=None,
-    description_fn=None):
+    disabled_items=None, excluded_items=None,
+    as_json=False, name_fn=None, value_fn=None, description_fn=None):
     """Convert an enumerable to JSON for a ChoiceEdit.
 
     :vocab: The enumeration to iterate over.
@@ -532,6 +532,8 @@
         # SimpleTerm objects have the object itself at item.value.
         if safe_hasattr(item, 'value'):
             item = item.value
+        if excluded_items and item in excluded_items:
+            continue
         if name_fn is not None:
             name = name_fn(item)
         else:

=== modified file 'lib/lp/app/javascript/choice.js'
--- lib/lp/app/javascript/choice.js	2012-05-15 16:51:06 +0000
+++ lib/lp/app/javascript/choice.js	2012-05-28 22:54:19 +0000
@@ -56,4 +56,43 @@
   widget.render();
 };
 
+namespace.addPopupChoice = function(field_name, choices) {
+    var legacy_field_node = Y.one('[id=field.' + field_name + ']');
+    var initial_field_value = legacy_field_node.get('value');
+
+    var choice_node = Y.Node.create([
+        '<span id="' + field_name + '-content"><span class="value"></span>',
+        '<a href="#"><img class="sprite edit editicon"/>&nbsp;</a></span>'
+        ].join(' '));
+
+    legacy_field_node.insertBefore(choice_node, legacy_field_node);
+    legacy_field_node.addClass('unseen');
+    var field_content = Y.one('#' + field_name + '-content');
+
+    var choice_edit = new Y.ChoiceSource({
+        contentBox: field_content,
+        value: initial_field_value,
+        title: 'Set ' + field_name + " as",
+        items: choices,
+        elementToFlash: field_content
+    });
+    choice_edit.render();
+
+    var update_selected_value_css = function(selected_value) {
+        Y.Array.each(choices, function(item) {
+            if (item.value === selected_value) {
+                field_content.addClass(item.css_class);
+            } else {
+                field_content.removeClass(item.css_class);
+            }
+        });
+    };
+    update_selected_value_css(initial_field_value);
+    choice_edit.on('save', function(e) {
+        var selected_value = choice_edit.get('value');
+        update_selected_value_css(selected_value);
+        legacy_field_node.set('value', selected_value);
+    });
+};
+
 }, "0.1", {"requires": ["lazr.choiceedit", "lp.client.plugins"]});

=== modified file 'lib/lp/app/widgets/tests/test_itemswidgets.py'
--- lib/lp/app/widgets/tests/test_itemswidgets.py	2012-05-16 23:56:08 +0000
+++ lib/lp/app/widgets/tests/test_itemswidgets.py	2012-05-28 22:54:19 +0000
@@ -298,3 +298,12 @@
                 self.ChoiceEnum, include_description=True)
         expected = [self._makeItemDict(e.value) for e in self.ChoiceEnum]
         self.assertEqual(expected, items)
+
+    def test_vocabulary_to_choice_edit_items_excluded_items(self):
+        # Excluded items are not included.
+        items = vocabulary_to_choice_edit_items(
+            self.ChoiceEnum, include_description=True,
+            excluded_items=[self.ChoiceEnum.ITEM_B])
+        overrides = {'description': ''}
+        expected = [self._makeItemDict(self.ChoiceEnum.ITEM_A, overrides)]
+        self.assertEqual(expected, items)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2012-05-25 14:38:42 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2012-05-28 22:54:19 +0000
@@ -58,6 +58,7 @@
     LaunchpadFormView,
     safe_action,
     )
+from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
 from lp.app.browser.stringformatter import FormattersAPI
 from lp.app.enums import ServiceUsage
 from lp.app.errors import (
@@ -98,6 +99,8 @@
     IOfficialBugTagTargetRestricted,
     )
 from lp.bugs.interfaces.bugtask import (
+    BugTaskImportance,
+    BugTaskStatus,
     IBugTaskSet,
     UNRESOLVED_BUGTASK_STATUSES,
     )
@@ -389,6 +392,21 @@
         cache.objects['enable_bugfiling_duplicate_search'] = (
             IProjectGroup.providedBy(self.context)
             or self.context.enable_bugfiling_duplicate_search)
+        bugtask_status_data = vocabulary_to_choice_edit_items(
+            BugTaskStatus, include_description=True, css_class_prefix='status',
+            excluded_items=[
+                BugTaskStatus.UNKNOWN,
+                BugTaskStatus.EXPIRED,
+                BugTaskStatus.INVALID,
+                BugTaskStatus.OPINION,
+                BugTaskStatus.WONTFIX,
+                BugTaskStatus.INCOMPLETE])
+        cache.objects['bugtask_status_data'] = bugtask_status_data
+        bugtask_importance_data = vocabulary_to_choice_edit_items(
+            BugTaskImportance, include_description=True,
+            css_class_prefix='importance',
+            excluded_items=[BugTaskImportance.UNKNOWN])
+        cache.objects['bugtask_importance_data'] = bugtask_importance_data
         if (self.extra_data_token is not None and
             not self.extra_data_to_process):
             # self.extra_data has been initialized in publishTraverse().

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-05-27 23:04:28 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-05-28 22:54:19 +0000
@@ -21,6 +21,10 @@
     IBugAddForm,
     IBugSet,
     )
+from lp.bugs.interfaces.bugtask import (
+    BugTaskImportance,
+    BugTaskStatus,
+    )
 from lp.bugs.publisher import BugsLayer
 from lp.registry.enums import (
     InformationType,
@@ -496,10 +500,59 @@
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestFileBugRequestCache, self).setUp()
+        self.useFixture(FeatureFixture({
+            'disclosure.enhanced_choice_popup.enabled': 'true'
+        }))
+
     def _assert_cache_values(self, view, duplicate_search):
         cache = IJSONRequestCache(view.request).objects
         self.assertEqual(
-            cache['enable_bugfiling_duplicate_search'], duplicate_search)
+            duplicate_search, cache['enable_bugfiling_duplicate_search'])
+        excluded_statuses = [
+            BugTaskStatus.UNKNOWN,
+            BugTaskStatus.EXPIRED,
+            BugTaskStatus.INVALID,
+            BugTaskStatus.OPINION,
+            BugTaskStatus.WONTFIX,
+            BugTaskStatus.INCOMPLETE]
+        bugtask_status_data = []
+        for item in BugTaskStatus:
+            item = item.value
+            if item in excluded_statuses:
+                continue
+            name = item.title
+            value = item.title
+            description = item.description
+            new_item = {'name': name, 'value': value,
+                        'description': description,
+                        'description_css_class': 'choice-description',
+                        'style': '',
+                        'help': '', 'disabled': False,
+                        'css_class': 'status' + item.name}
+            bugtask_status_data.append(new_item)
+        self.assertEqual(
+            bugtask_status_data, cache['bugtask_status_data'])
+        excluded_importances = [
+            BugTaskImportance.UNKNOWN]
+        bugtask_importance_data = []
+        for item in BugTaskImportance:
+            item = item.value
+            if item in excluded_importances:
+                continue
+            name = item.title
+            value = item.title
+            description = item.description
+            new_item = {'name': name, 'value': value,
+                        'description': description,
+                        'description_css_class': 'choice-description',
+                        'style': '',
+                        'help': '', 'disabled': False,
+                        'css_class': 'importance' + item.name}
+            bugtask_importance_data.append(new_item)
+        self.assertEqual(
+            bugtask_importance_data, cache['bugtask_importance_data'])
 
     def test_product(self):
         project = self.factory.makeProduct(official_malone=True)

=== modified file 'lib/lp/bugs/javascript/filebug.js'
--- lib/lp/bugs/javascript/filebug.js	2012-05-25 14:38:42 +0000
+++ lib/lp/bugs/javascript/filebug.js	2012-05-28 22:54:19 +0000
@@ -28,6 +28,7 @@
     } else {
         setup_security_related();
     }
+    setupChoiceWidgets();
     var filebug_privacy_text = "This report will be private. " +
         "You can disclose it later.";
     update_privacy_banner(
@@ -53,6 +54,12 @@
     }, "input[name='field.information_type']");
 };
 
+var setupChoiceWidgets = function() {
+    Y.lp.app.choice.addPopupChoice('status', LP.cache.bugtask_status_data);
+    Y.lp.app.choice.addPopupChoice(
+        'importance', LP.cache.bugtask_importance_data);
+};
+
 var setup_security_related = function() {
     var sec = Y.one('[id="field.security_related"]');
     if (!Y.Lang.isValue(sec)) {
@@ -71,5 +78,6 @@
 namespace.setup_filebug = setup_filebug;
 
 }, "0.1", {"requires": [
-    "base", "node", "event", "node-event-delegate",
-    "lp.app.banner.privacy", "lp.bugs.filebug_dupefinder"]});
+    "base", "node", "event", "node-event-delegate", "lazr.choiceedit",
+    "lp.app.banner.privacy", "lp.app.choice",
+    "lp.bugs.filebug_dupefinder"]});

=== modified file 'lib/lp/bugs/javascript/tests/test_filebug.html'
--- lib/lp/bugs/javascript/tests/test_filebug.html	2012-05-25 14:38:42 +0000
+++ lib/lp/bugs/javascript/tests/test_filebug.html	2012-05-28 22:54:19 +0000
@@ -28,6 +28,10 @@
       <script type="text/javascript"
           src="../../../../../build/js/lp/app/mustache.js"></script>
       <script type="text/javascript"
+          src="../../../../../build/js/lp/app/choice.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/choiceedit/choiceedit.js"></script>
+      <script type="text/javascript"
           src="../../../../../build/js/lp/app/expander.js"></script>
       <script type="text/javascript"
           src="../../../../../build/js/lp/app/formoverlay/formoverlay.js"></script>
@@ -98,6 +102,18 @@
             </tr>
             </tbody>
         </table>
+        <div class="value">
+            <select size="1" name="field.status" id="field.status">
+            <option value="New" selected="selected">New</option>
+            <option value="Incomplete">Incomplete</option>
+            </select>
+        </div>
+        <div class="value">
+            <select size="1" name="field.status" id="field.importance">
+            <option value="Undecided" selected="selected">Undecided</option>
+            <option value="High">High</option>
+            </select>
+        </div>
         </script>
     </body>
 </html>

=== modified file 'lib/lp/bugs/javascript/tests/test_filebug.js'
--- lib/lp/bugs/javascript/tests/test_filebug.js	2012-05-25 14:38:42 +0000
+++ lib/lp/bugs/javascript/tests/test_filebug.js	2012-05-28 22:54:19 +0000
@@ -13,7 +13,15 @@
             window.LP = {
                 links: {},
                 cache: {
-                    private_types: ['EMBARGOEDSECURITY', 'USERDATA']
+                    private_types: ['EMBARGOEDSECURITY', 'USERDATA'],
+                    bugtask_status_data: [
+                        {name: 'New', value: 'New'},
+                        {name: 'Incomplete', value: 'Incomplete'}
+                    ],
+                    bugtask_importance_data: [
+                        {name: 'Undecided', value: 'Undecided'},
+                        {name: 'High', value: 'High'}
+                    ]
                 }
             };
             this.fixture = Y.one('#fixture');
@@ -104,10 +112,56 @@
             Y.lp.bugs.filebug_dupefinder.setup_dupes = orig_setup_dupes;
             Y.lp.bugs.filebug_dupefinder.setup_dupe_finder
                 = orig_setup_dupe_finder;
+        },
+
+        // The bugtask status choice popup is rendered.
+        test_status_setup: function () {
+            Y.lp.bugs.filebug.setup_filebug(true);
+            var status_node = Y.one('#status-content .value');
+            Y.Assert.areEqual('New', status_node.get('text'));
+            var status_edit_node = Y.one('#status-content a img.edit');
+            Y.Assert.isNotNull(status_edit_node);
+            var legacy_dropdown = Y.one('[id=field.status]');
+            Y.Assert.isTrue(legacy_dropdown.hasClass('unseen'));
+        },
+
+        // The bugtask importance choice popup is rendered.
+        test_importance_setup: function () {
+            Y.lp.bugs.filebug.setup_filebug(true);
+            var importance_node = Y.one('#importance-content .value');
+            Y.Assert.areEqual('Undecided', importance_node.get('text'));
+            var importance_edit_node = Y.one('#status-content a img.edit');
+            Y.Assert.isNotNull(importance_edit_node);
+            var legacy_dropdown = Y.one('[id=field.importance]');
+            Y.Assert.isTrue(legacy_dropdown.hasClass('unseen'));
+        },
+
+        // The bugtask status choice popup updates the form.
+        test_status_selection: function() {
+            Y.lp.bugs.filebug.setup_filebug(true);
+            var status_popup = Y.one('#status-content a');
+            status_popup.simulate('click');
+            var status_choice = Y.one(
+                '.yui3-ichoicelist-content a[href=#Incomplete]');
+            status_choice.simulate('click');
+            var legacy_dropdown = Y.one('[id=field.status]');
+            Y.Assert.areEqual('Incomplete', legacy_dropdown.get('value'));
+        },
+
+        // The bugtask importance choice popup updates the form.
+        test_importance_selection: function() {
+            Y.lp.bugs.filebug.setup_filebug(true);
+            var status_popup = Y.one('#importance-content a');
+            status_popup.simulate('click');
+            var status_choice = Y.one(
+                '.yui3-ichoicelist-content a[href=#High]');
+            status_choice.simulate('click');
+            var legacy_dropdown = Y.one('[id=field.importance]');
+            Y.Assert.areEqual('High', legacy_dropdown.get('value'));
         }
     }));
 
 }, '0.1', {'requires': ['test', 'console', 'event', 'node-event-simulate',
-        'lp.app.banner.privacy', 'lp.bugs.filebug_dupefinder',
-        'lp.bugs.filebug'
+        'lp.app.banner.privacy', 'lp.app.choice',
+        'lp.bugs.filebug_dupefinder', 'lp.bugs.filebug'
         ]});