Merge lp:~cjwatson/launchpad/branch-unscan-affordances into lp:launchpad

[Colin Watson]

Improve handling of branches with various kinds of partial data.

This tidies up OOPSes on e.g. dogfood, and allows us to unscan branches without having the branches and any associated merge proposals displaying permanent "Updating" UI indications.

I could probably have made this shorter by renaming the other half of the pending_updates/pending_writes split, but I've always found the naming a bit confusing and thought it was clearer this way round (hence also why I renamed the Git analogue to match). I tried making pending_writes be a superset of pending_updates, but the test failures were a bit catastrophic, and users of this really do care about whether the actual scanned data is in sync rather than whether there happens to be a branch scan job in progress.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2016-11-11 12:51:58 +0000
+++ lib/lp/code/browser/branch.py	2017-11-08 11:16:30 +0000
@@ -451,9 +451,9 @@
         return self.context.control_format is None
 
     @property
-    def pending_writes(self):
-        """Whether or not there are pending writes for this branch."""
-        return self.context.pending_writes
+    def pending_updates(self):
+        """Whether or not there are pending updates for this branch."""
+        return self.context.pending_updates
 
     def bzr_download_url(self):
         """Return the generic URL for downloading the branch."""

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2017-05-24 12:04:18 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2017-11-08 11:16:30 +0000
@@ -370,7 +370,7 @@
                 return []
 
     @property
-    def pending_writes(self):
+    def pending_updates(self):
         """Needed to make the branch-revisions metal macro work."""
         return False
 
@@ -532,7 +532,7 @@
             diff = preview_diff.text.decode('utf-8')
         except UnicodeDecodeError:
             diff = preview_diff.text.decode('windows-1252', 'replace')
-        except LibrarianServerError:
+        except (LookupError, LibrarianServerError):
             self._diff_available = False
             diff = ''
         # Strip off the trailing carriage returns.
@@ -736,7 +736,7 @@
     def pending_diff(self):
         return (
             self.context.next_preview_diff_job is not None or
-            self.context.merge_source.pending_writes)
+            self.context.merge_source.pending_updates)
 
     @cachedproperty
     def preview_diff(self):

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2017-10-04 01:16:22 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2017-11-08 11:16:30 +0000
@@ -607,7 +607,7 @@
         logout()
         with StormStatementRecorder() as recorder:
             browser.open(branch_url)
-        self.assertThat(recorder, HasQueryCount(Equals(27)))
+        self.assertThat(recorder, HasQueryCount(Equals(28)))
 
 
 class TestBranchViewPrivateArtifacts(BrowserTestCase):

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2017-10-04 01:16:22 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2017-11-08 11:16:30 +0000
@@ -1395,6 +1395,26 @@
             markup = view()
             self.assertIn('The diff is not available at this time.', markup)
 
+    def test_preview_diff_lookup_error(self):
+        # The preview_diff will recover from a LookupError while getting the
+        # librarian content.  (This can happen e.g. on staging replicas of
+        # the production database.)
+        text = b''.join(chr(x) for x in range(255))
+        diff_bytes = b''.join(unified_diff(b'', text))
+        preview_diff = self.setPreviewDiff(diff_bytes)
+        transaction.commit()
+
+        def fake_open(*args):
+            raise LookupError
+
+        lfa = preview_diff.diff.diff_text
+        with monkey_patch(lfa, open=fake_open):
+            view = create_initialized_view(preview_diff, '+diff')
+            self.assertEqual('', view.preview_diff_text)
+            self.assertFalse(view.diff_available)
+            markup = view()
+            self.assertIn('The diff is not available at this time.', markup)
+
     def setPreviewDiff(self, preview_diff_bytes):
         return PreviewDiff.create(
             self.bmp, preview_diff_bytes, 'a', 'b', None, '')

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2016-11-11 12:51:58 +0000
+++ lib/lp/code/interfaces/branch.py	2017-11-08 11:16:30 +0000
@@ -538,6 +538,12 @@
     pending_writes = Attribute(
         "Whether there is new Bazaar data for this branch.")
 
+    pending_updates = Attribute(
+        "Whether there is an update job of some kind (mirroring or scanning) "
+        "pending for the Bazaar data in this branch.  Note that "
+        "pending_writes may be True and pending_updates False if a branch "
+        "has been unscanned.")
+
     def latest_revisions(quantity=10):
         """A specific number of the latest revisions in that branch."""
 

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2016-12-05 14:46:40 +0000
+++ lib/lp/code/interfaces/gitref.py	2017-11-08 11:16:30 +0000
@@ -358,7 +358,7 @@
                                    eager_load=False):
         """Return BranchMergeProposals dependent on merging this reference."""
 
-    pending_writes = Attribute(
+    pending_updates = Attribute(
         "Whether there are recent changes in this repository that have not "
         "yet been scanned.")
 

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2017-02-10 12:52:07 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2017-11-08 11:16:30 +0000
@@ -537,7 +537,7 @@
     def isRepositoryMergeable(other):
         """Is the other repository mergeable into this one (or vice versa)?"""
 
-    pending_writes = Attribute(
+    pending_updates = Attribute(
         "Whether there are recent changes in this repository that have not "
         "yet been scanned.")
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2016-11-11 14:24:38 +0000
+++ lib/lp/code/model/branch.py	2017-11-08 11:16:30 +0000
@@ -1163,25 +1163,53 @@
         return recipients
 
     @property
-    def pending_writes(self):
-        """See `IBranch`.
+    def _pending_mirror_operations(self):
+        """Does this branch have pending mirror operations?
 
-        A branch has pending writes if it has just been pushed to, if it has
-        been mirrored and not yet scanned or if it is in the middle of being
+        A branch has pending mirror operations if it is an imported branch
+        that has just been pushed to or if it is in the middle of being
         mirrored.
         """
         new_data_pushed = (
              self.branch_type == BranchType.IMPORTED
              and self.next_mirror_time is not None)
-        # XXX 2010-04-22, MichaelHudson: This should really look for a branch
-        # scan job.
-        pulled_but_not_scanned = self.last_mirrored_id != self.last_scanned_id
         pull_in_progress = (
             self.last_mirror_attempt is not None
             and (self.last_mirrored is None
                  or self.last_mirror_attempt > self.last_mirrored))
-        return (
-            new_data_pushed or pulled_but_not_scanned or pull_in_progress)
+        return new_data_pushed or pull_in_progress
+
+    @property
+    def pending_writes(self):
+        """See `IBranch`.
+
+        A branch has pending writes if it has pending mirror operations or
+        if it has been mirrored and not yet scanned.  Use this when you need
+        to know if the branch is in a condition where it is possible to run
+        other jobs on it: for example, a branch that has been unscanned
+        cannot support jobs being run for its related merge proposals.
+        """
+        pulled_but_not_scanned = self.last_mirrored_id != self.last_scanned_id
+        return self._pending_mirror_operations or pulled_but_not_scanned
+
+    @property
+    def pending_updates(self):
+        """See `IBranch`.
+
+        A branch has pending updates if it has pending mirror operations or
+        if it has a pending scan job.  Use this when you need to know if
+        there is work queued, for example when deciding whether to display
+        in-progress UI indicators.
+        """
+        from lp.code.model.branchjob import BranchJob, BranchJobType
+        jobs = Store.of(self).find(
+            BranchJob,
+            BranchJob.branch == self,
+            Job.id == BranchJob.jobID,
+            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
+            BranchJob.job_type == BranchJobType.SCAN_BRANCH)
+        pending_scan_job = not jobs.is_empty()
+        return self._pending_mirror_operations or pending_scan_job
 
     def getScannerData(self):
         """See `IBranch`."""

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2017-08-22 11:33:34 +0000
+++ lib/lp/code/model/gitref.py	2017-11-08 11:16:30 +0000
@@ -282,9 +282,9 @@
             prerequisite_path=self.path, eager_load=eager_load)
 
     @property
-    def pending_writes(self):
+    def pending_updates(self):
         """See `IGitRef`."""
-        return self.repository.pending_writes
+        return self.repository.pending_updates
 
     def _getLog(self, start, limit=None, stop=None, union_repository=None,
                 enable_hosting=None, enable_memcache=None, logger=None):
@@ -715,7 +715,7 @@
     createMergeProposal = _unimplemented
     getMergeProposals = _unimplemented
     getDependentMergeProposals = _unimplemented
-    pending_writes = False
+    pending_updates = False
 
     def getCommits(self, *args, **kwargs):
         """See `IGitRef`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2017-05-04 16:02:40 +0000
+++ lib/lp/code/model/gitrepository.py	2017-11-08 11:16:30 +0000
@@ -949,7 +949,7 @@
         return self.namespace.areRepositoriesMergeable(other.namespace)
 
     @property
-    def pending_writes(self):
+    def pending_updates(self):
         """See `IGitRepository`."""
         from lp.code.model.gitjob import (
             GitJob,

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2017-10-04 01:49:22 +0000
+++ lib/lp/code/model/tests/test_branch.py	2017-11-08 11:16:30 +0000
@@ -130,6 +130,8 @@
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
 from lp.services.features.testing import FeatureFixture
+from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.runner import JobRunner
 from lp.services.job.tests import (
     block_on_job,
     monitor_celery,
@@ -155,6 +157,7 @@
     TestCaseWithFactory,
     WebServiceTestCase,
     )
+from lp.testing.dbuser import dbuser
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import (
     CeleryBranchWriteJobLayer,
@@ -2364,89 +2367,133 @@
         self.assertNamespaceEqual(namespace, branch.namespace)
 
 
-class TestPendingWrites(TestCaseWithFactory):
+class TestPendingWritesAndUpdates(TestCaseWithFactory):
     """Are there changes to this branch not reflected in the database?"""
 
     layer = LaunchpadFunctionalLayer
 
     def test_new_branch_no_writes(self):
-        # New branches have no pending writes.
+        # New branches have no pending writes or pending updates.
         branch = self.factory.makeAnyBranch()
-        self.assertEqual(False, branch.pending_writes)
+        self.assertFalse(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
 
     def test_branchChanged_for_hosted(self):
         # If branchChanged has been called with a new tip revision id, there
-        # are pending writes.
+        # are pending writes and pending updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.HOSTED)
         with person_logged_in(branch.owner):
             branch.branchChanged('', 'new-tip', None, None, None)
-        self.assertEqual(True, branch.pending_writes)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
+
+    def test_unscanned_without_rescan(self):
+        # If a branch was unscanned without requesting a rescan, then there
+        # are pending writes but no pending updates.
+        self.useBzrBranches(direct_database=True)
+        branch, bzr_tree = self.create_branch_and_tree()
+        rev_id = self.factory.getUniqueString(b'rev-id')
+        bzr_tree.commit('Commit', committer='[e-mail]', rev_id=rev_id)
+        removeSecurityProxy(branch).branchChanged('', rev_id, None, None, None)
+        transaction.commit()
+        [job] = getUtility(IBranchScanJobSource).iterReady()
+        with dbuser('branchscanner'):
+            JobRunner([job]).runAll()
+        self.assertFalse(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
+        removeSecurityProxy(branch).unscan(rescan=False)
+        self.assertTrue(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
+
+    def test_unscanned_with_rescan(self):
+        # If a branch was unscanned and a rescan was requested, then there
+        # are pending writes and pending updates.
+        self.useBzrBranches(direct_database=True)
+        branch, bzr_tree = self.create_branch_and_tree()
+        rev_id = self.factory.getUniqueString(b'rev-id')
+        bzr_tree.commit('Commit', committer='[e-mail]', rev_id=rev_id)
+        removeSecurityProxy(branch).branchChanged('', rev_id, None, None, None)
+        transaction.commit()
+        [job] = getUtility(IBranchScanJobSource).iterReady()
+        with dbuser('branchscanner'):
+            JobRunner([job]).runAll()
+        self.assertFalse(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
+        removeSecurityProxy(branch).unscan(rescan=True)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
 
     def test_requestMirror_for_imported(self):
         # If an imported branch has a requested mirror, then we've just
-        # imported new changes. Therefore, pending writes.
+        # imported new changes. Therefore, pending writes and pending
+        # updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.IMPORTED)
         branch.requestMirror()
-        self.assertEqual(True, branch.pending_writes)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
 
     def test_requestMirror_for_mirrored(self):
-        # Mirrored branches *always* have a requested mirror. The fact that a
-        # mirror is requested has no bearing on whether there are pending
-        # writes. Thus, pending_writes is False.
+        # Mirrored branches *always* have a requested mirror. The fact that
+        # a mirror is requested has no bearing on whether there are pending
+        # writes or pending updates. Thus, pending_writes and
+        # pending_updates are both False.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.requestMirror()
-        self.assertEqual(False, branch.pending_writes)
+        self.assertFalse(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
 
     def test_pulled_but_not_scanned(self):
         # If a branch has been pulled (mirrored) but not scanned, then we have
         # yet to load the revisions into the database. This means there are
-        # pending writes.
+        # pending writes and pending updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(
             '', rev_id, None, None, None)
-        self.assertEqual(True, branch.pending_writes)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
 
     def test_pulled_and_scanned(self):
         # If a branch has been pulled and scanned, then there are no pending
-        # writes.
+        # writes or pending updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(
             '', rev_id, None, None, None)
-        # Cheat! The actual API for marking a branch as scanned is
-        # updateScannedDetails. That requires a revision in the database
+        # Cheat! The actual API for marking a branch as scanned is to run
+        # the BranchScanJob. That requires a revision in the database
         # though.
         removeSecurityProxy(branch).last_scanned_id = rev_id
-        self.assertEqual(False, branch.pending_writes)
+        [job] = getUtility(IBranchScanJobSource).iterReady()
+        removeSecurityProxy(job).job._status = JobStatus.COMPLETED
+        self.assertFalse(branch.pending_writes)
+        self.assertFalse(branch.pending_updates)
 
     def test_first_mirror_started(self):
         # If we have started mirroring the branch for the first time, then
-        # there are probably pending writes.
+        # there are probably pending writes and pending updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
-        self.assertEqual(True, branch.pending_writes)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
 
     def test_following_mirror_started(self):
         # If we have started mirroring the branch, then there are probably
-        # pending writes.
+        # pending writes and pending updates.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(
             '', rev_id, None, None, None)
-        # Cheat! The actual API for marking a branch as scanned is
-        # updateScannedDetails. That requires a revision in the database
-        # though.
-        removeSecurityProxy(branch).last_scanned_id = rev_id
-        # Cheat again! We can only tell if mirroring has started if the last
+        # Cheat! We can only tell if mirroring has started if the last
         # mirrored attempt is different from the last mirrored time. To ensure
         # this, we start the second mirror in a new transaction.
         transaction.commit()
         branch.startMirroring()
-        self.assertEqual(True, branch.pending_writes)
+        self.assertTrue(branch.pending_writes)
+        self.assertTrue(branch.pending_updates)
 
 
 class TestBranchPrivacy(TestCaseWithFactory):

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2017-10-04 01:29:35 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2017-11-08 11:16:30 +0000
@@ -960,28 +960,28 @@
         self.assertEqual(namespace, repository.namespace)
 
 
-class TestGitRepositoryPendingWrites(TestCaseWithFactory):
+class TestGitRepositoryPendingUpdates(TestCaseWithFactory):
     """Are there changes to this repository not reflected in the database?"""
 
     layer = LaunchpadFunctionalLayer
 
-    def test_new_repository_no_writes(self):
-        # New repositories have no pending writes.
+    def test_new_repository_no_updates(self):
+        # New repositories have no pending updates.
         repository = self.factory.makeGitRepository()
-        self.assertFalse(repository.pending_writes)
+        self.assertFalse(repository.pending_updates)
 
     def test_notify(self):
         # If the hosting service has just sent us a change notification,
-        # then there are pending writes, but running the ref-scanning job
+        # then there are pending updates, but running the ref-scanning job
         # clears that flag.
         git_api = GitAPI(None, None)
         repository = self.factory.makeGitRepository()
         self.assertIsNone(git_api.notify(repository.getInternalPath()))
-        self.assertTrue(repository.pending_writes)
+        self.assertTrue(repository.pending_updates)
         [job] = list(getUtility(IGitRefScanJobSource).iterReady())
         with dbuser("branchscanner"):
             JobRunner([job]).runAll()
-        self.assertFalse(repository.pending_writes)
+        self.assertFalse(repository.pending_updates)
 
 
 class TestGitRepositoryPrivacy(TestCaseWithFactory):

=== modified file 'lib/lp/code/templates/branch-index.pt'
--- lib/lp/code/templates/branch-index.pt	2016-10-13 12:43:14 +0000
+++ lib/lp/code/templates/branch-index.pt	2017-11-08 11:16:30 +0000
@@ -130,9 +130,9 @@
 
   </div>
 
-  <div class="yui-g" tal:condition="view/pending_writes">
+  <div class="yui-g" tal:condition="view/pending_updates">
     <div class="portlet">
-      <div id="branch-pending-writes" class="pending-update">
+      <div id="branch-pending-updates" class="pending-update">
         <h3>Updating branch...</h3>
         <p>
           Launchpad is processing new changes to this branch which will be

=== modified file 'lib/lp/code/templates/gitrepository-index.pt'
--- lib/lp/code/templates/gitrepository-index.pt	2016-10-13 12:43:14 +0000
+++ lib/lp/code/templates/gitrepository-index.pt	2017-11-08 11:16:30 +0000
@@ -72,9 +72,9 @@
     </div>
   </div>
 
-  <div class="yui-g" tal:condition="context/pending_writes">
+  <div class="yui-g" tal:condition="context/pending_updates">
     <div class="portlet">
-      <div id="repository-pending-writes" class="pending-update">
+      <div id="repository-pending-updates" class="pending-update">
         <h3>Updating repository...</h3>
         <p>
           Launchpad is processing new changes to this repository which will

=== modified file 'lib/lp/codehosting/tests/test_branchdistro.py'
--- lib/lp/codehosting/tests/test_branchdistro.py	2012-02-15 17:29:54 +0000
+++ lib/lp/codehosting/tests/test_branchdistro.py	2017-11-08 11:16:30 +0000
@@ -273,12 +273,12 @@
             new_branch).getScannerData()
         self.assertEqual(old_ancestry, new_ancestry)
         self.assertEqual(old_history, new_history)
-        self.assertFalse(new_branch.pending_writes)
         self.assertIs(None, new_branch.stacked_on)
         self.assertEqual(new_branch, db_branch.stacked_on)
         # The script doesn't have permission to create branch jobs, but just
         # to be insanely paranoid.
         switch_dbuser('launchpad')
+        self.assertFalse(new_branch.pending_writes)
         scan_jobs = list(getUtility(IBranchScanJobSource).iterReady())
         self.assertEqual(existing_scan_job_count, len(scan_jobs))