Launchpad itself

Merge lp:~intellectronica/launchpad/bugmail-ui-fixes into lp:launchpad

  1. bugmail-ui-fixes
  2. Merge into devel
Proposed by Eleanor Berger
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~intellectronica/launchpad/bugmail-ui-fixes
Merge into: lp:launchpad
Diff against target: 390 lines
10 files modified
lib/canonical/launchpad/browser/structuralsubscription.py (+30/-1)
lib/canonical/launchpad/database/structuralsubscription.py (+11/-0)
lib/canonical/launchpad/interfaces/structuralsubscription.py (+3/-0)
lib/lp/registry/browser/distribution.py (+2/-5)
lib/lp/registry/browser/distroseries.py (+5/-10)
lib/lp/registry/browser/milestone.py (+2/-6)
lib/lp/registry/browser/product.py (+3/-10)
lib/lp/registry/browser/productseries.py (+5/-12)
lib/lp/registry/configure.zcml (+14/-7)
lib/lp/registry/model/milestone.py (+4/-0)
To merge this branch: bzr merge lp:~intellectronica/launchpad/bugmail-ui-fixes
Reviewer Review Type Date Requested Status
Gavin Panella (community) code Approve
Martin Albisetti (community) ui Approve
Michael Nelson (community) ui Approve
Canonical Launchpad Engineering code ui Pending
Review via email: mp+13067@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Eleanor Berger (intellectronica) wrote :

This branch makes the menu link for subscribing to bug mail sensitive to the user's existing subsription. If the user is already subscribed (directly, or via one of the teams he's a member of), the link displays the edit icon and offers to edit subscription. If no subscription exists the link offers to subscribe and displays the add icon.

There are no lint warnings related to my changes (the usual lazr stuff is still there).

To test, run bin/test -vv -t xx-milestone-add-and-edit.txt -t xx-distroseries-index.txt -t xx-productseries-index.txt -t xx-productseries-add-and-edit.txt -t xx-bug-subscriptions.txt -t xx-distribution-bugs-page.txt -t xx-distrorelease-bugs-page.txt -t xx-product-bugs-page.txt > test-output.txt

To play with the UI, go to https://bugs.launchpad.dev/jokosher or https://bugs.launchpad.dev/ubuntu or any of the other structural subscription targets and try to subscribe and unsubscribe.

While working on this I realised that the form for editing structural subscriptions does not behave in the expected way, like all other boomerang forms. It doesn't offer a cancel link, and after submitting it displays the form again, instead of returning to the context. I plan to fix that in a future branch.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

> This branch makes the menu link for subscribing to bug mail sensitive to the
> user's existing subsription. If the user is already subscribed (directly, or
> via one of the teams he's a member of), the link displays the edit icon and
> offers to edit subscription. If no subscription exists the link offers to
> subscribe and displays the add icon.

Nice change Tom! That'll make it much easier for people to follow when they want to edit or remove subscriptions. I actually had to revert your changes to see what happens currently because what you've done is so natural.

Two things on that page that are unrelated to your change, but might be nice to look at, here's a screenshot to show them:

http://people.canonical.com/~michaeln/tmp/bug-subscription-empty-portlet.png

1. There seems to be an empty portlet below? It looks like some ajax is meant to load something there - I'm guessing you're already aware and there's probably already a bug about it.

