Launchpad itself

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

register-code-import
Merge into devel

Proposed by James Westby on 2010-04-01

Status:	Merged
Approved by:	Aaron Bentley on 2010-04-08
Approved revision:	no longer in the source branch.
Merged at revision:	not available
Proposed branch:	lp:~james-w/launchpad/register-code-import
Merge into:	lp:launchpad
Prerequisite:	lp:~james-w/launchpad/code-imports-for-source-packages
Diff against target:	732 lines (+302/-35) 18 files modified lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+5/-1) lib/lp/code/errors.py (+1/-0) lib/lp/code/interfaces/branch.py (+8/-1) lib/lp/code/interfaces/branchtarget.py (+3/-1) lib/lp/code/interfaces/codeimport.py (+5/-1) lib/lp/code/interfaces/hasbranches.py (+49/-5) lib/lp/code/interfaces/webservice.py (+6/-1) lib/lp/code/mail/tests/test_codeimport.py (+1/-1) lib/lp/code/model/branchtarget.py (+20/-2) lib/lp/code/model/codeimport.py (+6/-3) lib/lp/code/model/hasbranches.py (+15/-1) lib/lp/code/model/tests/test_codeimport.py (+40/-0) lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+1/-1) lib/lp/code/stories/webservice/xx-code-import.txt (+129/-7) lib/lp/registry/interfaces/product.py (+4/-2) lib/lp/registry/interfaces/sourcepackage.py (+3/-2) lib/lp/registry/model/product.py (+3/-3) lib/lp/registry/model/sourcepackage.py (+3/-3)
To merge this branch:	bzr merge lp:~james-w/launchpad/register-code-import
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Aaron Bentley (community)		2010-04-01	Approve on 2010-04-08
Review via email: mp+22668@code.launchpad.net

Commit message

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

Description of the change

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

Revision history for this message

James Westby (james-w) wrote on 2010-04-04:

Passes ec2, so requesting a review.

Thanks,

James

Revision history for this message

Aaron Bentley (abentley) wrote on 2010-04-05:

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

Revision history for this message

James Westby (james-w) wrote on 2010-04-05:

> 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

Revision history for this message

Aaron Bentley (abentley) wrote on 2010-04-06:

Download full text (14.5 KiB)

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'

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.

== 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/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] 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'),
     ^
     description=_('If True the proposal needs review.'
     'Otherwise, it will be work in progress.')),
     initial_comment=Text(
     title=_('Initial comment'),
     description=_("Registrant's initial description of proposal.")),
     commit_message=Text(
     title=_('Commit message'),
     description=_('Message to use when committing this merge.')),
     reviewers=List(value_type=Reference(schema=IPerson)),
     review_types=List(value_type=TextLine())
     )

@call_with(registrant=REQUEST_USER)

@export_factory_operation(Interface, [])
     def _createMergeProposal(
     registrant, target_branch, prerequisite_branch=None,
     needs_review=True, initial_comment=None, commit_message=None,
     reviewers=None, review_types=None):
     937: [C0322, IBranch.subscribe] Operator not preceded by a space
     schema=IPerson),
     ^
     notification_level=Choice(
     title=_("The level of notification to subscribe to."),
     vocabulary=BranchSubscriptionNotificationLevel),
     max_diff_lines=Choice(
     title=_("The max number of lines for diff email."),
     vocabulary=BranchSubscriptionDiffSize),
     code_review_level=Choice(
     title=_("The level of code review notification emails."),
     vocabulary=CodeReviewNotificationLevel))
     @operation_returns_entry(Interface) # Really IBranchSubscription
     @export_write_operation()
     def subscribe(person, notification_level, max_diff_lines,
     code_review_level):
     965: [C0322, IBranch.getSubscription] Operator not preceded by a space
     schema=IPerson))
     ^
     @operation_returns_entry(Interface) # Really IBranchSubscription
     @export_read_operation()
     def getSubscription(person):
     976: [C0322, IBranch.unsubscribe] Operator not preceded by a space
     title=_("The person to unsubscribe"),
     ^
     schema=IPerson))
     @export_write_operation()
     def unsubscribe(person):
     1226: [C0322, IBranchSet.getByUrls] Operator not preceded by a space
     title=u'A list of URLs of branches',
     ^
     description=(
     u'These can be URLs external to '
     u'Launchpad, lp: URLs, or http://bazaar.launchpad.net/ URLs, '
     u'or any mix of all these different kinds.'),
     value_type=TextLine(),
     required=True))
     @export_read_operation()
     def getByUrls(urls):

lib/lp/code/interfaces/branchtarget.py
     21: [F0401] Unable to import 'zope.interface'
     22: [F0401] Unable to import 'zope.security.interfaces'
     27: [F0401] Unable to import 'lazr.restful.fields'

lib/lp/code/interfaces/codeimport.py
     17: [F0401] Unable to import 'zope.interface'
     18: [F0401] Unable to import 'zope.schema'
     27: [F0401] Unable to import 'lazr.restful.declarations'
     29: [F0401] Unable to import 'lazr.restful.fields'

lib/lp/code/interfaces/hasbranches.py
     104: [C0301] Line too long (79/78)
     15: [F0401] Unable to import 'zope.interface'
     16: [F0401] Unable to import 'zope.schema'
     22: [F0401] Unable to import 'lazr.restful.declarations'
     25: [F0401] Unable to import 'lazr.restful.fields'
     42: [E0213, IHasBranches.getBranches] Method should have "self" as 
first argument
     42: [C0322, IHasBranches.getBranches] Operator not preceded by a space
     modified_since=Datetime(
     ^
     title=_('Limit the branches to those modified since this date.'),
     required=False))
     @call_with(visible_by_user=REQUEST_USER)
     @operation_returns_collection_of(Interface) # Really IBranch.
     @export_read_operation()
     def getBranches(status=None, visible_by_user=None,
     modified_since=None):
     74: [E0213, IHasMergeProposals.getMergeProposals] Method should 
have "self" as first argument
     100: [E0213, IHasRequestedReviews.getRequestedReviews] Method 
should have "self" as first argument
     128: [E0213, IHasCodeImports.newCodeImport] Method should have 
"self" as first argument
     128: [C0322, IHasCodeImports.newCodeImport] Operator not preceded 
by a space
     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):

lib/lp/code/interfaces/webservice.py
     13: [W0611] Unused import IBranchSubscription
     9: [W0611] Unused import IBranchSet
     9: [W0611] Unused import BranchCreatorNotOwner
     17: [W0611] Unused import IDiff
     16: [W0611] Unused import ICodeReviewVoteReference
     17: [W0611] Unused import IStaticDiff
     12: [W0611] Unused import IBranchMergeProposal
     9: [W0611] Unused import IBranch
     17: [W0611] Unused import IPreviewDiff
     8: [W0611] Unused import BranchMergeProposalExists
     9: [W0611] Unused import BranchExists
     9: [W0611] Unused import BranchCreatorNotMemberOfOwnerTeam
     14: [W0611] Unused import ICodeImport
     15: [W0611] Unused import ICodeReviewComment

lib/lp/code/model/branchtarget.py
     15: [F0401] Unable to import 'zope.component'
     16: [F0401] Unable to import 'zope.interface'
     17: [F0401] Unable to import 'zope.security.proxy'
     328: [W0231, ProductSeriesBranchTarget.__init__] __init__ method 
from base class 'ProductBranchTarget' is not called

lib/lp/code/model/codeimport.py
     23: [F0401] Unable to import 'zope.component'
     24: [F0401] Unable to import 'zope.event'
     25: [F0401] Unable to import 'zope.interface'
     27: [F0401] Unable to import 'lazr.lifecycle.event'

lib/lp/code/model/hasbranches.py
     64: [C0301] Line too long (79/78)
     14: [F0401] Unable to import 'zope.component'

lib/lp/code/model/tests/test_codeimport.py
     12: [F0401] Unable to import 'zope.component'
     13: [F0401] Unable to import 'zope.security.proxy'

lib/lp/registry/interfaces/product.py
     30: [F0401] Unable to import 'zope.interface'
     31: [F0401] Unable to import 'zope.schema'
     33: [F0401] Unable to import 'zope.schema.vocabulary'
     34: [F0401] Unable to import 'lazr.enum'
     73: [F0401] Unable to import 'lazr.restful.fields'
     74: [F0401] Unable to import 'lazr.restful.declarations'
     774: [C0322, IProductSet.createProduct] Operator not preceded by a 
space
     freshmeatproject='freshmeat_project', wikiurl='wiki_url',
     ^
     downloadurl='download_url',
     sourceforgeproject='sourceforge_project',
     programminglang='programming_lang')
     @export_factory_operation(
     IProduct, ['name', 'displayname', 'title', 'summary', 'description',
     'project', 'homepageurl', 'screenshotsurl',
     'downloadurl', 'freshmeatproject', 'wikiurl',
     'sourceforgeproject', 'programminglang',
     'license_reviewed', 'licenses', 'license_info',
     'registrant'])
     @export_operation_as('new_project')
     def createProduct(owner, name, displayname, title, summary,
     description=None, project=None, homepageurl=None,
     screenshotsurl=None, wikiurl=None,
     downloadurl=None, freshmeatproject=None,
     sourceforgeproject=None, programminglang=None,
     license_reviewed=False, mugshot=None, logo=None,
     icon=None, licenses=None, license_info=None,
     registrant=None):

