Merge lp:~sinzui/launchpad/upstream-downstream-bug-links-0 into lp:launchpad/db-devel

[Curtis Hovey]

This is my branch to add reciprocal links between packages and upstream
projects.

    lp:~sinzui/launchpad/upstream-downstream-bug-links-0
    Diff size: 404
    Launchpad bug:
        https://bugs.launchpad.net/bugs/3152
    Test command: ./bin/test -vv \
        -t TestBugTaskSearchListingView -t bugtask-searches
    Pre-implementation: Edwin, Deryck
    Target release: 10.12

Add reciprocal links between packages and upstream projects
-----------------------------------------------------------

The bug search page should contain a notice that bugs are also tracked
in a downstream package or upstream project. Contributors can search the
alternate project or package when investigating bugs.

This link already appears on project pages when the project does not use
Lp to track bugs. The link should also appear when the project uses Lp.
The package page needs the reciprocating link.

This implementation meets the requirements from bridging the gap, but I
am dissatisfied with it. There is an open bug that could be fixed in
another branch to address the awkward experience I had using this feature
in dev: When my search fails, and I see that there is an upstream/downstream
location to search, I expect the link to perform my search. re-typing text
is not difficult, but setting up an advanced search is :( The work to fix the
bug and two related bugs are intrinsic to the search form--the solution is
not obvious.

Screenshots
    http://people.canonical.com/~curtis/downstream-bugs.png
    http://people.canonical.com/~curtis/upstream-bugs.png
    http://people.canonical.com/~curtis/upstream-bugs-searched.png

Rules
-----

    * Refactor the views and templates so that the also-in-ubuntu block
      is available on the project bug search page.
    * Add and also-in-upstream block for DSP and SP pages.

QA
--

    * Visit https://bugs.launchpad.net/jokosher
    * Verify their is a message that bugs are also tracked in Ubuntu.
    * Follow the link to Ubuntu Jokosher bug page.
    * Verify there is a message that bugs are also tracked in Jokosher
      and there is a link back.

Lint
----

Linting changed files:
  lib/canonical/launchpad/icing/style.css
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/tests/test_buglisting.py
  lib/lp/bugs/templates/bugtarget-bugs.pt
  lib/lp/bugs/templates/bugtarget-macros-search.pt

Test
----

Added tests for the 3 moved/added properties, and verified the also-in-ubuntu
and also-in-upstream blocks in the markup.
    lib/lp/bugs/browser/tests/test_buglisting.py

Implementation
--------------

Extracted the external_bugtracker() method and also-in-ubuntu markup to a
base class and macro template so that they can be reused.
    lib/lp/bugs/browser/bugtarget.py
    lib/lp/bugs/templates/bugtarget-bugs.pt

Added has_bugtracker() and upstream_launchpad_project() to support the
cases in the template that need to show also-in-ubuntu and also-in-upstream.
The template markup needed revision to support presenting the additional
information with proper spacing. This change also removed the exceptional
CSS used by the search form when a search has not been performed.
    lib/lp/bugs/browser/bugtask.py
    lib/lp/bugs/templates/bugtarget-macros-search.pt

Removed obsolete style that changed the color of the text.
    lib/canonical/launchpad/icing/style.css

[Robert Collins (lifeless)]

Some things you may have already thought of that reading this prompted me to think about. Not really issues with your patch, just things we should do/address/think about.

Firstly, for a derived distro, the 'upstream' is the parent distro; for a package in a derived distro the upstream is that package in the parent distro if it exists, or the upstream otherwise. Or perhaps we should supply all the 'upstreams' at once. Similarly downstream:
product -> [0..N] distro packages.

We probably want a unified search, rather than folk having to manually search again and again and again to consider all locations (once they choose to expand their context, that is).

Duplicate detection will definitely want a unified backend search.

Your tests have a mixin - thats ok, but I think you'll find that one of:
 - using a Fixture
 - improving the Launchpad default Factory

is better: your mixin has only self.factory as state, so really doesn't need to be a mixin at all.

review: Approve

[Curtis Hovey (sinzui)]

On Sun, 2010-11-21 at 05:36 +0000, Robert Collins wrote:
> Firstly, for a derived distro, the 'upstream' is the parent distro;
> for a package in a derived distro the upstream is that package in the
> parent distro if it exists, or the upstream otherwise. Or perhaps we
> should supply all the 'upstreams' at once. Similarly downstream:
> product -> [0..N] distro packages.
>
> We probably want a unified search, rather than folk having to manually
> search again and again and again to consider all locations (once they
> choose to expand their context, that is).

Both these issues came up in UDS. The work was deemed to be out of scope
for bridging-the-gap. We recognise that doing the work for the user is
better than telling users where they can do more work. Adding the link
make the page consistent, but I do not think this is a great usability
improvement.

My conversations with mpt and barry makes me believe ubuntu contributors
want an option to expand the search of a bug target.
    [x] include bugs reported in upstream jokosher
    [x] include bugs reported in derived distributions

--
__Curtis C. Hovey_________
http://launchpad.net/

[Robert Collins (lifeless)]

That sounds nice.

-Rob

[Paul Hummer (rockstar)]

I think this is fine, but I think the better UI would be to have bugs from the sourcepackage show up in search for the project, and bugs from the projects sow up in the search for the sourcepackage.

I'm sure that will probably have some mega database issues, so there's a technical reason not to do that, but I think the idea has merit for the future.

review: Approve (ui)

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2010-10-15 01:48:05 +0000
+++ lib/canonical/launchpad/icing/style.css	2010-11-23 21:26:54 +0000
@@ -655,21 +655,6 @@
   color: #3840be;
 }
 
