Launchpad itself

Merge lp:~intellectronica/launchpad/oops-462742 into lp:launchpad

  1. oops-462742
  2. Merge into devel
Proposed by Eleanor Berger
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~intellectronica/launchpad/oops-462742
Merge into: lp:launchpad
Diff against target: 53 lines
2 files modified
lib/canonical/launchpad/database/structuralsubscription.py (+7/-6)
lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt (+17/-0)
To merge this branch: bzr merge lp:~intellectronica/launchpad/oops-462742
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+14104@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Eleanor Berger (intellectronica) wrote :

This branch fixes bug #462742 which caused pages that have the bug mail subscription link on them to oops for anonymous users if there are existing subscriptions for the project.

Revision history for this message
Björn Tillenius (bjornt) wrote :

On Wed, Oct 28, 2009 at 06:05:27PM -0000, Tom Berger wrote:
> === modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
> --- lib/canonical/launchpad/database/structuralsubscription.py 2009-10-23 13:51:40 +0000
> +++ lib/canonical/launchpad/database/structuralsubscription.py 2009-10-28 18:05:23 +0000
> @@ -331,9 +331,10 @@
> """See `IStructuralSubscriptionTarget`."""
> bug_subscriptions = self.getSubscriptions(
> min_bug_notification_level=BugNotificationLevel.METADATA)
> - for subscription in bug_subscriptions:
> - if (subscription.subscriber == user or
> - user.inTeam(subscription.subscriber)):
> - # The user has a bug subscription
> - return True
> + if IPerson.providedBy(user):

What different kinds of objects can 'user' be?

> + for subscription in bug_subscriptions:
> + if (subscription.subscriber == user or
> + user.inTeam(subscription.subscriber)):
> + # The user has a bug subscription
> + return True
> return False

--
Björn Tillenius | https://launchpad.net/~bjornt

Revision history for this message
Eleanor Berger (intellectronica) wrote :

2009/10/28 Björn Tillenius <email address hidden>:
> On Wed, Oct 28, 2009 at 06:05:27PM -0000, Tom Berger wrote:
>> === modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
>> --- lib/canonical/launchpad/database/structuralsubscription.py        2009-10-23 13:51:40 +0000
>> +++ lib/canonical/launchpad/database/structuralsubscription.py        2009-10-28 18:05:23 +0000
>> @@ -331,9 +331,10 @@
>>          """See `IStructuralSubscriptionTarget`."""
>>          bug_subscriptions = self.getSubscriptions(
>>              min_bug_notification_level=BugNotificationLevel.METADATA)
>> -        for subscription in bug_subscriptions:
>> -            if (subscription.subscriber == user or
>> -                user.inTeam(subscription.subscriber)):
>> -                # The user has a bug subscription
>> -                return True
>> +        if IPerson.providedBy(user):
>
> What different kinds of objects can 'user' be?

It can be None, but I felt that writing the condition like this made
it clearer. Would you have preferred comparing it to None instead?

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for fixing this oops.

review: Approve (code)
Revision history for this message
Björn Tillenius (bjornt) wrote :

On Wed, Oct 28, 2009 at 06:21:10PM -0000, Tom Berger wrote:
> 2009/10/28 Björn Tillenius <email address hidden>:
> > On Wed, Oct 28, 2009 at 06:05:27PM -0000, Tom Berger wrote:
> >> === modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
> >> --- lib/canonical/launchpad/database/structuralsubscription.py        2009-10-23 13:51:40 +0000
> >> +++ lib/canonical/launchpad/database/structuralsubscription.py        2009-10-28 18:05:23 +0000
> >> @@ -331,9 +331,10 @@
> >>          """See `IStructuralSubscriptionTarget`."""
> >>          bug_subscriptions = self.getSubscriptions(
> >>              min_bug_notification_level=BugNotificationLevel.METADATA)
> >> -        for subscription in bug_subscriptions:
> >> -            if (subscription.subscriber == user or
> >> -                user.inTeam(subscription.subscriber)):
> >> -                # The user has a bug subscription
> >> -                return True
> >> +        if IPerson.providedBy(user):
> >
> > What different kinds of objects can 'user' be?
>
> It can be None, but I felt that writing the condition like this made
> it clearer. Would you have preferred comparing it to None instead?

