Merge lp:~bac/launchpad/bug-490521 into lp:launchpad

[Brad Crittenden]

Change source package associations portlet to give an indication of whether various upstream attributes (bug tracker, bug supervisor, series branch, etc) have been configured.

[Brad Crittenden (bac)]

= Summary =

For a package that has been connected to an upstream project an indicator for the
different attributes is shown in the "Upstream connections" portlet.

== Proposed fix ==

Include the new 'yes-no' TALES macro written by Curtis and use it to visually
indicate whether bug supervisor, bug tracker, and the branch for the series are
populated.

== Pre-implementation notes ==

Lots of discussions with Curtis and Edwin.

== Implementation details ==

Hack in hard-coded uses of the new macro. If this UI design is accepted after
testing on edge it will be refactored into a new view so that the functionality can
be used elsewhere.

== Tests ==

bin/test -vvm lp.registry -t xx-sourcepackage-packaging.txt \
   -t sourcepackage-views.txt

== Demo and Q/A ==

See it in action at https://launchpad.dev/ubuntu/hoary/+source/netapplet

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/sourcepackage.py
  lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt
  lib/lp/app/templates/base-layout-macros.pt
  lib/lp/registry/browser/tests/sourcepackage-views.txt
  lib/lp/registry/templates/sourcepackage-portlet-associations.pt

== Pylint notices ==

lib/lp/registry/browser/sourcepackage.py
    29: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)

[Curtis Hovey (sinzui)]

...

> === modified file 'lib/lp/registry/browser/sourcepackage.py'
> --- lib/lp/registry/browser/sourcepackage.py 2010-01-29 15:03:49 +0000
> +++ lib/lp/registry/browser/sourcepackage.py 2010-02-11 20:08:17 +0000

...

> @@ -349,3 +348,22 @@
> 'The project %s was linked to this source package.' %
> upstream.displayname)
> self.next_url = self.request.getURL()
> +
> + @property
> + def has_bugtracker(self):
> + """Does the product have a bugtracker set?"""
> + def getTrackerName(bugtracker):
> + if bugtracker is None:
> + return False
> + else:
> + return True
> + product = self.context.productseries.product
> + if product.official_malone:
> + return True
> + bugtracker = product.bugtracker
> + if bugtracker is None:
> + if product.project is not None:
> + bugtracker = product.project.bugtracker
> + if bugtracker is None:
> + return False
> + return True

I see that I underestimated the complexity of this connection. I think this
needs better test coverage. I think a test of a new factory-made product
that gets a project without a bug tracker will work. Change the
official_malone and bug tracker a few times to show that has_bugtracker
returns the expected state.

...

> === modified file 'lib/lp/registry/templates/sourcepackage-portlet-associations.pt'
> --- lib/lp/registry/templates/sourcepackage-portlet-associations.pt 2010-02-01 13:13:23 +0000
> +++ lib/lp/registry/templates/sourcepackage-portlet-associations.pt 2010-02-11 20:08:17 +0000
> @@ -5,37 +5,51 @@
> omit-tag="">
>
> <div id="portlet-associations">
> - <h2>Upstream associations</h2>
> + <h2>Upstream connections</h2>

I am considering a similar portlet on the project page:
    Contributor connections

Is the title on this portlet clearer if it is:
    Upstream contributor connections

...

> + <dl>
> + <dt>
> + <a tal:replace="structure series/fmt:link" />
> + </dt>

We lost the link to change the linked series. This is needed by about 50
packages each release when the package is built from a new stable series.

We have a choice here: add an edit icon here:

    <a tal:replace="structure context/menu:overview/edit_packaging/fmt:icon" />

But I think the action will be more clear if we move it below because the user
may want to change the /project/ and series. See the end of this review.

> + <dd title="Series branch"
> + tal:define="bool series/branch">
> + Branch:
> + <metal:yes-no
> + use-macro="context/@@+base-layout-macros/yes-no" />
> + </dd>
> + </dl>

As I said on IRC, we need to show translations too...
...and you just showed me your fix in IRC. Fab.

> </div>
>
> <ul class="horizontal">

I think we should move the link to edit the Packaging link to this list
where we can use a clear label: Change upstream project and series. As
we agreed on the phone, if the link should be different if the upstream
had never been s...

review: Needs Fixing (code and ui)

[Brad Crittenden (bac)]

Curtis,

