Launchpad itself

Merge lp:~thumper/launchpad/bmp-notification-email-job into lp:launchpad

  1. bmp-notification-email-job
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merge reported by: Tim Penhey
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/bmp-notification-email-job
Merge into: lp:launchpad
Prerequisite: lp:~thumper/launchpad/new-reviewer-email-job
Diff against target: 423 lines (+187/-76)
5 files modified
lib/lp/code/configure.zcml (+10/-0)
lib/lp/code/interfaces/branchmergeproposal.py (+21/-0)
lib/lp/code/mail/branchmergeproposal.py (+22/-13)
lib/lp/code/mail/tests/test_branchmergeproposal.py (+60/-62)
lib/lp/code/model/branchmergeproposaljob.py (+74/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/bmp-notification-email-job
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+21703@code.launchpad.net

Description of the change

Pipe 3 of (approximately 7).

This branch changes the email being sent from update events to be sent using a job.

Not a lot to see here, just more of the same.

tests:
  mail/tests/test_branchmergeproposal

This branch is not going to be landed, but the branch at the end of the
pipeline that will be.

BTW the tests pass for this branch on ec2.

lib/lp/code/configure.zcml
 - just registering the job

lib/lp/code/interfaces/branchmergeproposal.py
 - the interface for the job and the job creation utility

lib/lp/code/mail/branchmergeproposal.py
 - Some of the delta checking logic has moved into the event subscriber
   function as we don't want to create a job for something that isn't
   even going to send out some mail.
 - If there are changes to see, then a job is created.
 - The job contains the text_delta that would be included in the email.

lib/lp/code/mail/tests/test_branchmergeproposal.py
 - tests for the job creation and the resulting email.

lib/lp/code/model/branchmergeproposaljob.py
 - the implementation of the job itself.
 - again this just defers most of the processing to the BMPMailer

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I'm not sure that all the docstrings in lib/lp/code/mail/tests/test_branchmergeproposal.py make sense any more -- can you have a whip through and make sure they're up to date?

Otherwise, it's all good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/configure.zcml'
2--- lib/lp/code/configure.zcml 2010-03-29 01:14:29 +0000
3+++ lib/lp/code/configure.zcml 2010-03-29 01:14:30 +0000
4@@ -943,6 +943,16 @@
5 </class>
6
7 <securedutility
8+ component="lp.code.model.branchmergeproposaljob.MergeProposalUpdatedEmailJob"
9+ provides="lp.code.interfaces.branchmergeproposal.IMergeProposalUpdatedEmailJobSource">
10+ <allow interface="lp.code.interfaces.branchmergeproposal.IMergeProposalUpdatedEmailJobSource"/>
11+ </securedutility>
12+ <class class="lp.code.model.branchmergeproposaljob.MergeProposalUpdatedEmailJob">
13+ <allow interface="lp.services.job.interfaces.job.IRunnableJob" />
14+ <allow interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalJob" />
15+ </class>
16+
17+ <securedutility
18 component="lp.code.model.branchjob.BranchUpgradeJob"
19 provides="lp.code.interfaces.branchjob.IBranchUpgradeJobSource">
20 <allow interface="lp.code.interfaces.branchjob.IBranchUpgradeJobSource"/>
21
22=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
23--- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 01:14:29 +0000
24+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 01:14:30 +0000
25@@ -18,6 +18,8 @@
26 'ICreateMergeProposalJobSource',
27 'IMergeProposalCreatedJob',
28 'IMergeProposalCreatedJobSource',
29+ 'IMergeProposalUpdatedEmailJob',
30+ 'IMergeProposalUpdatedEmailJobSource',
31 'IReviewRequestedEmailJob',
32 'IReviewRequestedEmailJobSource',
33 'IUpdatePreviewDiffJobSource',
34@@ -632,6 +634,25 @@
35 """
36
37
38+class IMergeProposalUpdatedEmailJob(IRunnableJob):
39+ """Interface for the job to sends email about merge proposal updates."""
40+
41+ editor = Attribute('The person that did the editing.')
42+ delta_text = Attribute('The textual representation of the changed fields.')
43+
44+
45+class IMergeProposalUpdatedEmailJobSource(IJobSource):
46+ """Create or retrieve jobs that email about merge proposal updates."""
47+
48+ def create(merge_proposal, delta_text, editor):
49+ """Create a job to email merge proposal updates to subscribers.
50+
51+ :param merge_proposal: The merge proposal that has been edited.
52+ :param delta_text: The text representation of the changed fields.
53+ :param editor: The person who did the editing.
54+ """
55+
56+
57 # XXX: JonathanLange 2010-01-06: This is only used in the scanner, perhaps it
58 # should be moved there.
59 def notify_modified(proposal, func, *args, **kwargs):
60
61=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
62--- lib/lp/code/mail/branchmergeproposal.py 2010-03-29 01:14:29 +0000
63+++ lib/lp/code/mail/branchmergeproposal.py 2010-03-29 01:14:30 +0000
64@@ -15,10 +15,12 @@
65 from lp.code.adapters.branch import BranchMergeProposalDelta
66 from lp.code.enums import CodeReviewNotificationLevel
67 from lp.code.interfaces.branchmergeproposal import (
68- IMergeProposalCreatedJobSource, IReviewRequestedEmailJobSource)
69+ IMergeProposalCreatedJobSource, IMergeProposalUpdatedEmailJobSource,
70+ IReviewRequestedEmailJobSource)
71 from lp.code.mail.branch import BranchMailer
72 from lp.registry.interfaces.person import IPerson
73 from lp.services.mail.basemailer import BaseMailer
74+from lp.services.utils import text_delta
75
76
77 def send_merge_proposal_created_notifications(merge_proposal, event):
78@@ -32,16 +34,24 @@
79
80 def send_merge_proposal_modified_notifications(merge_proposal, event):
81 """Notify branch subscribers when merge proposals are updated."""
82+ # Check the user.
83 if event.user is None:
84 return
85 if isinstance(event.user, UnauthenticatedPrincipal):
86 from_person = None
87 else:
88 from_person = IPerson(event.user)
89- mailer = BMPMailer.forModification(
90- event.object_before_modification, merge_proposal, from_person)
91- if mailer is not None:
92- mailer.sendAll()
93+ # Create a delta of the changes. If there are no changes to report, then
94+ # we're done.
95+ delta = BranchMergeProposalDelta.construct(
96+ event.object_before_modification, merge_proposal)
97+ if delta is None:
98+ return
99+ changes = text_delta(
100+ delta, delta.delta_values, delta.new_values, delta.interface)
101+ # Now create the job to send the email.
102+ getUtility(IMergeProposalUpdatedEmailJobSource).create(
103+ merge_proposal, changes, from_person)
104
105
106 def send_review_requested_notifications(vote_reference, event):
107@@ -57,13 +67,14 @@
108 requested_reviews=None, preview_diff=None,
109 direct_email=False):
110 BranchMailer.__init__(
111- self, subject, template_name, recipients, from_address, delta,
112+ self, subject, template_name, recipients, from_address,
113 message_id=message_id, notification_type='code-review')
114 self.merge_proposal = merge_proposal
115 if requested_reviews is None:
116 requested_reviews = []
117 self.requested_reviews = requested_reviews
118 self.preview_diff = preview_diff
119+ self.delta_text = delta
120 self.template_params = self._generateTemplateParams()
121 self.direct_email = direct_email
122
123@@ -95,8 +106,7 @@
124 preview_diff=merge_proposal.preview_diff)
125
126 @classmethod
127- def forModification(cls, old_merge_proposal, merge_proposal,
128- from_user=None):
129+ def forModification(cls, merge_proposal, delta_text, from_user):
130 """Return a mailer for BranchMergeProposal creation.
131
132 :param merge_proposal: The BranchMergeProposal that was created.
133@@ -111,14 +121,11 @@
134 from_address = cls._format_user_address(from_user)
135 else:
136 from_address = config.canonical.noreply_from_address
137- delta = BranchMergeProposalDelta.construct(
138- old_merge_proposal, merge_proposal)
139- if delta is None:
140- return None
141 return cls(
142 '%(proposal_title)s',
143 'branch-merge-proposal-updated.txt', recipients,
144- merge_proposal, from_address, delta, get_msgid())
145+ merge_proposal, from_address, delta=delta_text,
146+ message_id=get_msgid())
147
148 @classmethod
149 def forReviewRequest(cls, reason, merge_proposal, from_user):
150@@ -189,6 +196,8 @@
151 'whiteboard': '', # No more whiteboard.
152 'diff_cutoff_warning': '',
153 }
154+ if self.delta_text is not None:
155+ params['delta'] = self.delta_text
156
157 if proposal.prerequisite_branch is not None:
158 prereq_url = proposal.prerequisite_branch.bzr_identity
159
160=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
161--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-29 01:14:29 +0000
162+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-29 01:14:30 +0000
163@@ -25,7 +25,7 @@
164 from lp.code.model.branch import update_trigger_modified_fields
165 from lp.code.model.branchmergeproposaljob import (
166 BranchMergeProposalJob, BranchMergeProposalJobType,
167- ReviewRequestedEmailJob)
168+ MergeProposalUpdatedEmailJob, ReviewRequestedEmailJob)
169 from lp.code.model.codereviewvote import CodeReviewVoteReference
170 from canonical.launchpad.webapp import canonical_url
171 from lp.testing import login_person, TestCaseWithFactory
172@@ -280,95 +280,93 @@
173 warning_text = "The attached diff has been truncated due to its size."
174 self.assertTrue(warning_text in ctrl.body)
175
176- def test_forModificationNoModification(self):
177+ def getProposalUpdatedEmailJob(self, merge_proposal):
178+ """Return the merge proposal updated email job."""
179+ jobs = list(
180+ IStore(BranchMergeProposalJob).find(
181+ BranchMergeProposalJob,
182+ BranchMergeProposalJob.branch_merge_proposal ==
183+ merge_proposal,
184+ BranchMergeProposalJob.job_type ==
185+ BranchMergeProposalJobType.MERGE_PROPOSAL_UPDATED))
186+ if len(jobs) == 0:
187+ return None
188+ elif len(jobs) == 1:
189+ return MergeProposalUpdatedEmailJob(jobs[0])
190+ else:
191+ self.fail('There are more than one jobs.')
192+
193+ def test_no_job_created_if_no_delta(self):
194 """Ensure None is returned if no change has been made."""
195 merge_proposal, person = self.makeProposalWithSubscriber()
196 old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
197- self.assertEqual(None, BMPMailer.forModification(
198- old_merge_proposal, merge_proposal, merge_proposal.registrant))
199+ event = ObjectModifiedEvent(
200+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
201+ send_merge_proposal_modified_notifications(merge_proposal, event)
202+ self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
203
204- def makeMergeProposalMailerModification(self):
205+ def makeProposalUpdatedEmailJob(self):
206 """Fixture method providing a mailer for a modified merge proposal"""
207 merge_proposal, subscriber = self.makeProposalWithSubscriber()
208 old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
209 merge_proposal.requestReview()
210 merge_proposal.commit_message = 'new commit message'
211 merge_proposal.description = 'change description'
212+ event = ObjectModifiedEvent(
213+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
214+ send_merge_proposal_modified_notifications(merge_proposal, event)
215+ job = self.getProposalUpdatedEmailJob(merge_proposal)
216+ self.assertIsNot(None, job, 'Job was not created.')
217+ return job, subscriber
218+
219+ def test_forModificationHasMsgId(self):
220+ """Ensure the right delta is filled out if there is a change."""
221+ merge_proposal = self.factory.makeBranchMergeProposal()
222 mailer = BMPMailer.forModification(
223- old_merge_proposal, merge_proposal, merge_proposal.registrant)
224- return mailer, subscriber
225-
226- def test_forModificationWithModificationDelta(self):
227- """Ensure the right delta is filled out if there is a change."""
228- mailer, subscriber = self.makeMergeProposalMailerModification()
229- self.assertEqual('new commit message',
230- mailer.delta.commit_message)
231-
232- def test_forModificationHasMsgId(self):
233- """Ensure the right delta is filled out if there is a change."""
234- mailer, subscriber = self.makeMergeProposalMailerModification()
235- assert mailer.message_id is not None, 'message_id not set'
236+ merge_proposal, 'the diff', merge_proposal.registrant)
237+ self.assertIsNot(None, mailer.message_id, 'message_id not set')
238
239 def test_forModificationWithModificationTextDelta(self):
240 """Ensure the right delta is filled out if there is a change."""
241- mailer, subscriber = self.makeMergeProposalMailerModification()
242+ job, subscriber = self.makeProposalUpdatedEmailJob()
243 self.assertEqual(
244 ' Status: Work in progress => Needs review\n\n'
245 'Commit Message changed to:\n\nnew commit message\n\n'
246 'Description changed to:\n\nchange description',
247- mailer.textDelta())
248-
249- def test_generateEmail(self):
250- """Ensure that contents of modification mails are right."""
251- mailer, subscriber = self.makeMergeProposalMailerModification()
252- ctrl = mailer.generateEmail('baz.quxx@example.com', subscriber)
253- self.assertEqual('[Merge] '
254- 'lp://dev/~bob/super-product/fix-foo-for-bar into '
255- 'lp://dev/~mary/super-product/bar', ctrl.subject)
256- url = canonical_url(mailer.merge_proposal)
257- reason = mailer._recipients.getReason(
258- subscriber.preferredemail.email)[0].getReason()
259- self.assertEqual("""\
260-The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
261-
262- Status: Work in progress => Needs review
263-
264-Commit Message changed to:
265-
266-new commit message
267-
268-Description changed to:
269-
270-change description
271---\x20
272-%s
273-%s
274-""" % (url, reason), ctrl.body)
275+ job.delta_text)
276
277 def test_send_merge_proposal_modified_notifications(self):
278 """Should send emails when invoked with correct parameters."""
279- merge_proposal, subscriber = self.makeProposalWithSubscriber()
280- snapshot = BranchMergeProposalDelta.snapshot(merge_proposal)
281- merge_proposal.commit_message = 'new message'
282- event = ObjectModifiedEvent(merge_proposal, snapshot, None)
283+ job, subscriber = self.makeProposalUpdatedEmailJob()
284 pop_notifications()
285- send_merge_proposal_modified_notifications(merge_proposal, event)
286+ job.run()
287 emails = pop_notifications()
288 self.assertEqual(3, len(emails),
289 'There should be three emails sent out. One to the '
290 'explicit subscriber above, and one each to the '
291 'source branch owner and the target branch owner '
292 'who were implicitly subscribed to their branches.')
293-
294- def test_send_merge_proposal_modified_notifications_no_delta(self):
295- """Should not send emails if no delta."""
296- merge_proposal, subscriber = self.makeProposalWithSubscriber()
297- snapshot = BranchMergeProposalDelta.snapshot(merge_proposal)
298- event = ObjectModifiedEvent(merge_proposal, snapshot, None)
299- pop_notifications()
300- send_merge_proposal_modified_notifications(merge_proposal, event)
301- emails = pop_notifications()
302- self.assertEqual([], emails)
303+ email = emails[0]
304+ self.assertEqual('[Merge] '
305+ 'lp://dev/~bob/super-product/fix-foo-for-bar into\n\t'
306+ 'lp://dev/~mary/super-product/bar', email['subject'])
307+ self.assertEqual(dedent("""\
308+ The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
309+
310+ Status: Work in progress => Needs review
311+
312+ Commit Message changed to:
313+
314+ new commit message
315+
316+ Description changed to:
317+
318+ change description
319+ --\x20
320+ %s
321+ You are subscribed to branch lp://dev/~bob/super-product/fix-foo-for-bar.
322+ """) % canonical_url(job.branch_merge_proposal),
323+ email.get_payload(decode=True))
324
325 def assertRecipientsMatches(self, recipients, mailer):
326 """Assert that `mailer` will send to the people in `recipients`."""
327
328=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
329--- lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 01:14:29 +0000
330+++ lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 01:14:30 +0000
331@@ -52,7 +52,8 @@
332 IBranchMergeProposalJob, ICodeReviewCommentEmailJob,
333 ICodeReviewCommentEmailJobSource, ICreateMergeProposalJob,
334 ICreateMergeProposalJobSource, IMergeProposalCreatedJob,
335- IMergeProposalCreatedJobSource, IReviewRequestedEmailJob,
336+ IMergeProposalCreatedJobSource, IMergeProposalUpdatedEmailJob,
337+ IMergeProposalUpdatedEmailJobSource, IReviewRequestedEmailJob,
338 IReviewRequestedEmailJobSource, IUpdatePreviewDiffJobSource,
339 )
340 from lp.code.mail.branch import RecipientReason
341@@ -97,6 +98,13 @@
342 requested reviewer team asking them to review the proposal.
343 """)
344
345+ MERGE_PROPOSAL_UPDATED = DBItem(4, """
346+ Merge proposal updated
347+
348+ This job sends an email to the subscribers informing them of fields
349+ that have been changed on the merge proposal itself.
350+ """)
351+
352
353 class BranchMergeProposalJob(Storm):
354 """Base class for jobs related to branch merge proposals."""
355@@ -491,3 +499,68 @@
356 if self.requester is not None:
357 recipients.append(self.requester.preferredemail)
358 return recipients
359+
360+
361+class MergeProposalUpdatedEmailJob(BranchMergeProposalJobDerived):
362+ """Send email to the subscribers informing them of updated fields.
363+
364+ When attributes of the merge proposal are edited, we inform the
365+ subscribers.
366+ """
367+
368+ implements(IMergeProposalUpdatedEmailJob)
369+
370+ classProvides(IMergeProposalUpdatedEmailJobSource)
371+
372+ class_job_type = BranchMergeProposalJobType.MERGE_PROPOSAL_UPDATED
373+
374+ def run(self):
375+ """See `IRunnableJob`."""
376+ mailer = BMPMailer.forModification(
377+ self.branch_merge_proposal, self.delta_text, self.editor)
378+ mailer.sendAll()
379+
380+ @classmethod
381+ def create(cls, merge_proposal, delta_text, editor):
382+ """See `IReviewRequestedEmailJobSource`."""
383+ metadata = cls.getMetadata(delta_text, editor)
384+ job = BranchMergeProposalJob(
385+ merge_proposal, cls.class_job_type, metadata)
386+ return cls(job)
387+
388+ @staticmethod
389+ def getMetadata(delta_text, editor):
390+ metadata = {'delta_text': delta_text}
391+ if editor is not None:
392+ metadata['editor'] = editor.name;
393+ return metadata
394+
395+ @property
396+ def editor(self):
397+ """The person who updated the merge proposal."""
398+ editor_name = self.metadata.get('editor')
399+ if editor_name is None:
400+ return None
401+ else:
402+ return getUtility(IPersonSet).getByName(editor_name)
403+
404+ @property
405+ def delta_text(self):
406+ """The changes that were made to the merge proposal."""
407+ return self.metadata['delta_text']
408+
409+ def getOopsVars(self):
410+ """See `IRunnableJob`."""
411+ vars = BranchMergeProposalJobDerived.getOopsVars(self)
412+ vars.extend([
413+ ('editor', self.metadata.get('editor', '(not set)')),
414+ ('delta_text', self.metadata['delta_text']),
415+ ])
416+ return vars
417+
418+ def getErrorRecipients(self):
419+ """Return a list of email-ids to notify about user errors."""
420+ recipients = []
421+ if self.editor is not None:
422+ recipients.append(self.editor.preferredemail)
423+ return recipients