Merge pull request #194 from nautobot/updated-tests

test: Updated unit tests and minor fixes

nautobot_design_builder/changes.py

@@ -0,0 +1,75 @@
+"""Logic used to track changes and change logging."""
+
+from contextlib import contextmanager
+from typing import TYPE_CHECKING
+
+from django.db import models as django_models
+
+if TYPE_CHECKING:
+    from nautobot_design_builder.design import ModelInstance
+
+
+def _get_change_value(value):
+    if isinstance(value, django_models.Manager):
+        value = {item.pk for item in value.all()}
+    return value
+
+
+@contextmanager
+def change_log(model_instance: "ModelInstance", attr_name: str):
+    """Log changes for a field.
+
+    This context manager will record the value of a field prior to a change
+    as well as the value after the change. If the values are different then
+    a change record is added to the underlying model instance.
+
+    Args:
+        model_instance (ModelInstance): The model instance that is being updated.
+        attr_name (str): The attribute to be updated.
+    """
+    old_value = _get_change_value(getattr(model_instance.design_instance, attr_name))
+    yield
+    new_value = _get_change_value(getattr(model_instance.design_instance, attr_name))
+    if old_value != new_value:
+        if isinstance(old_value, set):
+            model_instance.design_metadata.changes[attr_name] = {
+                "old_items": old_value,
+                "new_items": new_value,
+            }
+            # Many-to-Many changes need to be logged on the parent,
+            # and this won't happen implicitly so we log the changes
+            # explicitly here.
+            #
+            # TODO: This has been commented out because I *think* that it is
+            # no longer needed since their is now a journal log created in the
+            # create_child method.
+            # model_instance.design_metadata.environment.journal.log(model_instance)
+        else:
+            model_instance.design_metadata.changes[attr_name] = {
+                "old_value": old_value,
+                "new_value": new_value,
+            }
+
+
+def revert_changed_dict(current_value: dict, original_value: dict, changed_value: dict) -> dict:
+    """Create a new dictionary that correctly returns a changed dictionary to its expected value.
+
+    In many cases Nautobot model object have dictionary attributes (configuration contexts, secret
+    params, etc). The design builder will attempt to revert design related attributes within
+    these dictionaries. Any dictionary items changed by the design will be reverted to their original
+    value and any items added outside of the process will be left alone. Dictionary items added
+    by the design wil be removed.
+
+    Args:
+        current_value (dict): The dictionary value as it is right now.
+        original_value (dict): The dictionary value before the change.
+        changed_value (dict): The dictionary value after the change.
+
+    Returns:
+        dict: The dictionary as we expect it to be without the design changes.
+    """
+    original_keys = set(original_value.keys())
+    changed_keys = set(changed_value.keys())
+    current_keys = set(current_value.keys())
+    design_keys = changed_keys.union(original_keys)
+    return {**{key: current_value[key] for key in current_keys.difference(design_keys)}, **original_value}

nautobot_design_builder/fields.py

@@ -39,7 +39,6 @@
 """
 
 from abc import ABC, abstractmethod
-from contextlib import contextmanager
 from typing import Any, Mapping, Type, TYPE_CHECKING
 
 from django.db import models as django_models
@@ -51,55 +50,14 @@
 from nautobot.core.graphql.utils import str_to_var_name
 from nautobot.extras.models import Relationship, RelationshipAssociation
 
+from nautobot_design_builder.changes import change_log
 from nautobot_design_builder.errors import DesignImplementationError
 from nautobot_design_builder.debug import debug_set
 
 if TYPE_CHECKING:
     from .design import ModelInstance
 
 
-def _get_change_value(value):
-    if isinstance(value, django_models.Manager):
-        value = {item.pk for item in value.all()}
-    return value
-
-
-@contextmanager
-def change_log(model_instance: "ModelInstance", attr_name: str):
-    """Log changes for a field.
-
-    This context manager will record the value of a field prior to a change
-    as well as the value after the change. If the values are different then
-    a change record is added to the underlying model instance.
-
-    Args:
-        model_instance (ModelInstance): The model instance that is being updated.
-        attr_name (str): The attribute to be updated.
-    """
-    old_value = _get_change_value(getattr(model_instance.design_instance, attr_name))
-    yield
-    new_value = _get_change_value(getattr(model_instance.design_instance, attr_name))
-    if old_value != new_value:
-        if isinstance(old_value, set):
-            model_instance.design_metadata.changes[attr_name] = {
-                "old_items": old_value,
-                "new_items": new_value,
-            }
-            # Many-to-Many changes need to be logged on the parent,
-            # and this won't happen implicitly so we log the changes
-            # explicitly here.
-            #
-            # TODO: This has been commented out because I *think* that it is
-            # no longer needed since their is now a journal log created in the
-            # create_child method.
-            # model_instance.design_metadata.environment.journal.log(model_instance)
-        else:
-            model_instance.design_metadata.changes[attr_name] = {
-                "old_value": old_value,
-                "new_value": new_value,
-            }
-
-
 class ModelField(ABC):
     """This represents any type of field (attribute or relationship) on a Nautobot model.
 

nautobot_design_builder/models.py

@@ -13,6 +13,8 @@
 from nautobot.extras.models import Job as JobModel, JobResult, Status, StatusField
 from nautobot.extras.utils import extras_features
 
+from nautobot_design_builder.changes import revert_changed_dict
+
 from .util import get_created_and_last_updated_usernames_for_model
 from . import choices
 from .errors import DesignValidationError
@@ -555,31 +557,6 @@ class Meta:  # noqa:D106
             ("change_set", "_design_object_type", "_design_object_id"),
         ]
 
-    @staticmethod
-    def update_current_value_from_dict(current_value, added_value, removed_value):
-        """Update current value if it's a dictionary.
-
-        The removed_value keys (the original one) are going to be recovered, the added_value ones
-        will be reverted, and the current_value ones that were not added by the design will be kept.
-        """
-        keys_to_remove = []
-        for key in current_value:
-            if key in added_value:
-                if key in removed_value:
-                    # Reverting the value of keys that existed before and the design deployment modified
-                    current_value[key] = removed_value[key]
-                else:
-                    keys_to_remove.append(key)
-
-        # Removing keys that were added by the design.
-        for key in keys_to_remove:
-            del current_value[key]
-
-        # Recovering old keys that the ChangeRecord deleted.
-        for key in removed_value:
-            if key not in added_value:
-                current_value[key] = removed_value[key]
-
     def revert(self, local_logger: logging.Logger = logger):  # pylint: disable=too-many-branches
         """Revert the changes that are represented in this change record.
 
@@ -627,7 +604,10 @@ def revert(self, local_logger: logging.Logger = logger):  # pylint: disable=too-
             )
         else:
             local_logger.info("Reverting change record", extra={"object": self.design_object})
-            for attr_name, change in self.changes.items():
+            changes = self.changes
+            if changes is None:
+                changes = {}
+            for attr_name, change in changes.items():
                 current_value = getattr(self.design_object, attr_name)
                 if "old_items" in change:
                     old_items = set(change["old_items"])
@@ -637,19 +617,16 @@ def revert(self, local_logger: logging.Logger = logger):  # pylint: disable=too-
                     current_items -= added_items
                     current_value.set(current_value.filter(pk__in=current_items))
                 else:
-                    old_value = change["old_value"]
-                    new_value = change["new_value"]
-
-                    if isinstance(old_value, dict):
+                    if isinstance(change["old_value"], dict):
                         # config-context like thing, only change the keys
                         # that were added/changed
-                        self.update_current_value_from_dict(
-                            current_value=current_value,
-                            added_value=new_value,
-                            removed_value=old_value if old_value else {},
+                        setattr(
+                            self.design_object,
+                            attr_name,
+                            revert_changed_dict(current_value, change["old_value"], change["new_value"]),
                         )
                     else:
-                        setattr(self.design_object, attr_name, old_value)
+                        setattr(self.design_object, attr_name, change["old_value"])
 
                 self.design_object.save()
                 local_logger.info(

nautobot_design_builder/tests/test_changes.py

@@ -0,0 +1,37 @@
+"""Tests for change utilities."""
+
+from unittest import TestCase
+
+from nautobot_design_builder.changes import revert_changed_dict
+
+
+class TestRevertChangedDict(TestCase):
+    """TestCase for the revert_changed_dict utility."""
+
+    def test_added_key(self):
+        original = {"key1": "initial-value"}
+        changed = {"key1": "initial-value", "key2": "new-value"}
+        current = {**changed}
+        got = revert_changed_dict(current, original, changed)
+        self.assertDictEqual(original, got)
+
+    def test_extra_key(self):
+        original = {"key1": "initial-value"}
+        changed = {"key1": "initial-value", "key2": "new-value"}
+        current = {"key1": "initial-value", "key2": "new-value", "key3": "key3-value"}
+        got = revert_changed_dict(current, original, changed)
+        self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got)
+
+    def test_missing_changed_value_without_current(self):
+        original = {"key1": "initial-value"}
+        changed = {"key2": "new-value"}
+        current = {"key2": "new-value", "key3": "key3-value"}
+        got = revert_changed_dict(current, original, changed)
+        self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got)
+
+    def test_missing_changed_value_with_current(self):
+        original = {"key1": "initial-value"}
+        changed = {"key2": "new-value"}
+        current = {"key1": "changed-value", "key2": "new-value", "key3": "key3-value"}
+        got = revert_changed_dict(current, original, changed)
+        self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got)

nautobot_design_builder/tests/test_model_change_record.py

@@ -1,10 +1,10 @@
 """Test ChangeRecord."""
 
-import unittest
 from unittest.mock import patch, Mock
 from nautobot.extras.models import Secret
 from nautobot.dcim.models import Manufacturer, DeviceType
 
+from nautobot_design_builder.design import Environment, ModelInstance
 from nautobot_design_builder.errors import DesignValidationError
 
 from .test_model_deployment import BaseDeploymentTest
@@ -155,34 +155,51 @@ def test_remove_dictionary_key(self):
             old_value,
         )
 
-    @unittest.skip
-    def test_new_key_reverted_without_original_and_with_a_new_one(self):
-        # TODO: I don't understand this test
-        secret = Secret.objects.get(id=self.secret.id)
-        secret.parameters["key2"] = "changed-value"
+    def test_only_protected_field_attributes_are_reverted(self):
+        """This test confirms that non-design dictionary keys are unaffected.
+
+        Any Nautobot JSON field stored as an object (dictionary) is protected
+        only for those object attributes (dictionary keys) that are part
+        of the design. This means that keys added outside of the design process
+        should remain when the design is reverted. This test confirms that
+        behavior.
+        """
+
+        original_params = {"key1": "initial-value"}
+        secret = Secret.objects.create(
+            name="test secret 1",
+            provider="environment-variable",
+            description="test description",
+            parameters=original_params,
+        )
         secret.save()
+        updated_params = {"key1": "initial-value", "key2": "changed-value"}
+        design_secret = ModelInstance.factory(Secret)(
+            Environment(),
+            {
+                "!create_or_update:name": "test secret 1",
+                "parameters": updated_params,
+            },
+        )
+        design_secret.save()
+        change_record = self.create_change_record(secret, design_secret.design_metadata.changes)
+        print(design_secret.design_metadata.changes)
         secret.refresh_from_db()
         self.assertDictEqual(
             secret.parameters,
-            {"key1": "initial-value", "key2": "changed-value"},
+            updated_params,
         )
 
-        # Delete the initial value and add a new one
-        del secret.parameters["key1"]
-        secret.parameters["key3"] = "changed-value"
+        # The "final params" is the design parameters "key2" plus a "key3" that
+        # has been added outside of the design process.
+        final_params = {"key2": "changed-value", "key3": "changed-value"}
+        secret.parameters = final_params
         secret.save()
-        self.assertDictEqual(
-            secret.parameters,
-            {
-                "key2": "changed-value",
-                "key3": "changed-value",
-            },
-        )
-
-        entry = self.create_change_record(secret, None)
-        entry.revert()
+        change_record.revert()
         secret.refresh_from_db()
-        self.assertDictEqual(self.secret.parameters, secret.parameters)
+        # reverting should bring back key1 and leave key3 alone (since it was added outside of the
+        # design process)
+        self.assertDictEqual(secret.parameters, {"key1": "initial-value", "key3": "changed-value"})
 
     @patch("nautobot.extras.models.Secret.save")
     def test_reverting_without_old_value(self, save_mock: Mock):
@@ -200,35 +217,6 @@ def test_reverting_without_old_value(self, save_mock: Mock):
             self.assertEqual(entry.design_object.parameters, {})
             save_mock.assert_called()
 
-    @unittest.skip
-    @patch("nautobot.extras.models.Secret.save")
-    def test_reverting_without_new_value(self, save_mock: Mock):
-        # TODO: I don't understand this test
-        with patch("nautobot.extras.models.Secret.refresh_from_db"):
-            secret = Secret(
-                name="test secret 1",
-                provider="environment-variable",
-                description="Description",
-                parameters={"key1": "value1"},
-            )
-            secret.parameters = None
-            entry = self.create_change_record(secret, secret)
-            self.assertEqual(entry.design_object.parameters, None)
-            entry.revert()
-            self.assertEqual(entry.design_object.parameters, {"key1": "value1"})
-            save_mock.assert_called()
-
-    @unittest.skip
-    def test_change_property(self):
-        """This test checks that the 'display' property is properly managed."""
-        updated_device_type = DeviceType.objects.get(id=self.device_type.id)
-        updated_device_type.model = "new name"
-        updated_device_type.save()
-        entry = self.create_change_record(updated_device_type, None)
-        entry.revert()
-        self.device_type.refresh_from_db()
-        self.assertEqual(self.device_type.model, "test device type")
-
     def test_change_foreign_key(self):
         new_manufacturer = Manufacturer.objects.create(name="new manufacturer")
         new_manufacturer.save()