It's common to compare explicitly to None. If I see someone using
IFoo.providedBy(object), I assume that object can be provide some other
interface that IFoo.

--
Björn Tillenius | https://launchpad.net/~bjornt

Revision history for this message
Eleanor Berger (intellectronica) wrote :

2009/10/28 Björn Tillenius <email address hidden>:
> On Wed, Oct 28, 2009 at 06:21:10PM -0000, Tom Berger wrote:
>> 2009/10/28 Björn Tillenius <email address hidden>:
>> > On Wed, Oct 28, 2009 at 06:05:27PM -0000, Tom Berger wrote:
>> >> === modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
>> >> --- lib/canonical/launchpad/database/structuralsubscription.py        2009-10-23 13:51:40 +0000
>> >> +++ lib/canonical/launchpad/database/structuralsubscription.py        2009-10-28 18:05:23 +0000
>> >> @@ -331,9 +331,10 @@
>> >>          """See `IStructuralSubscriptionTarget`."""
>> >>          bug_subscriptions = self.getSubscriptions(
>> >>              min_bug_notification_level=BugNotificationLevel.METADATA)
>> >> -        for subscription in bug_subscriptions:
>> >> -            if (subscription.subscriber == user or
>> >> -                user.inTeam(subscription.subscriber)):
>> >> -                # The user has a bug subscription
>> >> -                return True
>> >> +        if IPerson.providedBy(user):
>> >
>> > What different kinds of objects can 'user' be?
>>
>> It can be None, but I felt that writing the condition like this made
>> it clearer. Would you have preferred comparing it to None instead?
>
> It's common to compare explicitly to None. If I see someone using
> IFoo.providedBy(object), I assume that object can be provide some other
> interface that IFoo.

Yes, that makes sense. Changed.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/database/structuralsubscription.py'
2--- lib/canonical/launchpad/database/structuralsubscription.py 2009-10-23 13:51:40 +0000
3+++ lib/canonical/launchpad/database/structuralsubscription.py 2009-10-28 19:13:13 +0000
4@@ -22,7 +22,7 @@
5 IStructuralSubscription, IStructuralSubscriptionTarget,
6 UserCannotSubscribePerson)
7 from lp.registry.interfaces.person import (
8- validate_public_person, validate_person_not_private_membership)
9+ IPerson, validate_public_person, validate_person_not_private_membership)
10
11
12 class StructuralSubscription(SQLBase):
13@@ -331,9 +331,10 @@
14 """See `IStructuralSubscriptionTarget`."""
15 bug_subscriptions = self.getSubscriptions(
16 min_bug_notification_level=BugNotificationLevel.METADATA)
17- for subscription in bug_subscriptions:
18- if (subscription.subscriber == user or
19- user.inTeam(subscription.subscriber)):
20- # The user has a bug subscription
21- return True
22+ if user is not None:
23+ for subscription in bug_subscriptions:
24+ if (subscription.subscriber == user or
25+ user.inTeam(subscription.subscriber)):
26+ # The user has a bug subscription
27+ return True
28 return False
29
30=== modified file 'lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt'
31--- lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt 2009-06-12 16:36:02 +0000
32+++ lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt 2009-10-28 19:13:13 +0000
33@@ -177,3 +177,20 @@
34 Traceback (most recent call last):
35 ...
36 LookupError: label '\xa0Foo Bar'
37+
38+
39+== Bug #462742 ==
40+
41+Loading pages with the subscription menu works fine for anonymous users, even
42+when there are existing subscriptions.
43+
44+ >>> browser.open('http://launchpad.dev/bzr')
45+ >>> browser.getLink('Subscribe to bug mail').click()
46+ >>> browser.getControl(
47+ ... 'I want to receive these '
48+ ... 'notifications by e-mail.').selected = True
49+ >>> browser.getControl('Save these changes').click()
50+
51+ >>> anon_browser.open('http://launchpad.dev/bzr')
52+ >>> print anon_browser.title
53+ Bazaar Version Control System in Launchpad