Merge lp:~mwhudson/launchpad/no-hosted-area-branchChanged-on-branch into lp:launchpad

[Michael Hudson-Doyle]

Hi,

This branch moves the guts of the branchChanged method from the endpoint implementation to a method on the branch itself.

Cheers,
mwh

[Tim Penhey (thumper)]

review: Approve

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-04-27 02:17:46 +0000
+++ lib/lp/code/configure.zcml	2010-04-27 02:18:06 +0000
@@ -548,7 +548,7 @@
     <require
         permission="launchpad.Edit"
         attributes="destroySelf destroySelfBreakReferences setPrivate
-                    setOwner setTarget requestUpgrade"
+                    setOwner setTarget requestUpgrade branchChanged"
         set_attributes="name url mirror_status_message
                         description lifecycle_status
                         last_mirrored last_mirrored_id last_mirror_attempt
 
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/interfaces/branch.py	2010-04-27 02:18:06 +0000
@@ -1088,6 +1088,23 @@
     def startMirroring():
         """Signal that this branch is being mirrored."""
 
+    def branchChanged(stacked_on_url, last_revision_id, control_format,
+                      branch_format, repository_format):
+        """Record that a branch has been changed.
+
+        This method records the stacked on branch tip revision id and format
+        or the branch and creates a scan job if the tip revision id has
+        changed.
+
+        :param stacked_on_url: The unique name of the branch this branch is
+            stacked on, or '' if this branch is not stacked.
+        :param last_revision_id: The tip revision ID of the branch.
+        :param control_format: The entry from ControlFormat for the branch.
+        :param branch_format: The entry from BranchFormat for the branch.
+        :param repository_format: The entry from RepositoryFormat for the
+            branch.
+        """
+
     def mirrorComplete(last_revision_id):
         """Signal that a mirror attempt has completed successfully.
 
 
=== modified file 'lib/lp/code/interfaces/codehosting.py'
--- lib/lp/code/interfaces/codehosting.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/interfaces/codehosting.py	2010-04-27 02:18:06 +0000
@@ -166,16 +166,22 @@
         :param branchID: a branch ID.
         """
 
-    def branchChanged(branch_id, stacked_on_url, last_revision_id):
+    def branchChanged(login_id, branch_id, stacked_on_url, last_revision_id,
+                      control_string, branch_string, repository_string):
         """Record that a branch has been changed.
 
-        This method records the stacked on branch and tip revision id of the
-        branch and creates a scan job if the tip revision id has changed.
+        See `IBranch.branchChanged`.
 
-        :param branchID: The database id of the branch to operate on.
+        :param login_id: the person ID of the user changing the branch.
+        :param branch_id: The database id of the branch to operate on.
         :param stacked_on_url: The unique name of the branch this branch is
             stacked on, or '' if this branch is not stacked.
         :param last_revision_id: The tip revision ID of the branch.
+        :param control_string: The format string of the control directory of
+            the branch.
+        :param branch_string: The format string of the branch.
+        :param repository_string: The format string of the branch's
+            repository.
         """
 
     def translatePath(requester_id, path):
 
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/model/branch.py	2010-04-27 02:18:06 +0000
@@ -899,6 +899,35 @@
         self.last_mirror_attempt = UTC_NOW
         self.next_mirror_time = None
 
+    def branchChanged(self, stacked_on_location, last_revision_id,
+                      control_format, branch_format, repository_format):
+        """See `IBranch`."""
+        self.mirror_status_message = None
+        if stacked_on_location == '':
+            stacked_on_branch = None
+        else:
+            stacked_on_branch = getUtility(IBranchLookup).getByUniqueName(
+                stacked_on_location.strip('/'))
+            if stacked_on_branch is None:
+                self.mirror_status_message = (
+                    'Invalid stacked on location: ' + stacked_on_location)
+        self.stacked_on = stacked_on_branch
+        self.last_mirrored = UTC_NOW
+        self.mirror_failures = 0
+        if (self.next_mirror_time is None
+            and self.branch_type == BranchType.MIRRORED):
+            # No mirror was requested since we started mirroring.
+            increment = getUtility(IBranchPuller).MIRROR_TIME_INCREMENT
+            self.next_mirror_time = (
+                datetime.datetime.now(pytz.timezone('UTC')) + increment)
+        if self.last_mirrored_id != last_revision_id:
+            self.last_mirrored_id = last_revision_id
+            from lp.code.model.branchjob import BranchScanJob
+            BranchScanJob.create(self)
+        self.control_format = control_format
+        self.branch_format = branch_format
+        self.repository_format = repository_format
+
     def mirrorComplete(self, last_revision_id):
         """See `IBranch`."""
         if self.branch_type == BranchType.REMOTE:
 
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-04-27 02:18:06 +0000
@@ -38,7 +38,7 @@
     SpecificationBranch)
 from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
 from lp.bugs.model.bugbranch import BugBranch
