launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-async-update-related-bugs into lp:launchpad.

Commit message:
Move updating of related bugs to UpdatePreviewDiffJob.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-async-update-related-bugs/+merge/305017

Move updating of related bugs to UpdatePreviewDiffJob.

This is roughly as suggested in https://code.launchpad.net/~cjwatson/launchpad/fix-git-update-related-bugs/+merge/304941.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-async-update-related-bugs into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2016-07-29 12:14:39 +0000
+++ database/schema/security.cfg	2016-09-06 15:36:22 +0000
@@ -1979,11 +1979,17 @@
 public.branchmergeproposal              = SELECT, INSERT, UPDATE
 public.branchmergeproposaljob           = SELECT, INSERT
 public.branchsubscription               = SELECT
-public.bug                              = SELECT
+public.bug                              = SELECT, UPDATE
+public.bugactivity                      = SELECT, INSERT
 public.bugbranch                        = SELECT
+public.bugmute                          = SELECT
+public.bugnotification                  = SELECT, INSERT
+public.bugnotificationfilter            = SELECT, INSERT
+public.bugnotificationrecipient         = SELECT, INSERT
 public.bugsubscription                  = SELECT
 public.bugtask                          = SELECT
 public.bugtaskflat                      = SELECT
+public.bugwatch                         = SELECT
 public.codereviewinlinecomment          = SELECT, INSERT
 public.codereviewmessage                = SELECT, INSERT
 public.codereviewvote                   = SELECT, INSERT
@@ -2008,14 +2014,16 @@
 public.product                          = SELECT
 public.productseries                    = SELECT
 public.revision                         = SELECT
+public.revisionauthor                   = SELECT, INSERT
 public.seriessourcepackagebranch        = SELECT
 public.sourcepackagename                = SELECT
+public.structuralsubscription           = SELECT
 public.teammembership                   = SELECT
 public.teamparticipation                = SELECT
 public.validpersoncache                 = SELECT
 public.webhook                          = SELECT
 public.webhookjob                       = SELECT, INSERT
-public.xref                             = SELECT
+public.xref                             = SELECT, INSERT
 type=user
 
 [sharing-jobs]

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2016-09-02 18:21:07 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2016-09-06 15:36:22 +0000
@@ -511,8 +511,7 @@
     def updateLandingTargets(paths):
         """Update landing targets (MPs where this repository is the source).
 
-        For each merge proposal, update linked bugs and create
-        `UpdatePreviewDiffJob`s.
+        For each merge proposal, create `UpdatePreviewDiffJob`s.
 
         :param paths: A list of reference paths.  Any merge proposals whose
             source is this repository and one of these paths will have their

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2015-10-05 17:02:20 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2016-09-06 15:36:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to BranchMergeProposals are in here.
@@ -369,6 +369,8 @@
             # A no-op context manager.  (This could be simplified with
             # contextlib.ExitStack from Python 3.3.)
             server_context = contextmanager(lambda: (None for _ in [None]))()
+            # Update related bug links based on commits in the source branch.
+            self.branch_merge_proposal.updateRelatedBugsFromSource()
         with server_context:
             with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
                 PreviewDiff.fromBranchMergeProposal(self.branch_merge_proposal)

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-09-02 18:21:07 +0000
+++ lib/lp/code/model/gitrepository.py	2016-09-06 15:36:22 +0000
@@ -972,7 +972,6 @@
         from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
         jobs = []
         for merge_proposal in self.getActiveLandingTargets(paths):
-            merge_proposal.updateRelatedBugsFromSource()
             jobs.append(UpdatePreviewDiffJob.create(merge_proposal))
         return jobs
 

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2015-10-27 23:35:50 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2016-09-06 15:36:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for branch merge proposal jobs."""
@@ -9,6 +9,7 @@
     datetime,
     timedelta,
     )
+import hashlib
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.interfaces import IObjectModifiedEvent
@@ -75,6 +76,7 @@
     verifyObject,
     )
 from lp.testing.dbuser import dbuser
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     CeleryBzrsyncdJobLayer,
     CeleryJobLayer,
@@ -256,9 +258,27 @@
 
     def test_run_git(self):
         bmp, _, _, patch = self.createExampleGitMerge()
-        job = UpdatePreviewDiffJob.create(bmp)
-        with dbuser("merge-proposal-jobs"):
-            JobRunner([job]).runAll()
+        self.hosting_client.getLog = FakeMethod(result=[])
+        job = UpdatePreviewDiffJob.create(bmp)
+        with dbuser("merge-proposal-jobs"):
+            JobRunner([job]).runAll()
+        self.assertEqual(patch, bmp.preview_diff.text)
+
+    def test_run_git_updates_related_bugs(self):
+        # The merge proposal has its related bugs updated.
+        bug = self.factory.makeBug()
+        bmp, _, _, patch = self.createExampleGitMerge()
+        self.hosting_client.getLog = FakeMethod(result=[
+            {
+                u"sha1": unicode(hashlib.sha1("tip").hexdigest()),
+                u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
+                },
+            ])
+        job = UpdatePreviewDiffJob.create(bmp)
+        with dbuser("merge-proposal-jobs"):
+            JobRunner([job]).runAll()
+        self.assertEqual([bug], bmp.bugs)
+        self.assertEqual([bmp], bug.linked_merge_proposals)
         self.assertEqual(patch, bmp.preview_diff.text)
 
     def test_run_object_events(self):
