Merge lp:~james-w/launchpad/register-code-import into lp:launchpad

[James Westby]

There is now a newCodeImport method on Product and SourcePackage in the API.

Hi,

This exposes the creation of code imports over the API.

It first adds an "owner" argument, so that we can specify
this through the API, with associated tests.

It then exposes the IBranchTarget.newCodeImport() method
over the API using a small interface and mixin, due to it being
on an adapter.

It then adds a bunch of tests for this, including error cases
where the user may specify bad information.

For the last bit I used webservice_error so that they would get
400 not 500 responses, and there won't be OOPSes generated for them.

To do this I had to add the exception classes that have the decorator
to the interfaces/webservice.py file so that lazr.restful can register
views for them.

Doing this I discovered that the existing webservice_error in lp.code
wasn't working, and in fact had a test asserting it wasn't working,
so I fixed that along the way.

Thanks,

James

[James Westby (james-w)]

Passes ec2, so requesting a review.

Thanks,

James

[Aaron Bentley (abentley)]

It's nice of you to defer requesting a review until all tests are passing, but I think a lot of us don't observe that.

Was there a preimplementation call?

Was there any lint?

AIUI PEP8 requires that items within a class definition be separated by a single blank line, so a bunch of your webservice_error(400) lines need a blank line in front of them.

Why are you using export_write_operation rather than export_factory_operation on branchtarget?

On IHasCodeImports, you claim that newCodeImport returns BranchMergeProposal. I think this is inaccurate.

What would you think about expressing xx-code-import.txt's "Exceptions" heading as a unit test? Failing that, it needs two blank lines separating it from the previous section.

review: Needs Information

[James Westby (james-w)]

> It's nice of you to defer requesting a review until all tests are passing, but
> I think a lot of us don't observe that.
>
> Was there a preimplementation call?

No, but I chatted with wgrant about the strategy.

> Was there any lint?

I don't know, running "make lint" runs it on lots of unexpected files.

> AIUI PEP8 requires that items within a class definition be separated by a
> single blank line, so a bunch of your webservice_error(400) lines need a blank
> line in front of them.

Done.

> Why are you using export_write_operation rather than export_factory_operation
> on branchtarget?

That's not supposed to be there any more, deleted.

> On IHasCodeImports, you claim that newCodeImport returns BranchMergeProposal.
> I think this is inaccurate.

Fixed.

> What would you think about expressing xx-code-import.txt's "Exceptions"
> heading as a unit test? Failing that, it needs two blank lines separating it
> from the previous section.

I don't know how to do webservice tests as unit tests, if you can point me to
examples I will convert them.

Blank lines issue fixed anyway, and I updated the text to make the intent of
the tests clearer as well.

Thanks,

James

[Aaron Bentley (abentley)]

On 04/05/2010 05:39 PM, James Westby wrote:

>> Was there any lint?
>
> I don't know, running "make lint" runs it on lots of unexpected files.

When there are no uncommitted changes, "make lint" compares your tree to
the parent branch to determine which files it runs against. This means
it will also lint files changed by your prerequisite branch. You parent
branch may also be out of date.

You can also run bin/lint.sh with the list of files you modified. If
you use pipelines and have the lp-review-body plugin installed, you can
use lp-propose (formerly lp-submit) and it will use the previous pipe to
determine which files to lint.

Anyhow, there was lots of lint:
$ make lint
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
   lib/canonical/launchpad/interfaces/_schema_circular_imports.py
   lib/lp/code/errors.py
   lib/lp/code/interfaces/branch.py
   lib/lp/code/interfaces/branchtarget.py
   lib/lp/code/interfaces/codeimport.py
   lib/lp/code/interfaces/hasbranches.py
   lib/lp/code/interfaces/webservice.py
   lib/lp/code/model/branchtarget.py
   lib/lp/code/model/codeimport.py
   lib/lp/code/model/hasbranches.py
   lib/lp/code/model/tests/test_codeimport.py
   lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
   lib/lp/code/stories/webservice/xx-code-import.txt
   lib/lp/registry/interfaces/product.py
   lib/lp/registry/interfaces/sourcepackage.py
   lib/lp/registry/model/product.py
   lib/lp/registry/model/sourcepackage.py

== Pyflakes notices ==

lib/canonical/launchpad/interfaces/_schema_circular_imports.py
     43: 'IBranchTarget' imported but unused

lib/lp/code/interfaces/branch.py
     11: undefined name 'UnknownBranchTypeError' in __all__

lib/lp/code/interfaces/webservice.py
     8: 'BranchMergeProposalExists' imported but unused
     9: 'BranchCreatorNotOwner' imported but unused
     9: 'IBranchSet' imported but unused
     9: 'IBranch' imported but unused
     9: 'BranchExists' imported but unused
     9: 'BranchCreatorNotMemberOfOwnerTeam' imported but unused
     12: 'IBranchMergeProposal' imported but unused
     13: 'IBranchSubscription' imported but unused
     14: 'ICodeImport' imported but unused
     15: 'ICodeReviewComment' imported but unused
     16: 'ICodeReviewVoteReference' imported but unused
     17: 'IStaticDiff' imported but unused
     17: 'IDiff' imported but unused
     17: 'IPreviewDiff' imported but unused

