Merge branch 'nl/981-test-domain-migration-script' into za/additional-data-transferred-domains

docs/operations/data_migration.md

@@ -87,12 +87,12 @@ FILE 3: **escrow_domain_statuses.daily.gov.GOV.txt** -> has the map of domains a
 We need to run a few scripts to parse these files into our domain tables.
 We can do this both locally and in a sandbox.
 
-## OPTION 1: SANDBOX
-## Load migration data onto a production or sandbox environment
+### SANDBOX MIGRATION SETUP
+### Load migration data onto a production or sandbox environment
 **WARNING:** All files uploaded in this manner are temporary, i.e. they will be deleted when the app is restaged.
 Do not use this method to store data you want to keep around permanently.
 
-### STEP 1: Use scp to transfer data
+#### STEP 1: Use scp to transfer data
 CloudFoundry supports scp as means of transferring data locally to our environment. If you are dealing with a batch of files, try sending across a tar.gz and unpacking that.
 
 **Login to Cloud.gov**
@@ -134,7 +134,7 @@ Copy this code into the password prompt from earlier.
 
 NOTE: You can use different utilities to copy this onto the clipboard for you. If you are on Windows, try the command `cf ssh-code | clip`. On Mac, this will be `cf ssh-code | pbcopy`
 
-### STEP 2: Transfer uploaded files to the getgov directory
+#### STEP 2: Transfer uploaded files to the getgov directory
 Due to the nature of how Cloud.gov operates, the getgov directory is dynamically generated whenever the app is built under the tmp/ folder. We can directly upload files to the tmp/ folder but cannot target the generated getgov folder directly, as we need to spin up a shell to access this. From here, we can move those uploaded files into the getgov directory using the `cat` command. Note that you will have to repeat this for each file you want to move, so it is better to use a tar.gz for multiple, and unpack it inside of the `datamigration` folder.
 
 **SSH into your sandbox**
@@ -168,7 +168,7 @@ tar -xvf migrationdata/{FILE_NAME}.tar.gz -C migrationdata/ --strip-components=1
 
 
 
-#### Manual method
+##### Manual method
 If the `cat_files_into_getgov.py` script isn't working, follow these steps instead.
 
 **Move the desired file into the correct directory**
@@ -178,34 +178,37 @@ cat ../tmp/{filename} > migrationdata/{filename}
 ```
 
 
-### STEP 3: Load Transition Domain data into TransitionDomain table
-Run the following script to transfer the existing data on our .txt files to our DB.
-```shell
-./manage.py load_transition_domain migrationdata/escrow_domain_contacts.daily.gov.GOV.txt migrationdata/escrow_contacts.daily.gov.GOV.txt migrationdata/escrow_domain_statuses.daily.gov.GOV.txt
+#### You are now ready to run migration scripts (see "Running the Migration Scripts")
 ```
 
-## OPTION 2: LOCAL
-## Load migration data onto our local environments
+```
+### LOCAL MIGRATION SETUP (TESTING PURPOSES ONLY)
+### Load migration data onto our local environments
 
-Transferring this data from these files into our domain tables happens in two steps;
+***IMPORTANT: only use test data, to avoid publicizing PII in our public repo.***
 
-***IMPORTANT: only run the following locally, to avoid publicizing PII in our public repo.***
-
-### STEP 1: Load Transition Domain data into TransitionDomain table
-
-**SETUP**
-In order to use the management command, we need to add the files to a folder under `src/`.
+In order to run the scripts locally, we need to add the files to a folder under `src/`.
 This will allow Docker to mount the files to a container (under `/app`) for our use.  
 
  - Create a folder called `tmp` underneath `src/`
  - Add the above files to this folder
  - Open a terminal and navigate to `src/`
 
-Then run the following command  (This will parse the three files in your `tmp` folder and load the information into the TransitionDomain table);
+#### You are now ready to run migration scripts (see "Running the Migration Scripts")
+
+### RUNNING THE MIGRATION SCRIPTS
+
+**NOTE: Whil we recommend executing the following scripts individually (Steps 1-3), migrations can also be done 'all at once' using the "Run Migration Feature" in step 4.  Use with discretion.**
+
+#### 1 - Load Transition Domains
+Run the following command  (This will parse the three files in your `tmp` folder and load the information into the TransitionDomain table);
 ```shell
-docker compose run -T app ./manage.py load_transition_domain /app/tmp/escrow_domain_contacts.daily.gov.GOV.txt /app/tmp/escrow_contacts.daily.gov.GOV.txt /app/tmp/escrow_domain_statuses.daily.gov.GOV.txt
+docker compose run -T app ./manage.py load_transition_domain /app/tmp/escrow_domain_contacts.daily.gov.GOV.txt /app/tmp/escrow_contacts.daily.gov.GOV.txt /app/tmp/escrow_domain_statuses.daily.gov.GOV.txt --debug
 ```
 
+**For Sandbox**:
+Change "/app/tmp" to point to the sandbox directory
+
 **OPTIONAL COMMAND LINE ARGUMENTS**: 
 `--debug`
 This will print out additional, detailed logs.
@@ -214,7 +217,7 @@ This will print out additional, detailed logs.
 Directs the script to load only the first 100 entries into the table.  You can adjust this number as needed for testing purposes.  
 
 `--resetTable`
-This will delete all the data loaded into transtion_domain.  It is helpful if you want to see the entries reload from scratch or for clearing test data.
+This will delete all the data in transtion_domain.  It is helpful if you want to see the entries reload from scratch or for clearing test data.
 
 
 ### STEP 2: Transfer Transition Domain data into main Domain tables
@@ -224,7 +227,7 @@ Now that we've loaded all the data into TransitionDomain, we need to update the
 In the same terminal as used in STEP 1, run the command below; 
 (This will parse the data in TransitionDomain and either create a corresponding Domain object, OR, if a corresponding Domain already exists, it will update that Domain with the incoming status. It will also create DomainInvitation objects for each user associated with the domain):
 ```shell
-docker compose run -T app ./manage.py transfer_transition_domains_to_domains
+docker compose run -T app ./manage.py transfer_transition_domains_to_domains --debug
 ```
 
 **OPTIONAL COMMAND LINE ARGUMENTS**: 
@@ -232,4 +235,68 @@ docker compose run -T app ./manage.py transfer_transition_domains_to_domains
 This will print out additional, detailed logs.
 
 `--limitParse 100` 
