From e14ebdc2f5a43593f4fa71be1e35508dc1fbdafa Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 14 Dec 2023 14:49:21 -0700 Subject: [PATCH 01/31] Test sorting --- src/registrar/admin.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index def7c64b1..b06f50cbb 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -549,7 +549,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Columns list_display = [ - "requested_domain", + "get_requested_domain", "status", "organization_type", "created_at", @@ -557,6 +557,14 @@ class DomainApplicationAdmin(ListHeaderAdmin): "investigator", ] + def get_requested_domain(self, obj): + return obj.requested_domain + get_requested_domain.admin_order_field = 'requested_domain__name' # Allows column order sorting + get_requested_domain.short_description = 'Requested Domain' # Sets column's header + + + ordering = ['requested_domain__name'] + # Filters list_filter = ("status", "organization_type", "investigator") From d75742e2420a2cd74c12fa440a856a80299c8bf7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:18:51 -0700 Subject: [PATCH 02/31] Add mixin --- src/registrar/admin.py | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index b06f50cbb..81af24c84 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -543,7 +543,27 @@ class DomainApplicationAdminForm(forms.ModelForm): self.fields["status"].widget.choices = available_transitions -class DomainApplicationAdmin(ListHeaderAdmin): +class OrderableFieldsMixin: + orderable_fields = [] + + def __new__(cls, *args, **kwargs): + new_class = super().__new__(cls) + for field, sort_field in cls.orderable_fields: + setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) + return new_class + + @classmethod + def _create_orderable_field_method(cls, field, sort_field): + def method(obj): + attr = getattr(obj, field) + return attr + method.__name__ = f'get_{field}' + method.admin_order_field = f'{field}__{sort_field}' + method.short_description = field.replace('_', ' ').title() + return method + + +class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): """Custom domain applications admin class.""" @@ -553,17 +573,16 @@ class DomainApplicationAdmin(ListHeaderAdmin): "status", "organization_type", "created_at", - "submitter", - "investigator", + "get_submitter", + "get_investigator", ] - def get_requested_domain(self, obj): - return obj.requested_domain - get_requested_domain.admin_order_field = 'requested_domain__name' # Allows column order sorting - get_requested_domain.short_description = 'Requested Domain' # Sets column's header - - - ordering = ['requested_domain__name'] + orderable_fields = [ + ('requested_domain', 'name'), + # TODO figure out sorting twice at once + ("submitter", "first_name"), + ("investigator", "first_name"), + ] # Filters list_filter = ("status", "organization_type", "investigator") From 3b173e0ad439c28337a9b6507f6e37838b343805 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 15 Dec 2023 08:08:34 -0700 Subject: [PATCH 03/31] Update admin.py --- src/registrar/admin.py | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 81af24c84..f1a47e223 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -20,6 +20,25 @@ from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) +class OrderableFieldsMixin: + orderable_fk_fields = [] + + def __new__(cls, *args, **kwargs): + new_class = super().__new__(cls) + for field, sort_field in cls.orderable_fk_fields: + setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) + return new_class + + @classmethod + def _create_orderable_field_method(cls, field, sort_field): + def method(obj): + attr = getattr(obj, field) + return attr + method.__name__ = f'get_{field}' + method.admin_order_field = f'{field}__{sort_field}' + method.short_description = field.replace('_', ' ').title() + return method + class CustomLogEntryAdmin(LogEntryAdmin): """Overwrite the generated LogEntry admin class""" @@ -543,26 +562,6 @@ class DomainApplicationAdminForm(forms.ModelForm): self.fields["status"].widget.choices = available_transitions -class OrderableFieldsMixin: - orderable_fields = [] - - def __new__(cls, *args, **kwargs): - new_class = super().__new__(cls) - for field, sort_field in cls.orderable_fields: - setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) - return new_class - - @classmethod - def _create_orderable_field_method(cls, field, sort_field): - def method(obj): - attr = getattr(obj, field) - return attr - method.__name__ = f'get_{field}' - method.admin_order_field = f'{field}__{sort_field}' - method.short_description = field.replace('_', ' ').title() - return method - - class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): """Custom domain applications admin class.""" @@ -577,7 +576,7 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): "get_investigator", ] - orderable_fields = [ + orderable_fk_fields = [ ('requested_domain', 'name'), # TODO figure out sorting twice at once ("submitter", "first_name"), From 08072f5f0973b0871afe8f78742f3fb815c21f7f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 15 Dec 2023 13:35:08 -0700 Subject: [PATCH 04/31] Finish multi sort thing --- src/registrar/admin.py | 89 +++++++++++++++++++++------ src/registrar/views/utility/mixins.py | 19 ++++++ 2 files changed, 90 insertions(+), 18 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f1a47e223..63fb33095 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -13,6 +13,8 @@ from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models.domain import Domain from registrar.models.utility.admin_sort_fields import AdminSortFields from registrar.utility import csv_export +from registrar.views.utility.mixins import OrderableFieldsMixin +from django.contrib.admin.views.main import ORDER_VAR from . import models from auditlog.models import LogEntry # type: ignore from auditlog.admin import LogEntryAdmin # type: ignore @@ -20,24 +22,73 @@ from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) -class OrderableFieldsMixin: - orderable_fk_fields = [] +# Based off of this excellent example: https://djangosnippets.org/snippets/10471/ +class MultiFieldSortableChangeList(admin.views.main.ChangeList): + """ + This class overrides the behavior of column sorting in django admin tables in order + to allow multi field sorting - def __new__(cls, *args, **kwargs): - new_class = super().__new__(cls) - for field, sort_field in cls.orderable_fk_fields: - setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) - return new_class - @classmethod - def _create_orderable_field_method(cls, field, sort_field): - def method(obj): - attr = getattr(obj, field) - return attr - method.__name__ = f'get_{field}' - method.admin_order_field = f'{field}__{sort_field}' - method.short_description = field.replace('_', ' ').title() - return method + Usage: + + class MyCustomAdmin(admin.ModelAdmin): + + ... + + def get_changelist(self, request, **kwargs): + return MultiFieldSortableChangeList + + ... + + """ + def get_ordering(self, request, queryset): + """ + Returns the list of ordering fields for the change list. + First we check the get_ordering() method in model admin, then we check + the object's default ordering. Then, any manually-specified ordering + from the query string overrides anything. Finally, a deterministic + order is guaranteed by ensuring the primary key is used as the last + ordering field. + """ + params = self.params + ordering = list(self.model_admin.get_ordering(request) + or self._get_default_ordering()) + + if ORDER_VAR in params: + # Clear ordering and used params + ordering = [] + + order_params = params[ORDER_VAR].split('.') + for p in order_params: + try: + none, pfx, idx = p.rpartition('-') + field_name = self.list_display[int(idx)] + + order_fields = self.get_ordering_field(field_name) + + if isinstance(order_fields, list): + for order_field in order_fields: + if order_field: + ordering.append(pfx + order_field) + else: + ordering.append(pfx + order_fields) + + except (IndexError, ValueError) as err: + continue # Invalid ordering specified, skip it. + + # Add the given query's ordering fields, if any. + ordering.extend(queryset.query.order_by) + + # Ensure that the primary key is systematically present in the list of + # ordering fields so we can guarantee a deterministic order across all + # database backends. + pk_name = self.lookup_opts.pk.name + if not (set(ordering) & set(['pk', '-pk', pk_name, '-' + pk_name])): + # The two sets do not intersect, meaning the pk isn't present. So + # we add it. + ordering.append('-pk') + + return ordering class CustomLogEntryAdmin(LogEntryAdmin): @@ -578,8 +629,7 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): orderable_fk_fields = [ ('requested_domain', 'name'), - # TODO figure out sorting twice at once - ("submitter", "first_name"), + ("submitter", ["first_name"]), ("investigator", "first_name"), ] @@ -659,6 +709,9 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): ] filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") + + def get_changelist(self, request, **kwargs): + return MultiFieldSortableChangeList # lists in filter_horizontal are not sorted properly, sort them # by website diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 37c0f6e98..31df1de46 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -14,6 +14,25 @@ import logging logger = logging.getLogger(__name__) +class OrderableFieldsMixin: + orderable_fk_fields = [] + + def __new__(cls, *args, **kwargs): + new_class = super().__new__(cls) + for field, sort_field in cls.orderable_fk_fields: + setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) + return new_class + + @classmethod + def _create_orderable_field_method(cls, field, sort_field): + def method(obj): + attr = getattr(obj, field) + return attr + method.__name__ = f'get_{field}' + method.admin_order_field = f'{field}__{sort_field}' + method.short_description = field.replace('_', ' ').title() + return method + class PermissionsLoginMixin(PermissionRequiredMixin): From 451579768057d4083e237652f7899693b97a9c80 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 15 Dec 2023 14:11:44 -0700 Subject: [PATCH 05/31] Fix mixins --- src/registrar/admin.py | 34 +++++++++++++++++---------- src/registrar/views/utility/mixins.py | 19 +++++++++++---- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 63fb33095..cae9ae469 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -22,6 +22,7 @@ from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) + # Based off of this excellent example: https://djangosnippets.org/snippets/10471/ class MultiFieldSortableChangeList(admin.views.main.ChangeList): """ @@ -41,6 +42,7 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): ... """ + def get_ordering(self, request, queryset): """ Returns the list of ordering fields for the change list. @@ -51,17 +53,16 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): ordering field. """ params = self.params - ordering = list(self.model_admin.get_ordering(request) - or self._get_default_ordering()) - + ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) + if ORDER_VAR in params: # Clear ordering and used params ordering = [] - order_params = params[ORDER_VAR].split('.') + order_params = params[ORDER_VAR].split(".") for p in order_params: try: - none, pfx, idx = p.rpartition('-') + none, pfx, idx = p.rpartition("-") field_name = self.list_display[int(idx)] order_fields = self.get_ordering_field(field_name) @@ -83,10 +84,10 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): # ordering fields so we can guarantee a deterministic order across all # database backends. pk_name = self.lookup_opts.pk.name - if not (set(ordering) & set(['pk', '-pk', pk_name, '-' + pk_name])): + if not (set(ordering) & set(["pk", "-pk", pk_name, "-" + pk_name])): # The two sets do not intersect, meaning the pk isn't present. So # we add it. - ordering.append('-pk') + ordering.append("-pk") return ordering @@ -137,6 +138,16 @@ class AuditedAdmin(admin.ModelAdmin, AdminSortFields): class ListHeaderAdmin(AuditedAdmin): """Custom admin to add a descriptive subheader to list views.""" + def get_changelist(self, request, **kwargs): + """Returns a custom ChangeList class, as opposed to the default. + This is so we can override the behaviour of the `admin_order_field` field. + By default, django does not support ordering by multiple fields for this + particular field (i.e. self.admin_order_field=["first_name", "last_name"] is invalid). + + Reference: https://code.djangoproject.com/ticket/31975 + """ + return MultiFieldSortableChangeList + def changelist_view(self, request, extra_context=None): if extra_context is None: extra_context = {} @@ -628,9 +639,9 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): ] orderable_fk_fields = [ - ('requested_domain', 'name'), - ("submitter", ["first_name"]), - ("investigator", "first_name"), + ("requested_domain", "name"), + ("submitter", ["first_name", "last_name"]), + ("investigator", ["first_name", "last_name"]), ] # Filters @@ -709,9 +720,6 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): ] filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") - - def get_changelist(self, request, **kwargs): - return MultiFieldSortableChangeList # lists in filter_horizontal are not sorted properly, sort them # by website diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 31df1de46..13298f6a5 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -14,13 +14,14 @@ import logging logger = logging.getLogger(__name__) + class OrderableFieldsMixin: orderable_fk_fields = [] def __new__(cls, *args, **kwargs): new_class = super().__new__(cls) for field, sort_field in cls.orderable_fk_fields: - setattr(new_class, f'get_{field}', cls._create_orderable_field_method(field, sort_field)) + setattr(new_class, f"get_{field}", cls._create_orderable_field_method(field, sort_field)) return new_class @classmethod @@ -28,9 +29,19 @@ class OrderableFieldsMixin: def method(obj): attr = getattr(obj, field) return attr - method.__name__ = f'get_{field}' - method.admin_order_field = f'{field}__{sort_field}' - method.short_description = field.replace('_', ' ').title() + + method.__name__ = f"get_{field}" + + if isinstance(sort_field, list): + sort_list = [] + for sort_field_item in sort_field: + order_field_string = f"{field}__{sort_field_item}" + sort_list.append(order_field_string) + method.admin_order_field = sort_list + else: + method.admin_order_field = f"{field}__{sort_field}" + + method.short_description = field.replace("_", " ").title() return method From 43a595081f2d879abacebd0b84b91460268da0a6 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 15 Dec 2023 15:01:48 -0700 Subject: [PATCH 06/31] Fix other fields --- src/registrar/admin.py | 27 +++++++++---- src/registrar/views/utility/mixins.py | 56 ++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index cae9ae469..5fe6f3416 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -135,7 +135,7 @@ class AuditedAdmin(admin.ModelAdmin, AdminSortFields): return self.form_field_order_helper(form_field, db_field) -class ListHeaderAdmin(AuditedAdmin): +class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): """Custom admin to add a descriptive subheader to list views.""" def get_changelist(self, request, **kwargs): @@ -423,12 +423,18 @@ class UserDomainRoleAdmin(ListHeaderAdmin): _meta = Meta() + # TODO - maybe instead of get we just call it "sort"? # Columns list_display = [ - "user", - "domain", + "get_user", + "get_domain", "role", ] + + orderable_fk_fields = [ + ("domain", "name"), + ("user", ["first_name", "last_name"]), + ] # Search search_fields = [ @@ -490,13 +496,20 @@ class DomainInvitationAdmin(ListHeaderAdmin): class DomainInformationAdmin(ListHeaderAdmin): """Customize domain information admin class.""" + + # TODO - include the orderable fk fields inside list display # Columns list_display = [ - "domain", + "get_domain", "organization_type", "created_at", - "submitter", + "get_submitter", + ] + + orderable_fk_fields = [ + ("domain", "name"), + ("submitter", ["first_name", "last_name"]), ] # Filters @@ -624,7 +637,7 @@ class DomainApplicationAdminForm(forms.ModelForm): self.fields["status"].widget.choices = available_transitions -class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): +class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" @@ -720,7 +733,7 @@ class DomainApplicationAdmin(ListHeaderAdmin, OrderableFieldsMixin): ] filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") - + # lists in filter_horizontal are not sorted properly, sort them # by website def formfield_for_manytomany(self, db_field, request, **kwargs): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 13298f6a5..be7c59f95 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -16,9 +16,16 @@ logger = logging.getLogger(__name__) class OrderableFieldsMixin: + """ + Mixin to add multi-field ordering capabilities to a Django ModelAdmin on admin_order_field. + """ orderable_fk_fields = [] def __new__(cls, *args, **kwargs): + """ + This magic method is called when a new instance of the class (or subclass) is created. + It dynamically adds a new method to the class for each field in `orderable_fk_fields`. + """ new_class = super().__new__(cls) for field, sort_field in cls.orderable_fk_fields: setattr(new_class, f"get_{field}", cls._create_orderable_field_method(field, sort_field)) @@ -26,7 +33,54 @@ class OrderableFieldsMixin: @classmethod def _create_orderable_field_method(cls, field, sort_field): + """ + This class method is a factory for creating dynamic methods that will be attached to the ModelAdmin subclass. + It is used to customize how fk fields are ordered. By default, fks are ordered by id, so if you wish to + order by "name" instead, you need to manually override that. + + + In essence, this function will more or less generate code that looks like this, + for a given tuple defined in orderable_fk_fields: + + ``` + def get_requested_domain(self, obj): + return obj.requested_domain + # Allows column order sorting + get_requested_domain.admin_order_field = "requested_domain__name" + # Sets column's header + get_requested_domain.short_description = "requested domain" + ``` + + Or for fields with multiple order_fields: + + ``` + def get_submitter(self, obj): + return obj.submitter + # Allows column order sorting + get_requested_domain.admin_order_field = ["submitter__first_name", "submitter__last_name"] + # Sets column's header + get_requested_domain.short_description = "submitter" + ``` + + Parameters: + cls: The class that this method is being called on. In the context of this mixin, it would be the ModelAdmin subclass. + field: A string representing the name of the attribute that the dynamic method will fetch from the model instance. + sort_field: A string or list of strings representing the field(s) + that Django should sort by when the column is clicked in the admin interface. + + Returns: + method: The dynamically created method. + + The dynamically created method has the following attributes: + __name__: A string representing the name of the method. This is set to "get_{field}". + admin_order_field: A string or list of strings representing the field(s) that Django should sort by when the column is clicked in the admin interface. + short_description: A string used as the column header in the admin interface. Will replace underscores with spaces. + """ def method(obj): + """ + The dynamically created method. + When called on a model instance, it returns the value of the specified attribute. + """ attr = getattr(obj, field) return attr @@ -41,7 +95,7 @@ class OrderableFieldsMixin: else: method.admin_order_field = f"{field}__{sort_field}" - method.short_description = field.replace("_", " ").title() + method.short_description = field.replace("_", " ") return method From e5e5fc66e6897a0c764dfa880bac93456694bfe1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Dec 2023 10:59:12 -0700 Subject: [PATCH 07/31] Change list_display logic --- src/registrar/admin.py | 29 ++++++++---------- src/registrar/views/utility/mixins.py | 43 +++++++++++++++++++++------ 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5fe6f3416..4874998eb 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -27,7 +27,7 @@ logger = logging.getLogger(__name__) class MultiFieldSortableChangeList(admin.views.main.ChangeList): """ This class overrides the behavior of column sorting in django admin tables in order - to allow multi field sorting + to allow for multi field sorting on admin_order_field Usage: @@ -46,11 +46,9 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): def get_ordering(self, request, queryset): """ Returns the list of ordering fields for the change list. - First we check the get_ordering() method in model admin, then we check - the object's default ordering. Then, any manually-specified ordering - from the query string overrides anything. Finally, a deterministic - order is guaranteed by ensuring the primary key is used as the last - ordering field. + + Mostly identical to the base implementation, except that now it can return + a list of order_field objects rather than just one. """ params = self.params ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) @@ -74,7 +72,7 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): else: ordering.append(pfx + order_fields) - except (IndexError, ValueError) as err: + except (IndexError, ValueError): continue # Invalid ordering specified, skip it. # Add the given query's ordering fields, if any. @@ -423,11 +421,10 @@ class UserDomainRoleAdmin(ListHeaderAdmin): _meta = Meta() - # TODO - maybe instead of get we just call it "sort"? # Columns list_display = [ - "get_user", - "get_domain", + "user", + "domain", "role", ] @@ -496,15 +493,13 @@ class DomainInvitationAdmin(ListHeaderAdmin): class DomainInformationAdmin(ListHeaderAdmin): """Customize domain information admin class.""" - - # TODO - include the orderable fk fields inside list display # Columns list_display = [ - "get_domain", + "domain", "organization_type", "created_at", - "get_submitter", + "submitter", ] orderable_fk_fields = [ @@ -643,12 +638,12 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Columns list_display = [ - "get_requested_domain", + "requested_domain", "status", "organization_type", "created_at", - "get_submitter", - "get_investigator", + "submitter", + "investigator", ] orderable_fk_fields = [ diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index be7c59f95..3fb1ce4b1 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -27,17 +27,37 @@ class OrderableFieldsMixin: It dynamically adds a new method to the class for each field in `orderable_fk_fields`. """ new_class = super().__new__(cls) + + # Check if the list_display attribute exists, and if it does, create a local copy of that list. + list_display_exists = hasattr(cls, "list_display") + new_list_display = [] + if list_display_exists and isinstance(cls.list_display, list): + new_list_display = cls.list_display.copy() + for field, sort_field in cls.orderable_fk_fields: - setattr(new_class, f"get_{field}", cls._create_orderable_field_method(field, sort_field)) + updated_name = f"get_{field}" + + # For each item in orderable_fk_fields, create a function and associate it with admin_order_field. + setattr(new_class, updated_name, cls._create_orderable_field_method(field, sort_field)) + + # Update the list_display variable to use our newly created functions + if list_display_exists and field in cls.list_display: + index = new_list_display.index(field) + new_list_display[index] = updated_name + elif list_display_exists: + new_list_display.append(updated_name) + + # Replace the old list with the updated one + if list_display_exists: + cls.list_display = new_list_display + return new_class @classmethod def _create_orderable_field_method(cls, field, sort_field): """ This class method is a factory for creating dynamic methods that will be attached to the ModelAdmin subclass. - It is used to customize how fk fields are ordered. By default, fks are ordered by id, so if you wish to - order by "name" instead, you need to manually override that. - + It is used to customize how fk fields are ordered. In essence, this function will more or less generate code that looks like this, for a given tuple defined in orderable_fk_fields: @@ -47,7 +67,7 @@ class OrderableFieldsMixin: return obj.requested_domain # Allows column order sorting get_requested_domain.admin_order_field = "requested_domain__name" - # Sets column's header + # Sets column's header name get_requested_domain.short_description = "requested domain" ``` @@ -65,8 +85,7 @@ class OrderableFieldsMixin: Parameters: cls: The class that this method is being called on. In the context of this mixin, it would be the ModelAdmin subclass. field: A string representing the name of the attribute that the dynamic method will fetch from the model instance. - sort_field: A string or list of strings representing the field(s) - that Django should sort by when the column is clicked in the admin interface. + sort_field: A string or list of strings representing the field(s) to sort by (ex: "name" or "creator") Returns: method: The dynamically created method. @@ -78,23 +97,29 @@ class OrderableFieldsMixin: """ def method(obj): """ - The dynamically created method. - When called on a model instance, it returns the value of the specified attribute. + Method factory. """ attr = getattr(obj, field) return attr + # Set the function name. For instance, if the field is "domain", + # then this will generate a function called "get_domain" method.__name__ = f"get_{field}" + # Check if a list is passed in, or just a string. if isinstance(sort_field, list): sort_list = [] for sort_field_item in sort_field: order_field_string = f"{field}__{sort_field_item}" sort_list.append(order_field_string) + # If its a list, return an array of fields to sort on. + # For instance, ["creator__first_name", "creator__last_name"] method.admin_order_field = sort_list else: + # If its not a list, just return a string method.admin_order_field = f"{field}__{sort_field}" + # Infer the column name in a similar manner to how Django does method.short_description = field.replace("_", " ") return method From dc13fc6661cc93f75c252d116defabe72add154f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Dec 2023 11:19:20 -0700 Subject: [PATCH 08/31] Simplify logic --- src/registrar/views/utility/mixins.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 3fb1ce4b1..3fb7ab2f2 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -25,14 +25,18 @@ class OrderableFieldsMixin: """ This magic method is called when a new instance of the class (or subclass) is created. It dynamically adds a new method to the class for each field in `orderable_fk_fields`. + Then, it will update the `list_display` attribute such that it uses these generated methods. """ new_class = super().__new__(cls) + # If the class doesn't define anything for orderable_fk_fields, then we should + # just skip this additional logic + if not hasattr(cls, "orderable_fk_fields") or len(cls.orderable_fk_fields) == 0: + return new_class + # Check if the list_display attribute exists, and if it does, create a local copy of that list. - list_display_exists = hasattr(cls, "list_display") - new_list_display = [] - if list_display_exists and isinstance(cls.list_display, list): - new_list_display = cls.list_display.copy() + list_display_exists = hasattr(cls, "list_display") and isinstance(cls.list_display, list) + new_list_display = cls.list_display.copy() if list_display_exists else [] for field, sort_field in cls.orderable_fk_fields: updated_name = f"get_{field}" From 1d551215d9a64a0ec89b4121ed628e16a52d9f71 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 18 Dec 2023 14:54:24 -0700 Subject: [PATCH 09/31] Rough test cases --- src/registrar/admin.py | 3 +- src/registrar/tests/test_admin.py | 200 +++++++++++++++++++++++++- src/registrar/views/utility/mixins.py | 32 +++-- 3 files changed, 221 insertions(+), 14 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4874998eb..0aaac182b 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -134,7 +134,8 @@ class AuditedAdmin(admin.ModelAdmin, AdminSortFields): class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): - """Custom admin to add a descriptive subheader to list views.""" + """Custom admin to add a descriptive subheader to list views + and custom table sort behaviour""" def get_changelist(self, request, **kwargs): """Returns a custom ChangeList class, as opposed to the default. diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9d6add249..550a3c596 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -13,7 +13,6 @@ from registrar.admin import ( MyUserAdmin, AuditedAdmin, ContactAdmin, - UserDomainRoleAdmin, ) from registrar.models import ( Domain, @@ -21,9 +20,11 @@ from registrar.models import ( DomainInformation, User, DomainInvitation, + Contact, ) from registrar.models.user_domain_role import UserDomainRole from .common import ( + AuditedAdminMockData, completed_application, generic_domain_object, mock_user, @@ -323,6 +324,61 @@ class TestDomainApplicationAdmin(MockEppLib): self.superuser = create_superuser() self.staffuser = create_user() + def _assert_sort_helper(self, o_index, sort_field): + # 'o' (ordering) is based off the index position in the list_filter object, plus one. + # Q: Why is this not working?? + # domain_index = self.admin.list_filter.index("domain") + 1 + dummy_request = self.factory.get( + "/admin/registrar/DomainApplication/", + { + "o": o_index + }, + ) + dummy_request.user = self.superuser + + expected_sort_order = list(DomainApplication.objects.order_by(sort_field)) + returned_sort_order = list(self.admin.get_queryset(dummy_request)) + self.assertEqual(expected_sort_order, returned_sort_order) + + def test_domain_sortable(self): + """Tests if the UserDomainrole sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + # Assert that our sort works correctly + self._assert_sort_helper("1", "requested_domain") + + # Assert that sorting in reverse works correctly + self._assert_sort_helper("-1", "-requested_domain") + + def test_submitter_sortable(self): + """Tests if the UserDomainrole sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + # Assert that our sort works correctly + self._assert_sort_helper("1", "submitter") + + # Assert that sorting in reverse works correctly + #self._assert_sort_helper("-1", "-submitter") + + def test_investigator_sortable(self): + """Tests if the UserDomainrole sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + # Assert that our sort works correctly + self._assert_sort_helper("1", "investigator") + + # Assert that sorting in reverse works correctly + #self._assert_sort_helper("-1", "-investigator") + def test_short_org_name_in_applications_list(self): """ Make sure the short name is displaying in admin on the list page @@ -890,12 +946,94 @@ class DomainInvitationAdminTest(TestCase): self.assertContains(response, retrieved_html, count=1) +class DomainInformationAdminTest(TestCase): + def setUp(self): + """Setup environment for a mock admin user""" + self.site = AdminSite() + self.factory = RequestFactory() + self.admin = ListHeaderAdmin(model=DomainInformation, admin_site=self.site) + self.client = Client(HTTP_HOST="localhost:8080") + self.superuser = create_superuser() + self.mock_data_generator = AuditedAdminMockData() + + # Create fake DomainInformation objects + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Apple"), + submitter=self.mock_data_generator.dummy_contact("Zebra", "submitter") + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Zebra"), + submitter=self.mock_data_generator.dummy_contact("Apple", "submitter") + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Circus"), + submitter=self.mock_data_generator.dummy_contact("Xylophone", "submitter") + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Xylophone"), + submitter=self.mock_data_generator.dummy_contact("Circus", "submitter") + ) + + def tearDown(self): + """Delete all Users, Domains, and UserDomainRoles""" + DomainInformation.objects.all().delete() + DomainApplication.objects.all().delete() + Domain.objects.all().delete() + Contact.objects.all().delete() + User.objects.all().delete() + + def _assert_sort_helper(self, o_index, sort_field): + # 'o' (ordering) is based off the index position in the list_filter object, plus one. + # Q: Why is this not working?? + # domain_index = self.admin.list_filter.index("domain") + 1 + dummy_request = self.factory.get( + "/admin/registrar/DomainInformation/", + { + "o": o_index + }, + ) + dummy_request.user = self.superuser + + expected_sort_order = list(DomainInformation.objects.order_by(sort_field)) + returned_sort_order = list(self.admin.get_queryset(dummy_request)) + self.assertEqual(expected_sort_order, returned_sort_order) + + def test_domain_sortable(self): + """Tests if DomainInformation sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + # Assert that our sort works correctly + self._assert_sort_helper("1", "domain") + + # Assert that sorting in reverse works correctly + #self._assert_sort_helper("-1", "-domain") + + def test_submitter_sortable(self): + """Tests if DomainInformation sorts by submitter correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + # Assert that our sort works correctly + self._assert_sort_helper("1", "submitter") + + # Assert that sorting in reverse works correctly + #self._assert_sort_helper("-1", "-submitter") + + class UserDomainRoleAdminTest(TestCase): def setUp(self): """Setup environment for a mock admin user""" self.site = AdminSite() self.factory = RequestFactory() - self.admin = ListHeaderAdmin(model=UserDomainRoleAdmin, admin_site=None) + self.admin = ListHeaderAdmin(model=UserDomainRole, admin_site=self.site) self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() @@ -905,6 +1043,64 @@ class UserDomainRoleAdminTest(TestCase): Domain.objects.all().delete() UserDomainRole.objects.all().delete() + def _assert_sort_helper(self, o_index, sort_field): + # 'o' (ordering) is based off the index position in the list_filter object, plus one. + # Q: Why is this not working?? + # domain_index = self.admin.list_filter.index("domain") + 1 + dummy_request = self.factory.get( + "/admin/registrar/userdomainrole/", + { + "o": o_index + }, + ) + dummy_request.user = self.superuser + + expected_sort_order = list(UserDomainRole.objects.order_by(sort_field)) + returned_sort_order = list(self.admin.get_queryset(dummy_request)) + self.assertEqual(expected_sort_order, returned_sort_order) + + def test_domain_sortable(self): + """Tests if the UserDomainrole sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + fake_user = User.objects.create( + username="dummyuser", first_name="Stewart", last_name="Jones", email="AntarcticPolarBears@example.com" + ) + + # Create a list of UserDomainRoles that are in random order + mocks_to_create = ["jkl.gov", "ghi.gov", "abc.gov", "def.gov"] + for name in mocks_to_create: + fake_domain = Domain.objects.create(name=name) + UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") + + # Assert that our sort works correctly + self._assert_sort_helper("2", "domain") + + # Assert that sorting in reverse works correctly + self._assert_sort_helper("-2", "-domain") + + def test_user_sortable(self): + """Tests if the UserDomainrole sorts by user correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + mock_data_generator = AuditedAdminMockData() + + fake_domain = Domain.objects.create(name="igorville.gov") + # Create a list of UserDomainRoles that are in random order + mocks_to_create = ["jkl", "ghi", "abc", "def"] + for name in mocks_to_create: + # Creates a fake "User" object + fake_user = mock_data_generator.dummy_user(name, "user") + UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") + + # Assert that our sort works correctly + self._assert_sort_helper("1", "user") + + # Assert that sorting in reverse works correctly + self._assert_sort_helper("-1", "-user") + def test_email_not_in_search(self): """Tests the search bar in Django Admin for UserDomainRoleAdmin. Should return no results for an invalid email.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 3fb7ab2f2..5a5f366cc 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -19,6 +19,7 @@ class OrderableFieldsMixin: """ Mixin to add multi-field ordering capabilities to a Django ModelAdmin on admin_order_field. """ + custom_sort_name_prefix = "get_sortable_" orderable_fk_fields = [] def __new__(cls, *args, **kwargs): @@ -39,7 +40,7 @@ class OrderableFieldsMixin: new_list_display = cls.list_display.copy() if list_display_exists else [] for field, sort_field in cls.orderable_fk_fields: - updated_name = f"get_{field}" + updated_name = cls.custom_sort_name_prefix + field # For each item in orderable_fk_fields, create a function and associate it with admin_order_field. setattr(new_class, updated_name, cls._create_orderable_field_method(field, sort_field)) @@ -67,23 +68,23 @@ class OrderableFieldsMixin: for a given tuple defined in orderable_fk_fields: ``` - def get_requested_domain(self, obj): + def get_sortable_requested_domain(self, obj): return obj.requested_domain # Allows column order sorting - get_requested_domain.admin_order_field = "requested_domain__name" + get_sortable_requested_domain.admin_order_field = "requested_domain__name" # Sets column's header name - get_requested_domain.short_description = "requested domain" + get_sortable_requested_domain.short_description = "requested domain" ``` Or for fields with multiple order_fields: ``` - def get_submitter(self, obj): + def get_sortable_submitter(self, obj): return obj.submitter # Allows column order sorting - get_requested_domain.admin_order_field = ["submitter__first_name", "submitter__last_name"] + get_sortable_submitter.admin_order_field = ["submitter__first_name", "submitter__last_name"] # Sets column's header - get_requested_domain.short_description = "submitter" + get_sortable_submitter.short_description = "submitter" ``` Parameters: @@ -96,19 +97,28 @@ class OrderableFieldsMixin: The dynamically created method has the following attributes: __name__: A string representing the name of the method. This is set to "get_{field}". - admin_order_field: A string or list of strings representing the field(s) that Django should sort by when the column is clicked in the admin interface. + admin_order_field: A string or list of strings representing the field(s) that + Django should sort by when the column is clicked in the admin interface. short_description: A string used as the column header in the admin interface. Will replace underscores with spaces. """ def method(obj): """ - Method factory. + Template method for patterning. + + Returns (example): + ``` + def get_submitter(self, obj): + return obj.submitter + ``` """ attr = getattr(obj, field) return attr # Set the function name. For instance, if the field is "domain", - # then this will generate a function called "get_domain" - method.__name__ = f"get_{field}" + # then this will generate a function called "get_sort_domain". + # This is done rather than just setting the name to the attribute to avoid + # naming conflicts. + method.__name__ = cls.custom_sort_name_prefix + field # Check if a list is passed in, or just a string. if isinstance(sort_field, list): From c9c41cd479bcfbca602a3ccc2d68d15b79a49be2 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 19 Dec 2023 12:08:32 -0700 Subject: [PATCH 10/31] Fix faulty test cases (pt.1) Saving progress --- src/registrar/tests/test_admin.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 550a3c596..bedfcdca3 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3,7 +3,7 @@ from django.contrib.admin.sites import AdminSite from contextlib import ExitStack from django.contrib import messages from django.urls import reverse - +from django.contrib.sessions.middleware import SessionMiddleware from registrar.admin import ( DomainAdmin, DomainApplicationAdmin, @@ -336,22 +336,32 @@ class TestDomainApplicationAdmin(MockEppLib): ) dummy_request.user = self.superuser - expected_sort_order = list(DomainApplication.objects.order_by(sort_field)) - returned_sort_order = list(self.admin.get_queryset(dummy_request)) + # Mock a user request + middleware = SessionMiddleware(lambda req: req) + middleware.process_request(dummy_request) + dummy_request.session.save() + + expected_sort_order = list(DomainApplication.objects.order_by(*sort_field)) + + # Use changelist_view to get the sorted queryset + response = self.admin.changelist_view(dummy_request) + response.render() # Render the response before accessing its content + returned_sort_order = list(response.context_data["cl"].result_list) + self.assertEqual(expected_sort_order, returned_sort_order) def test_domain_sortable(self): - """Tests if the UserDomainrole sorts by domain correctly""" + """Tests if the DomainApplication sorts by domain correctly""" p = "adminpass" self.client.login(username="superuser", password=p) multiple_unalphabetical_domain_objects("application") # Assert that our sort works correctly - self._assert_sort_helper("1", "requested_domain") - + self._assert_sort_helper("1", ("requested_domain__name",)) + # Assert that sorting in reverse works correctly - self._assert_sort_helper("-1", "-requested_domain") + self._assert_sort_helper("-1", ("-requested_domain__name",)) def test_submitter_sortable(self): """Tests if the UserDomainrole sorts by domain correctly""" @@ -361,10 +371,10 @@ class TestDomainApplicationAdmin(MockEppLib): multiple_unalphabetical_domain_objects("application") # Assert that our sort works correctly - self._assert_sort_helper("1", "submitter") + self._assert_sort_helper("1", ("submitter__first_name", "submitter__last_name",)) # Assert that sorting in reverse works correctly - #self._assert_sort_helper("-1", "-submitter") + self._assert_sort_helper("-1", ("-submitter__first_name", "-submitter__last_name",)) def test_investigator_sortable(self): """Tests if the UserDomainrole sorts by domain correctly""" From d539740ae6a254aa46b525110acd20c8c33322d7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:16:48 -0700 Subject: [PATCH 11/31] Unit tests --- src/registrar/tests/common.py | 70 ++++++++++- src/registrar/tests/test_admin.py | 190 +++++++++++++++--------------- 2 files changed, 160 insertions(+), 100 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 5166e9c18..9546a5d6a 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -9,7 +9,7 @@ import uuid from django.test import TestCase from unittest.mock import MagicMock, Mock, patch from typing import List, Dict - +from django.contrib.sessions.middleware import SessionMiddleware from django.conf import settings from django.contrib.auth import get_user_model, login @@ -92,6 +92,71 @@ def less_console_noise(output_stream=None): # we opened output_stream so we have to close it output_stream.close() +class GenericTestHelper(TestCase): + """A helper class that contains various helper functions for TestCases""" + def __init__(self, admin, model=None, url=None, user=None, factory=None, **kwargs): + """ + Parameters: + admin (ModelAdmin): The Django ModelAdmin instance associated with the model. + model (django.db.models.Model, optional): The Django model associated with the admin page. + url (str, optional): The URL of the Django Admin page to test. + user (User, optional): The Django User who is making the request. + factory (RequestFactory, optional): An instance of Django's RequestFactory. + """ + super().__init__() + self.factory = factory + self.user = user + self.admin = admin + self.model = model + self.url = url + + def assert_table_sorted(self, o_index, sort_fields): + """ + This helper function validates the sorting functionality of a Django Admin table view. + + It creates a mock HTTP GET request to the provided URL with a specific ordering parameter, + and compares the returned sorted queryset with the expected sorted queryset. + + Parameters: + o_index (str): The index of the field in the table to sort by. This is passed as a string + to the 'o' parameter in the GET request. + sort_fields (tuple): The fields of the model to sort by. These fields are used to generate + the expected sorted queryset. + + + Example Usage: + ``` + self.assert_sort_helper( + self.factory, self.superuser, self.admin, self.url, DomainInformation, "1", ("domain__name",) + ) + ``` + + The function asserts that the returned sorted queryset from the admin page matches the + expected sorted queryset. If the assertion fails, it means the sorting functionality + on the admin page is not working as expected. + """ + # 'o' is a search param defined by the current index position in the + # table, plus one. + dummy_request = self.factory.get( + self.url, + {"o": o_index}, + ) + dummy_request.user = self.user + + # Mock a user request + middleware = SessionMiddleware(lambda req: req) + middleware.process_request(dummy_request) + dummy_request.session.save() + + expected_sort_order = list(self.model.objects.order_by(*sort_fields)) + + # Use changelist_view to get the sorted queryset + response = self.admin.changelist_view(dummy_request) + response.render() # Render the response before accessing its content + returned_sort_order = list(response.context_data["cl"].result_list) + + self.assertEqual(expected_sort_order, returned_sort_order) + class MockUserLogin: def __init__(self, get_response): @@ -273,6 +338,7 @@ class AuditedAdminMockData: creator: User = self.dummy_user(item_name, "creator"), } """ # noqa + creator = self.dummy_user(item_name, "creator") common_args = dict( organization_type=org_type, federal_type=federal_type, @@ -287,7 +353,7 @@ class AuditedAdminMockData: anything_else="There is more", authorizing_official=self.dummy_contact(item_name, "authorizing_official"), submitter=self.dummy_contact(item_name, "submitter"), - creator=self.dummy_user(item_name, "creator"), + creator=creator, ) return common_args diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index bedfcdca3..58f40bb43 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3,7 +3,6 @@ from django.contrib.admin.sites import AdminSite from contextlib import ExitStack from django.contrib import messages from django.urls import reverse -from django.contrib.sessions.middleware import SessionMiddleware from registrar.admin import ( DomainAdmin, DomainApplicationAdmin, @@ -13,6 +12,8 @@ from registrar.admin import ( MyUserAdmin, AuditedAdmin, ContactAdmin, + DomainInformationAdmin, + UserDomainRoleAdmin, ) from registrar.models import ( Domain, @@ -33,6 +34,7 @@ from .common import ( create_ready_domain, multiple_unalphabetical_domain_objects, MockEppLib, + GenericTestHelper, ) from django.contrib.sessions.backends.db import SessionStore from django.contrib.auth import get_user_model @@ -323,71 +325,73 @@ class TestDomainApplicationAdmin(MockEppLib): self.admin = DomainApplicationAdmin(model=DomainApplication, admin_site=self.site) self.superuser = create_superuser() self.staffuser = create_user() - - def _assert_sort_helper(self, o_index, sort_field): - # 'o' (ordering) is based off the index position in the list_filter object, plus one. - # Q: Why is this not working?? - # domain_index = self.admin.list_filter.index("domain") + 1 - dummy_request = self.factory.get( - "/admin/registrar/DomainApplication/", - { - "o": o_index - }, + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/DomainApplication/", + model=DomainApplication ) - dummy_request.user = self.superuser - - # Mock a user request - middleware = SessionMiddleware(lambda req: req) - middleware.process_request(dummy_request) - dummy_request.session.save() - - expected_sort_order = list(DomainApplication.objects.order_by(*sort_field)) - - # Use changelist_view to get the sorted queryset - response = self.admin.changelist_view(dummy_request) - response.render() # Render the response before accessing its content - returned_sort_order = list(response.context_data["cl"].result_list) - - self.assertEqual(expected_sort_order, returned_sort_order) def test_domain_sortable(self): """Tests if the DomainApplication sorts by domain correctly""" p = "adminpass" self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted("1", ("requested_domain__name",)) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-1", ("-requested_domain__name",)) + + def test_submitter_sortable(self): + """Tests if the DomainApplication sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) multiple_unalphabetical_domain_objects("application") - # Assert that our sort works correctly - self._assert_sort_helper("1", ("requested_domain__name",)) - - # Assert that sorting in reverse works correctly - self._assert_sort_helper("-1", ("-requested_domain__name",)) - - def test_submitter_sortable(self): - """Tests if the UserDomainrole sorts by domain correctly""" - p = "adminpass" - self.client.login(username="superuser", password=p) - - multiple_unalphabetical_domain_objects("application") + additional_application = generic_domain_object("application", "Xylophone") + new_user = User.objects.filter(username=additional_application.investigator.username).get() + new_user.first_name = "Xylophonic" + new_user.save() # Assert that our sort works correctly - self._assert_sort_helper("1", ("submitter__first_name", "submitter__last_name",)) - + self.test_helper.assert_table_sorted("5", ( + "submitter__first_name", + "submitter__last_name", + )) + # Assert that sorting in reverse works correctly - self._assert_sort_helper("-1", ("-submitter__first_name", "-submitter__last_name",)) - + self.test_helper.assert_table_sorted("-5", ( + "-submitter__first_name", + "-submitter__last_name", + )) + def test_investigator_sortable(self): - """Tests if the UserDomainrole sorts by domain correctly""" + """Tests if the DomainApplication sorts by domain correctly""" p = "adminpass" self.client.login(username="superuser", password=p) multiple_unalphabetical_domain_objects("application") + additional_application = generic_domain_object("application", "Xylophone") + new_user = User.objects.filter(username=additional_application.investigator.username).get() + new_user.first_name = "Xylophonic" + new_user.save() # Assert that our sort works correctly - self._assert_sort_helper("1", "investigator") - + self.test_helper.assert_table_sorted("6", ( + "investigator__first_name", + "investigator__last_name", + )) + # Assert that sorting in reverse works correctly - #self._assert_sort_helper("-1", "-investigator") + self.test_helper.assert_table_sorted("-6", ( + "-investigator__first_name", + "-investigator__last_name", + )) def test_short_org_name_in_applications_list(self): """ @@ -961,34 +965,42 @@ class DomainInformationAdminTest(TestCase): """Setup environment for a mock admin user""" self.site = AdminSite() self.factory = RequestFactory() - self.admin = ListHeaderAdmin(model=DomainInformation, admin_site=self.site) + self.admin = DomainInformationAdmin(model=DomainInformation, admin_site=self.site) self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() self.mock_data_generator = AuditedAdminMockData() + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/DomainInformation/", + model=DomainInformation + ) + # Create fake DomainInformation objects DomainInformation.objects.create( creator=self.mock_data_generator.dummy_user("fake", "creator"), domain=self.mock_data_generator.dummy_domain("Apple"), - submitter=self.mock_data_generator.dummy_contact("Zebra", "submitter") + submitter=self.mock_data_generator.dummy_contact("Zebra", "submitter"), ) DomainInformation.objects.create( creator=self.mock_data_generator.dummy_user("fake", "creator"), domain=self.mock_data_generator.dummy_domain("Zebra"), - submitter=self.mock_data_generator.dummy_contact("Apple", "submitter") + submitter=self.mock_data_generator.dummy_contact("Apple", "submitter"), ) DomainInformation.objects.create( creator=self.mock_data_generator.dummy_user("fake", "creator"), domain=self.mock_data_generator.dummy_domain("Circus"), - submitter=self.mock_data_generator.dummy_contact("Xylophone", "submitter") + submitter=self.mock_data_generator.dummy_contact("Xylophone", "submitter"), ) DomainInformation.objects.create( creator=self.mock_data_generator.dummy_user("fake", "creator"), domain=self.mock_data_generator.dummy_domain("Xylophone"), - submitter=self.mock_data_generator.dummy_contact("Circus", "submitter") + submitter=self.mock_data_generator.dummy_contact("Circus", "submitter"), ) def tearDown(self): @@ -999,43 +1011,34 @@ class DomainInformationAdminTest(TestCase): Contact.objects.all().delete() User.objects.all().delete() - def _assert_sort_helper(self, o_index, sort_field): - # 'o' (ordering) is based off the index position in the list_filter object, plus one. - # Q: Why is this not working?? - # domain_index = self.admin.list_filter.index("domain") + 1 - dummy_request = self.factory.get( - "/admin/registrar/DomainInformation/", - { - "o": o_index - }, - ) - dummy_request.user = self.superuser - - expected_sort_order = list(DomainInformation.objects.order_by(sort_field)) - returned_sort_order = list(self.admin.get_queryset(dummy_request)) - self.assertEqual(expected_sort_order, returned_sort_order) - def test_domain_sortable(self): """Tests if DomainInformation sorts by domain correctly""" p = "adminpass" self.client.login(username="superuser", password=p) # Assert that our sort works correctly - self._assert_sort_helper("1", "domain") - + self.test_helper.assert_table_sorted( + "1", ("domain__name",) + ) + # Assert that sorting in reverse works correctly - #self._assert_sort_helper("-1", "-domain") - + self.test_helper.assert_table_sorted( + "-1", ("-domain__name",) + ) + def test_submitter_sortable(self): """Tests if DomainInformation sorts by submitter correctly""" p = "adminpass" self.client.login(username="superuser", password=p) # Assert that our sort works correctly - self._assert_sort_helper("1", "submitter") - + self.test_helper.assert_table_sorted( + "4", + ("submitter__first_name", "submitter__last_name"), + ) + # Assert that sorting in reverse works correctly - #self._assert_sort_helper("-1", "-submitter") + self.test_helper.assert_table_sorted("-4", ("-submitter__first_name", "-submitter__last_name")) class UserDomainRoleAdminTest(TestCase): @@ -1043,9 +1046,16 @@ class UserDomainRoleAdminTest(TestCase): """Setup environment for a mock admin user""" self.site = AdminSite() self.factory = RequestFactory() - self.admin = ListHeaderAdmin(model=UserDomainRole, admin_site=self.site) + self.admin = UserDomainRoleAdmin(model=UserDomainRole, admin_site=self.site) self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/UserDomainRole/", + model=UserDomainRole + ) def tearDown(self): """Delete all Users, Domains, and UserDomainRoles""" @@ -1053,22 +1063,6 @@ class UserDomainRoleAdminTest(TestCase): Domain.objects.all().delete() UserDomainRole.objects.all().delete() - def _assert_sort_helper(self, o_index, sort_field): - # 'o' (ordering) is based off the index position in the list_filter object, plus one. - # Q: Why is this not working?? - # domain_index = self.admin.list_filter.index("domain") + 1 - dummy_request = self.factory.get( - "/admin/registrar/userdomainrole/", - { - "o": o_index - }, - ) - dummy_request.user = self.superuser - - expected_sort_order = list(UserDomainRole.objects.order_by(sort_field)) - returned_sort_order = list(self.admin.get_queryset(dummy_request)) - self.assertEqual(expected_sort_order, returned_sort_order) - def test_domain_sortable(self): """Tests if the UserDomainrole sorts by domain correctly""" p = "adminpass" @@ -1085,11 +1079,11 @@ class UserDomainRoleAdminTest(TestCase): UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") # Assert that our sort works correctly - self._assert_sort_helper("2", "domain") - + self.test_helper.assert_table_sorted("2", ("domain__name",)) + # Assert that sorting in reverse works correctly - self._assert_sort_helper("-2", "-domain") - + self.test_helper.assert_table_sorted("-2", ("-domain__name",)) + def test_user_sortable(self): """Tests if the UserDomainrole sorts by user correctly""" p = "adminpass" @@ -1106,10 +1100,10 @@ class UserDomainRoleAdminTest(TestCase): UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") # Assert that our sort works correctly - self._assert_sort_helper("1", "user") - + self.test_helper.assert_table_sorted("1", ("user__first_name", "user__last_name")) + # Assert that sorting in reverse works correctly - self._assert_sort_helper("-1", "-user") + self.test_helper.assert_table_sorted("-1", ("-user__first_name", "-user__last_name")) def test_email_not_in_search(self): """Tests the search bar in Django Admin for UserDomainRoleAdmin. From 12b2f93cb12dfb13fa548ad069796e5c54397054 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:18:38 -0700 Subject: [PATCH 12/31] Black formatting --- src/registrar/tests/common.py | 8 +++-- src/registrar/tests/test_admin.py | 60 +++++++++++++++++-------------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 9546a5d6a..93e13c87e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -92,14 +92,16 @@ def less_console_noise(output_stream=None): # we opened output_stream so we have to close it output_stream.close() + class GenericTestHelper(TestCase): """A helper class that contains various helper functions for TestCases""" + def __init__(self, admin, model=None, url=None, user=None, factory=None, **kwargs): """ Parameters: admin (ModelAdmin): The Django ModelAdmin instance associated with the model. - model (django.db.models.Model, optional): The Django model associated with the admin page. - url (str, optional): The URL of the Django Admin page to test. + model (django.db.models.Model, optional): The Django model associated with the admin page. + url (str, optional): The URL of the Django Admin page to test. user (User, optional): The Django User who is making the request. factory (RequestFactory, optional): An instance of Django's RequestFactory. """ @@ -109,7 +111,7 @@ class GenericTestHelper(TestCase): self.admin = admin self.model = model self.url = url - + def assert_table_sorted(self, o_index, sort_fields): """ This helper function validates the sorting functionality of a Django Admin table view. diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 58f40bb43..b42f7199e 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -330,14 +330,14 @@ class TestDomainApplicationAdmin(MockEppLib): user=self.superuser, admin=self.admin, url="/admin/registrar/DomainApplication/", - model=DomainApplication + model=DomainApplication, ) def test_domain_sortable(self): """Tests if the DomainApplication sorts by domain correctly""" p = "adminpass" self.client.login(username="superuser", password=p) - + multiple_unalphabetical_domain_objects("application") # Assert that our sort works correctly @@ -359,16 +359,22 @@ class TestDomainApplicationAdmin(MockEppLib): new_user.save() # Assert that our sort works correctly - self.test_helper.assert_table_sorted("5", ( - "submitter__first_name", - "submitter__last_name", - )) + self.test_helper.assert_table_sorted( + "5", + ( + "submitter__first_name", + "submitter__last_name", + ), + ) # Assert that sorting in reverse works correctly - self.test_helper.assert_table_sorted("-5", ( - "-submitter__first_name", - "-submitter__last_name", - )) + self.test_helper.assert_table_sorted( + "-5", + ( + "-submitter__first_name", + "-submitter__last_name", + ), + ) def test_investigator_sortable(self): """Tests if the DomainApplication sorts by domain correctly""" @@ -382,16 +388,22 @@ class TestDomainApplicationAdmin(MockEppLib): new_user.save() # Assert that our sort works correctly - self.test_helper.assert_table_sorted("6", ( - "investigator__first_name", - "investigator__last_name", - )) + self.test_helper.assert_table_sorted( + "6", + ( + "investigator__first_name", + "investigator__last_name", + ), + ) # Assert that sorting in reverse works correctly - self.test_helper.assert_table_sorted("-6", ( - "-investigator__first_name", - "-investigator__last_name", - )) + self.test_helper.assert_table_sorted( + "-6", + ( + "-investigator__first_name", + "-investigator__last_name", + ), + ) def test_short_org_name_in_applications_list(self): """ @@ -975,7 +987,7 @@ class DomainInformationAdminTest(TestCase): user=self.superuser, admin=self.admin, url="/admin/registrar/DomainInformation/", - model=DomainInformation + model=DomainInformation, ) # Create fake DomainInformation objects @@ -1017,14 +1029,10 @@ class DomainInformationAdminTest(TestCase): self.client.login(username="superuser", password=p) # Assert that our sort works correctly - self.test_helper.assert_table_sorted( - "1", ("domain__name",) - ) + self.test_helper.assert_table_sorted("1", ("domain__name",)) # Assert that sorting in reverse works correctly - self.test_helper.assert_table_sorted( - "-1", ("-domain__name",) - ) + self.test_helper.assert_table_sorted("-1", ("-domain__name",)) def test_submitter_sortable(self): """Tests if DomainInformation sorts by submitter correctly""" @@ -1054,7 +1062,7 @@ class UserDomainRoleAdminTest(TestCase): user=self.superuser, admin=self.admin, url="/admin/registrar/UserDomainRole/", - model=UserDomainRole + model=UserDomainRole, ) def tearDown(self): From c01b4ea750151c28a93b6ed18249df9ce81a2fa4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:43:18 -0700 Subject: [PATCH 13/31] Fix test case failures --- src/registrar/tests/common.py | 5 ++--- src/registrar/tests/test_admin.py | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 93e13c87e..db2bc6f84 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -5,7 +5,6 @@ import logging from contextlib import contextmanager import random from string import ascii_uppercase -import uuid from django.test import TestCase from unittest.mock import MagicMock, Mock, patch from typing import List, Dict @@ -229,14 +228,14 @@ class AuditedAdminMockData: user = User.objects.get_or_create( first_name="{} first_name:{}".format(item_name, short_hand), last_name="{} last_name:{}".format(item_name, short_hand), - username="{} username:{}".format(item_name + str(uuid.uuid4())[:8], short_hand), + username="{} username:{}".format(item_name, short_hand), )[0] return user def dummy_contact(self, item_name, short_hand): """Creates a dummy contact object""" contact = Contact.objects.get_or_create( - first_name="{} first_name:{}".format(item_name, short_hand), + first_name="{} first_name:{}".format(item_name + "fake_contact", short_hand), last_name="{} last_name:{}".format(item_name, short_hand), title="{} title:{}".format(item_name, short_hand), email="{}testy@town.com".format(item_name), diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index b42f7199e..89b1302b2 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1318,8 +1318,8 @@ class AuditedAdminTest(TestCase): tested_fields = [ DomainApplication.authorizing_official.field, DomainApplication.submitter.field, - # DomainApplication.investigator.field, - # DomainApplication.creator.field, + DomainApplication.investigator.field, + DomainApplication.creator.field, DomainApplication.requested_domain.field, ] @@ -1374,7 +1374,7 @@ class AuditedAdminTest(TestCase): tested_fields = [ DomainInformation.authorizing_official.field, DomainInformation.submitter.field, - # DomainInformation.creator.field, + DomainInformation.creator.field, (DomainInformation.domain.field, ["name"]), (DomainInformation.domain_application.field, ["requested_domain__name"]), ] From 4378816cdf0d152da16b6ffad21afe25eeb34b5f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:59:47 -0700 Subject: [PATCH 14/31] Fix signals bug --- src/registrar/models/contact.py | 4 ++-- src/registrar/signals.py | 2 +- src/registrar/tests/common.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index 6b3b6ddb2..ae5ca7d35 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -59,12 +59,12 @@ class Contact(TimeStampedModel): names = [n for n in [self.first_name, self.middle_name, self.last_name] if n] return " ".join(names) if names else "Unknown" - def save(self, *args, **kwargs): + def save(self, enable_custom_save=True, *args, **kwargs): # Call the parent class's save method to perform the actual save super().save(*args, **kwargs) # Update the related User object's first_name and last_name - if self.user: + if self.user and enable_custom_save: self.user.first_name = self.first_name self.user.last_name = self.last_name self.user.save() diff --git a/src/registrar/signals.py b/src/registrar/signals.py index 4e7768ef4..77ea3afac 100644 --- a/src/registrar/signals.py +++ b/src/registrar/signals.py @@ -46,7 +46,7 @@ def handle_profile(sender, instance, **kwargs): if len(contacts) >= 1 and is_new_user: # a matching contact contacts[0].user = instance - contacts[0].save() + contacts[0].save(enable_custom_save=False) if len(contacts) > 1: # multiple matches logger.warning( diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index db2bc6f84..07f14635e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -235,7 +235,7 @@ class AuditedAdminMockData: def dummy_contact(self, item_name, short_hand): """Creates a dummy contact object""" contact = Contact.objects.get_or_create( - first_name="{} first_name:{}".format(item_name + "fake_contact", short_hand), + first_name="{} first_name:{}".format(item_name, short_hand), last_name="{} last_name:{}".format(item_name, short_hand), title="{} title:{}".format(item_name, short_hand), email="{}testy@town.com".format(item_name), From 59dd53e8c766f6230fdee24848649044226c87ae Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 20 Dec 2023 08:19:21 -0700 Subject: [PATCH 15/31] Revert fix --- src/registrar/models/contact.py | 4 ++-- src/registrar/signals.py | 2 +- src/registrar/tests/common.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index ae5ca7d35..6b3b6ddb2 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -59,12 +59,12 @@ class Contact(TimeStampedModel): names = [n for n in [self.first_name, self.middle_name, self.last_name] if n] return " ".join(names) if names else "Unknown" - def save(self, enable_custom_save=True, *args, **kwargs): + def save(self, *args, **kwargs): # Call the parent class's save method to perform the actual save super().save(*args, **kwargs) # Update the related User object's first_name and last_name - if self.user and enable_custom_save: + if self.user: self.user.first_name = self.first_name self.user.last_name = self.last_name self.user.save() diff --git a/src/registrar/signals.py b/src/registrar/signals.py index 77ea3afac..4e7768ef4 100644 --- a/src/registrar/signals.py +++ b/src/registrar/signals.py @@ -46,7 +46,7 @@ def handle_profile(sender, instance, **kwargs): if len(contacts) >= 1 and is_new_user: # a matching contact contacts[0].user = instance - contacts[0].save(enable_custom_save=False) + contacts[0].save() if len(contacts) > 1: # multiple matches logger.warning( diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 07f14635e..db2bc6f84 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -235,7 +235,7 @@ class AuditedAdminMockData: def dummy_contact(self, item_name, short_hand): """Creates a dummy contact object""" contact = Contact.objects.get_or_create( - first_name="{} first_name:{}".format(item_name, short_hand), + first_name="{} first_name:{}".format(item_name + "fake_contact", short_hand), last_name="{} last_name:{}".format(item_name, short_hand), title="{} title:{}".format(item_name, short_hand), email="{}testy@town.com".format(item_name), From 5ba6f001f5975c6cb66d8c5de749c9b561cc9f98 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 20 Dec 2023 08:19:35 -0700 Subject: [PATCH 16/31] Revert "Fix test case failures" This reverts commit c01b4ea750151c28a93b6ed18249df9ce81a2fa4. --- src/registrar/tests/common.py | 5 +++-- src/registrar/tests/test_admin.py | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index db2bc6f84..93e13c87e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -5,6 +5,7 @@ import logging from contextlib import contextmanager import random from string import ascii_uppercase +import uuid from django.test import TestCase from unittest.mock import MagicMock, Mock, patch from typing import List, Dict @@ -228,14 +229,14 @@ class AuditedAdminMockData: user = User.objects.get_or_create( first_name="{} first_name:{}".format(item_name, short_hand), last_name="{} last_name:{}".format(item_name, short_hand), - username="{} username:{}".format(item_name, short_hand), + username="{} username:{}".format(item_name + str(uuid.uuid4())[:8], short_hand), )[0] return user def dummy_contact(self, item_name, short_hand): """Creates a dummy contact object""" contact = Contact.objects.get_or_create( - first_name="{} first_name:{}".format(item_name + "fake_contact", short_hand), + first_name="{} first_name:{}".format(item_name, short_hand), last_name="{} last_name:{}".format(item_name, short_hand), title="{} title:{}".format(item_name, short_hand), email="{}testy@town.com".format(item_name), diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 89b1302b2..b42f7199e 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1318,8 +1318,8 @@ class AuditedAdminTest(TestCase): tested_fields = [ DomainApplication.authorizing_official.field, DomainApplication.submitter.field, - DomainApplication.investigator.field, - DomainApplication.creator.field, + # DomainApplication.investigator.field, + # DomainApplication.creator.field, DomainApplication.requested_domain.field, ] @@ -1374,7 +1374,7 @@ class AuditedAdminTest(TestCase): tested_fields = [ DomainInformation.authorizing_official.field, DomainInformation.submitter.field, - DomainInformation.creator.field, + # DomainInformation.creator.field, (DomainInformation.domain.field, ["name"]), (DomainInformation.domain_application.field, ["requested_domain__name"]), ] From 1ba1780b055563a947f5cec5baef50d16dfad224 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:26:26 -0700 Subject: [PATCH 17/31] (Finally) fix test cases --- src/registrar/models/contact.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index 6b3b6ddb2..10033b46c 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -64,7 +64,7 @@ class Contact(TimeStampedModel): super().save(*args, **kwargs) # Update the related User object's first_name and last_name - if self.user: + if self.user and not self.user.first_name or not self.user.last_name: self.user.first_name = self.first_name self.user.last_name = self.last_name self.user.save() From 9504afcaa99450abd6f3d05135f68c6c1cb1f412 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 22 Dec 2023 12:22:09 -0700 Subject: [PATCH 18/31] Remove merge conflict --- src/registrar/admin.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index b428546c9..253934a3a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -772,13 +772,6 @@ class DomainApplicationAdmin(ListHeaderAdmin): filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") - # lists in filter_horizontal are not sorted properly, sort them - # by website - def formfield_for_manytomany(self, db_field, request, **kwargs): - if db_field.name in ("current_websites", "alternative_domains"): - kwargs["queryset"] = models.Website.objects.all().order_by("website") # Sort websites - return super().formfield_for_manytomany(db_field, request, **kwargs) - # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): if obj and obj.creator.status != models.User.RESTRICTED: From e9284f5dd0e347f59c62d6c277841a8dae269ea1 Mon Sep 17 00:00:00 2001 From: Cameron Dixon Date: Fri, 22 Dec 2023 22:24:15 -0500 Subject: [PATCH 19/31] Update SECURITY.md --- .github/SECURITY.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/SECURITY.md b/.github/SECURITY.md index fc27feff3..e6fba722b 100644 --- a/.github/SECURITY.md +++ b/.github/SECURITY.md @@ -1,5 +1,5 @@ -* If you've found a security or privacy issue on the **.gov top-level domain infrastructure**, submit it to our [vulnerabilty disclosure form](https://forms.office.com/Pages/ResponsePage.aspx?id=bOfNPG2UEkq7evydCEI1SqHke9Gh6wJEl3kQ5EjWUKlUMTZZS1lBVkxHUzZURFpLTkE2NEJFVlhVRi4u) or email dotgov@cisa.dhs.gov. -* If you see a security or privacy issue on **an individual .gov domain**, check [current-full.csv](https://flatgithub.com/cisagov/dotgov-data/blob/main/?filename=current-full.csv) or [Whois](https://domains.dotgov.gov/dotgov-web/registration/whois.xhtml) (same data) to check whether the domain has a security contact to report your finding directly. You are welcome to Cc dotgov@cisa.dhs.gov on the email. - * If you are unable to find a contact or receive no response from the security contact, email dotgov@cisa.dhs.gov. +* If you've found a security or privacy issue on the **.gov top-level domain infrastructure**, submit it to our [vulnerabilty disclosure form](https://forms.office.com/Pages/ResponsePage.aspx?id=bOfNPG2UEkq7evydCEI1SqHke9Gh6wJEl3kQ5EjWUKlUMTZZS1lBVkxHUzZURFpLTkE2NEJFVlhVRi4u) or email help@get.gov. +* If you see a security or privacy issue on **an individual .gov domain**, check [current-full.csv](https://flatgithub.com/cisagov/dotgov-data/blob/main/?filename=current-full.csv) to see whether the domain has a security contact to report your finding directly. You are welcome to Cc help@get.gov on the email. + * If you are unable to find a contact or receive no response from the security contact, email help@get.gov. Note that most federal (executive branch) agencies maintain a [vulnerability disclosure policy](https://github.com/cisagov/vdp-in-fceb/). From b1530b5e5b34bbaa9c4da009051bbdae16d57d56 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 07:41:43 -0700 Subject: [PATCH 20/31] Update extend_expiration_dates.py --- src/registrar/management/commands/extend_expiration_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/extend_expiration_dates.py b/src/registrar/management/commands/extend_expiration_dates.py index 59b0274ab..392713710 100644 --- a/src/registrar/management/commands/extend_expiration_dates.py +++ b/src/registrar/management/commands/extend_expiration_dates.py @@ -30,7 +30,7 @@ class Command(BaseCommand): self.update_skipped = [] self.update_failed = [] self.expiration_minimum_cutoff = date(2023, 11, 1) - self.expiration_maximum_cutoff = date(2023, 12, 30) + self.expiration_maximum_cutoff = date(2024, 12, 31) def add_arguments(self, parser): """Add command line arguments.""" From 7354b45aff40b61cd925d3cb812d582ca4380bfa Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 07:49:00 -0700 Subject: [PATCH 21/31] Update extend_expiration_dates.py --- src/registrar/management/commands/extend_expiration_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/extend_expiration_dates.py b/src/registrar/management/commands/extend_expiration_dates.py index 392713710..cefc38b9e 100644 --- a/src/registrar/management/commands/extend_expiration_dates.py +++ b/src/registrar/management/commands/extend_expiration_dates.py @@ -30,7 +30,7 @@ class Command(BaseCommand): self.update_skipped = [] self.update_failed = [] self.expiration_minimum_cutoff = date(2023, 11, 1) - self.expiration_maximum_cutoff = date(2024, 12, 31) + self.expiration_maximum_cutoff = date(2024, 12, 30) def add_arguments(self, parser): """Add command line arguments.""" From 7a534bb7748e3bde85d3997e08ac3eacc30d5648 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 09:31:52 -0700 Subject: [PATCH 22/31] Black formatting --- src/registrar/admin.py | 8 ++++---- src/registrar/views/utility/mixins.py | 29 +++++++++++++++------------ 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a4665765e..3058b283e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -47,7 +47,7 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): def get_ordering(self, request, queryset): """ Returns the list of ordering fields for the change list. - + Mostly identical to the base implementation, except that now it can return a list of order_field objects rather than just one. """ @@ -189,13 +189,13 @@ class AuditedAdmin(admin.ModelAdmin): class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): - """Custom admin to add a descriptive subheader to list views + """Custom admin to add a descriptive subheader to list views and custom table sort behaviour""" def get_changelist(self, request, **kwargs): """Returns a custom ChangeList class, as opposed to the default. This is so we can override the behaviour of the `admin_order_field` field. - By default, django does not support ordering by multiple fields for this + By default, django does not support ordering by multiple fields for this particular field (i.e. self.admin_order_field=["first_name", "last_name"] is invalid). Reference: https://code.djangoproject.com/ticket/31975 @@ -478,7 +478,7 @@ class UserDomainRoleAdmin(ListHeaderAdmin): "domain", "role", ] - + orderable_fk_fields = [ ("domain", "name"), ("user", ["first_name", "last_name"]), diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 0d9f955d9..199decb7c 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,5 +1,6 @@ """Permissions-related mixin classes.""" +from typing import List from django.contrib.auth.mixins import PermissionRequiredMixin from registrar.models import ( @@ -19,8 +20,9 @@ class OrderableFieldsMixin: """ Mixin to add multi-field ordering capabilities to a Django ModelAdmin on admin_order_field. """ + custom_sort_name_prefix = "get_sortable_" - orderable_fk_fields = [] + orderable_fk_fields: List[(str, List[str])] = [] def __new__(cls, *args, **kwargs): """ @@ -31,7 +33,7 @@ class OrderableFieldsMixin: new_class = super().__new__(cls) # If the class doesn't define anything for orderable_fk_fields, then we should - # just skip this additional logic + # just skip this additional logic if not hasattr(cls, "orderable_fk_fields") or len(cls.orderable_fk_fields) == 0: return new_class @@ -42,7 +44,7 @@ class OrderableFieldsMixin: for field, sort_field in cls.orderable_fk_fields: updated_name = cls.custom_sort_name_prefix + field - # For each item in orderable_fk_fields, create a function and associate it with admin_order_field. + # For each item in orderable_fk_fields, create a function and associate it with admin_order_field. setattr(new_class, updated_name, cls._create_orderable_field_method(field, sort_field)) # Update the list_display variable to use our newly created functions @@ -62,18 +64,18 @@ class OrderableFieldsMixin: def _create_orderable_field_method(cls, field, sort_field): """ This class method is a factory for creating dynamic methods that will be attached to the ModelAdmin subclass. - It is used to customize how fk fields are ordered. + It is used to customize how fk fields are ordered. - In essence, this function will more or less generate code that looks like this, + In essence, this function will more or less generate code that looks like this, for a given tuple defined in orderable_fk_fields: - + ``` def get_sortable_requested_domain(self, obj): return obj.requested_domain # Allows column order sorting get_sortable_requested_domain.admin_order_field = "requested_domain__name" # Sets column's header name - get_sortable_requested_domain.short_description = "requested domain" + get_sortable_requested_domain.short_description = "requested domain" ``` Or for fields with multiple order_fields: @@ -82,9 +84,9 @@ class OrderableFieldsMixin: def get_sortable_submitter(self, obj): return obj.submitter # Allows column order sorting - get_sortable_submitter.admin_order_field = ["submitter__first_name", "submitter__last_name"] - # Sets column's header - get_sortable_submitter.short_description = "submitter" + get_sortable_submitter.admin_order_field = ["submitter__first_name", "submitter__last_name"] + # Sets column's header + get_sortable_submitter.short_description = "submitter" ``` Parameters: @@ -97,15 +99,16 @@ class OrderableFieldsMixin: The dynamically created method has the following attributes: __name__: A string representing the name of the method. This is set to "get_{field}". - admin_order_field: A string or list of strings representing the field(s) that + admin_order_field: A string or list of strings representing the field(s) that Django should sort by when the column is clicked in the admin interface. short_description: A string used as the column header in the admin interface. Will replace underscores with spaces. """ + def method(obj): """ - Template method for patterning. + Template method for patterning. - Returns (example): + Returns (example): ``` def get_submitter(self, obj): return obj.submitter From c5376d46da900ceb62a898586ebf94d7e574e604 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 09:35:23 -0700 Subject: [PATCH 23/31] Fix typing issue --- src/registrar/views/utility/mixins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 199decb7c..7ad6fe8df 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,6 +1,6 @@ """Permissions-related mixin classes.""" -from typing import List +from typing import List, Tuple from django.contrib.auth.mixins import PermissionRequiredMixin from registrar.models import ( @@ -22,7 +22,7 @@ class OrderableFieldsMixin: """ custom_sort_name_prefix = "get_sortable_" - orderable_fk_fields: List[(str, List[str])] = [] + orderable_fk_fields: List[Tuple[str, List[str]]] = [] def __new__(cls, *args, **kwargs): """ From 0b147019ebf914c7badadb7d7f8efaa8ca845a6b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 09:55:41 -0700 Subject: [PATCH 24/31] Remove typing check --- src/registrar/views/utility/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 7ad6fe8df..4a32a92d6 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -22,7 +22,7 @@ class OrderableFieldsMixin: """ custom_sort_name_prefix = "get_sortable_" - orderable_fk_fields: List[Tuple[str, List[str]]] = [] + orderable_fk_fields = [] # type: ignore def __new__(cls, *args, **kwargs): """ From 65f7122a58e26cbe7f84ec4ef9293f3d221f3cb0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 10:31:55 -0700 Subject: [PATCH 25/31] Remove wonky test A test I added was acting unpredictably, and was redundant as compared to another test --- src/registrar/tests/test_admin.py | 46 ------------------------------- 1 file changed, 46 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 1e564a623..2cc6b48be 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1008,52 +1008,6 @@ class TestDomainApplicationAdmin(MockEppLib): ], ) - def test_investigator_filter_filters_correctly(self): - """ - This test verifies that the investigator filter in the admin interface for - the DomainApplication model works correctly. - - It creates two DomainApplication instances, each with a different investigator. - It then simulates a staff user logging in and applying the investigator filter - on the DomainApplication admin page. - - It then verifies that it was applied correctly. - The test checks that the response contains the expected DomainApplication pbjects - in the table. - """ - - # Create a mock DomainApplication object, with a fake investigator - application: DomainApplication = generic_domain_object("application", "SomeGuy") - investigator_user = User.objects.filter(username=application.investigator.username).get() - investigator_user.is_staff = True - investigator_user.save() - - # Create a second mock DomainApplication object, to test filtering - application: DomainApplication = generic_domain_object("application", "BadGuy") - another_user = User.objects.filter(username=application.investigator.username).get() - another_user.is_staff = True - another_user.save() - - p = "userpass" - self.client.login(username="staffuser", password=p) - response = self.client.get( - "/admin/registrar/domainapplication/", - { - "investigator__id__exact": investigator_user.id, - }, - follow=True, - ) - - expected_name = "SomeGuy first_name:investigator SomeGuy last_name:investigator" - # We expect to see this four times, two of them are from the html for the filter, - # and the other two are the html from the list entry in the table. - self.assertContains(response, expected_name, count=4) - - # Check that we don't also get the thing we aren't filtering for. - # We expect to see this two times in the filter - unexpected_name = "BadGuy first_name:investigator BadGuy last_name:investigator" - self.assertContains(response, unexpected_name, count=2) - def test_investigator_dropdown_displays_only_staff(self): """ This test verifies that the dropdown for the 'investigator' field in the DomainApplicationAdmin From e3e9eb8dbb5592fddfce5ab798bde0d6223f27c5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 10:53:47 -0700 Subject: [PATCH 26/31] Linting --- src/registrar/views/utility/mixins.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 4a32a92d6..0cf5970df 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -1,6 +1,5 @@ """Permissions-related mixin classes.""" -from typing import List, Tuple from django.contrib.auth.mixins import PermissionRequiredMixin from registrar.models import ( @@ -22,7 +21,7 @@ class OrderableFieldsMixin: """ custom_sort_name_prefix = "get_sortable_" - orderable_fk_fields = [] # type: ignore + orderable_fk_fields = [] # type: ignore def __new__(cls, *args, **kwargs): """ @@ -63,7 +62,8 @@ class OrderableFieldsMixin: @classmethod def _create_orderable_field_method(cls, field, sort_field): """ - This class method is a factory for creating dynamic methods that will be attached to the ModelAdmin subclass. + This class method is a factory for creating dynamic methods that will be attached + to the ModelAdmin subclass. It is used to customize how fk fields are ordered. In essence, this function will more or less generate code that looks like this, @@ -90,9 +90,12 @@ class OrderableFieldsMixin: ``` Parameters: - cls: The class that this method is being called on. In the context of this mixin, it would be the ModelAdmin subclass. - field: A string representing the name of the attribute that the dynamic method will fetch from the model instance. - sort_field: A string or list of strings representing the field(s) to sort by (ex: "name" or "creator") + cls: The class that this method is being called on. In the context of this mixin, + it would be the ModelAdmin subclass. + field: A string representing the name of the attribute that + the dynamic method will fetch from the model instance. + sort_field: A string or list of strings representing the + field(s) to sort by (ex: "name" or "creator") Returns: method: The dynamically created method. @@ -101,7 +104,8 @@ class OrderableFieldsMixin: __name__: A string representing the name of the method. This is set to "get_{field}". admin_order_field: A string or list of strings representing the field(s) that Django should sort by when the column is clicked in the admin interface. - short_description: A string used as the column header in the admin interface. Will replace underscores with spaces. + short_description: A string used as the column header in the admin interface. + Will replace underscores with spaces. """ def method(obj): From 337a13bbe729717e957d93569530a295c7f694ce Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 12:30:23 -0700 Subject: [PATCH 27/31] Update test_models.py --- src/registrar/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ceabef11e..6c82996ea 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -21,7 +21,7 @@ from django_fsm import TransitionNotAllowed boto3_mocking.clients.register_handler("sesv2", MockSESClient) - +# Test comment for push -- will remove # The DomainApplication submit method has a side effect of sending an email # with AWS SES, so mock that out in all of these test cases @boto3_mocking.patching From aa1e1b141d73f8f271e4b5dc6da7474f6b683b12 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 14:49:35 -0700 Subject: [PATCH 28/31] Add sorting by email --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3058b283e..aed06021c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -481,7 +481,7 @@ class UserDomainRoleAdmin(ListHeaderAdmin): orderable_fk_fields = [ ("domain", "name"), - ("user", ["first_name", "last_name"]), + ("user", ["first_name", "last_name", "email"]), ] # Search From f5e18eb3b16e6aca3ba1649298835917afb8268e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 14:59:45 -0700 Subject: [PATCH 29/31] Linter --- src/registrar/tests/test_models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 6c82996ea..6124b76f3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -21,6 +21,7 @@ from django_fsm import TransitionNotAllowed boto3_mocking.clients.register_handler("sesv2", MockSESClient) + # Test comment for push -- will remove # The DomainApplication submit method has a side effect of sending an email # with AWS SES, so mock that out in all of these test cases From 0c2681ebfd5e1955baa25e59fbc5f8fa5ada6b2b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 15:17:02 -0700 Subject: [PATCH 30/31] Fix casing --- src/registrar/models/domain_information.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain_information.py b/src/registrar/models/domain_information.py index e6c323128..bdff6061b 100644 --- a/src/registrar/models/domain_information.py +++ b/src/registrar/models/domain_information.py @@ -245,4 +245,4 @@ class DomainInformation(TimeStampedModel): return domain_info class Meta: - verbose_name_plural = "Domain Information" + verbose_name_plural = "Domain information" From 8311d94c7f9be1f082f160c5160001c81f93f0ad Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Dec 2023 15:26:18 -0700 Subject: [PATCH 31/31] Create 0058_alter_domaininformation_options.py --- .../0058_alter_domaininformation_options.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 src/registrar/migrations/0058_alter_domaininformation_options.py diff --git a/src/registrar/migrations/0058_alter_domaininformation_options.py b/src/registrar/migrations/0058_alter_domaininformation_options.py new file mode 100644 index 000000000..2e128cbda --- /dev/null +++ b/src/registrar/migrations/0058_alter_domaininformation_options.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.7 on 2023-12-27 22:26 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0057_domainapplication_submission_date"), + ] + + operations = [ + migrations.AlterModelOptions( + name="domaininformation", + options={"verbose_name_plural": "Domain information"}, + ), + ]