Merge lp:~abentley/launchpad/notify-merge into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
fix bug #382527 by sending our normal kind of modification email.

== Proposed fix ==
emit an ObjectModified event when a BMP is marked as merged. Allow BMP
emails to be sent when event.user is an UnauthenticatedPrincipal, like
when scripts run.

== Pre-implementation notes ==
Discussed with thumper and flacoste

== Implementation details ==
Until now, we've always used from_user to generate the from_address.
Now, we allow from_user to be None, and use
config.canonical.noreply_from_address when it is.

We provide from_user=None when event.user is an UnauthenticatedPrincipal.

== Tests ==
bin/test -t TestNotifyModified -t test_mergeProposalMergeDetected

== Demo and Q/A ==
Push up source and target branches. Propose a merge. Perform a merge
of source into target. Push target. You should get an email that the
proposal was merged.

= Launchpad lint =

The ILaunchpadCelebrities import is fixed in another branch.

The too-long line is hard to fix, because it is a test name.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/codehosting/scanner/tests/test_mergedetection.py
  lib/lp/code/interfaces/branchmergeproposal.py
  lib/lp/code/model/tests/test_branchmergeproposals.py
  lib/lp/code/mail/branchmergeproposal.py
  lib/lp/codehosting/scanner/mergedetection.py

== Pyflakes notices ==

lib/lp/code/model/tests/test_branchmergeproposals.py
    26: 'ILaunchpadCelebrities' imported but unused

== Pylint notices ==

lib/lp/codehosting/scanner/tests/test_mergedetection.py
    107: [C0301] Line too long (79/78)

lib/lp/code/model/tests/test_branchmergeproposals.py
    26: [W0611] Unused import ILaunchpadCelebrities
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkp8hhsACgkQ0F+nu1YWqI2MygCfaF/90QfTcLHiGBDQ3MaCd7Uj
Q3wAn0J4PjWp9KE304ZO6Dyk9ALLQPSW
=tfl8
-----END PGP SIGNATURE-----

[Brad Crittenden (bac)]

Nice branch Aaron. This change will make working with merge proposals much easier to track.

review: Approve

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2009-08-06 00:15:48 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2009-08-07 15:41:43 +0000
@@ -24,6 +24,8 @@
     'WrongBranchMergeProposal',
     ]
 
+from lazr.lifecycle.event import ObjectModifiedEvent
+from zope.event import notify
 from zope.interface import Attribute, Interface
 from zope.schema import (
     Bytes, Choice, Datetime, Int, Object, Text, TextLine)
@@ -595,3 +597,19 @@
 
     def iterReady():
         """Iterate through all ready MergeProposalCreatedJobs."""
+
+
+def notify_modified(proposal, func, *args, **kwargs):
+    """Call func, then notify about the changes it made.
+
+    :param proposal: the merge proposal to notify about.
+    :param func: The callable that will modify the merge proposal.
+    :param args: Additional arguments for the method.
+    :param kwargs: Keyword arguments for the method.
+    :return: The return value of the method.
+    """
+    from lp.code.adapters.branch import BranchMergeProposalDelta
+    snapshot = BranchMergeProposalDelta.snapshot(proposal)
+    result = func(*args, **kwargs)
+    notify(ObjectModifiedEvent(proposal, snapshot, []))
+    return result

=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
--- lib/lp/code/mail/branchmergeproposal.py	2009-08-05 02:18:13 +0000
+++ lib/lp/code/mail/branchmergeproposal.py	2009-08-07 19:39:48 +0000
@@ -6,8 +6,10 @@
 
 __metaclass__ = type
 
+from zope.app.security.principalregistry import UnauthenticatedPrincipal
 from zope.component import getUtility
 
+from canonical.config import config
 from canonical.launchpad.mail import get_msgid
 from canonical.launchpad.webapp import canonical_url
 from lp.code.adapters.branch import BranchMergeProposalDelta
@@ -32,8 +34,12 @@
     """Notify branch subscribers when merge proposals are updated."""
     if event.user is None:
         return
+    if isinstance(event.user, UnauthenticatedPrincipal):
+        from_person = None
+    else:
+        from_person = IPerson(event.user)
     mailer = BMPMailer.forModification(
-        event.object_before_modification, merge_proposal, IPerson(event.user))
+        event.object_before_modification, merge_proposal, from_person)
     if mailer is not None:
         mailer.sendAll()
 
@@ -104,18 +110,22 @@
             review_diff=merge_proposal.review_diff)
 
     @classmethod
-    def forModification(cls, old_merge_proposal, merge_proposal, from_user):
+    def forModification(cls, old_merge_proposal, merge_proposal,
+                        from_user=None):
         """Return a mailer for BranchMergeProposal creation.
 
         :param merge_proposal: The BranchMergeProposal that was created.
         :param from_user: The user that the creation notification should
-            come from.
+            come from.  Optional.
         """
         recipients = merge_proposal.getNotificationRecipients(
             CodeReviewNotificationLevel.STATUS)
