Merge lp:~james-w/launchpad/new-code-import-method-on-branchtarget into lp:launchpad

[James Westby]

New method IBranchTarget.newCodeImport() to create code imports for a given target.

Hi,

This adds a newCodeImport() method on IBranchTarget that is quite
a simple wrapper around ICodeImportSet.new().

The reason for this is basically to export it later from that
position.

It also adds a new property to IBranchTarget, supports_code_imports,
which indicates, surprisingly, whether the target supports code imports.
This is in the spirit of supports_merge_proposals. Currently it is
only enabled for products, so there is no change from before.

Thanks,

James

[Michael Hudson-Doyle (mwhudson)]

Two minor whitespace tweaks please, or rather the same one in two
places. Other than that, awesome!

> === modified file 'lib/lp/code/interfaces/branchtarget.py'
> --- lib/lp/code/interfaces/branchtarget.py 2010-01-05 08:18:23 +0000
> +++ lib/lp/code/interfaces/branchtarget.py 2010-03-26 01:45:34 +0000
> @@ -88,6 +88,9 @@
> supports_merge_proposals = Attribute(
> "Does this target support merge proposals at all?")
>
> + supports_code_imports = Attribute(
> + "Does this target support code imports at all?")
> +
> def areBranchesMergeable(other_target):
> """Are branches from other_target mergeable into this target."""
>
> @@ -113,3 +116,18 @@
>
> def getBugTask(bug):
> """Get the BugTask for a given bug related to the branch target."""
> +
> + def newCodeImport(registrant, branch_name, rcs_type,
> + url=None, cvs_root=None, cvs_module=None):

This should be formatted like so:

    def newCodeImport(registrant, branch_name, rcs_type, url=None,
                      cvs_root=None, cvs_module=None):

(https://dev.launchpad.net/PythonStyleGuide#Wrapping%20long%20arguments%20in%20function%20definitions)

> + """Create a new code import for this target.
> +
> + :param registrant: the `IPerson` who should be recorded as creating
> + the import and will own the resulting branch.
> + :param branch_name: the name the resulting branch should have.
> + :param rcs_type: the type of the foreign VCS.
> + :param url: the url to import from if the import isn't CVS.
> + :param cvs_root: if the import is from CVS the CVSROOT to import from.
> + :param cvs_module: if the import is from CVS the module to import.
> + :returns: an `ICodeImport`.
> + :raises AssertionError: if supports_code_imports is False.
> + """

> === modified file 'lib/lp/code/model/branchtarget.py'
> --- lib/lp/code/model/branchtarget.py 2010-01-05 08:18:23 +0000
> +++ lib/lp/code/model/branchtarget.py 2010-03-26 01:45:34 +0000
> @@ -19,6 +19,7 @@
> from lp.code.interfaces.branchcollection import IAllBranches
> from lp.code.interfaces.branchtarget import (
> check_default_stacked_on, IBranchTarget)
> +from lp.code.interfaces.codeimport import ICodeImportSet
> from lp.registry.interfaces.pocket import PackagePublishingPocket
> from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
>
> @@ -36,6 +37,12 @@
> def __ne__(self, other):
> return self.context != other.context
>
> + def newCodeImport(self, registrant, branch_name, rcs_type, url=None,
> + cvs_root=None, cvs_module=None):

Similarly.

> + return getUtility(ICodeImportSet).new(
> + registrant, self, branch_name, rcs_type, url=url,
> + cvs_root=cvs_root, cvs_module=cvs_module)
> +
>
> class PackageBranchTarget(_BaseBranchTarget):
> implements(IBranchTarget)
> @@ -95,6 +102,11 @@
> """See `IBranchTarget`."""
> return True
>
> + @property
> + def supports_code_imports(self):
> + """See `IBranchTarget`."""
> + return False
> +
> def areBranchesMergeable(self, other_target)...

[Michael Hudson-Doyle (mwhudson)]

review: Approve

[James Westby (james-w)]

Whitespace issues fixed.

Thanks,

James

=== modified file 'lib/lp/code/interfaces/branchtarget.py'
--- lib/lp/code/interfaces/branchtarget.py	2010-01-05 08:18:23 +0000
+++ lib/lp/code/interfaces/branchtarget.py	2010-04-02 02:44:29 +0000
@@ -88,6 +88,9 @@
     supports_merge_proposals = Attribute(
         "Does this target support merge proposals at all?")
 
+    supports_code_imports = Attribute(
+        "Does this target support code imports at all?")
+
     def areBranchesMergeable(other_target):
         """Are branches from other_target mergeable into this target."""
 
@@ -113,3 +116,18 @@
 
     def getBugTask(bug):
         """Get the BugTask for a given bug related to the branch target."""
+
+    def newCodeImport(registrant, branch_name, rcs_type, url=None,
+                      cvs_root=None, cvs_module=None):
+        """Create a new code import for this target.
+
+        :param registrant: the `IPerson` who should be recorded as creating
+            the import and will own the resulting branch.
+        :param branch_name: the name the resulting branch should have.
+        :param rcs_type: the type of the foreign VCS.
+        :param url: the url to import from if the import isn't CVS.
+        :param cvs_root: if the import is from CVS the CVSROOT to import from.
+        :param cvs_module: if the import is from CVS the module to import.
+        :returns: an `ICodeImport`.
+        :raises AssertionError: if supports_code_imports is False.
+        """

=== modified file 'lib/lp/code/model/branchtarget.py'
--- lib/lp/code/model/branchtarget.py	2010-01-05 08:18:23 +0000
+++ lib/lp/code/model/branchtarget.py	2010-04-02 02:44:29 +0000
@@ -19,6 +19,7 @@
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.code.interfaces.branchtarget import (
     check_default_stacked_on, IBranchTarget)
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
 
@@ -36,6 +37,12 @@
     def __ne__(self, other):
         return self.context != other.context
 
+    def newCodeImport(self, registrant, branch_name, rcs_type, url=None,
+                      cvs_root=None, cvs_module=None):
+        return getUtility(ICodeImportSet).new(
+            registrant, self, branch_name, rcs_type, url=url,
+            cvs_root=cvs_root, cvs_module=cvs_module)
+
 
 class PackageBranchTarget(_BaseBranchTarget):
     implements(IBranchTarget)
@@ -95,6 +102,11 @@
         """See `IBranchTarget`."""
         return True
 
+    @property
+    def supports_code_imports(self):
+        """See `IBranchTarget`."""
+        return False
+
     def areBranchesMergeable(self, other_target):
         """See `IBranchTarget`."""
         # Branches are mergable into a PackageTarget if the source package
@@ -182,6 +194,11 @@
         """See `IBranchTarget`."""
         return False
 
+    @property
+    def supports_code_imports(self):
+        """See `IBranchTarget`."""
+        return False
+
     def areBranchesMergeable(self, other_target):
         """See `IBranchTarget`."""
         return False
@@ -258,6 +275,11 @@
         """See `IBranchTarget`."""
         return True
 
+    @property
+    def supports_code_imports(self):
+        """See `IBranchTarget`."""
+        return True
+
     def areBranchesMergeable(self, other_target):
         """See `IBranchTarget`."""
         # Branches are mergable into a PackageTarget if the source package
@@ -311,6 +333,11 @@
         """See `IBranchTarget`."""
         return self.productseries
 
+    @property
+    def supports_code_imports(self):
+        """See `IBranchTarget`."""
+        return False
+
 
 def get_canonical_url_data_for_target(branch_target):
     """Return the `ICanonicalUrlData` for an `IBranchTarget`."""

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2010-04-02 02:44:28 +0000
+++ lib/lp/code/model/codeimport.py	2010-04-02 02:44:29 +0000
@@ -225,6 +225,8 @@
                 review_status = CodeImportReviewStatus.REVIEWED
             else:
                 review_status = CodeImportReviewStatus.NEW
+        if not target.supports_code_imports:
+            raise AssertionError("%r doesn't support code imports" % target)
         # Create the branch for the CodeImport.
         namespace = target.getNamespace(registrant)
         import_branch = namespace.createBranch(

=== modified file 'lib/lp/code/model/tests/test_branchtarget.py'
--- lib/lp/code/model/tests/test_branchtarget.py	2010-03-18 15:39:58 +0000
+++ lib/lp/code/model/tests/test_branchtarget.py	2010-04-02 02:44:29 +0000
@@ -14,8 +14,9 @@
     check_default_stacked_on,
     PackageBranchTarget, PersonBranchTarget, ProductBranchTarget,
     ProductSeriesBranchTarget)
-from lp.code.enums import BranchType
+from lp.code.enums import BranchType, RevisionControlSystems
 from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.code.interfaces.codeimport import ICodeImport
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from canonical.launchpad.webapp import canonical_url
@@ -176,6 +177,16 @@
             ubuntu_branches.teamowner)
         self.assertEqual(branch, self.target.default_merge_target)
 
