Merge lp:~deryck/launchpad/displayname-private-team-oops-634847 into lp:launchpad

Proposed by Deryck Hodge on 2010-09-21
Status: Merged
Approved by: Deryck Hodge on 2010-09-22
Approved revision: no longer in the source branch.
Merged at revision: 11617
Proposed branch: lp:~deryck/launchpad/displayname-private-team-oops-634847
Merge into: lp:launchpad
Diff against target: 87 lines (+50/-2)
2 files modified
lib/lp/bugs/model/bug.py (+7/-2)
lib/lp/bugs/tests/test_bug.py (+43/-0)
To merge this branch: bzr merge lp:~deryck/launchpad/displayname-private-team-oops-634847
Reviewer Review Type Date Requested Status
Abel Deuring (community) code 2010-09-21 Approve on 2010-09-22
Review via email: mp+36199@code.launchpad.net

Commit message

Fix an OOPS when trying to sort on displayname for bug subscribers.

Description of the change

This work fixes the OOPS reported in bug 634847, where a comment was
not being allowed to post to a bug. The error was because a team was
subscribed that was private, and getAlsoNotifiedSubscribers was
blowing up trying to access a forbidden attribute ("displayname") for
sorting.

This branch adds tests to ensure we can list subscribers when one is
private and fixes the couple spots in the code where this was a
problem. The fix I chose was to use lambda to get an object that was
not security proxied and then pass the displayname attribute as the
sort key. This allows the list that is returned to retain security
proxied objects.

Cheers,
deryck

To post a comment you must log in.
Abel Deuring (adeuring) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/model/bug.py'
2--- lib/lp/bugs/model/bug.py 2010-09-22 08:48:24 +0000
3+++ lib/lp/bugs/model/bug.py 2010-09-23 12:32:52 +0000
4@@ -842,8 +842,11 @@
5 self.getAlsoNotifiedSubscribers(recipients, level) +
6 self.getSubscribersFromDuplicates(recipients, level))
7
8+ # Remove security proxy for the sort key, but return
9+ # the regular proxied object.
10 return sorted(
11- indirect_subscribers, key=operator.attrgetter("displayname"))
12+ indirect_subscribers,
13+ key=lambda x: removeSecurityProxy(x).displayname)
14
15 def getSubscriptionsFromDuplicates(self, recipients=None):
16 """See `IBug`."""
17@@ -999,9 +1002,11 @@
18 # Direct subscriptions always take precedence over indirect
19 # subscriptions.
20 direct_subscribers = set(self.getDirectSubscribers())
21+ # Remove security proxy for the sort key, but return
22+ # the regular proxied object.
23 return sorted(
24 (also_notified_subscribers - direct_subscribers),
25- key=operator.attrgetter('displayname'))
26+ key=lambda x: removeSecurityProxy(x).displayname)
27
28 def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
29 level=None,
30
31=== modified file 'lib/lp/bugs/tests/test_bug.py'
32--- lib/lp/bugs/tests/test_bug.py 2010-08-31 00:55:52 +0000
33+++ lib/lp/bugs/tests/test_bug.py 2010-09-23 12:32:52 +0000
34@@ -6,6 +6,8 @@
35 __metaclass__ = type
36
37 from canonical.testing import DatabaseFunctionalLayer
38+from lp.registry.interfaces.person import PersonVisibility
39+from lp.registry.model.structuralsubscription import StructuralSubscription
40 from lp.testing import (
41 person_logged_in,
42 TestCaseWithFactory,
43@@ -65,3 +67,44 @@
44 set([person, team1, team2]),
45 set(real_bug.getSubscribersForPerson(person)))
46
47+ def test_get_also_notified_subscribers_with_private_team(self):
48+ product = self.factory.makeProduct()
49+ bug = self.factory.makeBug(product=product)
50+ member = self.factory.makePerson()
51+ team = self.factory.makeTeam(
52+ owner=member, visibility=PersonVisibility.PRIVATE)
53+ StructuralSubscription(
54+ product=product, subscriber=team, subscribed_by=member)
55+ self.assertTrue(team in bug.getAlsoNotifiedSubscribers())
56+
57+ def test_get_indirect_subscribers_with_private_team(self):
58+ product = self.factory.makeProduct()
59+ bug = self.factory.makeBug(product=product)
60+ member = self.factory.makePerson()
61+ team = self.factory.makeTeam(
62+ owner=member, visibility=PersonVisibility.PRIVATE)
63+ StructuralSubscription(
64+ product=product, subscriber=team, subscribed_by=member)
65+ self.assertTrue(team in bug.getIndirectSubscribers())
66+
67+ def test_get_direct_subscribers_with_private_team(self):
68+ product = self.factory.makeProduct()
69+ bug = self.factory.makeBug(product=product)
70+ member = self.factory.makePerson()
71+ team = self.factory.makeTeam(
72+ owner=member, visibility=PersonVisibility.PRIVATE)
73+ with person_logged_in(member):
74+ bug.subscribe(team, member)
75+ self.assertTrue(team in bug.getDirectSubscribers())
76+
77+ def test_get_subscribers_from_duplicates_with_private_team(self):
78+ product = self.factory.makeProduct()
79+ bug = self.factory.makeBug(product=product)
80+ dupe_bug = self.factory.makeBug()
81+ member = self.factory.makePerson()
82+ team = self.factory.makeTeam(
83+ owner=member, visibility=PersonVisibility.PRIVATE)
84+ with person_logged_in(member):
85+ dupe_bug.subscribe(team, member)
86+ dupe_bug.markAsDuplicate(bug)
87+ self.assertTrue(team in bug.getSubscribersFromDuplicates())