Merge lp:~stevenk/launchpad/local-codeimports-bad into lp:launchpad

[Steve Kowalik]

Deny creation of a code import if it is for a launchpad.net URL.

Deny creation of a code import if it is for a launchpad.net URL. I have refactored the portion of the validation method for both ProductSeries:+setbranch and CodeImport:+new-import (and CodeImport:+edit-import) that check the URL passed it.

I have also forced this branch to net-neutral by performing a bunch of whitespace cleanup as well as defining a new code enum, much like the same thing we did with PUBLIC_INFORMATION_TYPES and friends while on Disclosure.

This branch does not completely close the bug, since it can still occur after this lands. The next step is to clean up the production data.

[Ian Booth (wallyworld)]

Looks good. I like the refactoring.

review: Approve

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2012-10-09 01:07:52 +0000
+++ lib/lp/code/browser/codeimport.py	2012-11-08 02:13:22 +0000
@@ -12,8 +12,10 @@
     'CodeImportSetBreadcrumb',
     'CodeImportSetNavigation',
     'CodeImportSetView',
+    'validate_import_url',
     ]
 
+from urlparse import urlparse
 
 from BeautifulSoup import BeautifulSoup
 from lazr.restful.interface import (
@@ -49,6 +51,7 @@
     BranchSubscriptionNotificationLevel,
     CodeImportReviewStatus,
     CodeReviewNotificationLevel,
+    NON_CVS_RCS_TYPES,
     RevisionControlSystems,
     )
 from lp.code.errors import BranchExists
@@ -167,10 +170,7 @@
 
     def setSecondaryFieldError(self, field, error):
         """Set the field error only if there isn't an error already."""
-        if self.getFieldError(field):
-            # Leave this one as it is often required or a validator error.
-            pass
-        else:
+        if not self.getFieldError(field):
             self.setFieldError(field, error)
 
     def _validateCVS(self, cvs_root, cvs_module, existing_import=None):
@@ -201,16 +201,9 @@
             self.setSecondaryFieldError(
                 field_name, 'Enter the URL of a foreign VCS branch.')
         else:
-            code_import = getUtility(ICodeImportSet).getByURL(url)
-            if (code_import is not None and
-                code_import != existing_import):
-                self.setFieldError(
-                    field_name,
-                    structured("""
-                    This foreign branch URL is already specified for
-                    the imported branch <a href="%s">%s</a>.""",
-                    canonical_url(code_import.branch),
-                    code_import.branch.unique_name))
+            reason = validate_import_url(url, existing_import)
+            if reason:
+                self.setFieldError(field_name, reason)
 
 
 class NewCodeImportForm(Interface):
@@ -540,12 +533,8 @@
         # fields, and vice versa.
         if self.code_import.rcs_type == RevisionControlSystems.CVS:
             self.form_fields = self.form_fields.omit('url')
-        elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
-                                           RevisionControlSystems.BZR_SVN,
-                                           RevisionControlSystems.GIT,
-                                           RevisionControlSystems.BZR):
-            self.form_fields = self.form_fields.omit(
-                'cvs_root', 'cvs_module')
+        elif self.code_import.rcs_type in NON_CVS_RCS_TYPES:
+            self.form_fields = self.form_fields.omit('cvs_root', 'cvs_module')
         else:
             raise AssertionError('Unknown rcs_type for code import.')
 
@@ -575,10 +564,7 @@
             self._validateCVS(
                 data.get('cvs_root'), data.get('cvs_module'),
                 self.code_import)
-        elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
-                                           RevisionControlSystems.BZR_SVN,
-                                           RevisionControlSystems.GIT,
-                                           RevisionControlSystems.BZR):
+        elif self.code_import.rcs_type in NON_CVS_RCS_TYPES:
             self._validateURL(data.get('url'), self.code_import)
         else:
             raise AssertionError('Unknown rcs_type for code import.')
@@ -593,3 +579,19 @@
     def machines(self):
         """Get the machines, sorted alphabetically by hostname."""
         return getUtility(ICodeImportMachineSet).getAll()
+
+
+def validate_import_url(url, existing_import=None):
+    """Validate the given import URL."""
+    if urlparse(url).netloc.endswith('launchpad.net'):
+        return (
+            "You can not create imports for branches that are hosted by "
+            "Launchpad.")
+    code_import = getUtility(ICodeImportSet).getByURL(url)
+    if code_import is not None:
+        if existing_import and code_import == existing_import:
+            return None
+        return structured(
+            "This foreign branch URL is already specified for the imported "
+            "branch <a href='%s'>%s</a>.", canonical_url(code_import.branch),
+            code_import.branch.unique_name)

=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py	2012-10-11 04:14:37 +0000
+++ lib/lp/code/enums.py	2012-11-08 02:13:22 +0000
@@ -20,6 +20,7 @@
     'CodeImportReviewStatus',
     'CodeReviewNotificationLevel',
     'CodeReviewVote',
+    'NON_CVS_RCS_TYPES',
     'RevisionControlSystems',
     'UICreatableBranchType',
     ]
@@ -856,3 +857,7 @@
 
         The reviewer needs more information before making a decision.
         """)
+
+NON_CVS_RCS_TYPES = (
+    RevisionControlSystems.SVN, RevisionControlSystems.BZR_SVN,
+    RevisionControlSystems.GIT, RevisionControlSystems.BZR)

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2012-10-10 22:18:41 +0000
+++ lib/lp/code/model/codeimport.py	2012-11-08 02:13:22 +0000
@@ -38,6 +38,7 @@
     CodeImportJobState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    NON_CVS_RCS_TYPES,
     RevisionControlSystems,
     )
 from lp.code.errors import (
@@ -71,8 +72,7 @@
     _defaultOrder = ['id']
 
     date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
-    branch = ForeignKey(dbName='branch', foreignKey='Branch',
-                        notNull=True)
+    branch = ForeignKey(dbName='branch', foreignKey='Branch', notNull=True)
     registrant = ForeignKey(
         dbName='registrant', foreignKey='Person',
         storm_validator=validate_public_person, notNull=True)
@@ -243,10 +243,7 @@
         if rcs_type == RevisionControlSystems.CVS:
             assert cvs_root is not None and cvs_module is not None
             assert url is None
-        elif rcs_type in (RevisionControlSystems.SVN,
-                          RevisionControlSystems.BZR_SVN,
-                          RevisionControlSystems.GIT,
-                          RevisionControlSystems.BZR):
+        elif rcs_type in NON_CVS_RCS_TYPES:
             assert cvs_root is None and cvs_module is None
             assert url is not None
         else:

=== modified file 'lib/lp/code/stories/codeimport/xx-create-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2012-10-09 01:07:52 +0000
+++ lib/lp/code/stories/codeimport/xx-create-codeimport.txt	2012-11-08 02:13:22 +0000
@@ -97,6 +97,19 @@
     at http://user:[e-mail]/firefox/trunk.
     The next import is scheduled to run
     as soon as possible.
+    
+Specifing a Launchpad URL results in an error.
+
+    >>> browser.open("http://code.launchpad.dev/+code-imports/+new")
+    >>> browser.getControl('Branch Name').value = "invalid"
+    >>> browser.getControl('Branch URL', index=0).value = (
+    ...     "http://bazaar.launchpad.net/firefox/trunk")
+    >>> browser.getControl('Project').value = "firefox"
+    >>> browser.getControl('Request Import').click()
+    >>> for message in get_feedback_messages(browser.contents):
+    ...     print extract_text(message)
+    There is 1 error.
+    You can not create imports for branches that are hosted by Launchpad.
 
 Requesting a Subversion import
 ==============================

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2012-10-16 15:12:09 +0000
+++ lib/lp/registry/browser/productseries.py	2012-11-08 02:13:22 +0000
@@ -86,6 +86,7 @@
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.code.browser.branch import BranchNameValidationMixin
 from lp.code.browser.branchref import BranchRef
+from lp.code.browser.codeimport import validate_import_url
 from lp.code.enums import (
     BranchType,
     RevisionControlSystems,
@@ -156,8 +157,6 @@
         """Return the series branch."""
         if self.context.branch:
             return BranchRef(self.context.branch)
-        else:
-            return None
 
     @stepto('+pots')
     def pots(self):
@@ -660,10 +659,7 @@
         return 'Edit %s %s series' % (
             self.context.product.displayname, self.context.name)
 
-    @property
-    def page_title(self):
-        """The page title."""
-        return self.label
+    page_title = label
 
     def validate(self, data):
         """See `LaunchpadFormView`."""
@@ -697,10 +693,7 @@
         return 'Delete %s %s series' % (
             self.context.product.displayname, self.context.name)
 
-    @property
-    def page_title(self):
-        """The page title."""
-        return self.label
+    page_title = label
 
     @cachedproperty
     def milestones(self):
@@ -906,8 +899,7 @@
         """Validate data for link-lp-bzr case."""
         if 'branch_location' not in data:
             self.setFieldError(
-                'branch_location',
-                'The branch location must be set.')
+                'branch_location', 'The branch location must be set.')
 
     def _validateImportExternal(self, data):
         """Validate data for import external case."""
@@ -925,16 +917,9 @@
             self.setFieldError(
                 'repo_url', 'You must set the external repository URL.')
         else:
-            # Ensure this URL has not been imported before.
-            code_import = getUtility(ICodeImportSet).getByURL(repo_url)
-            if code_import is not None:
-                self.setFieldError(
-                    'repo_url',
-                    structured("""
-                    This foreign branch URL is already specified for
-                    the imported branch <a href="%s">%s</a>.""",
-                               canonical_url(code_import.branch),
-                               code_import.branch.unique_name))
+            reason = validate_import_url(repo_url)
+            if reason:
+                self.setFieldError('repo_url', reason)
 
         # RCS type is mandatory.
         # This condition should never happen since an initial value is set.
@@ -945,19 +930,15 @@
                 'You must specify the type of RCS for the remote host.')
         elif rcs_type == RevisionControlSystems.CVS:
             if 'cvs_module' not in data:
-                self.setFieldError(
-                    'cvs_module',
-                    'The CVS module must be set.')
+                self.setFieldError('cvs_module', 'The CVS module must be set.')
         self._validateBranch(data)
 
     def _validateBranch(self, data):
         """Validate that branch name and owner are set."""
         if 'branch_name' not in data:
-            self.setFieldError(
-                'branch_name', 'The branch name must be set.')
+            self.setFieldError('branch_name', 'The branch name must be set.')
         if 'branch_owner' not in data:
-            self.setFieldError(
-                'branch_owner', 'The branch owner must be set.')
+            self.setFieldError('branch_owner', 'The branch owner must be set.')
 
     def _setRequired(self, names, value):
         """Mark the widget field as optional."""
@@ -1043,12 +1024,8 @@
             branch_name = data.get('branch_name')
             branch_owner = data.get('branch_owner')
 
-            # Import or mirror an external branch.
             if branch_type == IMPORT_EXTERNAL:
-                # Either create an externally hosted bzr branch
-                # (a.k.a. 'mirrored') or create a new code import.
                 rcs_type = data.get('rcs_type')
-                # We need to create an import request.
                 if rcs_type == RevisionControlSystems.CVS:
                     cvs_root = data.get('repo_url')
                     cvs_module = data.get('cvs_module')
@@ -1069,8 +1046,7 @@
                         cvs_root=cvs_root,
                         cvs_module=cvs_module)
                 except BranchExists as e:
-                    self._setBranchExists(
-                        e.existing_branch, 'branch_name')
+                    self._setBranchExists(e.existing_branch, 'branch_name')
                     self.errors_in_action = True
                     # Abort transaction. This is normally handled
                     # by LaunchpadFormView, but we are already in
@@ -1177,8 +1153,7 @@
 class ProductSeriesRdfView(BaseRdfView):
     """A view that sets its mime-type to application/rdf+xml"""
 
-    template = ViewPageTemplateFile(
-        '../templates/productseries-rdf.pt')
+    template = ViewPageTemplateFile('../templates/productseries-rdf.pt')
 
     @property
     def filename(self):

=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-10-09 01:07:52 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-11-08 02:13:22 +0000
@@ -138,7 +138,6 @@
     ('repo_url'...The URI scheme "bzr+ssh" is not allowed.  Only URIs with
     the following schemes may be used: bzr, http, https'))
 
-
 A correct URL is accepted.
 
     >>> form = {
@@ -161,6 +160,17 @@
     blazer-branch
     >>> series.branch.registrant.name == driver.name
     True
+    
+External imports can not use an Launchpad URL.
+
+    >>> form['field.repo_url'] = 'http://bazaar.launchpad.net/firefox/foo'
+    >>> form['field.branch_name'] = 'chevette-branch'
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
+    >>> transaction.commit()
+    >>> for error in view.errors:
+    ...     print error
+    You can not create imports for branches that are hosted by Launchpad.
 
 Git branches cannnot use svn.
 
@@ -303,7 +313,7 @@
     <BLANKLINE>
     This foreign branch URL is already specified for
     the imported branch
-    <a href="http://.../chevy/chevette-branch">~.../chevy/chevette-branch</a>.
+    <a href='http://.../chevy/chevette-branch'>~.../chevy/chevette-branch</a>.
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
 
@@ -348,7 +358,7 @@
     ...     print error
     <BLANKLINE>
     This foreign branch URL is already specified for the imported branch
-    <a href="http://.../blazer-branch">...</a>.
+    <a href='http://.../blazer-branch'>...</a>.
     >>> print view.errors_in_action
     False
     >>> print view.next_url

=== modified file 'lib/lp/translations/browser/tests/productseries-views.txt'
--- lib/lp/translations/browser/tests/productseries-views.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/translations/browser/tests/productseries-views.txt	2012-11-08 02:13:22 +0000
@@ -4,8 +4,7 @@
 let's put this check in a convenient function.
 
     >>> from zope.component import getUtility
-    >>> from lp.code.interfaces.branchjob import (
-    ...     IRosettaUploadJobSource)
+    >>> from lp.code.interfaces.branchjob import IRosettaUploadJobSource
     >>> job_counter = 0
     >>> def isUploadJobCreatedForBranch(productseries,
     ...                                 force_translations_upload=None):
@@ -57,8 +56,7 @@
     ...     method='POST',
     ...     form={'field.branch': branch.unique_name,
     ...           'field.actions.update': 'Update'})
-    >>> view = ProductSeriesLinkBranchView(
-    ...     productseries, request)
+    >>> view = ProductSeriesLinkBranchView(productseries, request)
     >>> view.initialize()
     >>> print isUploadJobCreatedForBranch(productseries)
     True