lib/lp/registry/model/sourcepackage.py
     684: local variable 'displayname' is assigned to but never used

== Pylint notices ==

lib/canonical/launchpad/interfaces/_schema_circular_imports.py
     18: [F0401] Unable to import 'lazr.restful.declarations'
     43: [W0611] Unused import IBranchTarget

lib/lp/code/errors.py
     20: [F0401] Unable to import 'lazr.restful.declarations'

lib/lp/code/interfaces/branch.py
     442: [C0322, IBranch.setOwner] Operator not preceded by a space
     title=_("The new owner of the branch."),
     ^
     schema=IPerson))
     @export_write_operation()
     def setOwner(new_owner, user):
     451: [C0322, IBranch.setTarget] Opera...

[James Westby (james-w)]

On Tue, 06 Apr 2010 15:33:31 -0000, Aaron Bentley <email address hidden> wrote:
> On 04/05/2010 05:39 PM, James Westby wrote:
>
> >> Was there any lint?
> >
> > I don't know, running "make lint" runs it on lots of unexpected files.
>
> When there are no uncommitted changes, "make lint" compares your tree to
> the parent branch to determine which files it runs against. This means
> it will also lint files changed by your prerequisite branch. You parent
> branch may also be out of date.

I am using pipelines, so I would have thought it would run against the
previous pipe?

> You can also run bin/lint.sh with the list of files you modified. If
> you use pipelines and have the lp-review-body plugin installed, you can
> use lp-propose (formerly lp-submit) and it will use the previous pipe to
> determine which files to lint.

That's what I used (at least for one of these proposals,) but I didn't
have the extra plugin installed.

> lib/canonical/launchpad/interfaces/_schema_circular_imports.py
> 43: 'IBranchTarget' imported but unused

Fixed.

> lib/lp/code/interfaces/branch.py
> 11: undefined name 'UnknownBranchTypeError' in __all__

Fixed.

> lib/lp/code/interfaces/webservice.py
> 8: 'BranchMergeProposalExists' imported but unused
> 9: 'BranchCreatorNotOwner' imported but unused
> 9: 'IBranchSet' imported but unused
> 9: 'IBranch' imported but unused
> 9: 'BranchExists' imported but unused
> 9: 'BranchCreatorNotMemberOfOwnerTeam' imported but unused
> 12: 'IBranchMergeProposal' imported but unused
> 13: 'IBranchSubscription' imported but unused
> 14: 'ICodeImport' imported but unused
> 15: 'ICodeReviewComment' imported but unused
> 16: 'ICodeReviewVoteReference' imported but unused
> 17: 'IStaticDiff' imported but unused
> 17: 'IDiff' imported but unused
> 17: 'IPreviewDiff' imported but unused

Noise. This file is purely for importing.

> lib/lp/registry/model/sourcepackage.py
> 684: local variable 'displayname' is assigned to but never used

Fixed.

> lib/lp/code/interfaces/branch.py
> 442: [C0322, IBranch.setOwner] Operator not preceded by a space
> title=_("The new owner of the branch."),
> ^
> schema=IPerson))
> @export_write_operation()
> def setOwner(new_owner, user):
> 451: [C0322, IBranch.setTarget] Operator not preceded by a space
> title=_("The project the branch belongs to."),
> ^
> schema=Interface, required=False), # Really IProduct
> source_package=Reference(
> title=_("The source package the branch belongs to."),
> schema=Interface, required=False)) # Really ISourcePackage
> @export_write_operation()
> def setTarget(user, project=None, source_package=None):
> 511: [C0322, IBranch.isPersonTrustedReviewer] Operator not preceded
> by a space
> title=_("A person for which the reviewer status is in question."),
> ^
> schema=IPerson))
> @export_read_operation()
> def isPersonTrustedReviewer(reviewer):
> 760: [C0322, IBranch._createMergeProposal] Operator not preceded by
> a space
> needs_review=Bool(title=_('Needs review'),
> ...

[Aaron Bentley (abentley)]

On 04/07/2010 10:06 AM, James Westby wrote:
> On Tue, 06 Apr 2010 15:33:31 -0000, Aaron Bentley<email address hidden> wrote:
>> When there are no uncommitted changes, "make lint" compares your tree to
>> the parent branch to determine which files it runs against. This means
>> it will also lint files changed by your prerequisite branch. You parent
>> branch may also be out of date.
>
> I am using pipelines, so I would have thought it would run against the
> previous pipe?

No, I did not design it and it predates pipelines.

>> lib/lp/code/interfaces/webservice.py
>> 8: 'BranchMergeProposalExists' imported but unused
>> 9: 'BranchCreatorNotOwner' imported but unused
>> 9: 'IBranchSet' imported but unused
>> 9: 'IBranch' imported but unused
>> 9: 'BranchExists' imported but unused
>> 9: 'BranchCreatorNotMemberOfOwnerTeam' imported but unused
>> 12: 'IBranchMergeProposal' imported but unused
>> 13: 'IBranchSubscription' imported but unused
>> 14: 'ICodeImport' imported but unused
>> 15: 'ICodeReviewComment' imported but unused
>> 16: 'ICodeReviewVoteReference' imported but unused
>> 17: 'IStaticDiff' imported but unused
>> 17: 'IDiff' imported but unused
>> 17: 'IPreviewDiff' imported but unused
>
> Noise. This file is purely for importing.

It seems really weird to me. If it's there for external users, why isn't there an __all__? I'll chat with Leonard, but it doesn't need to block you.

review: Approve

=== added directory 'lib/canonical/launchpad/apidoc'
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-04-09 15:46:09 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-04-13 15:21:36 +0000
@@ -45,7 +45,7 @@
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.diff import IPreviewDiff
 from lp.code.interfaces.hasbranches import (
-    IHasBranches, IHasMergeProposals, IHasRequestedReviews)
+    IHasBranches, IHasCodeImports, IHasMergeProposals, IHasRequestedReviews)
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuild)
 from lp.registry.interfaces.distribution import IDistribution
