launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/codeimport-git-refactor-name-validation into lp:launchpad.

Commit message:
Rearrange branch/code-import name validation: Product:+configure-code now uses the code-import version.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1469459 in Launchpad itself: "import external code into a LP git repo (natively)"
  https://bugs.launchpad.net/launchpad/+bug/1469459

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-refactor-name-validation/+merge/308576

The code-import version of this is a better fit for Git-targeted imports.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/codeimport-git-refactor-name-validation into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2016-10-15 01:12:01 +0000
+++ lib/lp/code/browser/branch.py	2016-10-15 14:03:15 +0000
@@ -15,7 +15,6 @@
     'BranchReviewerEditView',
     'BranchMirrorStatusView',
     'BranchMirrorMixin',
-    'BranchNameValidationMixin',
     'BranchNavigation',
     'BranchEditMenu',
     'BranchUpgradeView',
@@ -617,22 +616,6 @@
         return self.context.getSpecificationLinks(self.user)
 
 
-class BranchNameValidationMixin:
-    """Provide name validation logic used by several branch view classes."""
-
-    def _setBranchExists(self, existing_branch, field_name='name'):
-        owner = existing_branch.owner
-        if owner == self.user:
-            prefix = "You already have"
-        else:
-            prefix = "%s already has" % owner.displayname
-        message = structured(
-            "%s a branch for <em>%s</em> called <em>%s</em>.",
-            prefix, existing_branch.target.displayname,
-            existing_branch.name)
-        self.setFieldError(field_name, message)
-
-
 class BranchEditFormView(LaunchpadEditFormView):
     """Base class for forms that edit a branch."""
 
@@ -1067,8 +1050,7 @@
                 self.form_fields = new_owner_field + self.form_fields
 
 
-class BranchEditView(CodeEditOwnerMixin, BranchEditFormView,
-                     BranchNameValidationMixin):
+class BranchEditView(CodeEditOwnerMixin, BranchEditFormView):
     """The main branch for editing the branch attributes."""
 
     @property
@@ -1095,6 +1077,18 @@
         if branch.branch_type in (BranchType.HOSTED, BranchType.IMPORTED):
             self.form_fields = self.form_fields.omit('url')
 
+    def _setBranchExists(self, existing_branch, field_name='name'):
+        owner = existing_branch.owner
+        if owner == self.user:
+            prefix = "You already have"
+        else:
+            prefix = "%s already has" % owner.displayname
+        message = structured(
+            "%s a branch for <em>%s</em> called <em>%s</em>.",
+            prefix, existing_branch.target.displayname,
+            existing_branch.name)
+        self.setFieldError(field_name, message)
+
     def validate(self, data):
         # Check that we're not moving a team branch to the +junk
         # pseudo project.

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2016-10-15 02:20:53 +0000
+++ lib/lp/code/browser/codeimport.py	2016-10-15 14:03:15 +0000
@@ -8,6 +8,7 @@
 __all__ = [
     'CodeImportEditView',
     'CodeImportMachineView',
+    'CodeImportNameValidationMixin',
     'CodeImportNewView',
     'CodeImportSetBreadcrumb',
     'CodeImportSetNavigation',
@@ -18,6 +19,7 @@
     'validate_import_url',
     ]
 
+from textwrap import dedent
 from urlparse import urlparse
 
 from BeautifulSoup import BeautifulSoup
@@ -237,6 +239,23 @@
                 self.setFieldError(field_name, reason)
 
 
+class CodeImportNameValidationMixin:
+    """Provide branch/repository name validation logic for code imports."""
+
+    def _setBranchExists(self, existing_branch, field_name):
+        self.setFieldError(
+            field_name,
+            structured(dedent("""
+            There is already an existing import for
+            <a href="%(product_url)s">%(product_name)s</a>
+            with the name of
+            <a href="%(branch_url)s">%(branch_name)s</a>."""),
+                       product_url=canonical_url(existing_branch.target),
+                       product_name=existing_branch.target.name,
+                       branch_url=canonical_url(existing_branch),
+                       branch_name=existing_branch.name))
+
+
 class NewCodeImportForm(Interface):
     """The fields presented on the form for editing a code import."""
 
@@ -303,7 +322,7 @@
         )
 
 
-class CodeImportNewView(CodeImportBaseView):
+class CodeImportNewView(CodeImportBaseView, CodeImportNameValidationMixin):
     """The view to request a new code import."""
 
     schema = NewCodeImportForm
@@ -420,20 +439,6 @@
             cvs_module=cvs_module,
             review_status=status)
 
-    def _setBranchExists(self, existing_branch):
-        """Set a field error indicating that the branch already exists."""
-        self.setFieldError(
-           'branch_name',
-            structured("""
-            There is already an existing import for
-            <a href="%(product_url)s">%(product_name)s</a>
-            with the name of
-            <a href="%(branch_url)s">%(branch_name)s</a>.""",
-                       product_url=canonical_url(existing_branch.target),
-                       product_name=existing_branch.target.name,
-                       branch_url=canonical_url(existing_branch),
-                       branch_name=existing_branch.name))
-
     @action(_('Request Import'), name='request_import')
     def request_import_action(self, action, data):
         """Create the code_import, and subscribe the user to the branch."""

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2016-10-13 14:46:41 +0000
+++ lib/lp/registry/browser/product.py	2016-10-15 14:03:15 +0000
@@ -140,9 +140,11 @@
     StructuralSubscriptionTargetTraversalMixin,
     )
 from lp.bugs.interfaces.bugtask import RESOLVED_BUGTASK_STATUSES
-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.browser.codeimport import (
+    CodeImportNameValidationMixin,
+    validate_import_url,
+    )
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
 from lp.code.browser.vcslisting import TargetDefaultVCSNavigationMixin
 from lp.code.enums import (
@@ -1725,7 +1727,7 @@
 
 class ProductSetBranchView(ReturnToReferrerMixin, LaunchpadFormView,
                            ProductView,
-                           BranchNameValidationMixin):
+                           CodeImportNameValidationMixin):
     """The view to set a branch default for the Product."""
 
     label = 'Configure code'

=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2016-10-15 00:59:49 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2016-10-15 14:03:15 +0000
@@ -349,8 +349,10 @@
     ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
-    Team Name ... already has a branch for
-    <em>Chevy</em> called <em>chevette-branch</em>.
+    There is already an existing import for
+    <a href="http://.../chevy";>chevy</a>
+    with the name of
+    <a href="http://.../chevy/chevette-branch";>chevette-branch</a>.
     >>> print view.errors_in_action
     True
     >>> print view.next_url