Merge lp:~jml/launchpad/free-code-imports into lp:launchpad

[Jonathan Lange]

New code imports will be owned by the person who registered them, rather than the vcs-imports special team.

[Jonathan Lange (jml)]

Hi Michael,

In this branch, I have tried to fix bug 387110 by setting the owner of a code import branch to be the registrant of the branch, rather than ~vcs-imports.

Doing this changed the email that gets sent. I'm not entirely sure what's going on here, but hope I've done the right thing.

jml

[Michael Hudson-Doyle (mwhudson)]

Hi, this is simple enough and a good idea. I think you'll need to run the page tests and at least look at the browser code for creating code imports before this can land though.

review: Needs Fixing

[Jonathan Lange (jml)]

> Hi, this is simple enough and a good idea. I think you'll need to run the
> page tests and at least look at the browser code for creating code imports
> before this can land though.

For those following at home, here's what's happening.

I've changed my branch so that it passes _almost_ all the tests. The only one it doesn't pass is the "edit" page test. The reason is that the way that code import permissions work now is a little bit confusing.

I'll address these as soon as I have a spare moment.

[Jonathan Lange (jml)]

> > Hi, this is simple enough and a good idea. I think you'll need to run the
> > page tests and at least look at the browser code for creating code imports
> > before this can land though.
>
> For those following at home, here's what's happening.
>
> I've changed my branch so that it passes _almost_ all the tests. The only one
> it doesn't pass is the "edit" page test. The reason is that the way that code
> import permissions work now is a little bit confusing.
>
> I'll address these as soon as I have a spare moment.

Addressed!

Now, if you register the code import, you are the owner of the branch, rather than vcs-imports. I've also tweaked things so that even if the branch is owned by vcs-imports, you can edit the branch.

We actually *don't* want to give people edit permissions on the CodeImport object itself, since we use 'launchpad.Edit' permissions to mean "you are allowed to administer code imports".

jml

[Michael Hudson-Doyle (mwhudson)]

I have a couple of questions I'd like you to answer. The page tests are somewhat icky and you've added a little duplication. Remove that and I reckon you should land this.

> === modified file 'lib/lp/code/browser/branch.py'
> --- lib/lp/code/browser/branch.py 2009-10-29 16:54:10 +0000
> +++ lib/lp/code/browser/branch.py 2009-11-10 04:22:15 +0000
> @@ -177,6 +177,7 @@
> if proposal.id == id:
> return proposal
>
> +
> class BranchEditMenu(NavigationMenu):
> """Edit menu for IBranch."""
>
> @@ -206,10 +207,14 @@
> return Link(
> '+whiteboard', text, icon='edit', enabled=enabled)
>
> - @enabled_with_permission('launchpad.Edit')
> + # XXX: Rewrite the page test to be smarter about who is actually logged
> + # in, handling cases of branch owner and vcs-imports member separately.

This is fair enough but I don't see why the comment is here!

> def edit_import(self):
> text = 'Edit import source or review import'
> - enabled = self.branch_is_import()
> + enabled = (
> + self.branch_is_import() and
> + check_permission('launchpad.Edit', self.context.code_import))
> return Link(
> '+edit-import', text, icon='edit', enabled=enabled)
>

> === modified file 'lib/lp/code/doc/codeimport.txt'
> --- lib/lp/code/doc/codeimport.txt 2009-07-01 13:16:44 +0000
> +++ lib/lp/code/doc/codeimport.txt 2009-11-10 04:22:15 +0000
> @@ -94,7 +94,7 @@
> Operator @vcs-imports
> >>> print message.get_payload()
> A new code import has been requested by No Privileges Person:
> - http://code.launchpad.dev/~vcs-imports/firefox/trunk-cvs
> + http://code.launchpad.dev/~no-priv/firefox/trunk-cvs
> -- =
> You are getting this email because you are a member of the vcs-imports team.
>
> @@ -207,6 +207,24 @@
> ... nopriv)
> >>> print_emails(group_similar=True)
> From: No Privileges Person <email address hidden>
> + To: <email address hidden>, ...
> + Subject: Code import product.../name... status: Reviewed
> + <BLANKLINE>
> + The import has been approved and an import will start shortly.
> + <BLANKLINE>
> + ... is now being imported from:
> + http://svn.example.com/project/trunk
> + instead of:
> + http://svn.example.com/project
> + <BLANKLINE>
> + -- =
> + <BLANKLINE>
> + http://code.launchpad.dev/~person.../product.../name...
> + You are getting this email because you are a member of the vcs-imports
> + team.
> + <BLANKLINE>
> + ----------------------------------------

This is a bit bizarre. Oh, I think the rules for subscriptions to
code import branches are a bit unusual, maybe? I guess it would be
good to understand why this is happening before landing.

> + From: No Privileges Person <email address hidden>
> To: <email address hidden>
> Subject: Code import product.../name... status: Reviewed
> <BLANKLINE>
> @@ -218,10 +236,11 @@
> http://svn.example.com/project
> <BLANKLINE>
> -- =
> - http://code.launchpad.dev/~vcs-imports/product.../name...
> + <BLANKLINE>
> + http://co...

review: Needs Information

[Jonathan Lange (jml)]

I've fixed everything you've pointed out, but haven't done anything about the two open questions:

  1. Why do I need to do the UnauthenticatedPrincipal dance?

  2. What's going on with email?

