launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21989
[Merge] lp:~cjwatson/launchpad/branch-unscan-affordances into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/branch-unscan-affordances into lp:launchpad.
Commit message:
Improve handling of branches with various kinds of partial data.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/branch-unscan-affordances/+merge/333251
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/branch-unscan-affordances into lp:launchpad.
=== 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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +0000
@@ -538,6 +538,10 @@
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.")
+
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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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='me@xxxxxxxxxxx', 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='me@xxxxxxxxxxx', 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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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-06 09:48:10 +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))
Follow ups