-        assert from_user.preferredemail is not None, (
-            'The sender must have an email address.')
-        from_address = cls._format_user_address(from_user)
+        if from_user is not None:
+            assert from_user.preferredemail is not None, (
+                'The sender must have an email address.')
+            from_address = cls._format_user_address(from_user)
+        else:
+            from_address = config.canonical.noreply_from_address
         delta = BranchMergeProposalDelta.construct(
                 old_merge_proposal, merge_proposal)
         if delta is None:

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-08-06 00:15:48 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-08-07 15:41:43 +0000
@@ -21,6 +21,7 @@
 from canonical.database.constants import UTC_NOW
 from canonical.testing import (
     DatabaseFunctionalLayer, LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
+from lazr.lifecycle.event import ObjectModifiedEvent
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.code.model.branchmergeproposaljob import (
@@ -42,7 +43,7 @@
     BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
     IBranchMergeProposalGetter, IBranchMergeProposalJob,
     ICreateMergeProposalJob, ICreateMergeProposalJobSource,
-    IMergeProposalCreatedJob, WrongBranchMergeProposal)
+    IMergeProposalCreatedJob, notify_modified, WrongBranchMergeProposal)
 from canonical.launchpad.interfaces.message import IMessageJob
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
@@ -1081,6 +1082,28 @@
             SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
 
 
+class TestNotifyModified(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_notify_modified_generates_notification(self):
+        """notify_modified generates an event.
+
+        notify_modified runs the callable with the specified args and kwargs,
+        and generates a ObjectModifiedEvent.
+        """
+        bmp = self.factory.makeBranchMergeProposal()
+        login_person(bmp.target_branch.owner)
+        # Approve branch to prevent enqueue from approving it, which would
+        # generate an undesired event.
+        bmp.approveBranch(bmp.target_branch.owner, revision_id='abc')
+        self.assertNotifies(
+            ObjectModifiedEvent, notify_modified, bmp, bmp.enqueue,
+            bmp.target_branch.owner, revision_id='abc')
+        self.assertEqual(BranchMergeProposalStatus.QUEUED, bmp.queue_status)
+        self.assertEqual('abc', bmp.queued_revision_id)
+
+
 class TestBranchMergeProposalJob(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/codehosting/scanner/mergedetection.py'
--- lib/lp/codehosting/scanner/mergedetection.py	2009-06-30 16:56:07 +0000
+++ lib/lp/codehosting/scanner/mergedetection.py	2009-08-06 19:07:32 +0000
@@ -18,7 +18,7 @@
 from lp.code.enums import BranchLifecycleStatus
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.code.interfaces.branchmergeproposal import (
-    BRANCH_MERGE_PROPOSAL_FINAL_STATES)
+    BRANCH_MERGE_PROPOSAL_FINAL_STATES, notify_modified)
 
 
 def is_series_branch(branch):
@@ -62,7 +62,7 @@
         if is_development_focus(target):
             mark_branch_merged(logger, source)
     else:
-        proposal.markAsMerged()
+        notify_modified(proposal, proposal.markAsMerged)
         # If there is an explicit merge proposal, change the branch's
         # status when it's been merged into a development focus or any
         # other series branch.

=== modified file 'lib/lp/codehosting/scanner/tests/test_mergedetection.py'
--- lib/lp/codehosting/scanner/tests/test_mergedetection.py	2009-06-30 16:56:07 +0000
+++ lib/lp/codehosting/scanner/tests/test_mergedetection.py	2009-08-06 19:07:32 +0000
@@ -22,6 +22,7 @@
 from lp.code.enums import BranchLifecycleStatus, BranchMergeProposalStatus
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.testing import TestCaseWithFactory
+from lp.testing.mail_helpers import pop_notifications
 from canonical.testing import LaunchpadZopelessLayer
 
 
@@ -261,6 +262,16 @@
         self.assertEqual(
             BranchLifecycleStatus.MERGED,
             proposal.source_branch.lifecycle_status)
+        notifications = pop_notifications()
+        self.assertIn('Work in progress => Merged',
+                      notifications[0].get_payload(decode=True))
+        self.assertEqual(
+            config.canonical.noreply_from_address, notifications[0]['From'])
+        recipients = set(msg['x-envelope-to'] for msg in notifications)
+        expected = set(
+            [proposal.source_branch.registrant.preferredemail.email,
+             proposal.target_branch.registrant.preferredemail.email])
+        self.assertEqual(expected, recipients)
 
     def test_mergeProposalMergeDetected_not_series(self):
         # If the target branch is not a series branch, then the merge proposal