Bugfix: Use custom workspace form when cloning

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Devel
 
 * Bugfix: `run_anvil_audit` now raises a `CommandException` if an API call fails.
+* Bugfix: use the workspace form specified in the workspace adapter when cloning a workspace.
 
 ## 0.26.0 (2024-11-08)
 

anvil_consortium_manager/__init__.py

@@ -1 +1 @@
-__version__ = "0.26.1.dev0"
+__version__ = "0.26.1.dev1"

anvil_consortium_manager/forms.py

@@ -214,83 +214,35 @@ def __init__(self, workspace_choices=[], *args, **kwargs):
         )
 
 
-class WorkspaceCloneForm(Bootstrap5MediaFormMixin, forms.ModelForm):
-    """Form to create a new workspace on AnVIL by cloning an existing workspace."""
-
-    # Only allow billing groups where we can create a workspace.
-    billing_project = forms.ModelChoiceField(
-        queryset=models.BillingProject.objects.filter(has_app_as_user=True),
-        widget=autocomplete.ModelSelect2(
-            url="anvil_consortium_manager:billing_projects:autocomplete",
-            attrs={"data-theme": "bootstrap-5"},
-        ),
-        help_text="""Select the billing project in which the workspace should be created.
-                  Only billing projects where this app is a user are shown.""",
-    )
-
-    class Meta:
-        model = models.Workspace
-        fields = (
-            "billing_project",
-            "name",
-            "authorization_domains",
-            "note",
-        )
-        widgets = {
-            "billing_project": autocomplete.ModelSelect2(
-                url="anvil_consortium_manager:billing_projects:autocomplete",
-                attrs={"data-theme": "bootstrap-5"},
-            ),
-            "authorization_domains": autocomplete.ModelSelect2Multiple(
-                url="anvil_consortium_manager:managed_groups:autocomplete",
-                attrs={"data-theme": "bootstrap-5"},
-            ),
-        }
-        help_texts = {
-            "authorization_domains": """Select the authorization domain(s) to use for this workspace.
-                                     This cannot be changed after creation.""",
-        }
+class WorkspaceCloneFormMixin:
+    """Form mixing to perform cleaning when cloning a workspace."""
 
     def __init__(self, workspace_to_clone, *args, **kwargs):
         super().__init__(*args, **kwargs)
         # Save this so we can modify the authorization domains to include this workspace's auth domain(s).
         self.workspace_to_clone = workspace_to_clone
         # Add the list of required auth domains to the help text.
-        if self.workspace_to_clone.authorization_domains.exists():
+        if "authorization_domains" in self.fields and self.workspace_to_clone.authorization_domains.exists():
             auth_domain_names = self.workspace_to_clone.authorization_domains.values_list("name", flat=True)
             extra_text = " You must also include the authorization domain(s) from the original workspace ({}).".format(
                 ", ".join(auth_domain_names)
             )
             self.fields["authorization_domains"].help_text = self.fields["authorization_domains"].help_text + extra_text
 
-    def clean_authorization_domains(self):
-        """Verify that all authorization domains from the original workspace are selected."""
-        authorization_domains = self.cleaned_data["authorization_domains"]
+    def clean(self):
+        # Do the auth domain checks in clean because it is easier to handle inheritance.
+        # We'll still assign the error to the authorization_domains field.
+        cleaned_data = super().clean()
+        authorization_domains = self.cleaned_data.get("authorization_domains", [])
         required_authorization_domains = self.workspace_to_clone.authorization_domains.all()
         missing = [g for g in required_authorization_domains if g not in authorization_domains]
         if missing:
             msg = "Must contain all original workspace authorization domains: {}".format(
                 # ", ".join([g.name for g in self.workspace_to_clone.authorization_domains.all()])
                 ", ".join(required_authorization_domains.values_list("name", flat=True))
             )
-            raise ValidationError(msg)
-        return authorization_domains
-
-    def clean(self):
-        # Check for the same case insensitive name in the same billing project.
-        billing_project = self.cleaned_data.get("billing_project", None)
-        name = self.cleaned_data.get("name", None)
-        if (
-            billing_project
-            and name
-            and models.Workspace.objects.filter(
-                billing_project=billing_project,
-                name__iexact=name,
-            ).exists()
-        ):
-            # The workspace already exists - raise a Validation error.
-            raise ValidationError("Workspace with this Billing Project and Name already exists.")
-        return self.cleaned_data
+            self.add_error("authorization_domains", ValidationError(msg))
+        return cleaned_data
 
 
 class DefaultWorkspaceDataForm(forms.ModelForm):

