launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-more-mail into lp:launchpad.

Commit message:
Allow merge-proposal-jobs DB user to access the GitSubscription table.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-more-mail/+merge/260753

Allow merge-proposal-jobs DB user to access the GitSubscription table.  I beefed up some tests along the way to catch this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-more-mail into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2015-05-26 13:35:50 +0000
+++ database/schema/security.cfg	2015-06-01 17:14:50 +0000
@@ -1940,6 +1940,7 @@
 public.emailaddress                     = SELECT
 public.gitref                           = SELECT
 public.gitrepository                    = SELECT
+public.gitsubscription                  = SELECT
 public.incrementaldiff                  = SELECT, INSERT
 public.job                              = SELECT, INSERT, UPDATE
 public.karma                            = SELECT, INSERT

=== modified file 'lib/lp/code/mail/tests/test_branch.py'
--- lib/lp/code/mail/tests/test_branch.py	2015-05-14 13:57:51 +0000
+++ lib/lp/code/mail/tests/test_branch.py	2015-06-01 17:14:50 +0000
@@ -158,9 +158,7 @@
             reason.getReason())
 
 
-# XXX cjwatson 2015-04-15: Enable this (by adding TestCaseWithFactory) once
-# BranchMergeProposal supports Git.
-class TestRecipientReasonGit(TestRecipientReasonMixin):
+class TestRecipientReasonGit(TestRecipientReasonMixin, TestCaseWithFactory):
     """Test RecipientReason for Git references."""
 
     def makeProposalWithSubscription(self, subscriber=None):
@@ -168,10 +166,10 @@
         if subscriber is None:
             subscriber = self.factory.makePerson()
         source_repository = self.factory.makeGitRepository()
-        source_ref = self.factory.makeGitRefs(repository=source_repository)
+        [source_ref] = self.factory.makeGitRefs(repository=source_repository)
         target_repository = self.factory.makeGitRepository(
             target=source_repository.target)