In answer to the first, I don't know why I had to make the change, other than that the tests made me do it. Perhaps Gary or Bjorn would know why.

As for the second, we should look into it at the sprint tomorrow.

[Michael Hudson-Doyle (mwhudson)]

I answered my own questions in the end, I think the behaviour in this branch is better than trunk in fact.

review: Approve

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2009-10-20 13:21:44 +0000
+++ lib/canonical/launchpad/security.py	2009-11-11 09:00:39 +0000
@@ -1591,9 +1591,25 @@
     usedfor = IBranch
 
     def checkAuthenticated(self, user):
-        return (user.inTeam(self.obj.owner) or
-                user_has_special_branch_access(user) or
-                can_upload_linked_package(user, self.obj))
+        can_edit = (
+            user.inTeam(self.obj.owner) or
+            user_has_special_branch_access(user) or
+            can_upload_linked_package(user, self.obj))
+        if can_edit:
+            return True
+        # It used to be the case that all import branches were owned by the
+        # special, restricted team ~vcs-imports. For these legacy code import
+        # branches, we still want the code import registrant to be able to
+        # edit them. Similarly, we still want vcs-imports members to be able
+        # to edit those branches.
+        code_import = self.obj.code_import
+        if code_import is None:
+            return False
+        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
+        return (
+            user.inTeam(vcs_imports)
+            or (self.obj.owner == vcs_imports
+                and user.inTeam(code_import.registrant)))
 
 
 def can_upload_linked_package(person, branch):

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2009-10-29 16:54:10 +0000
+++ lib/lp/code/browser/branch.py	2009-11-11 09:00:39 +0000
@@ -177,6 +177,7 @@
             if proposal.id == id:
                 return proposal
 
+
 class BranchEditMenu(NavigationMenu):
     """Edit menu for IBranch."""
 
@@ -206,10 +207,11 @@
         return Link(
             '+whiteboard', text, icon='edit', enabled=enabled)
 
-    @enabled_with_permission('launchpad.Edit')
     def edit_import(self):
         text = 'Edit import source or review import'
-        enabled = self.branch_is_import()
+        enabled = (
+            self.branch_is_import() and
+            check_permission('launchpad.Edit', self.context.code_import))
         return Link(
             '+edit-import', text, icon='edit', enabled=enabled)
 

=== modified file 'lib/lp/code/doc/codeimport.txt'
--- lib/lp/code/doc/codeimport.txt	2009-07-01 13:16:44 +0000
+++ lib/lp/code/doc/codeimport.txt	2009-11-11 09:00:39 +0000
@@ -94,7 +94,7 @@
     Operator @vcs-imports
     >>> print message.get_payload()
     A new code import has been requested by No Privileges Person:
-        http://code.launchpad.dev/~vcs-imports/firefox/trunk-cvs
+        http://code.launchpad.dev/~no-priv/firefox/trunk-cvs
     -- =
     You are getting this email because you are a member of the vcs-imports team.
 
@@ -207,6 +207,24 @@
     ...     nopriv)
     >>> print_emails(group_similar=True)
     From: No Privileges Person <[e-mail]>
+    To: [e-mail], ...
+    Subject: Code import product.../name... status: Reviewed
+    <BLANKLINE>
+    The import has been approved and an import will start shortly.
+    <BLANKLINE>
+    ... is now being imported from:
+        http://svn.example.com/project/trunk
+    instead of:
+        http://svn.example.com/project
+    <BLANKLINE>
+    -- =
+    <BLANKLINE>
+    http://code.launchpad.dev/~person.../product.../name...
+    You are getting this email because you are a member of the vcs-imports
+    team.
+    <BLANKLINE>
+    ----------------------------------------
+    From: No Privileges Person <[e-mail]>
     To: [e-mail]
     Subject: Code import product.../name... status: Reviewed
     <BLANKLINE>
@@ -218,10 +236,11 @@
         http://svn.example.com/project
     <BLANKLINE>
     -- =
-    http://code.launchpad.dev/~vcs-imports/product.../name...
+    <BLANKLINE>
+    http://code.launchpad.dev/~person.../product.../name...
     You are receiving this email as you are subscribed to the branch.
-    To unsubscribe from this branch go to http://code.launchpad.dev/~vcs-import=
-    s/product.../name.../+edit-subscription.
+    To unsubscribe from this branch go to .../+edit-subscription.
+    <BLANKLINE>
     ----------------------------------------
 
 updateFromData is smart enough to not send an email if no changes were
@@ -239,6 +258,10 @@
     ...     None)
     >>> print_emails(group_similar=True)
     From: [e-mail]
+    To: [e-mail], ...
+    Subject: Code import product.../name... status: Reviewed
+    ...
+    From: [e-mail]
     To: [e-mail]
     Subject: Code import product.../name... status: Reviewed
     ...
@@ -351,7 +374,7 @@
 and are not traversable in themselves.
 
     >>> print canonical_url(svn_import.branch)
-    http://code.launchpad.dev/~vcs-imports/firefox/trunk-svn
+    http://code.launchpad.dev/~no-priv/firefox/trunk-svn
     >>> print canonical_url(svn_import)
     Traceback (most recent call last):
     NoCanonicalUrl: ...
@@ -410,7 +433,7 @@
     >>> modify_event = cvs_import.updateFromData(data, nopriv)
     >>> print make_email_body_for_code_import_update(
     ...     cvs_import, modify_event, None)