-Directs the script to load only the first 100 entries into the table.  You can adjust this number as needed for testing purposes.  
+Directs the script to load only the first 100 entries into the table.  You can adjust this number as needed for testing purposes.  
+
+### STEP 3: Send Domain invitations
+### Run the send invitations script
+
+To send invitations for every transition domain in the transition domain table, execute the following command:
+`docker compose run -T app send_domain_invitations -s`
+
+### STEP 4: Test the results
+### Run the migration analyzer
+
+This script's main function is to scan the transition domain and domain tables for any anomalies.  It produces a simple report of missing or duplicate data.  NOTE: some missing data might be expected depending on the nature of our migrations so use best judgement when evaluating the results.
+
+**ANALYZE ONLY**
+To analyze our database without running migrations, execute the script without any optional arguments:
+`docker compose run -T app ./manage.py master_domain_migrations --debug`
+
+**RUN MIGRATIONS FEATURE**
+To run the migrations again (all above migration steps) before analyzing, execute the following command (read the documentation on the terminal arguments below.  Everything used by the migration scripts can also be passed into this script and will have the same effects).  NOTE: --debug and --prompt allow you to step through the migration process and exit it after each step if you need to.  It is recommended that you use these arguments when using the --runMigrations feature:
+`docker compose run -T app ./manage.py master_domain_migrations --runMigrations --debug --prompt`
+
+#### OPTIONAL ARGUMENTS
+`--runMigrations`
+A boolean (default to true), which triggers running
+all scripts (in sequence) for transition domain migrations
+
+`--migrationDirectory`
+**default="migrationData"** (<--This is the sandbox directory)
+The location of the files used for load_transition_domain migration script
+EXAMPLE USAGE:
+--migrationDirectory /app/tmp
+
+`--migrationFilenames`
+**default=escrow_domain_contacts.daily.gov.GOV.txt,escrow_contacts.daily.gov.GOV.txt,escrow_domain_statuses.daily.gov.GOV.txt** (<--These are the usual names for the files.  The script will throw a warning if it cannot find these exact files, in which case you will need to supply the correct filenames)
+The files used for load_transition_domain migration script.
+Must appear IN ORDER and comma-delimited: 
+EXAMPLE USAGE:
+--migrationFilenames domain_contacts_filename.txt,contacts_filename.txt,domain_statuses_filename.txt
+where...
+- domain_contacts_filename is the Data file with domain contact information
+- contacts_filename is the Data file with contact information
+- domain_statuses_filename is the Data file with domain status information
+
+`--sep`
+Delimiter for the migration scripts to correctly parse the given text files.
+(usually this can remain at default value of |)
+
+`--debug`
+A boolean (default to true), which activates additional print statements
+
+`--prompt`
+A boolean (default to true), which activates terminal prompts
+that allows the user to step through each portion of this
+script.
+
+`--limitParse`
+Used by the migration scripts (load_transition_domain) to set the limit for the
+number of data entries to insert.  Set to 0 (or just don't use this
+argument) to parse every entry. This was provided primarily for testing
+purposes
+
+`--resetTable`
+Used by the migration scripts to trigger a prompt for deleting all table entries.  
+Useful for testing purposes, but USE WITH CAUTION
+```

src/epplibwrapper/__init__.py

@@ -45,7 +45,7 @@ except NameError:
 # Attn: these imports should NOT be at the top of the file
 try:
     from .client import CLIENT, commands
-    from .errors import RegistryError, ErrorCode, CANNOT_CONTACT_REGISTRY, GENERIC_ERROR
+    from .errors import RegistryError, ErrorCode
     from epplib.models import common, info
     from epplib.responses import extensions
     from epplib import responses
@@ -61,6 +61,4 @@ __all__ = [
     "info",
     "ErrorCode",
     "RegistryError",
-    "CANNOT_CONTACT_REGISTRY",
-    "GENERIC_ERROR",
 ]

src/epplibwrapper/errors.py

@@ -1,8 +1,5 @@
 from enum import IntEnum
 
-CANNOT_CONTACT_REGISTRY = "Update failed. Cannot contact the registry."
-GENERIC_ERROR = "Value entered was wrong."
-
 
 class ErrorCode(IntEnum):
     """

src/registrar/assets/js/get-gov.js

@@ -231,41 +231,15 @@ function handleValidationClick(e) {
 
 
 /**
- * An IIFE that attaches a click handler for our dynamic nameservers form
- *
- * Only does something on a single page, but it should be fast enough to run
- * it everywhere.
+ * Prepare the namerservers and DS data forms delete buttons
+ * We will call this on the forms init, and also every time we add a form
+ * 
  */
-(function prepareNameserverForms() {
-  let serverForm = document.querySelectorAll(".server-form");
-  let container = document.querySelector("#form-container");
-  let addButton = document.querySelector("#add-nameserver-form");
-  let totalForms = document.querySelector("#id_form-TOTAL_FORMS");
-
-  let formNum = serverForm.length-1;
-  if (addButton)
-    addButton.addEventListener('click', addForm);
-
-  function addForm(e){
-      let newForm = serverForm[2].cloneNode(true);
-      let formNumberRegex = RegExp(`form-(\\d){1}-`,'g');
-      let formLabelRegex = RegExp(`Name server (\\d){1}`, 'g');
-      let formExampleRegex = RegExp(`ns(\\d){1}`, 'g');
-
-      formNum++;
-      newForm.innerHTML = newForm.innerHTML.replace(formNumberRegex, `form-${formNum}-`);
-      newForm.innerHTML = newForm.innerHTML.replace(formLabelRegex, `Name server ${formNum+1}`);
-      newForm.innerHTML = newForm.innerHTML.replace(formExampleRegex, `ns${formNum+1}`);
-      container.insertBefore(newForm, addButton);
-      newForm.querySelector("input").value = "";
-
-      totalForms.setAttribute('value', `${formNum+1}`);
-  }
-})();
-
-function prepareDeleteButtons() {
+function prepareDeleteButtons(formLabel) {
   let deleteButtons = document.querySelectorAll(".delete-record");
   let totalForms = document.querySelector("#id_form-TOTAL_FORMS");
+  let isNameserversForm = document.title.includes("DNS name servers |");
+  let addButton = document.querySelector("#add-form");
 
   // Loop through each delete button and attach the click event listener
   deleteButtons.forEach((deleteButton) => {
@@ -273,13 +247,15 @@ function prepareDeleteButtons() {
   });
 
   function removeForm(e){
-    let formToRemove = e.target.closest(".ds-record");
+    let formToRemove = e.target.closest(".repeatable-form");
     formToRemove.remove();
-    let forms = document.querySelectorAll(".ds-record");
+    let forms = document.querySelectorAll(".repeatable-form");
     totalForms.setAttribute('value', `${forms.length}`);
 
     let formNumberRegex = RegExp(`form-(\\d){1}-`, 'g');
-    let formLabelRegex = RegExp(`DS Data record (\\d){1}`, 'g');
+    let formLabelRegex = RegExp(`${formLabel} (\\d+){1}`, 'g');
+    // For the example on Nameservers
+    let formExampleRegex = RegExp(`ns(\\d+){1}`, 'g');
 
     forms.forEach((form, index) => {
       // Iterate over child nodes of the current element
@@ -294,48 +270,88 @@ function prepareDeleteButtons() {
         });
       });
 
-      Array.from(form.querySelectorAll('h2, legend')).forEach((node) => {
-        node.textContent = node.textContent.replace(formLabelRegex, `DS Data record ${index + 1}`);
+      // h2 and legend for DS form, label for nameservers  
+      Array.from(form.querySelectorAll('h2, legend, label, p')).forEach((node) => {
+        
+        // Ticket: 1192
+        // if (isNameserversForm && index <= 1 && !node.innerHTML.includes('*')) {
+        // // Create a new element
+        // const newElement = document.createElement('abbr');
+        // newElement.textContent = '*';
+        // // TODO: finish building abbr
+
+        // // Append the new element to the parent
+        // node.appendChild(newElement);
+        //   // Find the next sibling that is an input element
+        //   let nextInputElement = node.nextElementSibling;
+
+        //   while (nextInputElement) {
+        //     if (nextInputElement.tagName === 'INPUT') {
+        //       // Found the next input element
+        //       console.log(nextInputElement);
+        //       break;
+        //     }
+        //     nextInputElement = nextInputElement.nextElementSibling;
+        //   }
+        //   nextInputElement.required = true;
+        // }
+
+        // Ticket: 1192 - remove if
+        if (!(isNameserversForm && index <= 1)) {
+          node.textContent = node.textContent.replace(formLabelRegex, `${formLabel} ${index + 1}`);
+          node.textContent = node.textContent.replace(formExampleRegex, `ns${index + 1}`);
+        }
       });
+
+      // Display the add more button if we have less than 13 forms
+      if (isNameserversForm && forms.length <= 13) {
+        addButton.classList.remove("display-none")
+      }
     
     });
   }
 }
 
 /**
- * An IIFE that attaches a click handler for our dynamic DNSSEC forms
+ * An IIFE that attaches a click handler for our dynamic formsets
  *
+ * Only does something on a few pages, but it should be fast enough to run
+ * it everywhere.
  */
-(function prepareDNSSECForms() {
-  let serverForm = document.querySelectorAll(".ds-record");
+(function prepareFormsetsForms() {
+  let repeatableForm = document.querySelectorAll(".repeatable-form");
   let container = document.querySelector("#form-container");
-  let addButton = document.querySelector("#add-ds-form");
+  let addButton = document.querySelector("#add-form");
   let totalForms = document.querySelector("#id_form-TOTAL_FORMS");
+  let cloneIndex = 0;
+  let formLabel = '';
+  let isNameserversForm = document.title.includes("DNS name servers |");
+  if (isNameserversForm) {
+    cloneIndex = 2;
+    formLabel = "Name server";
+  } else if ((document.title.includes("DS Data |")) || (document.title.includes("Key Data |"))) {
+    formLabel = "DS Data record";
+  }
 
   // Attach click event listener on the delete buttons of the existing forms
-  prepareDeleteButtons();
+  prepareDeleteButtons(formLabel);
 
-  // Attack click event listener on the add button
   if (addButton)
     addButton.addEventListener('click', addForm);
 
-  /*
-   * Add a formset to the end of the form.
-   * For each element in the added formset, name the elements with the prefix,
-   * form-{#}-{element_name} where # is the index of the formset and element_name
-   * is the element's name.
-   * Additionally, update the form element's metadata, including totalForms' value.
-   */
   function addForm(e){
-      let forms = document.querySelectorAll(".ds-record");
+      let forms = document.querySelectorAll(".repeatable-form");
       let formNum = forms.length;
-      let newForm = serverForm[0].cloneNode(true);
+      let newForm = repeatableForm[cloneIndex].cloneNode(true);
       let formNumberRegex = RegExp(`form-(\\d){1}-`,'g');
-      let formLabelRegex = RegExp(`DS Data record (\\d){1}`, 'g');
+      let formLabelRegex = RegExp(`${formLabel} (\\d){1}`, 'g');
+      // For the eample on Nameservers
+      let formExampleRegex = RegExp(`ns(\\d){1}`, 'g');
 
       formNum++;
       newForm.innerHTML = newForm.innerHTML.replace(formNumberRegex, `form-${formNum-1}-`);
-      newForm.innerHTML = newForm.innerHTML.replace(formLabelRegex, `DS Data record ${formNum}`);
+      newForm.innerHTML = newForm.innerHTML.replace(formLabelRegex, `${formLabel} ${formNum}`);
+      newForm.innerHTML = newForm.innerHTML.replace(formExampleRegex, `ns${formNum}`);
       container.insertBefore(newForm, addButton);
 
       let inputs = newForm.querySelectorAll("input");
@@ -379,9 +395,13 @@ function prepareDeleteButtons() {
       totalForms.setAttribute('value', `${formNum}`);
 
       // Attach click event listener on the delete buttons of the new form
-      prepareDeleteButtons();
-  }
+      prepareDeleteButtons(formLabel);
 
+      // Hide the add more button if we have 13 forms
+      if (isNameserversForm && formNum == 13) {
+        addButton.classList.add("display-none")
+      }
+  }
 })();
 
 /**

src/registrar/assets/sass/_theme/_forms.scss

@@ -4,6 +4,10 @@
   margin-top: units(3);
 }
 
+.usa-form .usa-button.margin-bottom-075 {
+  margin-bottom: units(1.5);
+}
+
 .usa-form .usa-button.margin-top-1 {
   margin-top: units(1);
 }

src/registrar/forms/domain.py

@@ -5,8 +5,12 @@ from django.core.validators import MinValueValidator, MaxValueValidator, RegexVa
 from django.forms import formset_factory
 
 from phonenumber_field.widgets import RegionalPhoneNumberWidget
+from registrar.utility.errors import (
+    NameserverError,
+    NameserverErrorCodes as nsErrorCodes,
+)
 
-from ..models import Contact, DomainInformation
+from ..models import Contact, DomainInformation, Domain
 from .common import (
     ALGORITHM_CHOICES,
     DIGEST_TYPE_CHOICES,
@@ -19,16 +23,78 @@ class DomainAddUserForm(forms.Form):
     email = forms.EmailField(label="Email")
 
 
+class IPAddressField(forms.CharField):
+    def validate(self, value):
+        super().validate(value)  # Run the default CharField validation
+
+
 class DomainNameserverForm(forms.Form):
     """Form for changing nameservers."""
 
+    domain = forms.CharField(widget=forms.HiddenInput, required=False)
+
     server = forms.CharField(label="Name server", strip=True)
-    # when adding IPs to this form ensure they are stripped as well
+
+    ip = forms.CharField(label="IP Address (IPv4 or IPv6)", strip=True, required=False)
+
+    def clean(self):
+        # clean is called from clean_forms, which is called from is_valid
+        # after clean_fields.  it is used to determine form level errors.
+        # is_valid is typically called from view during a post
+        cleaned_data = super().clean()
+        self.clean_empty_strings(cleaned_data)
+        server = cleaned_data.get("server", "")
+        ip = cleaned_data.get("ip", None)
+        # remove ANY spaces in the ip field
+        ip = ip.replace(" ", "")
+        domain = cleaned_data.get("domain", "")
+
+        ip_list = self.extract_ip_list(ip)
+
+        if ip and not server and ip_list:
+            self.add_error("server", NameserverError(code=nsErrorCodes.MISSING_HOST))
+        elif server:
+            self.validate_nameserver_ip_combo(domain, server, ip_list)
+
+        return cleaned_data
+
+    def clean_empty_strings(self, cleaned_data):
+        ip = cleaned_data.get("ip", "")
+        if ip and len(ip.strip()) == 0:
+            cleaned_data["ip"] = None
+
+    def extract_ip_list(self, ip):
+        return [ip.strip() for ip in ip.split(",")] if ip else []
+
+    def validate_nameserver_ip_combo(self, domain, server, ip_list):
+        try:
+            Domain.checkHostIPCombo(domain, server, ip_list)
+        except NameserverError as e:
+            if e.code == nsErrorCodes.GLUE_RECORD_NOT_ALLOWED:
+                self.add_error(
+                    "server",
+                    NameserverError(
+                        code=nsErrorCodes.GLUE_RECORD_NOT_ALLOWED,
+                        nameserver=domain,
+                        ip=ip_list,
+                    ),
+                )
+            elif e.code == nsErrorCodes.MISSING_IP:
+                self.add_error(
+                    "ip",
+                    NameserverError(
+                        code=nsErrorCodes.MISSING_IP, nameserver=domain, ip=ip_list
+                    ),
+                )
+            else:
+                self.add_error("ip", str(e))
 
 
 NameserverFormset = formset_factory(
     DomainNameserverForm,
     extra=1,
+    max_num=13,
+    validate_max=True,
 )
 
 

src/registrar/management/commands/cat_files_into_getgov.py

@@ -3,9 +3,12 @@ import glob
 import logging
 
 import os
+import string
 
 from django.core.management import BaseCommand
 
+from registrar.management.commands.utility.terminal_helper import TerminalHelper
+
 
 logger = logging.getLogger(__name__)
 
@@ -27,6 +30,7 @@ class Command(BaseCommand):
     def handle(self, **options):
         file_extension: str = options.get("file_extension").lstrip(".")
         directory = options.get("directory")
+        helper = TerminalHelper()
 
         # file_extension is always coerced as str, Truthy is OK to use here.
         if not file_extension or not isinstance(file_extension, str):
@@ -38,29 +42,59 @@ class Command(BaseCommand):
 
         for src_file_path in matching_extensions:
             filename = os.path.basename(src_file_path)
+            exit_status = -1
             do_command = True
-            exit_status: int
 
             desired_file_path = f"{directory}/{filename}"
             if os.path.exists(desired_file_path):
                 # For linter
-                prompt = " Do you want to replace it? (y/n) "
-                replace = input(f"{desired_file_path} already exists. {prompt}")
-                if replace.lower() != "y":
+                prompt = "Do you want to replace it?"
+                replace = f"{desired_file_path} already exists. {prompt}"
+                if not helper.query_yes_no(replace):
                     do_command = False
 
-            if do_command:
-                copy_from = f"../tmp/{filename}"
-                self.cat(copy_from, desired_file_path)
-                exit_status = os.system(f"cat ../tmp/{filename} > {desired_file_path}")
-
-            if exit_status == 0:
-                logger.info(f"Successfully copied {filename}")
-            else:
-                logger.error(f"Failed to copy {filename}")
+            try:
+                if do_command:
+                    copy_from = f"../tmp/{filename}"
+                    exit_status = self.cat(copy_from, desired_file_path)
+            except ValueError as err:
+                raise err
+            finally:
+                if exit_status == 0:
+                    logger.info(f"Successfully copied {filename}")
+                else:
+                    logger.error(f"Failed to copy {filename}")
 
     def cat(self, copy_from, copy_to):
         """Runs the cat command to
         copy_from a location to copy_to a location"""
-        exit_status = os.system(f"cat {copy_from} > {copy_to}")
+
+        # copy_from will be system defined
+        self.check_file_path(copy_from, check_directory=False)
+        self.check_file_path(copy_to)
+
+        # This command can only be ran from inside cf ssh getgov-{sandbox}
+        # It has no utility when running locally, and to exploit this
+        # you would have to have ssh access anyway, which is a bigger problem.
+        exit_status = os.system(f"cat {copy_from} > {copy_to}")  # nosec
         return exit_status
+
+    def check_file_path(self, file_path: str, check_directory=True):
+        """Does a check on user input to ensure validity"""
+        if not isinstance(file_path, str):
+            raise ValueError("Invalid path provided")
+
+        # Remove any initial/final whitespace
+        file_path = file_path.strip()
+
+        # Check for any attempts to move up in the directory structure
+        if ".." in file_path and check_directory:
+            raise ValueError("Moving up in the directory structure is not allowed")
+
+        # Check for any invalid characters
+        valid_chars = f"/-_.() {string.ascii_letters}{string.digits}"
+        for char in file_path:
+            if char not in valid_chars:
+                raise ValueError(f"Invalid character {char} in file path")
+
+        return file_path

src/registrar/management/commands/master_domain_migrations.py

@@ -8,11 +8,6 @@
 import logging
 import argparse
 import sys
-import os
-
-from django.test import Client
-
-from django_fsm import TransitionNotAllowed  # type: ignore
 
 from django.core.management import BaseCommand
 from django.core.management import call_command
@@ -22,7 +17,6 @@ from registrar.models import (
     DomainInformation,
     DomainInvitation,
     TransitionDomain,
-    User,
 )
 
 from registrar.management.commands.utility.terminal_helper import (
@@ -100,19 +94,26 @@ class Command(BaseCommand):
         parser.add_argument(
             "--migrationDirectory",
             default="migrationData",
-            help="The location of the files used for load_transition_domain migration script",
+            help=(
+                "The location of the files used for"
+                "load_transition_domain migration script"
+            ),
         )
         parser.add_argument(
             "--migrationFilenames",
-            default="escrow_domain_contacts.daily.gov.GOV.txt,escrow_contacts.daily.gov.GOV.txt,escrow_domain_statuses.daily.gov.GOV.txt",
-            help="""The files used for load_transition_domain migration script.  
-            Must appear IN ORDER and separated by commas: 
+            default="""escrow_domain_contacts.daily.gov.GOV.txt,
+            escrow_contacts.daily.gov.GOV.txt,
+            escrow_domain_statuses.daily.gov.GOV.txt""",
+            help="""The files used for load_transition_domain migration script.
+            Must appear IN ORDER and separated by commas:
             domain_contacts_filename.txt,contacts_filename.txt,domain_statuses_filename.txt
-            
+
             where...
-            - domain_contacts_filename is the Data file with domain contact information
+            - domain_contacts_filename is the Data file with domain contact
+            information
             - contacts_filename is the Data file with contact information
-            - domain_statuses_filename is the Data file with domain status information""",
+            - domain_statuses_filename is the Data file with domain status
+            information""",
         )
 
         parser.add_argument(
@@ -196,13 +197,15 @@ class Command(BaseCommand):
             if len(matching_domain_informations) == 0:
                 TerminalHelper.print_conditional(
                     debug_on,
-                    f"""{TerminalColors.YELLOW}Missing Domain Information{TerminalColors.ENDC}""",
+                    f"""{TerminalColors.YELLOW}Missing Domain Information
+                    {TerminalColors.ENDC}""",
                 )
                 missing_domain_informations.append(transition_domain_name)
             if len(matching_domain_invitations) == 0:
                 TerminalHelper.print_conditional(
                     debug_on,
-                    f"""{TerminalColors.YELLOW}Missing Domain Invitation{TerminalColors.ENDC}""",
+                    f"""{TerminalColors.YELLOW}Missing Domain Invitation
+                    {TerminalColors.ENDC}""",
                 )
                 missing_domain_invites.append(transition_domain_name)
 
@@ -225,22 +228,26 @@ class Command(BaseCommand):
         logger.info(
             f"""{TerminalColors.OKGREEN}
             ============= FINISHED ANALYSIS ===============
-            
+
             {total_missing_domains} Missing Domains:
             (These are transition domains that are missing from the Domain Table)
-            {TerminalColors.YELLOW}{missing_domains_as_string}{TerminalColors.OKGREEN}
-
+            {TerminalColors.YELLOW}{missing_domains_as_string}
+            {TerminalColors.OKGREEN}
             {total_duplicate_domains} Duplicate Domains:
-            (These are transition domains which have duplicate entries in the Domain Table)
-            {TerminalColors.YELLOW}{duplicate_domains_as_string}{TerminalColors.OKGREEN}
-
+            (These are transition domains which have duplicate
+            entries in the Domain Table)
+            {TerminalColors.YELLOW}{duplicate_domains_as_string}
+            {TerminalColors.OKGREEN}
             {total_missing_domain_informations} Domain Information Entries missing:
-            (These are transition domains which have no entries in the Domain Information Table)
-            {TerminalColors.YELLOW}{missing_domain_informations_as_string}{TerminalColors.OKGREEN}
-
+            (These are transition domains which have no entries
+            in the Domain Information Table)
+            {TerminalColors.YELLOW}{missing_domain_informations_as_string}
+            {TerminalColors.OKGREEN}
             {total_missing_domain_invitations} Domain Invitations missing:
-            (These are transition domains which have no entires in the Domain Invitation Table)
-            {TerminalColors.YELLOW}{missing_domain_invites_as_string}{TerminalColors.OKGREEN}
+            (These are transition domains which have no entires in
+            the Domain Invitation Table)
+            {TerminalColors.YELLOW}{missing_domain_invites_as_string}
+            {TerminalColors.OKGREEN}
             {TerminalColors.ENDC}
             """
         )
@@ -377,13 +384,10 @@ class Command(BaseCommand):
                 logger.info(
                     f"""
                 {TerminalColors.YELLOW}
-                PLEASE Re-Run the script with the correct file location and filenames: 
-                
-                EXAMPLE:
-                docker compose run -T app ./manage.py test_domain_migration --runMigrations --migrationDirectory /app/tmp --migrationFilenames escrow_domain_contacts.daily.gov.GOV.txt escrow_contacts.daily.gov.GOV.txt escrow_domain_statuses.daily.gov.GOV.txt
-                
+                PLEASE Re-Run the script with the correct
+                file location and filenames:
                 """
-                )  # noqa
+                )
                 return
 
         # Proceed executing the migration scripts
@@ -400,33 +404,6 @@ class Command(BaseCommand):
         )
         self.run_transfer_script(debug_on, prompts_enabled)
 
-    def simulate_user_logins(self, debug_on):
-        """Simulates logins for users (this will add
-        Domain Information objects to our tables)"""
-
-        # logger.info(f""
-        #             f"{TerminalColors.OKCYAN}"
-        #             f"================== SIMULATING LOGINS =================="
-        #             f"{TerminalColors.ENDC}")
-
-        # for invite in DomainInvitation.objects.all(): #TODO: move to unit test
-        #     #DEBUG:
-        #     TerminalHelper.print_conditional(debug_on,
-        #                                      f"{TerminalColors.OKCYAN}"
-        #                                      f"Processing invite: {invite}"
-        #                                      f"{TerminalColors.ENDC}")
-        #     # get a user with this email address
-        #     user, user_created = User.objects.get_or_create(email=invite.email, username=invite.email)
-        #     #DEBUG:
-        #     TerminalHelper.print_conditional(user_created,
-        #                                      f"""{TerminalColors.OKCYAN}No user found (creating temporary user object){TerminalColors.ENDC}""")
-        #     TerminalHelper.print_conditional(debug_on,
-        #                                      f"""{TerminalColors.OKCYAN}Executing first-time login for user: {user}{TerminalColors.ENDC}""")
-        #     user.first_login()
-        #     if user_created:
-        #         logger.info(f"""{TerminalColors.YELLOW}(Deleting temporary user object){TerminalColors.ENDC}""")
-        #         user.delete()
-
     def handle(
         self,
         **options,
@@ -453,9 +430,6 @@ class Command(BaseCommand):
         debug_on = options.get("debug")
         prompts_enabled = options.get("prompt")
         run_migrations_enabled = options.get("runMigrations")
-        simulate_user_login_enabled = (
-            False  # TODO: delete? Moving to unit test... options.get("triggerLogins")
-        )
 
         TerminalHelper.print_conditional(
             debug_on,
@@ -538,30 +512,7 @@ class Command(BaseCommand):
             )
             prompt_continuation_of_analysis = True
 
-        # STEP 2 -- SIMULATE LOGINS
-        # Simulate user login for each user in domain
-        # invitation if specified by user OR if running
-        # migration scripts.
-        # (NOTE: Although users can choose to run login
-        # simulations separately (for testing purposes),
-        # if we are running all migration scripts, we should
-        # automatically execute this as the final step
-        # to ensure Domain Information objects get added
-        # to the database.)
-
-        if run_migrations_enabled and simulate_user_login_enabled:
-            if prompts_enabled:
-                simulate_user_login_enabled = TerminalHelper.query_yes_no(
-                    f"""{TerminalColors.FAIL}
-                Proceed with simulating user logins?
-                {TerminalColors.ENDC}"""
-                )
-                if not simulate_user_login_enabled:
-                    return
-            self.simulate_user_logins(debug_on)
-            prompt_continuation_of_analysis = True
-
-        # STEP 3 -- SEND INVITES
+        # STEP 2 -- SEND INVITES
         proceed_with_sending_invites = run_migrations_enabled
         if prompts_enabled and run_migrations_enabled:
             proceed_with_sending_invites = TerminalHelper.query_yes_no(
@@ -574,7 +525,7 @@ class Command(BaseCommand):
             self.run_send_invites_script(debug_on, prompts_enabled)
             prompt_continuation_of_analysis = True
 
-        # STEP 4 -- ANALYZE TABLES & GENERATE REPORT
+        # STEP 3 -- ANALYZE TABLES & GENERATE REPORT
         # Analyze tables for corrupt data...
         if prompt_continuation_of_analysis and prompts_enabled:
             # ^ (only prompt if we ran steps 1 and/or 2)

src/registrar/management/commands/utility/terminal_helper.py

@@ -1,5 +1,4 @@
 import logging
-import os
 import sys
 
 logger = logging.getLogger(__name__)
@@ -23,6 +22,7 @@ class TerminalColors:
 
 
 class TerminalHelper:
+    @staticmethod
     def query_yes_no(question: str, default="yes") -> bool:
         """Ask a yes/no question via raw_input() and return their answer.
 
@@ -53,6 +53,7 @@ class TerminalHelper:
             else:
                 logger.info("Please respond with 'yes' or 'no' " "(or 'y' or 'n').\n")
 
+    @staticmethod
     def print_conditional(print_condition: bool, print_statement: str):
         """This function reduces complexity of debug statements
         in other functions.
@@ -62,12 +63,13 @@ class TerminalHelper:
         if print_condition:
             logger.info(print_statement)
 
+    @staticmethod
     def prompt_for_execution(
         system_exit_on_terminate: bool, info_to_inspect: str, prompt_title: str
     ) -> bool:
         """Create to reduce code complexity.
         Prompts the user to inspect the given string
-        and asks if they wish to execute it.
+        and asks if they wish to proceed.
         Returns true if the user responds (y),
         Returns false if the user responds (n)"""
 

src/registrar/models/domain.py

@@ -262,8 +262,11 @@ class Domain(TimeStampedModel, DomainHelper):
         """Creates the host object in the registry
         doesn't add the created host to the domain
         returns ErrorCode (int)"""
-        if addrs is not None:
-            addresses = [epp.Ip(addr=addr) for addr in addrs]
+        if addrs is not None and addrs != []:
+            addresses = [
+                epp.Ip(addr=addr, ip="v6" if self.is_ipv6(addr) else None)
+                for addr in addrs
+            ]
             request = commands.CreateHost(name=host, addrs=addresses)
         else:
             request = commands.CreateHost(name=host)
@@ -274,7 +277,7 @@ class Domain(TimeStampedModel, DomainHelper):
             return response.code
         except RegistryError as e:
             logger.error("Error _create_host, code was %s error was %s" % (e.code, e))
-            return e.code
+            raise e
 
     def _convert_list_to_dict(self, listToConvert: list[tuple[str, list]]):
         """converts a list of hosts into a dictionary
@@ -293,14 +296,16 @@ class Domain(TimeStampedModel, DomainHelper):
                 newDict[tup[0]] = tup[1]
         return newDict
 
-    def isSubdomain(self, nameserver: str):
+    @classmethod
+    def isSubdomain(cls, name: str, nameserver: str):
         """Returns boolean if the domain name is found in the argument passed"""
         subdomain_pattern = r"([\w-]+\.)*"
-        full_pattern = subdomain_pattern + self.name
+        full_pattern = subdomain_pattern + name
         regex = re.compile(full_pattern)
         return bool(regex.match(nameserver))
 
-    def checkHostIPCombo(self, nameserver: str, ip: list[str]):
+    @classmethod
+    def checkHostIPCombo(cls, name: str, nameserver: str, ip: list[str]):
         """Checks the parameters past for a valid combination
         raises error if:
             - nameserver is a subdomain but is missing ip
@@ -314,22 +319,23 @@ class Domain(TimeStampedModel, DomainHelper):
             NameserverError (if exception hit)
         Returns:
             None"""
-        if self.isSubdomain(nameserver) and (ip is None or ip == []):
+        if cls.isSubdomain(name, nameserver) and (ip is None or ip == []):
             raise NameserverError(code=nsErrorCodes.MISSING_IP, nameserver=nameserver)
 
-        elif not self.isSubdomain(nameserver) and (ip is not None and ip != []):
+        elif not cls.isSubdomain(name, nameserver) and (ip is not None and ip != []):
             raise NameserverError(
                 code=nsErrorCodes.GLUE_RECORD_NOT_ALLOWED, nameserver=nameserver, ip=ip
             )
         elif ip is not None and ip != []:
             for addr in ip:
-                if not self._valid_ip_addr(addr):
+                if not cls._valid_ip_addr(addr):
                     raise NameserverError(
                         code=nsErrorCodes.INVALID_IP, nameserver=nameserver, ip=ip
                     )
         return None
 
-    def _valid_ip_addr(self, ipToTest: str):
+    @classmethod
+    def _valid_ip_addr(cls, ipToTest: str):
         """returns boolean if valid ip address string
         We currently only accept v4 or v6 ips
         returns:
@@ -382,7 +388,9 @@ class Domain(TimeStampedModel, DomainHelper):
                 if newHostDict[prevHost] is not None and set(
                     newHostDict[prevHost]
                 ) != set(addrs):
-                    self.checkHostIPCombo(nameserver=prevHost, ip=newHostDict[prevHost])
+                    self.__class__.checkHostIPCombo(
+                        name=self.name, nameserver=prevHost, ip=newHostDict[prevHost]
+                    )
                     updated_values.append((prevHost, newHostDict[prevHost]))
 
         new_values = {
@@ -392,7 +400,9 @@ class Domain(TimeStampedModel, DomainHelper):
         }
 
         for nameserver, ip in new_values.items():
-            self.checkHostIPCombo(nameserver=nameserver, ip=ip)
+            self.__class__.checkHostIPCombo(
+                name=self.name, nameserver=nameserver, ip=ip
+            )
 
         return (deleted_values, updated_values, new_values, previousHostDict)
 
@@ -567,7 +577,11 @@ class Domain(TimeStampedModel, DomainHelper):
         if len(hosts) > 13:
             raise NameserverError(code=nsErrorCodes.TOO_MANY_HOSTS)
 
-        if self.state not in [self.State.DNS_NEEDED, self.State.READY]:
+        if self.state not in [
+            self.State.DNS_NEEDED,
+            self.State.READY,
+            self.State.UNKNOWN,
+        ]:
             raise ActionNotAllowed("Nameservers can not be " "set in the current state")
 
         logger.info("Setting nameservers")
@@ -1351,7 +1365,7 @@ class Domain(TimeStampedModel, DomainHelper):
 
     @transition(
         field="state",
-        source=[State.DNS_NEEDED],
+        source=[State.DNS_NEEDED, State.READY],
         target=State.READY,
         # conditions=[dns_not_needed]
     )
@@ -1514,7 +1528,7 @@ class Domain(TimeStampedModel, DomainHelper):
             data = registry.send(req, cleaned=True).res_data[0]
             host = {
                 "name": name,
-                "addrs": getattr(data, "addrs", ...),
+                "addrs": [item.addr for item in getattr(data, "addrs", [])],
                 "cr_date": getattr(data, "cr_date", ...),
                 "statuses": getattr(data, "statuses", ...),
                 "tr_date": getattr(data, "tr_date", ...),
@@ -1539,10 +1553,9 @@ class Domain(TimeStampedModel, DomainHelper):
             return []
 
         for ip_addr in ip_list:
-            if self.is_ipv6(ip_addr):
-                edited_ip_list.append(epp.Ip(addr=ip_addr, ip="v6"))
-            else:  # default ip addr is v4
-                edited_ip_list.append(epp.Ip(addr=ip_addr))
+            edited_ip_list.append(
+                epp.Ip(addr=ip_addr, ip="v6" if self.is_ipv6(ip_addr) else None)
+            )
 
         return edited_ip_list
 
@@ -1580,7 +1593,7 @@ class Domain(TimeStampedModel, DomainHelper):
             return response.code
         except RegistryError as e:
             logger.error("Error _update_host, code was %s error was %s" % (e.code, e))
-            return e.code
+            raise e
 
     def addAndRemoveHostsFromDomain(
         self, hostsToAdd: list[str], hostsToDelete: list[str]

src/registrar/templates/domain_dnssec.html

@@ -7,7 +7,7 @@
 
   <h1>DNSSEC</h1>
 
-  <p>DNSSEC, or DNS Security Extensions, is additional security layer to protect your website. Enabling DNSSEC ensures that when someone visits your website, they can be certain that it’s connecting to the correct server, preventing potential hijacking or tampering with your domain's records.</p>
+  <p>DNSSEC, or DNS Security Extensions, is an additional security layer to protect your website. Enabling DNSSEC ensures that when someone visits your domain, they can be certain that it’s connecting to the correct server, preventing potential hijacking or tampering with your domain's records.</p>
 
   <form class="usa-form usa-form--text-width" method="post">
     {% csrf_token %}

src/registrar/templates/domain_dsdata.html

@@ -29,7 +29,7 @@
     {{ formset.management_form }}
 
     {% for form in formset %}
-    <fieldset class="ds-record">
+    <fieldset class="repeatable-form">
 
       <legend class="sr-only">DS Data record {{forloop.counter}}</legend>
 
@@ -74,7 +74,7 @@
     </fieldset>
     {% endfor %}
 
-    <button type="button" class="usa-button usa-button--unstyled display-block margin-bottom-2" id="add-ds-form">
+    <button type="button" class="usa-button usa-button--unstyled display-block margin-bottom-2" id="add-form">
       <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24" height="24">
         <use xlink:href="{%static 'img/sprite.svg'%}#add_circle"></use>
       </svg><span class="margin-left-05">Add new record</span>

src/registrar/templates/domain_nameservers.html

@@ -11,40 +11,79 @@
 
   <h1>DNS name servers</h1>
 
-  <p>Before your domain can be used we'll need information about your domain
-  name servers.</p>
+  <p>Before your domain can be used we’ll need information about your domain name servers. Name server records indicate which DNS server is authoritative for your domain.</p>
+
+  <p>Add a name server record by entering the address (e.g., ns1.nameserver.com) in the name server fields below. You must add at least two name servers (13 max).</p>
+
+  <div class="usa-alert usa-alert--slim usa-alert--info">
+    <div class="usa-alert__body">
+      <p class="margin-top-0">Add an IP address only when your name server's address includes your domain name (e.g., if your domain name is "example.gov" and your name server is "ns1.example.gov,” then an IP address is required.) To add multiple IP addresses, separate them with commas.</p>
+      <p class="margin-bottom-0">This step is uncommon unless you self-host your DNS or use custom addresses for your nameserver.</p>
+    </div>
+  </div>
 
   {% include "includes/required_fields.html" %}
 
-  <form class="usa-form usa-form--large" method="post" novalidate id="form-container">
+  <form class="usa-form usa-form--extra-large" method="post" novalidate id="form-container">
     {% csrf_token %}
     {{ formset.management_form }}
 
     {% for form in formset %}
-    <div class="server-form">
-    {% with sublabel_text="Example: ns"|concat:forloop.counter|concat:".example.com" %}
-      {% if forloop.counter <= 2 %}
-        {% with attr_required=True %}
-          {% input_with_errors form.server %}
-        {% endwith %}
-      {% else %}
-        {% input_with_errors form.server %}
-      {% endif %}
-    {% endwith %}
+    <div class="repeatable-form">
+      <div class="grid-row grid-gap-2 flex-end">
+        <div class="tablet:grid-col-5">
+          {{ form.domain }}
+          {% with sublabel_text="Example: ns"|concat:forloop.counter|concat:".example.com" %}
+            {% if forloop.counter <= 2 %}
+              {% with attr_required=True add_group_class="usa-form-group--unstyled-error" %}
+                {% input_with_errors form.server %}
+              {% endwith %}
+            {% else %}
+              {% input_with_errors form.server %}
+            {% endif %}
+          {% endwith %}
+        </div>
+        <div class="tablet:grid-col-5">
+          {% with sublabel_text="Example: 86.124.49.54 or 2001:db8::1234:5678" add_group_class="usa-form-group--unstyled-error" %}
+            {% input_with_errors form.ip %}
+          {% endwith %}
+        </div>
+        <div class="tablet:grid-col-2">
+          {% comment %} TODO: remove this if for 1192 {% endcomment %}
+          {% if forloop.counter > 2 %}
+            <button type="button" class="usa-button usa-button--unstyled display-block delete-record margin-bottom-075">
+              <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24" height="24">
+                <use xlink:href="{%static 'img/sprite.svg'%}#delete"></use>
+              </svg><span class="margin-left-05">Delete</span>
+            </button>
+          {% endif %}
+        </div>
+      </div>
     </div>
     {% endfor %}
 
-    <button type="button" class="usa-button usa-button--unstyled display-block" id="add-nameserver-form">
+    <button type="button" class="usa-button usa-button--unstyled display-block" id="add-form">
       <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24" height="24">
         <use xlink:href="{%static 'img/sprite.svg'%}#add_circle"></use>
       </svg><span class="margin-left-05">Add another name server</span>
     </button>
 
-    <button
-          type="submit"
-          class="usa-button"
-      >Save
-    </button>
-  </form>
+    {% comment %} Work around USWDS' button margins to add some spacing between the submit and the 'add more'
+    This solution still works when we remove the 'add more' at 13 forms {% endcomment %}
+    <div class="margin-top-2">
+      <button
+            type="submit"
+            class="usa-button"
+        >Save
+      </button>
 
+      <button
+            type="submit"
+            class="usa-button usa-button--outline"
+            name="btn-cancel-click"
+            aria-label="Reset the data in the Name Server form to the registry state (undo changes)"
+        >Cancel
+      </button>
+    </div>
+  </form>
 {% endblock %}  {# domain_content #}

src/registrar/tests/common.py

@@ -756,7 +756,7 @@ class MockEppLib(TestCase):
     mockDataInfoHosts = fakedEppObject(
         "lastPw",
         cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35),
-        addrs=["1.2.3.4", "2.3.4.5"],
+        addrs=[common.Ip(addr="1.2.3.4"), common.Ip(addr="2.3.4.5")],
     )
 
     mockDataHostChange = fakedEppObject(
@@ -813,7 +813,7 @@ class MockEppLib(TestCase):
             "ns2.nameserverwithip.gov",
             "ns3.nameserverwithip.gov",
         ],
-        addrs=["1.2.3.4", "2.3.4.5"],
+        addrs=[common.Ip(addr="1.2.3.4"), common.Ip(addr="2.3.4.5")],
     )
 
     justNameserver = fakedEppObject(

src/registrar/tests/test_models_domain.py

@@ -107,7 +107,7 @@ class TestDomainCache(MockEppLib):
         }
         expectedHostsDict = {
             "name": self.mockDataInfoDomain.hosts[0],
-            "addrs": self.mockDataInfoHosts.addrs,
+            "addrs": [item.addr for item in self.mockDataInfoHosts.addrs],
             "cr_date": self.mockDataInfoHosts.cr_date,
         }
 

src/registrar/tests/test_nameserver_error.py

@@ -10,10 +10,7 @@ class TestNameserverError(TestCase):
     def test_with_no_ip(self):
         """Test NameserverError when no ip address is passed"""
         nameserver = "nameserver val"
-        expected = (
-            f"Nameserver {nameserver} needs to have an "
-            "IP address because it is a subdomain"
-        )
+        expected = "Using your domain for a name server requires an IP address"
 
         nsException = NameserverError(
             code=nsErrorCodes.MISSING_IP, nameserver=nameserver
@@ -38,7 +35,7 @@ class TestNameserverError(TestCase):
         ip = "ip val"
         nameserver = "nameserver val"
 
-        expected = f"Nameserver {nameserver} has an invalid IP address: {ip}"
+        expected = f"{nameserver}: Enter an IP address in the required format."
         nsException = NameserverError(
             code=nsErrorCodes.INVALID_IP, nameserver=nameserver, ip=ip
         )

src/registrar/tests/test_transition_domain_migrations.py

@@ -1,4 +1,3 @@
-from unittest.mock import patch
 from django.test import TestCase
 
 from registrar.models import (
@@ -10,9 +9,6 @@ from registrar.models import (
     UserDomainRole,
 )
 
-from registrar.management.commands.master_domain_migrations import (
-    Command as master_migration_command,
-)
 from django.core.management import call_command
 
 
@@ -52,7 +48,7 @@ class TestLogins(TestCase):
         call_command("transfer_transition_domains_to_domains")
 
     def run_master_script(self):
-        command = call_command(
+        call_command(
             "master_domain_migrations",
             runMigrations=True,
             migrationDirectory=f"{self.test_data_file_location}",
@@ -154,23 +150,8 @@ class TestLogins(TestCase):
 
         self.run_master_script()
 
-        # # TODO: instead of patching....there has got to be a way of making sure subsequent commands use the django database
-        # # Patch subroutines for migrations
-        # def side_effect():
-        #     self.run_load_domains()
-        #     self.run_transfer_domains()
-        # patcher = patch("registrar.management.commands.master_domain_migrations.Command.run_migration_scripts")
-        # mocked_get = patcher.start()
-        # mocked_get.side_effect = side_effect
-        # # Patch subroutines for sending invitations
-        # def side_effect():
-        #     # TODO: what should happen here?
-        #     return
-        # patcher = patch("registrar.management.commands.master_domain_migrations.Command.run_send_invites_script")
-        # mocked_get = patcher.start()
-        # mocked_get.side_effect = side_effect
-
-        # STEP 2: (analyze the tables just like the migration script does, but add assert statements)
+        # STEP 2: (analyze the tables just like the
+        # migration script does, but add assert statements)
         expected_total_transition_domains = 8
         expected_total_domains = 4
         expected_total_domain_informations = 0
@@ -178,7 +159,8 @@ class TestLogins(TestCase):
 
         expected_missing_domains = 0
         expected_duplicate_domains = 0
-        # we expect 8 missing domain invites since the migration does not auto-login new users
+        # we expect 8 missing domain invites since the
+        # migration does not auto-login new users
         expected_missing_domain_informations = 8
         # we expect 1 missing invite from anomaly.gov (an injected error)
         expected_missing_domain_invitations = 1
@@ -196,7 +178,8 @@ class TestLogins(TestCase):
     def test_load_transition_domain(self):
         self.run_load_domains()
 
-        # STEP 2: (analyze the tables just like the migration script does, but add assert statements)
+        # STEP 2: (analyze the tables just like the migration
+        # script does, but add assert statements)
         expected_total_transition_domains = 8
         expected_total_domains = 0
         expected_total_domain_informations = 0

src/registrar/tests/test_views.py

@@ -10,6 +10,10 @@ from .common import MockEppLib, completed_application  # type: ignore
 from django_webtest import WebTest  # type: ignore
 import boto3_mocking  # type: ignore
 
+from registrar.utility.errors import (
+    NameserverError,
+    NameserverErrorCodes,
+)
 
 from registrar.models import (
     DomainApplication,
@@ -1442,20 +1446,165 @@ class TestDomainNameservers(TestDomainOverview):
         )
         self.assertContains(page, "DNS name servers")
 
-    @skip("Broken by adding registry connection fix in ticket 848")
-    def test_domain_nameservers_form(self):
-        """Can change domain's nameservers.
+    def test_domain_nameservers_form_submit_one_nameserver(self):
+        """Nameserver form submitted with one nameserver throws error.
 
         Uses self.app WebTest because we need to interact with forms.
         """
+        # initial nameservers page has one server with two ips
         nameservers_page = self.app.get(
             reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
         )
         session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
         self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form with only one nameserver, should error
+        # regarding required fields
         with less_console_noise():  # swallow log warning message
             result = nameservers_page.form.submit()
-        # form submission was a post, response should be a redirect
+        # form submission was a post with an error, response should be a 200
+        # error text appears twice, once at the top of the page, once around
+        # the required field.  form requires a minimum of 2 name servers
+        self.assertContains(result, "This field is required.", count=2, status_code=200)
+
+    def test_domain_nameservers_form_submit_subdomain_missing_ip(self):
+        """Nameserver form catches missing ip error on subdomain.
+
+        Uses self.app WebTest because we need to interact with forms.
+        """
+        # initial nameservers page has one server with two ips
+        nameservers_page = self.app.get(
+            reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
+        )
+        session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form without two hosts, both subdomains,
+        # only one has ips
+        nameservers_page.form["form-1-server"] = "ns2.igorville.gov"
+        with less_console_noise():  # swallow log warning message
+            result = nameservers_page.form.submit()
+        # form submission was a post with an error, response should be a 200
+        # error text appears twice, once at the top of the page, once around
+        # the required field.  subdomain missing an ip
+        self.assertContains(
+            result,
+            str(NameserverError(code=NameserverErrorCodes.MISSING_IP)),
+            count=2,
+            status_code=200,
+        )
+
+    def test_domain_nameservers_form_submit_missing_host(self):
+        """Nameserver form catches error when host is missing.
+
+        Uses self.app WebTest because we need to interact with forms.
+        """
+        # initial nameservers page has one server with two ips
+        nameservers_page = self.app.get(
+            reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
+        )
+        session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form without two hosts, both subdomains,
+        # only one has ips
+        nameservers_page.form["form-1-ip"] = "127.0.0.1"
+        with less_console_noise():  # swallow log warning message
+            result = nameservers_page.form.submit()
+        # form submission was a post with an error, response should be a 200
+        # error text appears twice, once at the top of the page, once around
+        # the required field.  nameserver has ip but missing host
+        self.assertContains(
+            result,
+            str(NameserverError(code=NameserverErrorCodes.MISSING_HOST)),
+            count=2,
+            status_code=200,
+        )
+
+    def test_domain_nameservers_form_submit_glue_record_not_allowed(self):
+        """Nameserver form catches error when IP is present
+        but host not subdomain.
+
+        Uses self.app WebTest because we need to interact with forms.
+        """
+        nameserver1 = "ns1.igorville.gov"
+        nameserver2 = "ns2.igorville.com"
+        valid_ip = "127.0.0.1"
+        # initial nameservers page has one server with two ips
+        nameservers_page = self.app.get(
+            reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
+        )
+        session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form without two hosts, both subdomains,
+        # only one has ips
+        nameservers_page.form["form-0-server"] = nameserver1
+        nameservers_page.form["form-1-server"] = nameserver2
+        nameservers_page.form["form-1-ip"] = valid_ip
+        with less_console_noise():  # swallow log warning message
+            result = nameservers_page.form.submit()
+        # form submission was a post with an error, response should be a 200
+        # error text appears twice, once at the top of the page, once around
+        # the required field.  nameserver has ip but missing host
+        self.assertContains(
+            result,
+            str(NameserverError(code=NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED)),
+            count=2,
+            status_code=200,
+        )
+
+    def test_domain_nameservers_form_submit_invalid_ip(self):
+        """Nameserver form catches invalid IP on submission.
+
+        Uses self.app WebTest because we need to interact with forms.
+        """
+        nameserver = "ns2.igorville.gov"
+        invalid_ip = "123"
+        # initial nameservers page has one server with two ips
+        nameservers_page = self.app.get(
+            reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
+        )
+        session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form without two hosts, both subdomains,
+        # only one has ips
+        nameservers_page.form["form-1-server"] = nameserver
+        nameservers_page.form["form-1-ip"] = invalid_ip
+        with less_console_noise():  # swallow log warning message
+            result = nameservers_page.form.submit()
+        # form submission was a post with an error, response should be a 200
+        # error text appears twice, once at the top of the page, once around
+        # the required field.  nameserver has ip but missing host
+        self.assertContains(
+            result,
+            str(
+                NameserverError(
+                    code=NameserverErrorCodes.INVALID_IP, nameserver=nameserver
+                )
+            ),
+            count=2,
+            status_code=200,
+        )
+
+    def test_domain_nameservers_form_submits_successfully(self):
+        """Nameserver form submits successfully with valid input.
+
+        Uses self.app WebTest because we need to interact with forms.
+        """
+        nameserver1 = "ns1.igorville.gov"
+        nameserver2 = "ns2.igorville.gov"
+        invalid_ip = "127.0.0.1"
+        # initial nameservers page has one server with two ips
+        nameservers_page = self.app.get(
+            reverse("domain-dns-nameservers", kwargs={"pk": self.domain.id})
+        )
+        session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        # attempt to submit the form without two hosts, both subdomains,
+        # only one has ips
+        nameservers_page.form["form-0-server"] = nameserver1
+        nameservers_page.form["form-1-server"] = nameserver2
+        nameservers_page.form["form-1-ip"] = invalid_ip
+        with less_console_noise():  # swallow log warning message
+            result = nameservers_page.form.submit()
+        # form submission was a successful post, response should be a 302
         self.assertEqual(result.status_code, 302)
         self.assertEqual(
             result["Location"],
@@ -1465,9 +1614,8 @@ class TestDomainNameservers(TestDomainOverview):
         page = result.follow()
         self.assertContains(page, "The name servers for this domain have been updated")
 
-    @skip("Broken by adding registry connection fix in ticket 848")
     def test_domain_nameservers_form_invalid(self):
-        """Can change domain's nameservers.
+        """Nameserver form does not submit with invalid data.
 
         Uses self.app WebTest because we need to interact with forms.
         """
@@ -1482,9 +1630,9 @@ class TestDomainNameservers(TestDomainOverview):
         with less_console_noise():  # swallow logged warning message
             result = nameservers_page.form.submit()
         # form submission was a post with an error, response should be a 200
-        # error text appears twice, once at the top of the page, once around
-        # the field.
-        self.assertContains(result, "This field is required", count=2, status_code=200)
+        # error text appears four times, twice at the top of the page,
+        # once around each required field.
+        self.assertContains(result, "This field is required", count=4, status_code=200)
 
 
 class TestDomainAuthorizingOfficial(TestDomainOverview):

src/registrar/utility/errors.py

@@ -20,6 +20,41 @@ class ActionNotAllowed(Exception):
     pass
 
 
+class GenericErrorCodes(IntEnum):
+    """Used across the registrar for
+    error mapping.
+    Overview of generic error codes:
+        - 1 GENERIC_ERROR a generic value error
+        - 2 CANNOT_CONTACT_REGISTRY a connection error w registry
+    """
+
+    GENERIC_ERROR = 1
+    CANNOT_CONTACT_REGISTRY = 2
+
+
+class GenericError(Exception):
+    """
+    GenericError class used to raise exceptions across
+    the registrar
+    """
+
+    _error_mapping = {
+        GenericErrorCodes.CANNOT_CONTACT_REGISTRY: (
+            "Update failed. Cannot contact the registry."
+        ),
+        GenericErrorCodes.GENERIC_ERROR: ("Value entered was wrong."),
+    }
+
+    def __init__(self, *args, code=None, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.code = code
+        if self.code in self._error_mapping:
+            self.message = self._error_mapping.get(self.code)
+
+    def __str__(self):
+        return f"{self.message}"
+
+
 class NameserverErrorCodes(IntEnum):
     """Used in the NameserverError class for
     error mapping.
@@ -29,6 +64,8 @@ class NameserverErrorCodes(IntEnum):
                                     value but is not a subdomain
         - 3 INVALID_IP  invalid ip address format or invalid version
         - 4 TOO_MANY_HOSTS  more than the max allowed host values
+        - 5 UNABLE_TO_UPDATE_DOMAIN unable to update the domain
+        - 6 MISSING_HOST host is missing for a nameserver
     """
 
     MISSING_IP = 1
@@ -36,6 +73,7 @@ class NameserverErrorCodes(IntEnum):
     INVALID_IP = 3
     TOO_MANY_HOSTS = 4
     UNABLE_TO_UPDATE_DOMAIN = 5
+    MISSING_HOST = 6
 
 
 class NameserverError(Exception):
@@ -45,11 +83,15 @@ class NameserverError(Exception):
     """
 
     _error_mapping = {
-        NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an "
-        "IP address because it is a subdomain",
-        NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked "
-        "because it is not a subdomain",
-        NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}",
+        NameserverErrorCodes.MISSING_IP: (
+            "Using your domain for a name server requires an IP address"
+        ),
+        NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: (
+            "Name server address does not match domain name"
+        ),
+        NameserverErrorCodes.INVALID_IP: (
+            "{}: Enter an IP address in the required format."
+        ),
         NameserverErrorCodes.TOO_MANY_HOSTS: (
             "Too many hosts provided, you may not have more than 13 nameservers."
         ),
@@ -57,6 +99,9 @@ class NameserverError(Exception):
             "Unable to update domain, changes were not applied."
             "Check logs as a Registry Error is the likely cause"
         ),
+        NameserverErrorCodes.MISSING_HOST: (
+            "Name server must be provided to enter IP address."
+        ),
     }
 
     def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs):
@@ -65,7 +110,7 @@ class NameserverError(Exception):
         if self.code in self._error_mapping:
             self.message = self._error_mapping.get(self.code)
             if nameserver is not None and ip is not None:
-                self.message = self.message.format(str(nameserver), str(ip))
+                self.message = self.message.format(str(nameserver))
             elif nameserver is not None:
                 self.message = self.message.format(str(nameserver))
             elif ip is not None:

src/registrar/views/domain.py

@@ -23,6 +23,12 @@ from registrar.models import (
     UserDomainRole,
 )
 from registrar.models.public_contact import PublicContact
+from registrar.utility.errors import (
+    GenericError,
+    GenericErrorCodes,
+    NameserverError,
+    NameserverErrorCodes as nsErrorCodes,
+)
 from registrar.models.utility.contact_error import ContactError
 
 from ..forms import (
@@ -40,8 +46,6 @@ from epplibwrapper import (
     common,
     extensions,
     RegistryError,
-    CANNOT_CONTACT_REGISTRY,
-    GENERIC_ERROR,
 )
 
 from ..utility.email import send_templated_email, EmailSendingError
@@ -215,6 +219,7 @@ class DomainNameserversView(DomainFormBaseView):
 
     template_name = "domain_nameservers.html"
     form_class = NameserverFormset
+    model = Domain
 
     def get_initial(self):
         """The initial value for the form (which is a formset here)."""
@@ -223,7 +228,9 @@ class DomainNameserversView(DomainFormBaseView):
 
         if nameservers is not None:
             # Add existing nameservers as initial data
-            initial_data.extend({"server": name} for name, *ip in nameservers)
+            initial_data.extend(
+                {"server": name, "ip": ",".join(ip)} for name, ip in nameservers
+            )
 
         # Ensure at least 3 fields, filled or empty
         while len(initial_data) < 2:
@@ -252,25 +259,82 @@ class DomainNameserversView(DomainFormBaseView):
                 form.fields["server"].required = True
             else:
                 form.fields["server"].required = False
+            form.fields["domain"].initial = self.object.name
         return formset
 
+    def post(self, request, *args, **kwargs):
+        """Form submission posts to this view.
+
+        This post method harmonizes using DomainBaseView and FormMixin
+        """
+        self._get_domain(request)
+        formset = self.get_form()
+
+        if "btn-cancel-click" in request.POST:
+            url = self.get_success_url()
+            return HttpResponseRedirect(url)
+
+        if formset.is_valid():
+            return self.form_valid(formset)
+        else:
+            return self.form_invalid(formset)
+
     def form_valid(self, formset):
         """The formset is valid, perform something with it."""
 
+        self.request.session["nameservers_form_domain"] = self.object
+
         # Set the nameservers from the formset
         nameservers = []
         for form in formset:
             try:
-                as_tuple = (form.cleaned_data["server"],)
+                ip_string = form.cleaned_data["ip"]
+                # ip_string will be None or a string of IP addresses
+                # comma-separated
+                ip_list = []
+                if ip_string:
+                    # Split the string into a list using a comma as the delimiter
+                    ip_list = ip_string.split(",")
+
+                as_tuple = (
+                    form.cleaned_data["server"],
+                    ip_list,
+                )
                 nameservers.append(as_tuple)
             except KeyError:
                 # no server information in this field, skip it
                 pass
-        self.object.nameservers = nameservers
 
-        messages.success(
-            self.request, "The name servers for this domain have been updated."
-        )
+        try:
+            self.object.nameservers = nameservers
+        except NameserverError as Err:
+            # NamserverErrors *should* be caught in form; if reached here,
+            # there was an uncaught error in submission (through EPP)
+            messages.error(
+                self.request, NameserverError(code=nsErrorCodes.UNABLE_TO_UPDATE_DOMAIN)
+            )
+            logger.error(f"Nameservers error: {Err}")
+        # TODO: registry is not throwing an error when no connection
+        except RegistryError as Err:
+            if Err.is_connection_error():
+                messages.error(
+                    self.request,
+                    GenericError(code=GenericErrorCodes.CANNOT_CONTACT_REGISTRY),
+                )
+                logger.error(f"Registry connection error: {Err}")
+            else:
+                messages.error(
+                    self.request, GenericError(code=GenericErrorCodes.GENERIC_ERROR)
+                )
+                logger.error(f"Registry error: {Err}")
+        else:
+            messages.success(
+                self.request,
+                "The name servers for this domain have been updated. "
+                "Keep in mind that DNS changes may take some time to "
+                "propagate across the internet. It can take anywhere "
+                "from a few minutes to 48 hours for your changes to take place.",
+            )
 
         # superclass has the redirect
         return super().form_valid(formset)
@@ -431,10 +495,15 @@ class DomainDsDataView(DomainFormBaseView):
             self.object.dnssecdata = dnssecdata
         except RegistryError as err:
             if err.is_connection_error():
-                messages.error(self.request, CANNOT_CONTACT_REGISTRY)
+                messages.error(
+                    self.request,
+                    GenericError(code=GenericErrorCodes.CANNOT_CONTACT_REGISTRY),
+                )
                 logger.error(f"Registry connection error: {err}")
             else:
-                messages.error(self.request, GENERIC_ERROR)
+                messages.error(
+                    self.request, GenericError(code=GenericErrorCodes.GENERIC_ERROR)
+                )
                 logger.error(f"Registry error: {err}")
             return self.form_invalid(formset)
         else:
@@ -510,7 +579,10 @@ class DomainSecurityEmailView(DomainFormBaseView):
         # If no default is created for security_contact,
         # then we cannot connect to the registry.
         if contact is None:
-            messages.error(self.request, CANNOT_CONTACT_REGISTRY)
+            messages.error(
+                self.request,
+                GenericError(code=GenericErrorCodes.CANNOT_CONTACT_REGISTRY),
+            )
             return redirect(self.get_success_url())
 
         contact.email = new_email
@@ -519,13 +591,20 @@ class DomainSecurityEmailView(DomainFormBaseView):
             contact.save()
         except RegistryError as Err:
             if Err.is_connection_error():
-                messages.error(self.request, CANNOT_CONTACT_REGISTRY)
+                messages.error(
+                    self.request,
+                    GenericError(code=GenericErrorCodes.CANNOT_CONTACT_REGISTRY),
+                )
                 logger.error(f"Registry connection error: {Err}")
             else:
-                messages.error(self.request, GENERIC_ERROR)
+                messages.error(
+                    self.request, GenericError(code=GenericErrorCodes.GENERIC_ERROR)
+                )
                 logger.error(f"Registry error: {Err}")
         except ContactError as Err:
-            messages.error(self.request, GENERIC_ERROR)
+            messages.error(
+                self.request, GenericError(code=GenericErrorCodes.GENERIC_ERROR)
+            )
             logger.error(f"Generic registry error: {Err}")
         else:
             messages.success(