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
=== 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 19:13:13 +0000
@@ -22,7 +22,7 @@
22 IStructuralSubscription, IStructuralSubscriptionTarget,22 IStructuralSubscription, IStructuralSubscriptionTarget,
23 UserCannotSubscribePerson)23 UserCannotSubscribePerson)
24from lp.registry.interfaces.person import (24from lp.registry.interfaces.person import (
25 validate_public_person, validate_person_not_private_membership)25 IPerson, validate_public_person, validate_person_not_private_membership)
2626
2727
28class StructuralSubscription(SQLBase):28class StructuralSubscription(SQLBase):
@@ -331,9 +331,10 @@
331 """See `IStructuralSubscriptionTarget`."""331 """See `IStructuralSubscriptionTarget`."""
332 bug_subscriptions = self.getSubscriptions(332 bug_subscriptions = self.getSubscriptions(
333 min_bug_notification_level=BugNotificationLevel.METADATA)333 min_bug_notification_level=BugNotificationLevel.METADATA)
334 for subscription in bug_subscriptions:334 if user is not None:
335 if (subscription.subscriber == user or335 for subscription in bug_subscriptions:
336 user.inTeam(subscription.subscriber)):336 if (subscription.subscriber == user or
337 # The user has a bug subscription337 user.inTeam(subscription.subscriber)):
338 return True338 # The user has a bug subscription
339 return True
339 return False340 return False
340341
=== modified file 'lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt'
--- lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt 2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/stories/structural-subscriptions/xx-bug-subscriptions.txt 2009-10-28 19:13:13 +0000
@@ -177,3 +177,20 @@
177 Traceback (most recent call last):177 Traceback (most recent call last):
178 ...178 ...
179 LookupError: label '\xa0Foo Bar'179 LookupError: label '\xa0Foo Bar'
180
181
182== Bug #462742 ==
183
184Loading pages with the subscription menu works fine for anonymous users, even
185when there are existing subscriptions.
186
187 >>> browser.open('http://launchpad.dev/bzr')
188 >>> browser.getLink('Subscribe to bug mail').click()
189 >>> browser.getControl(
190 ... 'I want to receive these '
191 ... 'notifications by e-mail.').selected = True
192 >>> browser.getControl('Save these changes').click()
193
194 >>> anon_browser.open('http://launchpad.dev/bzr')
195 >>> print anon_browser.title
196 Bazaar Version Control System in Launchpad