Launchpad itself

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

  1. bug-490521
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Approved by: Curtis Hovey
Approved revision: not available
Merged at revision: not available
Proposed branch: lp://staging/~bac/launchpad/bug-490521
Merge into: lp://staging/launchpad
Diff against target: 295 lines (+171/-36)
6 files modified
lib/lp/app/templates/base-layout-macros.pt (+7/-0)
lib/lp/registry/browser/sourcepackage.py (+16/-1)
lib/lp/registry/browser/tests/sourcepackage-views.txt (+81/-2)
lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt (+7/-3)
lib/lp/registry/templates/sourcepackage-portlet-associations.pt (+54/-27)
lib/lp/soyuz/stories/soyuz/xx-distroseries-sources.txt (+6/-3)
To merge this branch: bzr merge lp://staging/~bac/launchpad/bug-490521
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code and ui Approve
Review via email: mp+19133@code.staging.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= 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)

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (3.3 KiB)

...

> === 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...

Read more...

review: Needs Fixing (code and ui)
Revision history for this message
Brad Crittenden (bac) wrote :

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.

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (3.7 KiB)

> === 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...

Read more...

review: Approve (code and ui)
Revision history for this message
Brad Crittenden (bac) wrote :

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

Off to land now. Thanks for feedback.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
410 condition="python: count != 1"410 condition="python: count != 1"
411 replace="l_plural" />411 replace="l_plural" />
412</metal:plural-msg>412</metal:plural-msg>
413
414<metal:yes-no define-macro="yes-no">
415 <span tal:condition="bool"
416 class="sprite yes">&nbsp;<span class="invisible-link">yes</span></span>
417 <span tal:condition="not: bool"
418 class="sprite no">&nbsp;<span class="invisible-link">no</span></span>
419</metal:yes-no>
413</macros>420</macros>
414421
=== 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 @@
2828
29from lazr.restful.interface import copy_field29from lazr.restful.interface import copy_field
3030
31from canonical.cachedproperty import cachedproperty
32from canonical.widgets import LaunchpadRadioWidget31from canonical.widgets import LaunchpadRadioWidget
3332
34from canonical.launchpad import helpers33from canonical.launchpad import helpers
@@ -349,3 +348,19 @@
349 'The project %s was linked to this source package.' %348 'The project %s was linked to this source package.' %
350 upstream.displayname)349 upstream.displayname)
351 self.next_url = self.request.getURL()350 self.next_url = self.request.getURL()
351
352 @property
353 def has_bugtracker(self):
354 """Does the product have a bugtracker set?"""
355 if self.context.productseries is None:
356 return False
357 product = self.context.productseries.product
358 if product.official_malone:
359 return True
360 bugtracker = product.bugtracker
361 if bugtracker is None:
362 if product.project is not None:
363 bugtracker = product.project.bugtracker
364 if bugtracker is None:
365 return False
366 return True
352367
=== 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 @@
119 ... extract_text, find_tag_by_id)119 ... extract_text, find_tag_by_id)
120 >>> content = extract_text(find_tag_by_id(view.render(), 'upstreams'))120 >>> content = extract_text(find_tag_by_id(view.render(), 'upstreams'))
121 >>> print content121 >>> print content
122 Project:...Bonkers...122 Bonkers project
123 Series:...crazy...123 Bug supervisor: no
124 Bug tracker: no
125 Bonkers crazy series
126 Branch: no
124127
125A new source project that is not linked to an upstream will result in128A new source project that is not linked to an upstream will result in
126the portlet showing the suggested project.129the portlet showing the suggested project.
@@ -160,3 +163,79 @@
160 Registered upstream project:163 Registered upstream project:
161 Lernid...164 Lernid...
162 Lernid Dev...165 Lernid Dev...
166
167
168The view includes a property for determining if the project has a bug
169tracker, though the rules are somewhat complicated.
170
171If the view's package has no productseries set then has_bugtracker is False.
172
173
174 >>> product = factory.makeProduct(name='stinky', displayname='Stinky')
175 >>> productseries = factory.makeProductSeries(
176 ... name='stinkyseries', product=product)
177 >>> distroseries = factory.makeDistroRelease(name='wonky',
178 ... distribution=distribution)
179 >>> sourcepackagename = factory.makeSourcePackageName(name='stinkypackage')
180 >>> package = factory.makeSourcePackage(
181 ... sourcepackagename=sourcepackagename, distroseries=distroseries)
182
183 >>> view = create_initialized_view(package, name='+portlet-associations')
184
185 >>> print package.productseries
186 None
187 >>> print view.has_bugtracker
188 False
189
190So let's set the product series so we can do more interesting testing.
191
192 >>> package.setPackaging(productseries, product.owner)
193 >>> print package.productseries.name
194 stinkyseries
195
196If a product is not part of a project group and its bug tracker is not
197set then the view property is false.
198
199 >>> view = create_initialized_view(package, name='+portlet-associations')
200
201 >>> print product.official_malone
202 False
203 >>> print product.bugtracker
204 None
205 >>> print view.has_bugtracker
206 False
207
208Having official_malone set results in has_bugtracker being true.
209
210 >>> login_person(product.owner)
211 >>> product.official_malone = True
212 >>> print view.has_bugtracker
213 True
214
215Having a bug_tracker set also results in has_bugtracker being true (a
216bit of a tautology you'd think).
217
218 >>> product.official_malone = False
219 >>> bugtracker = factory.makeBugTracker()
220 >>> product.bugtracker = bugtracker
221 >>> print view.has_bugtracker
222 True
223
224If the product has no bug tracker and is in a project group with no
225bug tracker then the property is false.
226
227 >>> product.bugtracker = None
228 >>> project = factory.makeProject()
229 >>> print project.bugtracker
230 None
231 >>> product.project = project
232 >>> print view.has_bugtracker
233 False
234
235If the product's project does have a bug tracker then the product
236inherits it.
237
238 >>> login_person(project.owner)
239 >>> project.bugtracker = bugtracker
240 >>> print view.has_bugtracker
241 True
163242
=== 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 @@
28 >>> user_browser.getControl("Change").click()28 >>> user_browser.getControl("Change").click()
29 >>> print extract_text(find_tag_by_id(29 >>> print extract_text(find_tag_by_id(
30 ... user_browser.contents, 'upstreams'))30 ... user_browser.contents, 'upstreams'))
31 Mozilla Thunderbird ... project
31 Project Group: the Mozilla Project32 Project Group: the Mozilla Project
32 Project: Mozilla Thunderbird ...33 Bug supervisor: no
33 Series: trunk ...34 Bug tracker: no
35 Mozilla Thunderbird trunk series
36 Branch: no
37 Translations: no
3438
35He see the "Show upstream links" link and takes a look at the project's39He sees the "Show upstream links" link and takes a look at the project's
36packaging in distributions.40packaging in distributions.
3741
38 >>> user_browser.getLink('Show upstream links').click()42 >>> user_browser.getLink('Show upstream links').click()
3943
=== 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 @@
5 omit-tag="">5 omit-tag="">
66
7 <div id="portlet-associations">7 <div id="portlet-associations">
8 <h2>Upstream associations</h2>8 <h2>Upstream connections</h2>
99
10 <div class="portletBody portletContent"10 <div class="portletBody portletContent"
11 tal:define="series context/productseries">11 tal:define="series context/productseries">
12 <tal:has_series condition="series">12 <tal:has_series condition="series">
13 <div id="upstreams" class="two-column-list">13 <div id="upstreams" class="two-column-list">
14 <dl14 <dl>
15 tal:define="project series/product/project"15 <dt>
16 tal:condition="project">16 <a tal:replace="structure series/product/fmt:link" /> project
17 <dt>Project Group:</dt>17 </dt>
18 <dd>18
19 <a tal:replace="structure project/fmt:link" />19 <tal:has_pg define="project series/product/project"
20 </dd>20 condition="project">
21 </dl>21 <dd title="Project group">
2222 Project Group:
23 <dl>23 <a tal:replace="structure project/fmt:link" />
24 <dt>Project:</dt>24 </dd>
25 <dd>25 </tal:has_pg>
26 <a tal:replace="structure series/product/fmt:link" />26
27 </dd>27 <dd title="Bug supervisor"
28 </dl>28 tal:define="bool series/product/bug_supervisor">
2929 Bug supervisor:
30 <dl>30 <metal:yes-no
31 <dt>Series:</dt>31 use-macro="context/@@+base-layout-macros/yes-no" />
32 <dd>32 </dd>
33 <a33
34 tal:content="series/name"34 <dd title="Bug tracker"
35 tal:attributes="href series/fmt:url">Series</a>35 tal:define="bool view/has_bugtracker">
36 <a tal:replace="structure context/menu:overview/edit_packaging/fmt:icon" />36 Bug tracker:
37 </dd>37 <metal:yes-no
38 </dl>38 use-macro="context/@@+base-layout-macros/yes-no" />
39 </dd>
40 </dl>
41 <dl>
42 <dt>
43 <a tal:replace="structure series/fmt:link" />
44 </dt>
45 <dd title="Series branch"
46 tal:define="bool series/branch">
47 Branch:
48 <metal:yes-no
49 use-macro="context/@@+base-layout-macros/yes-no" />
50 </dd>
51 <dd title="Series translations auto import"
52 tal:condition="context/getTranslationTemplates"
53 tal:define="bool not:series/translations_autoimport_mode/enumvalue:NO_IMPORT">
54 Translations:
55 <metal:yes-no
56 use-macro="context/@@+base-layout-macros/yes-no" />
57 </dd>
58 </dl>
59
39 </div>60 </div>
4061
41 <ul class="horizontal">62 <ul class="horizontal">
42 <li>63 <li>
43 <a64 <a class="sprite info"
44 tal:attributes="href series/product/menu:overview/packages/fmt:url"65 tal:attributes="href series/product/menu:overview/packages/fmt:url"
45 >Show upstream links</a>66 >Show upstream links</a>
46 </li>67 </li>
68 <li>
69 <a class="sprite edit"
70 tal:attributes="
71 href context/menu:overview/edit_packaging/fmt:url"
72 >Change upstream project and series</a>
73 </li>
47 </ul>74 </ul>
48 </tal:has_series>75 </tal:has_series>
4976
5077
=== 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 @@
69 mozilla-firefox 0.969 mozilla-firefox 0.9
7070
71It also provides the upstream relationships for this source, Project,71It also provides the upstream relationships for this source, Project,
72Product and Branches:72Product, Branches, and Bugs:
7373
74 >>> print extract_text(find_tag_by_id(browser.contents, 'upstreams'))74 >>> print extract_text(find_tag_by_id(browser.contents, 'upstreams'))
75 Mozilla Firefox ... project
75 Project Group: the Mozilla Project76 Project Group: the Mozilla Project
76 Project: Mozilla Firefox ...77 Bug supervisor: no
77 Series: trunk Change upstream link78 Bug tracker: yes
79 Mozilla Firefox trunk series
80 Branch: no
7881
79The user can also download the files for the "currentrelease" (last82The user can also download the files for the "currentrelease" (last
80published version) if they are available:83published version) if they are available: