Merge lp:~stevenk/launchpad/drop-enhanced_choice_popup into lp:launchpad

[Steve Kowalik]

Destroy the disclosure.enhanced_choice_popup.enabled feature flag, it's been turned on by default for a while.

The feature flag disclosure.enhanced_choice_popup.enabled has been turned on by default for a while. Destroy it, and while I'm at it, rip out the doctest for EnumChoiceWidget and move it to the unit test.

[Ian Booth (wallyworld)]

Looks good to go. I'd rename the test case now that new tests are added or even keep the existing test case and add the new tests to a new one in the same module.

review: Approve

=== modified file 'lib/lp/app/browser/lazrjs.py'
--- lib/lp/app/browser/lazrjs.py	2012-08-10 04:48:36 +0000
+++ lib/lp/app/browser/lazrjs.py	2012-08-14 03:55:23 +0000
@@ -580,9 +580,7 @@
         if description_fn is None:
             description_fn = lambda item: getattr(item, 'description', '')
         description = ''
-        feature_flag = getFeatureFlag(
-            'disclosure.enhanced_choice_popup.enabled')
-        if include_description and feature_flag:
+        if include_description:
             description = description_fn(item)
         new_item = {
             'name': name,

=== modified file 'lib/lp/app/doc/lazr-js-widgets.txt'
--- lib/lp/app/doc/lazr-js-widgets.txt	2012-08-10 04:48:36 +0000
+++ lib/lp/app/doc/lazr-js-widgets.txt	2012-08-14 03:55:23 +0000
@@ -356,65 +356,6 @@
 TextLineEditorWidget above.
 
 
-EnumChoiceWidget
-----------------
-
-This widget is primarily there allow setting of enum fields easily
-using the popup chooser.
-
-    >>> from lp.app.browser.lazrjs import EnumChoiceWidget
-
-As with the other widgets, this one requires a context object and a Choice
-type field.  The rendering of the widget hooks up to the lazr ChoiceSource
-with the standard patch plugin.
-
-One of the different things about this widget is the styles that are added.
-Many enums have specific colour styles.  Generally these are the names of
-the enum values (the upper-case bit) with a defined prefix.  This prefix
-is passed in to the constructor of the widget.
-
-A list of items is generated from the vocabulary.
-
-If the user does not have edit rights, the widget just renders the text based
-on the current value of the field on the object:
-
-    >>> login(ANONYMOUS)
-    >>> from lp.code.interfaces.branch import IBranch
-    >>> branch = factory.makeAnyBranch()
-    >>> widget = EnumChoiceWidget(
-    ...     branch, IBranch['lifecycle_status'],
-    ...     header='Change status to', css_class_prefix='branchstatus')
-    >>> print widget()
-    <span id="edit-lifecycle_status">
-       <span class="value branchstatusDEVELOPMENT">Development</span>
-    </span>
-
-If the user has edit rights, an edit icon is rendered and some javascript is
-rendered to hook up the widget.
-
-    >>> ignored = login_person(branch.owner)
-    >>> print widget()
-    <span id="edit-lifecycle_status">
-        <span class="value branchstatusDEVELOPMENT">Development</span>
-        <span>
-          <a class="editicon sprite edit action-icon"
-             href="http://.../+edit"
-             title="">Edit</a>
-        </span>
-    </span>
-    <script>
-    LPJS.use('lp.app.choice', function(Y) {
-    ...
-    </script>
-
-
-Changing the edit link
-**********************
-
-The edit link can be changed in exactly the same way as for the
-TextLineEditorWidget above.
-
-
 InlineMultiCheckboxWidget
 -------------------
 

=== modified file 'lib/lp/app/widgets/tests/test_itemswidgets.py'
--- lib/lp/app/widgets/tests/test_itemswidgets.py	2012-07-26 07:54:40 +0000
+++ lib/lp/app/widgets/tests/test_itemswidgets.py	2012-08-14 03:55:23 +0000
@@ -3,9 +3,7 @@
 
 __metaclass__ = type
 
-
 import doctest
-from unittest import TestCase
 
 from lazr.enum import (
     EnumeratedType,
@@ -22,17 +20,24 @@
     SimpleVocabulary,
     )
 
-from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
+from lp.app.browser.lazrjs import (
+    EnumChoiceWidget,
+    vocabulary_to_choice_edit_items,
+    )
 from lp.app.widgets.itemswidgets import (
     LabeledMultiCheckBoxWidget,
     LaunchpadRadioWidget,
     LaunchpadRadioWidgetWithDescription,
     PlainMultiCheckBoxWidget,
     )
-from lp.services.features.testing import FeatureFixture
+from lp.code.interfaces.branch import IBranch
 from lp.services.webapp.menu import structured
 from lp.services.webapp.servers import LaunchpadTestRequest
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    ANONYMOUS,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -222,13 +227,8 @@
         self.assertEqual(expected, hint_html)
 
 
-class TestVocabularyToChoiceEditItems(TestCase):
-    """Tests for vocabulary_to_choice_edit_items.
-
-    This function is tested implicitly in lazr-js-widgets.txt.
-    Here we are adding some explicit tests for the behaviour enabled by
-    feature flag disclosure.enhanced_choice_popup.enabled.
-    """
+class TestEnumChoiceWidget(TestCaseWithFactory):
+    """Tests for EnumChoiceWidget and vocabulary_to_choice_edit_items."""
 
     layer = DatabaseFunctionalLayer
 
@@ -267,21 +267,17 @@
         self.assertEqual(expected, items)
 
     def test_vocabulary_to_choice_edit_items_no_description(self):
-        # Even if feature flag is on, there are no descriptions unless wanted.
-        feature_flag = {'disclosure.enhanced_choice_popup.enabled': 'on'}
-        with FeatureFixture(feature_flag):
-            overrides = {'description': ''}
-            items = vocabulary_to_choice_edit_items(self.ChoiceEnum)
+        # There are no descriptions unless wanted.
+        overrides = {'description': ''}
+        items = vocabulary_to_choice_edit_items(self.ChoiceEnum)
         expected = [self._makeItemDict(e.value, overrides)
                     for e in self.ChoiceEnum]
         self.assertEqual(expected, items)
 
     def test_vocabulary_to_choice_edit_items_with_description(self):
-        # The items list is as expected with the feature flag.
-        feature_flag = {'disclosure.enhanced_choice_popup.enabled': 'on'}
-        with FeatureFixture(feature_flag):
-            items = vocabulary_to_choice_edit_items(
-                self.ChoiceEnum, include_description=True)
+        # The items list is as expected.
+        items = vocabulary_to_choice_edit_items(
+            self.ChoiceEnum, include_description=True)
         expected = [self._makeItemDict(e.value) for e in self.ChoiceEnum]
         self.assertEqual(expected, items)
 
@@ -290,6 +286,50 @@
         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)]
+        expected = [self._makeItemDict(self.ChoiceEnum.ITEM_A)]
         self.assertEqual(expected, items)
+
+    def test_widget_with_anonymous_user(self):
+        # When the user is anonymous, the widget outputs static text.
+        branch = self.factory.makeAnyBranch()
+        widget = EnumChoiceWidget(
+            branch, IBranch['lifecycle_status'],
+            header='Change status to', css_class_prefix='branchstatus')
+        with person_logged_in(ANONYMOUS):
+            markup = widget()
+        expected = """
+        <span id="edit-lifecycle_status">
+            <span class="value branchstatusDEVELOPMENT">Development</span>
+        </span>
+        """
+        expected_matcher = DocTestMatches(
+            expected, (doctest.NORMALIZE_WHITESPACE |
+                       doctest.REPORT_NDIFF | doctest.ELLIPSIS))
+        self.assertThat(markup, expected_matcher)
+
+    def test_widget_with_user(self):
+        # When the user has edit permission, the widget allows changes via JS.
+        branch = self.factory.makeAnyBranch()
+        widget = EnumChoiceWidget(
+            branch, IBranch['lifecycle_status'],
+            header='Change status to', css_class_prefix='branchstatus')
+        with person_logged_in(branch.owner):
+            markup = widget()
+        expected = """
+        <span id="edit-lifecycle_status">
+            <span class="value branchstatusDEVELOPMENT">Development</span>
+            <span>
+            <a class="editicon sprite edit action-icon"
+                href="http://.../+edit"
+                title="">Edit</a>
+            </span>
+        </span>
+        <script>
+        LPJS.use('lp.app.choice', function(Y) {
+        ...
+        </script>
+        """
+        expected_matcher = DocTestMatches(
+            expected, (doctest.NORMALIZE_WHITESPACE |
+                       doctest.REPORT_NDIFF | doctest.ELLIPSIS))
+        self.assertThat(markup, expected_matcher)

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-08-03 00:30:51 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-08-14 03:55:23 +0000
@@ -29,7 +29,6 @@
     PRIVATE_INFORMATION_TYPES,
     )
 from lp.registry.interfaces.projectgroup import IProjectGroup
-from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     login,
@@ -551,12 +550,6 @@
 
     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, private_bugs=False):
         cache = IJSONRequestCache(view.request).objects
         self.assertEqual(

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-08-10 06:38:56 +0000
+++ lib/lp/services/features/flags.py	2012-08-14 03:55:23 +0000
@@ -233,13 +233,6 @@
      '',
      '',
      ''),
-    ('disclosure.enhanced_choice_popup.enabled',
-     'boolean',
-     ('If true, will include any available descriptive text with each choice '
-      'item in the selection popup.'),
-     '',
-     '',
-     ''),
     ('disclosure.enhanced_sharing.enabled',
      'boolean',
      ('If true, will allow the use of the new sharing view and apis used '