@@ -130,6 +130,10 @@
     IHasMergeProposals, 'getMergeProposals', IBranchMergeProposal)
 patch_collection_return_type(
     IHasRequestedReviews, 'getRequestedReviews', IBranchMergeProposal)
+patch_entry_return_type(
+    IHasCodeImports, 'newCodeImport', ICodeImport)
+patch_plain_parameter_type(
+    IHasCodeImports, 'newCodeImport', 'owner', IPerson)
 
 # IBugTask
 

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2010-03-10 18:53:50 +0000
+++ lib/lp/code/errors.py	2010-04-13 15:21:36 +0000
@@ -41,6 +41,7 @@
 
 class BranchMergeProposalExists(InvalidBranchMergeProposal):
     """Raised if there is already a matching BranchMergeProposal."""
+
     webservice_error(400) #Bad request.
 
 

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-04-09 02:08:39 +0000
+++ lib/lp/code/interfaces/branch.py	2010-04-13 15:21:36 +0000
@@ -49,7 +49,8 @@
     export_as_webservice_collection, export_as_webservice_entry,
     export_destructor_operation, export_factory_operation,
     export_operation_as, export_read_operation, export_write_operation,
-    exported, mutator_for, operation_parameters, operation_returns_entry)
+    exported, mutator_for, operation_parameters, operation_returns_entry,
+    webservice_error)
 
 from canonical.config import config
 
@@ -93,6 +94,8 @@
 class BranchExists(BranchCreationException):
     """Raised when creating a branch that already exists."""
 
+    webservice_error(400)
+
     def __init__(self, existing_branch):
         # XXX: TimPenhey 2009-07-12 bug=405214: This error
         # message logic is incorrect, but the exact text is being tested
@@ -135,6 +138,8 @@
     the branch to a team that they are not a member of.
     """
 
+    webservice_error(400)
+
 
 class BranchCreationNoTeamOwnedJunkBranches(BranchCreationException):
     """We forbid the creation of team-owned +junk branches.
@@ -158,6 +163,8 @@
     the branch to another user.
     """
 
+    webservice_error(400)
+
 
 class BranchTypeError(Exception):
     """An operation cannot be performed for a particular branch type.

=== modified file 'lib/lp/code/interfaces/branchtarget.py'
--- lib/lp/code/interfaces/branchtarget.py	2010-04-09 02:08:39 +0000
+++ lib/lp/code/interfaces/branchtarget.py	2010-04-13 15:21:36 +0000
@@ -121,7 +121,7 @@
         """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):
+                      cvs_root=None, cvs_module=None, owner=None):
         """Create a new code import for this target.
 
         :param registrant: the `IPerson` who should be recorded as creating
@@ -131,6 +131,8 @@
         :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.
+        :param owner: the `IPerson` to own the resulting branch, or None to
+            use registrant.
         :returns: an `ICodeImport`.
         :raises AssertionError: if supports_code_imports is False.
         """

=== modified file 'lib/lp/code/interfaces/codeimport.py'
--- lib/lp/code/interfaces/codeimport.py	2010-03-26 02:24:29 +0000
+++ lib/lp/code/interfaces/codeimport.py	2010-04-13 15:21:36 +0000
@@ -182,10 +182,14 @@
     """Interface representing the set of code imports."""
 
     def new(registrant, target, branch_name, rcs_type, url=None,
-            cvs_root=None, cvs_module=None, review_status=None):
+            cvs_root=None, cvs_module=None, review_status=None,
+            owner=None):
         """Create a new CodeImport.
 
         :param target: An `IBranchTarget` that the code is associated with.
