Merge lp:~adeuring/launchpad/bug-596944-browser into lp:launchpad/db-devel

[Abel Deuring]

[r=jcsackett,sinzui][ui=none][bug=596944] The forms to configure the bugtracker for products and for DSPs now contains a field enable_bugfiling_duplicate_search; bug filing pages show immediately the complete form if enable_bugfiling_duplicate_search is turned off.

This branch adds a field to enable/disable duplicate searches to the bug tracker configuration forms for distribution source packages and for products.

The template for filing bugs now renders the "main bug report form" immediately if the duplicate search is disabled.

I talked with Deryck about writing unit tests for the changes, but we think it is better toland the branch before PQM closes tomorrow, so I simply extended an existing page test instead.

test: ./bin/test -vvt xx-product-guided-filebug.txt

no lint, excpet complaints about Moin headers in the page test

[j.c.sackett (jcsackett)]

Abel--

This looks like a nice branch.

I would really appreciate it if you could clean up the moin headers to restructured text, since that's the direction we seem to be heading in docs/stories.

Also: this may be just prejudice on my part, but it might be worth getting an RC for this branch and building out the unittests for this. Not a blocker for the branch, but an idea.

Aside from that, thanks for cleaning up some formatting and link text while you were at it.

review: Approve (code*)

[Curtis Hovey (sinzui)]

The additions to the story test are not a story. If I remove the code, the text does not make sense. If I just read the code in the story, I do not know why an admin is making the change for a project he does not own (either the owner does not have permission or we are masking permission issues--do not use admin when unless only an admin can do the task). Users do not read URLs, they read browser title and page content. I personally cannot see how the change is being tests. I do not think a slow story is needed for these changes.

The change was to a view and I expect TestProductBugConfigurationView to be updated to verify the field is present and mutable by the project owner or bug supervisor. I think you could test for html ids to verify the correct template chunks were rendered
    self.assertIsNot(None, find_tag_by_id(view.render(), 'an-id'))

review: Needs Fixing (code)

[Abel Deuring (adeuring)]

I've added unit tests for both variants of the bug reporting form and changed an existing test for the edit form.

review: Resubmit

[Curtis Hovey (sinzui)]

Your test additions are lovely.

Remove the previous additions to lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt. We want to test something once and test it very well. The story additions do not document what the user is trying to accomplish, they are slow to execute, and could contradict the definitive tests when subsequent changes are made.

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2010-11-25 10:18:51 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-12-03 09:00:42 +0000
@@ -163,6 +163,8 @@
         IBugTarget['bug_reporting_guidelines'])
     bug_reported_acknowledgement = copy_field(
         IBugTarget['bug_reported_acknowledgement'])
+    enable_bugfiling_duplicate_search = copy_field(
+        IBugTarget['enable_bugfiling_duplicate_search'])
 
 
 def product_to_productbugconfiguration(product):
@@ -187,12 +189,13 @@
     def field_names(self):
         """Return the list of field names to display."""
         field_names = [
-                "bugtracker",
-                "enable_bug_expiration",
-                "remote_product",
-                "bug_reporting_guidelines",
-                "bug_reported_acknowledgement",
-                ]
+            "bugtracker",
+            "enable_bug_expiration",
+            "remote_product",
+            "bug_reporting_guidelines",
+            "bug_reported_acknowledgement",
+            "enable_bugfiling_duplicate_search",
+            ]
         if check_permission("launchpad.Edit", self.context):
             field_names.extend(["bug_supervisor", "security_contact"])
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_configure.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_configure.py	2010-11-02 21:38:30 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_configure.py	2010-12-03 09:00:42 +0000
@@ -38,6 +38,7 @@
             'field.remote_product': 'sf-boing',
             'field.bug_reporting_guidelines': 'guidelines',
             'field.bug_reported_acknowledgement': 'acknowledgement message',
+            'field.enable_bugfiling_duplicate_search': False,
             'field.actions.change': 'Change',
             }
 
@@ -49,7 +50,8 @@
         fields = [
             'bugtracker', 'enable_bug_expiration', 'remote_product',
             'bug_reporting_guidelines', 'bug_reported_acknowledgement',
-            'bug_supervisor', 'security_contact']
+            'enable_bugfiling_duplicate_search', 'bug_supervisor',
+            'security_contact']
         self.assertEqual(fields, view.field_names)
         self.assertEqual('http://launchpad.dev/boing', view.next_url)
         self.assertEqual('http://launchpad.dev/boing', view.cancel_url)