-.search-box {
-  text-align: left;
-  float: left;
-  border:1px solid #d6d6d6;
-  margin: 0.5em 0 0.5em 0;
-  color: #717171;
-  padding: 8px;
-  -moz-border-radius: 5px;
-  -o-border-radius: 5px;
-  -webkit-border-radius: 5px;
-}
-
-.search-box form.primary.search {
-  margin: 0.5em;
-}
 
 /* ====== Content area styles ====== */
 

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2010-11-02 21:38:30 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-11-23 21:26:54 +0000
@@ -59,7 +59,6 @@
     )
 from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
 from canonical.launchpad.interfaces.launchpad import (
-    IHasExternalBugTracker,
     ILaunchpadCelebrities,
     )
 from canonical.launchpad.searchbuilder import any
@@ -1275,18 +1274,6 @@
         return service_usage.bug_tracking_usage
 
     @property
-    def external_bugtracker(self):
-        """External bug tracking system designated for the context.
-
-        :returns: `IBugTracker` or None
-        """
-        has_external_bugtracker = IHasExternalBugTracker(self.context, None)
-        if has_external_bugtracker is None:
-            return None
-        else:
-            return has_external_bugtracker.getExternalBugTracker()
-
-    @property
     def bugtracker(self):
         """Description of the context's bugtracker.
 

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-10-31 20:18:45 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-11-23 21:26:54 +0000
@@ -130,7 +130,10 @@
     BugTargetLatestBugsFeedLink,
     FeedsMixin,
     )
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.launchpad import (
+    IHasExternalBugTracker,
+    ILaunchpadCelebrities,
+    )
 from canonical.launchpad.interfaces.validation import (
     valid_upstreamtask,
     validate_distrotask,
@@ -2300,6 +2303,52 @@
         service_usage = IServiceUsage(self.context)
         return service_usage.bug_tracking_usage
 
+    @cachedproperty
+    def external_bugtracker(self):
+        """External bug tracking system designated for the context.
+
+        :returns: `IBugTracker` or None
+        """
+        has_external_bugtracker = IHasExternalBugTracker(self.context, None)
+        if has_external_bugtracker is None:
+            return None
+        else:
+            return has_external_bugtracker.getExternalBugTracker()
+
+    @property
+    def has_bugtracker(self):
+        """Does the `IBugTarget` have a bug tracker or use Launchpad?"""
+        usage = IServiceUsage(self.context)
+        uses_lp = usage.bug_tracking_usage == ServiceUsage.LAUNCHPAD
+        if self.external_bugtracker or uses_lp:
+            return True
+        return False
+
+    @property
+    def upstream_launchpad_project(self):
+        """The linked upstream `IProduct` for the package.
+
+        If this `IBugTarget` is a `IDistributionSourcePackage` or an
+        `ISourcePackage` and it is linked to an upstream project that uses
+        Launchpad to track bugs, return the `IProduct`. Otherwise,
+        return None
+
+        :returns: `IProduct` or None
+        """
+        if self._sourcePackageContext():
+            sp = self.context
+        elif self._distroSourcePackageContext():
+            sp = self.context.development_version
+        else:
+            sp = None
+        if sp is not None:
+            packaging = sp.packaging
+            if packaging is not None:
+                product = packaging.productseries.product
+                if product.bug_tracking_usage == ServiceUsage.LAUNCHPAD:
+                    return product
+        return None
+
     @property
     def page_title(self):
         return "Bugs in %s" % self.context.title

=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py	2010-10-06 22:05:15 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py	2010-11-23 21:26:54 +0000
@@ -3,19 +3,28 @@
 
 __metaclass__ = type
 
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.testing.pages import (
     extract_text,
     find_tag_by_id,
     find_tags_by_class,
     )
-from canonical.launchpad.webapp import canonical_url
-from canonical.testing.layers import LaunchpadFunctionalLayer
-from lp.testing import BrowserTestCase
-
-
-class TestBugTaskSearchListingView(BrowserTestCase):
-
-    layer = LaunchpadFunctionalLayer
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    BrowserTestCase,
+    login_person,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class TestBugTaskSearchListingPage(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
 
     def _makeDistributionSourcePackage(self):
         distro = self.factory.makeDistribution('test-distro')
@@ -120,3 +129,126 @@
         self.assertIs(None,
                       find_tag_by_id(browser.contents, 'portlet-tags'),
                       "portlet-tags should not be shown.")
+
+
+class BugTargetTestCase(TestCaseWithFactory):
+    """Test helpers for setting up `IBugTarget` tests."""
+
+    def _makeBugTargetProduct(self, bug_tracker=None, packaging=False):
+        """Return a product that may use Launchpad or an external bug tracker.
+
+        bug_tracker may be None, 'launchpad', or 'external'.
+        """
+        product = self.factory.makeProduct()
+        if bug_tracker is not None:
+            with person_logged_in(product.owner):
+                if bug_tracker == 'launchpad':
+                    product.official_malone = True
+                else:
+                    product.bugtracker = self.factory.makeBugTracker()
+        if packaging:
+            self.factory.makePackagingLink(
+                productseries=product.development_focus, in_ubuntu=True)
+        return product
+
+
+class TestBugTaskSearchListingViewProduct(BugTargetTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_external_bugtracker_is_none(self):
+        bug_target = self._makeBugTargetProduct()
+        view = create_initialized_view(bug_target, '+bugs')
+        self.assertEqual(None, view.external_bugtracker)
+
+    def test_external_bugtracker(self):
+        bug_target = self._makeBugTargetProduct(bug_tracker='external')
+        view = create_initialized_view(bug_target, '+bugs')
+        self.assertEqual(bug_target.bugtracker, view.external_bugtracker)
+
+    def test_has_bugtracker_is_false(self):
+        bug_target = self.factory.makeProduct()
+        view = create_initialized_view(bug_target, '+bugs')
+        self.assertEqual(False, view.has_bugtracker)
+
+    def test_has_bugtracker_external_is_true(self):
+        bug_target = self._makeBugTargetProduct(bug_tracker='external')
+        view = create_initialized_view(bug_target, '+bugs')
+        self.assertEqual(True, view.has_bugtracker)
+
+    def test_has_bugtracker_launchpad_is_true(self):
+        bug_target = self._makeBugTargetProduct(bug_tracker='launchpad')
+        view = create_initialized_view(bug_target, '+bugs')
+        self.assertEqual(True, view.has_bugtracker)
+
+    def test_product_without_packaging_also_in_ubuntu_is_none(self):
+        bug_target = self._makeBugTargetProduct(bug_tracker='launchpad')
+        login_person(bug_target.owner)
+        view = create_initialized_view(
+            bug_target, '+bugs', principal=bug_target.owner)
+        self.assertEqual(None, find_tag_by_id(view(), 'also-in-ubuntu'))
+
+    def test_product_with_packaging_also_in_ubuntu(self):
+        bug_target = self._makeBugTargetProduct(
+            bug_tracker='launchpad', packaging=True)
+        login_person(bug_target.owner)
+        view = create_initialized_view(
+            bug_target, '+bugs', principal=bug_target.owner)
+        content = find_tag_by_id(view.render(), 'also-in-ubuntu')
+        link = canonical_url(
+            bug_target.ubuntu_packages[0], force_local_path=True)
+        self.assertEqual(link, content.a['href'])
+
+
+class TestBugTaskSearchListingViewDSP(BugTargetTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def _getBugTarget(self, obj):
+        """Return the `IBugTarget` under test.
+
+        Return the object that was passed. Sub-classes can redefine
+        this method.
+        """
+        return obj
+
+    def test_package_with_upstream_launchpad_project(self):
+        upstream_project = self._makeBugTargetProduct(
+            bug_tracker='launchpad', packaging=True)
+        login_person(upstream_project.owner)
+        bug_target = self._getBugTarget(
+            upstream_project.distrosourcepackages[0])
+        view = create_initialized_view(
+            bug_target, '+bugs', principal=upstream_project.owner)
+        self.assertEqual(upstream_project, view.upstream_launchpad_project)
+        content = find_tag_by_id(view.render(), 'also-in-upstream')
+        link = canonical_url(upstream_project, rootsite='bugs')
+        self.assertEqual(link, content.a['href'])
+
+    def test_package_with_upstream_nonlaunchpad_project(self):
+        upstream_project = self._makeBugTargetProduct(packaging=True)
+        login_person(upstream_project.owner)
+        bug_target = self._getBugTarget(
+            upstream_project.distrosourcepackages[0])
+        view = create_initialized_view(
+            bug_target, '+bugs', principal=upstream_project.owner)
+        self.assertEqual(None, view.upstream_launchpad_project)
+        self.assertEqual(None, find_tag_by_id(view(), 'also-in-upstream'))
+
+    def test_package_without_upstream_project(self):
+        observer = self.factory.makePerson()
+        dsp = self.factory.makeDistributionSourcePackage(
+            'test-dsp', distribution=getUtility(ILaunchpadCelebrities).ubuntu)
+        bug_target = self._getBugTarget(dsp)
+        login_person(observer)
+        view = create_initialized_view(
+            bug_target, '+bugs', principal=observer)
+        self.assertEqual(None, view.upstream_launchpad_project)
+        self.assertEqual(None, find_tag_by_id(view(), 'also-in-upstream'))
+
+
+class TestBugTaskSearchListingViewSP(TestBugTaskSearchListingViewDSP):
+
+        def _getBugTarget(self, dsp):
+            """Return the current `ISourcePackage` for the dsp."""
+            return dsp.development_version

=== modified file 'lib/lp/bugs/stories/bugs/xx-front-page-info.txt'
--- lib/lp/bugs/stories/bugs/xx-front-page-info.txt	2010-09-14 13:25:50 +0000
+++ lib/lp/bugs/stories/bugs/xx-front-page-info.txt	2010-11-23 21:26:54 +0000
@@ -69,18 +69,18 @@
     >>> bug = factory.makeBug(product=uses_malone)
     >>> logout()
     >>> anon_browser.open('http://bugs.launchpad.dev/uses-malone')
-    >>> print extract_text(
-    ...     find_tags_by_class(anon_browser.contents, 'search-box')[0])
+    >>> content = find_main_content(anon_browser.contents)
+    >>> print extract_text(find_tag_by_id(content, 'simple-bug-search'))
     by importance
     by status
     ...
     Advanced search
 
 
-    >>> print find_tag_by_id(anon_browser.contents, 'no-bugs-filed')
+    >>> print find_tag_by_id(content, 'no-bugs-filed')
     None
 
-    >>> hot_bugs = find_tag_by_id(anon_browser.contents, 'hot-bugs')
+    >>> hot_bugs = find_tag_by_id(content, 'hot-bugs')
     >>> print extract_text(hot_bugs)
     Summary   Status  Importance   Last changed
     #16  ...  New     Undecided    ...
@@ -95,18 +95,17 @@
     >>> logout()
 
     >>> anon_browser.open('http://bugs.launchpad.dev/uses-malone')
-    >>> print find_tag_by_id(anon_browser.contents, 'hot-bugs')
+    >>> content = find_main_content(anon_browser.contents)
+    >>> print find_tag_by_id(content, 'hot-bugs')
     None
 
-    >>> bug_list = find_tag_by_id(
-    ...     anon_browser.contents, 'no-bugs-filed')
+    >>> bug_list = find_tag_by_id(content, 'no-bugs-filed')
     >>> print extract_text(bug_list)
     There are currently no open bugs filed against Uses-malone.
 
 But since the project has a bug, the search box is still visible.
 
-    >>> print extract_text(
-    ...     find_tags_by_class(anon_browser.contents, 'search-box')[0])
+    >>> print extract_text(find_tag_by_id(content, 'simple-bug-search'))
     by importance
     by status
     ...

=== modified file 'lib/lp/bugs/templates/bugtarget-bugs.pt'
--- lib/lp/bugs/templates/bugtarget-bugs.pt	2010-09-13 21:25:14 +0000
+++ lib/lp/bugs/templates/bugtarget-bugs.pt	2010-11-23 21:26:54 +0000
@@ -101,7 +101,7 @@
       <tal:uses_launchpad_bugtracker
         condition="view/bug_tracking_usage/enumvalue:LAUNCHPAD">
       <tal:has_bugtasks condition="context/has_bugtasks">
-        <div class="search-box" style="margin-bottom:2em">
+        <div id="simple-bug-search" class="portlet-top" style="margin-bottom:2em">
           <metal:search
             use-macro="context/@@+bugtarget-macros-search/simple-search-form"
           />
@@ -111,7 +111,7 @@
         --></script>
       </tal:has_bugtasks>
 
-      <tal:has_hot_bugs condition="view/hot_bugs_info/bugtasks">
+      <div class="portlet" tal:condition="view/hot_bugs_info/bugtasks">
         <h2>Hot bugs</h2>
 
         <table class="listing" id="hot-bugs"
@@ -157,7 +157,7 @@
            tal:condition="view/hot_bugs_info/has_more_bugs">
           <a tal:attributes="href string:${context/fmt:url/+bugs}?orderby=-heat&field.status%3Alist=NEW&field.status%3Alist=INCOMPLETE_WITH_RESPONSE&field.status%3Alist=INCOMPLETE_WITHOUT_RESPONSE&field.status%3Alist=CONFIRMED&field.status%3Alist=TRIAGED&field.status%3Alist=INPROGRESS&field.status%3Alist=FIXCOMMITTED&field.omit_dupes=on">See all hot bugs</a>
         </p>
-      </tal:has_hot_bugs>
+      </div>
 
       <tal:no_hot_bugs condition="not: view/hot_bugs_info/bugtasks">
         <p id="no-bugs-filed"><strong>There are currently no
@@ -190,17 +190,8 @@
 
     <tal:also_in_ubuntu
       condition="not: view/bug_tracking_usage/enumvalue:LAUNCHPAD">
-      <p tal:define="packages context/ubuntu_packages | nothing"
-         tal:condition="packages"
-         id="also-in-ubuntu">
-        Ubuntu
-        <tal:also condition="view/external_bugtracker">also</tal:also>
-        tracks bugs for packages derived from this project:
-        <tal:packages repeat="package packages">
-          <span style="white-space: nowrap"
-                tal:content="structure package/fmt:link" /><tal:comma
-          condition="not:repeat/package/end">,</tal:comma></tal:packages>.
-      </p>
+      <metal:also-in-ubuntu
+        use-macro="context/@@+bugtarget-macros-search/also-in-ubuntu" />
     </tal:also_in_ubuntu>
 
     <div

=== modified file 'lib/lp/bugs/templates/bugtarget-macros-search.pt'
--- lib/lp/bugs/templates/bugtarget-macros-search.pt	2010-01-19 22:34:00 +0000
+++ lib/lp/bugs/templates/bugtarget-macros-search.pt	2010-11-23 21:26:54 +0000
@@ -3,6 +3,21 @@
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   omit-tag="">
 
+<metal:block define-macro="also-in-ubuntu">
+  <p id="also-in-ubuntu"
+    tal:define="packages context/ubuntu_packages | nothing"
+    tal:condition="packages">
+    Ubuntu
+    <tal:also condition="view/has_bugtracker">also</tal:also>
+    tracks bugs for packages derived from this project:
+    <tal:packages repeat="package packages">
+      <span style="white-space: nowrap"
+            tal:content="structure package/fmt:link" /><tal:comma
+      condition="not:repeat/package/end">,</tal:comma></tal:packages>.
+  </p>
+</metal:block>
+
+
 <metal:block define-macro="sortwidget">
   <tal:comment condition="nothing">
     This macro expects that the callsite's view class has a shouldShowTargetName
@@ -65,6 +80,7 @@
 <metal:block define-macro="simple-search-form">
 <form method="get" name="search" class="primary search"
       tal:attributes="action search_url|string:">
+  <p>
   <tal:searchbox replace="structure view/widgets/searchtext" />
   <metal:widget use-macro="context/@@+bugtarget-macros-search/sortwidget" />
   <input type="submit" name="search" value="Search" />
@@ -77,9 +93,22 @@
   <tal:widget replace="structure view/widgets/has_patch/hidden" />
   <tal:widget replace="structure view/widgets/component/hidden" />
   <tal:widget replace="structure view/widgets/has_no_package/hidden" />
-  <br /><br />
+  </p>
+  <p>
   <a tal:attributes="href advanced_search_url|string:?advanced=1"
   >Advanced search</a>
+  </p>
+
+  <metal:also-in-ubuntu
+    use-macro="context/@@+bugtarget-macros-search/also-in-ubuntu" />
+
+  <p id="also-in-upstream"
+    tal:define="product view/upstream_launchpad_project | nothing"
+    tal:condition="product">
+    <a tal:replace="structure product/fmt:link:bugs" /> also
+    tracks bugs for this package.
+  </p>
+
   <div metal:define-slot="extra-search-widgets">
   </div>
 </form>