Merge lp:~jelmer/launchpad/no-code-import-approval into lp:launchpad

[Jelmer Vernooij]

Remove the requirement for svn and cvs imports to be pre-approved by a Launchpad admin, fixing bug #418932.

=== modified file 'lib/lp/code/doc/codeimport-event.txt'
--- lib/lp/code/doc/codeimport-event.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/code/doc/codeimport-event.txt	2011-08-28 19:23:29 +0000
@@ -120,7 +120,7 @@
     >>> print_items(svn_create_event)
     CODE_IMPORT <muted>
     OWNER ...
-    REVIEW_STATUS u'NEW'
+    REVIEW_STATUS u'REVIEWED'
     ASSIGNEE None
     UPDATE_INTERVAL None
     URL u'svn://svn.example.com/trunk'
@@ -144,7 +144,7 @@
     >>> print_items(cvs_create_event)
     CODE_IMPORT <muted>
     OWNER ...
-    REVIEW_STATUS u'NEW'
+    REVIEW_STATUS u'REVIEWED'
     ASSIGNEE None
     UPDATE_INTERVAL None
     CVS_ROOT u':pserver:[e-mail]:/cvsroot'
@@ -206,7 +206,7 @@
 
     >>> from lp.code.enums import CodeImportReviewStatus
     >>> removeSecurityProxy(svn_import).review_status = (
-    ...     CodeImportReviewStatus.REVIEWED)
+    ...     CodeImportReviewStatus.SUSPENDED)
 
 After applying changes, the newModify method can create an event that
 details the changes that have been applied.
@@ -234,8 +234,8 @@
     >>> print_items(modify_event)
     CODE_IMPORT <muted>
     OWNER ...
-    REVIEW_STATUS u'REVIEWED'
-    OLD_REVIEW_STATUS u'NEW'
+    REVIEW_STATUS u'SUSPENDED'
+    OLD_REVIEW_STATUS u'REVIEWED'
     ASSIGNEE None
     UPDATE_INTERVAL None
     URL u'svn://svn.example.com/trunk'
 
=== modified file 'lib/lp/code/doc/codeimport.txt'
--- lib/lp/code/doc/codeimport.txt	2011-06-28 17:13:42 +0000
+++ lib/lp/code/doc/codeimport.txt	2011-08-28 19:23:29 +0000
@@ -243,7 +243,7 @@
     >>> code_import = factory.makeProductCodeImport(
     ...     svn_branch_url='http://svn.example.com/project')
     >>> print code_import.review_status.title
-    Pending Review
+    Reviewed
 
 When an import operator updates the code import emails are sent out to
 the branch subscribers and members of VCS Imports that describe the
@@ -275,8 +275,6 @@
     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:
@@ -293,8 +291,6 @@
     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:
 
=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2011-08-24 21:17:35 +0000
+++ lib/lp/code/model/codeimport.py	2011-08-28 19:23:29 +0000
@@ -198,7 +198,8 @@
             setattr(self, name, value)
         if 'review_status' in data:
             if data['review_status'] == CodeImportReviewStatus.REVIEWED:
-                CodeImportJobWorkflow().newJob(self)
+                if self.import_job is None:
+                    CodeImportJobWorkflow().newJob(self)
             else:
                 self._removeJob()
         event = event_set.newModify(self, user, token)
@@ -264,13 +265,8 @@
                 "Don't know how to sanity check source details for unknown "
                 "rcs_type %s"%rcs_type)
         if review_status is None:
-            # Auto approve git and hg imports.
-            if rcs_type in (
-                RevisionControlSystems.GIT, RevisionControlSystems.HG,
-                RevisionControlSystems.BZR):
-                review_status = CodeImportReviewStatus.REVIEWED
-            else:
-                review_status = CodeImportReviewStatus.NEW
+            # Auto approve imports.
+            review_status = CodeImportReviewStatus.REVIEWED
         if not target.supports_code_imports:
             raise AssertionError("%r doesn't support code imports" % target)
         if owner is None:
 
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2011-08-24 16:21:07 +0000
+++ lib/lp/code/model/tests/test_branch.py	2011-08-28 19:23:29 +0000
@@ -1510,7 +1510,6 @@
         """break_links allows deleting a code import branch."""
         code_import = self.factory.makeCodeImport()
         code_import_id = code_import.id
-        self.factory.makeCodeImportJob(code_import)
         code_import.branch.destroySelf(break_references=True)
         self.assertRaises(
             SQLObjectNotFound, CodeImport.get, code_import_id)
@@ -1575,7 +1574,6 @@
         """DeleteCodeImport.__call__ must delete the CodeImport."""
         code_import = self.factory.makeCodeImport()
         code_import_id = code_import.id
-        self.factory.makeCodeImportJob(code_import)
         DeleteCodeImport(code_import)()
         self.assertRaises(
             SQLObjectNotFound, CodeImport.get, code_import_id)
 
=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2011-08-28 08:36:14 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2011-08-28 19:23:29 +0000
@@ -56,20 +56,6 @@
 
     layer = DatabaseFunctionalLayer
 
-    def test_new_svn_import(self):
-        """A new subversion code import should have NEW status."""
-        code_import = CodeImportSet().new(
-            registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
-            branch_name='imported',
-            rcs_type=RevisionControlSystems.SVN,
-            url=self.factory.getUniqueURL())
-        self.assertEqual(
-            CodeImportReviewStatus.NEW,
-            code_import.review_status)
-        # No job is created for the import.
-        self.assertIs(None, code_import.import_job)
-
     def test_new_svn_import_svn_scheme(self):
         """A subversion import can use the svn:// scheme."""
         code_import = CodeImportSet().new(
@@ -79,10 +65,10 @@
             rcs_type=RevisionControlSystems.SVN,
             url=self.factory.getUniqueURL(scheme="svn"))
         self.assertEqual(
-            CodeImportReviewStatus.NEW,
+            CodeImportReviewStatus.REVIEWED,
             code_import.review_status)
         # No job is created for the import.
-        self.assertIs(None, code_import.import_job)
+        self.assertIsNot(None, code_import.import_job)
 
     def test_reviewed_svn_import(self):
         """A specific review status can be set for a new import."""
@@ -92,30 +78,15 @@
             branch_name='imported',
             rcs_type=RevisionControlSystems.SVN,
             url=self.factory.getUniqueURL(),
-            review_status=CodeImportReviewStatus.REVIEWED)
+            review_status=None)
         self.assertEqual(
             CodeImportReviewStatus.REVIEWED,
             code_import.review_status)
         # A job is created for the import.
         self.assertIsNot(None, code_import.import_job)
 
-    def test_new_cvs_import(self):
-        """A new CVS code import should have NEW status."""
-        code_import = CodeImportSet().new(
-            registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
-            branch_name='imported',
-            rcs_type=RevisionControlSystems.CVS,
-            cvs_root=self.factory.getUniqueURL(),
-            cvs_module='module')
-        self.assertEqual(
-            CodeImportReviewStatus.NEW,
-            code_import.review_status)
-        # No job is created for the import.
-        self.assertIs(None, code_import.import_job)
-
-    def test_reviewed_cvs_import(self):
-        """A specific review status can be set for a new import."""
+    def test_cvs_import_reviewed(self):
+        """A new CVS code import should have REVIEWED status."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
             target=IBranchTarget(self.factory.makeProduct()),
@@ -123,7 +94,7 @@
             rcs_type=RevisionControlSystems.CVS,
             cvs_root=self.factory.getUniqueURL(),
             cvs_module='module',
-            review_status=CodeImportReviewStatus.REVIEWED)
+            review_status=None)
         self.assertEqual(
             CodeImportReviewStatus.REVIEWED,
             code_import.review_status)
@@ -275,8 +246,7 @@
         """Ensure deleting CodeImport objects deletes associated jobs."""
         code_import = self.factory.makeCodeImport()
         login_person(getUtility(ILaunchpadCelebrities).vcs_imports.teamowner)
-        code_import_job = self.factory.makeCodeImportJob(code_import)
-        job_id = code_import_job.id
+        job_id = code_import.import_job.id
         CodeImportJobSet().getById(job_id)
         job = CodeImportJobSet().getById(job_id)
         assert job is not None
@@ -644,7 +614,8 @@
         # tryFailingImportAgain only succeeds for imports that are FAILING.
         outcomes = {}
         for status in CodeImportReviewStatus.items:
-            code_import = self.factory.makeCodeImport()
+            code_import = self.factory.makeCodeImport(
+                review_status=CodeImportReviewStatus.NEW)
             code_import.updateFromData(
                 {'review_status': status}, self.factory.makePerson())
             try:
@@ -726,9 +697,10 @@
         self.assertEqual(requester, e.requesting_user)
 
     def test_exception_on_disabled(self):
-        # get an SVN request, which isn't reviewed by default
+        # get an SVN request which is suspended
         code_import = self.factory.makeCodeImport(
-            svn_branch_url=self.factory.getUniqueURL())
+            svn_branch_url=self.factory.getUniqueURL(),
+            review_status=CodeImportReviewStatus.SUSPENDED)
         requester = self.factory.makePerson()
         # which leads to an exception if we try and ask for an import
         self.assertRaises(
 
=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2011-06-29 12:00:39 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2011-08-28 19:23:29 +0000
@@ -115,7 +115,8 @@
 
     def makeJob(self, state, date_due_delta, requesting_user=None):
         """Create a CodeImportJob object from a spec."""
-        code_import = self.factory.makeCodeImport()
+        code_import = self.factory.makeCodeImport(
+            review_status=CodeImportReviewStatus.NEW)
         job = self.factory.makeCodeImportJob(code_import)
         if state == CodeImportJobState.RUNNING:
             getUtility(ICodeImportJobWorkflow).startJob(job, self.machine)
@@ -387,11 +388,12 @@
     def test_wrongReviewStatus(self):
         # CodeImportJobWorkflow.newJob fails if the CodeImport review_status
         # is different from REVIEWED.
-        new_import = self.factory.makeCodeImport()
+        new_import = self.factory.makeCodeImport(
+            review_status=CodeImportReviewStatus.SUSPENDED)
         branch_name = new_import.branch.unique_name
         # Testing newJob failure.
         self.assertFailure(
-            "Review status of %s is not REVIEWED: NEW" % (branch_name,),
+            "Review status of %s is not REVIEWED: SUSPENDED" % (branch_name,),
             getUtility(ICodeImportJobWorkflow).newJob, new_import)
 
     def test_existingJob(self):
@@ -417,14 +419,17 @@
         # If there is no CodeImportResult for the CodeImport, then the new
         # CodeImportJob has date_due set to UTC_NOW.
         code_import = self.getCodeImportForDateDueTest()
-        job = getUtility(ICodeImportJobWorkflow).newJob(code_import)
-        self.assertSqlAttributeEqualsDate(job, 'date_due', UTC_NOW)
+        self.assertSqlAttributeEqualsDate(code_import.import_job, 'date_due',
+            UTC_NOW)
 
     def test_dateDueRecentPreviousResult(self):
         # If there is a CodeImportResult for the CodeImport that is more
         # recent than the effective_update_interval, then the new
         # CodeImportJob has date_due set in the future.
         code_import = self.getCodeImportForDateDueTest()
+        # A code import job is automatically started when a reviewed code import
+        # is created. Remove it, so a "clean" one can be created later.
+        removeSecurityProxy(code_import).import_job.destroySelf()
         # Create a CodeImportResult that started a long time ago. This one
         # must be superseded by the more recent one created below.
         machine = self.factory.makeCodeImportMachine()
@@ -469,8 +474,8 @@
             date_job_started=datetime(2000, 1, 1, 12, 0, 0, tzinfo=UTC),
             date_created=datetime(2000, 1, 1, 12, 5, 0, tzinfo=UTC))
         # When we create the job, its date due must be set to UTC_NOW.
-        job = getUtility(ICodeImportJobWorkflow).newJob(code_import)
-        self.assertSqlAttributeEqualsDate(job, 'date_due', UTC_NOW)
+        self.assertSqlAttributeEqualsDate(code_import.import_job, 'date_due',
+            UTC_NOW)
 
 
 class TestCodeImportJobWorkflowDeletePendingJob(TestCaseWithFactory,
@@ -500,7 +505,8 @@
     def test_noJob(self):
         # CodeImportJobWorkflow.deletePendingJob fails if the
         # CodeImport is not associated to a CodeImportJob.
-        new_import = self.factory.makeCodeImport()
+        new_import = self.factory.makeCodeImport(
+            review_status=CodeImportReviewStatus.NEW)
         branch_name = new_import.branch.unique_name
         # Testing deletePendingJob failure.
         self.assertFailure(
@@ -578,7 +584,7 @@
         # CodeImportJobWorkflow.requestJob sets requesting_user and
         # date_due if the current date_due is in the future.
         code_import = self.factory.makeCodeImport()
-        pending_job = self.factory.makeCodeImportJob(code_import)
+        pending_job = code_import.import_job
         person = self.factory.makePerson()
         # Set date_due in the future. ICodeImportJob does not allow setting
         # date_due, so we must use removeSecurityProxy.
@@ -877,8 +883,7 @@
         unchecked_result_fields.difference_update(['log_file', 'status'])
 
         code_import = self.factory.makeCodeImport()
-        removeSecurityProxy(code_import).review_status = \
-            CodeImportReviewStatus.REVIEWED
+        removeSecurityProxy(code_import).import_job.destroySelf()
         self.assertFinishJobPassesThroughJobField(
             'code_import', 'code_import', code_import)
         unchecked_result_fields.remove('code_import')
 
=== modified file 'lib/lp/code/stories/codeimport/xx-admin-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	2010-04-28 02:49:58 +0000
+++ lib/lp/code/stories/codeimport/xx-admin-codeimport.txt	2011-08-28 19:23:29 +0000
@@ -61,9 +61,11 @@
     ...     print extract_text(div)
     >>> import_browser.open(svn_import_location)
     >>> print_import_details(import_browser)
-    Import Status: Pending Review
+    Import Status: Reviewed
     This branch is an import of the Subversion branch
     from svn://svn.example.com/fooix/trunk.
+    The next import is scheduled to run
+    as soon as possible.
     Edit import source or review import
 
 
@@ -76,7 +78,6 @@
     >>> import_browser.getLink('Edit import source or review import').click()
     >>> print_submit_buttons(import_browser.contents)
     Update
-    Approve
     Mark Invalid
     Suspend
     Mark Failing
@@ -190,24 +191,6 @@
     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.
-    Edit import source or review import
-
-    >>> for message in get_feedback_messages(import_browser.contents):
-    ...     print extract_text(message)
-    The code import has been approved.
-
-
 Invalidating an import
 ++++++++++++++++++++++
 
 
=== modified file 'lib/lp/code/stories/codeimport/xx-create-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2010-09-28 19:25:54 +0000
+++ lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2011-08-28 19:23:29 +0000
@@ -72,9 +72,11 @@
 When the user clicks continue, the import branch is created
 
     >>> print extract_text(find_tag_by_id(browser.contents, "import-details"))
-    Import Status: Pending Review
+    Import Status: Reviewed
     This branch is an import of the Subversion branch
     from http://svn.example.com/firefox/trunk.
+    The next import is scheduled to run
+    as soon as possible.
     >>> browser.getLink("http://svn.example.com/firefox/trunk")
     <Link text='http://svn.example.com/firefox/trunk'
            url='http://svn.example.com/firefox/trunk'>
@@ -98,9 +100,11 @@
     >>> browser.getControl('Project').value = "firefox"
     >>> browser.getControl('Request Import').click()
     >>> print extract_text(find_tag_by_id(browser.contents, "import-details"))
-    Import Status: Pending Review
+    Import Status: Reviewed
     This branch is an import of the Subversion branch
     from http://user:[e-mail]/firefox/trunk.
+    The next import is scheduled to run
+    as soon as possible.
 
 
 Requesting a Git import
@@ -165,10 +169,11 @@
     >>> browser.getControl('Request Import').click()
 
     >>> print extract_text(find_tag_by_id(browser.contents, "import-details"))
-    Import Status: Pending Review
+    Import Status: Reviewed
     This branch is an import of the CVS module firefox from
     :pserver:[e-mail]:/mozilla/cvs.
-
+    The next import is scheduled to run
+    as soon as possible.
 
 Requesting a CVS import with invalid information
 ================================================
 
=== modified file 'lib/lp/code/stories/codeimport/xx-edit-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-edit-codeimport.txt	2010-03-18 15:39:58 +0000
+++ lib/lp/code/stories/codeimport/xx-edit-codeimport.txt	2011-08-28 19:23:29 +0000
@@ -43,9 +43,11 @@
     ...     print extract_text(div)
     >>> anon_browser.open(svn_import_location)
     >>> print_import_details(anon_browser)
-    Import Status: Pending Review
+    Import Status: Reviewed
     This branch is an import of the Subversion branch
     from svn://svn.example.com/fooix/trunk.
+    The next import is scheduled to run
+    as soon as possible.
 
 Because it's an svn:// URL, it doesn't get linkified:
 
 
=== modified file 'lib/lp/code/stories/webservice/xx-code-import.txt'
--- lib/lp/code/stories/webservice/xx-code-import.txt	2010-04-16 05:03:03 +0000
+++ lib/lp/code/stories/webservice/xx-code-import.txt	2011-08-28 19:23:29 +0000
@@ -55,7 +55,7 @@
     >>> print representation['branch_link']
     http://.../~import-owner/scruff/import
     >>> print representation['review_status']
-    Pending Review
+    Reviewed
     >>> print representation['rcs_type']
     Subversion via CSCVS
     >>> print representation['url']
@@ -105,7 +105,7 @@
     >>> print representation['branch_link']
     http://.../~import-owner/scruffbuntu/manic/scruff/import
     >>> print representation['review_status']
-    Pending Review
+    Reviewed
     >>> print representation['rcs_type']
     Subversion via CSCVS
     >>> print representation['url']
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-27 16:28:52 +0000
+++ lib/lp/testing/factory.py	2011-08-28 19:23:29 +0000
@@ -2150,34 +2150,32 @@
             else:
                 assert rcs_type in (RevisionControlSystems.SVN,
                                     RevisionControlSystems.BZR_SVN)
-            code_import = code_import_set.new(
+            return code_import_set.new(
                 registrant, target, branch_name, rcs_type=rcs_type,
-                url=svn_branch_url)
+                url=svn_branch_url, review_status=review_status)
         elif git_repo_url is not None:
             assert rcs_type in (None, RevisionControlSystems.GIT)
-            code_import = code_import_set.new(
+            return code_import_set.new(
                 registrant, target, branch_name,
                 rcs_type=RevisionControlSystems.GIT,
-                url=git_repo_url)
+                url=git_repo_url, review_status=review_status)
         elif hg_repo_url is not None:
-            code_import = code_import_set.new(
+            return code_import_set.new(
                 registrant, target, branch_name,
                 rcs_type=RevisionControlSystems.HG,
-                url=hg_repo_url)
+                url=hg_repo_url, review_status=review_status)
         elif bzr_branch_url is not None:
-            code_import = code_import_set.new(
+            return code_import_set.new(
                 registrant, target, branch_name,
                 rcs_type=RevisionControlSystems.BZR,
-                url=bzr_branch_url)
+                url=bzr_branch_url, review_status=review_status)
         else:
             assert rcs_type in (None, RevisionControlSystems.CVS)
-            code_import = code_import_set.new(
+            return code_import_set.new(
                 registrant, target, branch_name,
                 rcs_type=RevisionControlSystems.CVS,
-                cvs_root=cvs_root, cvs_module=cvs_module)
-        if review_status:
-            removeSecurityProxy(code_import).review_status = review_status
-        return code_import
+                cvs_root=cvs_root, cvs_module=cvs_module,
+                review_status=review_status)
 
     def makeChangelog(self, spn=None, versions=[]):
         """Create and return a LFA of a valid Debian-style changelog."""
 
=== modified file 'utilities/sourcedeps.cache'
--- utilities/sourcedeps.cache	2011-08-19 14:03:25 +0000
+++ utilities/sourcedeps.cache	2011-08-28 19:23:29 +0000
@@ -1,8 +1,4 @@
 {
-    "bzr-builder": [
-        68, 
-        "[e-mail]-20101123183213-777lz46xgagn1deg"
-    ], 
     "testresources": [
         16, 
         "[e-mail]-20050911111209-ee5da49011cf936a"
@@ -31,14 +27,18 @@
         24, 
         "[e-mail]-20100601182722-wo7h2fh0fvyw3aaq"
     ], 
-    "lpreview": [
-        23, 
-        "[e-mail]-20090720061538-euyh68ifavhy0pi8"
-    ], 
     "bzr-git": [
         259, 
         "[e-mail]-20110601140035-gl5merbechngjw5s"
     ], 
+    "loggerhead": [
+        454, 
+        "[e-mail]-20110811101303-ka12yvnq2p48e2t8"
+    ], 
+    "bzr-builder": [
+        68, 
+        "[e-mail]-20101123183213-777lz46xgagn1deg"
+    ], 
     "bzr-loom": [
         49, 
         "[e-mail]-20110601122412-54vo3k8yae9i2zve"
@@ -47,9 +47,9 @@
         4, 
         "sinzui-20090526164636-1swugzupwvjgomo4"
     ], 
-    "loggerhead": [
-        452, 
-        "[e-mail]-20110628173100-owrifrnckvoi60af"
+    "lpreview": [
+        23, 
+        "[e-mail]-20090720061538-euyh68ifavhy0pi8"
     ], 
     "difftacular": [
         6,