From c98392baacfcdf365469e4804b6cd18d8dfacb37 Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 18 Aug 2023 17:24:34 -0400 Subject: [PATCH 1/7] Add ineligible status on domain application, on user. Trigger fsm transition on domain application which triggers status ineligible on user. Edit permissions on domains and newly created application wizard perm class to block access to ineligible users. Run migrations. --- src/registrar/admin.py | 12 +++ ...r_status_alter_domainapplication_status.py | 42 ++++++++ src/registrar/models/domain_application.py | 15 ++- src/registrar/models/user.py | 27 +++++ src/registrar/tests/test_models.py | 99 +++++++++++++++++++ src/registrar/views/application.py | 6 +- src/registrar/views/utility/__init__.py | 1 + src/registrar/views/utility/mixins.py | 23 ++++- .../views/utility/permission_views.py | 18 +++- 9 files changed, 236 insertions(+), 7 deletions(-) create mode 100644 src/registrar/migrations/0029_user_status_alter_domainapplication_status.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 96b8aaa33..8426b7e40 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -98,6 +98,15 @@ class MyUserAdmin(BaseUserAdmin): """Custom user admin class to use our inlines.""" inlines = [UserContactInline] + + list_display = ("email", "first_name", "last_name", "is_staff", "is_superuser", "status") + + fieldsets = ( + (None, {'fields': ('username', 'password', 'status')}), # Add the 'status' field here + ('Personal Info', {'fields': ('first_name', 'last_name', 'email')}), + ('Permissions', {'fields': ('is_active', 'is_staff', 'is_superuser', 'groups', 'user_permissions')}), + ('Important dates', {'fields': ('last_login', 'date_joined')}), + ) def get_list_display(self, request): if not request.user.is_superuser: @@ -295,6 +304,9 @@ class DomainApplicationAdmin(ListHeaderAdmin): elif obj.status == models.DomainApplication.REJECTED: obj.status = original_obj.status obj.reject() + elif obj.status == models.DomainApplication.INELIGIBLE: + obj.status = original_obj.status + obj.reject_with_prejudice() else: logger.warning("Unknown status selected in django admin") diff --git a/src/registrar/migrations/0029_user_status_alter_domainapplication_status.py b/src/registrar/migrations/0029_user_status_alter_domainapplication_status.py new file mode 100644 index 000000000..504358665 --- /dev/null +++ b/src/registrar/migrations/0029_user_status_alter_domainapplication_status.py @@ -0,0 +1,42 @@ +# Generated by Django 4.2.1 on 2023-08-18 16:59 + +from django.db import migrations, models +import django_fsm + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0028_alter_domainapplication_status"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="status", + field=models.CharField( + blank=True, + choices=[("ineligible", "ineligible")], + default=None, + max_length=10, + null=True, + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="status", + field=django_fsm.FSMField( + choices=[ + ("started", "started"), + ("submitted", "submitted"), + ("in review", "in review"), + ("action needed", "action needed"), + ("approved", "approved"), + ("withdrawn", "withdrawn"), + ("rejected", "rejected"), + ("ineligible", "ineligible"), + ], + default="started", + max_length=50, + ), + ), + ] diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 67f1ee5d9..7a52d3185 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -26,6 +26,7 @@ class DomainApplication(TimeStampedModel): APPROVED = "approved" WITHDRAWN = "withdrawn" REJECTED = "rejected" + INELIGIBLE = "ineligible" STATUS_CHOICES = [ (STARTED, STARTED), (SUBMITTED, SUBMITTED), @@ -34,6 +35,7 @@ class DomainApplication(TimeStampedModel): (APPROVED, APPROVED), (WITHDRAWN, WITHDRAWN), (REJECTED, REJECTED), + (INELIGIBLE, INELIGIBLE) ] class StateTerritoryChoices(models.TextChoices): @@ -554,7 +556,7 @@ class DomainApplication(TimeStampedModel): ) @transition( - field="status", source=[SUBMITTED, IN_REVIEW, REJECTED], target=APPROVED + field="status", source=[SUBMITTED, IN_REVIEW, REJECTED, INELIGIBLE], target=APPROVED ) def approve(self): """Approve an application that has been submitted. @@ -602,6 +604,17 @@ class DomainApplication(TimeStampedModel): "emails/status_change_rejected.txt", "emails/status_change_rejected_subject.txt", ) + + @transition(field="status", source=[IN_REVIEW, APPROVED], target=INELIGIBLE) + def reject_with_prejudice(self): + """The applicant is a bad actor, reject with prejudice. + + No email As a side effect, but we block the applicant from editing + any existing domains and from submitting new apllications""" + + self.creator.block_user() + + # ## Form policies ### # diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 4cd8b6c90..b6402a2bf 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -16,6 +16,20 @@ class User(AbstractUser): A custom user model that performs identically to the default user model but can be customized later. """ + + # #### Constants for choice fields #### + INELIGIBLE = 'ineligible' + STATUS_CHOICES = ( + (INELIGIBLE, INELIGIBLE), + ) + + status = models.CharField( + max_length=10, + choices=STATUS_CHOICES, + default=None, # Set the default value to None + null=True, # Allow the field to be null + blank=True, # Allow the field to be blank + ) domains = models.ManyToManyField( "registrar.Domain", @@ -38,6 +52,19 @@ class User(AbstractUser): return self.email else: return self.username + + def block_user(self): + self.status = "ineligible" + self.save() + + def unblock_user(self): + self.status = None + self.save() + + def is_blocked(self): + if self.status == "ineligible": + return True + return False def first_login(self): """Callback when the user is authenticated for the very first time. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 997d5f4e2..8274908fa 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -170,6 +170,15 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.submit() + + def test_transition_not_allowed_ineligible_submitted(self): + """Create an application with status ineligible and call submit + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.submit() def test_transition_not_allowed_started_in_review(self): """Create an application with status started and call in_review @@ -224,6 +233,15 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.in_review() + + def test_transition_not_allowed_ineligible_in_review(self): + """Create an application with status ineligible and call in_review + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.in_review() def test_transition_not_allowed_started_action_needed(self): """Create an application with status started and call action_needed @@ -269,6 +287,15 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.action_needed() + + def test_transition_not_allowed_ineligible_action_needed(self): + """Create an application with status ineligible and call action_needed + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.action_needed() def test_transition_not_allowed_started_approved(self): """Create an application with status started and call approve @@ -350,6 +377,15 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.withdraw() + + def test_transition_not_allowed_ineligible_withdrawn(self): + """Create an application with status ineligible and call withdraw + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.withdraw() def test_transition_not_allowed_started_rejected(self): """Create an application with status started and call reject @@ -395,6 +431,69 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject() + + def test_transition_not_allowed_ineligible_rejected(self): + """Create an application with status ineligible and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_started_ineligible(self): + """Create an application with status started and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.STARTED) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_submitted_ineligible(self): + """Create an application with status submitted and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.SUBMITTED) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_action_needed_ineligible(self): + """Create an application with status action needed and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.ACTION_NEEDED) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_withdrawn_ineligible(self): + """Create an application with status withdrawn and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.WITHDRAWN) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_rejected_ineligible(self): + """Create an application with status rejected and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.REJECTED) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() + + def test_transition_not_allowed_ineligible_ineligible(self): + """Create an application with status ineligible and call reject + against transition rules""" + + application = completed_application(status=DomainApplication.INELIGIBLE) + + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() class TestPermissions(TestCase): diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index 256f4be40..fc9443847 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -12,7 +12,7 @@ from registrar.models import DomainApplication from registrar.utility import StrEnum from registrar.views.utility import StepsHelper -from .utility import DomainApplicationPermissionView +from .utility import DomainApplicationPermissionView, ApplicationWizardPermissionView logger = logging.getLogger(__name__) @@ -43,7 +43,7 @@ class Step(StrEnum): REVIEW = "review" -class ApplicationWizard(TemplateView): +class ApplicationWizard(ApplicationWizardPermissionView, TemplateView): """ A common set of methods and configuration. @@ -59,6 +59,8 @@ class ApplicationWizard(TemplateView): Any method not marked as internal can be overridden in a subclass, although not without consulting the base implementation, first. """ + + template_name = "" # uniquely namespace the wizard in urls.py # (this is not seen _in_ urls, only for Django's internal naming) diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 6c656c614..b782f59fd 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -5,4 +5,5 @@ from .permission_views import ( DomainPermissionView, DomainApplicationPermissionView, DomainInvitationPermissionDeleteView, + ApplicationWizardPermissionView ) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 05da75d35..715514bd8 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -39,9 +39,9 @@ class DomainPermission(PermissionsLoginMixin): ).exists(): return False - # ticket 796 - # if domain.application__status != 'approved' - # return false + # The user has an ineligible flag + if self.request.user.is_blocked(): + return False # if we need to check more about the nature of role, do it here. return True @@ -69,6 +69,23 @@ class DomainApplicationPermission(PermissionsLoginMixin): return False return True + + +class ApplicationWizardPermission(PermissionsLoginMixin): + + """Does the logged-in user have permission to start or edit an application?""" + + def has_permission(self): + """Check if this user has permission to start or edit an application. + + The user is in self.request.user + """ + + # The user has an ineligible flag + if self.request.user.is_blocked(): + return False + + return True class DomainInvitationPermission(PermissionsLoginMixin): diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index e52ed102c..d7b846377 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -2,7 +2,7 @@ import abc # abstract base class -from django.views.generic import DetailView, DeleteView +from django.views.generic import DetailView, DeleteView, TemplateView from registrar.models import Domain, DomainApplication, DomainInvitation @@ -10,6 +10,7 @@ from .mixins import ( DomainPermission, DomainApplicationPermission, DomainInvitationPermission, + ApplicationWizardPermission ) @@ -51,6 +52,21 @@ class DomainApplicationPermissionView(DomainApplicationPermission, DetailView, a @abc.abstractmethod def template_name(self): raise NotImplementedError + + +class ApplicationWizardPermissionView(ApplicationWizardPermission, TemplateView, abc.ABC): + + """Abstract base view for the application form that enforces permissions + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ + + # Abstract property enforces NotImplementedError on an attribute. + @property + @abc.abstractmethod + def template_name(self): + raise NotImplementedError class DomainInvitationPermissionDeleteView( From 96ea396da4f710c9eecc62f0b2d094f85069d764 Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 18 Aug 2023 18:52:47 -0400 Subject: [PATCH 2/7] Unit tests for transition sets user status to ineligible and deomain and application permissions --- src/registrar/models/domain_application.py | 4 +- src/registrar/tests/test_admin.py | 27 +++++++++++++ src/registrar/tests/test_views.py | 47 ++++++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 7a52d3185..e020676af 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -610,7 +610,9 @@ class DomainApplication(TimeStampedModel): """The applicant is a bad actor, reject with prejudice. No email As a side effect, but we block the applicant from editing - any existing domains and from submitting new apllications""" + any existing domains/applications and from submitting new aplications. + We do this by setting an ineligible status on the user, which the + permissions classes test against""" self.creator.block_user() diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 5f78eac3c..cc860e41c 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -274,6 +274,33 @@ class TestDomainApplicationAdmin(TestCase): # Perform assertions on the mock call itself mock_client_instance.send_email.assert_called_once() + + def test_save_model_sets_ineligible_status_on_user(self): + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + # Create a sample application + application = completed_application(status=DomainApplication.IN_REVIEW) + + # Create a mock request + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + + # Create an instance of the model admin + model_admin = DomainApplicationAdmin(DomainApplication, self.site) + + # Modify the application's property + application.status = DomainApplication.INELIGIBLE + + # Use the model admin's save_model method + model_admin.save_model(request, application, form=None, change=True) + + # Test that approved domain exists and equals requested domain + self.assertEqual( + application.creator.status, "ineligible" + ) def tearDown(self): DomainInformation.objects.all().delete() diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index feb553bf7..eccf769ea 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -100,6 +100,18 @@ class LoggedInTests(TestWithUser): response, "What kind of U.S.-based government organization do you represent?", ) + + def test_domain_application_form_with_ineligible_user(self): + """Application form not accessible for an ineligible user. + This test should be solid enough since all application wizard + views share the same permissions class""" + self.user.status = "ineligible" + self.user.save() + + with less_console_noise(): + response = self.client.get("/register/", follow=True) + print(response.status_code) + self.assertEqual(response.status_code, 403) class DomainApplicationTests(TestWithUser, WebTest): @@ -1422,6 +1434,20 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.assertContains( success_page, "The security email for this domain have been updated" ) + + def test_domain_overview_blocked_for_ineligible_user(self): + """We could easily duplicate this test for all domain management + views, but a single url test should be solid enough since all domain + management pages share the same permissions class""" + self.user.status = "ineligible" + self.user.save() + home_page = self.app.get("/") + self.assertContains(home_page, "igorville.gov") + with less_console_noise(): + response = self.client.get( + reverse("domain", kwargs={"pk": self.domain.id}) + ) + self.assertEqual(response.status_code, 403) class TestApplicationStatus(TestWithUser, WebTest): @@ -1446,6 +1472,27 @@ class TestApplicationStatus(TestWithUser, WebTest): self.assertContains(detail_page, "testy@town.com") self.assertContains(detail_page, "Admin Tester") self.assertContains(detail_page, "Status:") + + def test_application_status_with_ineligible_user(self): + """Checking application status page whith a blocked user. + The user should still have access to view.""" + self.user.status = "ineligible" + self.user.save() + + application = completed_application( + status=DomainApplication.SUBMITTED, user=self.user + ) + application.save() + + home_page = self.app.get("/") + self.assertContains(home_page, "city.gov") + # click the "Manage" link + detail_page = home_page.click("Manage") + self.assertContains(detail_page, "city.gov") + self.assertContains(detail_page, "Chief Tester") + self.assertContains(detail_page, "testy@town.com") + self.assertContains(detail_page, "Admin Tester") + self.assertContains(detail_page, "Status:") def test_application_withdraw(self): """Checking application status page""" From 4b82f5e131cdb885f5e61a2b9b78b27993703148 Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Thu, 24 Aug 2023 18:53:20 -0400 Subject: [PATCH 3/7] Add constraint on admin saving a domain application when its creator is ineligible, make all fields readonly when creator is ineligible --- src/registrar/admin.py | 138 +++++++++++++++++++++--------- src/registrar/tests/test_admin.py | 96 ++++++++++++++------- 2 files changed, 163 insertions(+), 71 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8426b7e40..ccdb7be3e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -4,11 +4,43 @@ from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.contenttypes.models import ContentType from django.http.response import HttpResponseRedirect from django.urls import reverse +# from django.forms.widgets import CheckboxSelectMultiple +# from django import forms from . import models logger = logging.getLogger(__name__) +# class TextDisplayWidget(forms.Widget): +# def render(self, name, value, attrs=None, renderer=None): +# if value: +# choices_dict = dict(self.choices) +# selected_values = set(value) +# display_values = [choices_dict[val] for val in selected_values] +# return ', '.join(display_values) +# return '' + + +# class DomainApplicationForm(forms.ModelForm): +# class Meta: +# model = models.DomainApplication +# fields = '__all__' + +# def __init__(self, *args, **kwargs): +# super().__init__(*args, **kwargs) + +# # Replace the current_websites field with text if a condition is met +# if self.instance and self.instance.creator.status == 'ineligible': +# self.fields['current_websites'].widget = TextDisplayWidget() +# self.fields['current_websites'].widget.choices = self.fields['current_websites'].choices + +# self.fields['other_contacts'].widget = TextDisplayWidget() +# self.fields['other_contacts'].widget.choices = self.fields['other_contacts'].choices + +# self.fields['alternative_domains'].widget = TextDisplayWidget() +# self.fields['alternative_domains'].widget.choices = self.fields['alternative_domains'].choices + + class AuditedAdmin(admin.ModelAdmin): """Custom admin to make auditing easier.""" @@ -181,6 +213,10 @@ class ContactAdmin(ListHeaderAdmin): class DomainApplicationAdmin(ListHeaderAdmin): """Customize the applications listing view.""" + + # Set multi-selects 'read-only' (hide selects and show data) + # based on user perms and application creator's status + #form = DomainApplicationForm # Columns list_display = [ @@ -255,7 +291,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): ] # Read only that we'll leverage for CISA Analysts - readonly_fields = [ + analyst_readonly_fields = [ "creator", "type_of_work", "more_organization_information", @@ -273,52 +309,72 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): - if change: # Check if the application is being edited - # Get the original application from the database - original_obj = models.DomainApplication.objects.get(pk=obj.pk) + if obj and obj.creator.status != "ineligible": + if change: # Check if the application is being edited + # Get the original application from the database + original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if obj.status != original_obj.status: - if obj.status == models.DomainApplication.STARTED: - # No conditions - pass - elif obj.status == models.DomainApplication.SUBMITTED: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. Same comment applies to - # transition method calls below. - obj.status = original_obj.status - obj.submit() - elif obj.status == models.DomainApplication.IN_REVIEW: - obj.status = original_obj.status - obj.in_review() - elif obj.status == models.DomainApplication.ACTION_NEEDED: - obj.status = original_obj.status - obj.action_needed() - elif obj.status == models.DomainApplication.APPROVED: - obj.status = original_obj.status - obj.approve() - elif obj.status == models.DomainApplication.WITHDRAWN: - obj.status = original_obj.status - obj.withdraw() - elif obj.status == models.DomainApplication.REJECTED: - obj.status = original_obj.status - obj.reject() - elif obj.status == models.DomainApplication.INELIGIBLE: - obj.status = original_obj.status - obj.reject_with_prejudice() - else: - logger.warning("Unknown status selected in django admin") + if obj.status != original_obj.status: + if obj.status == models.DomainApplication.STARTED: + # No conditions + pass + elif obj.status == models.DomainApplication.SUBMITTED: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. Same comment applies to + # transition method calls below. + obj.status = original_obj.status + obj.submit() + elif obj.status == models.DomainApplication.IN_REVIEW: + obj.status = original_obj.status + obj.in_review() + elif obj.status == models.DomainApplication.ACTION_NEEDED: + obj.status = original_obj.status + obj.action_needed() + elif obj.status == models.DomainApplication.APPROVED: + obj.status = original_obj.status + obj.approve() + elif obj.status == models.DomainApplication.WITHDRAWN: + obj.status = original_obj.status + obj.withdraw() + elif obj.status == models.DomainApplication.REJECTED: + obj.status = original_obj.status + obj.reject() + elif obj.status == models.DomainApplication.INELIGIBLE: + obj.status = original_obj.status + obj.reject_with_prejudice() + else: + logger.warning("Unknown status selected in django admin") - super().save_model(request, obj, form, change) + super().save_model(request, obj, form, change) + else: + messages.error(request, "This action is not permitted for applications with an ineligible creator.") def get_readonly_fields(self, request, obj=None): + + """Set the read-only state on form elements. + We have 2 conditions that determine which fields are read-only: + admin user permissions and the application creator's status, so + we'll use the baseline readonly_fields and extend it as needed. + """ + + readonly_fields = list(self.readonly_fields) + + # Check if the creator is ineligible + if obj and obj.creator.status == "ineligible": + # For fields like CharField, IntegerField, etc., the widget used is straightforward, + # and the readonly_fields list can control their behavior + readonly_fields.extend([field.name for field in self.model._meta.fields]) + # Add the multi-select fields to readonly_fields: + # Complex fields like ManyToManyField require special handling + readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) + if request.user.is_superuser: - # Superusers have full access, no fields are read-only - return [] + return readonly_fields else: - # Regular users can only view the specified fields - return self.readonly_fields + readonly_fields.extend([field for field in self.analyst_readonly_fields]) + return readonly_fields admin.site.register(models.User, MyUserAdmin) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index cc860e41c..6b6be1bd5 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -4,6 +4,7 @@ from registrar.admin import DomainApplicationAdmin, ListHeaderAdmin, MyUserAdmin from registrar.models import DomainApplication, DomainInformation, User from .common import completed_application, mock_user, create_superuser, create_user from django.contrib.auth import get_user_model +from unittest.mock import patch from django.conf import settings from unittest.mock import MagicMock @@ -14,6 +15,9 @@ class TestDomainApplicationAdmin(TestCase): def setUp(self): self.site = AdminSite() self.factory = RequestFactory() + self.admin = DomainApplicationAdmin(model=DomainApplication, admin_site=self.site) + self.superuser = create_superuser() + self.staffuser = create_user() @boto3_mocking.patching def test_save_model_sends_submitted_email(self): @@ -33,14 +37,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.SUBMITTED # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Access the arguments passed to send_email call_args = mock_client_instance.send_email.call_args @@ -79,14 +80,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.IN_REVIEW # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Access the arguments passed to send_email call_args = mock_client_instance.send_email.call_args @@ -125,14 +123,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.APPROVED # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Access the arguments passed to send_email call_args = mock_client_instance.send_email.call_args @@ -166,14 +161,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.APPROVED # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Test that approved domain exists and equals requested domain self.assertEqual( @@ -198,14 +190,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.ACTION_NEEDED # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Access the arguments passed to send_email call_args = mock_client_instance.send_email.call_args @@ -247,14 +236,11 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.REJECTED # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Access the arguments passed to send_email call_args = mock_client_instance.send_email.call_args @@ -288,19 +274,70 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) - # Create an instance of the model admin - model_admin = DomainApplicationAdmin(DomainApplication, self.site) - # Modify the application's property application.status = DomainApplication.INELIGIBLE # Use the model admin's save_model method - model_admin.save_model(request, application, form=None, change=True) + self.admin.save_model(request, application, form=None, change=True) # Test that approved domain exists and equals requested domain self.assertEqual( application.creator.status, "ineligible" ) + + def test_readonly_when_ineligible_creator(self): + application = completed_application(status=DomainApplication.IN_REVIEW) + application.creator.status = 'ineligible' + application.creator.save() + + request = self.factory.get('/') + request.user = self.superuser + + readonly_fields = self.admin.get_readonly_fields(request, application) + + expected_fields = ['id', 'created_at', 'updated_at', 'status', 'creator', 'investigator', 'organization_type', 'federally_recognized_tribe', 'state_recognized_tribe', 'tribe_name', 'federal_agency', 'federal_type', 'is_election_board', 'organization_name', 'address_line1', 'address_line2', 'city', 'state_territory', 'zipcode', 'urbanization', 'type_of_work', 'more_organization_information', 'authorizing_official', 'approved_domain', 'requested_domain', 'submitter', 'purpose', 'no_other_contacts_rationale', 'anything_else', 'is_policy_acknowledged', 'current_websites', 'other_contacts', 'alternative_domains'] + + self.assertEqual(readonly_fields, expected_fields) + + def test_readonly_fields_for_analyst(self): + request = self.factory.get('/') # Use the correct method and path + request.user = self.staffuser + + readonly_fields = self.admin.get_readonly_fields(request) + + expected_fields = ['creator', 'type_of_work', 'more_organization_information', 'address_line1', 'address_line2', 'zipcode', 'requested_domain', 'alternative_domains', 'purpose', 'submitter', 'no_other_contacts_rationale', 'anything_else', 'is_policy_acknowledged'] + + self.assertEqual(readonly_fields, expected_fields) + + def test_readonly_fields_for_superuser(self): + request = self.factory.get('/') # Use the correct method and path + request.user = self.superuser + + readonly_fields = self.admin.get_readonly_fields(request) + + expected_fields = [] + + self.assertEqual(readonly_fields, expected_fields) + + def test_saving_when_ineligible_creator(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.IN_REVIEW) + application.creator.status = 'ineligible' + application.creator.save() + + # Create a request object with a superuser + request = self.factory.get('/') + request.user = self.superuser + + with patch('django.contrib.messages.error') as mock_error: + # Simulate saving the model + self.admin.save_model(request, application, None, False) + + # Assert that the error message was called with the correct argument + mock_error.assert_called_once_with(request, "This action is not permitted for applications with an ineligible creator.") + + # Assert that the status has not changed + self.assertEqual(application.status, DomainApplication.IN_REVIEW) def tearDown(self): DomainInformation.objects.all().delete() @@ -381,8 +418,7 @@ class ListHeaderAdminTest(TestCase): DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() User.objects.all().delete() - self.superuser.delete() - + class MyUserAdminTest(TestCase): def setUp(self): From 91d1b9c1cc4cc3683d715d9af8d79b5012c15f2c Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Thu, 24 Aug 2023 19:09:57 -0400 Subject: [PATCH 4/7] Lint --- src/registrar/admin.py | 175 ++++++++++-------- src/registrar/models/domain_application.py | 12 +- src/registrar/models/user.py | 20 +- src/registrar/tests/test_admin.py | 118 ++++++++---- src/registrar/tests/test_models.py | 14 +- src/registrar/tests/test_views.py | 16 +- src/registrar/views/application.py | 2 +- src/registrar/views/utility/__init__.py | 2 +- src/registrar/views/utility/mixins.py | 4 +- .../views/utility/permission_views.py | 10 +- 10 files changed, 218 insertions(+), 155 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ccdb7be3e..eeb9a9375 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -4,43 +4,12 @@ from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.contenttypes.models import ContentType from django.http.response import HttpResponseRedirect from django.urls import reverse -# from django.forms.widgets import CheckboxSelectMultiple -# from django import forms + from . import models logger = logging.getLogger(__name__) -# class TextDisplayWidget(forms.Widget): -# def render(self, name, value, attrs=None, renderer=None): -# if value: -# choices_dict = dict(self.choices) -# selected_values = set(value) -# display_values = [choices_dict[val] for val in selected_values] -# return ', '.join(display_values) -# return '' - - -# class DomainApplicationForm(forms.ModelForm): -# class Meta: -# model = models.DomainApplication -# fields = '__all__' - -# def __init__(self, *args, **kwargs): -# super().__init__(*args, **kwargs) - -# # Replace the current_websites field with text if a condition is met -# if self.instance and self.instance.creator.status == 'ineligible': -# self.fields['current_websites'].widget = TextDisplayWidget() -# self.fields['current_websites'].widget.choices = self.fields['current_websites'].choices - -# self.fields['other_contacts'].widget = TextDisplayWidget() -# self.fields['other_contacts'].widget.choices = self.fields['other_contacts'].choices - -# self.fields['alternative_domains'].widget = TextDisplayWidget() -# self.fields['alternative_domains'].widget.choices = self.fields['alternative_domains'].choices - - class AuditedAdmin(admin.ModelAdmin): """Custom admin to make auditing easier.""" @@ -130,14 +99,35 @@ class MyUserAdmin(BaseUserAdmin): """Custom user admin class to use our inlines.""" inlines = [UserContactInline] - - list_display = ("email", "first_name", "last_name", "is_staff", "is_superuser", "status") - + + list_display = ( + "email", + "first_name", + "last_name", + "is_staff", + "is_superuser", + "status", + ) + fieldsets = ( - (None, {'fields': ('username', 'password', 'status')}), # Add the 'status' field here - ('Personal Info', {'fields': ('first_name', 'last_name', 'email')}), - ('Permissions', {'fields': ('is_active', 'is_staff', 'is_superuser', 'groups', 'user_permissions')}), - ('Important dates', {'fields': ('last_login', 'date_joined')}), + ( + None, + {"fields": ("username", "password", "status")}, + ), # Add the 'status' field here + ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ( + "Permissions", + { + "fields": ( + "is_active", + "is_staff", + "is_superuser", + "groups", + "user_permissions", + ) + }, + ), + ("Important dates", {"fields": ("last_login", "date_joined")}), ) def get_list_display(self, request): @@ -213,10 +203,10 @@ class ContactAdmin(ListHeaderAdmin): class DomainApplicationAdmin(ListHeaderAdmin): """Customize the applications listing view.""" - + # Set multi-selects 'read-only' (hide selects and show data) # based on user perms and application creator's status - #form = DomainApplicationForm + # form = DomainApplicationForm # Columns list_display = [ @@ -314,62 +304,85 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) + # if obj.status != original_obj.status: + # if obj.status == models.DomainApplication.STARTED: + # # No conditions + # pass + # elif obj.status == models.DomainApplication.SUBMITTED: + # # This is an fsm in model which will throw an error if the + # # transition condition is violated, so we roll back the + # # status to what it was before the admin user changed it and + # # let the fsm method set it. Same comment applies to + # # transition method calls below. + # obj.status = original_obj.status + # obj.submit() + # elif obj.status == models.DomainApplication.IN_REVIEW: + # obj.status = original_obj.status + # obj.in_review() + # elif obj.status == models.DomainApplication.ACTION_NEEDED: + # obj.status = original_obj.status + # obj.action_needed() + # elif obj.status == models.DomainApplication.APPROVED: + # obj.status = original_obj.status + # obj.approve() + # elif obj.status == models.DomainApplication.WITHDRAWN: + # obj.status = original_obj.status + # obj.withdraw() + # elif obj.status == models.DomainApplication.REJECTED: + # obj.status = original_obj.status + # obj.reject() + # elif obj.status == models.DomainApplication.INELIGIBLE: + # obj.status = original_obj.status + # obj.reject_with_prejudice() + # else: + # logger.warning("Unknown status selected in django admin") + if obj.status != original_obj.status: - if obj.status == models.DomainApplication.STARTED: - # No conditions - pass - elif obj.status == models.DomainApplication.SUBMITTED: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. Same comment applies to - # transition method calls below. - obj.status = original_obj.status - obj.submit() - elif obj.status == models.DomainApplication.IN_REVIEW: - obj.status = original_obj.status - obj.in_review() - elif obj.status == models.DomainApplication.ACTION_NEEDED: - obj.status = original_obj.status - obj.action_needed() - elif obj.status == models.DomainApplication.APPROVED: - obj.status = original_obj.status - obj.approve() - elif obj.status == models.DomainApplication.WITHDRAWN: - obj.status = original_obj.status - obj.withdraw() - elif obj.status == models.DomainApplication.REJECTED: - obj.status = original_obj.status - obj.reject() - elif obj.status == models.DomainApplication.INELIGIBLE: - obj.status = original_obj.status - obj.reject_with_prejudice() - else: + status_method_mapping = { + models.DomainApplication.STARTED: None, + models.DomainApplication.SUBMITTED: obj.submit, + models.DomainApplication.IN_REVIEW: obj.in_review, + models.DomainApplication.ACTION_NEEDED: obj.action_needed, + models.DomainApplication.APPROVED: obj.approve, + models.DomainApplication.WITHDRAWN: obj.withdraw, + models.DomainApplication.REJECTED: obj.reject, + models.DomainApplication.INELIGIBLE: obj.reject_with_prejudice, + } + selected_method = status_method_mapping.get(obj.status) + if selected_method is None: logger.warning("Unknown status selected in django admin") + else: + obj.status = original_obj.status + selected_method() super().save_model(request, obj, form, change) - else: - messages.error(request, "This action is not permitted for applications with an ineligible creator.") + else: + messages.error( + request, + "This action is not permitted for applications " + + "with an ineligible creator.", + ) def get_readonly_fields(self, request, obj=None): - - """Set the read-only state on form elements. + """Set the read-only state on form elements. We have 2 conditions that determine which fields are read-only: admin user permissions and the application creator's status, so we'll use the baseline readonly_fields and extend it as needed. """ - + readonly_fields = list(self.readonly_fields) - + # Check if the creator is ineligible if obj and obj.creator.status == "ineligible": - # For fields like CharField, IntegerField, etc., the widget used is straightforward, - # and the readonly_fields list can control their behavior + # For fields like CharField, IntegerField, etc., the widget used is + # straightforward and the readonly_fields list can control their behavior readonly_fields.extend([field.name for field in self.model._meta.fields]) # Add the multi-select fields to readonly_fields: # Complex fields like ManyToManyField require special handling - readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) - + readonly_fields.extend( + ["current_websites", "other_contacts", "alternative_domains"] + ) + if request.user.is_superuser: return readonly_fields else: diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index e020676af..7d017ab38 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -35,7 +35,7 @@ class DomainApplication(TimeStampedModel): (APPROVED, APPROVED), (WITHDRAWN, WITHDRAWN), (REJECTED, REJECTED), - (INELIGIBLE, INELIGIBLE) + (INELIGIBLE, INELIGIBLE), ] class StateTerritoryChoices(models.TextChoices): @@ -556,7 +556,9 @@ class DomainApplication(TimeStampedModel): ) @transition( - field="status", source=[SUBMITTED, IN_REVIEW, REJECTED, INELIGIBLE], target=APPROVED + field="status", + source=[SUBMITTED, IN_REVIEW, REJECTED, INELIGIBLE], + target=APPROVED, ) def approve(self): """Approve an application that has been submitted. @@ -604,7 +606,7 @@ class DomainApplication(TimeStampedModel): "emails/status_change_rejected.txt", "emails/status_change_rejected_subject.txt", ) - + @transition(field="status", source=[IN_REVIEW, APPROVED], target=INELIGIBLE) def reject_with_prejudice(self): """The applicant is a bad actor, reject with prejudice. @@ -613,11 +615,9 @@ class DomainApplication(TimeStampedModel): any existing domains/applications and from submitting new aplications. We do this by setting an ineligible status on the user, which the permissions classes test against""" - + self.creator.block_user() - - # ## Form policies ### # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b6402a2bf..9b3ba9162 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -16,19 +16,17 @@ class User(AbstractUser): A custom user model that performs identically to the default user model but can be customized later. """ - + # #### Constants for choice fields #### - INELIGIBLE = 'ineligible' - STATUS_CHOICES = ( - (INELIGIBLE, INELIGIBLE), - ) - + INELIGIBLE = "ineligible" + STATUS_CHOICES = ((INELIGIBLE, INELIGIBLE),) + status = models.CharField( max_length=10, choices=STATUS_CHOICES, default=None, # Set the default value to None - null=True, # Allow the field to be null - blank=True, # Allow the field to be blank + null=True, # Allow the field to be null + blank=True, # Allow the field to be blank ) domains = models.ManyToManyField( @@ -52,15 +50,15 @@ class User(AbstractUser): return self.email else: return self.username - + def block_user(self): self.status = "ineligible" self.save() - + def unblock_user(self): self.status = None self.save() - + def is_blocked(self): if self.status == "ineligible": return True diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 6b6be1bd5..7da38cb8a 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -15,7 +15,9 @@ class TestDomainApplicationAdmin(TestCase): def setUp(self): self.site = AdminSite() self.factory = RequestFactory() - self.admin = DomainApplicationAdmin(model=DomainApplication, admin_site=self.site) + self.admin = DomainApplicationAdmin( + model=DomainApplication, admin_site=self.site + ) self.superuser = create_superuser() self.staffuser = create_user() @@ -260,7 +262,7 @@ class TestDomainApplicationAdmin(TestCase): # Perform assertions on the mock call itself mock_client_instance.send_email.assert_called_once() - + def test_save_model_sets_ineligible_status_on_user(self): # make sure there is no user with this email EMAIL = "mayor@igorville.gov" @@ -281,60 +283,110 @@ class TestDomainApplicationAdmin(TestCase): self.admin.save_model(request, application, form=None, change=True) # Test that approved domain exists and equals requested domain - self.assertEqual( - application.creator.status, "ineligible" - ) - + self.assertEqual(application.creator.status, "ineligible") + def test_readonly_when_ineligible_creator(self): application = completed_application(status=DomainApplication.IN_REVIEW) - application.creator.status = 'ineligible' + application.creator.status = "ineligible" application.creator.save() - - request = self.factory.get('/') + + request = self.factory.get("/") request.user = self.superuser - + readonly_fields = self.admin.get_readonly_fields(request, application) - - expected_fields = ['id', 'created_at', 'updated_at', 'status', 'creator', 'investigator', 'organization_type', 'federally_recognized_tribe', 'state_recognized_tribe', 'tribe_name', 'federal_agency', 'federal_type', 'is_election_board', 'organization_name', 'address_line1', 'address_line2', 'city', 'state_territory', 'zipcode', 'urbanization', 'type_of_work', 'more_organization_information', 'authorizing_official', 'approved_domain', 'requested_domain', 'submitter', 'purpose', 'no_other_contacts_rationale', 'anything_else', 'is_policy_acknowledged', 'current_websites', 'other_contacts', 'alternative_domains'] - + + expected_fields = [ + "id", + "created_at", + "updated_at", + "status", + "creator", + "investigator", + "organization_type", + "federally_recognized_tribe", + "state_recognized_tribe", + "tribe_name", + "federal_agency", + "federal_type", + "is_election_board", + "organization_name", + "address_line1", + "address_line2", + "city", + "state_territory", + "zipcode", + "urbanization", + "type_of_work", + "more_organization_information", + "authorizing_official", + "approved_domain", + "requested_domain", + "submitter", + "purpose", + "no_other_contacts_rationale", + "anything_else", + "is_policy_acknowledged", + "current_websites", + "other_contacts", + "alternative_domains", + ] + self.assertEqual(readonly_fields, expected_fields) - + def test_readonly_fields_for_analyst(self): - request = self.factory.get('/') # Use the correct method and path + request = self.factory.get("/") # Use the correct method and path request.user = self.staffuser - + readonly_fields = self.admin.get_readonly_fields(request) - - expected_fields = ['creator', 'type_of_work', 'more_organization_information', 'address_line1', 'address_line2', 'zipcode', 'requested_domain', 'alternative_domains', 'purpose', 'submitter', 'no_other_contacts_rationale', 'anything_else', 'is_policy_acknowledged'] - + + expected_fields = [ + "creator", + "type_of_work", + "more_organization_information", + "address_line1", + "address_line2", + "zipcode", + "requested_domain", + "alternative_domains", + "purpose", + "submitter", + "no_other_contacts_rationale", + "anything_else", + "is_policy_acknowledged", + ] + self.assertEqual(readonly_fields, expected_fields) - + def test_readonly_fields_for_superuser(self): - request = self.factory.get('/') # Use the correct method and path + request = self.factory.get("/") # Use the correct method and path request.user = self.superuser - + readonly_fields = self.admin.get_readonly_fields(request) - + expected_fields = [] - + self.assertEqual(readonly_fields, expected_fields) - + def test_saving_when_ineligible_creator(self): # Create an instance of the model application = completed_application(status=DomainApplication.IN_REVIEW) - application.creator.status = 'ineligible' + application.creator.status = "ineligible" application.creator.save() - + # Create a request object with a superuser - request = self.factory.get('/') + request = self.factory.get("/") request.user = self.superuser - - with patch('django.contrib.messages.error') as mock_error: + + with patch("django.contrib.messages.error") as mock_error: # Simulate saving the model self.admin.save_model(request, application, None, False) - + # Assert that the error message was called with the correct argument - mock_error.assert_called_once_with(request, "This action is not permitted for applications with an ineligible creator.") + mock_error.assert_called_once_with( + request, + "This action is not permitted for applications with " + + "an ineligible creator.", + ) # Assert that the status has not changed self.assertEqual(application.status, DomainApplication.IN_REVIEW) @@ -418,7 +470,7 @@ class ListHeaderAdminTest(TestCase): DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() User.objects.all().delete() - + class MyUserAdminTest(TestCase): def setUp(self): diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8274908fa..ca1191061 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -170,7 +170,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.submit() - + def test_transition_not_allowed_ineligible_submitted(self): """Create an application with status ineligible and call submit against transition rules""" @@ -233,7 +233,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.in_review() - + def test_transition_not_allowed_ineligible_in_review(self): """Create an application with status ineligible and call in_review against transition rules""" @@ -287,7 +287,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.action_needed() - + def test_transition_not_allowed_ineligible_action_needed(self): """Create an application with status ineligible and call action_needed against transition rules""" @@ -377,7 +377,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.withdraw() - + def test_transition_not_allowed_ineligible_withdrawn(self): """Create an application with status ineligible and call withdraw against transition rules""" @@ -431,7 +431,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject() - + def test_transition_not_allowed_ineligible_rejected(self): """Create an application with status ineligible and call reject against transition rules""" @@ -440,7 +440,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject_with_prejudice() - + def test_transition_not_allowed_started_ineligible(self): """Create an application with status started and call reject against transition rules""" @@ -485,7 +485,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject_with_prejudice() - + def test_transition_not_allowed_ineligible_ineligible(self): """Create an application with status ineligible and call reject against transition rules""" diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index eccf769ea..0d1239cf3 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -100,14 +100,14 @@ class LoggedInTests(TestWithUser): response, "What kind of U.S.-based government organization do you represent?", ) - + def test_domain_application_form_with_ineligible_user(self): """Application form not accessible for an ineligible user. This test should be solid enough since all application wizard views share the same permissions class""" self.user.status = "ineligible" self.user.save() - + with less_console_noise(): response = self.client.get("/register/", follow=True) print(response.status_code) @@ -1434,7 +1434,7 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.assertContains( success_page, "The security email for this domain have been updated" ) - + def test_domain_overview_blocked_for_ineligible_user(self): """We could easily duplicate this test for all domain management views, but a single url test should be solid enough since all domain @@ -1443,10 +1443,8 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): self.user.save() home_page = self.app.get("/") self.assertContains(home_page, "igorville.gov") - with less_console_noise(): - response = self.client.get( - reverse("domain", kwargs={"pk": self.domain.id}) - ) + with less_console_noise(): + response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id})) self.assertEqual(response.status_code, 403) @@ -1472,13 +1470,13 @@ class TestApplicationStatus(TestWithUser, WebTest): self.assertContains(detail_page, "testy@town.com") self.assertContains(detail_page, "Admin Tester") self.assertContains(detail_page, "Status:") - + def test_application_status_with_ineligible_user(self): """Checking application status page whith a blocked user. The user should still have access to view.""" self.user.status = "ineligible" self.user.save() - + application = completed_application( status=DomainApplication.SUBMITTED, user=self.user ) diff --git a/src/registrar/views/application.py b/src/registrar/views/application.py index fc9443847..23d7348e9 100644 --- a/src/registrar/views/application.py +++ b/src/registrar/views/application.py @@ -59,7 +59,7 @@ class ApplicationWizard(ApplicationWizardPermissionView, TemplateView): Any method not marked as internal can be overridden in a subclass, although not without consulting the base implementation, first. """ - + template_name = "" # uniquely namespace the wizard in urls.py diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index b782f59fd..71d3edb91 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -5,5 +5,5 @@ from .permission_views import ( DomainPermissionView, DomainApplicationPermissionView, DomainInvitationPermissionDeleteView, - ApplicationWizardPermissionView + ApplicationWizardPermissionView, ) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 715514bd8..e85348d6c 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -69,7 +69,7 @@ class DomainApplicationPermission(PermissionsLoginMixin): return False return True - + class ApplicationWizardPermission(PermissionsLoginMixin): @@ -80,7 +80,7 @@ class ApplicationWizardPermission(PermissionsLoginMixin): The user is in self.request.user """ - + # The user has an ineligible flag if self.request.user.is_blocked(): return False diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index d7b846377..0ef4ff4e5 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -10,7 +10,7 @@ from .mixins import ( DomainPermission, DomainApplicationPermission, DomainInvitationPermission, - ApplicationWizardPermission + ApplicationWizardPermission, ) @@ -52,9 +52,11 @@ class DomainApplicationPermissionView(DomainApplicationPermission, DetailView, a @abc.abstractmethod def template_name(self): raise NotImplementedError - - -class ApplicationWizardPermissionView(ApplicationWizardPermission, TemplateView, abc.ABC): + + +class ApplicationWizardPermissionView( + ApplicationWizardPermission, TemplateView, abc.ABC +): """Abstract base view for the application form that enforces permissions From 6c308332b06d9e403d3d5347d204b61f57fab62b Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 25 Aug 2023 12:13:41 -0400 Subject: [PATCH 5/7] Add more controls and messaging, unit tests --- src/registrar/admin.py | 52 +++++++++++-------------------- src/registrar/tests/test_admin.py | 21 +++++++++++++ 2 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index eeb9a9375..4063efb5f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -304,39 +304,6 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) - # if obj.status != original_obj.status: - # if obj.status == models.DomainApplication.STARTED: - # # No conditions - # pass - # elif obj.status == models.DomainApplication.SUBMITTED: - # # This is an fsm in model which will throw an error if the - # # transition condition is violated, so we roll back the - # # status to what it was before the admin user changed it and - # # let the fsm method set it. Same comment applies to - # # transition method calls below. - # obj.status = original_obj.status - # obj.submit() - # elif obj.status == models.DomainApplication.IN_REVIEW: - # obj.status = original_obj.status - # obj.in_review() - # elif obj.status == models.DomainApplication.ACTION_NEEDED: - # obj.status = original_obj.status - # obj.action_needed() - # elif obj.status == models.DomainApplication.APPROVED: - # obj.status = original_obj.status - # obj.approve() - # elif obj.status == models.DomainApplication.WITHDRAWN: - # obj.status = original_obj.status - # obj.withdraw() - # elif obj.status == models.DomainApplication.REJECTED: - # obj.status = original_obj.status - # obj.reject() - # elif obj.status == models.DomainApplication.INELIGIBLE: - # obj.status = original_obj.status - # obj.reject_with_prejudice() - # else: - # logger.warning("Unknown status selected in django admin") - if obj.status != original_obj.status: status_method_mapping = { models.DomainApplication.STARTED: None, @@ -352,11 +319,18 @@ class DomainApplicationAdmin(ListHeaderAdmin): if selected_method is None: logger.warning("Unknown status selected in django admin") else: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. obj.status = original_obj.status selected_method() super().save_model(request, obj, form, change) else: + # Clear the success message + messages.set_level(request, messages.ERROR) + messages.error( request, "This action is not permitted for applications " @@ -389,6 +363,18 @@ class DomainApplicationAdmin(ListHeaderAdmin): readonly_fields.extend([field for field in self.analyst_readonly_fields]) return readonly_fields + def display_ineligible_warning(self, request, obj): + if obj and obj.creator.status == "ineligible": + messages.warning( + request, + "Cannot edit an application when its creator has a status of ineligible.", + ) + + def change_view(self, request, object_id, form_url="", extra_context=None): + obj = self.get_object(request, object_id) + self.display_ineligible_warning(request, obj) + return super().change_view(request, object_id, form_url, extra_context) + admin.site.register(models.User, MyUserAdmin) admin.site.register(models.UserDomainRole, AuditedAdmin) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 7da38cb8a..79d0501be 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -391,6 +391,27 @@ class TestDomainApplicationAdmin(TestCase): # Assert that the status has not changed self.assertEqual(application.status, DomainApplication.IN_REVIEW) + def test_change_view_with_ineligible_creator(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.IN_REVIEW) + application.creator.status = "ineligible" + application.creator.save() + + with patch("django.contrib.messages.warning") as mock_warning: + # Create a request object with a superuser + request = self.factory.get( + "/admin/your_app/domainapplication/{}/change/".format(application.pk) + ) + request.user = self.superuser + + self.admin.display_ineligible_warning(request, application) + + # Assert that the error message was called with the correct argument + mock_warning.assert_called_once_with( + request, + "Cannot edit an application when its creator has a status of ineligible.", + ) + def tearDown(self): DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() From a676da486ac186f40ff8c8feeef84d09af18297b Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 25 Aug 2023 14:26:50 -0400 Subject: [PATCH 6/7] lint --- src/registrar/admin.py | 3 ++- src/registrar/tests/test_admin.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4063efb5f..2d42fec07 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -367,7 +367,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): if obj and obj.creator.status == "ineligible": messages.warning( request, - "Cannot edit an application when its creator has a status of ineligible.", + "Cannot edit an application when its creator " + + "has a status of ineligible.", ) def change_view(self, request, object_id, form_url="", extra_context=None): diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 79d0501be..0ae1c660f 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -409,7 +409,8 @@ class TestDomainApplicationAdmin(TestCase): # Assert that the error message was called with the correct argument mock_warning.assert_called_once_with( request, - "Cannot edit an application when its creator has a status of ineligible.", + "Cannot edit an application when its creator " + + "has a status of ineligible.", ) def tearDown(self): From 501cddebde7172cdf91ae05c458cf571df733f9e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 29 Aug 2023 14:16:39 -0400 Subject: [PATCH 7/7] Change ineligible to restricted on User, fix status tempplate for DAs --- src/registrar/admin.py | 19 +++++++------- .../migrations/0030_alter_user_status.py | 23 +++++++++++++++++ src/registrar/models/domain_application.py | 2 +- src/registrar/models/user.py | 16 ++++++------ .../templates/application_status.html | 1 + src/registrar/tests/test_admin.py | 25 +++++++++---------- src/registrar/tests/test_views.py | 4 +-- src/registrar/views/utility/mixins.py | 4 +-- 8 files changed, 57 insertions(+), 37 deletions(-) create mode 100644 src/registrar/migrations/0030_alter_user_status.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a832d34bd..4696a15bf 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -116,7 +116,7 @@ class MyUserAdmin(BaseUserAdmin): ( None, {"fields": ("username", "password", "status")}, - ), # Add the 'status' field here + ), ("Personal Info", {"fields": ("first_name", "last_name", "email")}), ( "Permissions", @@ -301,7 +301,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): - if obj and obj.creator.status != "ineligible": + if obj and obj.creator.status != models.User.RESTRICTED: if change: # Check if the application is being edited # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) @@ -336,7 +336,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): messages.error( request, "This action is not permitted for applications " - + "with an ineligible creator.", + + "with a restricted creator.", ) def get_readonly_fields(self, request, obj=None): @@ -348,8 +348,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): readonly_fields = list(self.readonly_fields) - # Check if the creator is ineligible - if obj and obj.creator.status == "ineligible": + # Check if the creator is restricted + if obj and obj.creator.status == models.User.RESTRICTED: # For fields like CharField, IntegerField, etc., the widget used is # straightforward and the readonly_fields list can control their behavior readonly_fields.extend([field.name for field in self.model._meta.fields]) @@ -365,17 +365,16 @@ class DomainApplicationAdmin(ListHeaderAdmin): readonly_fields.extend([field for field in self.analyst_readonly_fields]) return readonly_fields - def display_ineligible_warning(self, request, obj): - if obj and obj.creator.status == "ineligible": + def display_restricted_warning(self, request, obj): + if obj and obj.creator.status == models.User.RESTRICTED: messages.warning( request, - "Cannot edit an application when its creator " - + "has a status of ineligible.", + "Cannot edit an application with a restricted creator.", ) def change_view(self, request, object_id, form_url="", extra_context=None): obj = self.get_object(request, object_id) - self.display_ineligible_warning(request, obj) + self.display_restricted_warning(request, obj) return super().change_view(request, object_id, form_url, extra_context) diff --git a/src/registrar/migrations/0030_alter_user_status.py b/src/registrar/migrations/0030_alter_user_status.py new file mode 100644 index 000000000..7dd27bfa4 --- /dev/null +++ b/src/registrar/migrations/0030_alter_user_status.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.1 on 2023-08-29 17:09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0029_user_status_alter_domainapplication_status"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="status", + field=models.CharField( + blank=True, + choices=[("restricted", "restricted")], + default=None, + max_length=10, + null=True, + ), + ), + ] diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index c06985079..b1230b703 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -621,7 +621,7 @@ class DomainApplication(TimeStampedModel): We do this by setting an ineligible status on the user, which the permissions classes test against""" - self.creator.block_user() + self.creator.restrict_user() # ## Form policies ### # diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 9b3ba9162..5cf1dd71f 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -18,8 +18,8 @@ class User(AbstractUser): """ # #### Constants for choice fields #### - INELIGIBLE = "ineligible" - STATUS_CHOICES = ((INELIGIBLE, INELIGIBLE),) + RESTRICTED = "restricted" + STATUS_CHOICES = ((RESTRICTED, RESTRICTED),) status = models.CharField( max_length=10, @@ -51,18 +51,16 @@ class User(AbstractUser): else: return self.username - def block_user(self): - self.status = "ineligible" + def restrict_user(self): + self.status = self.RESTRICTED self.save() - def unblock_user(self): + def unrestrict_user(self): self.status = None self.save() - def is_blocked(self): - if self.status == "ineligible": - return True - return False + def is_restricted(self): + return self.status == self.RESTRICTED def first_login(self): """Callback when the user is authenticated for the very first time. diff --git a/src/registrar/templates/application_status.html b/src/registrar/templates/application_status.html index 99f6a1d4c..c95f6a98d 100644 --- a/src/registrar/templates/application_status.html +++ b/src/registrar/templates/application_status.html @@ -23,6 +23,7 @@ {% elif domainapplication.status == 'in review' %} In Review {% elif domainapplication.status == 'rejected' %} Rejected {% elif domainapplication.status == 'submitted' %} Submitted + {% elif domainapplication.status == 'ineligible' %} Ineligible {% else %}ERROR Please contact technical support/dev {% endif %}

diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 4a38d3576..fc5478dd9 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -284,7 +284,7 @@ class TestDomainApplicationAdmin(TestCase): # Perform assertions on the mock call itself mock_client_instance.send_email.assert_called_once() - def test_save_model_sets_ineligible_status_on_user(self): + def test_save_model_sets_restricted_status_on_user(self): # make sure there is no user with this email EMAIL = "mayor@igorville.gov" User.objects.filter(email=EMAIL).delete() @@ -304,11 +304,11 @@ class TestDomainApplicationAdmin(TestCase): self.admin.save_model(request, application, form=None, change=True) # Test that approved domain exists and equals requested domain - self.assertEqual(application.creator.status, "ineligible") + self.assertEqual(application.creator.status, "restricted") - def test_readonly_when_ineligible_creator(self): + def test_readonly_when_restricted_creator(self): application = completed_application(status=DomainApplication.IN_REVIEW) - application.creator.status = "ineligible" + application.creator.status = User.RESTRICTED application.creator.save() request = self.factory.get("/") @@ -388,10 +388,10 @@ class TestDomainApplicationAdmin(TestCase): self.assertEqual(readonly_fields, expected_fields) - def test_saving_when_ineligible_creator(self): + def test_saving_when_restricted_creator(self): # Create an instance of the model application = completed_application(status=DomainApplication.IN_REVIEW) - application.creator.status = "ineligible" + application.creator.status = User.RESTRICTED application.creator.save() # Create a request object with a superuser @@ -405,17 +405,17 @@ class TestDomainApplicationAdmin(TestCase): # Assert that the error message was called with the correct argument mock_error.assert_called_once_with( request, - "This action is not permitted for applications with " - + "an ineligible creator.", + "This action is not permitted for applications " + + "with a restricted creator.", ) # Assert that the status has not changed self.assertEqual(application.status, DomainApplication.IN_REVIEW) - def test_change_view_with_ineligible_creator(self): + def test_change_view_with_restricted_creator(self): # Create an instance of the model application = completed_application(status=DomainApplication.IN_REVIEW) - application.creator.status = "ineligible" + application.creator.status = User.RESTRICTED application.creator.save() with patch("django.contrib.messages.warning") as mock_warning: @@ -425,13 +425,12 @@ class TestDomainApplicationAdmin(TestCase): ) request.user = self.superuser - self.admin.display_ineligible_warning(request, application) + self.admin.display_restricted_warning(request, application) # Assert that the error message was called with the correct argument mock_warning.assert_called_once_with( request, - "Cannot edit an application when its creator " - + "has a status of ineligible.", + "Cannot edit an application with a restricted creator.", ) def tearDown(self): diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 0d1239cf3..017cb4d8d 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -105,7 +105,7 @@ class LoggedInTests(TestWithUser): """Application form not accessible for an ineligible user. This test should be solid enough since all application wizard views share the same permissions class""" - self.user.status = "ineligible" + self.user.status = User.RESTRICTED self.user.save() with less_console_noise(): @@ -1439,7 +1439,7 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest): """We could easily duplicate this test for all domain management views, but a single url test should be solid enough since all domain management pages share the same permissions class""" - self.user.status = "ineligible" + self.user.status = User.RESTRICTED self.user.save() home_page = self.app.get("/") self.assertContains(home_page, "igorville.gov") diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index e85348d6c..363709a21 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -40,7 +40,7 @@ class DomainPermission(PermissionsLoginMixin): return False # The user has an ineligible flag - if self.request.user.is_blocked(): + if self.request.user.is_restricted(): return False # if we need to check more about the nature of role, do it here. @@ -82,7 +82,7 @@ class ApplicationWizardPermission(PermissionsLoginMixin): """ # The user has an ineligible flag - if self.request.user.is_blocked(): + if self.request.user.is_restricted(): return False return True