Merge lp:~gmb/launchpad/rollback-13050 into lp:launchpad
- rollback-13050
- Merge into devel
Proposed by
Graham Binns
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 13065 | ||||
Proposed branch: | lp:~gmb/launchpad/rollback-13050 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
257 lines (+9/-149) 5 files modified
lib/lp/bugs/browser/bug.py (+3/-2) lib/lp/bugs/browser/tests/test_bug_views.py (+0/-61) lib/lp/bugs/doc/bugsubscription.txt (+1/-1) lib/lp/bugs/model/bug.py (+5/-62) lib/lp/bugs/tests/test_bugsubscription.py (+0/-23) |
||||
To merge this branch: | bzr merge lp:~gmb/launchpad/rollback-13050 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | Approve | ||
Review via email: mp+61265@code.launchpad.net |
Commit message
[r=gmb][ui=none][bug=783948] [rollback=13050] Revert bad revision r13050, which may have caused bug 783948.
Description of the change
This branch reverts devel r13050, which may have resulted in bug 783948 manifesting.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/bugs/browser/bug.py' | |||
2 | --- lib/lp/bugs/browser/bug.py 2011-05-16 17:43:29 +0000 | |||
3 | +++ lib/lp/bugs/browser/bug.py 2011-05-17 15:03:32 +0000 | |||
4 | @@ -536,7 +536,6 @@ | |||
5 | 536 | @cachedproperty | 536 | @cachedproperty |
6 | 537 | def user_should_see_mute_link(self): | 537 | def user_should_see_mute_link(self): |
7 | 538 | """Return True if the user should see the Mute link.""" | 538 | """Return True if the user should see the Mute link.""" |
8 | 539 | user_is_subscribed = False | ||
9 | 540 | if features.getFeatureFlag('malone.advanced-subscriptions.enabled'): | 539 | if features.getFeatureFlag('malone.advanced-subscriptions.enabled'): |
10 | 541 | user_is_subscribed = ( | 540 | user_is_subscribed = ( |
11 | 542 | # Note that we don't have to check for isMuted(), since | 541 | # Note that we don't have to check for isMuted(), since |
12 | @@ -545,7 +544,9 @@ | |||
13 | 545 | self.context.isSubscribed(self.user) or | 544 | self.context.isSubscribed(self.user) or |
14 | 546 | self.context.isSubscribedToDupes(self.user) or | 545 | self.context.isSubscribedToDupes(self.user) or |
15 | 547 | self.context.personIsAlsoNotifiedSubscriber(self.user)) | 546 | self.context.personIsAlsoNotifiedSubscriber(self.user)) |
17 | 548 | return user_is_subscribed | 547 | return user_is_subscribed |
18 | 548 | else: | ||
19 | 549 | return False | ||
20 | 549 | 550 | ||
21 | 550 | @cachedproperty | 551 | @cachedproperty |
22 | 551 | def _bug_attachments(self): | 552 | def _bug_attachments(self): |
23 | 552 | 553 | ||
24 | === modified file 'lib/lp/bugs/browser/tests/test_bug_views.py' | |||
25 | --- lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-12 10:36:43 +0000 | |||
26 | +++ lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-17 15:03:32 +0000 | |||
27 | @@ -177,64 +177,3 @@ | |||
28 | 177 | self.assertTrue( | 177 | self.assertTrue( |
29 | 178 | self._hasCSSClass(html, 'mute-link-container', 'hidden'), | 178 | self._hasCSSClass(html, 'mute-link-container', 'hidden'), |
30 | 179 | 'No "hidden" CSS class in mute-link-container.') | 179 | 'No "hidden" CSS class in mute-link-container.') |
31 | 180 | |||
32 | 181 | def test_mute_link_shown_for_team_subscription_to_duplicate(self): | ||
33 | 182 | # If the person belongs to a team with a structural subscription to a | ||
34 | 183 | # duplicate of this bug, then the mute link will be displayed to them. | ||
35 | 184 | person = self.factory.makePerson(name="a-person") | ||
36 | 185 | team_owner = self.factory.makePerson(name="team-owner") | ||
37 | 186 | team = self.factory.makeTeam(owner=team_owner, name="subscribed-team") | ||
38 | 187 | dupe = self.factory.makeBug() | ||
39 | 188 | with person_logged_in(self.bug.owner): | ||
40 | 189 | dupe.markAsDuplicate(self.bug) | ||
41 | 190 | with FeatureFixture({self.feature_flag_1: 'on'}): | ||
42 | 191 | with person_logged_in(team_owner): | ||
43 | 192 | team.addMember(person, team_owner) | ||
44 | 193 | dupe_target = dupe.default_bugtask.target | ||
45 | 194 | dupe_target.addBugSubscription(team, team_owner) | ||
46 | 195 | with person_logged_in(person): | ||
47 | 196 | self.assertFalse(self.bug.isMuted(person)) | ||
48 | 197 | # This is a sanity check for the test. | ||
49 | 198 | self.assertFalse(self.bug.duplicates.is_empty()) | ||
50 | 199 | self.assertEqual(self.bug, dupe.duplicateof) | ||
51 | 200 | self.assertTrue( | ||
52 | 201 | self.bug.personIsAlsoNotifiedSubscriber( | ||
53 | 202 | person), "Person should be a notified subscriber") | ||
54 | 203 | view = create_initialized_view( | ||
55 | 204 | self.bug, name="+portlet-subscribers") | ||
56 | 205 | self.assertTrue(view.user_should_see_mute_link, | ||
57 | 206 | "User should see mute link.") | ||
58 | 207 | contents = view.render() | ||
59 | 208 | self.assertTrue('mute_subscription' in contents, | ||
60 | 209 | "'mute_subscription' not in contents.") | ||
61 | 210 | create_initialized_view( | ||
62 | 211 | self.bug.default_bugtask, name="+mute", | ||
63 | 212 | form={'field.actions.mute': 'Mute bug mail'}) | ||
64 | 213 | self.assertTrue(self.bug.isMuted(person)) | ||
65 | 214 | |||
66 | 215 | def test_mute_subscription_link_shown_for_team_direct_subscription(self): | ||
67 | 216 | # If the person belongs to a team with a direct subscription, | ||
68 | 217 | # then the mute link will be displayed to them. | ||
69 | 218 | person = self.factory.makePerson(name="a-person") | ||
70 | 219 | team_owner = self.factory.makePerson(name="team-owner") | ||
71 | 220 | team = self.factory.makeTeam(owner=team_owner, name="subscribed-team") | ||
72 | 221 | with FeatureFixture({self.feature_flag_1: 'on'}): | ||
73 | 222 | with person_logged_in(team_owner): | ||
74 | 223 | team.addMember(person, team_owner) | ||
75 | 224 | self.bug.subscribe(team, team_owner) | ||
76 | 225 | with person_logged_in(person): | ||
77 | 226 | self.assertFalse(self.bug.isMuted(person)) | ||
78 | 227 | self.assertTrue( | ||
79 | 228 | self.bug.personIsAlsoNotifiedSubscriber(person), | ||
80 | 229 | "Person should be a notified subscriber") | ||
81 | 230 | view = create_initialized_view( | ||
82 | 231 | self.bug, name="+portlet-subscribers") | ||
83 | 232 | self.assertTrue(view.user_should_see_mute_link, | ||
84 | 233 | "User should see mute link.") | ||
85 | 234 | contents = view.render() | ||
86 | 235 | self.assertTrue('mute_subscription' in contents, | ||
87 | 236 | "'mute_subscription' not in contents.") | ||
88 | 237 | create_initialized_view( | ||
89 | 238 | self.bug.default_bugtask, name="+mute", | ||
90 | 239 | form={'field.actions.mute': 'Mute bug mail'}) | ||
91 | 240 | self.assertTrue(self.bug.isMuted(person)) | ||
92 | 241 | 180 | ||
93 | === modified file 'lib/lp/bugs/doc/bugsubscription.txt' | |||
94 | --- lib/lp/bugs/doc/bugsubscription.txt 2011-05-16 17:40:00 +0000 | |||
95 | +++ lib/lp/bugs/doc/bugsubscription.txt 2011-05-17 15:03:32 +0000 | |||
96 | @@ -423,7 +423,7 @@ | |||
97 | 423 | [] | 423 | [] |
98 | 424 | 424 | ||
99 | 425 | >>> linux_source_bug.getSubscribersFromDuplicates() | 425 | >>> linux_source_bug.getSubscribersFromDuplicates() |
101 | 426 | [] | 426 | () |
102 | 427 | 427 | ||
103 | 428 | Direct subscriptions always take precedence over indirect | 428 | Direct subscriptions always take precedence over indirect |
104 | 429 | subscriptions. So, if we unmark the above bug as private, | 429 | subscriptions. So, if we unmark the above bug as private, |
105 | 430 | 430 | ||
106 | === modified file 'lib/lp/bugs/model/bug.py' | |||
107 | --- lib/lp/bugs/model/bug.py 2011-05-17 05:43:19 +0000 | |||
108 | +++ lib/lp/bugs/model/bug.py 2011-05-17 15:03:32 +0000 | |||
109 | @@ -953,30 +953,18 @@ | |||
110 | 953 | See the comment in getDirectSubscribers for a description of the | 953 | See the comment in getDirectSubscribers for a description of the |
111 | 954 | recipients argument. | 954 | recipients argument. |
112 | 955 | """ | 955 | """ |
113 | 956 | if self.private: | ||
114 | 957 | # We short-circuit for private bugs, since actually | ||
115 | 958 | # returning something non-empty here causes things to break | ||
116 | 959 | # in fun and interesting ways (see bug 780248). | ||
117 | 960 | return [] | ||
118 | 961 | |||
119 | 962 | if level is None: | 956 | if level is None: |
120 | 963 | level = BugNotificationLevel.NOTHING | 957 | level = BugNotificationLevel.NOTHING |
121 | 964 | info = self.getSubscriptionInfo(level) | 958 | info = self.getSubscriptionInfo(level) |
122 | 965 | 959 | ||
123 | 966 | if recipients is not None: | 960 | if recipients is not None: |
125 | 967 | # Pre-load duplicates | 961 | # Pre-load duplicate bugs. |
126 | 968 | list(self.duplicates) | 962 | list(self.duplicates) |
127 | 969 | for subscription in info.duplicate_only_subscriptions: | 963 | for subscription in info.duplicate_only_subscriptions: |
128 | 970 | recipients.addDupeSubscriber( | 964 | recipients.addDupeSubscriber( |
129 | 971 | subscription.person, subscription.bug) | 965 | subscription.person, subscription.bug) |
130 | 972 | for subscription in info.structural_subscriptions_from_duplicates: | ||
131 | 973 | recipients.addDupeSubscriber( | ||
132 | 974 | subscription.subscriber) | ||
133 | 975 | 966 | ||
138 | 976 | unified_subscribers = ( | 967 | return info.duplicate_only_subscriptions.subscribers.sorted |
135 | 977 | info.duplicate_only_subscriptions.subscribers.union( | ||
136 | 978 | info.structural_subscriptions_from_duplicates.subscribers)) | ||
137 | 979 | return unified_subscribers.sorted | ||
139 | 980 | 968 | ||
140 | 981 | def getSubscribersForPerson(self, person): | 969 | def getSubscribersForPerson(self, person): |
141 | 982 | """See `IBug.""" | 970 | """See `IBug.""" |
142 | @@ -1041,16 +1029,12 @@ | |||
143 | 1041 | include_master_dupe_subscribers=False): | 1029 | include_master_dupe_subscribers=False): |
144 | 1042 | """See `IBug`.""" | 1030 | """See `IBug`.""" |
145 | 1043 | recipients = BugNotificationRecipients(duplicateof=duplicateof) | 1031 | recipients = BugNotificationRecipients(duplicateof=duplicateof) |
146 | 1044 | # Call getDirectSubscribers to update the recipients list with direct | ||
147 | 1045 | # subscribers. The results of the method call are not used. | ||
148 | 1046 | self.getDirectSubscribers(recipients, level=level) | 1032 | self.getDirectSubscribers(recipients, level=level) |
149 | 1047 | if self.private: | 1033 | if self.private: |
150 | 1048 | assert self.getIndirectSubscribers() == [], ( | 1034 | assert self.getIndirectSubscribers() == [], ( |
151 | 1049 | "Indirect subscribers found on private bug. " | 1035 | "Indirect subscribers found on private bug. " |
152 | 1050 | "A private bug should never have implicit subscribers!") | 1036 | "A private bug should never have implicit subscribers!") |
153 | 1051 | else: | 1037 | else: |
154 | 1052 | # Call getIndirectSubscribers to update the recipients list with direct | ||
155 | 1053 | # subscribers. The results of the method call are not used. | ||
156 | 1054 | self.getIndirectSubscribers(recipients, level=level) | 1038 | self.getIndirectSubscribers(recipients, level=level) |
157 | 1055 | if include_master_dupe_subscribers and self.duplicateof: | 1039 | if include_master_dupe_subscribers and self.duplicateof: |
158 | 1056 | # This bug is a public duplicate of another bug, so include | 1040 | # This bug is a public duplicate of another bug, so include |
159 | @@ -1917,22 +1901,6 @@ | |||
160 | 1917 | 1901 | ||
161 | 1918 | def personIsAlsoNotifiedSubscriber(self, person): | 1902 | def personIsAlsoNotifiedSubscriber(self, person): |
162 | 1919 | """See `IBug`.""" | 1903 | """See `IBug`.""" |
163 | 1920 | # This is here to avoid circular imports. | ||
164 | 1921 | from lp.bugs.mail.bugnotificationrecipients import ( | ||
165 | 1922 | BugNotificationRecipients, | ||
166 | 1923 | ) | ||
167 | 1924 | |||
168 | 1925 | def check_person_in_team(person, list_of_people): | ||
169 | 1926 | """Is the person in one of the teams? | ||
170 | 1927 | |||
171 | 1928 | Given a person and a list of people/teams, see if the person | ||
172 | 1929 | belongs to one of the teams. | ||
173 | 1930 | """ | ||
174 | 1931 | for subscriber in list_of_people: | ||
175 | 1932 | if subscriber.is_team and person.inTeam(subscriber): | ||
176 | 1933 | return True | ||
177 | 1934 | return False | ||
178 | 1935 | |||
179 | 1936 | # We have to use getAlsoNotifiedSubscribers() here and iterate | 1904 | # We have to use getAlsoNotifiedSubscribers() here and iterate |
180 | 1937 | # over what it returns because "also notified subscribers" is | 1905 | # over what it returns because "also notified subscribers" is |
181 | 1938 | # actually a composite of bug contacts, structural subscribers | 1906 | # actually a composite of bug contacts, structural subscribers |
182 | @@ -1943,16 +1911,9 @@ | |||
183 | 1943 | return True | 1911 | return True |
184 | 1944 | # Otherwise check to see if the person is a member of any of the | 1912 | # Otherwise check to see if the person is a member of any of the |
185 | 1945 | # subscribed teams. | 1913 | # subscribed teams. |
196 | 1946 | if check_person_in_team(person, also_notified_subscribers): | 1914 | for subscriber in also_notified_subscribers: |
197 | 1947 | return True | 1915 | if subscriber.is_team and person.inTeam(subscriber): |
198 | 1948 | 1916 | return True | |
189 | 1949 | direct_subscribers = self.getDirectSubscribers() | ||
190 | 1950 | if check_person_in_team(person, direct_subscribers): | ||
191 | 1951 | return True | ||
192 | 1952 | duplicate_subscribers = self.getSubscribersFromDuplicates( | ||
193 | 1953 | recipients=BugNotificationRecipients()) | ||
194 | 1954 | if check_person_in_team(person, duplicate_subscribers): | ||
195 | 1955 | return True | ||
199 | 1956 | return False | 1917 | return False |
200 | 1957 | 1918 | ||
201 | 1958 | def personIsSubscribedToDuplicate(self, person): | 1919 | def personIsSubscribedToDuplicate(self, person): |
202 | @@ -2312,24 +2273,6 @@ | |||
203 | 2312 | return get_structural_subscriptions_for_bug(self.bug) | 2273 | return get_structural_subscriptions_for_bug(self.bug) |
204 | 2313 | 2274 | ||
205 | 2314 | @cachedproperty | 2275 | @cachedproperty |
206 | 2315 | @freeze(StructuralSubscriptionSet) | ||
207 | 2316 | def structural_subscriptions_from_duplicates(self): | ||
208 | 2317 | """Structural subscriptions from the bug's duplicates.""" | ||
209 | 2318 | self.duplicate_subscriptions.subscribers # Pre-load subscribers. | ||
210 | 2319 | higher_precedence = ( | ||
211 | 2320 | self.direct_subscriptions.subscribers.union( | ||
212 | 2321 | self.also_notified_subscribers)) | ||
213 | 2322 | all_duplicate_structural_subscriptions = list() | ||
214 | 2323 | for duplicate in self.bug.duplicates: | ||
215 | 2324 | duplicate_struct_subs = get_structural_subscriptions_for_bug( | ||
216 | 2325 | duplicate) | ||
217 | 2326 | all_duplicate_structural_subscriptions += duplicate_struct_subs | ||
218 | 2327 | return ( | ||
219 | 2328 | subscription for subscription in | ||
220 | 2329 | all_duplicate_structural_subscriptions | ||
221 | 2330 | if subscription.subscriber not in higher_precedence) | ||
222 | 2331 | |||
223 | 2332 | @cachedproperty | ||
224 | 2333 | @freeze(BugSubscriberSet) | 2276 | @freeze(BugSubscriberSet) |
225 | 2334 | def all_assignees(self): | 2277 | def all_assignees(self): |
226 | 2335 | """Assignees of the bug's tasks.""" | 2278 | """Assignees of the bug's tasks.""" |
227 | 2336 | 2279 | ||
228 | === modified file 'lib/lp/bugs/tests/test_bugsubscription.py' | |||
229 | --- lib/lp/bugs/tests/test_bugsubscription.py 2011-05-10 14:13:24 +0000 | |||
230 | +++ lib/lp/bugs/tests/test_bugsubscription.py 2011-05-17 15:03:32 +0000 | |||
231 | @@ -146,26 +146,3 @@ | |||
232 | 146 | self.bug.id, team_2.name, self.subscriber) | 146 | self.bug.id, team_2.name, self.subscriber) |
233 | 147 | self.assertThat( | 147 | self.assertThat( |
234 | 148 | collector, HasQueryCount(LessThan(25))) | 148 | collector, HasQueryCount(LessThan(25))) |
235 | 149 | |||
236 | 150 | |||
237 | 151 | class TestBugSubscriptionMethods(TestCaseWithFactory): | ||
238 | 152 | """A TestCase for the subscription-related methods of `Bug`.""" | ||
239 | 153 | |||
240 | 154 | layer = DatabaseFunctionalLayer | ||
241 | 155 | |||
242 | 156 | def test_getSubscribersFromDuplicates_returns_empty_when_private(self): | ||
243 | 157 | # If a private bug has duplicates its | ||
244 | 158 | # getSubscribersFromDuplicates() method should return an empty | ||
245 | 159 | # set. | ||
246 | 160 | # This is a regression test for bug 780248. | ||
247 | 161 | bug = self.factory.makeBug() | ||
248 | 162 | duplicate = self.factory.makeBug() | ||
249 | 163 | team = self.factory.makeTeam() | ||
250 | 164 | with person_logged_in(team.teamowner): | ||
251 | 165 | duplicate.default_bugtask.product.addSubscription( | ||
252 | 166 | team, team.teamowner) | ||
253 | 167 | with person_logged_in(bug.owner): | ||
254 | 168 | bug.setPrivate(True, bug.owner) | ||
255 | 169 | duplicate.markAsDuplicate(bug) | ||
256 | 170 | duplicate_subscribers = bug.getSubscribersFromDuplicates() | ||
257 | 171 | self.assertEqual(0, len(duplicate_subscribers)) |
Self-approving after checking that rolling back fixed the problem.