Code review comment for lp:~thumper/launchpad/new-reviewer-email-job

Hi Tim,

There are some real issues that 'make lint' exposes. Good luck
finding them amongst all of the noise. :)

Otherwise this is a fine branch with the exception of a few items marked below.

Thanks.

--Brad

> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:29 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:31 +0000
> @@ -613,6 +615,23 @@
> """Create a job to email subscribers about the comment."""
>
>
> +class IReviewRequestedEmailJob(IRunnableJob):
> + """Interface for the job to sends review request emails."""
> +
> + reviewer = Attribute('The person or team asked to do the review.')
> + requester = Attribute('The person who as asked for the review.')

s/as/has/

> === modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
> --- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-05 03:35:10 +0000
> +++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-29 00:59:31 +0000
> @@ -431,6 +437,76 @@
> login_person(self.owner)
> return person
>
> + def getReviewNotificationEmail(self):
> + """Return the review requested email job for the test's proposal."""
> + [job] = list(
> + IStore(BranchMergeProposalJob).find(
> + BranchMergeProposalJob,
> + BranchMergeProposalJob.branch_merge_proposal == self.bmp,
> + BranchMergeProposalJob.job_type ==
> + BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
> + return ReviewRequestedEmailJob(job)
> +
> + def test_nominateReview_creates_job(self):
> + # When a reviewer is nominated, a job is created to send out the
> + # review request email.
> + reviewer = self.factory.makePerson()
> + pop_notifications()
> + vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
> + # No email is sent.
> + sent_mail = pop_notifications()
> + self.assertEqual([], sent_mail)
> + # A job is created.
> + review_request_job = self.getReviewNotificationEmail()
> + self.assertEqual(self.bmp, review_request_job.branch_merge_proposal)
> + self.assertEqual(reviewer, review_request_job.reviewer)
> + self.assertEqual(self.bmp.registrant, review_request_job.requester)
> +
> + def test_nominateReview_email_content(self):
> + # The email that is sent contains the description of the proposal, and
> + # a link to the proposal.
> + reviewer = self.factory.makePerson()
> + self.bmp.description = 'This branch is awesome.'
> + vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
> + review_request_job = self.getReviewNotificationEmail()
> + review_request_job.run()
> + [sent_mail] = pop_notifications()
> + self.assertEqual(
> + dedent("""\
> + You have been requested to review the proposed merge of %(source)s into %(target)s.
> +
> + This branch is awesome.
> +
> + --
> + %(bmp)s
> + You are requested to review the proposed merge of %(source)s into %(target)s.
> + """ % {
> + 'source': self.bmp.source_branch.bzr_identity,
> + 'target': self.bmp.target_branch.bzr_identity,
> + 'bmp': canonical_url(self.bmp)}),
> + sent_mail.get_payload(decode=True))

I think this would be easier to read if you extracted the creation of
the expected message out of the call to assertEqual.

> +
> + def test_nominateReview_emails_team_address(self):
> + # If a review request is made for a team, the members of the team are
> + # sent an email.
> + eric = self.factory.makePerson(
> + displayname='Eric the Viking', <email address hidden>')
> + black_beard = self.factory.makePerson(
> + displayname='Black Beard', <email address hidden>')

You know Blackbeard was done in not far from here...

> + review_team = self.factory.makeTeam(owner=eric)
> + login_person(eric)
> + review_team.addMember(black_beard, eric)
> + pop_notifications()
> + login_person(self.owner)
> + vote_reference = self.bmp.nominateReviewer(review_team, self.owner, None)
> + review_request_job = self.getReviewNotificationEmail()
> + review_request_job.run()
> + sent_mail = pop_notifications()
> + self.assertEqual(
> + ['Black Beard <email address hidden>',
> + 'Eric the Viking <email address hidden>'],
> + sorted(mail['to'] for mail in sent_mail))
> +
> def test_requestReviewWithPrivateEmail(self):
> # We can request a review, even when one of the parties involved has a
> # private email address.
> @@ -438,11 +514,9 @@
> # Request a review and prepare the mailer.
> vote_reference = self.bmp.nominateReviewer(
> candidate, self.owner, None)
> - reason = RecipientReason.forReviewer(vote_reference, candidate)
> - mailer = BMPMailer.forReviewRequest(reason, self.bmp, self.owner)
> # Send the mail.
> - pop_notifications()
> - mailer.sendAll()
> + review_request_job = self.getReviewNotificationEmail()
> + review_request_job.run()
> mails = pop_notifications()
> self.assertEqual(1, len(mails))
> candidate = removeSecurityProxy(candidate)

> === modified file 'lib/lp/code/model/branchmergeproposaljob.py'
> --- lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 00:59:29 +0000
> +++ lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 00:59:31 +0000
> @@ -418,3 +429,65 @@
> """Return a list of email-ids to notify about user errors."""
> commenter = self.code_review_comment.message.owner
> return [commenter.preferredemail]
> +
> +
> +class ReviewRequestedEmailJob(BranchMergeProposalJobDerived):
> + """Send email to the reviewer telling them to review the proposal.
> +
> + Provides class methods to create and retrieve such jobs.
> + """
> +
> + implements(IReviewRequestedEmailJob)
> +
> + classProvides(IReviewRequestedEmailJobSource)
> +
> + class_job_type = BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL
> +
> + def run(self):
> + """See `IRunnableJob`."""
> + reason = RecipientReason.forReviewer(
> + self.branch_merge_proposal, True, self.reviewer)
> + mailer = BMPMailer.forReviewRequest(
> + reason, self.branch_merge_proposal, self.requester)
> + mailer.sendAll()
> +
> + @classmethod
> + def create(cls, review_request):
> + """See `IReviewRequestedEmailJobSource`."""
> + metadata = cls.getMetadata(review_request)
> + bmp = review_request.branch_merge_proposal
> + job = BranchMergeProposalJob(bmp, cls.class_job_type, metadata)
> + return cls(job)
> +
> + @staticmethod
> + def getMetadata(review_request):
> + return {
> + 'reviewer': review_request.reviewer.name,
> + 'requester': review_request.registrant.name,
> + }
> +
> + @property
> + def reviewer(self):
> + """The person or team who has been asked to review."""
> + return getUtility(IPersonSet).getByName(self.metadata['reviewer'])
> +
> + @property
> + def requester(self):
> + """The person who requested the review to be done."""
> + return getUtility(IPersonSet).getByName(self.metadata['requester'])
> +
> + def getOopsVars(self):
> + """See `IRunnableJob`."""
> + vars = BranchMergeProposalJobDerived.getOopsVars(self)
> + vars.extend([
> + ('reviewer', self.metadata['reviewer']),
> + ('requester', self.metadata['requester']),
> + ])
> + return vars
> +
> + def getErrorRecipients(self):
> + """Return a list of email-ids to notify about user errors."""
> + recipients = []
> + if self.requester is not None:
> + recipients.append(self.requester.preferredemail)
> + return recipients