-        target_ref = self.factory.makeGitRefs(repository=target_repository)
+        [target_ref] = self.factory.makeGitRefs(repository=target_repository)
         merge_proposal = source_ref.addLandingTarget(
             source_repository.owner, target_ref)
         subscription = merge_proposal.merge_source.subscribe(

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2015-05-14 13:57:51 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2015-06-01 17:14:50 +0000
@@ -78,6 +78,24 @@
 from lp.testing.mail_helpers import pop_notifications
 
 
+class BzrMixin:
+    """Mixin for Bazaar-based tests."""
+
+    def makeBranchMergeProposal(self, merge_source=None, merge_target=None,
+                                **kwargs):
+        return self.factory.makeBranchMergeProposal(
+            source_branch=merge_source, target_branch=merge_target, **kwargs)
+
+
+class GitMixin:
+    """Mixin for Git-based tests."""
+
+    def makeBranchMergeProposal(self, merge_source=None, merge_target=None,
+                                **kwargs):
+        return self.factory.makeBranchMergeProposalForGit(
+            source_ref=merge_source, target_ref=merge_target, **kwargs)
+
+
 class TestBranchMergeProposalJob(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
@@ -116,19 +134,19 @@
         self.assertEqual(job, MergeProposalNeedsReviewEmailJob.get(job.id))
 
 
-class TestMergeProposalNeedsReviewEmailJob(TestCaseWithFactory):
+class TestMergeProposalNeedsReviewEmailJobMixin:
 
     layer = LaunchpadZopelessLayer
 
     def test_providesInterface(self):
         """MergeProposalNeedsReviewEmailJob provides expected interfaces."""
-        bmp = self.factory.makeBranchMergeProposal()
+        bmp = self.makeBranchMergeProposal()
         job = MergeProposalNeedsReviewEmailJob.create(bmp)
         verifyObject(IMergeProposalNeedsReviewEmailJob, job)
         verifyObject(IBranchMergeProposalJob, job)
 
     def test_getOperationDescription(self):
-        bmp = self.factory.makeBranchMergeProposal()
+        bmp = self.makeBranchMergeProposal()
         job = MergeProposalNeedsReviewEmailJob.create(bmp)
         self.assertTrue(
             job.getOperationDescription().startswith(
@@ -138,40 +156,50 @@
         self.assertNotIn('+bar', diff.diff.text)
         self.assertIn('+qux', diff.diff.text)
 
-    def createProposalWithEmptyBranches(self):
-        target_branch, tree = self.create_branch_and_tree()
-        # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
-        # required to generate the revision-id.
-        with override_environ(BZR_EMAIL='me@xxxxxxxxxxx'):
-            tree.commit('test')
-        source_branch = self.factory.makeProductBranch(
-            product=target_branch.product)
-        self.createBzrBranch(source_branch, tree.branch)
-        return self.factory.makeBranchMergeProposal(
-            source_branch=source_branch, target_branch=target_branch)
-
     def test_run_sends_email(self):
         """MergeProposalCreationJob.run sends an email."""
-        self.useBzrBranches(direct_database=True)
         bmp = self.createProposalWithEmptyBranches()
         job = MergeProposalNeedsReviewEmailJob.create(bmp)
         self.assertEqual([], pop_notifications())
-        job.run()
+        with dbuser("merge-proposal-jobs"):
+            JobRunner([job]).runAll()
         self.assertEqual(2, len(pop_notifications()))
 
     def test_getOopsMailController(self):
         """The registrant is notified about merge proposal creation issues."""
-        bmp = self.factory.makeBranchMergeProposal()
+        bmp = self.makeBranchMergeProposal()
         job = MergeProposalNeedsReviewEmailJob.create(bmp)
         ctrl = job.getOopsMailController('1234')
         self.assertEqual([bmp.registrant.preferredemail.email], ctrl.to_addrs)
         message = (
             'notifying people about the proposal to merge %s into %s' %
-            (bmp.source_branch.bzr_identity, bmp.target_branch.bzr_identity))
+            (bmp.merge_source.identity, bmp.merge_target.identity))
         self.assertIn(message, ctrl.body)
 
+
+class TestMergeProposalNeedsReviewEmailJobBzr(
+    TestMergeProposalNeedsReviewEmailJobMixin, BzrMixin, TestCaseWithFactory):
+
+    def createProposalWithEmptyBranches(self):
+        target_branch, tree = self.create_branch_and_tree()
+        # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
+        # required to generate the revision-id.
+        with override_environ(BZR_EMAIL='me@xxxxxxxxxxx'):
+            tree.commit('test')
+        source_branch = self.factory.makeProductBranch(
+            product=target_branch.product)
+        self.createBzrBranch(source_branch, tree.branch)
+        return self.makeBranchMergeProposal(
+            merge_source=source_branch, merge_target=target_branch)
+
+    def test_run_sends_email(self):
+        self.useBzrBranches(direct_database=True)
+        parent = super(TestMergeProposalNeedsReviewEmailJobBzr, self)
+        parent.test_run_sends_email()
+
     def test_MergeProposalCreateJob_with_sourcepackage_branch(self):
         """Jobs for merge proposals with sourcepackage branches work."""
+        # XXX cjwatson 2015-06-01: Port this test to Git as well.
         self.useBzrBranches(direct_database=True)
         bmp = self.factory.makeBranchMergeProposal(
             target_branch=self.factory.makePackageBranch())
@@ -184,7 +212,14 @@
         self.factory.makeRevisionsForBranch(bmp.source_branch, count=1)
         job = MergeProposalNeedsReviewEmailJob.create(bmp)
         with dbuser("merge-proposal-jobs"):
-            job.run()
+            JobRunner([job]).runAll()
+
+
+class TestMergeProposalNeedsReviewEmailJobGit(
+    TestMergeProposalNeedsReviewEmailJobMixin, GitMixin, TestCaseWithFactory):
+
+    def createProposalWithEmptyBranches(self):
+        return self.makeBranchMergeProposal()
 
 
 class TestUpdatePreviewDiffJob(DiffTestCase):
@@ -196,7 +231,7 @@
         verifyObject(IUpdatePreviewDiffJobSource, UpdatePreviewDiffJob)
 
     def test_providesInterface(self):
-        """MergeProposalNeedsReviewEmailJob provides expected interfaces."""
+        """UpdatePreviewDiffJob provides expected interfaces."""
         bmp = self.factory.makeBranchMergeProposal()
         job = UpdatePreviewDiffJob.create(bmp)
         verifyObject(IUpdatePreviewDiffJob, job)