Merge lp:~wgrant/launchpad/rework-bug-default-type-1 into lp:launchpad

[William Grant]

Merge FileBugReportingGuidelines into FileBugViewBase, as ProjectGroup:+filebug doesn't need it rendered separately any more.

This branch is the second in a series of maybe four to rework bug information type defaults, which will eventually allow us to default to Proprietary when a project is so configured.

Some awkwardness arose during UI refactoring due to the split between FileBugViewBase and FileBugReportingGuidelines (which -- contrary to its name -- includes not just the bug reporting guidelines, but the privacy widgets too). I eventually realised that the split was for some code in ProjectGroup:+filebug that has since been removed, so I opted to merge the views, templates and tests.

The only minor awkwardness was that the form relies on the JSON request cache being fully populated, but form action templates are rendered in LaunchpadFormView.initialize(). So we can't do the super() call first as is usual. This problem was accidentally worked around with the old structure, since the JSON cache ended up populated by the separately initialised subview.

[Richard Harding (rharding)]

Think I follow. Thanks for the cleanup.

review: Approve

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2012-07-31 05:46:45 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2012-08-01 01:08:21 +0000
@@ -243,30 +243,40 @@
         self.updateContextFromData(data)
 
 
-class FileBugReportingGuidelines(LaunchpadFormView):
-    """Provides access to common bug reporting attributes.
-
-    Attributes provided are: information_type and bug_reporting_guidelines.
-
-    This view is a superclass of `FileBugViewBase` so that non-ajax browsers
-    can load the file bug form, and it is also invoked directly via an XHR
-    request to provide an HTML snippet for Javascript enabled browsers.
-    """
+class FileBugViewBase(LaunchpadFormView):
+    """Base class for views related to filing a bug."""
+
+    implements(IBrowserPublisher)
 
     schema = IBug
 
-    @property
-    def field_names(self):
-        """Return the list of field names to display."""
-        if self.is_bug_supervisor:
-            return ['information_type']
-        else:
-            return ['security_related']
-
     custom_widget('information_type', LaunchpadRadioWidgetWithDescription)
 
+    extra_data_token = None
+    advanced_form = False
+    frontpage_form = False
+    data_parser = None
+
+    def __init__(self, context, request):
+        LaunchpadFormView.__init__(self, context, request)
+        self.extra_data = FileBugData()
+
     def initialize(self):
-        super(FileBugReportingGuidelines, self).initialize()
+        # redirect_ubuntu_filebug is a cached_property.
+        # Access it first just to compute its value. Because it
+        # makes a DB access to get the bug supervisor, it causes
+        # trouble in tests when form validation errors occur. Because the
+        # transaction is doomed, the storm cache is invalidated and accessing
+        # the property will result in a a LostObjectError, because
+        # the created objects disappeared. Not likely a problem in production
+        # since the objects will still be in the DB, but doesn't hurt there
+        # either. It makes for better diagnosis of failing tests.
+        if self.redirect_ubuntu_filebug:
+            pass
+
+        # The JSON cache must be populated before the super call, since
+        # the form is rendered during LaunchpadFormView's initialize()
+        # when an action is invokved.
         cache = IJSONRequestCache(self.request)
         cache.objects['private_types'] = [
             type.name for type in PRIVATE_INFORMATION_TYPES]
@@ -296,125 +306,12 @@
             css_class_prefix='importance',
             excluded_items=[BugTaskImportance.UNKNOWN])
         cache.objects['bugtask_importance_data'] = bugtask_importance_data
-
-    def setUpFields(self):
-        """Set up the form fields. See `LaunchpadFormView`."""
-        super(FileBugReportingGuidelines, self).setUpFields()
-
-        # Project groups are special. The Next button sends you to
-        # Product:+filebug, so we need none of the usual stuff.
-        if IProjectGroup.providedBy(self.context):
-            return
-
-        if self.is_bug_supervisor:
-            info_type_vocab = InformationTypeVocabulary(
-                types=self.context.pillar.getAllowedBugInformationTypes())
-            information_type_field = copy_field(
-                IBug['information_type'], readonly=False,
-                vocabulary=info_type_vocab)
-            self.form_fields = self.form_fields.omit('information_type')
-            self.form_fields += Fields(information_type_field)
-        else:
-            security_related_field = copy_field(
-                IBug['security_related'], readonly=False)
-            self.form_fields = self.form_fields.omit('security_related')
-            self.form_fields += Fields(security_related_field)
-
-    @property
-    def initial_values(self):
-        """See `LaunchpadFormView`."""
-        value = InformationType.PUBLIC
-        if (self.context and IProduct.providedBy(self.context) and
-            self.context.private_bugs):
-            value = InformationType.USERDATA
-        return {'information_type': value}
-
-    @property
-    def bug_reporting_guidelines(self):
-        """Guidelines for filing bugs in the current context.
-
-        Returns a list of dicts, with each dict containing values for
-        "preamble" and "content".
-        """
-
-        def target_name(target):
-            # IProjectGroup can be considered the target of a bug during
-            # the bug filing process, but does not extend IBugTarget
-            # and ultimately cannot actually be the target of a
-            # bug. Hence this function to determine a suitable
-            # name/title to display. Hurrumph.
-            if IBugTarget.providedBy(target):
-                return target.bugtargetdisplayname
-            else:
-                return target.displayname
-
-        guidelines = []
-        bugtarget = self.context
-        if bugtarget is not None:
-            content = bugtarget.bug_reporting_guidelines
-            if content is not None and len(content) > 0:
-                guidelines.append({
-                        "source": target_name(bugtarget),
-                        "content": content,
-                        })
-            # Distribution source packages are shown with both their
-            # own reporting guidelines and those of their
-            # distribution.
-            if IDistributionSourcePackage.providedBy(bugtarget):
-                distribution = bugtarget.distribution
-                content = distribution.bug_reporting_guidelines
-                if content is not None and len(content) > 0:
-                    guidelines.append({
-                            "source": target_name(distribution),
-                            "content": content,
-                            })
-        return guidelines
-
-    def getMainContext(self):
-        if IDistributionSourcePackage.providedBy(self.context):
-            return self.context.distribution
-        else:
-            return self.context
-
-    @cachedproperty
-    def is_bug_supervisor(self):
-        """ Return True if the logged in user is a bug supervisor."""
-        context = self.getMainContext()
-        return BugTask.userHasBugSupervisorPrivilegesContext(
-            context, self.user)
-
-
-class FileBugViewBase(FileBugReportingGuidelines, LaunchpadFormView):
-    """Base class for views related to filing a bug."""
-
-    implements(IBrowserPublisher)
-
-    extra_data_token = None
-    advanced_form = False
-    frontpage_form = False
-    data_parser = None
-
-    def __init__(self, context, request):
-        LaunchpadFormView.__init__(self, context, request)
-        self.extra_data = FileBugData()
-
-    def initialize(self):
-        # redirect_ubuntu_filebug is a cached_property.
-        # Access it first just to compute its value. Because it
-        # makes a DB access to get the bug supervisor, it causes
-        # trouble in tests when form validation errors occur. Because the
-        # transaction is doomed, the storm cache is invalidated and accessing
-        # the property will result in a a LostObjectError, because
-        # the created objects disappeared. Not likely a problem in production
-        # since the objects will still be in the DB, but doesn't hurt there
-        # either. It makes for better diagnosis of failing tests.
-        if self.redirect_ubuntu_filebug:
-            pass
-        super(FileBugViewBase, self).initialize()
-        cache = IJSONRequestCache(self.request)
         cache.objects['enable_bugfiling_duplicate_search'] = (
             IProjectGroup.providedBy(self.context)
             or self.context.enable_bugfiling_duplicate_search)
+
+        super(FileBugViewBase, self).initialize()
+
         if (self.extra_data_token is not None and
             not self.extra_data_to_process):
             # self.extra_data has been initialized in publishTraverse().
@@ -492,10 +389,17 @@
     @property
     def initial_values(self):
         """Give packagename a default value, if applicable."""
-        if not IDistributionSourcePackage.providedBy(self.context):
-            return {}
-
-        return {'packagename': self.context.name}
+        if (self.context and IProduct.providedBy(self.context)
+            and self.context.private_bugs):
+            type = InformationType.USERDATA
+        else:
+            type = InformationType.PUBLIC
+        values = {'information_type': type}
+
+        if IDistributionSourcePackage.providedBy(self.context):
+            values['packagename'] = self.context.name
+
+        return values
 
     def contextIsProduct(self):
         return IProduct.providedBy(self.context)