-    ~vcs-imports/firefox/trunk-cvs is now being imported from:
+    ~no-priv/firefox/trunk-cvs is now being imported from:
         hello from :pserver:[e-mail]:/var/cvsroot
     instead of:
         hello from :pserver:[e-mail]:/cvsroot
@@ -421,7 +444,7 @@
     >>> modify_event = git_import.updateFromData(data, nopriv)
     >>> print make_email_body_for_code_import_update(
     ...     git_import, modify_event, None)
-    ~vcs-imports/firefox/trunk-git is now being imported from:
+    ~no-priv/firefox/trunk-git is now being imported from:
         git://git.example.com/goodbye.git
     instead of:
         git://git.example.com/hello.git

=== modified file 'lib/lp/code/mail/codeimport.py'
--- lib/lp/code/mail/codeimport.py	2009-07-17 00:26:05 +0000
+++ lib/lp/code/mail/codeimport.py	2009-11-11 09:00:39 +0000
@@ -8,6 +8,7 @@
 import textwrap
 
 from zope.component import getUtility
+from zope.app.security.interfaces import IUnauthenticatedPrincipal
 
 from canonical.config import config
 from canonical.launchpad.helpers import (
@@ -23,11 +24,11 @@
 
 def new_import(code_import, event):
     """Email the vcs-imports team about a new code import."""
-    if event.user is None:
+    if (event.user is None
+        or IUnauthenticatedPrincipal.providedBy(event.user)):
         # If there is no logged in user, then we are most likely in a
         # test.
         return
-
     user = IPerson(event.user)
     subject = 'New code import: %s/%s' % (
         code_import.product.name, code_import.branch.name)

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2009-10-30 22:56:45 +0000
+++ lib/lp/code/model/branch.py	2009-11-11 09:00:39 +0000
@@ -591,8 +591,6 @@
         series_set = getUtility(IFindOfficialBranchLinks)
         alteration_operations.extend(
             map(ClearOfficialPackageBranch, series_set.findForBranch(self)))
-        if self.code_import is not None:
-            deletion_operations.append(DeleteCodeImport(self.code_import))
         return (alteration_operations, deletion_operations)
 
     def deletionRequirements(self):
@@ -623,6 +621,12 @@
             operation()
         for operation in deletion_operations:
             operation()
+        # Special-case code import, since users don't have lp.Edit on them,
+        # since if you can delete a branch you should be able to delete the
+        # code import and since deleting the code import object itself isn't
+        # actually a very interesting thing to tell the user about.
+        if self.code_import is not None:
+            DeleteCodeImport(self.code_import)()
 
     def associatedProductSeries(self):
         """See `IBranch`."""

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2009-08-31 09:52:42 +0000
+++ lib/lp/code/model/codeimport.py	2009-11-11 09:00:39 +0000
@@ -33,7 +33,6 @@
 from canonical.database.sqlbase import SQLBase, quote, sqlvalues
 from lp.code.model.codeimportjob import CodeImportJobWorkflow
 from lp.registry.model.productseries import ProductSeries
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.code.enums import (
     BranchType, CodeImportResultStatus, CodeImportReviewStatus,
@@ -238,11 +237,10 @@
             else:
                 review_status = CodeImportReviewStatus.NEW
         # Create the branch for the CodeImport.
-        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
-        namespace = get_branch_namespace(vcs_imports, product)
+        namespace = get_branch_namespace(registrant, product)
         import_branch = namespace.createBranch(
             branch_type=BranchType.IMPORTED, name=branch_name,
-            registrant=vcs_imports)
+            registrant=registrant)
 
         code_import = CodeImport(
             registrant=registrant, owner=registrant, branch=import_branch,

=== added file 'lib/lp/code/stories/codeimport/xx-admin-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	2009-11-11 09:00:39 +0000
@@ -0,0 +1,277 @@
+= Administrating code imports =
+
+The code import details are displayed on the main branch page for
+imported branches.  If the logged in user is an import operator
+(member of VCS imports or Launchpad admin) then they can see a link
+to edit the details.
+
+    >>> from canonical.launchpad.ftests import ANONYMOUS, login, logout
+    >>> login('[e-mail]')
+
+    >>> svn_import = factory.makeCodeImport(
+    ...     svn_branch_url='svn://svn.example.com/fooix/trunk')
+    >>> from canonical.launchpad.webapp import canonical_url
+    >>> svn_import_location = str(canonical_url(svn_import.branch))
+    >>> svn_import_branch_unique_name = svn_import.branch.unique_name
+
+    >>> cvs_import = factory.makeCodeImport(
+    ...     cvs_root=":pserver:[e-mail]:/fooix",
+    ...     cvs_module="fooix")
+    >>> cvs_import_location = str(canonical_url(cvs_import.branch))
+    >>> cvs_import_branch_unique_name = cvs_import.branch.unique_name
+
+    >>> git_import = factory.makeCodeImport(
+    ...     git_repo_url="git://git.example.org/fooix")
+    >>> git_import_location = str(canonical_url(git_import.branch))
+    >>> git_import_branch_unique_name = git_import.branch.unique_name
+
+    >>> import_browser = setupBrowser(
+    ...     auth='Basic [e-mail]:test')
+
+
+== Editing the import ==
+
+Both VCS Imports members and Launchpad administrators can edit the
+import:
+
+    >>> import_browser.open(svn_import_location)
+    >>> print_import_details(import_browser)
+    Import Status: Pending Review
+    This branch is an import of the Subversion branch
+    from svn://svn.example.com/fooix/trunk.
+
+
+== Editing details ==
+
+There are a number of buttons shown on the editing page for
+import operators.
+
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> print_submit_buttons(import_browser.contents)
+    Update
+    Approve
+    Mark Invalid
+    Suspend
+    Mark Failing
+
+A cancel link is also shown next to the buttons, that takes the user
+back to the main branch details page.
+
+    >>> import_browser.getLink('Cancel').url == svn_import_location
+    True
+
+Only the fields that are relevant to the type of import are shown
+in the form.
+
+    >>> def print_form_fields(browser):
+    ...     tags = find_tags_by_class(browser.contents, 'textType')
+    ...     for tag in tags:
+    ...         print '%s: %s' % (tag['name'], tag['value'])
+
+    >>> print_form_fields(import_browser)
+    field.svn_branch_url: svn://svn.example.com/fooix/trunk
+
+    >>> import_browser.open(cvs_import_location)
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> print_form_fields(import_browser)
+    field.cvs_root: :pserver:[e-mail]:/fooix
+    field.cvs_module: fooix
+
+    >>> import_browser.open(git_import_location)
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> print_form_fields(import_browser)
+    field.git_repo_url: git://git.example.org/fooix
+
+=== Editing the import location ===
+
+The +edit-import page allows the import operator to edit the location
+an import is from.
+
+This is true for Subversion imports,
+
+    >>> import_browser.open(svn_import_location + '/+edit-import')
+    >>> import_browser.getControl('Branch URL').value = \
+    ...     'svn://svn-new.example.com/fooix/trunk'
+    >>> import_browser.getControl('Update').click()
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been updated.
+
+CVS imports,
+
+    >>> import_browser.open(cvs_import_location + '/+edit-import')
+    >>> import_browser.getControl('Repository').value = \
+    ...     ':pserver:[e-mail]:/fooix'
+    >>> import_browser.getControl('Module').value = \
+    ...     'fooix2'
+    >>> import_browser.getControl('Update').click()
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been updated.
+
+and Git imports.
+
+    >>> import_browser.open(git_import_location + '/+edit-import')
+    >>> import_browser.getControl('Repo URL').value = \
+    ...     'git://git-new.example.org/fooix'
+    >>> import_browser.getControl('Update').click()
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been updated.
+
+
+=== Approving an import ===
+
+When a code import is approved, a pending job is created for it.
+
+    >>> import_browser.open(svn_import_location + '/+edit-import')
+    >>> import_browser.getControl('Approve').click()
+    >>> print_import_details(import_browser)
+    Import Status: Reviewed
+    ...
+    The next import is scheduled to run as soon as possible.
+
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been approved.
+
+
+=== Invalidating an import ===
+
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> import_browser.getControl('Mark Invalid').click()
+    >>> print_import_details(import_browser)
+    Import Status: Invalid
+    ...
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been set as invalid.
+
+
+=== Suspending an import ===
+
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> import_browser.getControl('Suspend').click()
+    >>> print_import_details(import_browser)
+    Import Status: Suspended
+    ...
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been suspended.
+
+
+=== Marking an import as failing ===
+
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> import_browser.getControl('Mark Failing').click()
+    >>> print_import_details(import_browser)
+    Import Status: Failed
+    ...
+    >>> for message in get_feedback_messages(import_browser.contents):
+    ...     print extract_text(message)
+    The code import has been marked as failing.
+
+
+== Import details for a running job ==
+
+If the job for an approved import is running, then the import details
+says how long ago since it started.
+
+    >>> import_browser.getLink('Edit import source or review import').click()
+    >>> import_browser.getControl('Approve').click()
+    >>> print_import_details(import_browser)
+    Import Status: Reviewed
+    ...
+    The next import is scheduled to run as soon as possible.
+
+Now set the job as running.
+
+    >>> login('[e-mail]')
+    >>> from canonical.launchpad.testing.codeimporthelpers import (
+    ...     get_import_for_branch_name, make_running_import)
+    >>> code_import = get_import_for_branch_name(
+    ...     svn_import_branch_unique_name)
+
+Set the started time to 2h 20m ago, and the approximate datetime
+should show this as 2 hours.
+
+    >>> from datetime import datetime, timedelta
+    >>> import pytz
+    >>> date_started = datetime.now(pytz.UTC) - timedelta(hours=2, minutes=20)
+    >>> code_import = make_running_import(
+    ...     code_import, date_started=date_started, factory=factory,
+    ...     logtail="Changeset 1\nChangeset 2")
+    >>> logout()
+
+    >>> import_browser.open(svn_import_location)
+    >>> print_import_details(import_browser)
+    Import Status: Reviewed
+    ...
+    An import is currently running on machine-..., and was started 2 hours
+    ago.  The last few lines of the job's output were:
+        Changeset 1
+        Changeset 2
+
+
+== Import details for a import that has been imported successfully ==
+
+If a branch has been successfully imported in the past, then the date
+that it was last successful is shown, as well as when the next import
+will be run -- which is, by default for Subversion, six hours after the
+last import completed, and so in this case in about three hours.
+
+    >>> login('[e-mail]')
+    >>> from canonical.launchpad.testing.codeimporthelpers import (
+    ...     make_finished_import)
+    >>> date_finished = datetime(2007,9,10,12, tzinfo=pytz.UTC)
+    >>> code_import = get_import_for_branch_name(
+    ...     svn_import_branch_unique_name)
+    >>> code_import = make_finished_import(code_import, factory=factory,
+    ...                                    date_finished=date_finished)
+    >>> logout()
+
+    >>> import_browser.open(svn_import_location)
+    >>> print_import_details(import_browser)
+    Import Status: Reviewed
+    This branch is an import of the Subversion branch from
+        svn://svn-new.example.com/fooix/trunk.
+    The next import is scheduled to run in 3 hours.
+    Last successful import was on 2007-09-10.
+    ...
+
+
+== Requesting an import ==
+
+If an import is waiting for its next update, any logged in user can
+click a button to request an immediate import.
+
+    >>> sample_person_browser = setupBrowser(
+    ...     auth='Basic [e-mail]:test')
+    >>> sample_person_browser.open(import_browser.url)
+    >>> sample_person_browser.getControl('Import Now')
+    <SubmitControl ...>
+
+Anonymous users cannot see this button.
+
+    >>> anon_browser.open(import_browser.url)
+    >>> anon_browser.getControl('Import Now')
+    Traceback (most recent call last):
+      ...
+    LookupError: label 'Import Now'
+
+If the logged in user clicks this button, the import will be scheduled
+to run ASAP and the fact that the import has been requested is
+displayed.
+
+    >>> sample_person_browser.getControl('Import Now').click()
+    >>> for msg in get_feedback_messages(sample_person_browser.contents):
+    ...     print msg
+    Import will run as soon as possible.
+    >>> print_import_details(sample_person_browser)
+    Import Status: Reviewed
+    This branch is an import of the Subversion branch from
+        svn://svn-new.example.com/fooix/trunk.
+    The next import is scheduled to run as soon as possible (requested
+    by Sample Person).
+    Last successful import was on 2007-09-10.
+    ...

=== modified file 'lib/lp/code/stories/codeimport/xx-codeimport-machines.txt'
--- lib/lp/code/stories/codeimport/xx-codeimport-machines.txt	2009-06-02 08:20:49 +0000
+++ lib/lp/code/stories/codeimport/xx-codeimport-machines.txt	2009-11-11 09:00:39 +0000
@@ -50,13 +50,13 @@
     >>> print_table(browser)
     Machine                            Status
     apollo (2 jobs running)            Online
-        lp://dev/~vcs-imports/.../...
+        lp://dev/~.../.../...
             Started: ... ago      Last heartbeat: ... ago
-        lp://dev/~vcs-imports/.../...
+        lp://dev/~.../.../...
             Started: ... ago      Last heartbeat: ... ago
     bazaar-importer (0 jobs running)   Online
     odin (1 job running)               Online
-        lp://dev/~vcs-imports/.../...
+        lp://dev/~.../.../...
             Started: ... ago      Last heartbeat: ... ago
     saturn                             Offline
 
@@ -79,10 +79,10 @@
 
     >>> print extract_text(find_tag_by_id(browser.contents, 'current-jobs'))
     Current jobs
-    lp://dev/~vcs-imports/.../... Started: ... ago Last heartbeat: ... ago
+    lp://dev/~.../.../... Started: ... ago Last heartbeat: ... ago
          Creating changeset 1
          Creating changeset 2
-    lp://dev/~vcs-imports/.../... Started: ... ago Last heartbeat: ... ago
+    lp://dev/~.../.../... Started: ... ago Last heartbeat: ... ago
 
 And also shows the most recent ten events for the machine, with the most
 recent event first.
@@ -92,8 +92,8 @@
     ...     print extract_text(recent_events)
     >>> print_events(browser)
     Related events
-    ...: Job Started lp://dev/~vcs-imports/.../...
-    ...: Job Started lp://dev/~vcs-imports/.../...
+    ...: Job Started lp://dev/~.../.../...
+    ...: Job Started lp://dev/~.../.../...
     ...: Machine Online
 
 

=== modified file 'lib/lp/code/stories/codeimport/xx-create-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2009-10-19 17:31:15 +0000
+++ lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2009-11-11 09:00:39 +0000
@@ -149,7 +149,7 @@
     ...     print extract_text(message)
     There is 1 error.
     Those CVS details are already specified for
-    the imported branch ~vcs-imports/firefox/import2.
+    the imported branch ~no-priv/firefox/import2.
 
     >>> browser.getControl('Subversion').click()
     >>> browser.getControl('Branch URL').value = (
@@ -160,7 +160,7 @@
     ...     print extract_text(message)
     There is 1 error.
     This Subversion branch URL is already specified for
-    the imported branch ~vcs-imports/firefox/imported.
+    the imported branch ~no-priv/firefox/imported.
 
 
 Requesting an import whose name would clash

=== modified file 'lib/lp/code/stories/codeimport/xx-edit-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-edit-codeimport.txt	2009-09-18 05:49:48 +0000
+++ lib/lp/code/stories/codeimport/xx-edit-codeimport.txt	2009-11-11 09:00:39 +0000
@@ -1,32 +1,29 @@
 = Editing code import details =
 
-The code import details are displayed on the main branch page for
-imported branches.  If the logged in user is an import operator
-(member of VCS imports or Launchpad admin) then they can see a link
-to edit the details.
+Not everyone can edit code import details, these stories show what it's like
+to edit a code import if you are the owner of the code import branch.
+
+The code import details are displayed on the main branch page for imported
+branches. If the logged-in user is allowed to edit the code import then they
+can see a link to edit the details.
 
     >>> from canonical.launchpad.ftests import ANONYMOUS, login, logout
-    >>> login('[e-mail]')
+    >>> from canonical.launchpad.webapp import canonical_url
+
+    >>> login(ANONYMOUS)
+    >>> registrant = factory.makePerson(password='test')
     >>> svn_import = factory.makeCodeImport(
-    ...     svn_branch_url='svn://svn.example.com/fooix/trunk')
-    >>> from canonical.launchpad.webapp import canonical_url
+    ...     svn_branch_url='svn://svn.example.com/fooix/trunk',
+    ...     registrant=registrant)
     >>> svn_import_location = str(canonical_url(svn_import.branch))
     >>> svn_import_branch_unique_name = svn_import.branch.unique_name
-    >>> cvs_import = factory.makeCodeImport(
-    ...     cvs_root=":pserver:[e-mail]:/fooix",
-    ...     cvs_module="fooix")
-    >>> cvs_import_location = str(canonical_url(cvs_import.branch))
-    >>> cvs_import_branch_unique_name = cvs_import.branch.unique_name
-    >>> git_import = factory.makeCodeImport(
-    ...     git_repo_url="git://git.example.org/fooix")
-    >>> git_import_location = str(canonical_url(git_import.branch))
-    >>> git_import_branch_unique_name = git_import.branch.unique_name
+
     >>> hosted_branch = factory.makeAnyBranch()
     >>> hosted_branch_location = str(canonical_url(hosted_branch))
     >>> logout()
 
     >>> import_browser = setupBrowser(
-    ...     auth='Basic [e-mail]:test')
+    ...     auth='Basic %s:test' % str(registrant.preferredemail.email))
 
 
 == Import details on the branch page ==
@@ -42,34 +39,19 @@
     ...     div = find_tag_by_id(
     ...         browser.contents, 'branch-import-details').div.div
     ...     print extract_text(div)
-    >>> browser.open(svn_import_location)
-    >>> print_import_details(browser)
+    >>> anon_browser.open(svn_import_location)
+    >>> print_import_details(anon_browser)
     Import Status: Pending Review
     This branch is an import of the Subversion branch
     from svn://svn.example.com/fooix/trunk.
 
 Because it's an svn:// URL, it doesn't get linkified:
 
-    >>> browser.getLink("svn://svn.example.com/fooix/trunk")
+    >>> anon_browser.getLink("svn://svn.example.com/fooix/trunk")
     Traceback (most recent call last):
     ...
     LinkNotFoundError
 
-Both VCS Imports members and Launchpad administrators can edit the
-import, though:
-
-    >>> import_browser.open(svn_import_location)
-    >>> print_import_details(import_browser)
-    Import Status: Pending Review
-    This branch is an import of the Subversion branch
-    from svn://svn.example.com/fooix/trunk.
-
-    >>> admin_browser.open(svn_import_location)
-    >>> print_import_details(admin_browser)
-    Import Status: Pending Review
-    This branch is an import of the Subversion branch
-    from svn://svn.example.com/fooix/trunk.
-
 
 == Not authorized versus 404 ==
 
@@ -77,7 +59,7 @@
 they will get a not authorised page if the branch has a code import,
 and a 404 if the branch doesn't have an import.
 
-    >>> browser.open(svn_import_location + '/+edit-import')
+    >>> anon_browser.open(svn_import_location + '/+edit-import')
     Traceback (most recent call last):
     Unauthorized: (... 'launchpad.Edit')
 
@@ -88,174 +70,10 @@
 
 == Editing details ==
 
-There are a number of buttons shown on the editing page for
-import operators.
-
-    >>> import_browser.getLink('Edit import source or review import').click()
-    >>> print_submit_buttons(import_browser.contents)
-    Update
-    Approve
-    Mark Invalid
-    Suspend
-    Mark Failing
-
-A cancel link is also shown next to the buttons, that takes the user
-back to the main branch details page.
-
-    >>> import_browser.getLink('Cancel').url == svn_import_location
-    True
-
-Only the fields that are relevant to the type of import are shown
-in the form.
-
-    >>> def print_form_fields(browser):
-    ...     tags = find_tags_by_class(browser.contents, 'textType')
-    ...     for tag in tags:
-    ...         print '%s: %s' % (tag['name'], tag['value'])
-
-    >>> print_form_fields(import_browser)
-    field.svn_branch_url: svn://svn.example.com/fooix/trunk
-
-    >>> admin_browser.open(cvs_import_location)
-    >>> admin_browser.getLink('Edit import source or review import').click()
-    >>> print_form_fields(admin_browser)
-    field.cvs_root: :pserver:[e-mail]:/fooix
-    field.cvs_module: fooix
-
-    >>> admin_browser.open(git_import_location)
-    >>> admin_browser.getLink('Edit import source or review import').click()
-    >>> print_form_fields(admin_browser)
-    field.git_repo_url: git://git.example.org/fooix
-
-=== Editing the import location ===
-
-The +edit-import page allows the import operator to edit the location
-an import is from.
-
-This is true for Subversion imports,
-
-    >>> import_browser.open(svn_import_location + '/+edit-import')
-    >>> import_browser.getControl('Branch URL').value = \
-    ...     'svn://svn-new.example.com/fooix/trunk'
-    >>> import_browser.getControl('Update').click()
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been updated.
-
-CVS imports,
-
-    >>> import_browser.open(cvs_import_location + '/+edit-import')
-    >>> import_browser.getControl('Repository').value = \
-    ...     ':pserver:[e-mail]:/fooix'
-    >>> import_browser.getControl('Module').value = \
-    ...     'fooix2'
-    >>> import_browser.getControl('Update').click()
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been updated.
-
-and Git imports.
-
-    >>> import_browser.open(git_import_location + '/+edit-import')
-    >>> import_browser.getControl('Repo URL').value = \
-    ...     'git://git-new.example.org/fooix'
-    >>> import_browser.getControl('Update').click()
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been updated.
-
-
-=== Approving an import ===
-
-When a code import is approved, a pending job is created for it.
-
-    >>> import_browser.open(svn_import_location + '/+edit-import')
-    >>> import_browser.getControl('Approve').click()
-    >>> print_import_details(import_browser)
-    Import Status: Reviewed
-    ...
-    The next import is scheduled to run as soon as possible.
-
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been approved.
-
-
-=== Invalidating an import ===
-
-    >>> import_browser.getLink('Edit import source or review import').click()
-    >>> import_browser.getControl('Mark Invalid').click()
-    >>> print_import_details(import_browser)
-    Import Status: Invalid
-    ...
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been set as invalid.
-
-
-=== Suspending an import ===
-
-    >>> import_browser.getLink('Edit import source or review import').click()
-    >>> import_browser.getControl('Suspend').click()
-    >>> print_import_details(import_browser)
-    Import Status: Suspended
-    ...
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been suspended.
-
-
-=== Marking an import as failing ===
-
-    >>> import_browser.getLink('Edit import source or review import').click()
-    >>> import_browser.getControl('Mark Failing').click()
-    >>> print_import_details(import_browser)
-    Import Status: Failed
-    ...
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been marked as failing.
-
-
-== Import details for a running job ==
-
-If the job for an approved import is running, then the import details
-says how long ago since it started.
-
-    >>> import_browser.getLink('Edit import source or review import').click()
-    >>> import_browser.getControl('Approve').click()
-    >>> print_import_details(import_browser)
-    Import Status: Reviewed
-    ...
-    The next import is scheduled to run as soon as possible.
-
-Now set the job as running.
-
-    >>> login('[e-mail]')
-    >>> from canonical.launchpad.testing.codeimporthelpers import (
-    ...     get_import_for_branch_name, make_running_import)
-    >>> code_import = get_import_for_branch_name(
-    ...     svn_import_branch_unique_name)
-
-Set the started time to 2h 20m ago, and the approximate datetime
-should show this as 2 hours.
-
-    >>> from datetime import datetime, timedelta
-    >>> import pytz
-    >>> date_started = datetime.now(pytz.UTC) - timedelta(hours=2, minutes=20)
-    >>> code_import = make_running_import(
-    ...     code_import, date_started=date_started, factory=factory,
-    ...     logtail="Changeset 1\nChangeset 2")
-    >>> logout()
-
     >>> import_browser.open(svn_import_location)
-    >>> print_import_details(import_browser)
-    Import Status: Reviewed
-    ...
-    An import is currently running on machine-..., and was started 2 hours
-    ago.  The last few lines of the job's output were:
-        Changeset 1
-        Changeset 2
+    >>> import_browser.getLink('Edit import source or review import')
+    Traceback (most recent call last):
+    LinkNotFoundError
 
 
 == Import details for a import that has been imported successfully ==
@@ -265,10 +83,16 @@
 will be run -- which is, by default for Subversion, six hours after the
 last import completed, and so in this case in about three hours.
 
+We log in as "david.allouche" since he has super-user privileges for code
+imports.
+
     >>> login('[e-mail]')
     >>> from canonical.launchpad.testing.codeimporthelpers import (
-    ...     make_finished_import)
-    >>> date_finished = datetime(2007,9,10,12, tzinfo=pytz.UTC)
+    ...     make_finished_import, get_import_for_branch_name,
+    ...     make_running_import)
+    >>> from datetime import datetime
+    >>> import pytz
+    >>> date_finished = datetime(2007, 9, 10, 12, tzinfo=pytz.UTC)
     >>> code_import = get_import_for_branch_name(
     ...     svn_import_branch_unique_name)
     >>> code_import = make_finished_import(code_import, factory=factory,
@@ -279,8 +103,8 @@
     >>> print_import_details(import_browser)
     Import Status: Reviewed
     This branch is an import of the Subversion branch from
-        svn://svn-new.example.com/fooix/trunk.
-    The next import is scheduled to run in 3 hours.
+        svn://svn.example.com/fooix/trunk.
+    The next import is scheduled to run in 5 hours.
     Last successful import was on 2007-09-10.
     ...
 
@@ -315,8 +139,18 @@
     >>> print_import_details(sample_person_browser)
     Import Status: Reviewed
     This branch is an import of the Subversion branch from
-        svn://svn-new.example.com/fooix/trunk.
+        svn://svn.example.com/fooix/trunk.
     The next import is scheduled to run as soon as possible (requested
     by Sample Person).
     Last successful import was on 2007-09-10.
     ...
+
+
+== Deleting an import ==
+
+If you own the branch that has the code import, you can delete the branch.
+
+    >>> import_browser.getLink('Delete branch').click()
+    >>> import_browser.getControl('Delete').click()
+    >>> print_feedback_messages(import_browser.contents)
+    Branch ... deleted.

=== modified file 'lib/lp/code/stories/codeimport/xx-failing-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-failing-codeimport.txt	2009-05-13 03:41:09 +0000
+++ lib/lp/code/stories/codeimport/xx-failing-codeimport.txt	2009-11-11 09:00:39 +0000
@@ -9,8 +9,9 @@
     >>> from lp.code.interfaces.codeimportresult import CodeImportResultStatus
     >>> from canonical.config import config
     >>> product = factory.makeProduct(name='imported')
+    >>> owner = factory.makePerson(name='import-owner')
     >>> code_import = factory.makeCodeImport(
-    ...     product=product, branch_name='trunk')
+    ...     product=product, branch_name='trunk', registrant=owner)
     >>> for i in range(config.codeimport.consecutive_failure_limit):
     ...     dummy = make_finished_import(
     ...         code_import, CodeImportResultStatus.FAILURE,
@@ -20,7 +21,7 @@
 This is shown on the branch index page:
 
     >>> user_browser.open(
-    ...     'http://code.launchpad.dev/~vcs-imports/imported/trunk')
+    ...     'http://code.launchpad.dev/~import-owner/imported/trunk')
     >>> print extract_text(find_tag_by_id(user_browser.contents, 'failing-try-again'))
     The import has been suspended because it failed 5 or more times in succession.
 
@@ -33,7 +34,7 @@
 Anonymous users do not see this button, however.
 
     >>> anon_browser.open(
-    ...     'http://code.launchpad.dev/~vcs-imports/imported/trunk')
+    ...     'http://code.launchpad.dev/~import-owner/imported/trunk')
     >>> anon_browser.getControl('Try Again')
     Traceback (most recent call last):
       ...

=== modified file 'lib/lp/code/tests/test_branch.py'
--- lib/lp/code/tests/test_branch.py	2009-10-20 07:47:54 +0000
+++ lib/lp/code/tests/test_branch.py	2009-11-11 09:00:39 +0000
@@ -15,7 +15,7 @@
 from lp.archiveuploader.permission import verify_upload
 from lp.code.enums import (
     BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
-    CodeReviewNotificationLevel)
+    BranchType, CodeReviewNotificationLevel)
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.distroseries import DistroSeriesStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -231,6 +231,16 @@
         branch = self.factory.makeAnyBranch(owner=team)
         self.assertCanEdit(person, branch)
 
+    def test_vcs_imports_members_can_edit_import_branch(self):
+        # Even if a branch isn't owned by vcs-imports, vcs-imports members can
+        # edit it if it has a code import associated with it.
+        person = self.factory.makePerson()
+        branch = self.factory.makeCodeImport().branch
+        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
+        removeSecurityProxy(vcs_imports).addMember(
+            person, vcs_imports.teamowner)
+        self.assertCanEdit(person, branch)
+
     def makeOfficialPackageBranch(self):
         """Make a branch linked to the pocket of a source package."""
         branch = self.factory.makePackageBranch()
