From c1df03d831c48c1d4cbb25afe2fa806ee687da5c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:15:42 -0400 Subject: [PATCH] test and lint --- src/registrar/admin.py | 2 - src/registrar/config/settings.py | 8 +- src/registrar/config/urls.py | 2 +- src/registrar/tests/test_admin.py | 225 ++++++++++++++++++++++++++- src/registrar/views/__init__.py | 1 + src/registrar/views/transfer_user.py | 82 +++++----- 6 files changed, 274 insertions(+), 46 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4a34ac0f8..3ad5e3ea0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -41,8 +41,6 @@ from django.core.exceptions import ObjectDoesNotExist from django.contrib.admin.widgets import FilteredSelectMultiple from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ -from django.shortcuts import get_object_or_404, render -from django.urls import path logger = logging.getLogger(__name__) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index d1f8d2384..7965424bc 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,14 +357,18 @@ CSP_FORM_ACTION = allowed_sources # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN CSP_DEFAULT_SRC = ("'self'",) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css", "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css"] +CSP_STYLE_SRC = [ + "'self'", + "https://www.ssa.gov/accessibility/andi/andi.css", + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css", +] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js", "https://www.ssa.gov", "https://ajax.googleapis.com", - "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js" + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js", ] CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index ee32930f1..0a8e00350 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -130,7 +130,7 @@ urlpatterns = [ AnalyticsView.as_view(), name="analytics", ), - path('admin/registrar/user//transfer/', TransferUserView.as_view(), name='transfer_user'), + path("admin/registrar/user//transfer/", TransferUserView.as_view(), name="transfer_user"), path( "admin/api/get-senior-official-from-federal-agency-json/", get_senior_official_from_federal_agency_json, diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..f051325a6 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ from registrar.models import ( from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_domain_role import UserDomainRole +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -60,7 +61,8 @@ from .common import ( ) from django.contrib.sessions.backends.db import SessionStore from django.contrib.auth import get_user_model -from unittest.mock import patch, Mock +from unittest.mock import ANY, patch, Mock +from django_webtest import WebTest # type: ignore import logging @@ -2121,3 +2123,224 @@ class TestPortfolioAdmin(TestCase): self.assertIn("request1.gov", domain_requests) self.assertIn("request2.gov", domain_requests) self.assertIn('
    ', domain_requests) + + +class TestTransferUser(WebTest): + """User transfer custom admin page""" + + # csrf checks do not work well with WebTest. + # We disable them here. + csrf_checks = False + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.site = AdminSite() + cls.superuser = create_superuser() + cls.admin = PortfolioAdmin(model=Portfolio, admin_site=cls.site) + cls.factory = RequestFactory() + + def setUp(self): + self.app.set_user(self.superuser) + self.user1, _ = User.objects.get_or_create( + username="madmax", first_name="Max", last_name="Rokatanski", title="Road warrior" + ) + self.user2, _ = User.objects.get_or_create( + username="furiosa", first_name="Furiosa", last_name="Jabassa", title="Imperator" + ) + + def tearDown(self): + Suborganization.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + Domain.objects.all().delete() + Portfolio.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + def test_transfer_user_shows_current_and_selected_user_information(self): + """Assert we pull the current user info and display it on the transfer page""" + completed_domain_request(user=self.user1, name="wasteland.gov") + domain_request = completed_domain_request( + user=self.user1, name="citadel.gov", status=DomainRequest.DomainRequestStatus.SUBMITTED + ) + domain_request.status = DomainRequest.DomainRequestStatus.APPROVED + domain_request.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + self.assertContains(user_transfer_page, "madmax") + self.assertContains(user_transfer_page, "Max") + self.assertContains(user_transfer_page, "Rokatanski") + self.assertContains(user_transfer_page, "Road warrior") + self.assertContains(user_transfer_page, "wasteland.gov") + self.assertContains(user_transfer_page, "citadel.gov") + + select_form = user_transfer_page.forms[0] + select_form["selected_user"] = str(self.user2.id) + preview_result = select_form.submit() + + self.assertContains(preview_result, "furiosa") + self.assertContains(preview_result, "Furiosa") + self.assertContains(preview_result, "Jabassa") + self.assertContains(preview_result, "Imperator") + + @less_console_noise_decorator + def test_transfer_user_transfers_portfolio_roles_and_permissions_and_portfolio_creator(self): + """Assert that a portfolio gets copied over + NOTE: Should be revised for #2644""" + portfolio = Portfolio.objects.create(organization_name="Citadel", creator=self.user2) + self.user2.portfolio = portfolio + self.user2.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + self.user2.portfolio_additional_permissions = [ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ] + self.user2.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + + self.user1.refresh_from_db() + portfolio.refresh_from_db() + + self.assertEquals(portfolio.creator, self.user1) + self.assertEquals(self.user1.portfolio, portfolio) + self.assertEquals(self.user1.portfolio_roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + self.assertEquals( + self.user1.portfolio_additional_permissions, + [UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO], + ) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_request_creator_and_investigator(self): + """Assert that domain request fields get transferred""" + domain_request = completed_domain_request(user=self.user2, name="wasteland.gov", investigator=self.user2) + + self.assertEquals(domain_request.creator, self.user2) + self.assertEquals(domain_request.investigator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_request.refresh_from_db() + + self.assertEquals(domain_request.creator, self.user1) + self.assertEquals(domain_request.investigator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_information_creator(self): + """Assert that domain fields get transferred""" + domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user2) + + self.assertEquals(domain_information.creator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_information.refresh_from_db() + + self.assertEquals(domain_information.creator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_role(self): + """Assert that user domain role get transferred""" + domain_1, _ = Domain.objects.get_or_create(name="chrome.gov", state=Domain.State.READY) + domain_2, _ = Domain.objects.get_or_create(name="v8.gov", state=Domain.State.READY) + user_domain_role1, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_1, role=UserDomainRole.Roles.MANAGER + ) + user_domain_role2, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_2, role=UserDomainRole.Roles.MANAGER + ) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + user_domain_role1.refresh_from_db() + user_domain_role2.refresh_from_db() + + self.assertEquals(user_domain_role1.user, self.user1) + self.assertEquals(user_domain_role2.user, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_verified_by_staff_requestor(self): + """Assert that verified by staff creator gets transferred""" + vip, _ = VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + vip.refresh_from_db() + + self.assertEquals(vip.requestor, self.user1) + + @less_console_noise_decorator + def test_transfer_user_deletes_old_user(self): + """Assert that the slected user gets deleted""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + # Refresh user2 from the database and check if it still exists + with self.assertRaises(User.DoesNotExist): + self.user2.refresh_from_db() + + @less_console_noise_decorator + def test_transfer_user_throws_transfer_and_delete_success_messages(self): + """Test that success messages for data transfer and user deletion are displayed.""" + # Ensure the setup for VerifiedByStaff + VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + with patch("django.contrib.messages.success") as mock_success_message: + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + after_submit = submit_form.submit().follow() + + self.assertContains(after_submit, "

    Change user

    ") + + mock_success_message.assert_any_call( + ANY, + ( + "Data transferred successfully for the following objects: ['Changed requestor " + + 'from "Furiosa Jabassa " to "Max Rokatanski " on immortan.joe@citadel.com\']' + ), + ) + + mock_success_message.assert_any_call(ANY, f"Deleted {self.user2} {self.user2.username}") + + @less_console_noise_decorator + def test_transfer_user_throws_error_message(self): + """Test that an error message is thrown if the transfer fails.""" + with patch( + "registrar.views.TransferUserView.transfer_user_fields_and_log", side_effect=Exception("Simulated Error") + ): + with patch("django.contrib.messages.error") as mock_error: + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit().follow() + + # Assert that the error message was called with the correct argument + mock_error.assert_called_once_with(ANY, "An error occurred during the transfer: Simulated Error") + + @less_console_noise_decorator + def test_transfer_user_modal(self): + """Assert modal on page""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + self.assertContains(user_transfer_page, "But you know what you're doing, right?") diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py index f6e87dd07..2b830d958 100644 --- a/src/registrar/views/__init__.py +++ b/src/registrar/views/__init__.py @@ -19,3 +19,4 @@ from .user_profile import UserProfileView, FinishProfileSetupView from .health import * from .index import * from .portfolios import * +from .transfer_user import TransferUserView diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index 5e367f04d..cc901d5ab 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -15,57 +15,60 @@ from registrar.models.verified_by_staff import VerifiedByStaff logger = logging.getLogger(__name__) + class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" JOINS = [ - (DomainRequest, 'creator'), - (DomainInformation, 'creator'), - (Portfolio, 'creator'), - (DomainRequest, 'investigator'), - (UserDomainRole, 'user'), - (VerifiedByStaff, 'requestor'), + (DomainRequest, "creator"), + (DomainInformation, "creator"), + (Portfolio, "creator"), + (DomainRequest, "investigator"), + (UserDomainRole, "user"), + (VerifiedByStaff, "requestor"), ] - USER_FIELDS = ['portfolio', 'portfolio_roles', 'portfolio_additional_permissions'] + USER_FIELDS = ["portfolio", "portfolio_roles", "portfolio_additional_permissions"] def get(self, request, user_id): """current_user referes to the 'source' user where the button that redirects to this view was clicked. other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. - + This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" current_user = get_object_or_404(User, pk=user_id) - other_users = User.objects.exclude(pk=user_id).order_by('first_name', 'last_name') # Exclude the current user from the dropdown + other_users = User.objects.exclude(pk=user_id).order_by( + "first_name", "last_name" + ) # Exclude the current user from the dropdown # Get the default admin site context, needed for the sidenav admin_context = site.each_context(request) context = { - 'current_user': current_user, - 'other_users': other_users, - 'logged_in_user': request.user, + "current_user": current_user, + "other_users": other_users, + "logged_in_user": request.user, **admin_context, # Include the admin context - 'current_user_domains': self.get_domains(current_user), - 'current_user_domain_requests': self.get_domain_requests(current_user) + "current_user_domains": self.get_domains(current_user), + "current_user_domain_requests": self.get_domain_requests(current_user), } - selected_user_id = request.GET.get('selected_user') + selected_user_id = request.GET.get("selected_user") if selected_user_id: selected_user = get_object_or_404(User, pk=selected_user_id) - context['selected_user'] = selected_user - context['selected_user_domains'] = self.get_domains(selected_user) - context['selected_user_domain_requests'] = self.get_domain_requests(selected_user) + context["selected_user"] = selected_user + context["selected_user_domains"] = self.get_domains(selected_user) + context["selected_user_domain_requests"] = self.get_domain_requests(selected_user) - return render(request, 'admin/transfer_user.html', context) + return render(request, "admin/transfer_user.html", context) def post(self, request, user_id): """This handles the transfer from selected_user to current_user then deletes selected_user. - + NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" current_user = get_object_or_404(User, pk=user_id) - selected_user_id = request.POST.get('selected_user') + selected_user_id = request.POST.get("selected_user") selected_user = get_object_or_404(User, pk=selected_user_id) try: @@ -89,53 +92,52 @@ class TransferUserView(View): except Exception as e: messages.error(request, f"An error occurred during the transfer: {e}") - return redirect('admin:registrar_user_change', object_id=user_id) + return redirect("admin:registrar_user_change", object_id=user_id) @classmethod - def update_joins_and_log(cls, model_class, field_name, old_user, new_user, change_logs): + def update_joins_and_log(cls, model_class, field_name, selected_user, current_user, change_logs): """ Helper function to update the user join fields for a given model and log the changes. """ - filter_kwargs = {field_name: old_user} + filter_kwargs = {field_name: selected_user} updated_objects = model_class.objects.filter(**filter_kwargs) for obj in updated_objects: # Check for duplicate UserDomainRole before updating if model_class == UserDomainRole: - if model_class.objects.filter(user=new_user, domain=obj.domain).exists(): + if model_class.objects.filter(user=current_user, domain=obj.domain).exists(): continue # Skip the update to avoid a duplicate # Update the field on the object and save it - setattr(obj, field_name, new_user) + setattr(obj, field_name, current_user) obj.save() # Log the change - cls.log_change(obj, field_name, old_user, new_user, change_logs) + cls.log_change(obj, field_name, selected_user, current_user, change_logs) @classmethod - def transfer_user_fields_and_log(cls, old_user, new_user, change_logs): + def transfer_user_fields_and_log(cls, selected_user, current_user, change_logs): """ - Transfers portfolio fields from the old_user to the new_user. + Transfers portfolio fields from the selected_user to the current_user. Logs the changes for each transferred field. NOTE: This will be refactored in #2644 """ - for field in cls.USER_FIELDS: - old_value = getattr(old_user, field, None) + field_value = getattr(selected_user, field, None) - if old_value: - setattr(new_user, field, old_value) - cls.log_change(new_user, field, old_value, old_value, change_logs) + if field_value: + setattr(current_user, field, field_value) + cls.log_change(current_user, field, field_value, field_value, change_logs) - new_user.save() + current_user.save() @classmethod - def log_change(cls, obj, field_name, old_value, new_value, change_logs): + def log_change(cls, obj, field_name, field_value, new_value, change_logs): """Logs the change for a specific field on an object""" - log_entry = f'Changed {field_name} from "{old_value}" to "{new_value}" on {obj}' - + log_entry = f'Changed {field_name} from "{field_value}" to "{new_value}" on {obj}' + logger.info(log_entry) # Collect the related object for the success message @@ -149,10 +151,10 @@ class TransferUserView(View): domains = Domain.objects.filter(id__in=domain_ids) return domains - + @classmethod def get_domain_requests(cls, user): """A simplified version of domain_requests_json""" domain_requests = DomainRequest.objects.filter(creator=user) - return domain_requests \ No newline at end of file + return domain_requests