anvil_consortium_manager/tests/test_forms.py

@@ -469,15 +469,22 @@ def test_invalid_missing_workspace(self):
         self.assertEqual(len(form.errors), 1)
 
 
-class WorkspaceCloneFormTest(TestCase):
-    """Tests for the WorkspaceCloneForm."""
-
-    form_class = forms.WorkspaceCloneForm
+class WorkspaceCloneFormMixinTest(TestCase):
+    """Tests for the WorkspaceCloneFormMixin."""
 
     def setUp(self):
         """Create a workspace to clone for use in tests."""
         self.workspace_to_clone = factories.WorkspaceFactory.create()
 
+    def get_form_class(self):
+        """Create a subclass using the mixin."""
+
+        class TestForm(forms.WorkspaceCloneFormMixin, forms.WorkspaceForm):
+            class Meta(forms.WorkspaceForm.Meta):
+                pass
+
+        return TestForm
+
     def test_valid_no_required_auth_domains(self):
         """Form is valid with a workspace to clone with no auth domains, and no auth domains selected."""
         billing_project = factories.BillingProjectFactory.create()
@@ -486,7 +493,7 @@ def test_valid_no_required_auth_domains(self):
             "name": "test-workspace",
             "authorization_domains": [],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_valid_no_required_auth_domains_with_one_selected_auth_domain(self):
@@ -498,7 +505,7 @@ def test_valid_no_required_auth_domains_with_one_selected_auth_domain(self):
             "name": "test-workspace",
             "authorization_domains": [new_auth_domain],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_valid_no_required_auth_domains_with_two_selected_auth_domains(self):
@@ -511,7 +518,7 @@ def test_valid_no_required_auth_domains_with_two_selected_auth_domains(self):
             "name": "test-workspace",
             "authorization_domains": [new_auth_domain_1, new_auth_domain_2],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_valid_one_required_auth_domains(self):
@@ -524,7 +531,7 @@ def test_valid_one_required_auth_domains(self):
             "name": "test-workspace",
             "authorization_domains": [auth_domain],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_invalid_one_required_auth_domains_no_auth_domains_selected(self):
@@ -537,7 +544,7 @@ def test_invalid_one_required_auth_domains_no_auth_domains_selected(self):
             "name": "test-workspace",
             "authorization_domains": [],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertFalse(form.is_valid())
         self.assertEqual(len(form.errors), 1)
         self.assertIn("authorization_domains", form.errors)
@@ -559,7 +566,7 @@ def test_invalid_one_required_auth_domains_different_auth_domains_selected(self)
             "name": "test-workspace",
             "authorization_domains": [other_auth_domain],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertFalse(form.is_valid())
         self.assertEqual(len(form.errors), 1)
         self.assertIn("authorization_domains", form.errors)
@@ -581,7 +588,7 @@ def test_valid_one_required_auth_domains_with_extra_selected_auth_domain(self):
             "name": "test-workspace",
             "authorization_domains": [auth_domain, new_auth_domain],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_valid_two_required_auth_domains(self):
@@ -595,7 +602,7 @@ def test_valid_two_required_auth_domains(self):
             "name": "test-workspace",
             "authorization_domains": [auth_domain_1, auth_domain_2],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
     def test_invalid_two_required_auth_domains_no_auth_domains_selected(self):
@@ -609,7 +616,7 @@ def test_invalid_two_required_auth_domains_no_auth_domains_selected(self):
             "name": "test-workspace",
             "authorization_domains": [],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertFalse(form.is_valid())
         self.assertEqual(len(form.errors), 1)
         self.assertIn("authorization_domains", form.errors)
@@ -632,7 +639,7 @@ def test_invalid_two_required_auth_domains_one_auth_domain_selected(self):
             "name": "test-workspace",
             "authorization_domains": [auth_domain_1],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertFalse(form.is_valid())
         self.assertEqual(len(form.errors), 1)
         self.assertIn("authorization_domains", form.errors)
