Merge ~twom/launchpad:oci-policy-git-branch-picker-format into launchpad:master

[Tom Wardill]

Add validator to OCIRecipeAdd and OCIRecipeEdit views

Ensure that any new OCIRecipes, and any that are updated, use the branch format of `2.0-20.04`.

[Thiago F. Pappacena (pappacena)]

LGTM

review: Approve

diff --git a/lib/lp/app/validators/validation.py b/lib/lp/app/validators/validation.py
index e38af13..e9ef8b1 100644
--- a/lib/lp/app/validators/validation.py
+++ b/lib/lp/app/validators/validation.py
@@ -9,8 +9,11 @@ __all__ = [
     'can_be_nominated_for_series',
     'valid_cve_sequence',
     'validate_new_team_email',
+    'validate_oci_branch_name',
     ]
 
+import re
+
 from zope.component import getUtility
 
 from lp import _
@@ -79,3 +82,25 @@ def validate_new_team_email(email):
     _validate_email(email)
     _check_email_availability(email)
     return True
+
+
+def validate_oci_branch_name(branch_name):
+    """Check that a git ref name matches appversion-ubuntuversion."""
+    split = branch_name.split('-')
+    # if we've not got at least two components
+    if len(split) < 2:
+        return False
+    app_version = split[0:-1]
+    ubuntu_version = split[-1]
+    # 20.04 format
+    ubuntu_match = re.match("\d{2}\.\d{2}", ubuntu_version)
+    if not ubuntu_match:
+        return False
+    # disallow risks in app version number
+    for risk in ["stable", "candidate", "beta", "edge"]:
+        if risk in app_version:
+            return False
+    # no '/' as they're a delimiter
+    if '/' in app_version:
+        return False
+    return True
diff --git a/lib/lp/code/browser/widgets/gitref.py b/lib/lp/code/browser/widgets/gitref.py
index cbe203d..64f27c5 100644
--- a/lib/lp/code/browser/widgets/gitref.py
+++ b/lib/lp/code/browser/widgets/gitref.py
@@ -107,6 +107,8 @@ class GitRefWidget(BrowserWidget, InputWidget):
     # If True, only allow reference paths to be branches (refs/heads/*).
     require_branch = False
 
+    branch_validator = None
+
     def setUpSubWidgets(self):
         if self._widgets_set_up:
             return
@@ -126,6 +128,9 @@ class GitRefWidget(BrowserWidget, InputWidget):
                 self, field.__name__, field, IInputWidget, prefix=self.name)
         self._widgets_set_up = True
 
+    def setBranchFormatValidator(self, branch_validator):
+        self.branch_validator = branch_validator
+
     def setRenderedValue(self, value, with_path=True):
         """See `IWidget`."""
         self.setUpSubWidgets()
@@ -205,9 +210,16 @@ class GitRefWidget(BrowserWidget, InputWidget):
             ref = None
         if not ref and (repository or self.context.required):
             raise WidgetInputError(
+                self.name, self.label,
+                LaunchpadValidationError(
+                    "Please enter a Git branch path."))
+        if self.branch_validator and ref is not None:
+            valid, message = self.branch_validator(ref)
+            if not valid:
+                raise WidgetInputError(
                     self.name, self.label,
                     LaunchpadValidationError(
-                        "Please enter a Git branch path."))
+                        message))
         return ref
 
     def error(self):
diff --git a/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py b/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
index 98e223a..9b8e76c 100644
--- a/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
+++ b/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
@@ -274,6 +274,32 @@ class TestGitRefWidget(WithScenarios, TestCaseWithFactory):
         self.widget.request = LaunchpadTestRequest(form=form)
         self.assertEqual(ref, self.widget.getInputValue())
 
+    def test_getInputValue_with_branch_validator_valid(self):
+        def validator(ref):
+            return True, ""
+        [ref] = self.factory.makeGitRefs()
+        form = {
+            "field.git_ref.repository": ref.repository.unique_name,
+            "field.git_ref.path": ref.path,
+            }
+        self.widget.setBranchFormatValidator(validator)
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertEqual(ref, self.widget.getInputValue())
+
+    def test_getInputValue_with_branch_validator_invalid(self):
+        def validator(ref):
+            return False, "Not in correct format"
+        [ref] = self.factory.makeGitRefs()
+        form = {
+            "field.git_ref.repository": ref.repository.unique_name,
+            "field.git_ref.path": ref.path,
+            }
+        self.widget.setBranchFormatValidator(validator)
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertGetInputValueError(
+            form,
+            "Not in correct format")
+
     def test_call(self):
         # The __call__ method sets up the widgets.
         markup = self.widget()
diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index abaff05..4f4253a 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -54,6 +54,7 @@ from lp.app.browser.tales import (
     GitRepositoryFormatterAPI,
     )
 from lp.app.errors import UnexpectedFormData
+from lp.app.validators.validation import validate_oci_branch_name
 from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.code.browser.widgets.gitref import GitRefWidget
@@ -754,6 +755,16 @@ class OCIRecipeFormMixin:
             return True
         return False
 
+    def _branch_format_validator(self, ref):
+        result = validate_oci_branch_name(ref.name)
+        if result:
+            return result, ""
+        message = (
+            "Branch does not match format "
+            "'applicationversion-ubuntuversion', eg. 'v1.0-20.04'"
+        )
+        return False, message
+
 
 class OCIRecipeAddView(LaunchpadFormView, EnableProcessorsMixin,
                        OCIRecipeFormMixin):
@@ -805,6 +816,7 @@ class OCIRecipeAddView(LaunchpadFormView, EnableProcessorsMixin,
         path = self.context.getDefaultGitRepositoryPath(self.user)
         widget = self.widgets["git_ref"]
         widget.setUpSubWidgets()
+        widget.setBranchFormatValidator(self._branch_format_validator)
         widget.repository_widget.setRenderedValue(path)
         if widget.error():
             # Do not override more important git_ref errors.
@@ -950,6 +962,7 @@ class OCIRecipeEditView(BaseOCIRecipeEditView, EnableProcessorsMixin,
         oci_proj_url = canonical_url(oci_proj)
         widget = self.widgets["git_ref"]
         widget.setUpSubWidgets()
+        widget.setBranchFormatValidator(self._branch_format_validator)
         if widget.error():
             # Do not override more important git_ref errors.
             return
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 22634c1..970c49c 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -190,7 +190,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
     def test_create_new_recipe(self):
         oci_project = self.factory.makeOCIProject()
         oci_project_display = oci_project.display_name
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         source_display = git_ref.display_name
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
@@ -227,9 +228,24 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
             "Official recipe:\nNo",
             MatchesTagText(content, "official-recipe"))
 
+    def test_create_new_recipe_invalid_format(self):
+        oci_project = self.factory.makeOCIProject()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/invalid'])
+        browser = self.getViewBrowser(
+            oci_project, view_name="+new-recipe", user=self.person)
+        browser.getControl(name="field.name").value = "recipe-name"
+        browser.getControl("Description").value = "Recipe description"
+        browser.getControl(name="field.git_ref.repository").value = (
+            git_ref.repository.identity)
+        browser.getControl(name="field.git_ref.path").value = git_ref.path
+        browser.getControl("Create OCI recipe").click()
+        self.assertIn("Branch does not match format", browser.contents)
+
     def test_create_new_recipe_with_build_args(self):
         oci_project = self.factory.makeOCIProject()
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         browser.getControl(name="field.name").value = "recipe-name"
@@ -289,7 +305,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
     def test_create_new_recipe_processors(self):
         self.setUpDistroSeries()
         oci_project = self.factory.makeOCIProject(pillar=self.distribution)
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         processors = browser.getControl(name="field.processors")
@@ -358,7 +375,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
         distribution = self.factory.makeDistribution(
             oci_project_admin=self.person)
         oci_project = self.factory.makeOCIProject(pillar=distribution)
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         browser.getControl(name="field.name").value = "recipe-name"
@@ -381,11 +399,13 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
 
         # do it once
         oci_project = self.factory.makeOCIProject(pillar=distribution)
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
 
         # and then do it again
         oci_project2 = self.factory.makeOCIProject(pillar=distribution)
-        [git_ref2] = self.factory.makeGitRefs()
+        [git_ref2] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v3.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         browser.getControl(name="field.name").value = "recipe-name"
@@ -429,7 +449,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
         distribution = self.factory.makeDistribution(
             oci_project_admin=distro_owner)
         oci_project = self.factory.makeOCIProject(pillar=distribution)
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         browser.getControl(name="field.name").value = "recipe-name"
@@ -448,7 +469,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
 
     def test_create_recipe_doesnt_override_gitref_errors(self):
         oci_project = self.factory.makeOCIProject()
-        [git_ref] = self.factory.makeGitRefs()
+        [git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         browser = self.getViewBrowser(
             oci_project, view_name="+new-recipe", user=self.person)
         browser.getControl(name="field.name").value = "recipe-name"
@@ -561,7 +583,8 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
             oci_project=oci_project, git_ref=old_git_ref)
         self.factory.makeTeam(
             name="new-team", displayname="New Team", members=[self.person])
-        [new_git_ref] = self.factory.makeGitRefs()
+        [new_git_ref] = self.factory.makeGitRefs(
+            paths=['/refs/heads/v2.0-20.04'])
         self.factory.makeOCIPushRule(recipe=recipe)
 
         browser = self.getViewBrowser(recipe, user=self.person)
@@ -596,6 +619,25 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
             "Build schedule:\nBuilt daily\nEdit OCI recipe\n",
             MatchesTagText(content, "build-schedule"))
 
