From ed9625e373d490cb66dbbad6aeaf9fad629d8386 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 25 Apr 2024 16:33:36 -0400 Subject: [PATCH 01/13] Implement audit trail for status changes on domain request change form --- src/registrar/models/domain_request.py | 9 +++++ .../admin/includes/detail_table_fieldset.html | 32 ++++++++++++++- src/registrar/tests/test_admin.py | 40 +++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 9ed35f489..234e0746d 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -16,12 +16,21 @@ from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError from itertools import chain +from auditlog.models import AuditlogHistoryField # type: ignore + logger = logging.getLogger(__name__) class DomainRequest(TimeStampedModel): """A registrant's domain request for a new domain.""" + # https://django-auditlog.readthedocs.io/en/latest/usage.html#object-history + # If we note any performace degradation due to this addition, + # we can query the auditlogs table in admin.py and add the results to + # extra_context in the change_view method for DomainRequestAdmin + # This is the more straightforward way so trying it first. + history = AuditlogHistoryField() + # Constants for choice fields class DomainRequestStatus(models.TextChoices): STARTED = "started", "Started" diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 26baddff7..645f29bea 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -73,7 +73,37 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% block after_help_text %} - {% if field.field.name == "creator" %} + {% if field.field.name == "status" and original_object.history.count > 0 %} +
+ +
+ + + + + + + + + + + {% for log_entry in original_object.history.all %} + {% for key, value in log_entry.changes_display_dict.items %} + {% if key == "status" %} + + + + + + + {% endif %} + {% endfor %} + {% endfor %} + +
FromToChanged ByChange Date
{{ value.0|default:"None" }}{{ value.1|default:"None" }}{{ log_entry.actor|default:"None" }}{{ log_entry.timestamp|default:"None" }}
+
+
+ {% elif field.field.name == "creator" %}
{% include "django/admin/includes/contact_detail_list.html" with user=original_object.creator no_title_top_padding=field.is_readonly %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 42baae6ef..9b6ce6c09 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -887,6 +887,46 @@ class TestDomainRequestAdmin(MockEppLib): ] self.test_helper.assert_response_contains_distinct_values(response, expected_values) + @less_console_noise_decorator + def test_status_logs(self): + """ + Tests that the status changes are shown in a table on the domain request change form + """ + + # Create a fake domain request and domain + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.STARTED) + + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + + # Table will contain From None To Started + self.assertContains(response, '') + self.assertContains(response, "None") + self.assertContains(response, "Started", count=1) + self.assertNotContains(response, "Submitted") + + domain_request.submit() + domain_request.save() + + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Table will contain From None To Started + # Table will contain From Started To Submitted + self.assertContains(response, "None") + self.assertContains(response, "Started", count=2) + self.assertContains(response, "Submitted") + @less_console_noise_decorator def test_analyst_can_see_and_edit_alternative_domain(self): """Tests if an analyst can still see and edit the alternative domain field""" From 87e8310c6da76935b962b5e46d5518d72d00814c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 25 Apr 2024 17:25:06 -0400 Subject: [PATCH 02/13] Set auditlog migration vars to False --- src/registrar/config/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 9817476bb..ff9302a40 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -785,6 +785,6 @@ if DEBUG: # Run: # cf run-task getgov-<> --wait --command 'python manage.py auditlogmigratejson --traceback' --name auditlogmigratejson # on our staging and stable, then remove these 2 variables or set to False -AUDITLOG_TWO_STEP_MIGRATION = True +AUDITLOG_TWO_STEP_MIGRATION = False -AUDITLOG_USE_TEXT_CHANGES_IF_JSON_IS_NOT_PRESENT = True +AUDITLOG_USE_TEXT_CHANGES_IF_JSON_IS_NOT_PRESENT = False From 8a8a6491f99bdc2736167de8042ffe9138f69368 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 25 Apr 2024 18:44:00 -0400 Subject: [PATCH 03/13] Teak table design and sorting --- src/registrar/assets/sass/_theme/_admin.scss | 19 ++++++++++++++++++- .../admin/includes/detail_table_fieldset.html | 11 +++++------ src/registrar/templatetags/custom_filters.py | 6 ++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index f5717d067..e0db9ac57 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -112,12 +112,20 @@ html[data-theme="light"] { .change-list .usa-table--borderless thead th, .change-list .usa-table thead td, .change-list .usa-table thead th, + .change-form .usa-table, + .change-form .usa-table--striped tbody tr:nth-child(odd) td, + .change-form .usa-table--borderless thead th, + .change-form .usa-table thead td, + .change-form .usa-table thead th, body.dashboard, body.change-list, body.change-form, .analytics { color: var(--body-fg); } + .usa-table td { + background-color: transparent; + } } // Firefox needs this to be specifically set @@ -127,11 +135,20 @@ html[data-theme="dark"] { .change-list .usa-table--borderless thead th, .change-list .usa-table thead td, .change-list .usa-table thead th, + .change-form .usa-table, + .change-form .usa-table--striped tbody tr:nth-child(odd) td, + .change-form .usa-table--borderless thead th, + .change-form .usa-table thead td, + .change-form .usa-table thead th, body.dashboard, body.change-list, - body.change-form { + body.change-form, + .analytics { color: var(--body-fg); } + .usa-table td { + background-color: transparent; + } } #branding h1 a:link, #branding h1 a:visited { diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 645f29bea..92eb433a9 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -1,5 +1,6 @@ {% extends "admin/fieldset.html" %} {% load static url_helpers %} +{% load custom_filters %} {% comment %} This is using a custom implementation fieldset.html (see admin/fieldset.html) @@ -80,18 +81,16 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) - - - - + + + - {% for log_entry in original_object.history.all %} + {% for log_entry in original_object.history.all|reverse_list %} {% for key, value in log_entry.changes_display_dict.items %} {% if key == "status" %} - diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index 9fa5c9aa9..bbed7b57c 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -67,3 +67,9 @@ def get_organization_long_name(generic_org_type): @register.filter(name="has_permission") def has_permission(user, permission): return user.has_perm(permission) + + +@register.filter(name='reverse_list') +def reverse_list(value): + return reversed(value) + From 909a9689f8db61b83c9884207ce335c96af2837f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 25 Apr 2024 18:47:06 -0400 Subject: [PATCH 04/13] lint --- src/registrar/templatetags/custom_filters.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index bbed7b57c..a9e546891 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -69,7 +69,6 @@ def has_permission(user, permission): return user.has_perm(permission) -@register.filter(name='reverse_list') +@register.filter(name="reverse_list") def reverse_list(value): return reversed(value) - From 0cdfa907ada05cedb687d9b4d211bee2d56879c3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 25 Apr 2024 19:08:03 -0400 Subject: [PATCH 05/13] Revise unit test to account for ordering --- src/registrar/tests/test_admin.py | 69 +++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 6de4bd181..89760ce24 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -890,7 +890,8 @@ class TestDomainRequestAdmin(MockEppLib): @less_console_noise_decorator def test_status_logs(self): """ - Tests that the status changes are shown in a table on the domain request change form + Tests that the status changes are shown in a table on the domain request change form, + accurately and in chronological order. """ # Create a fake domain request and domain @@ -907,9 +908,7 @@ class TestDomainRequestAdmin(MockEppLib): self.assertEqual(response.status_code, 200) self.assertContains(response, domain_request.requested_domain.name) - # Table will contain From None To Started - self.assertContains(response, '") + # Table will contain one row for Started self.assertContains(response, "", count=1) self.assertNotContains(response, "") @@ -921,11 +920,63 @@ class TestDomainRequestAdmin(MockEppLib): follow=True, ) - # Table will contain From None To Started - # Table will contain From Started To Submitted - self.assertContains(response, "") - self.assertContains(response, "", count=2) - self.assertContains(response, "") + # Table will contain and extra row for Submitted + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + + domain_request.in_review() + domain_request.save() + + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Table will contain and extra row for In review + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + + domain_request.action_needed() + domain_request.save() + + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Table will contain and extra row for Action needed + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + + domain_request.in_review() + domain_request.save() + + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Define the expected sequence of status changes + expected_status_changes = [ + "", + "", + "", + "", + "", + ] + + # Test for the order of status changes + for status_change in expected_status_changes: + self.assertContains(response, status_change, html=True) + + # Table now contains 2 rows for Approved + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=1) + self.assertContains(response, "", count=2) + self.assertContains(response, "", count=1) @less_console_noise_decorator def test_analyst_can_see_and_edit_alternative_domain(self): From 658ab94622fd03771f69d73890ca61fa6d0eb46b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 21:11:21 -0400 Subject: [PATCH 06/13] Set auditlog migration vars to True, will set to False in a separate PR --- src/registrar/config/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index ff9302a40..9817476bb 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -785,6 +785,6 @@ if DEBUG: # Run: # cf run-task getgov-<> --wait --command 'python manage.py auditlogmigratejson --traceback' --name auditlogmigratejson # on our staging and stable, then remove these 2 variables or set to False -AUDITLOG_TWO_STEP_MIGRATION = False +AUDITLOG_TWO_STEP_MIGRATION = True -AUDITLOG_USE_TEXT_CHANGES_IF_JSON_IS_NOT_PRESENT = False +AUDITLOG_USE_TEXT_CHANGES_IF_JSON_IS_NOT_PRESENT = True From cf3f87b1fd35ecd28d5abb2f5426b6cf4831116c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 21:42:23 -0400 Subject: [PATCH 07/13] Cleanup --- .../templates/django/admin/includes/detail_table_fieldset.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 88e4a9122..3a861837b 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -96,7 +96,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
FromToChanged ByChange DateStatusUserChanged at
{{ value.0|default:"None" }} {{ value.1|default:"None" }} {{ log_entry.actor|default:"None" }} {{ log_entry.timestamp|default:"None" }}') - self.assertContains(response, "NoneStartedSubmittedNoneStartedSubmittedStartedSubmittedStartedSubmittedIn reviewStartedSubmittedIn reviewAction neededStartedSubmittedIn reviewAction neededIn reviewStartedSubmittedIn reviewAction needed
- {% if field.field.name == "creator" %} + {% elif field.field.name == "creator" %}
{% include "django/admin/includes/contact_detail_list.html" with user=original_object.creator no_title_top_padding=field.is_readonly user_verification_type=original_object.creator.get_verification_type_display%} From 947e476a718c7c004e8d5b58612a23564c042586 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 21:48:14 -0400 Subject: [PATCH 08/13] lint --- src/registrar/tests/test_admin.py | 32 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 71ac289c9..fd3a5fe9c 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -977,26 +977,26 @@ class TestDomainRequestAdmin(MockEppLib): self.assertContains(response, "Submitted", count=1) self.assertContains(response, "In review", count=2) self.assertContains(response, "Action needed", count=1) - + def test_collaspe_toggle_button_markup(self): - """ - Tests for the correct collapse toggle button markup - """ + """ + Tests for the correct collapse toggle button markup + """ - # Create a fake domain request and domain - domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) + # Create a fake domain request and domain + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) - p = "adminpass" - self.client.login(username="superuser", password=p) - response = self.client.get( - "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), - follow=True, - ) + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) - # Make sure the page loaded, and that we're on the right page - self.assertEqual(response.status_code, 200) - self.assertContains(response, domain_request.requested_domain.name) - self.test_helper.assertContains(response, "Show details") + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + self.test_helper.assertContains(response, "Show details") @less_console_noise_decorator def test_analyst_can_see_and_edit_alternative_domain(self): From d9faf7478aa84caa4c0e73d817c5c63193517f7a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 22:47:50 -0400 Subject: [PATCH 09/13] Change ordering to descending --- .../django/admin/includes/detail_table_fieldset.html | 3 +-- src/registrar/templatetags/custom_filters.py | 5 ----- src/registrar/tests/test_admin.py | 4 ++-- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 3a861837b..84a56cbce 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -1,6 +1,5 @@ {% extends "admin/fieldset.html" %} {% load static url_helpers %} -{% load custom_filters %} {% comment %} This is using a custom implementation fieldset.html (see admin/fieldset.html) @@ -81,7 +80,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) - {% for log_entry in original_object.history.all|reverse_list %} + {% for log_entry in original_object.history.all %} {% for key, value in log_entry.changes_display_dict.items %} {% if key == "status" %} diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index a9e546891..9fa5c9aa9 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -67,8 +67,3 @@ def get_organization_long_name(generic_org_type): @register.filter(name="has_permission") def has_permission(user, permission): return user.has_perm(permission) - - -@register.filter(name="reverse_list") -def reverse_list(value): - return reversed(value) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index fd3a5fe9c..cf00994be 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -961,11 +961,11 @@ class TestDomainRequestAdmin(MockEppLib): # Define the expected sequence of status changes expected_status_changes = [ - "Started", - "Submitted", "In review", "Action needed", "In review", + "Submitted", + "Started", ] # Test for the order of status changes From f702570d105315ddf257b9e220cc2364b80be2a3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 22:57:48 -0400 Subject: [PATCH 10/13] Tweak margin --- .../templates/django/admin/includes/detail_table_fieldset.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 84a56cbce..c7bb6325e 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -68,7 +68,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% block after_help_text %} {% if field.field.name == "status" and original_object.history.count > 0 %} -
+
From b71c34617e179890f1c4e92e64fc68028bded1a3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 1 May 2024 14:33:05 -0400 Subject: [PATCH 11/13] cleanup --- src/registrar/models/domain_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index b78b77dd8..75fbadc3e 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -27,7 +27,7 @@ class DomainRequest(TimeStampedModel): # https://django-auditlog.readthedocs.io/en/latest/usage.html#object-history # If we note any performace degradation due to this addition, # we can query the auditlogs table in admin.py and add the results to - # extra_context in the change_view method for DomainRequestAdmin + # extra_context in the change_view method for DomainRequestAdmin. # This is the more straightforward way so trying it first. history = AuditlogHistoryField() From 80efe8352716791bbb77426db103e5d84cd4d89b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 1 May 2024 18:55:35 -0400 Subject: [PATCH 12/13] trigger PR --- src/registrar/models/domain_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 75fbadc3e..a3cfe4792 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -28,7 +28,7 @@ class DomainRequest(TimeStampedModel): # If we note any performace degradation due to this addition, # we can query the auditlogs table in admin.py and add the results to # extra_context in the change_view method for DomainRequestAdmin. - # This is the more straightforward way so trying it first. + # This is the more straightforward way, so trying it first. history = AuditlogHistoryField() # Constants for choice fields From ed94173392056d104c771031e973f4ac6a2f20b0 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 2 May 2024 15:03:14 -0400 Subject: [PATCH 13/13] trigger PR --- src/registrar/models/domain_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index a3cfe4792..75fbadc3e 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -28,7 +28,7 @@ class DomainRequest(TimeStampedModel): # If we note any performace degradation due to this addition, # we can query the auditlogs table in admin.py and add the results to # extra_context in the change_view method for DomainRequestAdmin. - # This is the more straightforward way, so trying it first. + # This is the more straightforward way so trying it first. history = AuditlogHistoryField() # Constants for choice fields