@@ -657,7 +664,7 @@ def test_invalid_two_required_auth_domains_different_auth_domains_selected(self)
             "name": "test-workspace",
             "authorization_domains": [other_auth_domain_1, other_auth_domain_2],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertFalse(form.is_valid())
         self.assertEqual(len(form.errors), 1)
         self.assertIn("authorization_domains", form.errors)
@@ -681,9 +688,80 @@ def test_valid_two_required_auth_domains_with_extra_selected_auth_domain(self):
             "name": "test-workspace",
             "authorization_domains": [auth_domain_1, auth_domain_2, new_auth_domain],
         }
-        form = self.form_class(self.workspace_to_clone, data=form_data)
+        form = self.get_form_class()(self.workspace_to_clone, data=form_data)
         self.assertTrue(form.is_valid())
 
+    def test_custom_workspace_form_with_clean_auth_domain_error_in_custom_form(self):
+        # Create a test workspace form with a custom clean_authorization_domains method.
+        class TestWorkspaceForm(forms.WorkspaceForm):
+            class Meta:
+                model = models.Workspace
+                fields = ("billing_project", "name", "authorization_domains")
+
+            def clean_authorization_domains(self):
+                # No return statement because the test never gets there, and it breaks coverage.
+                authorization_domains = self.cleaned_data.get("authorization_domains")
+                if authorization_domains:
+                    for auth_domain in authorization_domains:
+                        print(auth_domain.name)
+                        if auth_domain.name == "invalid-name":
+                            raise forms.ValidationError("Test error")
+
+        class TestWorkspaceCloneForm(forms.WorkspaceCloneFormMixin, TestWorkspaceForm):
+            class Meta(TestWorkspaceForm.Meta):
+                pass
+
+        auth_domain = factories.ManagedGroupFactory.create(name="invalid-name")
+        billing_project = factories.BillingProjectFactory.create()
+        form_data = {
+            "billing_project": billing_project,
+            "name": "test-workspace",
+            "authorization_domains": [auth_domain],
+        }
+        form = TestWorkspaceCloneForm(self.workspace_to_clone, data=form_data)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(len(form.errors), 1)
+        self.assertIn("authorization_domains", form.errors)
+        self.assertEqual(len(form.errors["authorization_domains"]), 1)
+        self.assertIn(
+            "Test error",
+            form.errors["authorization_domains"][0],
+        )
+
+    def test_custom_workspace_form_with_clean_auth_domain_error_in_mixin(self):
+        # Create a test workspace form with a custom clean_authorization_domains method.
+        class TestWorkspaceForm(forms.WorkspaceForm):
+            class Meta:
+                model = models.Workspace
+                fields = ("billing_project", "name", "authorization_domains")
+
+            def clean_authorization_domains(self):
+                authorization_domains = self.cleaned_data.get("authorization_domains")
+                return authorization_domains
+
+        class TestWorkspaceCloneForm(forms.WorkspaceCloneFormMixin, TestWorkspaceForm):
+            class Meta(TestWorkspaceForm.Meta):
+                pass
+
+        auth_domain = factories.ManagedGroupFactory.create(name="other-name")
+        self.workspace_to_clone.authorization_domains.add(auth_domain)
+        billing_project = factories.BillingProjectFactory.create()
+        form_data = {
+            "billing_project": billing_project,
+            "name": "test-workspace",
+            "authorization_domains": [],
+        }
+        form = TestWorkspaceCloneForm(self.workspace_to_clone, data=form_data)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(len(form.errors), 1)
+        self.assertIn("authorization_domains", form.errors)
+        self.assertEqual(len(form.errors["authorization_domains"]), 1)
+        self.assertIn(
+            "contain all original workspace authorization domains",
+            form.errors["authorization_domains"][0],
+        )
+        self.assertIn(auth_domain.name, form.errors["authorization_domains"][0])
+
 
 class GroupGroupMembershipFormTest(TestCase):
     form_class = forms.GroupGroupMembershipForm

anvil_consortium_manager/tests/test_views.py

@@ -8222,7 +8222,7 @@ def test_adapter_does_not_create_objects_if_workspace_data_form_invalid(self):
         self.assertEqual(app_models.TestWorkspaceData.objects.count(), 0)
         self.assertEqual(len(responses.calls), 0)
 
-    def test_adapter_custom_form_class(self):
+    def test_adapter_custom_workspace_form_class(self):
         """No workspace is created if custom workspace form is invalid."""
         # Overriding settings doesn't work, because appconfig.ready has already run and
         # registered the default adapter. Instead, unregister the default and register the
@@ -10078,7 +10078,9 @@ def test_has_form_in_context(self):
             )
         )
         self.assertTrue("form" in response.context_data)
-        self.assertIsInstance(response.context_data["form"], forms.WorkspaceCloneForm)
+        # self.assertIsInstance(response.context_data["form"], (forms.WorkspaceForm, forms.WorkspaceCloneFormMixin))
+        self.assertIsInstance(response.context_data["form"], forms.WorkspaceForm)
+        self.assertIsInstance(response.context_data["form"], forms.WorkspaceCloneFormMixin)
 
     def test_has_formset_in_context(self):
         """Response includes a formset for the workspace_data model."""
@@ -10782,7 +10784,7 @@ def test_not_user_of_billing_project(self):
         form = response.context_data["form"]
         self.assertFalse(form.is_valid())
         self.assertIn("billing_project", form.errors.keys())
-        self.assertIn("valid choice", form.errors["billing_project"][0])
+        self.assertIn("must have has_app_as_user set to True", form.errors["billing_project"][0])
         # No workspace was created.
         self.assertEqual(models.Workspace.objects.count(), 1)
         self.assertIn(self.workspace_to_clone, models.Workspace.objects.all())
@@ -10906,6 +10908,56 @@ def test_adapter_does_not_create_objects_if_workspace_data_form_invalid(self):
         self.assertIn(self.workspace_to_clone, models.Workspace.objects.all())
         self.assertEqual(app_models.TestWorkspaceData.objects.count(), 0)
 
+    def test_adapter_custom_workspace_form_class(self):
+        """Form uses the custom workspace form as a superclass."""
+        workspace_adapter_registry.unregister(DefaultWorkspaceAdapter)
+        workspace_adapter_registry.register(TestWorkspaceAdapter)
+        self.workspace_type = "test"
+        self.client.force_login(self.user)
+        response = self.client.get(
+            self.get_url(
+                self.workspace_to_clone.billing_project.name,
+                self.workspace_to_clone.name,
+                self.workspace_type,
+            )
+        )
+        self.assertTrue("form" in response.context_data)
+        self.assertIsInstance(response.context_data["form"], app_forms.TestWorkspaceForm)
+        self.assertIsInstance(response.context_data["form"], forms.WorkspaceCloneFormMixin)
+
+    def test_adapter_custom_workspace_form_with_error_in_workspace_form(self):
+        """Form uses the custom workspace form as a superclass."""
+        workspace_adapter_registry.unregister(DefaultWorkspaceAdapter)
+        workspace_adapter_registry.register(TestWorkspaceAdapter)
+        billing_project = factories.BillingProjectFactory.create()
+        self.workspace_type = "test"
+        self.client.force_login(self.user)
+        response = self.client.post(
+            self.get_url(
+                self.workspace_to_clone.billing_project.name,
+                self.workspace_to_clone.name,
+                self.workspace_type,
+            ),
+            {
+                "billing_project": billing_project.pk,
+                "name": "test-fail",
+                # Default workspace data for formset.
+                "workspacedata-TOTAL_FORMS": 1,
+                "workspacedata-INITIAL_FORMS": 0,
+                "workspacedata-MIN_NUM_FORMS": 1,
+                "workspacedata-MAX_NUM_FORMS": 1,
+                "workspacedata-0-study_name": "test study",
+            },
+        )
+        self.assertEqual(response.status_code, 200)
+        form = response.context_data["form"]
+        self.assertFalse(form.is_valid())
+        self.assertIn("name", form.errors.keys())
+        self.assertIn("Workspace name cannot be", form.errors["name"][0])
+        self.assertEqual(models.Workspace.objects.count(), 1)  # the workspace to clone
+        self.assertEqual(app_models.TestWorkspaceData.objects.count(), 0)
+        self.assertEqual(len(responses.calls), 0)
+
     def test_workspace_to_clone_does_not_exist_on_anvil(self):
         """Shows a method if an AnVIL API 404 error occurs."""
         billing_project = factories.BillingProjectFactory.create()