-from lp.code.bzr import BranchFormat, RepositoryFormat
+from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import (
     BranchLifecycleStatus, BranchSubscriptionNotificationLevel, BranchType,
     BranchVisibilityRule, CodeReviewNotificationLevel)
@@ -47,7 +47,8 @@
     BranchCannotBePrivate, BranchCannotBePublic,
     BranchCreatorNotMemberOfOwnerTeam, BranchCreatorNotOwner,
     BranchTargetError, CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING)
-from lp.code.interfaces.branchjob import IBranchUpgradeJobSource
+from lp.code.interfaces.branchjob import (
+    IBranchUpgradeJobSource, IBranchScanJobSource)
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
 from lp.code.interfaces.branchmergeproposal import (
@@ -94,6 +95,110 @@
         self.assertEqual(None, branch.code_import)
 
 
+class TestBranchChanged(TestCaseWithFactory):
+    """Tests for `IBranch.branchChanged`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.arbitrary_formats = (
+            ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_6,
+            RepositoryFormat.BZR_CHK_2A)
+
+    def test_branchChanged_sets_last_mirrored_id(self):
+        # branchChanged sets the last_mirrored_id attribute on the branch.
+        revid = self.factory.getUniqueString()
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged('', revid, *self.arbitrary_formats)
+        self.assertEqual(revid, branch.last_mirrored_id)
+
+    def test_branchChanged_sets_stacked_on(self):
+        # branchChanged sets the stacked_on attribute based on the unique_name
+        # passed in.
+        branch = self.factory.makeAnyBranch()
+        stacked_on = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged(
+            stacked_on.unique_name, '', *self.arbitrary_formats)
+        self.assertEqual(stacked_on, branch.stacked_on)
+
+    def test_branchChanged_unsets_stacked_on(self):
+        # branchChanged clears the stacked_on attribute on the branch if '' is
+        # passed in as the stacked_on location.
+        branch = self.factory.makeAnyBranch()
+        removeSecurityProxy(branch).stacked_on = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged('', '', *self.arbitrary_formats)
+        self.assertIs(None, branch.stacked_on)
+
+    def test_branchChanged_sets_last_mirrored(self):
+        # branchChanged sets the last_mirrored attribute on the branch to the
+        # current time.
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged('', '', *self.arbitrary_formats)
+        self.assertSqlAttributeEqualsDate(
+            branch, 'last_mirrored', UTC_NOW)
+
+    def test_branchChanged_records_bogus_stacked_on_url(self):
+        # If a bogus location is passed in as the stacked_on parameter,
+        # mirror_status_message is set to indicate the problem and stacked_on
+        # set to None.
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged('~does/not/exist', '', *self.arbitrary_formats)
+        self.assertIs(None, branch.stacked_on)
+        self.assertTrue('~does/not/exist' in branch.mirror_status_message)
+
+    def test_branchChanged_clears_mirror_status_message_if_no_error(self):
+        # branchChanged() clears any error that's currently mentioned in
+        # mirror_status_message.
+        branch = self.factory.makeAnyBranch()
+        removeSecurityProxy(branch).mirror_status_message = 'foo'
+        login_person(branch.owner)
+        branch.branchChanged('', '', *self.arbitrary_formats)
+        self.assertIs(None, branch.mirror_status_message)
+
+    def test_branchChanged_creates_scan_job(self):
+        # branchChanged() creates a scan job for the branch.
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        jobs = list(getUtility(IBranchScanJobSource).iterReady())
+        self.assertEqual(0, len(jobs))
+        branch.branchChanged('', 'rev1', *self.arbitrary_formats)
+        jobs = list(getUtility(IBranchScanJobSource).iterReady())
+        self.assertEqual(1, len(jobs))
+
+    def test_branchChanged_doesnt_create_scan_job_for_noop_change(self):
+        # branchChanged() doesn't create a scan job if the tip revision id
+        # hasn't changed.
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        removeSecurityProxy(branch).last_mirrored_id = 'rev1'
+        jobs = list(getUtility(IBranchScanJobSource).iterReady())
+        self.assertEqual(0, len(jobs))
+        branch.branchChanged('', 'rev1', *self.arbitrary_formats)
+        jobs = list(getUtility(IBranchScanJobSource).iterReady())
+        self.assertEqual(0, len(jobs))
+
+    def test_branchChanged_packs_format(self):
+        # branchChanged sets the branch_format etc attributes to the passed in
+        # values.
+        branch = self.factory.makeAnyBranch()
+        login_person(branch.owner)
+        branch.branchChanged(
+            '', 'rev1', ControlFormat.BZR_METADIR_1,
+            BranchFormat.BZR_BRANCH_6, RepositoryFormat.BZR_KNITPACK_1)
+        login(ANONYMOUS)
+        self.assertEqual(
+            (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_6,
+             RepositoryFormat.BZR_KNITPACK_1),
+            (branch.control_format, branch.branch_format,
+             branch.repository_format))
+
+
 class TestBranchGetRevision(TestCaseWithFactory):
     """Make sure that `Branch.getBranchRevision` works as expected."""
 
 
=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:18:06 +0000
@@ -22,7 +22,6 @@
 from zope.security.proxy import removeSecurityProxy
 from zope.security.management import endInteraction
 
-from canonical.database.constants import UTC_NOW
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.webapp import LaunchpadXMLRPCView
 from canonical.launchpad.webapp.authorization import check_permission
@@ -36,7 +35,6 @@
 from lp.code.bzr import BranchFormat, ControlFormat, RepositoryFormat
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import BranchCreationException
-from lp.code.interfaces.branchjob import IBranchScanJobSource
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchnamespace import (
     InvalidNamespace, lookup_branch_namespace, split_unique_name)
@@ -240,44 +238,38 @@
             return True
         return run_with_login(login_id, request_mirror)
 
-    def branchChanged(self, branch_id, stacked_on_location, last_revision_id,
-                      control_string, branch_string, repository_string):
+    def branchChanged(self, login_id, branch_id, stacked_on_location,
+                      last_revision_id, control_string, branch_string,
+                      repository_string):
         """See `ICodehostingAPI`."""
-        branch_set = removeSecurityProxy(getUtility(IBranchLookup))
-        branch = branch_set.get(branch_id)
-        if branch is None:
-            return faults.NoBranchWithID(branch_id)
-        branch.mirror_status_message = None
-        if stacked_on_location == '':
-            stacked_on_branch = None
-        else:
-            stacked_on_branch = branch_set.getByUniqueName(
-                stacked_on_location.strip('/'))
-            if stacked_on_branch is None:
-                branch.mirror_status_message = (
-                    'Invalid stacked on location: ' + stacked_on_location)
-        branch.stacked_on = stacked_on_branch
-        branch.last_mirrored = UTC_NOW
-        if branch.last_mirrored_id != last_revision_id:
-            branch.last_mirrored_id = last_revision_id
-            getUtility(IBranchScanJobSource).create(branch)
-
-        def match_title(enum, title, default):
-            for value in enum.items:
-                if value.title == title:
-                    return value
-            else:
-                return default
-
-        branch.control_format = match_title(
-            ControlFormat, control_string, ControlFormat.UNRECOGNIZED)
-        branch.branch_format = match_title(
-            BranchFormat, branch_string, BranchFormat.UNRECOGNIZED)
-        branch.repository_format = match_title(
-            RepositoryFormat, repository_string,
-            RepositoryFormat.UNRECOGNIZED)
-
-        return True
+        def branch_changed(request_mirror):
+            branch_set = getUtility(IBranchLookup)
+            branch = branch_set.get(branch_id)
+            if branch is None:
+                return faults.NoBranchWithID(branch_id)
+
+            def match_title(enum, title, default):
+                for value in enum.items:
+                    if value.title == title:
+                        return value
+                else:
+                    return default
+
+            control_format = match_title(
+                ControlFormat, control_string, ControlFormat.UNRECOGNIZED)
+            branch_format = match_title(
+                BranchFormat, branch_string, BranchFormat.UNRECOGNIZED)
+            repository_format = match_title(
+                RepositoryFormat, repository_string,
+                RepositoryFormat.UNRECOGNIZED)
+
+            branch.branchChanged(
+                stacked_on_location, last_revision_id, control_format,
+                branch_format, repository_format)
+
+            return True
+
+        return run_with_login(login_id, branch_changed)
 
     def _serializeBranch(self, requester, branch, trailing_path):
         if requester == LAUNCHPAD_SERVICES:
 
=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:17:46 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:18:06 +0000
@@ -578,102 +578,34 @@
         return self.getFormatStringsForFormatName('default')
 
     def test_branchChanged_sets_last_mirrored_id(self):
-        # branchChanged sets the last_mirrored_id attribute on the branch.
+        # branchChanged does many things but lets just check the setting of
+        # last_mirrored_id here.  The other things are tested in unit tests.
         revid = self.factory.getUniqueString()
         branch = self.factory.makeAnyBranch()
         self.codehosting_api.branchChanged(
-            branch.id, '', revid, *self.arbitrary_format_strings)
+            branch.owner.id, branch.id, '', revid,
+            *self.arbitrary_format_strings)
+        login(ANONYMOUS)
         self.assertEqual(revid, branch.last_mirrored_id)
 
-    def test_branchChanged_sets_stacked_on(self):
-        # branchChanged sets the stacked_on attribute based on the unique_name
-        # passed in.
-        branch = self.factory.makeAnyBranch()
-        stacked_on = self.factory.makeAnyBranch()
-        self.codehosting_api.branchChanged(
-            branch.id, stacked_on.unique_name, '',
-            *self.arbitrary_format_strings)
-        self.assertEqual(stacked_on, branch.stacked_on)
-
-    def test_branchChanged_unsets_stacked_on(self):
-        # branchChanged clears the stacked_on attribute on the branch if '' is
-        # passed in as the stacked_on location.
-        branch = self.factory.makeAnyBranch()
-        removeSecurityProxy(branch).stacked_on = self.factory.makeAnyBranch()
-        self.codehosting_api.branchChanged(
-            branch.id, '', '', *self.arbitrary_format_strings)
-        self.assertIs(None, branch.stacked_on)
-
-    def test_branchChanged_sets_last_mirrored(self):
-        # branchChanged sets the last_mirrored attribute on the branch to the
-        # current time.
-        branch = self.factory.makeAnyBranch()
-        self.codehosting_api.branchChanged(
-            branch.id, '', '', *self.arbitrary_format_strings)
-        if self.frontend == LaunchpadDatabaseFrontend:
-            self.assertSqlAttributeEqualsDate(
-                branch, 'last_mirrored', UTC_NOW)
-        else:
-            self.assertIs(UTC_NOW, branch.last_mirrored)
-
-    def test_branchChanged_records_bogus_stacked_on_url(self):
-        # If a bogus location is passed in as the stacked_on parameter,
-        # mirror_status_message is set to indicate the problem and stacked_on
-        # set to None.
-        branch = self.factory.makeAnyBranch()
-        self.codehosting_api.branchChanged(
-            branch.id, '~does/not/exist', '', *self.arbitrary_format_strings)
-        self.assertIs(None, branch.stacked_on)
-        self.assertTrue('~does/not/exist' in branch.mirror_status_message)
-
-    def test_branchChanged_clears_mirror_status_message_if_no_error(self):
-        # branchChanged() clears any error that's currently mentioned in
-        # mirror_status_message.
-        branch = self.factory.makeAnyBranch()
-        removeSecurityProxy(branch).mirror_status_message = 'foo'
-        self.codehosting_api.branchChanged(
-            branch.id, '', '', *self.arbitrary_format_strings)
-        self.assertIs(None, branch.mirror_status_message)
-
     def test_branchChanged_fault_on_unknown_id(self):
         # If the id passed in doesn't match an existing branch, the fault
         # "NoBranchWithID" is returned.
         unused_id = -1
         expected_fault = faults.NoBranchWithID(unused_id)
         received_fault = self.codehosting_api.branchChanged(
-            unused_id, '', '', *self.arbitrary_format_strings)
+            1, unused_id, '', '', *self.arbitrary_format_strings)
+        login(ANONYMOUS)
         self.assertEqual(
             (expected_fault.faultCode, expected_fault.faultString),
             (received_fault.faultCode, received_fault.faultString))
 
-    def test_branchChanged_creates_scan_job(self):
-        # branchChanged() creates a scan job for the branch.
-        if self.frontend != LaunchpadDatabaseFrontend:
-            return
-        branch = self.factory.makeAnyBranch()
-        jobs = list(getUtility(IBranchScanJobSource).iterReady())
-        self.assertEqual(0, len(jobs))
-        self.codehosting_api.branchChanged(
-            branch.id, '', 'rev1', *self.arbitrary_format_strings)
-        jobs = list(getUtility(IBranchScanJobSource).iterReady())
-        self.assertEqual(1, len(jobs))
-
-    def test_branchChanged_doesnt_create_scan_job_for_noop_change(self):
-        if self.frontend != LaunchpadDatabaseFrontend:
-            return
-        branch = self.factory.makeAnyBranch()
-        removeSecurityProxy(branch).last_mirrored_id = 'rev1'
-        jobs = list(getUtility(IBranchScanJobSource).iterReady())
-        self.assertEqual(0, len(jobs))
-        self.codehosting_api.branchChanged(
-            branch.id, '', 'rev1', *self.arbitrary_format_strings)
-        jobs = list(getUtility(IBranchScanJobSource).iterReady())
-        self.assertEqual(0, len(jobs))
-
     def test_branchChanged_2a_format(self):
         branch = self.factory.makeAnyBranch()
         self.codehosting_api.branchChanged(
-            branch.id, '', 'rev1', *self.getFormatStringsForFormatName('2a'))
+            branch.owner.id, branch.id, '', 'rev1',
+            *self.getFormatStringsForFormatName('2a'))
+        login(ANONYMOUS)
         self.assertEqual(
             (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_7,
              RepositoryFormat.BZR_CHK_2A),
@@ -683,8 +615,9 @@
     def test_branchChanged_packs_format(self):
         branch = self.factory.makeAnyBranch()
         self.codehosting_api.branchChanged(
-            branch.id, '', 'rev1',
+            branch.owner.id, branch.id, '', 'rev1',
             *self.getFormatStringsForFormatName('pack-0.92'))
+        login(ANONYMOUS)
         self.assertEqual(
             (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_6,
              RepositoryFormat.BZR_KNITPACK_1),
@@ -694,8 +627,9 @@
     def test_branchChanged_knits_format(self):
         branch = self.factory.makeAnyBranch()
         self.codehosting_api.branchChanged(
-            branch.id, '', 'rev1',
+            branch.owner.id, branch.id, '', 'rev1',
             *self.getFormatStringsForFormatName('knit'))
+        login(ANONYMOUS)
         self.assertEqual(
             (ControlFormat.BZR_METADIR_1, BranchFormat.BZR_BRANCH_5,
              RepositoryFormat.BZR_KNIT_1),
 
=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2010-04-27 02:17:46 +0000
+++ lib/lp/codehosting/inmemory.py	2010-04-27 02:18:06 +0000
@@ -614,8 +614,9 @@
     def requestMirror(self, requester_id, branch_id):
         self._branch_set.get(branch_id).requestMirror()
 
-    def branchChanged(self, branch_id, stacked_on_location, last_revision_id,
-                      control_string, branch_string, repository_string):
+    def branchChanged(self, login_id, branch_id, stacked_on_location,
+                      last_revision_id, control_string, branch_string,
+                      repository_string):
         branch = self._branch_set._find(id=branch_id)
         if branch is None:
             return faults.NoBranchWithID(branch_id)
 
=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py	2010-04-27 02:17:46 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py	2010-04-27 02:18:06 +0000
@@ -126,8 +126,9 @@
         """
         return defer.maybeDeferred(
             self._codehosting_endpoint.callRemote,
-            'branchChanged', branch_id, stacked_on_url, last_revision_id,
-            control_string, branch_string, repository_string)
+            'branchChanged', self._user_id, branch_id, stacked_on_url,
+            last_revision_id, control_string, branch_string,
+            repository_string)
 
     def translatePath(self, path):
         """Translate 'path'."""