+    def test_doesnt_support_code_imports(self):
+        self.assertFalse(self.target.supports_code_imports)
+
+    def test_creating_code_import_fails(self):
+        self.assertRaises(
+            AssertionError, self.target.newCodeImport,
+                self.factory.makePerson(),
+                self.factory.getUniqueString("name-"),
+                RevisionControlSystems.GIT, url=self.factory.getUniqueURL())
+
 
 class TestPersonBranchTarget(TestCaseWithFactory, BaseBranchTargetTests):
 
@@ -258,6 +269,16 @@
         self.target._retargetBranch(removeSecurityProxy(branch))
         self.assertEqual(self.target, branch.target)
 
+    def test_doesnt_support_code_imports(self):
+        self.assertFalse(self.target.supports_code_imports)
+
+    def test_creating_code_import_fails(self):
+        self.assertRaises(
+            AssertionError, self.target.newCodeImport,
+                self.factory.makePerson(),
+                self.factory.getUniqueString("name-"),
+                RevisionControlSystems.GIT, url=self.factory.getUniqueURL())
+
 
 class TestProductBranchTarget(TestCaseWithFactory, BaseBranchTargetTests):
 
@@ -364,6 +385,23 @@
             setattr, self.original.development_focus, 'branch', branch)
         self.assertEqual(branch, self.target.default_merge_target)
 
+    def test_supports_code_imports(self):
+        self.assertTrue(self.target.supports_code_imports)
+
+    def test_creating_code_import_succeeds(self):
+        target_url = self.factory.getUniqueURL()
+        branch_name = self.factory.getUniqueString("name-")
+        owner = self.factory.makePerson()
+        code_import = self.target.newCodeImport(
+            owner, branch_name, RevisionControlSystems.GIT, url=target_url)
+        code_import = removeSecurityProxy(code_import)
+        self.assertProvides(code_import, ICodeImport)
+        self.assertEqual(target_url, code_import.url)
+        self.assertEqual(branch_name, code_import.branch.name)
+        self.assertEqual(owner, code_import.registrant)
+        self.assertEqual(owner, code_import.branch.owner)
+        self.assertEqual(self.target, code_import.branch.target)
+
 
 class TestProductSeriesBranchTarget(TestCaseWithFactory):
 
@@ -378,6 +416,16 @@
         target = IBranchTarget(self.original)
         self.assertIsInstance(target, ProductSeriesBranchTarget)
 
+    def test_doesnt_support_code_imports(self):
+        self.assertFalse(self.target.supports_code_imports)
+
+    def test_creating_code_import_fails(self):
+        self.assertRaises(
+            AssertionError, self.target.newCodeImport,
+                self.factory.makePerson(),
+                self.factory.getUniqueString("name-"),
+                RevisionControlSystems.GIT, url=self.factory.getUniqueURL())
+
 
 class TestCheckDefaultStackedOnBranch(TestCaseWithFactory):
     """Only certain branches are allowed to be default stacked-on branches."""

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2010-04-02 02:44:28 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2010-04-02 02:44:29 +0000
@@ -120,9 +120,20 @@
         self.assertEqual(
             CodeImportReviewStatus.REVIEWED,
             code_import.review_status)
-        # No job is created for the import.
+        # A job is created for the import.
         self.assertIsNot(None, code_import.import_job)
 
+    def test_junk_code_import_rejected(self):
+        """You are not allowed to create code imports targetting +junk."""
+        registrant = self.factory.makePerson()
+        self.assertRaises(AssertionError, CodeImportSet().new,
+            registrant=registrant,
+            target=IBranchTarget(registrant),
+            branch_name='imported',
+            rcs_type=RevisionControlSystems.HG,
+            url=self.factory.getUniqueURL(),
+            review_status=None)
+
 
 class TestCodeImportDeletion(TestCaseWithFactory):
     """Test the deletion of CodeImports."""