@@ -63,7 +65,8 @@
         self.assertEqual(label, view.label)
         fields = [
             'bugtracker', 'enable_bug_expiration', 'remote_product',
-            'bug_reporting_guidelines', 'bug_reported_acknowledgement']
+            'bug_reporting_guidelines', 'bug_reported_acknowledgement',
+            'enable_bugfiling_duplicate_search']
         self.assertEqual(fields, view.field_names)
         self.assertEqual('http://launchpad.dev/boing', view.next_url)
         self.assertEqual('http://launchpad.dev/boing', view.cancel_url)
@@ -87,6 +90,7 @@
         self.assertEqual(
             'acknowledgement message',
             self.product.bug_reported_acknowledgement)
+        self.assertFalse(self.product.enable_bugfiling_duplicate_search)
 
     def test_bug_supervisor_invalid(self):
         # Verify that invalid bug_supervisor states are reported.

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2010-12-03 09:00:42 +0000
@@ -7,6 +7,7 @@
 import unittest
 
 from canonical.launchpad.ftests import login
+from canonical.launchpad.testing.pages import find_tag_by_id
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.bugs.browser.bugtarget import FileBugInlineFormView
@@ -257,6 +258,42 @@
             [notification.message
              for notification in view.request.response.notifications])
 
+    def test_bug_filing_view_with_dupe_search_enabled(self):
+        # When a user files a bug for a product where searching for
+        # duplicate bugs is enabled, he is asked to provide a
+        # summary of the bug. This summary is used to find possible
+        # existing duplicates f this bug.
+        product = self.factory.makeProduct()
+        login_person(product.owner)
+        product.official_malone = True
+        product.enable_bugfiling_duplicate_search = True
+        user = self.factory.makePerson()
+        login_person(user)
+        view = create_initialized_view(
+            product, name='+filebug', principal=user)
+        html = view.render()
+        self.assertIsNot(None, find_tag_by_id(html, 'filebug-search-form'))
+        # The main bug filing form is not shown.
+        self.assertIs(None, find_tag_by_id(html, 'filebug-form'))
+
+    def test_bug_filing_view_with_dupe_search_disabled(self):
+        # When a user files a bug for a product where searching for
+        # duplicate bugs is disabled, he can directly enter all
+        # details of the bug.
+        product = self.factory.makeProduct()
+        login_person(product.owner)
+        product.official_malone = True
+        product.enable_bugfiling_duplicate_search = False
+        user = self.factory.makePerson()
+        login_person(user)
+        view = create_initialized_view(
+            product, name='+filebug', principal=user)
+        html = view.render()
+        self.assertIsNot(None, find_tag_by_id(html, 'filebug-form'))
+        # The search form to fing possible duplicates is not shown.
+        self.assertIs(None, find_tag_by_id(html, 'filebug-search-form'))
+
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.makeSuite(TestBugTargetFileBugConfirmationMessage))

=== modified file 'lib/lp/bugs/interfaces/bugtarget.py'
--- lib/lp/bugs/interfaces/bugtarget.py	2010-11-30 21:39:25 +0000
+++ lib/lp/bugs/interfaces/bugtarget.py	2010-12-03 09:00:42 +0000
@@ -299,6 +299,13 @@
 
     enable_bugfiling_duplicate_search = Bool(
         title=u"Search for possible duplicate bugs when a new bug is filed",
+        description=(
+            u"If enabled, Launchpad searches the project for bugs which "
+            u"could match the summary given by the bug reporter. However, "
+            u"this can lead users to mistake an existing bug as the one "
+            u"they want to report. This can happen for example for hardware "
+            u"related bugs where the one symptom can be caused by "
+            u"completely different hardware and drivers."),
         required=False)
 
     def createBug(bug_params):