@@ -363,6 +383,7 @@
         self.useFixture(FeatureFixture(
             {BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
         bmp = self.createExampleGitMerge()[0]
+        self.hosting_client.getLog = FakeMethod(result=[])
         hook = self.factory.makeWebhook(
             target=bmp.target_git_repository,
             event_types=["merge-proposal:0.1"])

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2016-07-01 20:07:09 +0000
+++ lib/lp/code/model/tests/test_diff.py	2016-09-06 15:36:22 +0000
@@ -148,10 +148,11 @@
                 prerequisite)
 
     def installFakeGitMergeDiff(self, result=None, failure=None):
-        hosting_client = FakeGitHostingClient()
-        hosting_client.getMergeDiff = FakeMethod(
+        self.hosting_client = FakeGitHostingClient()
+        self.hosting_client.getMergeDiff = FakeMethod(
             result=result, failure=failure)
-        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+        self.useFixture(
+            ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
 
     def createExampleGitMerge(self):
         """Create an example Git-based merge scenario.

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2016-09-02 14:36:28 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2016-09-06 15:36:22 +0000
@@ -274,7 +274,7 @@
 class TestGitRefCreateMergeProposal(TestCaseWithFactory):
     """Exercise all the code paths for creating a merge proposal."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestGitRefCreateMergeProposal, self).setUp()
@@ -288,11 +288,6 @@
                 name=u"target", owner=self.user, target=self.project)
             [self.prerequisite] = self.factory.makeGitRefs(
                 name=u"prerequisite", owner=self.user, target=self.project)
-        self.log = []
-        self.hosting_client = FakeMethod()
-        self.hosting_client.getLog = FakeMethod(result=self.log)
-        self.useFixture(
-            ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
 
     def assertOnePendingReview(self, proposal, reviewer, review_type=None):
         # There should be one pending vote for the reviewer with the specified
@@ -433,17 +428,6 @@
         votes = {(vote.reviewer, vote.review_type) for vote in bmp.votes}
         self.assertEqual({(person1, "review1"), (person2, "review2")}, votes)
 
-    def test_updates_related_bugs(self):
-        """The merge proposal has its related bugs updated."""
-        bug = self.factory.makeBug()
-        self.log.append({
-            u"sha1": unicode(hashlib.sha1("tip").hexdigest()),
-            u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
-            })
-        proposal = self.source.addLandingTarget(self.user, self.target)
-        self.assertEqual([bug], proposal.bugs)
-        self.assertEqual([proposal], bug.linked_merge_proposals)
-
 
 class TestGitRefWebservice(TestCaseWithFactory):
     """Tests for the webservice."""

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-09-02 18:21:07 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-09-06 15:36:22 +0000
@@ -986,7 +986,7 @@
 class TestGitRepositoryRefs(TestCaseWithFactory):
     """Tests for ref handling."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def test__convertRefInfo(self):
         # _convertRefInfo converts a valid info dictionary.
@@ -1107,9 +1107,6 @@
         return [UpdatePreviewDiffJob(job) for job in jobs]
 
     def test_update_schedules_diff_update(self):
-        hosting_client = FakeMethod()
-        hosting_client.getLog = FakeMethod(result=[])
-        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
         repository = self.factory.makeGitRepository()
         [ref] = self.factory.makeGitRefs(repository=repository)
         self.assertRefsMatch(repository.refs, repository, [ref.path])
@@ -1821,31 +1818,10 @@
 
 class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
 
-    layer = LaunchpadFunctionalLayer
-
-    def test_updates_related_bugs(self):
-        """All non-final merge proposals have their related bugs updated."""
-        bug = self.factory.makeBug()
-        bmp = self.factory.makeBranchMergeProposalForGit()
-        self.assertEqual([], bmp.bugs)
-        self.assertEqual([], bug.linked_merge_proposals)
-        hosting_client = FakeMethod()
-        hosting_client.getLog = FakeMethod(result=[
-            {
-                u"sha1": bmp.source_git_commit_sha1,
-                u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
-                },
-            ])
-        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
-        bmp.source_git_repository.updateLandingTargets([bmp.source_git_path])
-        self.assertEqual([bug], bmp.bugs)
-        self.assertEqual([bmp], bug.linked_merge_proposals)
+    layer = DatabaseFunctionalLayer
 
     def test_schedules_diff_updates(self):
         """Create jobs for all merge proposals."""
-        hosting_client = FakeMethod()
-        hosting_client.getLog = FakeMethod(result=[])
-        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
         bmp1 = self.factory.makeBranchMergeProposalForGit()
         bmp2 = self.factory.makeBranchMergeProposalForGit(
             source_ref=bmp1.source_git_ref)

=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py	2016-09-02 14:36:28 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py	2016-09-06 15:36:22 +0000
@@ -63,10 +63,8 @@
 def merge_proposal_created(merge_proposal, event):
     """A new merge proposal has been created.
 
-    Update related bug links based on commits in the source branch; create a
-    job to update the diff for the merge proposal; trigger webhooks.
+    Create a job to update the diff for the merge proposal; trigger webhooks.
     """
-    merge_proposal.updateRelatedBugsFromSource()
     getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
     if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
         payload = {