launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/mp-job-delete-bug-link into lp:launchpad.

Commit message:
Fix crash when unlinking a bug from a Git-based MP in UpdatePreviewDiffJob.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/mp-job-delete-bug-link/+merge/324323

https://oops.canonical.com/?oopsid=OOPS-7b67d0c0ffaca3e59d2f030b69c4e36b
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/mp-job-delete-bug-link into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2017-04-08 16:58:52 +0000
+++ database/schema/security.cfg	2017-05-19 16:07:20 +0000
@@ -2018,7 +2018,7 @@
 public.validpersoncache                 = SELECT
 public.webhook                          = SELECT
 public.webhookjob                       = SELECT, INSERT
-public.xref                             = SELECT, INSERT
+public.xref                             = SELECT, INSERT, DELETE
 type=user
 
 [sharing-jobs]

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2016-09-09 12:37:25 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2017-05-19 16:07:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database class for branch merge proposals."""
@@ -500,19 +500,21 @@
         remove_bugs = load(Bug, set(
             bug_id for bug_id in current_bug_ids - new_bug_ids
             if current_bug_ids_from_source[bug_id]))
-        for bug in remove_bugs:
-            self.unlinkBug(bug, check_permissions=False)
         add_bugs = load(Bug, new_bug_ids - current_bug_ids)
-        # XXX cjwatson 2016-06-11: We could perhaps set creator and
-        # date_created based on commit information, but then we'd have to
-        # work out what to do in the case of multiple commits referring to
-        # the same bug, updating properties if more such commits arrive
-        # later, etc.  This is simple and does the job for now.
-        for bug in add_bugs:
-            self.linkBug(
-                bug, user=getUtility(ILaunchpadCelebrities).janitor,
-                check_permissions=False,
-                props={'metadata': {'from_source': True}})
+        if remove_bugs or add_bugs:
+            janitor = getUtility(ILaunchpadCelebrities).janitor
+            for bug in remove_bugs:
+                self.unlinkBug(bug, user=janitor, check_permissions=False)
+            # XXX cjwatson 2016-06-11: We could perhaps set creator and
+            # date_created based on commit information, but then we'd have
+            # to work out what to do in the case of multiple commits
+            # referring to the same bug, updating properties if more such
+            # commits arrive later, etc.  This is simple and does the job
+            # for now.
+            for bug in add_bugs:
+                self.linkBug(
+                    bug, user=janitor, check_permissions=False,
+                    props={'metadata': {'from_source': True}})
 
     @property
     def address(self):

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2016-11-09 17:18:21 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2017-05-19 16:07:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BranchMergeProposals."""
@@ -80,7 +80,6 @@
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
-from lp.services.memcache.testing import MemcacheFixture
 from lp.services.webapp import canonical_url
 from lp.services.xref.interfaces import IXRefSet
 from lp.testing import (
@@ -1486,9 +1485,7 @@
 
     def setUp(self):
         super(TestBranchMergeProposalBugsGit, self).setUp()
-        self.hosting_fixture = self.useFixture(GitHostingFixture(
-            disable_memcache=False))
-        self.memcache_fixture = self.useFixture(MemcacheFixture())
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
 
     def _makeBranchMergeProposal(self):
         return self.factory.makeBranchMergeProposalForGit()
@@ -1537,7 +1534,7 @@
                 u"message": u"LP: #%d" % bug.id,
                 }
             for i, bug in enumerate(bugs)]
-        self.memcache_fixture.clear()
+        self.hosting_fixture.memcache_fixture.clear()
 
     def test_updateRelatedBugsFromSource_no_links(self):
         # updateRelatedBugsFromSource does nothing if there are no related

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2016-10-02 00:33:51 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2017-05-19 16:07:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 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."""
@@ -284,6 +284,15 @@
         self.assertEqual([bug], bmp.bugs)
         self.assertEqual([bmp], bug.linked_merge_proposals)
         self.assertEqual(patch, bmp.preview_diff.text)
+        # If somebody rewrites history to remove the bug reference, then the
+        # bug link is removed from the merge proposal.
+        self.hosting_fixture.getLog.result = []
+        self.hosting_fixture.memcache_fixture.clear()
+        job = UpdatePreviewDiffJob.create(bmp)
+        with dbuser("merge-proposal-jobs"):
+            JobRunner([job]).runAll()
+        self.assertEqual([], bmp.bugs)
+        self.assertEqual([], bug.linked_merge_proposals)
 
     def test_run_object_events(self):
         # While the job runs a single IObjectModifiedEvent is issued when the

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2016-10-11 14:26:31 +0000
+++ lib/lp/code/tests/helpers.py	2017-05-19 16:07:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions for code testing live here."""
@@ -331,4 +331,4 @@
             # makes it awkward to repeat the same call with different log
             # responses.  For convenience, we make it easy to disable that
             # here.
-            self.useFixture(MemcacheFixture())
+            self.memcache_fixture = self.useFixture(MemcacheFixture())