+    def test_edit_recipe_invalid_branch(self):
+        oci_project = self.factory.makeOCIProject()
+        repository = self.factory.makeGitRepository()
+        [old_git_ref] = self.factory.makeGitRefs(repository=repository)
+        recipe = self.factory.makeOCIRecipe(
+            registrant=self.person, owner=self.person,
+            oci_project=oci_project, git_ref=old_git_ref)
+        self.factory.makeTeam(
+            name="new-team", displayname="New Team", members=[self.person])
+        [new_git_ref] = self.factory.makeGitRefs(
+            repository=repository, paths=['/refs/heads/invalid'])
+        self.factory.makeOCIPushRule(recipe=recipe)
+
+        browser = self.getViewBrowser(recipe, user=self.person)
+        browser.getLink("Edit OCI recipe").click()
+        browser.getControl(name="field.git_ref.path").value = new_git_ref.path
+        browser.getControl("Update OCI recipe").click()
+        self.assertIn("Branch does not match format", browser.contents)
+
     def test_edit_recipe_sets_date_last_modified(self):
         # Editing an OCI recipe sets the date_last_modified property.
         date_created = datetime(2000, 1, 1, tzinfo=pytz.UTC)
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index fd14762..41493ee 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -12,7 +12,6 @@ __all__ = [
     'OCIRecipeSet',
     ]
 
-import re
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 import pytz
@@ -47,6 +46,7 @@ from zope.security.proxy import (
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.security import IAuthorization
+from lp.app.validators.validation import validate_oci_branch_name
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
@@ -201,25 +201,7 @@ class OCIRecipe(Storm, WebhookTargetMixin):
 
     @property
     def is_valid_branch_format(self):
-        name = self.git_ref.name
-        split = name.split('-')
-        # if we've not got at least two components
-        if len(split) < 2:
-            return False
-        app_version = split[0:-1]
-        ubuntu_version = split[-1]
-        # 20.04 format
-        ubuntu_match = re.match("\d{2}\.\d{2}", ubuntu_version)
-        if not ubuntu_match:
-            return False
-        # disallow risks in app version number
-        for risk in ["stable", "candidate", "beta", "edge"]:
-            if risk in app_version:
-                return False
-        # no '/' as they're a delimiter
-        if '/' in app_version:
-            return False
-        return True
+        return validate_oci_branch_name(self.git_ref.name)
 
     @property
     def build_args(self):
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 0bd8f83..df269c0 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4937,7 +4937,9 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         if oci_project is None:
             oci_project = self.makeOCIProject()
         if git_ref is None:
-            [git_ref] = self.makeGitRefs()
+            component = self.getUniqueUnicode()
+            paths = [u'refs/heads/{}-20.04'.format(component)]
+            [git_ref] = self.makeGitRefs(paths=paths)
         if build_file is None:
             build_file = self.getUniqueUnicode(u"build_file_for")
         if build_path is None: