From 0378f1570e821ff1ea43f36d4f8a50b8c59a861e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 25 Aug 2023 08:34:13 -0600 Subject: [PATCH] Code cleanup Had some older code lying around --- src/registrar/admin.py | 17 +++--------- src/registrar/tests/test_admin.py | 38 +++++++++++---------------- src/registrar/views/utility/mixins.py | 20 ++++---------- 3 files changed, 24 insertions(+), 51 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5d6b8c9b8..80f39dad6 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -171,24 +171,14 @@ class DomainAdmin(ListHeaderAdmin): return HttpResponseRedirect(reverse("domain", args=(obj.id,))) return super().response_change(request, obj) - # Sets domain_id as a context var - def change_view(self, request, object_id, form_url="", extra_context=None): + def change_view(self, request, object_id): + # If the analyst was recently editing if "analyst_action" in request.session: - # If an analyst performed an edit action, # delete the session variable del request.session["analyst_action"] # delete the associated location del request.session["analyst_action_location"] - - extra_context = extra_context or {} - extra_context["domain_id"] = object_id - - return super().change_view( - request, - object_id, - form_url, - extra_context=extra_context, - ) + return super().change_view(request, object_id) def has_change_permission(self, request, obj=None): # Fixes a bug wherein users which are only is_staff @@ -196,7 +186,6 @@ class DomainAdmin(ListHeaderAdmin): # but cannot access this page when it is a request of type POST. if request.user.is_staff: return True - return super().has_change_permission(request, obj) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9d60e6ba3..35e64582d 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -14,10 +14,9 @@ from registrar.models import ( DomainInformation, User, DomainInvitation, + Domain ) -from registrar.models.domain import Domain from .common import ( - AuditedAdminMockData, completed_application, generic_domain_object, mock_user, @@ -25,11 +24,8 @@ from .common import ( create_user, multiple_unalphabetical_domain_objects, ) - from django.contrib.sessions.backends.db import SessionStore - from django.contrib.auth import get_user_model - from django.conf import settings from unittest.mock import MagicMock import boto3_mocking # type: ignore @@ -651,7 +647,6 @@ class DomainSessionVariableTest(TestCase): self.factory = RequestFactory() self.admin = DomainAdmin(Domain, None) self.client = Client(HTTP_HOST="localhost:8080") - self.superuser = create_superuser() def test_session_variables_set_correctly(self): """Checks if session variables are being set correctly""" @@ -660,12 +655,7 @@ class DomainSessionVariableTest(TestCase): self.client.login(username="superuser", password=p) dummy_domain_information: DomainInformation = generic_domain_object("information", "session") - request = self.factory.post( - reverse('admin:registrar_domain_change', args=(dummy_domain_information.domain.pk,)), - {'_edit_domain': 'true'}, - follow=True - ) - + request = self.get_factory_post_edit_domain(dummy_domain_information.domain.pk) self.populate_session_values(request, dummy_domain_information.domain) self.assertEqual(request.session['analyst_action'], 'edit') @@ -676,19 +666,13 @@ class DomainSessionVariableTest(TestCase): p = "adminpass" self.client.login(username="superuser", password=p) - # We need to create multiple of these to ensure data is consistent across all of them - dummy_domain_information_list: [DomainInformation] = multiple_unalphabetical_domain_objects("invitation") + dummy_domain_information_list: [DomainInformation] = multiple_unalphabetical_domain_objects("information") for item in dummy_domain_information_list: - request = self.factory.post( - reverse('admin:registrar_domain_change', args=(item.pk,)), - {'_edit_domain': 'true'}, - follow=True - ) - + request = self.get_factory_post_edit_domain(item.domain.pk) self.populate_session_values(request, item) self.assertEqual(request.session['analyst_action'], 'edit') - self.assertEqual(request.session['analyst_action_location'], item.pk) + self.assertEqual(request.session['analyst_action_location'], item.domain.pk) def test_session_variables_concurrent_requests(self): """ Simulates two requests at once """ @@ -730,4 +714,14 @@ class DomainSessionVariableTest(TestCase): request.user = self.client request.session = SessionStore() request.session.create() - self.admin.response_change(request, domain_object) \ No newline at end of file + self.admin.response_change(request, domain_object) + + def get_factory_post_edit_domain(self, primary_key): + """Posts to registrar domain change + with the edit domain button 'clicked', + then returns the factory object""" + return self.factory.post( + reverse('admin:registrar_domain_change', args=(primary_key,)), + {'_edit_domain': 'true'}, + follow=True + ) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 13ea70684..5d1101422 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -2,9 +2,7 @@ from django.contrib.auth.mixins import PermissionRequiredMixin -from registrar.models import DomainApplication, DomainInvitation - -from registrar.models import DomainInformation, UserDomainRole +from registrar.models import DomainApplication, DomainInvitation, DomainInformation, UserDomainRole import logging logger = logging.getLogger(__name__) @@ -34,27 +32,20 @@ class DomainPermission(PermissionsLoginMixin): return False pk = self.kwargs["pk"] - # If pk is none then something went very wrong... if pk is None: raise ValueError("Primary key is None") - # Checks if the creator is the user requesting this item - - user_is_creator: bool = UserDomainRole.objects.filter( - user=self.request.user, domain__id=pk - ).exists() - # user needs to have a role on the domain - if user_is_creator: + if UserDomainRole.objects.filter( + user=self.request.user, domain__id=pk + ).exists(): return True # ticket 806 requested_domain: DomainInformation = None - try: requested_domain = DomainInformation.objects.get(id=pk) - except DomainInformation.DoesNotExist: # Q: While testing, I saw that, application-wide, if you go to a domain # that does not exist, for example, @@ -62,7 +53,7 @@ class DomainPermission(PermissionsLoginMixin): # the page throws a 403 error, instead of a 404. # Do we want it to throw a 404 instead? # Basically, should this be Http404()? - logger.warning(f"Domain with PK {pk} does not exist") + logger.debug(f"Domain with PK {pk} does not exist") return False # Analysts may manage domains, when they are in these statuses: @@ -79,7 +70,6 @@ class DomainPermission(PermissionsLoginMixin): ) session = self.request.session - # Check if the user is attempting a valid edit action. # If analyst_action is present, analyst_action_location will be present. # if it isn't, then it either suggests tampering