+        :param owner: The `IPerson` to set as the owner of the branch, or
+            None to use registrant. registrant must be a member of owner to
+            do this.
         """
 
     def get(id):

=== modified file 'lib/lp/code/interfaces/hasbranches.py'
--- lib/lp/code/interfaces/hasbranches.py	2010-02-08 14:37:50 +0000
+++ lib/lp/code/interfaces/hasbranches.py	2010-04-13 15:21:36 +0000
@@ -1,25 +1,30 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+# pylint: disable-msg=E0213
+
 """Interface definitions for IHas<code related bits>."""
 
 __metaclass__ = type
 __all__ = [
     'IHasBranches',
+    'IHasCodeImports',
     'IHasMergeProposals',
     'IHasRequestedReviews',
     ]
 
 
 from zope.interface import Interface
-from zope.schema import Choice, Datetime, List
+from zope.schema import Choice, Datetime, List, TextLine
 
 from canonical.launchpad import _
-from lp.code.enums import BranchLifecycleStatus, BranchMergeProposalStatus
+from lp.code.enums import (
+    BranchLifecycleStatus, BranchMergeProposalStatus, RevisionControlSystems)
 
 from lazr.restful.declarations import (
-    REQUEST_USER, call_with, export_read_operation, operation_parameters,
-    operation_returns_collection_of)
+    REQUEST_USER, call_with, export_factory_operation, export_read_operation,
+    operation_parameters, operation_returns_collection_of)
+from lazr.restful.fields import Reference
 
 
 class IHasBranches(Interface):
@@ -98,7 +103,7 @@
     @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
     @export_read_operation()
     def getRequestedReviews(status=None, visible_by_user=None):
-        """Returns merge proposals that a review was requested from the person.
+        """Returns merge proposals where a person was asked to review.
 
         This does not include merge proposals that were requested from
         teams that the person is part of. If status is not passed then
@@ -108,3 +113,42 @@
         :param visible_by_user: Normally the user who is asking.
         :returns: A list of `IBranchMergeProposal`.
         """
+
+
+class IHasCodeImports(Interface):
+    """Some things can have code imports that target them.
+
+    This interface defines the common methods that for working with them.
+    """
+
+    # In order to minimise dependancies the returns_collection is defined as
+    # Interface here and defined fully in the circular imports file.
+
+    @operation_parameters(
+        branch_name=TextLine(
+            title=_('Name of branch to create'), required=True),
+        rcs_type=Choice(vocabulary=RevisionControlSystems, required=True),
+        url=TextLine(title=_('Foreign VCS URL')),
+        cvs_root=TextLine(title=_('CVS root URL')),
+        cvs_module=TextLine(title=_('CVS module to import')),
+        owner=Reference(title=_('Owner of the resulting branch'),
+            schema=Interface)
+        )
+    @call_with(registrant=REQUEST_USER)
+    @export_factory_operation(Interface, []) # Really ICodeImport.
+    def newCodeImport(registrant=None, branch_name=None, rcs_type=None,
+                      url=None, cvs_root=None, cvs_module=None, owner=None):
+        """Create a new code import.
+
+        :param registrant: The IPerson to record as the registrant of the
+            import
+        :param branch_name: The name of the branch to create.
+        :param rcs_type: The type of the foreign VCS.
+        :param url: The URL to import from if the VCS type uses a single URL
+            (i.e. isn't CVS).
+        :param cvs_root: The CVSROOT for a CVS import.
+        :param cvs_module: The module to import for a CVS import.
+        :param owner: Who should own the created branch, or None for it to
+            be the same as the registrant, or the caller over the API.
+        :returns: An instance of `ICodeImport`.
+        """

=== modified file 'lib/lp/code/interfaces/webservice.py'
--- lib/lp/code/interfaces/webservice.py	2010-03-16 19:04:48 +0000
+++ lib/lp/code/interfaces/webservice.py	2010-04-13 15:21:36 +0000
@@ -3,7 +3,12 @@
 
 """All the interfaces that are exposed through the webservice."""
 
-from lp.code.interfaces.branch import IBranch, IBranchSet
+# The exceptions are imported so that they can produce the special
+# status code defined by webservice_error when they are raised.
+from lp.code.errors import BranchMergeProposalExists
+from lp.code.interfaces.branch import (
+    IBranch, IBranchSet, BranchCreatorNotMemberOfOwnerTeam,
+    BranchCreatorNotOwner, BranchExists)
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.branchsubscription import IBranchSubscription
 from lp.code.interfaces.codeimport import ICodeImport

=== modified file 'lib/lp/code/mail/tests/test_codeimport.py'
--- lib/lp/code/mail/tests/test_codeimport.py	2010-04-13 15:21:34 +0000
+++ lib/lp/code/mail/tests/test_codeimport.py	2010-04-13 15:21:36 +0000
@@ -118,7 +118,7 @@
         series = self.factory.makeDistroSeries(
             name='manic', distribution=distro)
         fooix = self.factory.makeSourcePackage(
-            sourcename='fooix', distroseries=series)
+            sourcepackagename='fooix', distroseries=series)
         # Eric needs to be logged in for the mail to be sent.
         login_person(eric)
         code_import = self.factory.makePackageCodeImport(

=== modified file 'lib/lp/code/model/branchtarget.py'
--- lib/lp/code/model/branchtarget.py	2010-04-13 15:21:34 +0000
+++ lib/lp/code/model/branchtarget.py	2010-04-13 15:21:36 +0000
@@ -37,10 +37,11 @@
         return self.context != other.context
 
     def newCodeImport(self, registrant, branch_name, rcs_type, url=None,
-                      cvs_root=None, cvs_module=None):
+                      cvs_root=None, cvs_module=None, owner=None):
+        """See `IBranchTarget`."""
         return getUtility(ICodeImportSet).new(
             registrant, self, branch_name, rcs_type, url=url,
-            cvs_root=cvs_root, cvs_module=cvs_module)
+            cvs_root=cvs_root, cvs_module=cvs_module, owner=owner)
 
 
 class PackageBranchTarget(_BaseBranchTarget):
@@ -336,6 +337,23 @@
         branch.sourcepackagename = None
 
 
+class ProductSeriesBranchTarget(ProductBranchTarget):
+
+    def __init__(self, productseries):
+        ProductBranchTarget.__init__(self, productseries.product)
+        self.productseries = productseries
+
+    @property
+    def context(self):
+        """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`."""
     return ICanonicalUrlData(branch_target.context)

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2010-03-26 01:41:16 +0000
+++ lib/lp/code/model/codeimport.py	2010-04-13 15:21:36 +0000
@@ -203,7 +203,8 @@
     implements(ICodeImportSet)
 
     def new(self, registrant, target, branch_name, rcs_type,
-            url=None, cvs_root=None, cvs_module=None, review_status=None):
+            url=None, cvs_root=None, cvs_module=None, review_status=None,
+            owner=None):
         """See `ICodeImportSet`."""
         if rcs_type == RevisionControlSystems.CVS:
             assert cvs_root is not None and cvs_module is not None
@@ -227,14 +228,16 @@
                 review_status = CodeImportReviewStatus.NEW
         if not target.supports_code_imports:
             raise AssertionError("%r doesn't support code imports" % target)
+        if owner is None:
+            owner = registrant
         # Create the branch for the CodeImport.
-        namespace = target.getNamespace(registrant)
+        namespace = target.getNamespace(owner)
         import_branch = namespace.createBranch(
             branch_type=BranchType.IMPORTED, name=branch_name,
             registrant=registrant)
 
         code_import = CodeImport(
-            registrant=registrant, owner=registrant, branch=import_branch,
+            registrant=registrant, owner=owner, branch=import_branch,
             rcs_type=rcs_type, url=url,
             cvs_root=cvs_root, cvs_module=cvs_module,
             review_status=review_status)

=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py	2010-03-09 17:30:18 +0000
+++ lib/lp/code/model/hasbranches.py	2010-04-13 15:21:36 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'HasBranchesMixin',
+    'HasCodeImportsMixin',
     'HasMergeProposalsMixin',
     'HasRequestedReviewsMixin',
     ]
@@ -16,6 +17,7 @@
 from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
 from lp.code.interfaces.branchcollection import (
     IAllBranches, IBranchCollection)
+from lp.code.interfaces.branchtarget import IBranchTarget
 
 
 class HasBranchesMixin:
@@ -59,5 +61,17 @@
 
         visible_branches = getUtility(IAllBranches).visibleByUser(
             visible_by_user)
-        proposals = visible_branches.getMergeProposalsForReviewer(self, status)
+        proposals = visible_branches.getMergeProposalsForReviewer(
+            self, status)
         return proposals
+
+
+class HasCodeImportsMixin:
+
+    def newCodeImport(self, registrant=None, branch_name=None,
+            rcs_type=None, url=None, cvs_root=None, cvs_module=None,
+            owner=None):
+        """See `IHasCodeImports`."""
+        return IBranchTarget(self).newCodeImport(registrant, branch_name,
+                rcs_type, url=url, cvs_root=cvs_root, cvs_module=cvs_module,
+                owner=owner)

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2010-04-13 15:21:34 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2010-04-13 15:21:36 +0000
@@ -16,6 +16,7 @@
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportjob import CodeImportJob, CodeImportJobSet
 from lp.code.model.codeimportresult import CodeImportResult
