Better logging / test cases

src/registrar/tests/common.py

@@ -519,6 +519,7 @@ def multiple_unalphabetical_domain_objects(
         applications.append(application)
     return applications
 
+
 def generic_domain_object(domain_type, object_name):
     """Returns a generic domain object of
     domain_type 'application', 'information', or 'invitation'"""

src/registrar/tests/test_admin.py

@@ -14,7 +14,7 @@ from registrar.models import (
     DomainInformation,
     User,
     DomainInvitation,
-    Domain
+    Domain,
 )
 from .common import (
     completed_application,
@@ -643,12 +643,13 @@ class AuditedAdminTest(TestCase):
 
 class DomainSessionVariableTest(TestCase):
     """Test cases for session variables in Django Admin"""
+
     def setUp(self):
         self.factory = RequestFactory()
         self.admin = DomainAdmin(Domain, None)
         self.client = Client(HTTP_HOST="localhost:8080")
 
-    def test_session_variables_set_correctly(self):
+    def test_session_vars_set_correctly(self):
         """Checks if session variables are being set correctly"""
 
         p = "adminpass"
@@ -657,26 +658,85 @@ class DomainSessionVariableTest(TestCase):
         dummy_domain_information = generic_domain_object("information", "session")
         request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk)
         self.populate_session_values(request, dummy_domain_information.domain)
+        self.assertEqual(request.session["analyst_action"], "edit")
+        self.assertEqual(
+            request.session["analyst_action_location"],
+            dummy_domain_information.domain.pk,
+        )
 
-        self.assertEqual(request.session['analyst_action'], 'edit')
+    def test_session_vars_set_correctly_hardcoded_domain(self):
-        self.assertEqual(request.session['analyst_action_location'], dummy_domain_information.domain.pk)
+        """Checks if session variables are being set correctly"""
-
-    def test_session_variables_retain_information(self):
-        """ Checks to see if session variables retain old information """
 
         p = "adminpass"
         self.client.login(username="superuser", password=p)
 
-        dummy_domain_information_list = multiple_unalphabetical_domain_objects("information")
+        dummy_domain_information: Domain = generic_domain_object(
+            "information", "session"
+        )
+        dummy_domain_information.domain.pk = 1
+        request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk)
+        self.populate_session_values(request, dummy_domain_information.domain)
+        self.assertEqual(request.session["analyst_action"], "edit")
+        self.assertEqual(request.session["analyst_action_location"], 1)
+
+    def test_session_variables_reset_correctly(self):
+        """Checks if incorrect session variables get overridden"""
+
+        p = "adminpass"
+        self.client.login(username="superuser", password=p)
+
+        dummy_domain_information = generic_domain_object("information", "session")
+        request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk)
+
+        self.populate_session_values(
+            request, dummy_domain_information.domain, preloadBadData=True
+        )
+
+        self.assertEqual(request.session["analyst_action"], "edit")
+        self.assertEqual(
+            request.session["analyst_action_location"],
+            dummy_domain_information.domain.pk,
+        )
+
+    def test_unauthorized_user(self):
+        """Checks for when a user has invalid perms"""
+
+        p = "userpass"
+        self.client.login(username="staffuser", password=p)
+
+        dummy_domain_information = generic_domain_object("information", "session")
+        request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk)
+        self.populate_session_values(request, dummy_domain_information.domain)
+
+        self.assertEqual(request.session["analyst_action"], "edit")
+        self.assertEqual(
+            request.session["analyst_action_location"],
+            dummy_domain_information.domain.pk,
+        )
+
+    def test_session_variables_retain_information(self):
+        """Checks to see if session variables retain old information"""
+
+        p = "adminpass"
+        self.client.login(username="superuser", password=p)
+
+        dummy_domain_information_list = multiple_unalphabetical_domain_objects(
+            "information"
+        )
         for item in dummy_domain_information_list:
             request = self.get_factory_post_edit_domain(item.domain.pk)
+            logger.debug(
+                f"Before populate - Domain Pk: {item.domain.pk} obj pk: {item.pk}"
+            )
             self.populate_session_values(request, item)
-
+            logger.debug(
-            self.assertEqual(request.session['analyst_action'], 'edit')
+                f"After populate - Domain Pk: {item.domain.pk} obj pk: {item.pk}"
-            self.assertEqual(request.session['analyst_action_location'], item.domain.pk)
+            )
+            self.assertEqual(request.session["analyst_action"], "edit")
+            self.assertEqual(request.session["analyst_action_location"], item.domain.pk)
 
     def test_session_variables_concurrent_requests(self):
-        """ Simulates two requests at once """
+        """Simulates two requests at once"""
 
         p = "adminpass"
         self.client.login(username="superuser", password=p)
@@ -691,23 +751,29 @@ class DomainSessionVariableTest(TestCase):
         self.populate_session_values(request_second, info_second.domain)
 
         # Check if anything got nulled out
-        self.assertNotEqual(request_first.session['analyst_action'], None)
+        self.assertNotEqual(request_first.session["analyst_action"], None)
-        self.assertNotEqual(request_second.session['analyst_action'], None)
+        self.assertNotEqual(request_second.session["analyst_action"], None)
-        self.assertNotEqual(request_first.session['analyst_action_location'], None)
+        self.assertNotEqual(request_first.session["analyst_action_location"], None)
-        self.assertNotEqual(request_second.session['analyst_action_location'], None)
+        self.assertNotEqual(request_second.session["analyst_action_location"], None)
 
         # Check if they are both the same action 'type'
