Review feedback: FSM unit tests, indexes, comments, etc.

docs/architecture/decisions/0015-use-django-fsm.md

@@ -0,0 +1,42 @@
+# 15. Use Django-FSM library for domain application state
+
+Date: 2022-11-03
+
+## Status
+
+Accepted
+
+## Context
+
+The applications that registrants submit for domains move through a variety of
+different states or stages as they are processed by CISA staff.  Traditionally,
+there would be a “domain application” data model with a “status” field. The
+rules in the application code that control what changes are permitted to the
+statuses are called “domain logic”.
+
+In a large piece of software, domain logic often spreads around the code base
+because while handling a single request like “mark this application as
+approved”, requirements can be enforced at many different points during the
+process.
+
+Finite state machines <https://en.wikipedia.org/wiki/Finite-state_machine> are
+a mathematical model where an object can be in exactly one of a fixed set of
+states and can change states (or “transition”) according to fixed rules.
+
+## Decision
+
+We will use the django-fsm library to represent the status of our domain
+registration applications as a finite state machine. The library allows us to
+list what statuses are possible and describe which state transitions are
+possible (e.g. Can an approved application ever be marked as “in-process”?).
+
+## Consequences
+
+This should help us to keep domain logic localized within each model class. It
+should also make it easier to design the workflow stages and translate them
+clearly into application code.
+
+There is a possible negative impact that our finite state machine library might
+be unfamiliar to developers, but we can mitigate the impact by documenting this
+decision and lavishly commenting our business logic and transition methods.
+

docs/developer/README.md

@@ -112,3 +112,10 @@ Within the `registrar/assets` folder, the `_theme` folder contains three files i
 3. `styles.css` a entry point or index for the styles, forwards all of the other style files used in the project (i.e. the USWDS source code, the settings, and all custom stylesheets).
 
 You can also compile the sass at any time using `npx gulp compile`. Similarly, you can copy over other static assets (images and javascript files), using `npx gulp copyAssets`.
+
+## Finite State Machines
+
+In an effort to keep our domain logic centralized, we are representing the state of 
+objects in the application using the [django-fsm](https://github.com/viewflow/django-fsm)
+library. See the [ADR number 15](../architecture/decisions/0015-use-django-fs.md) for
+more information on the topic.

src/registrar/config/settings.py

@@ -86,11 +86,12 @@ INSTALLED_APPS = [
     "djangooidc",
     # library to simplify form templating
     "widget_tweaks",
+    # library for Finite State Machine statuses
+    "django_fsm",
     # let's be sure to install our own application!
     "registrar",
     # Our internal API application
     "api",
-    "django_fsm",
 ]
 
 # Middleware are routines for processing web requests.

src/registrar/migrations/0002_contact_website_domainapplication.py

@@ -1,4 +1,4 @@
-# Generated by Django 4.1.3 on 2022-11-07 19:39
+# Generated by Django 4.1.3 on 2022-11-08 20:17
 
 from django.conf import settings
 from django.db import migrations, models
@@ -25,12 +25,24 @@ class Migration(migrations.Migration):
                         verbose_name="ID",
                     ),
                 ),
-                ("first_name", models.TextField(help_text="First name", null=True)),
+                (
+                    "first_name",
+                    models.TextField(db_index=True, help_text="First name", null=True),
+                ),
                 ("middle_name", models.TextField(help_text="Middle name", null=True)),
-                ("last_name", models.TextField(help_text="Last name", null=True)),
+                (
+                    "last_name",
+                    models.TextField(db_index=True, help_text="Last name", null=True),
+                ),
                 ("title", models.TextField(help_text="Title", null=True)),
-                ("email", models.TextField(help_text="Email", null=True)),
+                (
-                ("phone", models.TextField(help_text="Phone", null=True)),
+                    "email",
+                    models.TextField(db_index=True, help_text="Email", null=True),
+                ),
+                (
+                    "phone",
+                    models.TextField(db_index=True, help_text="Phone", null=True),
+                ),
             ],
         ),
         migrations.CreateModel(
@@ -73,7 +85,6 @@ class Migration(migrations.Migration):
                         ],
                         default="started",
                         max_length=50,