@@ -588,7 +492,7 @@
 
     def setUpWidgets(self):
         """Customize the onKeyPress event of the package name chooser."""
-        LaunchpadFormView.setUpWidgets(self)
+        super(FileBugViewBase, self).setUpWidgets()
 
         if "packagename" in self.field_names:
             self.widgets["packagename"].onKeyPress = (
@@ -598,6 +502,25 @@
         """Set up the form fields. See `LaunchpadFormView`."""
         super(FileBugViewBase, self).setUpFields()
 
+        # Project groups are special. The Next button sends you to
+        # Product:+filebug, so we need none of the usual stuff.
+        if IProjectGroup.providedBy(self.context):
+            return
+
+        if self.is_bug_supervisor:
+            info_type_vocab = InformationTypeVocabulary(
+                types=self.context.pillar.getAllowedBugInformationTypes())
+            information_type_field = copy_field(
+                IBug['information_type'], readonly=False,
+                vocabulary=info_type_vocab)
+            self.form_fields = self.form_fields.omit('information_type')
+            self.form_fields += Fields(information_type_field)
+        else:
+            security_related_field = copy_field(
+                IBug['security_related'], readonly=False)
+            self.form_fields = self.form_fields.omit('security_related')
+            self.form_fields += Fields(security_related_field)
+
         # Override the vocabulary for the subscribe_to_existing_bug
         # field.
         subscribe_field = Choice(
@@ -1017,6 +940,60 @@
         else:
             return True
 
+    @property
+    def bug_reporting_guidelines(self):
+        """Guidelines for filing bugs in the current context.
+
+        Returns a list of dicts, with each dict containing values for
+        "preamble" and "content".
+        """
+
+        def target_name(target):
+            # IProjectGroup can be considered the target of a bug during
+            # the bug filing process, but does not extend IBugTarget
+            # and ultimately cannot actually be the target of a
+            # bug. Hence this function to determine a suitable
+            # name/title to display. Hurrumph.
+            if IBugTarget.providedBy(target):
+                return target.bugtargetdisplayname
+            else:
+                return target.displayname
+
+        guidelines = []
+        bugtarget = self.context
+        if bugtarget is not None:
+            content = bugtarget.bug_reporting_guidelines
+            if content is not None and len(content) > 0:
+                guidelines.append({
+                        "source": target_name(bugtarget),
+                        "content": content,
+                        })
+            # Distribution source packages are shown with both their
+            # own reporting guidelines and those of their
+            # distribution.
+            if IDistributionSourcePackage.providedBy(bugtarget):
+                distribution = bugtarget.distribution
+                content = distribution.bug_reporting_guidelines
+                if content is not None and len(content) > 0:
+                    guidelines.append({
+                            "source": target_name(distribution),
+                            "content": content,
+                            })
+        return guidelines
+
+    def getMainContext(self):
+        if IDistributionSourcePackage.providedBy(self.context):
+            return self.context.distribution
+        else:
+            return self.context
+
+    @cachedproperty
+    def is_bug_supervisor(self):
+        """ Return True if the logged in user is a bug supervisor."""
+        context = self.getMainContext()
+        return BugTask.userHasBugSupervisorPrivilegesContext(
+            context, self.user)
+
 
 class FileBugAdvancedView(FileBugViewBase):
     """Browser view for filing a bug.
@@ -1131,7 +1108,7 @@
     show_summary_in_results = True
 
     def initialize(self):
-        FilebugShowSimilarBugsView.initialize(self)
+        super(FileBugGuidedView, self).initialize()
         if self.redirect_ubuntu_filebug:
             # The user is trying to file a new Ubuntu bug via the web
             # interface and without using apport. Redirect to a page
 
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2012-07-29 23:38:26 +0000
+++ lib/lp/bugs/browser/configure.zcml	2012-08-01 01:08:21 +0000
@@ -103,12 +103,6 @@
             template="../templates/bugtarget-filebug-inline-form.pt"
             permission="launchpad.AnyPerson"/>
         <browser:page
-            for="lp.bugs.interfaces.bugtarget.IHasBugs"
-            class="lp.bugs.browser.bugtarget.FileBugReportingGuidelines"
-            template="../templates/bugtarget-filebug-guidelines.pt"
-            permission="launchpad.AnyPerson"
-            name="+filebug-reporting-guidelines"/>
-        <browser:page
             name="+manage-official-tags"
             for="lp.bugs.interfaces.bugtarget.IOfficialBugTagTargetRestricted"
             class="lp.bugs.browser.bugtarget.OfficialBugTagsManageView"
 
=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-07-26 07:54:40 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-08-01 01:08:21 +0000
@@ -340,17 +340,11 @@
         self.assertEqual(0, len(view.errors))
         self.assertTrue(view.added_bug is not None)
 
-
-class TestFileBugReportingGuidelines(TestCaseWithFactory):
-
-    layer = DatabaseFunctionalLayer
-
     def test_filebug_reporting_details(self):
         product = self.factory.makeProduct()
         login_person(product.owner)
         product.bug_reporting_guidelines = "Include bug details"
-        view = create_initialized_view(
-            product, '+filebug-reporting-guidelines')
+        view = create_initialized_view(product, '+filebug')
         expected_guidelines = [{
             "source": product.displayname, "content": u"Include bug details",
             }]
@@ -522,53 +516,6 @@
         self.assertIn("Thank you for your bug report.", msg)
 
 
-class TestFileBugGuidelinesRequestCache(TestCaseWithFactory):
-    # Tests to ensure the request cache contains the expected values for
-    # file bug guidelines views.
-
-    layer = DatabaseFunctionalLayer
-
-    def _assert_cache_values(self, view, private_bugs, duplicate_search):
-        cache = IJSONRequestCache(view.request).objects
-        self.assertContentEqual(cache['private_types'], [
-            type.name for type in PRIVATE_INFORMATION_TYPES])
-        self.assertEqual(cache['bug_private_by_default'], private_bugs)
-
-    def test_product(self):
-        project = self.factory.makeProduct(official_malone=True)
-        user = self.factory.makePerson()
-        login_person(user)
-        view = create_initialized_view(project,
-            '+filebug-reporting-guidelines', principal=user)
-        self._assert_cache_values(view, False, True)
-
-    def test_product_default_private(self):
-        product = self.factory.makeProduct(official_malone=True)
-        removeSecurityProxy(product).private_bugs = True
-        user = self.factory.makePerson()
-        login_person(user)
-        view = create_initialized_view(product,
-            '+filebug-reporting-guidelines', principal=user)
-        self._assert_cache_values(view, True, True)
-
-    def test_product_no_duplicate_search(self):
-        product = self.factory.makeProduct(official_malone=True)
-        removeSecurityProxy(product).enable_bugfiling_duplicate_search = False
-        user = self.factory.makePerson()
-        login_person(user)
-        view = create_initialized_view(product,
-            '+filebug-reporting-guidelines', principal=user)
-        self._assert_cache_values(view, False, False)
-
-    def test_project_group(self):
-        project = self.factory.makeProject()
-        user = self.factory.makePerson()
-        login_person(user)
-        view = create_initialized_view(project,
-            '+filebug-reporting-guidelines', principal=user)
-        self._assert_cache_values(view, False, True)
-
-
 class TestFileBugRequestCache(TestCaseWithFactory):
     # Tests to ensure the request cache contains the expected values for
     # file bug views.
@@ -581,7 +528,7 @@
             'disclosure.enhanced_choice_popup.enabled': 'true'
         }))
 
-    def _assert_cache_values(self, view, duplicate_search, private_only=False):
+    def _assert_cache_values(self, view, duplicate_search, private_bugs=False):
         cache = IJSONRequestCache(view.request).objects
         self.assertEqual(
             duplicate_search, cache['enable_bugfiling_duplicate_search'])
@@ -628,6 +575,9 @@
             bugtask_importance_data.append(new_item)
         self.assertEqual(
             bugtask_importance_data, cache['bugtask_importance_data'])
+        self.assertContentEqual(cache['private_types'], [
+            type.name for type in PRIVATE_INFORMATION_TYPES])
+        self.assertEqual(cache['bug_private_by_default'], private_bugs)
         bugtask_info_type_data = []
         if not IProjectGroup.providedBy(view.context):
             for item in view.context.getAllowedBugInformationTypes():
 
=== removed file 'lib/lp/bugs/templates/bugtarget-filebug-guidelines.pt'
--- lib/lp/bugs/templates/bugtarget-filebug-guidelines.pt	2012-07-05 00:49:00 +0000
+++ lib/lp/bugs/templates/bugtarget-filebug-guidelines.pt	1970-01-01 00:00:00 +0000
@@ -1,51 +0,0 @@
-<tal:root
-        xmlns:tal="http://xml.zope.org/namespaces/tal"
-        xmlns:metal="http://xml.zope.org/namespaces/metal"
-        xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-        >
-
-  <tr id="extra-filebug-details"><td colspan="2" width="100%"><table><tbody>
-  <tr>
-    <metal:bug_reporting_guidelines
-            use-macro="context/@@+filebug-macros/bug_reporting_guidelines" />
-  </tr>
-
-  <tr tal:define="security_context view/getMainContext">
-  <tal:information_type tal:condition="view/is_bug_supervisor">
-    <td colspan="2" width="100%"
-        tal:define="widget nocall: view/widgets/information_type|nothing"
-        tal:condition="widget">
-      <label tal:attributes="for widget/name">
-        This bug contains information that is:
-      </label>
-      <input tal:replace="structure widget" />
-    </td>
-  </tal:information_type>
-  <tal:security_related tal:condition="not: view/is_bug_supervisor">
-      <td colspan="2" width="100%"
-        tal:define="widget nocall: view/widgets/security_related|nothing"
-        tal:condition="widget">
-      <table>
-        <tbody>
-          <tr>
-            <td>
-              <input type="checkbox" tal:replace="structure widget" />
-            </td>
-            <td>
-              <label tal:attributes="for widget/name">
-                This bug is a security vulnerability
-              </label>
-              <div>
-                  The security group for
-                  <tal:security-context content="security_context/displayname" />
-                  will be notified.
-              </div>
-            </td>
-          </tr>
-        </tbody>
-      </table>
-    </td>
-  </tal:security_related>
-  </tr>
-  </tbody></table></td></tr>
-</tal:root>
 
=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt	2012-06-15 16:23:50 +0000
+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt	2012-08-01 01:08:21 +0000
@@ -57,8 +57,50 @@
     <metal:row use-macro="context/@@launchpad_form/widget_row" />
   </metal:description>
 
-  <tr id="extra-filebug-details"
-    tal:replace="structure context/@@+filebug-reporting-guidelines" />
+  <tr id="extra-filebug-details"><td colspan="2" width="100%"><table><tbody>
+  <tr>
+    <metal:bug_reporting_guidelines
+            use-macro="context/@@+filebug-macros/bug_reporting_guidelines" />
+  </tr>
+
+  <tr tal:define="security_context view/getMainContext">
+  <tal:information_type tal:condition="view/is_bug_supervisor">
+    <td colspan="2" width="100%"
+        tal:define="widget nocall: view/widgets/information_type|nothing"
+        tal:condition="widget">
+      <label tal:attributes="for widget/name">
+        This bug contains information that is:
+      </label>
+      <input tal:replace="structure widget" />
+    </td>
+  </tal:information_type>
+  <tal:security_related tal:condition="not: view/is_bug_supervisor">
+      <td colspan="2" width="100%"
+        tal:define="widget nocall: view/widgets/security_related|nothing"
+        tal:condition="widget">
+      <table>
+        <tbody>
+          <tr>
+            <td>
+              <input type="checkbox" tal:replace="structure widget" />
+            </td>
+            <td>
+              <label tal:attributes="for widget/name">
+                This bug is a security vulnerability
+              </label>
+              <div>
+                  The security group for
+                  <tal:security-context content="security_context/displayname" />
+                  will be notified.
+              </div>
+            </td>
+          </tr>
+        </tbody>
+      </table>
+    </td>
+  </tal:security_related>
+  </tr>
+  </tbody></table></td></tr>
 
   <tr>
     <td colspan="2">