=== modified file 'lib/lp/bugs/templates/bugtarget-filebug-search.pt'
--- lib/lp/bugs/templates/bugtarget-filebug-search.pt	2010-10-10 21:54:16 +0000
+++ lib/lp/bugs/templates/bugtarget-filebug-search.pt	2010-12-03 09:00:42 +0000
@@ -14,7 +14,8 @@
             tal:define="lp_js string:${icingroot}/build"
             tal:attributes="src string:${lp_js}/bugs/filebug-dupefinder.js"></script>
 
-    <script type="text/javascript">
+    <script type="text/javascript"
+            tal:condition="context/enable_bugfiling_duplicate_search">
         LPS.use(
           'base', 'node', 'oop', 'event', 'lp.bugs.filebug_dupefinder',
           function(Y) {
@@ -51,88 +52,96 @@
         <div class="top-portlet"
             tal:define="launchpad_form_id string:filebug-search-form"
             tal:condition="not:view/extra_data_to_process">
-          <div metal:use-macro="context/@@launchpad_form/form">
-
-        <table metal:fill-slot="widgets">
-
-          <tal:product_widget
-              tal:define="widget nocall:view/widgets/product|nothing"
-              tal:condition="widget">
-            <metal:widget metal:use-macro="context/@@launchpad_form/widget_row" />
-          </tal:product_widget>
-
-          <tal:bugtarget
-              tal:define="widget nocall:view/widgets/bugtarget|nothing"
-              tal:condition="widget">
-            <metal:widget metal:use-macro="context/@@launchpad_form/widget_row" />
-          </tal:bugtarget>
-
-          <tal:hidden_tags tal:replace="structure view/widgets/tags/hidden" />
-
-          <tr>
-            <td colspan="2">
-              <p>
-                Please describe the bug in a few words, for example, "weather
-                applet crashes on logout":
-              </p>
-            </td>
-          </tr>
-          <tal:title_widget tal:define="widget nocall:view/widgets/title">
-            <tal:comment replace="nothing">
-              The desire to have more control over the styling of this widget
-              prevents us from using the widget_row macro here.
-            </tal:comment>
-            <tr>
-              <td tal:define="field_name widget/context/__name__;
-                              error python:view.getFieldError(field_name);
-                              error_class python:('error' if error else None);"
-                  tal:attributes="class error_class"
-                  style="text-align: left; padding-left: 5em;">
-                <div>
-                  <label tal:attributes="for widget/name"
-                         tal:content="string:${widget/label}:">Label</label>
-                  <input tal:replace="structure widget" />
-                </div>
-                <div class="message" tal:condition="error"
-                     tal:content="structure error">Error message</div>
-                <p class="formHelp"
-                   tal:condition="widget/hint"
-                   tal:content="widget/hint">Some Help Text
-                </p>
-              </td>
-              <td style="text-align: left;">
-                <input tal:replace="structure view/actions/field.actions.search/render" />
-                <span id="spinner" style="text-align: center" class="unseen">
-                  <img src="/@@/spinner" />
-                </span>
-              </td>
-            </tr>
-          </tal:title_widget>
-        </table>
-
-        <div metal:fill-slot="buttons">
-          <tal:comment replace="nothing">
-            We add this to hide the standard action buttons.
-          </tal:comment>
-        </div>
-      </div>
-    </div>
-
-    <div id="possible-duplicates" style="text-align: left;">
-    </div>
-    <div id="filebug-form-container" style="display: none;">
-    </div>
-
-    <p style="display: none">
-      <a id="filebug-base-url"
-          tal:attributes="href view/inline_filebug_base_url"></a>
-      <a id="filebug-form-url"
-          tal:attributes="href view/inline_filebug_form_url"></a>
-      <a id="duplicate-search-url"
-          tal:attributes="href view/duplicate_search_url"></a>
-    </p>
-    </tal:uses-malone>
-  </div>
+          <div tal:condition="not: context/enable_bugfiling_duplicate_search"
+               omit-tag="">
+            <metal:no-dupe-form
+                metal:use-macro="context/@@+filebug-macros/simple-filebug-form" />
+          </div>
+
+          <div tal:condition="context/enable_bugfiling_duplicate_search">
+            <div metal:use-macro="context/@@launchpad_form/form">
+
+              <table metal:fill-slot="widgets">
+
+                <tal:product_widget
+                    tal:define="widget nocall:view/widgets/product|nothing"
+                    tal:condition="widget">
+                  <metal:widget metal:use-macro="context/@@launchpad_form/widget_row" />
+                </tal:product_widget>
+
+                <tal:bugtarget
+                    tal:define="widget nocall:view/widgets/bugtarget|nothing"
+                    tal:condition="widget">
+                  <metal:widget metal:use-macro="context/@@launchpad_form/widget_row" />
+                </tal:bugtarget>
+
+                <tal:hidden_tags tal:replace="structure view/widgets/tags/hidden" />
+
+                <tr>
+                  <td colspan="2">
+                    <p>
+                      Please describe the bug in a few words, for example, "weather
+                      applet crashes on logout":
+                    </p>
+                  </td>
+                </tr>
+                <tal:title_widget tal:define="widget nocall:view/widgets/title">
+                  <tal:comment replace="nothing">
+                    The desire to have more control over the styling of this widget
+                    prevents us from using the widget_row macro here.
+                  </tal:comment>
+                  <tr>
+                    <td tal:define="field_name widget/context/__name__;
+                                    error python:view.getFieldError(field_name);
+                                    error_class python:('error' if error else None);"
+                        tal:attributes="class error_class"
+                        style="text-align: left; padding-left: 5em;">
+                      <div>
+                        <label tal:attributes="for widget/name"
+                               tal:content="string:${widget/label}:">Label</label>
+                        <input tal:replace="structure widget" />
+                      </div>
+                      <div class="message" tal:condition="error"
+                           tal:content="structure error">Error message</div>
+                      <p class="formHelp"
+                         tal:condition="widget/hint"
+                         tal:content="widget/hint">Some Help Text
+                      </p>
+                    </td>
+                    <td style="text-align: left;">
+                      <input tal:replace="structure view/actions/field.actions.search/render" />
+                      <span id="spinner" style="text-align: center" class="unseen">
+                        <img src="/@@/spinner" />
+                      </span>
+                    </td>
+                  </tr>
+                </tal:title_widget>
+              </table>
+
+              <div metal:fill-slot="buttons">
+                <tal:comment replace="nothing">
+                  We add this to hide the standard action buttons.
+                </tal:comment>
+              </div>
+            </div>
+          </div>
+        </div>
+
+        <div id="possible-duplicates" style="text-align: left;">
+        </div>
+        <div id="filebug-form-container" style="display: none;">
+        </div>
+
+        <p style="display: none">
+          <a id="filebug-base-url"
+              tal:attributes="href view/inline_filebug_base_url"></a>
+          <a id="filebug-form-url"
+              tal:attributes="href view/inline_filebug_form_url"></a>
+          <a id="duplicate-search-url"
+              tal:attributes="href view/duplicate_search_url"></a>
+        </p>
+      </tal:uses-malone>
+    </div>
 
   </div>
 

=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py	2010-11-19 03:11:05 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py	2010-12-03 09:00:42 +0000
@@ -133,7 +133,7 @@
         """Edit the details of this source package."""
         # This is titled "Edit bug reporting guidelines" because that
         # is the only editable property of a source package right now.
-        return Link('+edit', 'Edit bug reporting guidelines', icon='edit')
+        return Link('+edit', 'Configure bug tracker', icon='edit')
 
     def new_bugs(self):
         base_path = "+bugs"
@@ -544,6 +544,7 @@
     field_names = [
         'bug_reporting_guidelines',
         'bug_reported_acknowledgement',
+        'enable_bugfiling_duplicate_search',
         ]
 
     @property

=== modified file 'lib/lp/registry/browser/tests/distributionsourcepackage-views.txt'
--- lib/lp/registry/browser/tests/distributionsourcepackage-views.txt	2010-11-17 00:21:51 +0000
+++ lib/lp/registry/browser/tests/distributionsourcepackage-views.txt	2010-12-03 09:00:42 +0000
@@ -199,7 +199,8 @@
 The view allows the user the set the bug_reporting_guidelines field.
 
     >>> view.field_names
-    ['bug_reporting_guidelines', 'bug_reported_acknowledgement']
+    ['bug_reporting_guidelines', 'bug_reported_acknowledgement',
+    'enable_bugfiling_duplicate_search']
 
     >>> print package.bug_reporting_guidelines
     None