launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01714
[Merge] lp:~thumper/launchpad/less-wip-emails into lp:launchpad/devel
Tim Penhey has proposed merging lp:~thumper/launchpad/less-wip-emails into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#667554 State change and reviewer request emails go out for work in progress
https://bugs.launchpad.net/bugs/667554
During the QA of the change where the initial code review email doesn't go out for work in progress merge proposals, it became clear that we should be suppressing other emails, for example the review requests while work in progress, and the state change emails.
This means we won't get an email that says WIP -> Needs Review, followed by the initial review email, we'll just get the initial review email.
Also all of the reviewers get the initial review request, so extra review request emails don't go out when the proposal is work in progress.
tests:
code.mail.tests.test_branchmergeproposal
There was also some test refactoring to have the tests use updated factory methods instead of rolling their own.
--
https://code.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/less-wip-emails into lp:launchpad/devel.
=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-04 19:50:45 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-28 02:53:22 +0000
@@ -9,7 +9,9 @@
from unittest import TestLoader
from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
import transaction
+from zope.interface import providedBy
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.interfaces.lpstorm import IStore
@@ -18,8 +20,8 @@
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
)
-from lp.code.adapters.branch import BranchMergeProposalDelta
from lp.code.enums import (
+ BranchMergeProposalStatus,
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
)
@@ -51,7 +53,9 @@
super(TestMergeProposalMailing, self).setUp('admin@xxxxxxxxxxxxx')
def makeProposalWithSubscriber(self, diff_text=None,
- initial_comment=None, prerequisite=False):
+ initial_comment=None,
+ prerequisite=False,
+ needs_review=True):
if diff_text is not None:
preview_diff = PreviewDiff.create(
diff_text,
@@ -68,8 +72,13 @@
prerequisite_branch = self.factory.makeProductBranch(product)
else:
prerequisite_branch = None
+ if needs_review:
+ initial_status = BranchMergeProposalStatus.NEEDS_REVIEW
+ else:
+ initial_status = BranchMergeProposalStatus.WORK_IN_PROGRESS
bmp = self.factory.makeBranchMergeProposal(
registrant=registrant, product=product,
+ set_state=initial_status,
prerequisite_branch=prerequisite_branch,
preview_diff=preview_diff, initial_comment=initial_comment)
subscriber = self.factory.makePerson(displayname='Baz Quxx',
@@ -308,7 +317,21 @@
def test_no_job_created_if_no_delta(self):
"""Ensure None is returned if no change has been made."""
merge_proposal, person = self.makeProposalWithSubscriber()
- old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
+ old_merge_proposal = Snapshot(
+ merge_proposal, providing=providedBy(merge_proposal))
+ event = ObjectModifiedEvent(
+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
+ merge_proposal_modified(merge_proposal, event)
+ self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
+
+ def test_no_job_created_if_work_in_progress(self):
+ """Ensure None is returned if no change has been made."""
+ merge_proposal, person = self.makeProposalWithSubscriber(
+ needs_review=False)
+ old_merge_proposal = Snapshot(
+ merge_proposal, providing=providedBy(merge_proposal))
+ merge_proposal.commit_message = 'new commit message'
+ merge_proposal.description = 'change description'
event = ObjectModifiedEvent(
merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
merge_proposal_modified(merge_proposal, event)
@@ -317,7 +340,8 @@
def makeProposalUpdatedEmailJob(self):
"""Fixture method providing a mailer for a modified merge proposal"""
merge_proposal, subscriber = self.makeProposalWithSubscriber()
- old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
+ old_merge_proposal = Snapshot(
+ merge_proposal, providing=providedBy(merge_proposal))
merge_proposal.requestReview()
merge_proposal.commit_message = 'new commit message'
merge_proposal.description = 'change description'
@@ -339,7 +363,6 @@
"""Ensure the right delta is filled out if there is a change."""
job, subscriber = self.makeProposalUpdatedEmailJob()
self.assertEqual(
- ' Status: Work in progress => Needs review\n\n'
'Commit Message changed to:\n\nnew commit message\n\n'
'Description changed to:\n\nchange description',
job.delta_text)
@@ -363,8 +386,6 @@
expected = dedent("""\
The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
- Status: Work in progress => Needs review
-
Commit Message changed to:
new commit message
@@ -431,52 +452,64 @@
layer = DatabaseFunctionalLayer
- def setUp(self):
- TestCaseWithFactory.setUp(self)
- self.owner = self.factory.makePerson()
- login_person(self.owner)
- self.bmp = self.factory.makeBranchMergeProposal(registrant=self.owner)
-
- def makePersonWithHiddenEmail(self):
- """Make an arbitrary person with hidden email addresses."""
- person = self.factory.makePerson()
- login_person(person)
- person.hide_email_addresses = True
- login_person(self.owner)
- return person
-
- def getReviewNotificationEmail(self):
+ def getReviewNotificationEmail(self, bmp):
"""Return the review requested email job for the test's proposal."""
[job] = list(
IStore(BranchMergeProposalJob).find(
BranchMergeProposalJob,
- BranchMergeProposalJob.branch_merge_proposal == self.bmp,
+ BranchMergeProposalJob.branch_merge_proposal == bmp,
BranchMergeProposalJob.job_type ==
BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
return ReviewRequestedEmailJob(job)
+ def test_nominateReview_no_job_if_work_in_progress(self):
+ # When a reviewer is nominated for a proposal that is work in
+ # progress, there is no email job created.
+ bmp = self.factory.makeBranchMergeProposal(
+ set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
+ login_person(bmp.registrant)
+ reviewer = self.factory.makePerson()
+ pop_notifications()
+ bmp.nominateReviewer(reviewer, bmp.registrant, None)
+ # No email is sent.
+ sent_mail = pop_notifications()
+ self.assertEqual([], sent_mail)
+ # No job created.
+ job = IStore(BranchMergeProposalJob).find(
+ BranchMergeProposalJob,
+ BranchMergeProposalJob.branch_merge_proposal == bmp,
+ BranchMergeProposalJob.job_type ==
+ BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL).any()
+ self.assertIs(None, job)
+
def test_nominateReview_creates_job(self):
# When a reviewer is nominated, a job is created to send out the
# review request email.
+ bmp = self.factory.makeBranchMergeProposal(
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ login_person(bmp.registrant)
reviewer = self.factory.makePerson()
pop_notifications()
- self.bmp.nominateReviewer(reviewer, self.owner, None)
+ bmp.nominateReviewer(reviewer, bmp.registrant, 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)
+ review_request_job = self.getReviewNotificationEmail(bmp)
+ self.assertEqual(bmp, review_request_job.branch_merge_proposal)
self.assertEqual(reviewer, review_request_job.reviewer)
- self.assertEqual(self.bmp.registrant, review_request_job.requester)
+ self.assertEqual(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.
+ bmp = self.factory.makeBranchMergeProposal(
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ login_person(bmp.registrant)
reviewer = self.factory.makePerson()
- self.bmp.description = 'This branch is awesome.'
- self.bmp.nominateReviewer(reviewer, self.owner, None)
- review_request_job = self.getReviewNotificationEmail()
+ bmp.description = 'This branch is awesome.'
+ bmp.nominateReviewer(reviewer, bmp.registrant, None)
+ review_request_job = self.getReviewNotificationEmail(bmp)
review_request_job.run()
[sent_mail] = pop_notifications()
expected = dedent("""\
@@ -488,25 +521,25 @@
%(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)})
+ 'source': bmp.source_branch.bzr_identity,
+ 'target': bmp.target_branch.bzr_identity,
+ 'bmp': canonical_url(bmp)})
self.assertEqual(expected, sent_mail.get_payload(decode=True))
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.
+ bmp = self.factory.makeBranchMergeProposal(
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ login_person(bmp.registrant)
eric = self.factory.makePerson(
displayname='Eric the Viking', email='eric@xxxxxxxxxxxxxxxxxxx')
black_beard = self.factory.makePerson(
displayname='Black Beard', email='black@xxxxxxxxxxxxxxxxxxx')
- review_team = self.factory.makeTeam(owner=eric)
- login_person(eric)
- review_team.addMember(black_beard, eric)
+ review_team = self.factory.makeTeam(owner=eric, members=[black_beard])
pop_notifications()
- login_person(self.owner)
- self.bmp.nominateReviewer(review_team, self.owner, None)
- review_request_job = self.getReviewNotificationEmail()
+ bmp.nominateReviewer(review_team, bmp.registrant, None)
+ review_request_job = self.getReviewNotificationEmail(bmp)
review_request_job.run()
sent_mail = pop_notifications()
self.assertEqual(
@@ -517,11 +550,14 @@
def test_requestReviewWithPrivateEmail(self):
# We can request a review, even when one of the parties involved has a
# private email address.
- candidate = self.makePersonWithHiddenEmail()
+ bmp = self.factory.makeBranchMergeProposal(
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ login_person(bmp.registrant)
+ candidate = self.factory.makePerson(hide_email_addresses=True)
# Request a review and prepare the mailer.
- self.bmp.nominateReviewer(candidate, self.owner, None)
+ bmp.nominateReviewer(candidate, bmp.registrant, None)
# Send the mail.
- review_request_job = self.getReviewNotificationEmail()
+ review_request_job = self.getReviewNotificationEmail(bmp)
review_request_job.run()
mails = pop_notifications()
self.assertEqual(1, len(mails))
=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py 2010-10-18 03:39:28 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py 2010-10-28 02:53:22 +0000
@@ -10,6 +10,7 @@
from zope.component import getUtility
from lp.code.adapters.branch import BranchMergeProposalDelta
+from lp.code.enums import BranchMergeProposalStatus
from lp.code.interfaces.branchmergeproposal import (
IMergeProposalNeedsReviewEmailJobSource,
IMergeProposalUpdatedEmailJobSource,
@@ -48,6 +49,11 @@
from_person = None
else:
from_person = IPerson(event.user)
+ # If the merge proposal was work in progress, then we don't want to send
+ # out an email as the needs review email will cover that.
+ old_status = event.object_before_modification.queue_status
+ if old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
+ return
# Create a delta of the changes. If there are no changes to report, then
# we're done.
delta = BranchMergeProposalDelta.construct(
@@ -63,5 +69,8 @@
def review_requested(vote_reference, event):
"""Notify the reviewer that they have been requested to review."""
- getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
+ # Don't send email if the proposal is work in progress.
+ bmp_status = vote_reference.branch_merge_proposal.queue_status
+ if bmp_status != BranchMergeProposalStatus.WORK_IN_PROGRESS:
+ getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