-                        protected=True,
                     ),
                 ),
                 (
@@ -122,7 +133,9 @@ class Migration(migrations.Migration):
                 ),
                 (
                     "organization_name",
-                    models.TextField(help_text="Organization name", null=True),
+                    models.TextField(
+                        db_index=True, help_text="Organization name", null=True
+                    ),
                 ),
                 (
                     "street_address",
@@ -146,7 +159,9 @@ class Migration(migrations.Migration):
                 ),
                 (
                     "zip_code",
-                    models.CharField(help_text="ZIP code", max_length=10, null=True),
+                    models.CharField(
+                        db_index=True, help_text="ZIP code", max_length=10, null=True
+                    ),
                 ),
                 (
                     "purpose",

src/registrar/models/models.py

@@ -103,7 +103,7 @@ class Website(models.Model):
     DOMAIN_REGEX = re.compile(r"^(?!-)[A-Za-z0-9-]{1,63}(?<!-)\.[A-Za-z]{2,6}")
 
     @classmethod
-    def string_could_be_domain(cls, domain):
+    def string_could_be_domain(cls, domain: str) -> bool:
         """Return True if the string could be a domain name, otherwise False.
 
         TODO: when we have a Domain class, this could be a classmethod there.
@@ -112,21 +112,34 @@ class Website(models.Model):
             return True
         return False
 
+    def could_be_domain(self) -> bool:
+        """Could this instance be a domain?"""
+        # short-circuit if self.website is null/None
+        if not self.website:
+            return False
+        return self.string_could_be_domain(str(self.website))
+
+    def __str__(self) -> str:
+        return str(self.website)
+
 
 class Contact(models.Model):
 
     """Contact information follows a similar pattern for each contact."""
 
-    first_name = models.TextField(null=True, help_text="First name")
+    first_name = models.TextField(null=True, help_text="First name", db_index=True)
     middle_name = models.TextField(null=True, help_text="Middle name")
-    last_name = models.TextField(null=True, help_text="Last name")
+    last_name = models.TextField(null=True, help_text="Last name", db_index=True)
     title = models.TextField(null=True, help_text="Title")
-    email = models.TextField(null=True, help_text="Email")
+    email = models.TextField(null=True, help_text="Email", db_index=True)
-    phone = models.TextField(null=True, help_text="Phone")
+    phone = models.TextField(null=True, help_text="Phone", db_index=True)
 
 
 class DomainApplication(TimeStampedModel):
 
+    """A registrant's application for a new domain."""
+
+    # #### Contants for choice fields ####
     STARTED = "started"
     SUBMITTED = "submitted"
     INVESTIGATING = "investigating"
@@ -137,22 +150,6 @@ class DomainApplication(TimeStampedModel):
         (INVESTIGATING, INVESTIGATING),
         (APPROVED, APPROVED),
     ]
-    status = FSMField(
-        choices=STATUS_CHOICES,  # possible states as an array of constants
-        default=STARTED,  # sensible default
-        protected=True,  # cannot change state directly, must use methods!
-    )
-    creator = models.ForeignKey(
-        User, on_delete=models.PROTECT, related_name="applications_created"
-    )
-    investigator = models.ForeignKey(
-        User,
-        null=True,
-        on_delete=models.SET_NULL,
-        related_name="applications_investigating",
-    )
-
-    # data fields from the initial form
 
     FEDERAL = "federal"
     INTERSTATE = "interstate"
@@ -178,14 +175,35 @@ class DomainApplication(TimeStampedModel):
         (CITY, "a city, town, township, village, etc."),
         (SPECIAL_DISTRICT, "an independent organization within a single state"),
     ]
-    organization_type = models.CharField(
-        max_length=255, choices=ORGANIZATION_CHOICES, help_text="Type of Organization"
-    )
 
     EXECUTIVE = "Executive"
     JUDICIAL = "Judicial"
     LEGISLATIVE = "Legislative"
     BRANCH_CHOICES = [(x, x) for x in (EXECUTIVE, JUDICIAL, LEGISLATIVE)]
+
+    # #### Internal fields about the application #####
+    status = FSMField(
+        choices=STATUS_CHOICES,  # possible states as an array of constants
+        default=STARTED,  # sensible default
+        protected=False,  # can change state directly, particularly in Django admin
+    )
+    # This is the application user who created this application. The contact
+    # information that they gave is in the `submitter` field
+    creator = models.ForeignKey(
+        User, on_delete=models.PROTECT, related_name="applications_created"
+    )
+    investigator = models.ForeignKey(
+        User,
+        null=True,
+        on_delete=models.SET_NULL,
+        related_name="applications_investigating",
+    )
+
+    # ##### data fields from the initial form #####
+    organization_type = models.CharField(
+        max_length=255, choices=ORGANIZATION_CHOICES, help_text="Type of Organization"
+    )
+
     federal_branch = models.CharField(
         max_length=50,
         choices=BRANCH_CHOICES,
@@ -197,14 +215,18 @@ class DomainApplication(TimeStampedModel):
         null=True, help_text="Is your ogranization an election office?"
     )
 
-    organization_name = models.TextField(null=True, help_text="Organization name")
+    organization_name = models.TextField(
+        null=True, help_text="Organization name", db_index=True
+    )
     street_address = models.TextField(null=True, help_text="Street Address")
     unit_type = models.CharField(max_length=15, null=True, help_text="Unit type")
     unit_number = models.CharField(max_length=255, null=True, help_text="Unit number")
     state_territory = models.CharField(
         max_length=2, null=True, help_text="State/Territory"
     )
-    zip_code = models.CharField(max_length=10, null=True, help_text="ZIP code")
+    zip_code = models.CharField(
+        max_length=10, null=True, help_text="ZIP code", db_index=True
+    )
 
     authorizing_official = models.ForeignKey(
         Contact,
@@ -225,6 +247,8 @@ class DomainApplication(TimeStampedModel):
     )
     alternative_domains = models.ManyToManyField(Website, related_name="alternatives+")
 
+    # This is the contact information provided by the applicant. The
+    # application user who created it is in the `creator` field.
     submitter = models.ForeignKey(
         Contact,
         null=True,
@@ -250,16 +274,17 @@ class DomainApplication(TimeStampedModel):
         null=True, help_text="Acknowledged .gov acceptable use policy"
     )
 
-    def can_submit(self):
+    @transition(field="status", source=STARTED, target=SUBMITTED)
-        """Return True if this instance can be marked as submitted."""
-        if not Website.string_could_be_domain(self.requested_domain):
-            return False
-        return True
-
-    @transition(
-        field="status", source=STARTED, target=SUBMITTED, conditions=[can_submit]
-    )
     def submit(self):
         """Submit an application that is started."""
-        # don't need to do anything inside this method although we could
+
+        # check our conditions here inside the `submit` method so that we
+        # can raise more informative exceptions
+
+        # requested_domain could be None here
+        if (not self.requested_domain) or (not self.requested_domain.could_be_domain()):
+            raise ValueError("Requested domain is not a legal domain name.")
+
+        # if no exception was raised, then we don't need to do anything
+        # inside this method, keep the `pass` here to remind us of that
         pass

src/registrar/tests/test_models.py

@@ -46,3 +46,19 @@ class TestDomainApplication(TestCase):
         application.alternative_domains.add(gov_website)
         application.other_contacts.add(contact)
         application.save()
+
+    def test_status_fsm_submit_fail(self):
+        user, _ = User.objects.get_or_create()
+        application = DomainApplication.objects.create(creator=user)
+        with self.assertRaises(ValueError):
+            # can't submit an application with a null domain name
+            application.submit()
+
+    def test_status_fsm_submit_succeed(self):
+        user, _ = User.objects.get_or_create()
+        site = Website.objects.create(website="igorville.gov")
+        application = DomainApplication.objects.create(
+            creator=user, requested_domain=site
+        )
+        application.submit()
+        self.assertEqual(application.status, application.SUBMITTED)