I have made all of the requested changes except changing the portlet title. It doesn't seem clearer to me but we can discuss it more before landing.

The incremental diff is at: http://pastebin.ubuntu.com/374657/

Thanks for the review and the great suggestions.

[Curtis Hovey (sinzui)]

> === modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
> --- lib/lp/registry/browser/tests/sourcepackage-views.txt 2010-02-11 19:55:34 +0000
> +++ lib/lp/registry/browser/tests/sourcepackage-views.txt 2010-02-11 23:29:36 +0000
...

> +So let's set the product series so we can do more interesting testing.
> +
> + >>> login_person(product.owner)
> + >>> form = {
> + ... 'field.productseries': 'stinky/stinkyseries',
> + ... 'field.actions.change': 'Change',
> + ... }
> + >>> view = create_initialized_view(
> + ... package, name='+edit-packaging', form=form,
> + ... principal=product.owner)
> + >>> view.errors
> + []
> + >>> print package.productseries.name
> + stinkyseries

This is duplicating a test from packaging-views and will break if we change
that form. Just create a packaging link so that we can focus on the property.

    package.setPackaging(productseries, product.owner)

> +If a product is not part of a project group and its bug tracker is not
> +set then the view property is false.
> +
> + >>> view = create_initialized_view(package, name='+portlet-associations')
> +
> + >>> print product.official_malone
> + False
> + >>> print product.bugtracker
> + None
> + >>> print view.has_bugtracker
> + False
> +
> +Having official_malone set results in has_bugtracker being true.
> +
> + >>> product.official_malone = True
> + >>> print view.has_bugtracker
> + True
> +
> +Having a bug_tracker set also results in has_bugtracker being true (a
> +bit of a tautology you'd think).
> +
> + >>> product.official_malone = False
> + >>> from zope.component import getUtility
> + >>> from canonical.launchpad.interfaces import IBugTrackerSet
> + >>> product.bugtracker = getUtility(IBugTrackerSet)['mozilla.org']
> + >>> print view.has_bugtracker
> + True

This is easier. Graham got tired of looking up trackers in his tests.

    product.bugtracker = factory.makeBugTracker()

...

> === modified file 'lib/lp/registry/templates/sourcepackage-portlet-associations.pt'
> --- lib/lp/registry/templates/sourcepackage-portlet-associations.pt 2010-02-11 19:55:34 +0000
> +++ lib/lp/registry/templates/sourcepackage-portlet-associations.pt 2010-02-11 22:12:10 +0000
> @@ -48,16 +48,30 @@
> <metal:yes-no
> use-macro="context/@@+base-layout-macros/yes-no" />
> </dd>
> + <dd title="Series translations auto import"
> + tal:condition="context/getTranslationTemplates"
> + tal:define="bool not:series/translations_autoimport_mode/enumvalue:NO_IMPORT">
> + Translations:
> + <metal:yes-no
> + use-macro="context/@@+base-layout-macros/yes-no" />
> + </dd>
> </dl>
>
> </div>
>
> <ul class="horizontal">
> <li>
> - <a
> - tal:attributes="href series/product/menu:overview/packages/fmt:url"
> + <a tal:attributes="href series/product/menu:overview/packages/fmt:url;
> + class string:sprite info"
> >Show upstr...

review: Approve (code and ui)

[Brad Crittenden (bac)]

And another diff at http://pastebin.ubuntu.com/374736/

Off to land now. Thanks for feedback.

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-02-11 20:44:48 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-02-16 17:50:30 +0000
@@ -410,4 +410,11 @@
     condition="python: count != 1"
     replace="l_plural" />
 </metal:plural-msg>
+
+<metal:yes-no define-macro="yes-no">
+  <span tal:condition="bool"
+    class="sprite yes">&nbsp;<span class="invisible-link">yes</span></span>
+  <span tal:condition="not: bool"
+    class="sprite no">&nbsp;<span class="invisible-link">no</span></span>
+</metal:yes-no>
 </macros>

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2010-01-29 15:03:49 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2010-02-16 17:50:30 +0000
@@ -28,7 +28,6 @@
 
 from lazr.restful.interface import copy_field
 
-from canonical.cachedproperty import cachedproperty
 from canonical.widgets import LaunchpadRadioWidget
 
 from canonical.launchpad import helpers
@@ -349,3 +348,19 @@
             'The project %s was linked to this source package.' %
             upstream.displayname)
         self.next_url = self.request.getURL()