@@ -307,6 +317,28 @@
         # permissions granted above.
         self.assertCanEdit(person, branch)
 
+    def test_arbitrary_person_cannot_edit(self):
+        # Arbitrary people cannot edit branches, you have to be someone
+        # special.
+        branch = self.factory.makeAnyBranch()
+        person = self.factory.makePerson()
+        self.assertCannotEdit(person, branch)
+
+    def test_code_import_registrant_can_edit(self):
+        # It used to be the case that all import branches were owned by the
+        # special, restricted team ~vcs-imports. This made a lot of work for
+        # the Launchpad development team, since they needed to delete and
+        # rename import branches whenever people wanted it. To reduce this
+        # work a little, whoever registered of a code import branch is allowed
+        # to edit the branch, even if they aren't one of the owners.
+        registrant = self.factory.makePerson()
+        code_import = self.factory.makeCodeImport(registrant=registrant)
+        branch = code_import.branch
+        removeSecurityProxy(branch).setOwner(
+            getUtility(ILaunchpadCelebrities).vcs_imports,
+            getUtility(ILaunchpadCelebrities).vcs_imports)
+        self.assertCanEdit(registrant, branch)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-10-30 17:32:39 +0000
+++ lib/lp/testing/factory.py	2009-11-11 09:00:39 +0000
@@ -1193,7 +1193,7 @@
 
     def makeCodeImport(self, svn_branch_url=None, cvs_root=None,
                        cvs_module=None, product=None, branch_name=None,
-                       git_repo_url=None):
+                       git_repo_url=None, registrant=None):
         """Create and return a new, arbitrary code import.
 
         The type of code import will be inferred from the source details
@@ -1207,7 +1207,8 @@
             product = self.makeProduct()
         if branch_name is None:
             branch_name = self.getUniqueString('name')
-        registrant = self.makePerson()
+        if registrant is None:
+            registrant = self.makePerson()
 
         code_import_set = getUtility(ICodeImportSet)
         if svn_branch_url is not None: