Merge lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel
- bug164196-2
- Merge into db-devel
Status: | Merged |
---|---|
Approved by: | Данило Шеган |
Approved revision: | no longer in the source branch. |
Merged at revision: | 10215 |
Proposed branch: | lp:~gary/launchpad/bug164196-2 |
Merge into: | lp:launchpad/db-devel |
Prerequisite: | lp:~gary/launchpad/bug164196-1 |
Diff against target: |
1212 lines (+691/-80) 8 files modified
lib/lp/bugs/adapters/bugchange.py (+38/-12) lib/lp/bugs/browser/bugtask.py (+13/-6) lib/lp/bugs/doc/bugactivity.txt (+8/-3) lib/lp/bugs/doc/bugnotification-sending.txt (+23/-21) lib/lp/bugs/model/bugactivity.py (+35/-3) lib/lp/bugs/scripts/bugnotification.py (+64/-18) lib/lp/bugs/scripts/tests/test_bugnotification.py (+426/-16) lib/lp/bugs/tests/test_bugchanges.py (+84/-1) |
To merge this branch: | bzr merge lp:~gary/launchpad/bug164196-2 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Данило Шеган (community) | Approve | ||
Review via email: mp+49977@code.launchpad.net |
Commit message
use the activity attributes on bug notifications to implement the logic to silence emails for actions that are done and then undone in rapid succession.
Description of the change
This is the second of three branches that address bug 164196. The other two are lp:~gary/launchpad/bug164196-1 and lp:~gary/launchpad/bug164196-3. My pre-implementation call for these changes was with Graham Binns.
This branch uses the activity attributes on bug notifications introduced in the first branch to implement the logic to silence notifications for actions that are done and then undone in rapid succession (that is, to do what the bug requests). Undone actions are omitted from emails, and if the emails then have no content, they are not sent.
The only thing left to do is to make sure that the omitted notification objects are marked as processed so that they are not perpetually considered for subsequent notification cronscript runs. This is tackled in the last branch of the series.
The implementation is all in lib/lp/
`get_key` probably holds the subtlest code here: we normalize the bug activity's data to give us a key that will work for items that are linked/added and unlinked/removed. For instance, if a change adds one branch but then removes another, both of these should be reported. We can only squelch the notification if the same branch is added and removed. A less subtle but important part of get_key is that it normalizes the "duplicate" names. Unlike other changes, the activity text changes depending on what happens, so we need to normalize that here.
I renamed the local variable "person_
The bulk of the branch is the tests for these relatively small changes. I changed the doctest enough for it to pass (and mention the behavior in passing), and updated some test infrastructure in lib/lp/
Speaking of subclasses, I don't love what I've done to assemble these tests from subclasses and mixins, but it certainly has precedence in our codebase, and the alternative, with lots of repetition, is even less attractive.
I removed the "def test_suite():" boilerplate because, AIUI, we have now 'fessed up to the fact that it didn't accomplish anything practical, and are actively encouraging removal.
Thank you
Gary Poster (gary) wrote : | # |
I'll wait for Данило's review of my changes, because they ended up being fairly extensive.
Данило Шеган (danilo) wrote : | # |
Gary, thanks for the improvements.
This looks much, much, nicer — sorry for putting you through the
trouble, but I think it was worth it. :)
review approve
merge approve
Just one minor tidbit below:
>=== modified file 'lib/lp/
...
>+
>+class MockBugActivity:
>+ """A mock BugActivity user for testing."""
A typo most likely: s/user/used/.
Cheers,
Danilo
Robert Collins (lifeless) wrote : | # |
BTW, there are mocks for bugactivity etc in the bugcomment tests
today; may want to snarf those and reused (if you haven't already, I
haven't read your diff).
Cheers,
Rob
Preview Diff
1 | === modified file 'lib/lp/bugs/adapters/bugchange.py' | |||
2 | --- lib/lp/bugs/adapters/bugchange.py 2011-02-04 01:15:13 +0000 | |||
3 | +++ lib/lp/bugs/adapters/bugchange.py 2011-02-17 17:46:51 +0000 | |||
4 | @@ -5,8 +5,20 @@ | |||
5 | 5 | 5 | ||
6 | 6 | __metaclass__ = type | 6 | __metaclass__ = type |
7 | 7 | __all__ = [ | 7 | __all__ = [ |
8 | 8 | 'ATTACHMENT_ADDED', | ||
9 | 9 | 'ATTACHMENT_REMOVED', | ||
10 | 10 | 'BRANCH_LINKED', | ||
11 | 11 | 'BRANCH_UNLINKED', | ||
12 | 12 | 'BUG_WATCH_ADDED', | ||
13 | 13 | 'BUG_WATCH_REMOVED', | ||
14 | 14 | 'CHANGED_DUPLICATE_MARKER', | ||
15 | 15 | 'CVE_LINKED', | ||
16 | 16 | 'CVE_UNLINKED', | ||
17 | 17 | 'MARKED_AS_DUPLICATE', | ||
18 | 18 | 'REMOVED_DUPLICATE_MARKER', | ||
19 | 8 | 'BranchLinkedToBug', | 19 | 'BranchLinkedToBug', |
20 | 9 | 'BranchUnlinkedFromBug', | 20 | 'BranchUnlinkedFromBug', |
21 | 21 | 'BugAttachmentChange', | ||
22 | 10 | 'BugConvertedToQuestion', | 22 | 'BugConvertedToQuestion', |
23 | 11 | 'BugDescriptionChange', | 23 | 'BugDescriptionChange', |
24 | 12 | 'BugDuplicateChange', | 24 | 'BugDuplicateChange', |
25 | @@ -40,13 +52,27 @@ | |||
26 | 40 | from lp.bugs.enum import BugNotificationLevel | 52 | from lp.bugs.enum import BugNotificationLevel |
27 | 41 | from lp.bugs.interfaces.bugchange import IBugChange | 53 | from lp.bugs.interfaces.bugchange import IBugChange |
28 | 42 | from lp.bugs.interfaces.bugtask import ( | 54 | from lp.bugs.interfaces.bugtask import ( |
29 | 43 | BugTaskStatus, | ||
30 | 44 | IBugTask, | 55 | IBugTask, |
31 | 45 | RESOLVED_BUGTASK_STATUSES, | 56 | RESOLVED_BUGTASK_STATUSES, |
32 | 46 | UNRESOLVED_BUGTASK_STATUSES, | 57 | UNRESOLVED_BUGTASK_STATUSES, |
33 | 47 | ) | 58 | ) |
34 | 48 | from lp.registry.interfaces.product import IProduct | 59 | from lp.registry.interfaces.product import IProduct |
35 | 49 | 60 | ||
36 | 61 | # These are used lp.bugs.model.bugactivity.BugActivity.attribute to normalize | ||
37 | 62 | # the output from these change objects into the attribute that actually | ||
38 | 63 | # changed. It is fragile, but a reasonable incremental step. | ||
39 | 64 | ATTACHMENT_ADDED = "attachment added" | ||
40 | 65 | ATTACHMENT_REMOVED = "attachment removed" | ||
41 | 66 | BRANCH_LINKED = 'branch linked' | ||
42 | 67 | BRANCH_UNLINKED = 'branch unlinked' | ||
43 | 68 | BUG_WATCH_ADDED = 'bug watch added' | ||
44 | 69 | BUG_WATCH_REMOVED = 'bug watch removed' | ||
45 | 70 | CHANGED_DUPLICATE_MARKER = 'changed duplicate marker' | ||
46 | 71 | CVE_LINKED = 'cve linked' | ||
47 | 72 | CVE_UNLINKED = 'cve unlinked' | ||
48 | 73 | MARKED_AS_DUPLICATE = 'marked as duplicate' | ||
49 | 74 | REMOVED_DUPLICATE_MARKER = 'removed duplicate marker' | ||
50 | 75 | |||
51 | 50 | 76 | ||
52 | 51 | class NoBugChangeFoundError(Exception): | 77 | class NoBugChangeFoundError(Exception): |
53 | 52 | """Raised when a BugChange class can't be found for an object.""" | 78 | """Raised when a BugChange class can't be found for an object.""" |
54 | @@ -254,7 +280,7 @@ | |||
55 | 254 | def getBugActivity(self): | 280 | def getBugActivity(self): |
56 | 255 | """See `IBugChange`.""" | 281 | """See `IBugChange`.""" |
57 | 256 | return dict( | 282 | return dict( |
59 | 257 | whatchanged='bug watch added', | 283 | whatchanged=BUG_WATCH_ADDED, |
60 | 258 | newvalue=self.bug_watch.url) | 284 | newvalue=self.bug_watch.url) |
61 | 259 | 285 | ||
62 | 260 | def getBugNotification(self): | 286 | def getBugNotification(self): |
63 | @@ -278,7 +304,7 @@ | |||
64 | 278 | def getBugActivity(self): | 304 | def getBugActivity(self): |
65 | 279 | """See `IBugChange`.""" | 305 | """See `IBugChange`.""" |
66 | 280 | return dict( | 306 | return dict( |
68 | 281 | whatchanged='bug watch removed', | 307 | whatchanged=BUG_WATCH_REMOVED, |
69 | 282 | oldvalue=self.bug_watch.url) | 308 | oldvalue=self.bug_watch.url) |
70 | 283 | 309 | ||
71 | 284 | def getBugNotification(self): | 310 | def getBugNotification(self): |
72 | @@ -305,7 +331,7 @@ | |||
73 | 305 | if self.branch.private: | 331 | if self.branch.private: |
74 | 306 | return None | 332 | return None |
75 | 307 | return dict( | 333 | return dict( |
77 | 308 | whatchanged='branch linked', | 334 | whatchanged=BRANCH_LINKED, |
78 | 309 | newvalue=self.branch.bzr_identity) | 335 | newvalue=self.branch.bzr_identity) |
79 | 310 | 336 | ||
80 | 311 | def getBugNotification(self): | 337 | def getBugNotification(self): |
81 | @@ -328,7 +354,7 @@ | |||
82 | 328 | if self.branch.private: | 354 | if self.branch.private: |
83 | 329 | return None | 355 | return None |
84 | 330 | return dict( | 356 | return dict( |
86 | 331 | whatchanged='branch unlinked', | 357 | whatchanged=BRANCH_UNLINKED, |
87 | 332 | oldvalue=self.branch.bzr_identity) | 358 | oldvalue=self.branch.bzr_identity) |
88 | 333 | 359 | ||
89 | 334 | def getBugNotification(self): | 360 | def getBugNotification(self): |
90 | @@ -397,18 +423,18 @@ | |||
91 | 397 | def getBugActivity(self): | 423 | def getBugActivity(self): |
92 | 398 | if self.old_value is not None and self.new_value is not None: | 424 | if self.old_value is not None and self.new_value is not None: |
93 | 399 | return { | 425 | return { |
95 | 400 | 'whatchanged': 'changed duplicate marker', | 426 | 'whatchanged': CHANGED_DUPLICATE_MARKER, |
96 | 401 | 'oldvalue': str(self.old_value.id), | 427 | 'oldvalue': str(self.old_value.id), |
97 | 402 | 'newvalue': str(self.new_value.id), | 428 | 'newvalue': str(self.new_value.id), |
98 | 403 | } | 429 | } |
99 | 404 | elif self.old_value is None: | 430 | elif self.old_value is None: |
100 | 405 | return { | 431 | return { |
102 | 406 | 'whatchanged': 'marked as duplicate', | 432 | 'whatchanged': MARKED_AS_DUPLICATE, |
103 | 407 | 'newvalue': str(self.new_value.id), | 433 | 'newvalue': str(self.new_value.id), |
104 | 408 | } | 434 | } |
105 | 409 | elif self.new_value is None: | 435 | elif self.new_value is None: |
106 | 410 | return { | 436 | return { |
108 | 411 | 'whatchanged': 'removed duplicate marker', | 437 | 'whatchanged': REMOVED_DUPLICATE_MARKER, |
109 | 412 | 'oldvalue': str(self.old_value.id), | 438 | 'oldvalue': str(self.old_value.id), |
110 | 413 | } | 439 | } |
111 | 414 | else: | 440 | else: |
112 | @@ -605,13 +631,13 @@ | |||
113 | 605 | 631 | ||
114 | 606 | def getBugActivity(self): | 632 | def getBugActivity(self): |
115 | 607 | if self.old_value is None: | 633 | if self.old_value is None: |
117 | 608 | what_changed = "attachment added" | 634 | what_changed = ATTACHMENT_ADDED |
118 | 609 | old_value = None | 635 | old_value = None |
119 | 610 | new_value = "%s %s" % ( | 636 | new_value = "%s %s" % ( |
120 | 611 | self.new_value.title, | 637 | self.new_value.title, |
121 | 612 | download_url_of_bugattachment(self.new_value)) | 638 | download_url_of_bugattachment(self.new_value)) |
122 | 613 | else: | 639 | else: |
124 | 614 | what_changed = "attachment removed" | 640 | what_changed = ATTACHMENT_REMOVED |
125 | 615 | attachment = self.new_value | 641 | attachment = self.new_value |
126 | 616 | old_value = "%s %s" % ( | 642 | old_value = "%s %s" % ( |
127 | 617 | self.old_value.title, | 643 | self.old_value.title, |
128 | @@ -656,7 +682,7 @@ | |||
129 | 656 | """See `IBugChange`.""" | 682 | """See `IBugChange`.""" |
130 | 657 | return dict( | 683 | return dict( |
131 | 658 | newvalue=self.cve.sequence, | 684 | newvalue=self.cve.sequence, |
133 | 659 | whatchanged='cve linked') | 685 | whatchanged=CVE_LINKED) |
134 | 660 | 686 | ||
135 | 661 | def getBugNotification(self): | 687 | def getBugNotification(self): |
136 | 662 | """See `IBugChange`.""" | 688 | """See `IBugChange`.""" |
137 | @@ -674,7 +700,7 @@ | |||
138 | 674 | """See `IBugChange`.""" | 700 | """See `IBugChange`.""" |
139 | 675 | return dict( | 701 | return dict( |
140 | 676 | oldvalue=self.cve.sequence, | 702 | oldvalue=self.cve.sequence, |
142 | 677 | whatchanged='cve unlinked') | 703 | whatchanged=CVE_UNLINKED) |
143 | 678 | 704 | ||
144 | 679 | def getBugNotification(self): | 705 | def getBugNotification(self): |
145 | 680 | """See `IBugChange`.""" | 706 | """See `IBugChange`.""" |
146 | 681 | 707 | ||
147 | === modified file 'lib/lp/bugs/browser/bugtask.py' | |||
148 | --- lib/lp/bugs/browser/bugtask.py 2011-02-16 02:55:17 +0000 | |||
149 | +++ lib/lp/bugs/browser/bugtask.py 2011-02-17 17:46:51 +0000 | |||
150 | @@ -3814,7 +3814,13 @@ | |||
151 | 3814 | @property | 3814 | @property |
152 | 3815 | def change_summary(self): | 3815 | def change_summary(self): |
153 | 3816 | """Return a formatted summary of the change.""" | 3816 | """Return a formatted summary of the change.""" |
155 | 3817 | return self.attribute | 3817 | if self.target is not None: |
156 | 3818 | # This is a bug task. We want the attribute, as filtered out. | ||
157 | 3819 | return self.attribute | ||
158 | 3820 | else: | ||
159 | 3821 | # Otherwise, the attribute is more normalized than what we want. | ||
160 | 3822 | # Use "whatchanged," which sometimes is more descriptive. | ||
161 | 3823 | return self.whatchanged | ||
162 | 3818 | 3824 | ||
163 | 3819 | @property | 3825 | @property |
164 | 3820 | def _formatted_tags_change(self): | 3826 | def _formatted_tags_change(self): |
165 | @@ -3858,30 +3864,31 @@ | |||
166 | 3858 | 'old_value': self.oldvalue, | 3864 | 'old_value': self.oldvalue, |
167 | 3859 | 'new_value': self.newvalue, | 3865 | 'new_value': self.newvalue, |
168 | 3860 | } | 3866 | } |
170 | 3861 | if self.attribute == 'summary': | 3867 | attribute = self.attribute |
171 | 3868 | if attribute == 'title': | ||
172 | 3862 | # We display summary changes as a unified diff, replacing | 3869 | # We display summary changes as a unified diff, replacing |
173 | 3863 | # \ns with <br />s so that the lines are separated properly. | 3870 | # \ns with <br />s so that the lines are separated properly. |
174 | 3864 | diff = cgi.escape( | 3871 | diff = cgi.escape( |
175 | 3865 | get_unified_diff(self.oldvalue, self.newvalue, 72), True) | 3872 | get_unified_diff(self.oldvalue, self.newvalue, 72), True) |
176 | 3866 | return diff.replace("\n", "<br />") | 3873 | return diff.replace("\n", "<br />") |
177 | 3867 | 3874 | ||
179 | 3868 | elif self.attribute == 'description': | 3875 | elif attribute == 'description': |
180 | 3869 | # Description changes can be quite long, so we just return | 3876 | # Description changes can be quite long, so we just return |
181 | 3870 | # 'updated' rather than returning the whole new description | 3877 | # 'updated' rather than returning the whole new description |
182 | 3871 | # or a diff. | 3878 | # or a diff. |
183 | 3872 | return 'updated' | 3879 | return 'updated' |
184 | 3873 | 3880 | ||
186 | 3874 | elif self.attribute == 'tags': | 3881 | elif attribute == 'tags': |
187 | 3875 | # We special-case tags because we can work out what's been | 3882 | # We special-case tags because we can work out what's been |
188 | 3876 | # added and what's been removed. | 3883 | # added and what's been removed. |
189 | 3877 | return self._formatted_tags_change.replace('\n', '<br />') | 3884 | return self._formatted_tags_change.replace('\n', '<br />') |
190 | 3878 | 3885 | ||
192 | 3879 | elif self.attribute == 'assignee': | 3886 | elif attribute == 'assignee': |
193 | 3880 | for key in return_dict: | 3887 | for key in return_dict: |
194 | 3881 | if return_dict[key] is None: | 3888 | if return_dict[key] is None: |
195 | 3882 | return_dict[key] = 'nobody' | 3889 | return_dict[key] = 'nobody' |
196 | 3883 | 3890 | ||
198 | 3884 | elif self.attribute == 'milestone': | 3891 | elif attribute == 'milestone': |
199 | 3885 | for key in return_dict: | 3892 | for key in return_dict: |
200 | 3886 | if return_dict[key] is None: | 3893 | if return_dict[key] is None: |
201 | 3887 | return_dict[key] = 'none' | 3894 | return_dict[key] = 'none' |
202 | 3888 | 3895 | ||
203 | === modified file 'lib/lp/bugs/doc/bugactivity.txt' | |||
204 | --- lib/lp/bugs/doc/bugactivity.txt 2011-02-11 18:33:04 +0000 | |||
205 | +++ lib/lp/bugs/doc/bugactivity.txt 2011-02-17 17:46:51 +0000 | |||
206 | @@ -113,13 +113,18 @@ | |||
207 | 113 | u'status' | 113 | u'status' |
208 | 114 | 114 | ||
209 | 115 | If the activity is not for a bug task, `target` is None, and `attribute` is | 115 | If the activity is not for a bug task, `target` is None, and `attribute` is |
212 | 116 | the same as `whatchanged`. For instance, look at the attributes on the | 116 | typically the same as `whatchanged`. However, in some cases (ideally, |
213 | 117 | previous activity. | 117 | whenever necessary), it is normalized to show the actual attribute name. |
214 | 118 | For instance, look at the attributes on the previous activity. | ||
215 | 118 | 119 | ||
216 | 119 | >>> print bug.activity[-2].target | 120 | >>> print bug.activity[-2].target |
217 | 120 | None | 121 | None |
218 | 122 | >>> bug.activity[-2].whatchanged | ||
219 | 123 | u'summary' | ||
220 | 121 | >>> bug.activity[-2].attribute | 124 | >>> bug.activity[-2].attribute |
222 | 122 | u'summary' | 125 | 'title' |
223 | 126 | |||
224 | 127 | (This is covered more comprehensively in tests/test_bugchanges.py). | ||
225 | 123 | 128 | ||
226 | 124 | Upstream product assignment edited | 129 | Upstream product assignment edited |
227 | 125 | ================================== | 130 | ================================== |
228 | 126 | 131 | ||
229 | === modified file 'lib/lp/bugs/doc/bugnotification-sending.txt' | |||
230 | --- lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-17 15:20:36 +0000 | |||
231 | +++ lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-17 17:46:51 +0000 | |||
232 | @@ -293,14 +293,10 @@ | |||
233 | 293 | - Old summary | 293 | - Old summary |
234 | 294 | + New summary | 294 | + New summary |
235 | 295 | <BLANKLINE> | 295 | <BLANKLINE> |
236 | 296 | ** Visibility changed to: Private | ||
237 | 297 | <BLANKLINE> | ||
238 | 298 | ** Summary changed: | 296 | ** Summary changed: |
239 | 299 | - New summary | 297 | - New summary |
240 | 300 | + Another summary | 298 | + Another summary |
241 | 301 | <BLANKLINE> | 299 | <BLANKLINE> |
242 | 302 | ** Visibility changed to: Public | ||
243 | 303 | <BLANKLINE> | ||
244 | 304 | -- | 300 | -- |
245 | 305 | You received this bug notification because you are a member of Ubuntu | 301 | You received this bug notification because you are a member of Ubuntu |
246 | 306 | Team, which is the registrant for Ubuntu. | 302 | Team, which is the registrant for Ubuntu. |
247 | @@ -312,6 +308,29 @@ | |||
248 | 312 | To: test@canonical.com | 308 | To: test@canonical.com |
249 | 313 | ... | 309 | ... |
250 | 314 | 310 | ||
251 | 311 | If you look carefully, there's a surprise in that output: the visibility | ||
252 | 312 | changes are not reported. This is because they are done and then undone | ||
253 | 313 | within the same notification. Undone changes like that are omitted. | ||
254 | 314 | moreover, if the email only would have reported done/undone changes, it | ||
255 | 315 | is not sent at all. This is tested elsewhere (see | ||
256 | 316 | lp/bugs/tests/test_bugnotification.py), and not demonstrated here. | ||
257 | 317 | |||
258 | 318 | Another thing worth noting is that there's a blank line before the | ||
259 | 319 | signature, and the signature marker has a trailing space. | ||
260 | 320 | |||
261 | 321 | >>> message.get_payload(decode=True).splitlines() | ||
262 | 322 | [..., | ||
263 | 323 | '', | ||
264 | 324 | '-- ', | ||
265 | 325 | 'You received this bug notification because you are a direct subscriber', | ||
266 | 326 | 'of the bug.', | ||
267 | 327 | 'http://bugs.launchpad.dev/bugs/1', | ||
268 | 328 | '', | ||
269 | 329 | 'Title:', | ||
270 | 330 | ' Firefox does not support SVG'...] | ||
271 | 331 | |||
272 | 332 | >>> flush_notifications() | ||
273 | 333 | |||
274 | 315 | We send the notification only if the user hasn't done any other changes | 334 | We send the notification only if the user hasn't done any other changes |
275 | 316 | for the last 5 minutes: | 335 | for the last 5 minutes: |
276 | 317 | 336 | ||
277 | @@ -328,22 +347,6 @@ | |||
278 | 328 | 347 | ||
279 | 329 | >>> flush_notifications() | 348 | >>> flush_notifications() |
280 | 330 | 349 | ||
281 | 331 | There's a blank line before the signature, and the signature marker has | ||
282 | 332 | a trailing space. | ||
283 | 333 | |||
284 | 334 | >>> message.get_payload(decode=True).splitlines() | ||
285 | 335 | [..., | ||
286 | 336 | '', | ||
287 | 337 | '-- ', | ||
288 | 338 | 'You received this bug notification because you are a direct subscriber', | ||
289 | 339 | 'of the bug.', | ||
290 | 340 | 'http://bugs.launchpad.dev/bugs/1', | ||
291 | 341 | '', | ||
292 | 342 | 'Title:', | ||
293 | 343 | ' Firefox does not support SVG'...] | ||
294 | 344 | |||
295 | 345 | >>> flush_notifications() | ||
296 | 346 | |||
297 | 347 | If a team without a contact address is subscribed to the bug, the | 350 | If a team without a contact address is subscribed to the bug, the |
298 | 348 | notification will be sent to all members individually. | 351 | notification will be sent to all members individually. |
299 | 349 | 352 | ||
300 | @@ -379,7 +382,6 @@ | |||
301 | 379 | 382 | ||
302 | 380 | >>> flush_notifications() | 383 | >>> flush_notifications() |
303 | 381 | 384 | ||
304 | 382 | |||
305 | 383 | Duplicates | 385 | Duplicates |
306 | 384 | ---------- | 386 | ---------- |
307 | 385 | 387 | ||
308 | 386 | 388 | ||
309 | === modified file 'lib/lp/bugs/model/bugactivity.py' | |||
310 | --- lib/lp/bugs/model/bugactivity.py 2011-02-11 18:15:28 +0000 | |||
311 | +++ lib/lp/bugs/model/bugactivity.py 2011-02-17 17:46:51 +0000 | |||
312 | @@ -17,6 +17,19 @@ | |||
313 | 17 | 17 | ||
314 | 18 | from canonical.database.datetimecol import UtcDateTimeCol | 18 | from canonical.database.datetimecol import UtcDateTimeCol |
315 | 19 | from canonical.database.sqlbase import SQLBase | 19 | from canonical.database.sqlbase import SQLBase |
316 | 20 | from lp.bugs.adapters.bugchange import ( | ||
317 | 21 | ATTACHMENT_ADDED, | ||
318 | 22 | ATTACHMENT_REMOVED, | ||
319 | 23 | BRANCH_LINKED, | ||
320 | 24 | BRANCH_UNLINKED, | ||
321 | 25 | BUG_WATCH_ADDED, | ||
322 | 26 | BUG_WATCH_REMOVED, | ||
323 | 27 | CHANGED_DUPLICATE_MARKER, | ||
324 | 28 | CVE_LINKED, | ||
325 | 29 | CVE_UNLINKED, | ||
326 | 30 | MARKED_AS_DUPLICATE, | ||
327 | 31 | REMOVED_DUPLICATE_MARKER, | ||
328 | 32 | ) | ||
329 | 20 | from lp.bugs.interfaces.bugactivity import ( | 33 | from lp.bugs.interfaces.bugactivity import ( |
330 | 21 | IBugActivity, | 34 | IBugActivity, |
331 | 22 | IBugActivitySet, | 35 | IBugActivitySet, |
332 | @@ -68,12 +81,31 @@ | |||
333 | 68 | `attribute` is determined based on the `whatchanged` string. | 81 | `attribute` is determined based on the `whatchanged` string. |
334 | 69 | 82 | ||
335 | 70 | :return: The attribute name of the item if `whatchanged` is of | 83 | :return: The attribute name of the item if `whatchanged` is of |
338 | 71 | the form <target_name>: <attribute>. Otherwise, return the | 84 | the form <target_name>: <attribute>. If we know how to determine |
339 | 72 | original `whatchanged` string. | 85 | the attribute by normalizing whatchanged, we return that. |
340 | 86 | Otherwise, return the original `whatchanged` string. | ||
341 | 73 | """ | 87 | """ |
342 | 74 | match = self.bugtask_change_re.match(self.whatchanged) | 88 | match = self.bugtask_change_re.match(self.whatchanged) |
343 | 75 | if match is None: | 89 | if match is None: |
345 | 76 | return self.whatchanged | 90 | result = self.whatchanged |
346 | 91 | # Now we normalize names, as necessary. This is fragile, but | ||
347 | 92 | # a reasonable incremental step. These are consumed in | ||
348 | 93 | # lp.bugs.scripts.bugnotification.get_activity_key. | ||
349 | 94 | if result in (CHANGED_DUPLICATE_MARKER, | ||
350 | 95 | MARKED_AS_DUPLICATE, | ||
351 | 96 | REMOVED_DUPLICATE_MARKER): | ||
352 | 97 | result = 'duplicateof' | ||
353 | 98 | elif result in (ATTACHMENT_ADDED, ATTACHMENT_REMOVED): | ||
354 | 99 | result = 'attachments' | ||
355 | 100 | elif result in (BRANCH_LINKED, BRANCH_UNLINKED): | ||
356 | 101 | result = 'linked_branches' | ||
357 | 102 | elif result in (BUG_WATCH_ADDED, BUG_WATCH_REMOVED): | ||
358 | 103 | result = 'watches' | ||
359 | 104 | elif result in (CVE_LINKED, CVE_UNLINKED): | ||
360 | 105 | result = 'cves' | ||
361 | 106 | elif result == 'summary': | ||
362 | 107 | result = 'title' | ||
363 | 108 | return result | ||
364 | 77 | else: | 109 | else: |
365 | 78 | return match.groupdict()['attribute'] | 110 | return match.groupdict()['attribute'] |
366 | 79 | 111 | ||
367 | 80 | 112 | ||
368 | === modified file 'lib/lp/bugs/scripts/bugnotification.py' | |||
369 | --- lib/lp/bugs/scripts/bugnotification.py 2011-02-16 14:11:46 +0000 | |||
370 | +++ lib/lp/bugs/scripts/bugnotification.py 2011-02-17 17:46:51 +0000 | |||
371 | @@ -31,6 +31,39 @@ | |||
372 | 31 | from lp.services.mail.mailwrapper import MailWrapper | 31 | from lp.services.mail.mailwrapper import MailWrapper |
373 | 32 | 32 | ||
374 | 33 | 33 | ||
375 | 34 | def get_activity_key(notification): | ||
376 | 35 | """Given a notification, return a key for the activity if it exists. | ||
377 | 36 | |||
378 | 37 | The key will be used to determine whether changes for the activity are | ||
379 | 38 | undone within the same batch of notifications (which are supposed to | ||
380 | 39 | be all for the same bug when they get to this function). Therefore, | ||
381 | 40 | the activity's attribute is a good start for the key. | ||
382 | 41 | |||
383 | 42 | If the activity was on a bugtask, we will also want to distinguish | ||
384 | 43 | by bugtask, because, for instance, changing a status from INPROGRESS | ||
385 | 44 | to FIXCOMMITED on one bug task is not undone if the status changes | ||
386 | 45 | from FIXCOMMITTED to INPROGRESS on another bugtask. | ||
387 | 46 | |||
388 | 47 | Similarly, if the activity is about adding or removing something | ||
389 | 48 | that we can have multiple of, like a branch or an attachment, the | ||
390 | 49 | key should include information on that value, because adding one | ||
391 | 50 | attachment is not undone by removing another one. | ||
392 | 51 | """ | ||
393 | 52 | activity = notification.activity | ||
394 | 53 | if activity is not None: | ||
395 | 54 | key = activity.attribute | ||
396 | 55 | if activity.target is not None: | ||
397 | 56 | key = ':'.join((activity.target, key)) | ||
398 | 57 | if key in ('attachments', 'watches', 'cves', 'linked_branches'): | ||
399 | 58 | # We are intentionally leaving bug task bugwatches out of this | ||
400 | 59 | # list, so we use the key rather than the activity.attribute. | ||
401 | 60 | if activity.oldvalue is not None: | ||
402 | 61 | key = ':'.join((key, activity.oldvalue)) | ||
403 | 62 | elif activity.newvalue is not None: | ||
404 | 63 | key = ':'.join((key, activity.newvalue)) | ||
405 | 64 | return key | ||
406 | 65 | |||
407 | 66 | |||
408 | 34 | def construct_email_notifications(bug_notifications): | 67 | def construct_email_notifications(bug_notifications): |
409 | 35 | """Construct an email from a list of related bug notifications. | 68 | """Construct an email from a list of related bug notifications. |
410 | 36 | 69 | ||
411 | @@ -39,31 +72,45 @@ | |||
412 | 39 | """ | 72 | """ |
413 | 40 | first_notification = bug_notifications[0] | 73 | first_notification = bug_notifications[0] |
414 | 41 | bug = first_notification.bug | 74 | bug = first_notification.bug |
416 | 42 | person_causing_change = first_notification.message.owner | 75 | actor = first_notification.message.owner |
417 | 43 | subject = first_notification.message.subject | 76 | subject = first_notification.message.subject |
418 | 44 | 77 | ||
419 | 45 | comment = None | 78 | comment = None |
420 | 46 | references = [] | 79 | references = [] |
421 | 47 | text_notifications = [] | 80 | text_notifications = [] |
432 | 48 | 81 | old_values = {} | |
433 | 49 | recipients = {} | 82 | new_values = {} |
424 | 50 | for notification in bug_notifications: | ||
425 | 51 | for recipient in notification.recipients: | ||
426 | 52 | email_people = emailPeople(recipient.person) | ||
427 | 53 | if (not person_causing_change.selfgenerated_bugnotifications and | ||
428 | 54 | person_causing_change in email_people): | ||
429 | 55 | email_people.remove(person_causing_change) | ||
430 | 56 | for email_person in email_people: | ||
431 | 57 | recipients[email_person] = recipient | ||
434 | 58 | 83 | ||
435 | 59 | for notification in bug_notifications: | 84 | for notification in bug_notifications: |
436 | 60 | assert notification.bug == bug, bug.id | 85 | assert notification.bug == bug, bug.id |
439 | 61 | assert notification.message.owner == person_causing_change, ( | 86 | assert notification.message.owner == actor, actor.id |
438 | 62 | person_causing_change.id) | ||
440 | 63 | if notification.is_comment: | 87 | if notification.is_comment: |
441 | 64 | assert comment is None, ( | 88 | assert comment is None, ( |
442 | 65 | "Only one of the notifications is allowed to be a comment.") | 89 | "Only one of the notifications is allowed to be a comment.") |
443 | 66 | comment = notification.message | 90 | comment = notification.message |
444 | 91 | else: | ||
445 | 92 | key = get_activity_key(notification) | ||
446 | 93 | if key is not None: | ||
447 | 94 | if key not in old_values: | ||
448 | 95 | old_values[key] = notification.activity.oldvalue | ||
449 | 96 | new_values[key] = notification.activity.newvalue | ||
450 | 97 | |||
451 | 98 | recipients = {} | ||
452 | 99 | filtered_notifications = [] | ||
453 | 100 | for notification in bug_notifications: | ||
454 | 101 | key = get_activity_key(notification) | ||
455 | 102 | if (notification.is_comment or | ||
456 | 103 | key is None or | ||
457 | 104 | old_values[key] != new_values[key]): | ||
458 | 105 | # We will report this notification. | ||
459 | 106 | filtered_notifications.append(notification) | ||
460 | 107 | for recipient in notification.recipients: | ||
461 | 108 | email_people = emailPeople(recipient.person) | ||
462 | 109 | if (not actor.selfgenerated_bugnotifications and | ||
463 | 110 | actor in email_people): | ||
464 | 111 | email_people.remove(actor) | ||
465 | 112 | for email_person in email_people: | ||
466 | 113 | recipients[email_person] = recipient | ||
467 | 67 | 114 | ||
468 | 68 | if bug.duplicateof is not None: | 115 | if bug.duplicateof is not None: |
469 | 69 | text_notifications.append( | 116 | text_notifications.append( |
470 | @@ -88,7 +135,7 @@ | |||
471 | 88 | msgid = first_notification.message.rfc822msgid | 135 | msgid = first_notification.message.rfc822msgid |
472 | 89 | email_date = first_notification.message.datecreated | 136 | email_date = first_notification.message.datecreated |
473 | 90 | 137 | ||
475 | 91 | for notification in bug_notifications: | 138 | for notification in filtered_notifications: |
476 | 92 | if notification.message == comment: | 139 | if notification.message == comment: |
477 | 93 | # Comments were just handled in the previous if block. | 140 | # Comments were just handled in the previous if block. |
478 | 94 | continue | 141 | continue |
479 | @@ -104,9 +151,8 @@ | |||
480 | 104 | messages = [] | 151 | messages = [] |
481 | 105 | mail_wrapper = MailWrapper(width=72) | 152 | mail_wrapper = MailWrapper(width=72) |
482 | 106 | content = '\n\n'.join(text_notifications) | 153 | content = '\n\n'.join(text_notifications) |
486 | 107 | from_address = get_bugmail_from_address(person_causing_change, bug) | 154 | from_address = get_bugmail_from_address(actor, bug) |
487 | 108 | bug_notification_builder = BugNotificationBuilder( | 155 | bug_notification_builder = BugNotificationBuilder(bug, actor) |
485 | 109 | bug, person_causing_change) | ||
488 | 110 | sorted_recipients = sorted( | 156 | sorted_recipients = sorted( |
489 | 111 | recipients.items(), key=lambda t: t[0].preferredemail.email) | 157 | recipients.items(), key=lambda t: t[0].preferredemail.email) |
490 | 112 | for email_person, recipient in sorted_recipients: | 158 | for email_person, recipient in sorted_recipients: |
491 | @@ -157,7 +203,7 @@ | |||
492 | 157 | rationale, references, msgid) | 203 | rationale, references, msgid) |
493 | 158 | messages.append(msg) | 204 | messages.append(msg) |
494 | 159 | 205 | ||
496 | 160 | return bug_notifications, messages | 206 | return filtered_notifications, messages |
497 | 161 | 207 | ||
498 | 162 | 208 | ||
499 | 163 | def notification_comment_batches(notifications): | 209 | def notification_comment_batches(notifications): |
500 | 164 | 210 | ||
501 | === modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py' | |||
502 | --- lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-02-17 09:17:00 +0000 | |||
503 | +++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-02-17 17:46:51 +0000 | |||
504 | @@ -4,31 +4,57 @@ | |||
505 | 4 | 4 | ||
506 | 5 | __metaclass__ = type | 5 | __metaclass__ = type |
507 | 6 | 6 | ||
509 | 7 | from datetime import datetime | 7 | from datetime import datetime, timedelta |
510 | 8 | import unittest | 8 | import unittest |
511 | 9 | 9 | ||
512 | 10 | import pytz | 10 | import pytz |
513 | 11 | from testtools.matchers import Not | ||
514 | 12 | from transaction import commit | ||
515 | 11 | from zope.component import getUtility | 13 | from zope.component import getUtility |
516 | 12 | from zope.interface import implements | 14 | from zope.interface import implements |
517 | 13 | 15 | ||
518 | 14 | from canonical.config import config | 16 | from canonical.config import config |
521 | 15 | from canonical.database.sqlbase import commit | 17 | from canonical.database.sqlbase import flush_database_updates |
522 | 16 | from lp.bugs.model.bugtask import BugTask | 18 | from canonical.launchpad.ftests import login |
523 | 17 | from canonical.launchpad.helpers import get_contact_email_addresses | 19 | from canonical.launchpad.helpers import get_contact_email_addresses |
524 | 18 | from canonical.launchpad.interfaces.message import IMessageSet | 20 | from canonical.launchpad.interfaces.message import IMessageSet |
525 | 19 | from canonical.testing.layers import LaunchpadZopelessLayer | 21 | from canonical.testing.layers import LaunchpadZopelessLayer |
526 | 22 | from lp.bugs.adapters.bugchange import ( | ||
527 | 23 | BranchLinkedToBug, | ||
528 | 24 | BranchUnlinkedFromBug, | ||
529 | 25 | BugAttachmentChange, | ||
530 | 26 | BugDuplicateChange, | ||
531 | 27 | BugTagsChange, | ||
532 | 28 | BugTaskStatusChange, | ||
533 | 29 | BugTitleChange, | ||
534 | 30 | BugVisibilityChange, | ||
535 | 31 | BugWatchAdded, | ||
536 | 32 | BugWatchRemoved, | ||
537 | 33 | CveLinkedToBug, | ||
538 | 34 | CveUnlinkedFromBug, | ||
539 | 35 | ) | ||
540 | 20 | from lp.bugs.interfaces.bug import ( | 36 | from lp.bugs.interfaces.bug import ( |
541 | 21 | IBug, | 37 | IBug, |
542 | 22 | IBugSet, | 38 | IBugSet, |
543 | 23 | ) | 39 | ) |
544 | 40 | from lp.bugs.interfaces.bugnotification import IBugNotificationSet | ||
545 | 41 | from lp.bugs.interfaces.bugtask import BugTaskStatus | ||
546 | 24 | from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients | 42 | from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients |
547 | 43 | from lp.bugs.model.bugtask import BugTask | ||
548 | 25 | from lp.bugs.scripts.bugnotification import ( | 44 | from lp.bugs.scripts.bugnotification import ( |
549 | 26 | get_email_notifications, | 45 | get_email_notifications, |
550 | 46 | get_activity_key, | ||
551 | 27 | notification_batches, | 47 | notification_batches, |
552 | 28 | notification_comment_batches, | 48 | notification_comment_batches, |
553 | 29 | ) | 49 | ) |
554 | 30 | from lp.registry.interfaces.person import IPersonSet | 50 | from lp.registry.interfaces.person import IPersonSet |
555 | 31 | from lp.registry.interfaces.product import IProductSet | 51 | from lp.registry.interfaces.product import IProductSet |
556 | 52 | from lp.services.propertycache import cachedproperty | ||
557 | 53 | from lp.testing import ( | ||
558 | 54 | TestCase, | ||
559 | 55 | TestCaseWithFactory) | ||
560 | 56 | from lp.testing.dbuser import lp_dbuser | ||
561 | 57 | from lp.testing.matchers import Contains | ||
562 | 32 | 58 | ||
563 | 33 | 59 | ||
564 | 34 | class MockBug: | 60 | class MockBug: |
565 | @@ -105,6 +131,67 @@ | |||
566 | 105 | self.is_comment = is_comment | 131 | self.is_comment = is_comment |
567 | 106 | self.date_emailed = date_emailed | 132 | self.date_emailed = date_emailed |
568 | 107 | self.recipients = [MockBugNotificationRecipient()] | 133 | self.recipients = [MockBugNotificationRecipient()] |
569 | 134 | self.activity = None | ||
570 | 135 | |||
571 | 136 | |||
572 | 137 | class FakeNotification: | ||
573 | 138 | """An even simpler fake notification. | ||
574 | 139 | |||
575 | 140 | Used by TestGetActivityKey, TestNotificationCommentBatches and | ||
576 | 141 | TestNotificationBatches.""" | ||
577 | 142 | |||
578 | 143 | class Message(object): | ||
579 | 144 | pass | ||
580 | 145 | |||
581 | 146 | def __init__(self, is_comment=False, bug=None, owner=None): | ||
582 | 147 | self.is_comment = is_comment | ||
583 | 148 | self.bug = bug | ||
584 | 149 | self.message = self.Message() | ||
585 | 150 | self.message.owner = owner | ||
586 | 151 | self.activity = None | ||
587 | 152 | |||
588 | 153 | |||
589 | 154 | class MockBugActivity: | ||
590 | 155 | """A mock BugActivity used for testing.""" | ||
591 | 156 | def __init__(self, target=None, attribute=None, | ||
592 | 157 | oldvalue=None, newvalue=None): | ||
593 | 158 | self.target = target | ||
594 | 159 | self.attribute = attribute | ||
595 | 160 | self.oldvalue = oldvalue | ||
596 | 161 | self.newvalue = newvalue | ||
597 | 162 | |||
598 | 163 | |||
599 | 164 | class TestGetActivityKey(TestCase): | ||
600 | 165 | """Tests for get_activity_key().""" | ||
601 | 166 | |||
602 | 167 | def test_no_activity(self): | ||
603 | 168 | self.assertEqual(get_activity_key(FakeNotification()), None) | ||
604 | 169 | |||
605 | 170 | def test_normal_bug_attribute_activity(self): | ||
606 | 171 | notification = FakeNotification() | ||
607 | 172 | notification.activity = MockBugActivity(attribute='title') | ||
608 | 173 | self.assertEqual(get_activity_key(notification), 'title') | ||
609 | 174 | |||
610 | 175 | def test_collection_bug_attribute_added_activity(self): | ||
611 | 176 | notification = FakeNotification() | ||
612 | 177 | notification.activity = MockBugActivity( | ||
613 | 178 | attribute='cves', newvalue='some cve identifier') | ||
614 | 179 | self.assertEqual(get_activity_key(notification), | ||
615 | 180 | 'cves:some cve identifier') | ||
616 | 181 | |||
617 | 182 | def test_collection_bug_attribute_removed_activity(self): | ||
618 | 183 | notification = FakeNotification() | ||
619 | 184 | notification.activity = MockBugActivity( | ||
620 | 185 | attribute='cves', oldvalue='some cve identifier') | ||
621 | 186 | self.assertEqual(get_activity_key(notification), | ||
622 | 187 | 'cves:some cve identifier') | ||
623 | 188 | |||
624 | 189 | def test_bugtask_attribute_activity(self): | ||
625 | 190 | notification = FakeNotification() | ||
626 | 191 | notification.activity = MockBugActivity( | ||
627 | 192 | attribute='status', target='some bug task identifier') | ||
628 | 193 | self.assertEqual(get_activity_key(notification), | ||
629 | 194 | 'some bug task identifier:status') | ||
630 | 108 | 195 | ||
631 | 109 | 196 | ||
632 | 110 | class TestGetEmailNotifications(unittest.TestCase): | 197 | class TestGetEmailNotifications(unittest.TestCase): |
633 | @@ -240,19 +327,6 @@ | |||
634 | 240 | self.assertEqual(bug_four.id, 4) | 327 | self.assertEqual(bug_four.id, 4) |
635 | 241 | 328 | ||
636 | 242 | 329 | ||
637 | 243 | class FakeNotification: | ||
638 | 244 | """Used by TestNotificationCommentBatches and TestNotificationBatches.""" | ||
639 | 245 | |||
640 | 246 | class Message(object): | ||
641 | 247 | pass | ||
642 | 248 | |||
643 | 249 | def __init__(self, is_comment=False, bug=None, owner=None): | ||
644 | 250 | self.is_comment = is_comment | ||
645 | 251 | self.bug = bug | ||
646 | 252 | self.message = self.Message() | ||
647 | 253 | self.message.owner = owner | ||
648 | 254 | |||
649 | 255 | |||
650 | 256 | class TestNotificationCommentBatches(unittest.TestCase): | 330 | class TestNotificationCommentBatches(unittest.TestCase): |
651 | 257 | """Tests of `notification_comment_batches`.""" | 331 | """Tests of `notification_comment_batches`.""" |
652 | 258 | 332 | ||
653 | @@ -456,3 +530,339 @@ | |||
654 | 456 | expected = [notifications[0:3], notifications[3:4]] | 530 | expected = [notifications[0:3], notifications[3:4]] |
655 | 457 | observed = list(notification_batches(notifications)) | 531 | observed = list(notification_batches(notifications)) |
656 | 458 | self.assertEquals(expected, observed) | 532 | self.assertEquals(expected, observed) |
657 | 533 | |||
658 | 534 | |||
659 | 535 | class EmailNotificationTestBase(TestCaseWithFactory): | ||
660 | 536 | |||
661 | 537 | layer = LaunchpadZopelessLayer | ||
662 | 538 | |||
663 | 539 | def setUp(self): | ||
664 | 540 | super(EmailNotificationTestBase, self).setUp() | ||
665 | 541 | login('foo.bar@canonical.com') | ||
666 | 542 | self.product_owner = self.factory.makePerson(name="product-owner") | ||
667 | 543 | self.person = self.factory.makePerson(name="sample-person") | ||
668 | 544 | self.product = self.factory.makeProduct(owner=self.product_owner) | ||
669 | 545 | self.product_subscriber = self.factory.makePerson( | ||
670 | 546 | name="product-subscriber") | ||
671 | 547 | self.product.addBugSubscription( | ||
672 | 548 | self.product_subscriber, self.product_subscriber) | ||
673 | 549 | self.bug_subscriber = self.factory.makePerson(name="bug-subscriber") | ||
674 | 550 | self.bug_owner = self.factory.makePerson(name="bug-owner") | ||
675 | 551 | self.bug = self.factory.makeBug( | ||
676 | 552 | product=self.product, private=False, owner=self.bug_owner) | ||
677 | 553 | self.reporter = self.bug.owner | ||
678 | 554 | self.bug.subscribe(self.bug_subscriber, self.reporter) | ||
679 | 555 | [self.product_bugtask] = self.bug.bugtasks | ||
680 | 556 | commit() | ||
681 | 557 | login('test@canonical.com') | ||
682 | 558 | self.layer.switchDbUser(config.malone.bugnotification_dbuser) | ||
683 | 559 | self.now = datetime.now(pytz.timezone('UTC')) | ||
684 | 560 | self.ten_minutes_ago = self.now - timedelta(minutes=10) | ||
685 | 561 | self.notification_set = getUtility(IBugNotificationSet) | ||
686 | 562 | for notification in self.notification_set.getNotificationsToSend(): | ||
687 | 563 | notification.date_emailed = self.now | ||
688 | 564 | flush_database_updates() | ||
689 | 565 | |||
690 | 566 | def tearDown(self): | ||
691 | 567 | for notification in self.notification_set.getNotificationsToSend(): | ||
692 | 568 | notification.date_emailed = self.now | ||
693 | 569 | flush_database_updates() | ||
694 | 570 | super(EmailNotificationTestBase, self).tearDown() | ||
695 | 571 | |||
696 | 572 | def get_messages(self): | ||
697 | 573 | notifications = self.notification_set.getNotificationsToSend() | ||
698 | 574 | email_notifications = get_email_notifications(notifications) | ||
699 | 575 | for bug_notifications, messages in email_notifications: | ||
700 | 576 | for message in messages: | ||
701 | 577 | yield message, message.get_payload(decode=True) | ||
702 | 578 | |||
703 | 579 | |||
704 | 580 | class EmailNotificationsBugMixin: | ||
705 | 581 | |||
706 | 582 | change_class = change_name = old = new = alt = unexpected_text = None | ||
707 | 583 | |||
708 | 584 | def change(self, old, new): | ||
709 | 585 | self.bug.addChange( | ||
710 | 586 | self.change_class( | ||
711 | 587 | self.ten_minutes_ago, self.person, self.change_name, | ||
712 | 588 | old, new)) | ||
713 | 589 | |||
714 | 590 | def change_other(self): | ||
715 | 591 | self.bug.addChange( | ||
716 | 592 | BugVisibilityChange( | ||
717 | 593 | self.ten_minutes_ago, self.person, "private", | ||
718 | 594 | False, True)) | ||
719 | 595 | |||
720 | 596 | def test_change_seen(self): | ||
721 | 597 | # A smoketest. | ||
722 | 598 | self.change(self.old, self.new) | ||
723 | 599 | message, body = self.get_messages().next() | ||
724 | 600 | self.assertThat(body, Contains(self.unexpected_text)) | ||
725 | 601 | |||
726 | 602 | def test_undone_change_sends_no_emails(self): | ||
727 | 603 | self.change(self.old, self.new) | ||
728 | 604 | self.change(self.new, self.old) | ||
729 | 605 | self.assertEqual(list(self.get_messages()), []) | ||
730 | 606 | |||
731 | 607 | def test_undone_change_is_not_included(self): | ||
732 | 608 | self.change(self.old, self.new) | ||
733 | 609 | self.change(self.new, self.old) | ||
734 | 610 | self.change_other() | ||
735 | 611 | message, body = self.get_messages().next() | ||
736 | 612 | self.assertThat(body, Not(Contains(self.unexpected_text))) | ||
737 | 613 | |||
738 | 614 | def test_multiple_undone_changes_sends_no_emails(self): | ||
739 | 615 | self.change(self.old, self.new) | ||
740 | 616 | self.change(self.new, self.alt) | ||
741 | 617 | self.change(self.alt, self.old) | ||
742 | 618 | self.assertEqual(list(self.get_messages()), []) | ||
743 | 619 | |||
744 | 620 | |||
745 | 621 | class EmailNotificationsBugNotRequiredMixin(EmailNotificationsBugMixin): | ||
746 | 622 | # This test collection is for attributes that can be None. | ||
747 | 623 | def test_added_removed_sends_no_emails(self): | ||
748 | 624 | self.change(None, self.old) | ||
749 | 625 | self.change(self.old, None) | ||
750 | 626 | self.assertEqual(list(self.get_messages()), []) | ||
751 | 627 | |||
752 | 628 | def test_removed_added_sends_no_emails(self): | ||
753 | 629 | self.change(self.old, None) | ||
754 | 630 | self.change(None, self.old) | ||
755 | 631 | self.assertEqual(list(self.get_messages()), []) | ||
756 | 632 | |||
757 | 633 | def test_duplicate_marked_changed_removed_sends_no_emails(self): | ||
758 | 634 | self.change(None, self.old) | ||
759 | 635 | self.change(self.old, self.new) | ||
760 | 636 | self.change(self.new, None) | ||
761 | 637 | self.assertEqual(list(self.get_messages()), []) | ||
762 | 638 | |||
763 | 639 | |||
764 | 640 | class EmailNotificationsBugTaskMixin(EmailNotificationsBugMixin): | ||
765 | 641 | |||
766 | 642 | def change(self, old, new, index=0): | ||
767 | 643 | self.bug.addChange( | ||
768 | 644 | self.change_class( | ||
769 | 645 | self.bug.bugtasks[index], self.ten_minutes_ago, | ||
770 | 646 | self.person, self.change_name, old, new)) | ||
771 | 647 | |||
772 | 648 | def test_changing_on_different_bugtasks_is_not_undoing(self): | ||
773 | 649 | with lp_dbuser(): | ||
774 | 650 | product2 = self.factory.makeProduct(owner=self.product_owner) | ||
775 | 651 | self.bug.addTask(self.product_owner, product2) | ||
776 | 652 | self.change(self.old, self.new, index=0) | ||
777 | 653 | self.change(self.new, self.old, index=1) | ||
778 | 654 | message, body = self.get_messages().next() | ||
779 | 655 | self.assertThat(body, Contains(self.unexpected_text)) | ||
780 | 656 | |||
781 | 657 | |||
782 | 658 | class EmailNotificationsAddedRemovedMixin: | ||
783 | 659 | |||
784 | 660 | old = new = added_message = removed_message = None | ||
785 | 661 | |||
786 | 662 | def add(self, item): | ||
787 | 663 | raise NotImplementedError | ||
788 | 664 | remove = add | ||
789 | 665 | |||
790 | 666 | def test_added_seen(self): | ||
791 | 667 | self.add(self.old) | ||
792 | 668 | message, body = self.get_messages().next() | ||
793 | 669 | self.assertThat(body, Contains(self.added_message)) | ||
794 | 670 | |||
795 | 671 | def test_added_removed_sends_no_emails(self): | ||
796 | 672 | self.add(self.old) | ||
797 | 673 | self.remove(self.old) | ||
798 | 674 | self.assertEqual(list(self.get_messages()), []) | ||
799 | 675 | |||
800 | 676 | def test_removed_added_sends_no_emails(self): | ||
801 | 677 | self.remove(self.old) | ||
802 | 678 | self.add(self.old) | ||
803 | 679 | self.assertEqual(list(self.get_messages()), []) | ||
804 | 680 | |||
805 | 681 | def test_added_another_removed_sends_emails(self): | ||
806 | 682 | self.add(self.old) | ||
807 | 683 | self.remove(self.new) | ||
808 | 684 | message, body = self.get_messages().next() | ||
809 | 685 | self.assertThat(body, Contains(self.added_message)) | ||
810 | 686 | self.assertThat(body, Contains(self.removed_message)) | ||
811 | 687 | |||
812 | 688 | |||
813 | 689 | class TestEmailNotificationsBugTitle( | ||
814 | 690 | EmailNotificationsBugMixin, EmailNotificationTestBase): | ||
815 | 691 | |||
816 | 692 | change_class = BugTitleChange | ||
817 | 693 | change_name = "title" | ||
818 | 694 | old = "Old summary" | ||
819 | 695 | new = "New summary" | ||
820 | 696 | alt = "Another summary" | ||
821 | 697 | unexpected_text = '** Summary changed:' | ||
822 | 698 | |||
823 | 699 | |||
824 | 700 | class TestEmailNotificationsBugTags( | ||
825 | 701 | EmailNotificationsBugMixin, EmailNotificationTestBase): | ||
826 | 702 | |||
827 | 703 | change_class = BugTagsChange | ||
828 | 704 | change_name = "tags" | ||
829 | 705 | old = ['foo', 'bar', 'baz'] | ||
830 | 706 | new = ['foo', 'bar'] | ||
831 | 707 | alt = ['bing', 'shazam'] | ||
832 | 708 | unexpected_text = '** Tags' | ||
833 | 709 | |||
834 | 710 | def test_undone_ordered_set_sends_no_email(self): | ||
835 | 711 | # Tags use ordered sets to generate change descriptions, which we | ||
836 | 712 | # demonstrate here. | ||
837 | 713 | self.change(['foo', 'bar', 'baz'], ['foo', 'bar']) | ||
838 | 714 | self.change(['foo', 'bar'], ['baz', 'bar', 'foo', 'bar']) | ||
839 | 715 | self.assertEqual(list(self.get_messages()), []) | ||
840 | 716 | |||
841 | 717 | |||
842 | 718 | class TestEmailNotificationsBugDuplicate( | ||
843 | 719 | EmailNotificationsBugNotRequiredMixin, EmailNotificationTestBase): | ||
844 | 720 | |||
845 | 721 | change_class = BugDuplicateChange | ||
846 | 722 | change_name = "duplicateof" | ||
847 | 723 | unexpected_text = 'duplicate' | ||
848 | 724 | |||
849 | 725 | def _bug(self): | ||
850 | 726 | with lp_dbuser(): | ||
851 | 727 | return self.factory.makeBug() | ||
852 | 728 | |||
853 | 729 | old = cachedproperty('old')(_bug) | ||
854 | 730 | new = cachedproperty('new')(_bug) | ||
855 | 731 | alt = cachedproperty('alt')(_bug) | ||
856 | 732 | |||
857 | 733 | |||
858 | 734 | class TestEmailNotificationsBugTaskStatus( | ||
859 | 735 | EmailNotificationsBugTaskMixin, EmailNotificationTestBase): | ||
860 | 736 | |||
861 | 737 | change_class = BugTaskStatusChange | ||
862 | 738 | change_name = "status" | ||
863 | 739 | old = BugTaskStatus.TRIAGED | ||
864 | 740 | new = BugTaskStatus.INPROGRESS | ||
865 | 741 | alt = BugTaskStatus.INVALID | ||
866 | 742 | unexpected_text = 'Status: ' | ||
867 | 743 | |||
868 | 744 | |||
869 | 745 | class TestEmailNotificationsBugWatch( | ||
870 | 746 | EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase): | ||
871 | 747 | |||
872 | 748 | # Note that this is for bugwatches added to bugs. Bugwatches added | ||
873 | 749 | # to bugtasks are separate animals AIUI, and we don't try to combine | ||
874 | 750 | # them here for notifications. Bugtasks have only zero or one | ||
875 | 751 | # bugwatch, so they can be handled just as a simple bugtask attribute | ||
876 | 752 | # change, like status. | ||
877 | 753 | |||
878 | 754 | added_message = '** Bug watch added:' | ||
879 | 755 | removed_message = '** Bug watch removed:' | ||
880 | 756 | |||
881 | 757 | @cachedproperty | ||
882 | 758 | def tracker(self): | ||
883 | 759 | with lp_dbuser(): | ||
884 | 760 | return self.factory.makeBugTracker() | ||
885 | 761 | |||
886 | 762 | def _watch(self, identifier='123'): | ||
887 | 763 | with lp_dbuser(): | ||
888 | 764 | # This actually creates a notification all by itself. However, | ||
889 | 765 | # it won't be sent out for another five minutes. Therefore, | ||
890 | 766 | # we send out separate change notifications. | ||
891 | 767 | return self.bug.addWatch( | ||
892 | 768 | self.tracker, identifier, self.product_owner) | ||
893 | 769 | |||
894 | 770 | old = cachedproperty('old')(_watch) | ||
895 | 771 | new = cachedproperty('new')(lambda self: self._watch('456')) | ||
896 | 772 | |||
897 | 773 | def add(self, item): | ||
898 | 774 | with lp_dbuser(): | ||
899 | 775 | self.bug.addChange( | ||
900 | 776 | BugWatchAdded( | ||
901 | 777 | self.ten_minutes_ago, self.product_owner, item)) | ||
902 | 778 | |||
903 | 779 | def remove(self, item): | ||
904 | 780 | with lp_dbuser(): | ||
905 | 781 | self.bug.addChange( | ||
906 | 782 | BugWatchRemoved( | ||
907 | 783 | self.ten_minutes_ago, self.product_owner, item)) | ||
908 | 784 | |||
909 | 785 | |||
910 | 786 | class TestEmailNotificationsBranch( | ||
911 | 787 | EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase): | ||
912 | 788 | |||
913 | 789 | added_message = '** Branch linked:' | ||
914 | 790 | removed_message = '** Branch unlinked:' | ||
915 | 791 | |||
916 | 792 | def _branch(self): | ||
917 | 793 | with lp_dbuser(): | ||
918 | 794 | return self.factory.makeBranch() | ||
919 | 795 | |||
920 | 796 | old = cachedproperty('old')(_branch) | ||
921 | 797 | new = cachedproperty('new')(_branch) | ||
922 | 798 | |||
923 | 799 | def add(self, item): | ||
924 | 800 | with lp_dbuser(): | ||
925 | 801 | self.bug.addChange( | ||
926 | 802 | BranchLinkedToBug( | ||
927 | 803 | self.ten_minutes_ago, self.person, item, self.bug)) | ||
928 | 804 | |||
929 | 805 | def remove(self, item): | ||
930 | 806 | with lp_dbuser(): | ||
931 | 807 | self.bug.addChange( | ||
932 | 808 | BranchUnlinkedFromBug( | ||
933 | 809 | self.ten_minutes_ago, self.person, item, self.bug)) | ||
934 | 810 | |||
935 | 811 | |||
936 | 812 | class TestEmailNotificationsCVE( | ||
937 | 813 | EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase): | ||
938 | 814 | |||
939 | 815 | added_message = '** CVE added:' | ||
940 | 816 | removed_message = '** CVE removed:' | ||
941 | 817 | |||
942 | 818 | def _cve(self, sequence): | ||
943 | 819 | with lp_dbuser(): | ||
944 | 820 | return self.factory.makeCVE(sequence) | ||
945 | 821 | |||
946 | 822 | old = cachedproperty('old')(lambda self: self._cve('2020-1234')) | ||
947 | 823 | new = cachedproperty('new')(lambda self: self._cve('2020-5678')) | ||
948 | 824 | |||
949 | 825 | def add(self, item): | ||
950 | 826 | with lp_dbuser(): | ||
951 | 827 | self.bug.addChange( | ||
952 | 828 | CveLinkedToBug( | ||
953 | 829 | self.ten_minutes_ago, self.person, item)) | ||
954 | 830 | |||
955 | 831 | def remove(self, item): | ||
956 | 832 | with lp_dbuser(): | ||
957 | 833 | self.bug.addChange( | ||
958 | 834 | CveUnlinkedFromBug( | ||
959 | 835 | self.ten_minutes_ago, self.person, item)) | ||
960 | 836 | |||
961 | 837 | |||
962 | 838 | class TestEmailNotificationsAttachments( | ||
963 | 839 | EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase): | ||
964 | 840 | |||
965 | 841 | added_message = '** Attachment added:' | ||
966 | 842 | removed_message = '** Attachment removed:' | ||
967 | 843 | |||
968 | 844 | def _attachment(self): | ||
969 | 845 | with lp_dbuser(): | ||
970 | 846 | # This actually creates a notification all by itself, via an | ||
971 | 847 | # event subscriber. However, it won't be sent out for | ||
972 | 848 | # another five minutes. Therefore, we send out separate | ||
973 | 849 | # change notifications. | ||
974 | 850 | return self.bug.addAttachment( | ||
975 | 851 | self.person, 'content', 'a comment', 'stuff.txt') | ||
976 | 852 | |||
977 | 853 | old = cachedproperty('old')(_attachment) | ||
978 | 854 | new = cachedproperty('new')(_attachment) | ||
979 | 855 | |||
980 | 856 | def add(self, item): | ||
981 | 857 | with lp_dbuser(): | ||
982 | 858 | self.bug.addChange( | ||
983 | 859 | BugAttachmentChange( | ||
984 | 860 | self.ten_minutes_ago, self.person, 'attachment', | ||
985 | 861 | None, item)) | ||
986 | 862 | |||
987 | 863 | def remove(self, item): | ||
988 | 864 | with lp_dbuser(): | ||
989 | 865 | self.bug.addChange( | ||
990 | 866 | BugAttachmentChange( | ||
991 | 867 | self.ten_minutes_ago, self.person, 'attachment', | ||
992 | 868 | item, None)) | ||
993 | 459 | 869 | ||
994 | === modified file 'lib/lp/bugs/tests/test_bugchanges.py' | |||
995 | --- lib/lp/bugs/tests/test_bugchanges.py 2011-02-16 12:05:44 +0000 | |||
996 | +++ lib/lp/bugs/tests/test_bugchanges.py 2011-02-17 17:46:51 +0000 | |||
997 | @@ -8,12 +8,12 @@ | |||
998 | 8 | ObjectModifiedEvent, | 8 | ObjectModifiedEvent, |
999 | 9 | ) | 9 | ) |
1000 | 10 | from lazr.lifecycle.snapshot import Snapshot | 10 | from lazr.lifecycle.snapshot import Snapshot |
1001 | 11 | from testtools.matchers import StartsWith | ||
1002 | 11 | from zope.component import getUtility | 12 | from zope.component import getUtility |
1003 | 12 | from zope.event import notify | 13 | from zope.event import notify |
1004 | 13 | from zope.interface import providedBy | 14 | from zope.interface import providedBy |
1005 | 14 | 15 | ||
1006 | 15 | from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias | 16 | from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias |
1007 | 16 | from lp.bugs.model.bugnotification import BugNotification | ||
1008 | 17 | from canonical.launchpad.webapp.interfaces import ILaunchBag | 17 | from canonical.launchpad.webapp.interfaces import ILaunchBag |
1009 | 18 | from canonical.launchpad.webapp.publisher import canonical_url | 18 | from canonical.launchpad.webapp.publisher import canonical_url |
1010 | 19 | from canonical.testing.layers import LaunchpadFunctionalLayer | 19 | from canonical.testing.layers import LaunchpadFunctionalLayer |
1011 | @@ -24,6 +24,7 @@ | |||
1012 | 24 | BugTaskStatus, | 24 | BugTaskStatus, |
1013 | 25 | ) | 25 | ) |
1014 | 26 | from lp.bugs.interfaces.cve import ICveSet | 26 | from lp.bugs.interfaces.cve import ICveSet |
1015 | 27 | from lp.bugs.model.bugnotification import BugNotification | ||
1016 | 27 | from lp.bugs.scripts.bugnotification import construct_email_notifications | 28 | from lp.bugs.scripts.bugnotification import construct_email_notifications |
1017 | 28 | from lp.testing import ( | 29 | from lp.testing import ( |
1018 | 29 | person_logged_in, | 30 | person_logged_in, |
1019 | @@ -220,6 +221,11 @@ | |||
1020 | 220 | # the Bug's notifications. | 221 | # the Bug's notifications. |
1021 | 221 | old_title = self.changeAttribute(self.bug, 'title', '42') | 222 | old_title = self.changeAttribute(self.bug, 'title', '42') |
1022 | 222 | 223 | ||
1023 | 224 | # This checks the activity's attribute and target attributes. | ||
1024 | 225 | activity = self.bug.activity[-1] | ||
1025 | 226 | self.assertEqual(activity.attribute, 'title') | ||
1026 | 227 | self.assertEqual(activity.target, None) | ||
1027 | 228 | |||
1028 | 223 | title_change_activity = { | 229 | title_change_activity = { |
1029 | 224 | 'whatchanged': 'summary', | 230 | 'whatchanged': 'summary', |
1030 | 225 | 'oldvalue': old_title, | 231 | 'oldvalue': old_title, |
1031 | @@ -270,6 +276,11 @@ | |||
1032 | 270 | bugtracker = self.factory.makeBugTracker() | 276 | bugtracker = self.factory.makeBugTracker() |
1033 | 271 | bug_watch = self.bug.addWatch(bugtracker, '42', self.user) | 277 | bug_watch = self.bug.addWatch(bugtracker, '42', self.user) |
1034 | 272 | 278 | ||
1035 | 279 | # This checks the activity's attribute and target attributes. | ||
1036 | 280 | activity = self.bug.activity[-1] | ||
1037 | 281 | self.assertEqual(activity.attribute, 'watches') | ||
1038 | 282 | self.assertEqual(activity.target, None) | ||
1039 | 283 | |||
1040 | 273 | bugwatch_activity = { | 284 | bugwatch_activity = { |
1041 | 274 | 'person': self.user, | 285 | 'person': self.user, |
1042 | 275 | 'whatchanged': 'bug watch added', | 286 | 'whatchanged': 'bug watch added', |
1043 | @@ -337,6 +348,11 @@ | |||
1044 | 337 | self.saveOldChanges() | 348 | self.saveOldChanges() |
1045 | 338 | self.bug.removeWatch(bug_watch, self.user) | 349 | self.bug.removeWatch(bug_watch, self.user) |
1046 | 339 | 350 | ||
1047 | 351 | # This checks the activity's attribute and target attributes. | ||
1048 | 352 | activity = self.bug.activity[-1] | ||
1049 | 353 | self.assertEqual(activity.attribute, 'watches') | ||
1050 | 354 | self.assertEqual(activity.target, None) | ||
1051 | 355 | |||
1052 | 340 | bugwatch_activity = { | 356 | bugwatch_activity = { |
1053 | 341 | 'person': self.user, | 357 | 'person': self.user, |
1054 | 342 | 'whatchanged': 'bug watch removed', | 358 | 'whatchanged': 'bug watch removed', |
1055 | @@ -413,6 +429,12 @@ | |||
1056 | 413 | # sends an e-mail notification. | 429 | # sends an e-mail notification. |
1057 | 414 | branch = self.factory.makeBranch() | 430 | branch = self.factory.makeBranch() |
1058 | 415 | self.bug.linkBranch(branch, self.user) | 431 | self.bug.linkBranch(branch, self.user) |
1059 | 432 | |||
1060 | 433 | # This checks the activity's attribute and target attributes. | ||
1061 | 434 | activity = self.bug.activity[-1] | ||
1062 | 435 | self.assertEqual(activity.attribute, 'linked_branches') | ||
1063 | 436 | self.assertEqual(activity.target, None) | ||
1064 | 437 | |||
1065 | 416 | added_activity = { | 438 | added_activity = { |
1066 | 417 | 'person': self.user, | 439 | 'person': self.user, |
1067 | 418 | 'whatchanged': 'branch linked', | 440 | 'whatchanged': 'branch linked', |
1068 | @@ -459,6 +481,12 @@ | |||
1069 | 459 | self.bug.linkBranch(branch, self.user) | 481 | self.bug.linkBranch(branch, self.user) |
1070 | 460 | self.saveOldChanges() | 482 | self.saveOldChanges() |
1071 | 461 | self.bug.unlinkBranch(branch, self.user) | 483 | self.bug.unlinkBranch(branch, self.user) |
1072 | 484 | |||
1073 | 485 | # This checks the activity's attribute and target attributes. | ||
1074 | 486 | activity = self.bug.activity[-1] | ||
1075 | 487 | self.assertEqual(activity.attribute, 'linked_branches') | ||
1076 | 488 | self.assertEqual(activity.target, None) | ||
1077 | 489 | |||
1078 | 462 | added_activity = { | 490 | added_activity = { |
1079 | 463 | 'person': self.user, | 491 | 'person': self.user, |
1080 | 464 | 'whatchanged': 'branch unlinked', | 492 | 'whatchanged': 'branch unlinked', |
1081 | @@ -654,6 +682,11 @@ | |||
1082 | 654 | cve = getUtility(ICveSet)['1999-8979'] | 682 | cve = getUtility(ICveSet)['1999-8979'] |
1083 | 655 | self.bug.linkCVE(cve, self.user) | 683 | self.bug.linkCVE(cve, self.user) |
1084 | 656 | 684 | ||
1085 | 685 | # This checks the activity's attribute and target attributes. | ||
1086 | 686 | activity = self.bug.activity[-1] | ||
1087 | 687 | self.assertEqual(activity.attribute, 'cves') | ||
1088 | 688 | self.assertEqual(activity.target, None) | ||
1089 | 689 | |||
1090 | 657 | cve_linked_activity = { | 690 | cve_linked_activity = { |
1091 | 658 | 'person': self.user, | 691 | 'person': self.user, |
1092 | 659 | 'whatchanged': 'cve linked', | 692 | 'whatchanged': 'cve linked', |
1093 | @@ -680,6 +713,11 @@ | |||
1094 | 680 | self.saveOldChanges() | 713 | self.saveOldChanges() |
1095 | 681 | self.bug.unlinkCVE(cve, self.user) | 714 | self.bug.unlinkCVE(cve, self.user) |
1096 | 682 | 715 | ||
1097 | 716 | # This checks the activity's attribute and target attributes. | ||
1098 | 717 | activity = self.bug.activity[-1] | ||
1099 | 718 | self.assertEqual(activity.attribute, 'cves') | ||
1100 | 719 | self.assertEqual(activity.target, None) | ||
1101 | 720 | |||
1102 | 683 | cve_unlinked_activity = { | 721 | cve_unlinked_activity = { |
1103 | 684 | 'person': self.user, | 722 | 'person': self.user, |
1104 | 685 | 'whatchanged': 'cve unlinked', | 723 | 'whatchanged': 'cve unlinked', |
1105 | @@ -708,6 +746,11 @@ | |||
1106 | 708 | attachment = self.factory.makeBugAttachment( | 746 | attachment = self.factory.makeBugAttachment( |
1107 | 709 | bug=self.bug, owner=self.user, comment=message) | 747 | bug=self.bug, owner=self.user, comment=message) |
1108 | 710 | 748 | ||
1109 | 749 | # This checks the activity's attribute and target attributes. | ||
1110 | 750 | activity = self.bug.activity[-1] | ||
1111 | 751 | self.assertEqual(activity.attribute, 'attachments') | ||
1112 | 752 | self.assertEqual(activity.target, None) | ||
1113 | 753 | |||
1114 | 711 | attachment_added_activity = { | 754 | attachment_added_activity = { |
1115 | 712 | 'person': self.user, | 755 | 'person': self.user, |
1116 | 713 | 'whatchanged': 'attachment added', | 756 | 'whatchanged': 'attachment added', |
1117 | @@ -741,6 +784,11 @@ | |||
1118 | 741 | 784 | ||
1119 | 742 | attachment.removeFromBug(user=self.user) | 785 | attachment.removeFromBug(user=self.user) |
1120 | 743 | 786 | ||
1121 | 787 | # This checks the activity's attribute and target attributes. | ||
1122 | 788 | activity = self.bug.activity[-1] | ||
1123 | 789 | self.assertEqual(activity.attribute, 'attachments') | ||
1124 | 790 | self.assertEqual(activity.target, None) | ||
1125 | 791 | |||
1126 | 744 | attachment_removed_activity = { | 792 | attachment_removed_activity = { |
1127 | 745 | 'person': self.user, | 793 | 'person': self.user, |
1128 | 746 | 'whatchanged': 'attachment removed', | 794 | 'whatchanged': 'attachment removed', |
1129 | @@ -858,6 +906,11 @@ | |||
1130 | 858 | self.bug_task, bug_task_before_modification, | 906 | self.bug_task, bug_task_before_modification, |
1131 | 859 | ['importance'], user=self.user)) | 907 | ['importance'], user=self.user)) |
1132 | 860 | 908 | ||
1133 | 909 | # This checks the activity's attribute and target attributes. | ||
1134 | 910 | activity = self.bug.activity[-1] | ||
1135 | 911 | self.assertEqual(activity.attribute, 'importance') | ||
1136 | 912 | self.assertThat(activity.target, StartsWith(u'product-name')) | ||
1137 | 913 | |||
1138 | 861 | expected_activity = { | 914 | expected_activity = { |
1139 | 862 | 'person': self.user, | 915 | 'person': self.user, |
1140 | 863 | 'whatchanged': '%s: importance' % self.bug_task.bugtargetname, | 916 | 'whatchanged': '%s: importance' % self.bug_task.bugtargetname, |
1141 | @@ -1304,6 +1357,11 @@ | |||
1142 | 1304 | level=BugNotificationLevel.METADATA).getRecipients() | 1357 | level=BugNotificationLevel.METADATA).getRecipients() |
1143 | 1305 | self.changeAttribute(duplicate_bug, 'duplicateof', self.bug) | 1358 | self.changeAttribute(duplicate_bug, 'duplicateof', self.bug) |
1144 | 1306 | 1359 | ||
1145 | 1360 | # This checks the activity's attribute and target attributes. | ||
1146 | 1361 | activity = duplicate_bug.activity[-1] | ||
1147 | 1362 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1148 | 1363 | self.assertEqual(activity.target, None) | ||
1149 | 1364 | |||
1150 | 1307 | expected_activity = { | 1365 | expected_activity = { |
1151 | 1308 | 'person': self.user, | 1366 | 'person': self.user, |
1152 | 1309 | 'whatchanged': 'marked as duplicate', | 1367 | 'whatchanged': 'marked as duplicate', |
1153 | @@ -1351,6 +1409,11 @@ | |||
1154 | 1351 | self.saveOldChanges(duplicate_bug) | 1409 | self.saveOldChanges(duplicate_bug) |
1155 | 1352 | self.changeAttribute(duplicate_bug, 'duplicateof', None) | 1410 | self.changeAttribute(duplicate_bug, 'duplicateof', None) |
1156 | 1353 | 1411 | ||
1157 | 1412 | # This checks the activity's attribute and target attributes. | ||
1158 | 1413 | activity = duplicate_bug.activity[-1] | ||
1159 | 1414 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1160 | 1415 | self.assertEqual(activity.target, None) | ||
1161 | 1416 | |||
1162 | 1354 | expected_activity = { | 1417 | expected_activity = { |
1163 | 1355 | 'person': self.user, | 1418 | 'person': self.user, |
1164 | 1356 | 'whatchanged': 'removed duplicate marker', | 1419 | 'whatchanged': 'removed duplicate marker', |
1165 | @@ -1382,6 +1445,11 @@ | |||
1166 | 1382 | self.saveOldChanges() | 1445 | self.saveOldChanges() |
1167 | 1383 | self.changeAttribute(self.bug, 'duplicateof', bug_two) | 1446 | self.changeAttribute(self.bug, 'duplicateof', bug_two) |
1168 | 1384 | 1447 | ||
1169 | 1448 | # This checks the activity's attribute and target attributes. | ||
1170 | 1449 | activity = self.bug.activity[-1] | ||
1171 | 1450 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1172 | 1451 | self.assertEqual(activity.target, None) | ||
1173 | 1452 | |||
1174 | 1385 | expected_activity = { | 1453 | expected_activity = { |
1175 | 1386 | 'person': self.user, | 1454 | 'person': self.user, |
1176 | 1387 | 'whatchanged': 'changed duplicate marker', | 1455 | 'whatchanged': 'changed duplicate marker', |
1177 | @@ -1429,6 +1497,11 @@ | |||
1178 | 1429 | level=BugNotificationLevel.METADATA).getRecipients() | 1497 | level=BugNotificationLevel.METADATA).getRecipients() |
1179 | 1430 | self.changeAttribute(public_bug, 'duplicateof', private_bug) | 1498 | self.changeAttribute(public_bug, 'duplicateof', private_bug) |
1180 | 1431 | 1499 | ||
1181 | 1500 | # This checks the activity's attribute and target attributes. | ||
1182 | 1501 | activity = public_bug.activity[-1] | ||
1183 | 1502 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1184 | 1503 | self.assertEqual(activity.target, None) | ||
1185 | 1504 | |||
1186 | 1432 | expected_activity = { | 1505 | expected_activity = { |
1187 | 1433 | 'person': self.user, | 1506 | 'person': self.user, |
1188 | 1434 | 'whatchanged': 'marked as duplicate', | 1507 | 'whatchanged': 'marked as duplicate', |
1189 | @@ -1468,6 +1541,11 @@ | |||
1190 | 1468 | 1541 | ||
1191 | 1469 | self.changeAttribute(public_bug, 'duplicateof', None) | 1542 | self.changeAttribute(public_bug, 'duplicateof', None) |
1192 | 1470 | 1543 | ||
1193 | 1544 | # This checks the activity's attribute and target attributes. | ||
1194 | 1545 | activity = public_bug.activity[-1] | ||
1195 | 1546 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1196 | 1547 | self.assertEqual(activity.target, None) | ||
1197 | 1548 | |||
1198 | 1471 | expected_activity = { | 1549 | expected_activity = { |
1199 | 1472 | 'person': self.user, | 1550 | 'person': self.user, |
1200 | 1473 | 'whatchanged': 'removed duplicate marker', | 1551 | 'whatchanged': 'removed duplicate marker', |
1201 | @@ -1504,6 +1582,11 @@ | |||
1202 | 1504 | 1582 | ||
1203 | 1505 | self.changeAttribute(duplicate_bug, 'duplicateof', public_bug) | 1583 | self.changeAttribute(duplicate_bug, 'duplicateof', public_bug) |
1204 | 1506 | 1584 | ||
1205 | 1585 | # This checks the activity's attribute and target attributes. | ||
1206 | 1586 | activity = duplicate_bug.activity[-1] | ||
1207 | 1587 | self.assertEqual(activity.attribute, 'duplicateof') | ||
1208 | 1588 | self.assertEqual(activity.target, None) | ||
1209 | 1589 | |||
1210 | 1507 | expected_activity = { | 1590 | expected_activity = { |
1211 | 1508 | 'person': self.user, | 1591 | 'person': self.user, |
1212 | 1509 | 'whatchanged': 'changed duplicate marker', | 1592 | 'whatchanged': 'changed duplicate marker', |
Conclusions from IRC.
To increase trust in what's going on in parsing whatchanged, we should:
1) add docstring and unittest for get_key
2) move duplicateof code to target computed attribute code, and add test
3) change get_key to use target/attribute
As for the rest, I really like the test coverage: great job there!
And a side note for s/person_ causing_ change/ actor/: wordy name was introduced instead of 'person' to help make code easier to understand/read. It's up to you to decide which you want to keep as long as you take code readability into account :)
Considering we have agreed on a few changes, I am marking this as 'approved', though I'd be happy to take a look at incremental diff if it's a significant change.