+from lp.code.interfaces.branch import BranchCreatorNotMemberOfOwnerTeam
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.registry.interfaces.person import IPersonSet
 from lp.code.enums import (
@@ -155,6 +156,45 @@
         # And a job is still created
         self.assertIsNot(None, code_import.import_job)
 
+    def test_set_owner(self):
+        """Test that we can create an import owned by someone else."""
+        registrant = self.factory.makePerson()
+        owner = self.factory.makeTeam()
+        removeSecurityProxy(registrant).join(owner)
+        source_package = self.factory.makeSourcePackage()
+        target = IBranchTarget(source_package)
+        code_import = CodeImportSet().new(
+            registrant=registrant,
+            target=target,
+            branch_name='imported',
+            rcs_type=RevisionControlSystems.HG,
+            url=self.factory.getUniqueURL(),
+            review_status=None, owner=owner)
+        code_import = removeSecurityProxy(code_import)
+        self.assertEqual(registrant, code_import.registrant)
+        self.assertEqual(owner, code_import.branch.owner)
+        self.assertEqual(registrant, code_import.branch.registrant)
+        self.assertEqual(target, code_import.branch.target)
+        self.assertEqual(source_package, code_import.branch.sourcepackage)
+        # And a job is still created
+        self.assertIsNot(None, code_import.import_job)
+
+    def test_registrant_must_be_in_owner(self):
+        """Test that we can't create an import for an arbitrary team."""
+        registrant = self.factory.makePerson()
+        owner = self.factory.makeTeam()
+        source_package = self.factory.makeSourcePackage()
+        target = IBranchTarget(source_package)
+        self.assertRaises(
+            BranchCreatorNotMemberOfOwnerTeam,
+            CodeImportSet().new,
+            registrant=registrant,
+            target=target,
+            branch_name='imported',
+            rcs_type=RevisionControlSystems.HG,
+            url=self.factory.getUniqueURL(),
+            review_status=None, owner=owner)
+
 
 class TestCodeImportDeletion(TestCaseWithFactory):
     """Test the deletion of CodeImports."""

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2010-04-12 15:53:56 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2010-04-13 15:21:36 +0000
@@ -84,7 +84,7 @@
     ...     initial_comment='Merge\nit!', needs_review=True,
     ...     commit_message='It was merged!\n', reviewers=[reviewer_url],
     ...     review_types=['green'])
-    HTTP/1.1 500 Internal Server Error ...
+    HTTP/1.1 400 Bad Request ...
     BranchMergeProposalExists: There is already a branch merge proposal
     registered for branch ... to land on ... that is still active.
 

=== modified file 'lib/lp/code/stories/webservice/xx-code-import.txt'
--- lib/lp/code/stories/webservice/xx-code-import.txt	2010-04-13 15:21:34 +0000
+++ lib/lp/code/stories/webservice/xx-code-import.txt	2010-04-13 15:21:36 +0000
@@ -5,24 +5,33 @@
 if it is an import branch.
 
     >>> from zope.security.proxy import removeSecurityProxy
+    >>> from canonical.launchpad.testing.pages import webservice_for_person
+    >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
 
 First we create some objects for use in the tests.
 
     >>> login(ANONYMOUS)
     >>> person = factory.makePerson(name='import-owner')
+    >>> team = factory.makeTeam(name='import-owner-team')
+    >>> other_team = factory.makeTeam(name='other-team')
+    >>> other_person = factory.makePerson(name='other-person')
+    >>> removeSecurityProxy(person).join(team)
     >>> product = factory.makeProduct(name='scruff')
+    >>> svn_branch_url = "http://svn.domain.com/source"
     >>> code_import = removeSecurityProxy(factory.makeProductCodeImport(
     ...    registrant=person, product=product, branch_name='import',
-    ...    svn_branch_url="http://svn.domain.com/source"))
+    ...    svn_branch_url=svn_branch_url))
     >>> no_import_branch = removeSecurityProxy(factory.makeProductBranch(
     ...     owner=person, product=product, name='no-import'))
     >>> logout()
+    >>> import_webservice = webservice_for_person(
+    ...     person, permission=OAuthPermission.WRITE_PUBLIC)
 
 If we query a branch with no import then we find that it tells us
 it doesn't have one.
 
     >>> branch_url = '/' + no_import_branch.unique_name
-    >>> response = webservice.get(branch_url)
+    >>> response = import_webservice.get(branch_url)
     >>> representation = response.jsonBody()
     >>> print representation['code_import_link']
     None
@@ -31,7 +40,7 @@
 representation.
 
     >>> branch_url = '/' + code_import.branch.unique_name
-    >>> response = webservice.get(branch_url)
+    >>> response = import_webservice.get(branch_url)
     >>> representation = response.jsonBody()
     >>> print representation['code_import_link']
     http://.../~import-owner/scruff/import/+code-import
@@ -39,7 +48,7 @@
 We can get some information about the import using this URL.
 
     >>> import_url = representation['code_import_link']
-    >>> response = webservice.get(import_url)
+    >>> response = import_webservice.get(import_url)
     >>> representation = response.jsonBody()
     >>> print representation['self_link'] == import_url
     True
@@ -69,17 +78,19 @@
     >>> distroseries = factory.makeDistroSeries(
     ...     name='manic', distribution=distribution)
     >>> source_package = factory.makeSourcePackage(
-    ...     sourcename='scruff', distroseries=distroseries)
+    ...     sourcepackagename='scruff', distroseries=distroseries)
     >>> code_import = removeSecurityProxy(factory.makePackageCodeImport(
     ...    registrant=person, sourcepackage=source_package,
     ...    branch_name='import',
     ...    svn_branch_url="http://svn.domain.com/package_source"))
     >>> logout()
+    >>> import_webservice = webservice_for_person(
+    ...     person, permission=OAuthPermission.WRITE_PUBLIC)
 
 There is a link on the branch object
 
     >>> branch_url = '/' + code_import.branch.unique_name
-    >>> response = webservice.get(branch_url)
+    >>> response = import_webservice.get(branch_url)
     >>> representation = response.jsonBody()
     >>> print representation['code_import_link']
     http://.../~import-owner/scruffbuntu/manic/scruff/import/+code-import
@@ -87,7 +98,7 @@
 and there is information available about the import itsef.
 
     >>> import_url = representation['code_import_link']
-    >>> response = webservice.get(import_url)
+    >>> response = import_webservice.get(import_url)
     >>> representation = response.jsonBody()
     >>> print representation['self_link'] == import_url
     True
@@ -106,3 +117,114 @@
     >>> print representation['date_last_successful']
     None
 
+== Creating Imports ==
+
+We can create an import using the API by calling a method on the project.
+
+    >>> product_url = '/' + product.name
+    >>> new_remote_url = factory.getUniqueURL()
+    >>> response = import_webservice.named_post(product_url, 'newCodeImport',
+    ...    branch_name='new-import', rcs_type='Git',
+    ...    url=new_remote_url)
+    >>> print response.status
+    201
+    >>> location = response.getHeader('Location')
+    >>> response = import_webservice.get(location)
+    >>> representation = response.jsonBody()
+    >>> print representation['self_link']
+    http://.../~import-owner/scruff/new-import/+code-import
+    >>> print representation['branch_link']
+    http://.../~import-owner/scruff/new-import
+    >>> print representation['rcs_type']
+    Git
+    >>> print representation['url'] == new_remote_url
+    True
+    >>> print representation['cvs_root']
+    None
+    >>> print representation['cvs_module']
+    None
+    >>> print representation['date_last_successful']
+    None
+
+If we must we can create a CVS import.
+
+    >>> product_url = '/' + product.name
+    >>> new_remote_url = factory.getUniqueURL()
+    >>> response = import_webservice.named_post(product_url, 'newCodeImport',
+    ...    branch_name='cvs-import', rcs_type='Concurrent Versions System',
+    ...    cvs_root=new_remote_url, cvs_module="foo")
+    >>> print response.status
+    201
+    >>> location = response.getHeader('Location')
+    >>> response = import_webservice.get(location)
+    >>> representation = response.jsonBody()
+    >>> print representation['self_link']
+    http://.../~import-owner/scruff/cvs-import/+code-import
+    >>> print representation['branch_link']
+    http://.../~import-owner/scruff/cvs-import
+    >>> print representation['rcs_type']
+    Concurrent Versions System
+    >>> print representation['url']
+    None
+    >>> print representation['cvs_root'] == new_remote_url
+    True
+    >>> print representation['cvs_module'] == "foo"
+    True
+    >>> print representation['date_last_successful']
+    None
+
+We can also create an import targetting a source package.
+
+    >>> source_package_url = (
+    ...     '/' + distribution.name + '/' + distroseries.name + '/+source/'
+    ...     + source_package.name)
+    >>> new_remote_url = factory.getUniqueURL()
+    >>> response = import_webservice.named_post(source_package_url,
+    ...    'newCodeImport', branch_name='new-import', rcs_type='Mercurial',
+    ...    url=new_remote_url)
+    >>> print response.status
+    201
+    >>> location = response.getHeader('Location')
+    >>> response = import_webservice.get(location)
+    >>> representation = response.jsonBody()
+    >>> print representation['self_link']
+    http://.../~import-owner/scruffbuntu/manic/scruff/new-import/+code-import
+    >>> print representation['branch_link']
+    http://.../~import-owner/scruffbuntu/manic/scruff/new-import
+    >>> print representation['rcs_type']
+    Mercurial
+    >>> print representation['url'] == new_remote_url
+    True
+    >>> print representation['cvs_root']
+    None
+    >>> print representation['cvs_module']
+    None
+    >>> print representation['date_last_successful']
+    None
+
+If we wish to create a branch owned by a team we are part of then we can.
+
+    >>> team_url = import_webservice.getAbsoluteUrl('/~import-owner-team')
+    >>> new_remote_url = factory.getUniqueURL()
+    >>> response = import_webservice.named_post(product_url, 'newCodeImport',
+    ...    branch_name='team-import', rcs_type='Git',
+    ...    url=new_remote_url, owner=team_url)
+    >>> print response.status
+    201
+    >>> location = response.getHeader('Location')
+    >>> response = import_webservice.get(location)
+    >>> representation = response.jsonBody()
+    >>> print representation['self_link']
+    http://.../~import-owner-team/scruff/team-import/+code-import
+    >>> print representation['branch_link']
+    http://.../~import-owner-team/scruff/team-import
+    >>> print representation['rcs_type']
+    Git
+    >>> print representation['url'] == new_remote_url
+    True
+    >>> print representation['cvs_root']
+    None
+    >>> print representation['cvs_module']
+    None
+    >>> print representation['date_last_successful']
+    None

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2010-04-07 19:23:28 +0000
+++ lib/lp/registry/interfaces/product.py	2010-04-13 15:21:36 +0000
@@ -43,7 +43,8 @@
 from lp.app.interfaces.headings import IRootContext
 from lp.code.interfaces.branchvisibilitypolicy import (
     IHasBranchVisibilityPolicy)
-from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals
+from lp.code.interfaces.hasbranches import (
+    IHasBranches, IHasCodeImports, IHasMergeProposals)
 from lp.code.interfaces.hasrecipes import IHasRecipes
 from lp.bugs.interfaces.bugtarget import (
     IBugTarget, IOfficialBugTagTargetPublic, IOfficialBugTagTargetRestricted)
@@ -339,7 +340,8 @@
     IHasLogo, IHasMentoringOffers, IHasMergeProposals, IHasMilestones,
     IHasMugshot, IHasOwner, IHasSecurityContact, IHasSprints,
     ITranslationPolicy, IKarmaContext, ILaunchpadUsage, IMakesAnnouncements,
-    IOfficialBugTagTargetPublic, IPillar, ISpecificationTarget, IHasRecipes):
+    IOfficialBugTagTargetPublic, IPillar, ISpecificationTarget, IHasRecipes,
+    IHasCodeImports):
     """Public IProduct properties."""
 
     # XXX Mark Shuttleworth 2004-10-12: Let's get rid of ID's in interfaces

=== modified file 'lib/lp/registry/interfaces/sourcepackage.py'
--- lib/lp/registry/interfaces/sourcepackage.py	2010-03-02 15:30:18 +0000
+++ lib/lp/registry/interfaces/sourcepackage.py	2010-04-13 15:21:36 +0000
@@ -22,7 +22,8 @@
 
 from canonical.launchpad import _
 from lp.bugs.interfaces.bugtarget import IBugTarget, IHasOfficialBugTags
-from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals
+from lp.code.interfaces.hasbranches import (
+    IHasBranches, IHasCodeImports, IHasMergeProposals)
 from lp.soyuz.interfaces.component import IComponent
 from lazr.restful.fields import Reference, ReferenceChoice
 from lazr.restful.declarations import (
@@ -32,7 +33,7 @@
 
 
 class ISourcePackage(IBugTarget, IHasBranches, IHasMergeProposals,
-                     IHasOfficialBugTags):
+                     IHasOfficialBugTags, IHasCodeImports):
     """A SourcePackage. See the MagicSourcePackage specification. This
     interface preserves as much as possible of the old SourcePackage
     interface from the SourcePackage table, with the new table-less

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2010-04-09 19:11:36 +0000
+++ lib/lp/registry/model/product.py	2010-04-13 15:21:36 +0000
@@ -33,7 +33,8 @@
 from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.code.model.branchvisibilitypolicy import (
     BranchVisibilityPolicyMixin)
-from lp.code.model.hasbranches import HasBranchesMixin, HasMergeProposalsMixin
+from lp.code.model.hasbranches import (
+    HasBranchesMixin, HasCodeImportsMixin, HasMergeProposalsMixin)
 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.bugs.interfaces.bugtarget import IHasBugHeat
@@ -173,8 +174,7 @@
               HasAliasMixin, StructuralSubscriptionTargetMixin,
               HasMilestonesMixin, OfficialBugTagTargetMixin, HasBranchesMixin,
               HasCustomLanguageCodesMixin, HasMergeProposalsMixin,
-              HasBugHeatMixin):
-
+              HasBugHeatMixin, HasCodeImportsMixin):
     """A Product."""
 
     implements(

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2010-04-12 08:29:02 +0000
+++ lib/lp/registry/model/sourcepackage.py	2010-04-13 15:21:36 +0000
@@ -23,7 +23,8 @@
 from canonical.lazr.utils import smartquote
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.code.model.branch import Branch
-from lp.code.model.hasbranches import HasBranchesMixin, HasMergeProposalsMixin
+from lp.code.model.hasbranches import (
+    HasBranchesMixin, HasCodeImportsMixin, HasMergeProposalsMixin)
 from lp.bugs.interfaces.bugtarget import IHasBugHeat
 from lp.bugs.model.bug import get_bug_tags_open_count
 from lp.bugs.model.bugtarget import BugTargetBase, HasBugHeatMixin
@@ -160,7 +161,7 @@
 class SourcePackage(BugTargetBase, SourcePackageQuestionTargetMixin,
                     HasTranslationImportsMixin, HasTranslationTemplatesMixin,
                     HasBranchesMixin, HasMergeProposalsMixin,
-                    HasBugHeatMixin):
+                    HasBugHeatMixin, HasCodeImportsMixin):
     """A source package, e.g. apache2, in a distroseries.
 
     This object is not a true database object, but rather attempts to
@@ -681,7 +682,6 @@
         our_format = PackageUploadCustomFormat.ROSETTA_TRANSLATIONS
 
         packagename = self.sourcepackagename.name
-        displayname = self.displayname
         distro = self.distroseries.distribution
 
         histories = distro.main_archive.getPublishedSources(