lib/lp/registry/interfaces/sourcepackage.py
     19: [F0401] Unable to import 'zope.interface'
     20: [F0401] Unable to import 'zope.schema'
     21: [F0401] Unable to import 'lazr.enum'
     28: [F0401] Unable to import 'lazr.restful.fields'
     29: [F0401] Unable to import 'lazr.restful.declarations'
     189: [C0322, ISourcePackage.getBranch] Operator not preceded by a space
     vocabulary=DBEnumeratedType))
     ^

@operation_returns_entry(Interface)
     @export_read_operation()
     def getBranch(pocket):
     207: [C0322, ISourcePackage.setBranch] Operator not preceded by a space
     vocabulary=DBEnumeratedType),
     ^
     branch=Reference(Interface, title=_("Branch"), required=False))
     @call_with(registrant=REQUEST_USER)
     @export_write_operation()
     def setBranch(pocket, branch, registrant):

lib/lp/registry/model/product.py
     22: [F0401] Unable to import 'zope.interface'
     23: [F0401] Unable to import 'zope.component'
     24: [F0401] Unable to import 'zope.security.proxy'
     27: [F0401] Unable to import 'lazr.delegates'

lib/lp/registry/model/sourcepackage.py
     15: [F0401] Unable to import 'zope.interface'
     16: [F0401] Unable to import 'zope.component'

Some of these may be bogus, but many of them should be fixed.  You 
should suppress the pylint warnings in lib/lp/code/interfaces/hasbranches.py

>> 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.

What works in doctests will usually work in unit tests:

=== added file 'lib/lp/code/tests/test_imports.py'
--- lib/lp/code/tests/test_imports.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/tests/test_imports.py	2010-04-06 15:24:50 +0000
@@ -0,0 +1,39 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for code import API."""
+
+__metaclass__ = type
+
+
+from unittest import TestLoader
+
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.launchpad.testing.pages import webservice_for_person
+from canonical.launchpad.webapp.interfaces import OAuthPermission
+from lp.testing import ANONYMOUS, login, logout, TestCaseWithFactory
+
+
+class TestCodeImportAPI(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_cannot_create_other_team_import(self):
+        login(ANONYMOUS)
+        person = self.factory.makePerson(name='import-owner')
+        other_team = self.factory.makeTeam(name='other-team')
+        product = self.factory.makeProduct(name='scruff')
+        product_url = '/' + product.name
+        new_remote_url = self.factory.getUniqueURL()
+        logout()
+        import_webservice = webservice_for_person(
+            person, permission=OAuthPermission.WRITE_PUBLIC)
+        other_team_url = import_webservice.getAbsoluteUrl('/~other-team')
+        result = import_webservice.named_post(
+            product_url, 'newCodeImport', branch_name='other-team-import',
+            rcs_type='Git', url=new_remote_url, owner=other_team_url)
+        self.assertEqual(400, result.status)
+
+
+def test_suite():
+    return TestLoader().loadTestsFromName(__name__)

Aaron

Revision history for this message

James Westby (james-w) wrote on 2010-04-07:

Download full text (9.6 KiB)

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.

On Tue, 06 Apr 2010 15:33:31 -0000, Aaron Bentley <aaron@canonical.com> 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'),
>      ^
>      description=_('If True the proposal needs review.'
>      'Otherwise, it will be work in progress.')),
>      initial_comment=Text(
>      title=_('Initial comment'),
>      description=_("Registrant's initial description of proposal.")),
>      commit_message=Text(
>      title=_('Commit message'),
>      description=_('Message to use when committing this merge.')),
>      reviewers=List(value_type=Reference(schema=IPerson)),
>      review_types=List(value_type=TextLine())
>      )
> 
> 
>      @call_with(registrant=REQUEST_USER)
> 
>      @export_factory_operation(Interface, [])
>      def _createMergeProposal(
>      registrant, target_branch, prerequisite_branch=None,
>      needs_review=True, initial_comment=None, commit_message=None,
>      reviewers=None, review_types=None):
>      937: [C0322, IBranch.subscribe] Operator not preceded by a space
>      schema=IPerson),
>      ^
>      notification_level=Choice(
>      title=_("The level of notification to subscribe to."),
>      vocabulary=BranchSubscriptionNotificationLevel),
>      max_diff_lines=Choice(
>      title=_("The max number of lines for diff email."),
>      vocabulary=BranchSubscriptionDiffSize),
>      code_review_level=Choice(
>      title=_("The level of code review notification emails."),
>      vocabulary=CodeReviewNotificationLevel))
>      @operation_returns_entry(Interface) # Really IBranchSubscription
>      @export_write_operation()
>      def subscribe(person, notification_level, max_diff_lines,
>      code_review_level):
>      965: [C0322, IBranch.getSubscription] Operator not preceded by a space
>      schema=IPerson))
>      ^
>      @operation_returns_entry(Interface) # Really IBranchSubscription
>      @export_read_operation()
>      def getSubscription(person):
>      976: [C0322, IBranch.unsubscribe] Operator not preceded by a space
>      title=_("The person to unsubscribe"),
>      ^
>      schema=IPerson))
>      @export_write_operation()
>      def unsubscribe(person):
>      1226: [C0322, IBranchSet.getByUrls] Operator not preceded by a space
>      title=u'A list of URLs of branches',
>      ^
>      description=(
>      u'These can be URLs external to '
>      u'Launchpad, lp: URLs, or http://bazaar.launchpad.net/ URLs, '
>      u'or any mix of all these different kinds.'),
>      value_type=TextLine(),
>      required=True))
>      @export_read_operation()
>      def getByUrls(urls):

Noise.

> lib/lp/code/interfaces/hasbranches.py
>      104: [C0301] Line too long (79/78)

Fixed.

>      42: [E0213, IHasBranches.getBranches] Method should have "self" as 
> first argument
>      42: [C0322, IHasBranches.getBranches] Operator not preceded by a space
>      modified_since=Datetime(
>      ^
>      title=_('Limit the branches to those modified since this date.'),
>      required=False))
>      @call_with(visible_by_user=REQUEST_USER)
>      @operation_returns_collection_of(Interface) # Really IBranch.
>      @export_read_operation()
>      def getBranches(status=None, visible_by_user=None,
>      modified_since=None):
>      74: [E0213, IHasMergeProposals.getMergeProposals] Method should 
> have "self" as first argument
>      100: [E0213, IHasRequestedReviews.getRequestedReviews] Method 
> should have "self" as first argument
>      128: [E0213, IHasCodeImports.newCodeImport] Method should have 
> "self" as first argument
>      128: [C0322, IHasCodeImports.newCodeImport] Operator not preceded 
> by a space
>      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):

Noise.

> lib/lp/code/model/branchtarget.py
>      328: [W0231, ProductSeriesBranchTarget.__init__] __init__ method 
> from base class 'ProductBranchTarget' is not called

Fixed.

> lib/lp/code/model/hasbranches.py
>      64: [C0301] Line too long (79/78)

Fixed

> lib/lp/registry/interfaces/product.py
>      774: [C0322, IProductSet.createProduct] Operator not preceded by a 
> space
>      freshmeatproject='freshmeat_project', wikiurl='wiki_url',
>      ^
>      downloadurl='download_url',
>      sourceforgeproject='sourceforge_project',
>      programminglang='programming_lang')
>      @export_factory_operation(
>      IProduct, ['name', 'displayname', 'title', 'summary', 'description',
>      'project', 'homepageurl', 'screenshotsurl',
>      'downloadurl', 'freshmeatproject', 'wikiurl',
>      'sourceforgeproject', 'programminglang',
>      'license_reviewed', 'licenses', 'license_info',
>      'registrant'])
>      @export_operation_as('new_project')
>      def createProduct(owner, name, displayname, title, summary,
>      description=None, project=None, homepageurl=None,
>      screenshotsurl=None, wikiurl=None,
>      downloadurl=None, freshmeatproject=None,
>      sourceforgeproject=None, programminglang=None,
>      license_reviewed=False, mugshot=None, logo=None,
>      icon=None, licenses=None, license_info=None,
>      registrant=None):

Noise.

> lib/lp/registry/interfaces/sourcepackage.py
>      189: [C0322, ISourcePackage.getBranch] Operator not preceded by a space
>      vocabulary=DBEnumeratedType))
>      ^
> 
> 
>      @operation_returns_entry(Interface)
>      @export_read_operation()
>      def getBranch(pocket):
>      207: [C0322, ISourcePackage.setBranch] Operator not preceded by a space
>      vocabulary=DBEnumeratedType),
>      ^
>      branch=Reference(Interface, title=_("Branch"), required=False))
>      @call_with(registrant=REQUEST_USER)
>      @export_write_operation()
>      def setBranch(pocket, branch, registrant):

Noise.

> Some of these may be bogus, but many of them should be fixed.  You 
> should suppress the pylint warnings in lib/lp/code/interfaces/hasbranches.py

Done.

> >> 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.
> 
> What works in doctests will usually work in unit tests:

Thanks, changed.

James

Revision history for this message

Aaron Bentley (abentley) wrote on 2010-04-08:

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

James Troup

James Westby

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

 === 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(