+
+    @property
+    def has_bugtracker(self):
+        """Does the product have a bugtracker set?"""
+        if self.context.productseries is None:
+            return False
+        product = self.context.productseries.product
+        if product.official_malone:
+            return True
+        bugtracker = product.bugtracker
+        if bugtracker is None:
+            if product.project is not None:
+                bugtracker = product.project.bugtracker
+        if bugtracker is None:
+            return False
+        return True

=== modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
--- lib/lp/registry/browser/tests/sourcepackage-views.txt	2010-01-29 15:03:49 +0000
+++ lib/lp/registry/browser/tests/sourcepackage-views.txt	2010-02-16 17:50:30 +0000
@@ -119,8 +119,11 @@
     ...     extract_text, find_tag_by_id)
     >>> content = extract_text(find_tag_by_id(view.render(), 'upstreams'))
     >>> print content
-    Project:...Bonkers...
-    Series:...crazy...
+    Bonkers project
+    Bug supervisor: no
+    Bug tracker: no
+    Bonkers crazy series
+    Branch: no
 
 A new source project that is not linked to an upstream will result in
 the portlet showing the suggested project.
@@ -160,3 +163,79 @@
     Registered upstream project:
     Lernid...
     Lernid Dev...
+
+
+The view includes a property for determining if the project has a bug
+tracker, though the rules are somewhat complicated.
+
+If the view's package has no productseries set then has_bugtracker is False.
+
+
+    >>> product = factory.makeProduct(name='stinky', displayname='Stinky')
+    >>> productseries = factory.makeProductSeries(
+    ...     name='stinkyseries', product=product)
+    >>> distroseries = factory.makeDistroRelease(name='wonky',
+    ...     distribution=distribution)
+    >>> sourcepackagename = factory.makeSourcePackageName(name='stinkypackage')
+    >>> package = factory.makeSourcePackage(
+    ...     sourcepackagename=sourcepackagename, distroseries=distroseries)
+
+    >>> view = create_initialized_view(package, name='+portlet-associations')
+
+    >>> print package.productseries
+    None
+    >>> print view.has_bugtracker
+    False
+
+So let's set the product series so we can do more interesting testing.
+
+    >>> package.setPackaging(productseries, product.owner)
+    >>> print package.productseries.name
+    stinkyseries
+
+If a product is not part of a project group and its bug tracker is not
+set then the view property is false.
+
+    >>> view = create_initialized_view(package, name='+portlet-associations')
+
+    >>> print product.official_malone
+    False
+    >>> print product.bugtracker
+    None
+    >>> print view.has_bugtracker
+    False
+
+Having official_malone set results in has_bugtracker being true.
+
+    >>> login_person(product.owner)
+    >>> product.official_malone = True
+    >>> print view.has_bugtracker
+    True
+
+Having a bug_tracker set also results in has_bugtracker being true (a
+bit of a tautology you'd think).
+
+    >>> product.official_malone = False
+    >>> bugtracker = factory.makeBugTracker()
+    >>> product.bugtracker = bugtracker
+    >>> print view.has_bugtracker
+    True
+
+If the product has no bug tracker and is in a project group with no
+bug tracker then the property is false.
+
+    >>> product.bugtracker = None
+    >>> project = factory.makeProject()
+    >>> print project.bugtracker
+    None
+    >>> product.project = project
+    >>> print view.has_bugtracker
+    False
+
+If the product's project does have a bug tracker then the product
+inherits it.
+
+    >>> login_person(project.owner)
+    >>> project.bugtracker = bugtracker
+    >>> print view.has_bugtracker
+    True

=== modified file 'lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt'
--- lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-01-25 23:23:27 +0000
+++ lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-02-16 17:50:30 +0000
@@ -28,11 +28,15 @@
     >>> user_browser.getControl("Change").click()
     >>> print extract_text(find_tag_by_id(
     ...     user_browser.contents, 'upstreams'))
+    Mozilla Thunderbird ... project
     Project Group: the Mozilla Project
-    Project: Mozilla Thunderbird  ...
-    Series: trunk ...
+    Bug supervisor: no
+    Bug tracker: no
+    Mozilla Thunderbird trunk series
+    Branch: no
+    Translations: no
 
-He see the "Show upstream links" link and takes a look at the project's
+He sees the "Show upstream links" link and takes a look at the project's
 packaging in distributions.
 
     >>> user_browser.getLink('Show upstream links').click()

=== modified file 'lib/lp/registry/templates/sourcepackage-portlet-associations.pt'
--- lib/lp/registry/templates/sourcepackage-portlet-associations.pt	2010-02-01 13:13:23 +0000
+++ lib/lp/registry/templates/sourcepackage-portlet-associations.pt	2010-02-16 17:50:30 +0000
@@ -5,45 +5,72 @@
    omit-tag="">
 
   <div id="portlet-associations">
-    <h2>Upstream associations</h2>
+    <h2>Upstream connections</h2>
 
     <div class="portletBody portletContent"
          tal:define="series context/productseries">
       <tal:has_series condition="series">
         <div id="upstreams" class="two-column-list">
-          <dl
-             tal:define="project series/product/project"
-             tal:condition="project">
-            <dt>Project Group:</dt>
-            <dd>
-              <a tal:replace="structure project/fmt:link" />
-            </dd>
-          </dl>
-
-          <dl>
-            <dt>Project:</dt>
-            <dd>
-              <a tal:replace="structure series/product/fmt:link" />
-            </dd>
-          </dl>
-
-          <dl>
-            <dt>Series:</dt>
-            <dd>
-              <a
-                 tal:content="series/name"
-                 tal:attributes="href series/fmt:url">Series</a>
-              <a tal:replace="structure context/menu:overview/edit_packaging/fmt:icon" />
-            </dd>
-          </dl>
+          <dl>
+            <dt>
+              <a tal:replace="structure series/product/fmt:link" /> project
+            </dt>
+
+            <tal:has_pg define="project series/product/project"
+                        condition="project">
+              <dd title="Project group">
+                Project Group:
+                <a tal:replace="structure project/fmt:link" />
+              </dd>
+            </tal:has_pg>
+
+            <dd title="Bug supervisor"
+                tal:define="bool series/product/bug_supervisor">
+              Bug supervisor:
+              <metal:yes-no
+                 use-macro="context/@@+base-layout-macros/yes-no" />
+            </dd>
+
+            <dd title="Bug tracker"
+                tal:define="bool view/has_bugtracker">
+              Bug tracker:
+              <metal:yes-no
+                 use-macro="context/@@+base-layout-macros/yes-no" />
+            </dd>
+          </dl>
+          <dl>
+            <dt>
+              <a tal:replace="structure series/fmt:link" />
+            </dt>
+            <dd title="Series branch"
+                tal:define="bool series/branch">
+              Branch:
+              <metal:yes-no
+                 use-macro="context/@@+base-layout-macros/yes-no" />
+            </dd>
+            <dd title="Series translations auto import"
+                tal:condition="context/getTranslationTemplates"
+                tal:define="bool not:series/translations_autoimport_mode/enumvalue:NO_IMPORT">
+              Translations:
+              <metal:yes-no
+                 use-macro="context/@@+base-layout-macros/yes-no" />
+            </dd>
+          </dl>
+
         </div>
 
         <ul class="horizontal">
           <li>
-            <a
+            <a class="sprite info"
                tal:attributes="href series/product/menu:overview/packages/fmt:url"
                >Show upstream links</a>
           </li>
+          <li>
+            <a class="sprite edit"
+               tal:attributes="
+                 href context/menu:overview/edit_packaging/fmt:url"
+               >Change upstream project and series</a>
+          </li>
         </ul>
       </tal:has_series>
 

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-distroseries-sources.txt'
--- lib/lp/soyuz/stories/soyuz/xx-distroseries-sources.txt	2010-01-20 14:29:59 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-distroseries-sources.txt	2010-02-16 17:50:30 +0000
@@ -69,12 +69,15 @@
     mozilla-firefox 0.9
 
 It also provides the upstream relationships for this source, Project,
-Product and Branches:
+Product, Branches, and Bugs:
 
   >>> print extract_text(find_tag_by_id(browser.contents, 'upstreams'))
+  Mozilla Firefox ... project
   Project Group: the Mozilla Project
-  Project: Mozilla Firefox ...
-  Series: trunk Change upstream link
+  Bug supervisor: no
+  Bug tracker: yes
+  Mozilla Firefox trunk series
+  Branch: no
 
 The user can also download the files for the "currentrelease" (last
 published version) if they are available: