diff --git a/apps/forms/base.py b/apps/forms/base.py index a6c74898..78d02895 100644 --- a/apps/forms/base.py +++ b/apps/forms/base.py @@ -45,6 +45,13 @@ def _setup_form_fields(self): # Handle name self.fields["name"].initial = "" + # Initialize the tags field to existing tags or empty list + if self.instance and self.instance.pk: + self.instance.refresh_from_db() + self._original_tags = list(self.instance.tags.all()) + else: + self._original_tags = [] + def _setup_form_helper(self): # Create a footer for submit form or cancel self.footer = Div( @@ -83,6 +90,13 @@ def clean_note_on_linkonly_privacy(self): return note_on_linkonly_privacy + def clean_tags(self): + cleaned_data = super().clean() + tags = cleaned_data.get("tags", None) + if tags is None: + return [] + return tags + def validate_subdomain(self, subdomain_input): # If user did not input subdomain, set it to our standard release name if not subdomain_input: @@ -117,6 +131,16 @@ def validate_subdomain(self, subdomain_input): return SubdomainTuple(subdomain_input, True) + @property + def changed_data(self): + # Override the default changed_data to handle the tags field + changed_data = super().changed_data + if "tags" in changed_data: + new_tags = self.cleaned_data.get("tags", []) + if list(new_tags) == self._original_tags: + changed_data.remove("tags") + return changed_data + class Meta: # Specify model to be used model = BaseAppInstance diff --git a/apps/forms/dash.py b/apps/forms/dash.py index e552e158..544bd689 100644 --- a/apps/forms/dash.py +++ b/apps/forms/dash.py @@ -41,6 +41,15 @@ def _setup_form_helper(self): self.helper.layout = Layout(body, self.footer) + @property + def changed_data(self): + # Override the default changed_data to handle that volume not part of this app type. + # TODO: Consider adding to all app forms that do not contain volume. + changed_data = super().changed_data + if "volume" in changed_data: + changed_data.remove("volume") + return changed_data + class Meta: model = DashInstance fields = [ diff --git a/apps/helpers.py b/apps/helpers.py index 8a8bb353..555f7e8b 100644 --- a/apps/helpers.py +++ b/apps/helpers.py @@ -355,7 +355,7 @@ def get_URI(instance): @transaction.atomic -def create_instance_from_form(form, project, app_slug, app_id=None): +def create_instance_from_form(form, project, app_slug, app_id=None) -> int: """ Create or update an instance from a form. This function handles both the creation of new instances and the updating of existing ones based on the presence of an app_id. @@ -374,6 +374,9 @@ def create_instance_from_form(form, project, app_slug, app_id=None): """ from .tasks import deploy_resource + assert form is not None, "This function requires a form object" + assert project is not None, "This function requires a project object" + new_app = app_id is None logger.debug(f"Creating or updating a user app via UI form for app_id={app_id}, new_app={new_app}") @@ -416,24 +419,30 @@ def create_instance_from_form(form, project, app_slug, app_id=None): instance = form.save(commit=False) # Handle status creation or retrieval - # TODO: Status. Check this use. + # TODO: Remove after well tested # We no longer update the app status based on user actions. # status = get_or_create_status(instance, app_id) + # TODO: Clean up some of the asserts after well tested # Retrieve or create the subdomain subdomain, created = Subdomain.objects.get_or_create( subdomain=subdomain_name, project=project, is_created_by_user=is_created_by_user ) + assert subdomain is not None + assert subdomain.subdomain == subdomain_name + + subdomain = Subdomain.objects.get(subdomain=subdomain_name, project=project, is_created_by_user=is_created_by_user) + assert subdomain is not None + assert subdomain.subdomain == subdomain_name + assert subdomain.project.name == project.name, f"2 {subdomain.project} should equal {project.name}" if not new_app: - # Previously: if app_id: handle_subdomain_change(instance, subdomain, subdomain_name) app_slug = handle_shiny_proxy_case(instance, app_slug, app_id) app = get_app(app_slug) - # TODO: Status. Check this use: setup_instance(instance, subdomain, app, project, user_action) id = save_instance_and_related_data(instance, form) @@ -441,9 +450,6 @@ def create_instance_from_form(form, project, app_slug, app_id=None): if do_deploy: logger.debug(f"Now deploying resource app with app_id = {app_id}") - - # TODO: Status. Also set the last user action to Updating - deploy_resource.delay(instance.serialize()) else: logger.debug(f"Not re-deploying this app with app_id = {app_id}") @@ -471,12 +477,10 @@ def handle_subdomain_change(instance: Any, subdomain: str, subdomain_name: str) from .tasks import delete_resource assert instance is not None, "instance is required" - # type(instance) is for example DashInstance - # assert instance.subdomain is not None, f"subdomain is required but is None for instance {instance.name}" if instance.subdomain is None: - # TODO: Check this. It is possible for subdomain to be missing. - # But is this valid? + # The subdomain is not yet created, nothing to do + logger.debug("The subdomain is not yet created, nothing to do") return if instance.subdomain.subdomain != subdomain_name: @@ -507,16 +511,14 @@ def get_app(app_slug): raise ValueError(f"App with slug {app_slug} not found") -# TODO: Status. def setup_instance(instance, subdomain, app, project, user_action=None, is_created_by_user=False): instance.subdomain = subdomain instance.app = app instance.chart = instance.app.chart instance.project = project instance.owner = project.owner - # TODO: Status. Is this unit tested? instance.latest_user_action = user_action - # TODO: Status. We do not set app status any longer. + # TODO: Remove after well tested # instance.app_status = status diff --git a/apps/tests/test_app_helper_utils.py b/apps/tests/test_app_helper_utils.py index db967ca0..879aa35e 100644 --- a/apps/tests/test_app_helper_utils.py +++ b/apps/tests/test_app_helper_utils.py @@ -9,6 +9,7 @@ from projects.models import Flavor, Project from ..app_registry import APP_REGISTRY +from ..forms import DashForm from ..helpers import create_instance_from_form, get_subdomain_name from ..models import Apps, DashInstance, K8sUserAppStatus, Subdomain from ..types_.subdomain import SubdomainTuple @@ -58,10 +59,9 @@ def test_create_instance_from_form_valid_input(self): self.assertIsNotNone(id) self.assertTrue(id > 0) - # Get app instance and verify the status codes + # Get app instance and verify the instance properties including status codes app_instance = DashInstance.objects.get(pk=id) - # Validate app instance properties self.assertIsNotNone(app_instance) self.assertEqual(app_instance.latest_user_action, "Creating") self.assertIsNone(app_instance.k8s_user_app_status) @@ -79,10 +79,16 @@ def test_create_instance_from_form_valid_input(self): self.assertTrue( subdomain_name.startswith("r"), f"The subdomain should begin with r but was {subdomain_name}" ) + self.assertFalse(app_instance.subdomain.is_created_by_user) mock_task.assert_called_once() +# Mock the tasks that manipulate k8s resources. +# Note that these are passed to the test functions in reverse order. +# The delete_resource task is used sync (wuthout delay) in helpers. +@patch("apps.tasks.deploy_resource.delay") +@patch("apps.tasks.delete_resource") class UpdateExistingAppInstanceTestCase(TestCase): """ Test case for helper function create_instance_from_form @@ -102,15 +108,30 @@ def setUp(self) -> None: user_can_delete=False, ) - self.subdomain_name = "test-subdomain" - subdomain = Subdomain.objects.create(subdomain=self.subdomain_name) + # Define the original values + self.name = "test-app-name-original" + self.description = "app-form-description" + self.port = 8000 + self.image = "test-image-orig" + self.subdomain_name = "test-subdomain-update-app" + self.source_code_url = "https://someurlthatdoesnotexist.com" + + is_created_by_user = True + subdomain = Subdomain.objects.create( + subdomain=self.subdomain_name, project=self.project, is_created_by_user=is_created_by_user + ) k8s_user_app_status = K8sUserAppStatus.objects.create() + self.app_instance = DashInstance.objects.create( + app=self.app, access="public", owner=self.user, - name="test-app-name-original", - app=self.app, + name=self.name, + description=self.description, + port=self.port, + image=self.image, + source_code_url=self.source_code_url, project=self.project, subdomain=subdomain, k8s_user_app_status=k8s_user_app_status, @@ -120,90 +141,198 @@ def setUp(self) -> None: self.assertTrue(self.app_instance.id > 0) self.assertIsNotNone(self.app_instance.subdomain) self.assertIsNotNone(self.app_instance.subdomain.subdomain) + self.assertEqual(self.app_instance.subdomain.subdomain, self.subdomain_name) + self.assertTrue(self.app_instance.subdomain.is_created_by_user) - def test_update_instance_from_form_modify_port_should_redeploy(self): + def test_update_instance_from_form_modify_port_should_redeploy(self, mock_delete, mock_deploy): """ Test function create_instance_from_form to update an existing app instance to modify properties that should result in a re-deployment. + The modified property in this test is the Port. """ # Create the form data - # TODO: Need to investigate subdomain. Does it always need to be included? - # If included and the same, then get Subdomain already exists. Please choose another one. + # Fields requiring special consideration: tags, volume, subdomain + # In the below data dict, we modify the app port: data = { - "name": "test-app-name-new", - "description": "app-form-description", + "name": self.name, + "description": self.description, "access": "public", "port": 9999, - "image": "some-image", - "source_code_url": "https://someurlthatdoesnotexist.com", + "image": self.image, + "source_code_url": self.source_code_url, + "subdomain": self.subdomain_name, } - _, form_class = APP_REGISTRY.get(self.app_slug) - form = form_class(data, project_pk=self.project.pk) - - self.assertTrue(form.is_valid(), f"The form should be valid but has errors: {form.errors}") - - with patch("apps.tasks.deploy_resource.delay") as mock_task: - id = create_instance_from_form(form, self.project, self.app_slug, app_id=self.app_instance.id) + changed_fields = ["port"] - self.assertIsNotNone(id) - self.assertTrue(id > 0) + # Apply the form and validate the result + self._verify_update_instance_from_form(data, changed_fields) - # Get app instance and verify the status codes - app_instance = DashInstance.objects.get(pk=id) + # Modifying the port should cause a re-deploy: + mock_deploy.assert_called_once() + # Not modifying the subdomain should not cause a delete: + mock_delete.assert_not_called() - # Validate app instance properties - self.assertIsNotNone(app_instance) - self.assertEqual(app_instance.latest_user_action, "Changing") - self.assertIsNone(app_instance.k8s_user_app_status) - self.assertEqual(app_instance.name, data.get("name")) - self.assertEqual(app_instance.description, data.get("description")) - self.assertEqual(app_instance.access, data.get("access")) - self.assertEqual(app_instance.port, data.get("port")) - self.assertEqual(app_instance.image, data.get("image")) - self.assertEqual(app_instance.source_code_url, data.get("source_code_url")) - - mock_task.assert_called_once() - - def test_update_instance_from_form_modify_image_should_redeploy(self): + def test_update_instance_from_form_modify_image_should_redeploy(self, mock_delete, mock_deploy): """ Test function create_instance_from_form to update an existing app instance to modify properties that should result in a re-deployment. + The modified property in this test is the Image. """ - pass + # Create the form data + # Fields requiring special consideration: tags, volume, subdomain + # In the below data dict, we modify the app image: + data = { + "name": self.name, + "description": self.description, + "access": "public", + "port": self.port, + "image": "test-image-new", + "source_code_url": self.source_code_url, + "subdomain": self.subdomain_name, + } + + changed_fields = ["image"] - def test_update_instance_from_form_modify_subdomain_should_redeploy(self): + # Apply the form and validate the result + self._verify_update_instance_from_form(data, changed_fields) + + # Modifying the image should cause a re-deploy: + mock_deploy.assert_called_once() + # Not modifying the subdomain should not cause a delete: + mock_delete.assert_not_called() + + def test_update_instance_from_form_modify_subdomain_should_redeploy(self, mock_delete, mock_deploy): """ Test function create_instance_from_form to update an existing app instance to modify properties that should result in a re-deployment. + The modified property in this test is the Subdomain. + Modifying the subdomain also results in a delete resource call. """ - # is_created_by_user = False - # subdomain = SubdomainTuple(self.subdomain_name, is_created_by_user) + # Create the form data + # Fields requiring special consideration: tags, volume, subdomain + # In the below data dict, we modify the app subdomain: + data = { + "name": self.name, + "description": self.description, + "access": "public", + "port": self.port, + "image": self.image, + "source_code_url": self.source_code_url, + "subdomain": "test-subdomain-update-app-new", + } + + changed_fields = ["subdomain"] - pass + # Apply the form and validate the result + self._verify_update_instance_from_form(data, changed_fields) - def test_update_instance_from_form_modify_no_redeploy_values(self): + # Modifying the subdomain should cause a re-deploy: + mock_deploy.assert_called_once() + # Modifying the subdomain SHOULD cause a delete: + mock_delete.assert_called_once() + + def test_update_instance_from_form_modify_no_redeploy_values(self, mock_delete, mock_deploy): """ Test function create_instance_from_form to update an existing app instance - to modify only properties that do not result in a re-deployment. + to modify only properties that should NOT result in a re-deployment. """ - # TODO: - pass + f = DashForm() + self.assertIsNotNone(f) + + model_class, form_class = APP_REGISTRY.get(self.app_slug) + + instance = model_class.objects.get(pk=self.app_instance.id) + self.assertIsNotNone(instance) + self.assertIsInstance(instance, DashInstance) + + # Create the form data + # Fields requiring special consideration: tags, volume, subdomain + # In the below data dict, we modify only properties that do not lead to re-deployment: + data = { + "name": "test-app-name-new", + "description": "app-form-description-new", + "access": "public", + "port": self.port, + "image": self.image, + "source_code_url": "https://someurlthatdoesnotexist.com/new", + "subdomain": self.subdomain_name, + "tags": None, + } + + changed_fields = ["name", "description", "source_code_url"] + + # Apply the form and validate the result + self._verify_update_instance_from_form(data, changed_fields) + + # Not modifying any re-deployment fields should NOT cause a re-deploy: + mock_deploy.assert_not_called() + # Not modifying the subdomain should not cause a delete: + mock_delete.assert_not_called() + + def _verify_update_instance_from_form(self, data: dict, changed_fields: list[str]) -> None: + """Helper function to verify the result of the create_instance_from_form function tests.""" + + f = DashForm() + self.assertIsNotNone(f) + + model_class, form_class = APP_REGISTRY.get(self.app_slug) + + instance = model_class.objects.get(pk=self.app_instance.id) + self.assertIsNotNone(instance) + self.assertIsInstance(instance, DashInstance) + + # Perform the form validation and tests using an existing app instance + form = form_class(data, project_pk=self.project.pk, instance=instance) + + self.assertTrue(form.is_valid(), f"The form should be valid but has errors: {form.errors}") + + self.assertIsNotNone(form.changed_data) + self.assertEqual(form.changed_data, changed_fields) + + id = create_instance_from_form(form, self.project, self.app_slug, app_id=self.app_instance.id) + + self.assertIsNotNone(id) + self.assertTrue(id > 0) + + # Get app instance and verify the instance properties including status codes + app_instance = DashInstance.objects.get(pk=id) + + self.assertIsNotNone(app_instance) + self.assertEqual(app_instance.latest_user_action, "Changing") + self.assertIsNotNone(app_instance.k8s_user_app_status) + self.assertIsNone(app_instance.k8s_user_app_status.status) + self.assertEqual(app_instance.name, data.get("name")) + self.assertEqual(app_instance.description, data.get("description")) + self.assertEqual(app_instance.access, data.get("access")) + self.assertEqual(app_instance.port, data.get("port")) + self.assertEqual(app_instance.image, data.get("image")) + self.assertEqual(app_instance.source_code_url, data.get("source_code_url")) + + # Verify the subdomain. Determine from the data dict. + self.assertIsNotNone(app_instance.subdomain) + self.assertIsNotNone(app_instance.subdomain.subdomain) + + expected_subdomain_name = data.get("subdomain", None) + if expected_subdomain_name is None: + # The subdomain was not changed from the original + expected_subdomain_name = self.subdomain_name - # mock_task.assert_ not called + self.assertEqual(app_instance.subdomain.subdomain, expected_subdomain_name) + self.assertTrue(app_instance.subdomain.is_created_by_user) @pytest.mark.django_db def test_get_subdomain_name(): """Test function get_subdomain_name using form data with a subdomain.""" - expected_subomain_name = "test-subdomain" - is_created_by_user = False - subdomain = SubdomainTuple(expected_subomain_name, is_created_by_user) + expected_subdomain_name = "test-subdomain-get-from-form" + is_created_by_user = True + subdomain = SubdomainTuple(expected_subdomain_name, is_created_by_user) data = { "name": "app-form-name", @@ -223,9 +352,9 @@ def test_get_subdomain_name(): subdomain_name, is_created_by_user = get_subdomain_name(form) assert ( - subdomain_name == expected_subomain_name + subdomain_name == expected_subdomain_name ), f"The determined subdomain name {subdomain_name} should equal \ - the expected name {expected_subomain_name}" + the expected name {expected_subdomain_name}" # The function overrides the input is_created_by_user setting it to True # because the user specified the subdomain. diff --git a/apps/views.py b/apps/views.py index 36b8bd63..49700e0a 100644 --- a/apps/views.py +++ b/apps/views.py @@ -241,7 +241,7 @@ def get(self, request, project, app_slug, app_id=None): @transaction.atomic def post(self, request, project, app_slug, app_id=None): - # App id is used when updataing an instance + # App id is used when updating an existing app instance # TODO Same as in get method project_slug = project