-        self.assertEqual(request_first.session['analyst_action'], 'edit')
+        self.assertEqual(request_first.session["analyst_action"], "edit")
-        self.assertEqual(request_second.session['analyst_action'], 'edit')
+        self.assertEqual(request_second.session["analyst_action"], "edit")
 
         # Check their locations, and ensure they aren't the same across both
-        self.assertNotEqual(request_first.session['analyst_action_location'], request_second.session['analyst_action_location'])
+        self.assertNotEqual(
+            request_first.session["analyst_action_location"],
+            request_second.session["analyst_action_location"],
+        )
 
-    def populate_session_values(self, request, domain_object):
+    def populate_session_values(self, request, domain_object, preloadBadData=False):
         """Boilerplate for creating mock sessions"""
         request.user = self.client
         request.session = SessionStore()
         request.session.create()
+        if preloadBadData:
+            request.session["analyst_action"] = "invalid"
+            request.session["analyst_action_location"] = "bad location"
         self.admin.response_change(request, domain_object)
 
     def get_factory_post_edit_domain(self, primary_key):
@@ -715,7 +781,7 @@ class DomainSessionVariableTest(TestCase):
         with the edit domain button 'clicked',
         then returns the factory object"""
         return self.factory.post(
-            reverse('admin:registrar_domain_change', args=(primary_key,)),
+            reverse("admin:registrar_domain_change", args=(primary_key,)),
-            {'_edit_domain': 'true'},
+            {"_edit_domain": "true"},
-            follow=True
+            follow=True,
         )

src/registrar/views/domain.py

@@ -83,9 +83,10 @@ class DomainOrgNameAddressView(DomainPermissionView, FormMixin):
         # Q: Is there a more efficent way to do this?
         # It would be ideal if we didn't have to repeat this.
         if self.request.user.is_staff or self.request.user.is_superuser:
+            changes = {field: form.cleaned_data[field] for field in form.changed_data}
             # if they are editing from an '/admin' redirect, log their actions
             self.log_analyst_form_actions(
-                self.form_class.__name__, self.get_object().domain_info
+                self.form_class.__name__, self.get_object().domain_info, changes
             )
 
         # superclass has the redirect

src/registrar/views/utility/mixins.py

@@ -49,7 +49,7 @@ class DomainPermission(PermissionsLoginMixin):
 
         # ticket 806
         requested_domain = None
-        if(DomainInformation.objects.filter(id=pk).exists()):
+        if DomainInformation.objects.filter(id=pk).exists():
             requested_domain = DomainInformation.objects.get(id=pk)
 
         # Analysts may manage domains, when they are in these statuses:
@@ -77,17 +77,14 @@ class DomainPermission(PermissionsLoginMixin):
         # a status or DomainInformation... aka a status of 'None'
         # This checks that it has a status, before checking if it does
         # Otherwise, analysts can edit these domains
-        if (requested_domain is not None):
+        if requested_domain is not None:
             can_do_action = (
                 can_do_action
                 and requested_domain.domain_application.status in valid_domain_statuses
             )
         # If the valid session keys exist, if the user is permissioned,
         # and if its in a valid status
-        if (
+        if can_do_action and user_is_analyst_or_superuser:
-            can_do_action
-            and user_is_analyst_or_superuser
-        ):
             return True
 
         # ticket 796

src/registrar/views/utility/permission_views.py

@@ -46,10 +46,17 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
 
         return context
 
-    def log_analyst_form_actions(self, form_class_name, printable_object_info):
+    def log_analyst_form_actions(
+        self, form_class_name, printable_object_info, changes=None
+    ):
         """Generates a log for when key 'analyst_action' exists on the session.
-        Follows this format: f"{user_type} {self.request.user}
+        Follows this format:
-        edited {form_class_name} in {printable_object_info}"
+
+        for field, new_value in changes.items():
+                "{user_type} '{self.request.user}'
+                set field '{field}': '{new_value}'
+                under class '{form_class_name}'
+                in domain '{printable_object_info}'"
         """
         if "analyst_action" in self.request.session:
             action = self.request.session["analyst_action"]
@@ -67,7 +74,20 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
                     # Q: do we want to be logging on every changed field?
                     # I could see that becoming spammy log-wise,
                     # but it may also be important.
-
+                    if changes is not None:
+                        # Logs every change made to the domain field
+                        # noqa for readability/format
+                        for field, new_value in changes.items():
+                            logger.info(
+                                f"""
+                                An analyst or superuser made alterations to a domain: 
+                                {user_type} '{self.request.user}' 
+                                set field '{field}': '{new_value}' 
+                                under class '{form_class_name}' 
+                                in domain '{printable_object_info}'
+                                """  # noqa
+                            )
+                    else:
                         # noqa here as breaking this up further leaves it hard to read
                         logger.info(
                             f"{user_type} {self.request.user} edited {form_class_name} in {printable_object_info}"  # noqa