Don't reset repo on invalid WorkspaceCreateForm.

`WorkspaceCreateForm` constructs repo and branch `ChoiceFields` dynamically
based on the state of GitHub at request-time. The code that does this was not
doing anything to maintain the initial value from any submitted form POST data.
The Django machinery does not do this automatically for these fields as they
are dynamically constructed. Therefore if an invalid form was submitted (e.g.,
with a name that is not a valid slug), these fields were reverting to their
default value of the first element in the list, which is bad UX as they need to
reselect the repo and branch on validation failure.

Set the `initial` property on each field to the value from the POST data, or
`None` if not present.

`initial` from the `ChoiceField` API is not automatically respected by our
`form_select` components, so set the initial selected value in the template.

Make the branching logic clearer and add more comments.

Add dummy invalid repo option. This hints that the user needs to do something,
and prevent auto-selecting the first repo by default if the user does not
change the field. We expect that this can never be submiited because the branch
choices will not be populated on an unbound form. If it somehow were, the
`clean()` already validates that the repo URL can be found in
`repos_with_branches`, which won't be the case for our dummy value.

jobserver/forms.py

@@ -110,36 +110,57 @@ def __init__(self, repos_with_branches, *args, **kwargs):
         # repo is selected.
         self.repos_with_branches = repos_with_branches
 
-        # Construct the repo field.
-        repo_choices = [(r["url"], r["name"]) for r in self.repos_with_branches]
-        self.fields["repo"] = forms.ChoiceField(
-            label="Repo",
-            choices=repo_choices,
-        )
+        # Get selected repo and branch from POSTed form data, if available,
+        # so we can maintain that choice in the fields that we construct.
+        if self.data:
+            posted_repo = self.data.get("repo")
+            posted_branch = self.data.get("branch")
+        else:
+            posted_repo = None
+            posted_branch = None
 
         # Find the repo data dictionary matching the POSTed name, if any.
         if self.data and "repo" in self.data:
             try:
                 repo = next(
-                    (
-                        r
-                        for r in self.repos_with_branches
-                        if r["url"] == self.data["repo"]
-                    ),
+                    (r for r in self.repos_with_branches if r["url"] == posted_repo),
                 )
             except StopIteration:
                 raise forms.ValidationError(
                     "No matching repos found, please reload the page and try again"
                 )
         else:
-            repo = self.repos_with_branches[0]
+            repo = None
+
+        # Construct the repo field.
+        # Add a dummy invalid option to hint that the user needs to do
+        # something, and prevent auto-selecting the first repo by default if
+        # the user does not change the field.
+        dummy_repo_choice = (None, "Please select a repo...")
+
+        repo_choices = [dummy_repo_choice] + [
+            (r["url"], r["name"]) for r in self.repos_with_branches
+        ]
+        self.fields["repo"] = forms.ChoiceField(
+            label="Repo",
+            choices=repo_choices,
+            initial=repo["url"] if repo else None,
+        )
 
         # Construct the initial branch field, to be updated dynamically by
         # JavaScript each time a different repo is selected.
-        branch_choices = [(b, b) for b in repo["branches"]]
+        if repo:
+            branch_choices = [(b, b) for b in repo["branches"]]
+        else:
+            # On initial GET the dummy repo option will be selected, so there
+            # are no valid branches available. This prevents submitting the
+            # form without changing the repo, as the clean() validates that
+            # the selected repo's branch appears in self.repos_with_branches.
+            branch_choices = []
         self.fields["branch"] = forms.ChoiceField(
             label="Branch",
             choices=branch_choices,
+            initial=posted_branch,
         )
 
     def clean(self):

templates/workspace/create.html

@@ -36,9 +36,9 @@
 
         {% form_textarea field=form.purpose label="Workspace purpose" required=True %}
 
-        {% form_select field=form.repo label="GitHub repo" choices=form.repo.field.choices hint_text="If your repo doesn’t show up here, reach out to the OpenSAFELY team on Slack." required=True %}
+        {% form_select field=form.repo label="GitHub repo" choices=form.repo.field.choices hint_text="If your repo doesn’t show up here, reach out to the OpenSAFELY team on Slack." required=True selected=form.repo.initial %}
 
-        {% form_select field=form.branch label="Branch" choices=form.branch.field.choices required=True %}
+        {% form_select field=form.branch label="Branch" choices=form.branch.field.choices required=True selected=form.branch.initial %}
       </div>
     {% /card %}
 

tests/unit/jobserver/test_forms.py

@@ -101,9 +101,10 @@ def test_workspacecreateform_unbound():
     When the form is instantiated with multiple repos and branches and no data
     then:
         * The form is unbound.
-        * The repo choices include (url, name) pairs for each repo.
-        * The branch choices include (name, name) pairs for each branch
-          in the first repo (the default selection).
+        * The repo choices include (url, name) pairs for each repo and
+          a leading dummy option.
+        * The branch choices are set to None (as the leading dummy
+          option has no valid branches to select).
         * The repo and branch initial values are set to None.
     """
     repos_with_branches = [
@@ -122,23 +123,11 @@ def test_workspacecreateform_unbound():
 
     assert not form.is_bound
     assert form.fields["repo"].choices == [
+        (None, "Please select a repo..."),
         ("http://example.com/derp/test-repo", "test-repo"),
         ("http://example.com/derp/test-repo2", "test-repo2"),
     ]
-    assert form.fields["branch"].choices == [
-        (
-            "test-branch-1a",
-            "test-branch-1a",
-        ),
-        (
-            "test-branch-1b",
-            "test-branch-1b",
-        ),
-        (
-            "test-branch-1c",
-            "test-branch-1c",
-        ),
-    ]
+    assert form.fields["branch"].choices == []
     assert form.fields["repo"].initial is None
     assert form.fields["branch"].initial is None
 
@@ -155,9 +144,11 @@ def test_workspacecreateform_success(name, cleaned_name):
     When the form is instantiated with multiple repos and branches wth valid
     data selecting the second repo and third branch, then:
         * The form is bound.
-        * The repo choices include (url, name) pairs for each repo.
+        * The repo choices include (url, name) pairs for each repo and
+          a leading dummy option.
         * The branch choices include (name, name) pairs for each branch
           in the second repo (the one selected in data).
+        * The repo and branch initial values match those in the data.
         * The form validates.
         * The data ends up in the cleaned_data.
         * The name field is cleaned by converting to lower case.
@@ -185,6 +176,7 @@ def test_workspacecreateform_success(name, cleaned_name):
 
     assert form.is_bound
     assert form.fields["repo"].choices == [
+        (None, "Please select a repo..."),
         ("http://example.com/derp/test-repo", "test-repo"),
         ("http://example.com/derp/test-repo2", "test-repo2"),
     ]
@@ -202,6 +194,9 @@ def test_workspacecreateform_success(name, cleaned_name):
             "test-branch-2c",
         ),
     ]
+    assert form.fields["repo"].initial == "http://example.com/derp/test-repo2"
+    assert form.fields["branch"].initial == "test-branch-2c"
+
     assert form.is_valid()
     assert form.cleaned_data["name"] == cleaned_name
     assert form.cleaned_data["repo"] == "http://example.com/derp/test-repo2"