launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mp-pending-diff into lp:launchpad.

Commit message:
Make the pending diff indicator work for Git-based merge proposals.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mp-pending-diff/+merge/259029

Make the pending diff indicator work for Git-based merge proposals.

GitRepository.pending_write is rather simpler than the Branch analogue; it just checks whether there's a pending ref-scanning job.  The pending diff indicator may over-report, because notify doesn't tell us which refs have changed, but ref scans are pretty quick so it shouldn't be for very long.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mp-pending-diff into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2015-04-29 12:39:23 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2015-05-13 16:59:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Views, navigation and actions for BranchMergeProposals."""
@@ -697,11 +697,9 @@
 
     @property
     def pending_diff(self):
-        if self.context.source_branch is None:
-            return False
         return (
             self.context.next_preview_diff_job is not None or
-            self.context.source_branch.pending_writes)
+            self.context.merge_source.pending_writes)
 
     @cachedproperty
     def preview_diff(self):

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-05-05 12:27:34 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-05-13 16:59:36 +0000
@@ -53,6 +53,7 @@
     add_revision_to_branch,
     make_merge_proposal_without_reviewers,
     )
+from lp.code.xmlrpc.git import GitAPI
 from lp.registry.enums import (
     PersonVisibility,
     TeamMembershipPolicy,
@@ -1279,6 +1280,18 @@
             bmp.source_branch.branchChanged(None, 'rev-1', None, None, None)
         self.assertTrue(view.pending_diff)
 
+    def test_pending_diff_with_pending_git_repository(self):
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+        bmp = self.factory.makeBranchMergeProposalForGit()
+        bmp.next_preview_diff_job.start()
+        bmp.next_preview_diff_job.fail()
+        view = create_initialized_view(bmp, '+index')
+        self.assertFalse(view.pending_diff)
+        git_api = GitAPI(None, None)
+        self.assertIsNone(
+            git_api.notify(bmp.source_git_repository.getInternalPath()))
+        self.assertTrue(view.pending_diff)
+
     def test_subscribe_to_merge_proposal_events_flag_disabled(self):
         # If the longpoll.merge_proposals.enabled flag is not enabled the user
         # is *not* subscribed to events relating to the merge proposal.

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2015-05-12 17:20:22 +0000
+++ lib/lp/code/interfaces/gitref.py	2015-05-13 16:59:36 +0000
@@ -277,6 +277,10 @@
                           merged_revision_ids=None, eager_load=False):
         """Return matching BranchMergeProposals."""
 
+    pending_writes = Attribute(
+        "Whether there are recent changes in this repository that have not "
+        "yet been scanned.")
+
 
 class IGitRefBatchNavigator(ITableBatchNavigator):
     pass

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2015-05-07 16:49:41 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2015-05-13 16:59:36 +0000
@@ -456,6 +456,10 @@
     def isRepositoryMergeable(other):
         """Is the other repository mergeable into this one (or vice versa)?"""
 
+    pending_writes = Attribute(
+        "Whether there are recent changes in this repository that have not "
+        "yet been scanned.")
+
 
 class IGitRepositoryModerateAttributes(Interface):
     """IGitRepository attributes that can be edited by more than one community.

=== modified file 'lib/lp/code/model/gitjob.py'
--- lib/lp/code/model/gitjob.py	2015-05-13 15:32:18 +0000
+++ lib/lp/code/model/gitjob.py	2015-05-13 16:59:36 +0000
@@ -5,6 +5,7 @@
 
 __all__ = [
     'GitJob',
+    'GitJobType',
     'GitRefScanJob',
     ]
 

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2015-04-28 16:39:15 +0000
+++ lib/lp/code/model/gitref.py	2015-05-13 16:59:36 +0000
@@ -198,6 +198,11 @@
             status, target_repository=self.repository, target_path=self.path,
             merged_revision_ids=merged_revision_ids, eager_load=eager_load)
 
+    @property
+    def pending_writes(self):
+        """See `IGitRef`."""
+        return self.repository.pending_writes
+
 
 class GitRef(StormBase, GitRefMixin):
     """See `IGitRef`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-05-07 16:49:41 +0000
+++ lib/lp/code/model/gitrepository.py	2015-05-13 16:59:36 +0000
@@ -121,6 +121,8 @@
     Values,
     )
 from lp.services.features import getFeatureFlag
+from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.model.job import Job
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
 from lp.services.propertycache import (
     cachedproperty,
@@ -763,6 +765,21 @@
         """See `IGitRepository`."""
         return self.namespace.areRepositoriesMergeable(other.namespace)
 
+    @property
+    def pending_writes(self):
+        """See `IGitRepository`."""
+        from lp.code.model.gitjob import (
+            GitJob,
+            GitJobType,
+            )
+        jobs = Store.of(self).find(
+            GitJob,
+            GitJob.repository == self,
+            GitJob.job_type == GitJobType.REF_SCAN,
+            GitJob.job == Job.id,
+            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+        return not jobs.is_empty()
+
     def destroySelf(self):
         raise NotImplementedError
 

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-05-07 15:03:27 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-05-13 16:59:36 +0000
@@ -47,6 +47,7 @@
     GitTargetError,
     )
 from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
+from lp.code.interfaces.gitjob import IGitRefScanJobSource
 from lp.code.interfaces.gitnamespace import (
     IGitNamespacePolicy,
     IGitNamespaceSet,
@@ -58,6 +59,7 @@
     )
 from lp.code.interfaces.revision import IRevisionSet
 from lp.code.model.gitrepository import GitRepository
+from lp.code.xmlrpc.git import GitAPI
 from lp.registry.enums import (
     BranchSharingPolicy,
     PersonVisibility,
@@ -76,6 +78,7 @@
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
+from lp.services.job.runner import JobRunner
 from lp.services.mail import stub
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import OAuthPermission
@@ -88,9 +91,11 @@
     TestCaseWithFactory,
     verifyObject,
     )
+from lp.testing.dbuser import dbuser
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
     ZopelessDatabaseLayer,
     )
 from lp.testing.pages import webservice_for_person
@@ -442,6 +447,34 @@
         self.assertEqual(namespace, repository.namespace)
 
 
+class TestGitRepositoryPendingWrites(TestCaseWithFactory):
+    """Are there changes to this repository not reflected in the database?"""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositoryPendingWrites, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+    def test_new_repository_no_writes(self):
+        # New repositories have no pending writes.
+        repository = self.factory.makeGitRepository()
+        self.assertFalse(repository.pending_writes)
+
+    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
+        # clears that flag.
+        git_api = GitAPI(None, None)
+        repository = self.factory.makeGitRepository()
+        self.assertIsNone(git_api.notify(repository.getInternalPath()))
+        self.assertTrue(repository.pending_writes)
+        [job] = list(getUtility(IGitRefScanJobSource).iterReady())
+        with dbuser("branchscanner"):
+            JobRunner([job]).runAll()
+        self.assertFalse(repository.pending_writes)
+
+
 class TestGitRepositoryPrivacy(TestCaseWithFactory):
     """Tests for Git repository privacy."""