2. Is there any reason why the "Bug tracker: Launchpad, Bug supervisor:...." section is sitting over on the right like that? It looks exactly like the type of info that is usually a main-area portlet with an h2 heading and a dl (like the Project information at https://edge.launchpad.net/firefox ). Let me know what you think.

[snip]

>
> While working on this I realised that the form for editing structural
> subscriptions does not behave in the expected way, like all other boomerang
> forms. It doesn't offer a cancel link, and after submitting it displays the
> form again, instead of returning to the context. I plan to fix that in a
> future branch.

Great - A few other things to consider on that page when you get to it (in case you didn't already notice them).

First, the heading/breadcrumbs are currently:

Subscribe to Bugs in Jokosher Audio Editor
Jokosher >> Bugs >> Subscribe to Bugs in Jokosher Audio E...

My preference here would be for:

Subscribe to Bugs in Jokosher Audio Editor
Jokosher >> Bugs >> Subscribe

but I've sent an email out to the dev list about this yesterday, and it seems that the consensus is for:

Subscribe
Jokosher >> Bugs >> Subscribe

If you think that ^^ would be wrong, please voice your opinion on the email list :)

Secondly, that tip about "Managing Launchpad Bugs e-mail really doesn't belong there in the side-bar?

Anyway, they're unrelated to this branch. Great work!

review: Approve (ui)
Revision history for this message
Martin Albisetti (beuno) wrote :

Nothing to add!

review: Approve (ui)
Revision history for this message
Gavin Panella (allenap) wrote :

<allenap> intellectronica: I really like your bugmail-ui-fixes branch :)
<allenap> intellectronica: Two questions.
<allenap> intellectronica: Line 49, when it does self.context.context, is that because self.context is a view?
<intellectronica> allenap: exactly. context can be either the target itself or a view
<allenap> intellectronica: Could you add a comment to the else: clause to say that?
<intellectronica> allenap: sure, that's a good idea
<allenap> intellectronica: Question the second. The zcml. Is there an common interface where you can declare the permissions for userHasBugSubscriptions, rather than doing it 7 times?
<intellectronica> allenap: i wish. but no, that's not possible because of the stupid way in which zcml handles the interaction between permissions specified by interface and byb attributes
<allenap> intellectronica: Okay. r=me :)
<intellectronica> allenap: thanks!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/browser/structuralsubscription.py'
2--- lib/canonical/launchpad/browser/structuralsubscription.py 2009-09-17 17:12:58 +0000
3+++ lib/canonical/launchpad/browser/structuralsubscription.py 2009-10-22 09:45:25 +0000
4@@ -4,6 +4,7 @@
5 __metaclass__ = type
6
7 __all__ = [
8+ 'StructuralSubscriptionMenuMixin',
9 'StructuralSubscriptionTargetTraversalMixin',
10 'StructuralSubscriptionView',
11 ]
12@@ -19,13 +20,15 @@
13 from canonical.launchpad.interfaces import (
14 BugNotificationLevel, IDistributionSourcePackage,
15 IStructuralSubscriptionForm)
16+from canonical.launchpad.interfaces.structuralsubscription import (
17+ IStructuralSubscriptionTarget)
18 from lp.registry.interfaces.person import IPersonSet
19 from canonical.launchpad.webapp import (
20 LaunchpadFormView, action, canonical_url, custom_widget, stepthrough)
21 from canonical.launchpad.webapp.authorization import check_permission
22+from canonical.launchpad.webapp.menu import Link
23 from canonical.widgets import LabeledMultiCheckBoxWidget
24
25-
26 class StructuralSubscriptionView(LaunchpadFormView):
27 """View class for structural subscriptions."""
28
29@@ -277,3 +280,29 @@
30 """Traverses +subscription portions of URLs."""
31 person = getUtility(IPersonSet).getByName(name)
32 return self.context.getSubscription(person)
33+
34+
35+class StructuralSubscriptionMenuMixin:
36+ """Mix-in class providing the subscription add/edit menu link."""
37+
38+ def subscribe(self):
39+ """The subscribe menu link.
40+
41+ If the user, or any of the teams he's a member of, already has a
42+ subscription to the context, the link offer to edit the subscriptions
43+ and displays the edit icon. Otherwise, the link offers to subscribe
44+ and displays the add icon.
45+ """
46+ if IStructuralSubscriptionTarget.providedBy(self.context):
47+ sst = self.context
48+ else:
49+ # self.context is a view, and the target is its context
50+ sst = self.context.context
51+
52+ if sst.userHasBugSubscriptions(self.user):
53+ text = 'Edit bug mail subscription'
54+ icon = 'edit'
55+ else:
56+ text = 'Subscribe to bug mail'
57+ icon = 'add'
58+ return Link('+subscribe', text, icon=icon)
59
60=== modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
61--- lib/canonical/launchpad/database/structuralsubscription.py 2009-08-25 12:04:58 +0000
62+++ lib/canonical/launchpad/database/structuralsubscription.py 2009-10-22 09:45:25 +0000
63@@ -326,3 +326,14 @@
64 else:
65 raise AssertionError(
66 '%s is not a valid structural subscription target.', self)
67+
68+ def userHasBugSubscriptions(self, user):
69+ """See `IStructuralSubscriptionTarget`."""
70+ bug_subscriptions = self.getSubscriptions(
71+ min_bug_notification_level=BugNotificationLevel.METADATA)
72+ for subscription in bug_subscriptions:
73+ if (subscription.subscriber == user or
74+ user.inTeam(subscription.subscriber)):
75+ # The user has a bug subscription
76+ return True
77+ return False
78
79=== modified file 'lib/canonical/launchpad/interfaces/structuralsubscription.py'
80--- lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-09-09 14:26:18 +0000
81+++ lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-10-22 09:45:25 +0000
82@@ -240,6 +240,9 @@
83
84 target_type_display = Attribute("The type of the target, for display.")
85
86+ def userHasBugSubscriptions(user):
87+ """Is `user` subscribed, directly or via a team, to bug mail?"""
88+
89
90 class IStructuralSubscriptionForm(Interface):
91 """Schema for the structural subscription form."""
92
93=== modified file 'lib/lp/registry/browser/distribution.py'
94--- lib/lp/registry/browser/distribution.py 2009-10-15 10:26:57 +0000
95+++ lib/lp/registry/browser/distribution.py 2009-10-22 09:45:25 +0000
96@@ -76,6 +76,7 @@
97 from lp.soyuz.interfaces.publishedpackage import (
98 IPublishedPackageSet)
99 from canonical.launchpad.browser.structuralsubscription import (
100+ StructuralSubscriptionMenuMixin,
101 StructuralSubscriptionTargetTraversalMixin)
102 from canonical.launchpad.webapp import (
103 action, ApplicationMenu, canonical_url, ContextMenu, custom_widget,
104@@ -421,7 +422,7 @@
105 return Link('+addseries', text, icon='add')
106
107
108-class DistributionBugsMenu(ApplicationMenu):
109+class DistributionBugsMenu(ApplicationMenu, StructuralSubscriptionMenuMixin):
110
111 usedfor = IDistribution
112 facet = 'bugs'
113@@ -451,10 +452,6 @@
114 text = 'Report a bug'
115 return Link('+filebug', text, icon='bug')
116
117- def subscribe(self):
118- text = 'Subscribe to bug mail'
119- return Link('+subscribe', text, icon='edit')
120-
121
122 class DistributionSpecificationsMenu(NavigationMenu,
123 HasSpecificationsMenuMixin):
124
125=== modified file 'lib/lp/registry/browser/distroseries.py'
126--- lib/lp/registry/browser/distroseries.py 2009-09-25 17:00:20 +0000
127+++ lib/lp/registry/browser/distroseries.py 2009-10-22 09:45:25 +0000
128@@ -41,6 +41,7 @@
129 IDistroSeriesLanguageSet)
130 from lp.services.worlddata.interfaces.language import ILanguageSet
131 from canonical.launchpad.browser.structuralsubscription import (
132+ StructuralSubscriptionMenuMixin,
133 StructuralSubscriptionTargetTraversalMixin)
134 from canonical.launchpad.interfaces.launchpad import (
135 ILaunchBag, NotFoundError)
136@@ -142,7 +143,8 @@
137 'translations']
138
139
140-class DistroSeriesOverviewMenu(ApplicationMenu):
141+class DistroSeriesOverviewMenu(
142+ ApplicationMenu, StructuralSubscriptionMenuMixin):
143
144 usedfor = IDistroSeries
145 facet = 'overview'
146@@ -202,12 +204,8 @@
147 text = 'Show uploads'
148 return Link('+queue', text, icon='info')
149
150- def subscribe(self):
151- text = 'Subscribe to bug mail'
152- return Link('+subscribe', text, icon='edit')
153-
154-
155-class DistroSeriesBugsMenu(ApplicationMenu):
156+
157+class DistroSeriesBugsMenu(ApplicationMenu, StructuralSubscriptionMenuMixin):
158
159 usedfor = IDistroSeries
160 facet = 'bugs'
161@@ -223,9 +221,6 @@
162 def nominations(self):
163 return Link('+nominations', 'Review nominations', icon='bug')
164
165- def subscribe(self):
166- return Link('+subscribe', 'Subscribe to bug mail')
167-
168
169 class DistroSeriesSpecificationsMenu(NavigationMenu,
170 HasSpecificationsMenuMixin):
171
172=== modified file 'lib/lp/registry/browser/milestone.py'
173--- lib/lp/registry/browser/milestone.py 2009-09-22 16:24:23 +0000
174+++ lib/lp/registry/browser/milestone.py 2009-10-22 09:45:25 +0000
175@@ -33,6 +33,7 @@
176 IMilestone, IMilestoneSet, IProjectMilestone)
177 from lp.registry.interfaces.product import IProduct
178 from canonical.launchpad.browser.structuralsubscription import (
179+ StructuralSubscriptionMenuMixin,
180 StructuralSubscriptionTargetTraversalMixin)
181 from canonical.launchpad.webapp import (
182 action, canonical_url, custom_widget,
183@@ -58,7 +59,7 @@
184 usedfor = IMilestone
185
186
187-class MilestoneLinkMixin:
188+class MilestoneLinkMixin(StructuralSubscriptionMenuMixin):
189 """The menu for this milestone."""
190
191 @enabled_with_permission('launchpad.Edit')
192@@ -72,11 +73,6 @@
193 return Link(
194 '+edit', text, icon='edit', summary=summary, enabled=enabled)
195
196- def subscribe(self):
197- """The link to subscribe to bug mail."""
198- enabled = not IProjectMilestone.providedBy(self.context)
199- return Link('+subscribe', 'Subscribe to bug mail',
200- icon='edit', enabled=enabled)
201
202 @enabled_with_permission('launchpad.Edit')
203 def create_release(self):
204
205=== modified file 'lib/lp/registry/browser/product.py'
206--- lib/lp/registry/browser/product.py 2009-10-20 16:45:23 +0000
207+++ lib/lp/registry/browser/product.py 2009-10-22 09:45:25 +0000
208@@ -89,6 +89,7 @@
209 from lp.answers.browser.questiontarget import (
210 QuestionTargetFacetMixin, QuestionTargetTraversalMixin)
211 from canonical.launchpad.browser.structuralsubscription import (
212+ StructuralSubscriptionMenuMixin,
213 StructuralSubscriptionTargetTraversalMixin)
214 from canonical.launchpad.mail import format_address, simple_sendmail
215 from canonical.launchpad.webapp import (
216@@ -311,7 +312,7 @@
217 return Link('+branchvisibility', text)
218
219
220-class ProductEditLinksMixin:
221+class ProductEditLinksMixin(StructuralSubscriptionMenuMixin):
222 """A mixin class for menus that need Product edit links."""
223
224 @enabled_with_permission('launchpad.Edit')
225@@ -339,10 +340,6 @@
226 text = 'Administer'
227 return Link('+admin', text, icon='edit')
228
229- def subscribe(self):
230- text = 'Subscribe to bug mail'
231- return Link('+subscribe', text, icon='edit')
232-
233
234 class IProductEditMenu(Interface):
235 """A marker interface for the 'Change details' navigation menu."""
236@@ -431,7 +428,7 @@
237 return Link('+branchvisibility', text, icon='edit')
238
239
240-class ProductBugsMenu(ApplicationMenu):
241+class ProductBugsMenu(ApplicationMenu, StructuralSubscriptionMenuMixin):
242
243 usedfor = IProduct
244 facet = 'bugs'
245@@ -460,10 +457,6 @@
246 text = 'Change security contact'
247 return Link('+securitycontact', text, icon='edit')
248
249- def subscribe(self):
250- text = 'Subscribe to bug mail'
251- return Link('+subscribe', text, icon='edit')
252-
253
254 class ProductSpecificationsMenu(NavigationMenu,
255 HasSpecificationsMenuMixin):
256
257=== modified file 'lib/lp/registry/browser/productseries.py'
258--- lib/lp/registry/browser/productseries.py 2009-10-22 07:23:57 +0000
259+++ lib/lp/registry/browser/productseries.py 2009-10-22 09:45:25 +0000
260@@ -54,6 +54,7 @@
261 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
262 from lp.registry.browser import StatusCount
263 from canonical.launchpad.browser.structuralsubscription import (
264+ StructuralSubscriptionMenuMixin,
265 StructuralSubscriptionTargetTraversalMixin)
266 from lp.translations.interfaces.potemplate import IPOTemplateSet
267 from lp.translations.interfaces.productserieslanguage import (
268@@ -148,7 +149,8 @@
269 return Link(link, text, summary=summary)
270
271
272-class ProductSeriesOverviewMenu(ApplicationMenu):
273+class ProductSeriesOverviewMenu(
274+ ApplicationMenu, StructuralSubscriptionMenuMixin):
275 """The overview menu."""
276 usedfor = IProductSeries
277 facet = 'overview'
278@@ -219,13 +221,8 @@
279 text = 'Download RDF metadata'
280 return Link('+rdf', text, icon='download')
281
282- def subscribe(self):
283- """Return a link to subscribe to bug mail."""
284- text = 'Subscribe to bug mail'
285- return Link('+subscribe', text, icon='edit')
286-
287-
288-class ProductSeriesBugsMenu(ApplicationMenu):
289+
290+class ProductSeriesBugsMenu(ApplicationMenu, StructuralSubscriptionMenuMixin):
291 """The bugs menu."""
292 usedfor = IProductSeries
293 facet = 'bugs'
294@@ -243,10 +240,6 @@
295 """Return a link to review bugs nominated for this series."""
296 return Link('+nominations', 'Review nominations', icon='bug')
297
298- def subscribe(self):
299- """Return a link to subscribe to bug mail."""
300- return Link('+subscribe', 'Subscribe to bug mail')
301-
302
303 class ProductSeriesSpecificationsMenu(NavigationMenu,
304 HasSpecificationsMenuMixin):
305
306=== modified file 'lib/lp/registry/configure.zcml'
307--- lib/lp/registry/configure.zcml 2009-10-20 13:21:44 +0000
308+++ lib/lp/registry/configure.zcml 2009-10-22 09:45:25 +0000
309@@ -149,7 +149,8 @@
310 getSubscription
311 getSubscriptions
312 parent_subscription_target
313- target_type_display"/>
314+ target_type_display
315+ userHasBugSubscriptions"/>
316 <require
317 permission="launchpad.AnyPerson"
318 attributes="
319@@ -285,7 +286,8 @@
320 getSubscription
321 getBugNotificationsRecipients
322 parent_subscription_target
323- target_type_display"/>
324+ target_type_display
325+ userHasBugSubscriptions"/>
326 <require
327 permission="launchpad.AnyPerson"
328 attributes="
329@@ -395,7 +397,8 @@
330 target_type_display
331 official_bug_tags
332 findRelatedArchives
333- findRelatedArchivePublications"/>
334+ findRelatedArchivePublications
335+ userHasBugSubscriptions"/>
336 <require
337 permission="launchpad.AnyPerson"
338 attributes="
339@@ -903,7 +906,8 @@
340 getSubscription
341 getBugNotificationsRecipients
342 parent_subscription_target
343- target_type_display"/>
344+ target_type_display
345+ userHasBugSubscriptions"/>
346 <require
347 permission="launchpad.AnyPerson"
348 attributes="
349@@ -1150,7 +1154,8 @@
350 getSubscription
351 getBugNotificationsRecipients
352 parent_subscription_target
353- target_type_display"/>
354+ target_type_display
355+ userHasBugSubscriptions"/>
356 <require
357 permission="launchpad.AnyPerson"
358 attributes="
359@@ -1297,7 +1302,8 @@
360 getSubscription
361 getSubscriptions
362 parent_subscription_target
363- target_type_display"/>
364+ target_type_display
365+ userHasBugSubscriptions"/>
366 <require
367 permission="launchpad.AnyPerson"
368 attributes="
369@@ -1407,7 +1413,8 @@
370 getSubscription
371 getBugNotificationsRecipients
372 parent_subscription_target
373- target_type_display"/>
374+ target_type_display
375+ userHasBugSubscriptions"/>
376 <require
377 permission="launchpad.AnyPerson"
378 attributes="
379
380=== modified file 'lib/lp/registry/model/milestone.py'
381--- lib/lp/registry/model/milestone.py 2009-09-29 18:55:25 +0000
382+++ lib/lp/registry/model/milestone.py 2009-10-22 09:45:25 +0000
383@@ -301,3 +301,7 @@
384 def official_bug_tags(self):
385 """See `IHasBugs`."""
386 return self.target.official_bug_tags
387+
388+ def userHasBugSubscriptions(self, user):
389+ """See `IStructuralSubscriptionTarget`."""
390+ return False