From 1cfa08a83aa97a179bb752a47e99839366d0e92a Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 6 Jun 2024 15:48:00 -0700 Subject: [PATCH 01/32] started adr --- .../decisions/0026-django-waffle-library.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 docs/architecture/decisions/0026-django-waffle-library.md diff --git a/docs/architecture/decisions/0026-django-waffle-library.md b/docs/architecture/decisions/0026-django-waffle-library.md new file mode 100644 index 000000000..878cb81ed --- /dev/null +++ b/docs/architecture/decisions/0026-django-waffle-library.md @@ -0,0 +1,28 @@ +# 24. Production Release Cadence + +Date: 2024-07-06 + +## Status + +Approved + +## Context + + + +## Considered Options + +**Option 1:** Releasing to stable/staging once a sprint + +**Option 2:** Releasing to stable/staging once a week + + +In both of the above scenarios, the release date would fall on the same day of the week that the sprint starts which is currently a Wednesday. Additionally, in both scenarios the release commits would eventually be tagged with both a staging and stable tag. Furthermore, critical bugs or features would be exempt from these restrictions based on the product owner's discretion. + +## Decision + + + +## Consequences + + From bfc3b74283843ae48cfec6f8c319e2b163236a2e Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 13 Jun 2024 14:10:15 -0700 Subject: [PATCH 02/32] updated adr --- .../decisions/0026-django-waffle-library.md | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/architecture/decisions/0026-django-waffle-library.md b/docs/architecture/decisions/0026-django-waffle-library.md index 878cb81ed..a8dbfccac 100644 --- a/docs/architecture/decisions/0026-django-waffle-library.md +++ b/docs/architecture/decisions/0026-django-waffle-library.md @@ -8,16 +8,41 @@ Approved ## Context +We release finished code twice a week allowing features to reach users fast. However, several upcoming features required a series of changes that would need to be done over a couple sprints and should not display to users until we are all done. Thus, if we followed our normal process, users would see half-finished features. + +At the same time, some of these features should only be turned on for users upon request (and likely during user research). We would want a way for our CISA users to be able to turn this feature on and off for people without requiring a lengthy process or code changes. + +This brought us to finding solutions that could fix one or both of these problems. ## Considered Options -**Option 1:** Releasing to stable/staging once a sprint +**Option 1:** Environment variables +Environment variables would allow implementation over multiple sprints, by allowing us to set a true or false value to the given variable. When the varaible is on (true) the feature shows, otherwise it remains hidden. ANother benefit is that it is free and we already use environment variables for other things in our code. -**Option 2:** Releasing to stable/staging once a week +The down side, is that to see what the current setting is on a sandbox you would need to go to cloud.gov or use the cf cli to see the current settings. This is very technical, meaning only developers would really be able to see what features were set and we would be the only ones to adjust them. It would also be really easy to accidentally have the feature on or off without noticing. This also would not solve the problem of being able to turn features on and off easily for a given user group. +**Option 2:** Feature branches +Like environrment variables, using feature branches would be free and allow us to iterate on development of big features over multiple sprints. We would make a feature branch that developers working on that feature would push and pull from to iterate on. This quickly brings us to the downsides of this approach. + +WIth feature branches we do not solve the problem of being able to turn features on and off easily for a user group. More importantly, by working in a seperated branch for more than a sprint we easily run the risk of having out of sync migrations and merge conflicts that would slow down development time and cause frustration. Out of sync migrations can also cause technical issues on sandboxes as well, which further adds to development frustration. + +**Option 3:** Feature flags +Feature flags are free, allow us to implement thigns over multiple sprints, and some libar + +Waffle feature flag libary + + +**Option 3a:** Feature flags with waffle +The Waffle feature flag library is a highly reccomended django libary for handling large features. It has clear documentation on how to turn on feature flags for user groups as this is one of the main problems it attempts to solve. It also provides Samples which can turn on flags for a certain percent of users and Switches which can be used to hollistically turn features on and off. The reviews from those who used it were highly favorable, some even mentioning how it beat out competitors like gargoyl. It's also comaptible with django admin, providing a quick way to add the view of the flags in django admin so any user with admin access can modify flags for their sandbox. + + +**Option 3b:** Feature flags with gargoyl +Another feature flag libary with django, but with more forks of the libary, less consitent maintanence and reviews saying it wasn't as easy to work with as waffle. The reviews and less robust documentation were immediatly huge cons to this as an option + +**Option 3c:** Paid feature flag system with github integration- LaunchDarkly +LaunchDarkly is a Fedramp'd solution with excellent reviews for controlling feature flags straight from github to promote any team member to be in easy control of setting feature flags. However, the big con to this was that it would be a paid solution and would take time to procure thus slowing down our ability to start on these large features. -In both of the above scenarios, the release date would fall on the same day of the week that the sprint starts which is currently a Wednesday. Additionally, in both scenarios the release commits would eventually be tagged with both a staging and stable tag. Furthermore, critical bugs or features would be exempt from these restrictions based on the product owner's discretion. ## Decision From cdaf4afd2a72c4f4504a69a54505807be4d3afe1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 17 Jun 2024 14:43:06 -0600 Subject: [PATCH 03/32] script --- .../commands/transfer_federal_agency.py | 35 ++++++++ .../commands/utility/terminal_helper.py | 86 ++++++++++++------- 2 files changed, 92 insertions(+), 29 deletions(-) create mode 100644 src/registrar/management/commands/transfer_federal_agency.py diff --git a/src/registrar/management/commands/transfer_federal_agency.py b/src/registrar/management/commands/transfer_federal_agency.py new file mode 100644 index 000000000..3be7d452f --- /dev/null +++ b/src/registrar/management/commands/transfer_federal_agency.py @@ -0,0 +1,35 @@ +import logging +from django.core.management import BaseCommand +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors +from registrar.models import FederalAgency, DomainRequest +from registrar.models.utility.generic_helper import convert_queryset_to_dict + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand, PopulateScriptTemplate): + help = "Loops through each valid User object and updates its verification_type value" + prompt_title = "Do you wish to update all Federal Agencies?" + + def handle(self, **kwargs): + """Loops through each valid User object and updates its verification_type value""" + + # Get all existing domain requests + self.all_domain_requests = DomainRequest.objects.select_related("federal_agency").distinct() + + filter_condition = { + "agency__isnull": False, + } + updated_fields = ["federal_type"] + self.mass_update_records(FederalAgency, filter_condition, updated_fields) + + def update_record(self, record: FederalAgency): + """Defines how we update the federal_type field""" + request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() + record.federal_type = request.federal_type + + def should_skip_record(self, record) -> bool: # noqa + """Defines the conditions in which we should skip updating a record.""" + request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() + return not request or not request.federal_agency + diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index db3e4a9d3..bc5b0cc8c 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -64,53 +64,77 @@ class PopulateScriptTemplate(ABC): Contains an ABC for generic populate scripts """ - def mass_populate_field(self, sender, filter_conditions, fields_to_update): - """Loops through each valid "sender" object - specified by filter_conditions - and - updates fields defined by fields_to_update using populate_function. + prompt_title = "Do you wish to proceed?" - You must define populate_field before you can use this function. + def mass_update_records(self, sender, filter_conditions, fields_to_update, debug=True): + """Loops through each valid "sender" object - specified by filter_conditions - and + updates fields defined by fields_to_update using update_record. + + You must define update_record before you can use this function. """ - objects = sender.objects.filter(**filter_conditions) + records = sender.objects.filter(**filter_conditions) + readable_class_name = self.get_class_name(sender) # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, info_to_inspect=f""" ==Proposed Changes== - Number of {sender} objects to change: {len(objects)} + Number of {readable_class_name} objects to change: {len(records)} These fields will be updated on each record: {fields_to_update} """, - prompt_title="Do you wish to patch this data?", + prompt_title=self.prompt_title, ) logger.info("Updating...") to_update: List[sender] = [] + to_skip: List[sender] = [] failed_to_update: List[sender] = [] - for updated_object in objects: + for record in records: try: - self.populate_field(updated_object) - to_update.append(updated_object) + if not self.should_skip_record(record): + self.update_record(record) + to_update.append(record) + else: + to_skip.append(record) except Exception as err: - failed_to_update.append(updated_object) + fail_message = self.get_failure_message(record) + failed_to_update.append(record) logger.error(err) - logger.error(f"{TerminalColors.FAIL}" f"Failed to update {updated_object}" f"{TerminalColors.ENDC}") + logger.error(fail_message) - # Do a bulk update on the first_ready field + # Do a bulk update on the desired field ScriptDataHelper.bulk_update_fields(sender, to_update, fields_to_update) # Log what happened - TerminalHelper.log_script_run_summary(to_update, failed_to_update, skipped=[], debug=True) + TerminalHelper.log_script_run_summary(to_update, failed_to_update, to_skip, debug, display_as_str=True) + + def get_class_name(self, sender) -> str: + """Returns the class name that we want to display for the terminal prompt. + Example: DomainRequest => "Domain Request" + """ + return sender._meta.verbose_name if getattr(sender, "_meta") else sender + + def get_failure_message(self, record) -> str: + """Returns the message that we will display if a record fails to update""" + return f"{TerminalColors.FAIL}" f"Failed to update {record}" f"{TerminalColors.ENDC}" + + def should_skip_record(self, record) -> bool: # noqa + """Defines the conditions in which we should skip updating a record.""" + # By default - don't skip + return False @abstractmethod - def populate_field(self, field_to_update): + def update_record(self, record): """Defines how we update each field. Must be defined before using mass_populate_field.""" raise NotImplementedError + class TerminalHelper: @staticmethod - def log_script_run_summary(to_update, failed_to_update, skipped, debug: bool, log_header=None): + def log_script_run_summary(to_update, failed_to_update, skipped, debug: bool, log_header=None, display_as_str=False): """Prints success, failed, and skipped counts, as well as all affected objects.""" update_success_count = len(to_update) @@ -121,20 +145,24 @@ class TerminalHelper: log_header = "============= FINISHED ===============" # Prepare debug messages - debug_messages = { - "success": (f"{TerminalColors.OKCYAN}Updated: {to_update}{TerminalColors.ENDC}\n"), - "skipped": (f"{TerminalColors.YELLOW}Skipped: {skipped}{TerminalColors.ENDC}\n"), - "failed": (f"{TerminalColors.FAIL}Failed: {failed_to_update}{TerminalColors.ENDC}\n"), - } + if debug: + updated_display = [str(u) for u in to_update] if display_as_str else to_update + skipped_display = [str(s) for s in skipped] if display_as_str else skipped + failed_display = [str(f) for f in failed_to_update] if display_as_str else failed_to_update + debug_messages = { + "success": (f"{TerminalColors.OKCYAN}Updated: {updated_display}{TerminalColors.ENDC}\n"), + "skipped": (f"{TerminalColors.YELLOW}Skipped: {skipped_display}{TerminalColors.ENDC}\n"), + "failed": (f"{TerminalColors.FAIL}Failed: {failed_display}{TerminalColors.ENDC}\n"), + } - # Print out a list of everything that was changed, if we have any changes to log. - # Otherwise, don't print anything. - TerminalHelper.print_conditional( - debug, - f"{debug_messages.get('success') if update_success_count > 0 else ''}" - f"{debug_messages.get('skipped') if update_skipped_count > 0 else ''}" - f"{debug_messages.get('failed') if update_failed_count > 0 else ''}", - ) + # Print out a list of everything that was changed, if we have any changes to log. + # Otherwise, don't print anything. + TerminalHelper.print_conditional( + debug, + f"{debug_messages.get('success') if update_success_count > 0 else ''}" + f"{debug_messages.get('skipped') if update_skipped_count > 0 else ''}" + f"{debug_messages.get('failed') if update_failed_count > 0 else ''}", + ) if update_failed_count == 0 and update_skipped_count == 0: logger.info( From 9e57be5abadf84e68fff4aa75f7526133e3333c1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 17 Jun 2024 15:20:44 -0600 Subject: [PATCH 04/32] Add documentaiton --- docs/developer/management_script_helpers.md | 108 ++++++++++++++++++ .../commands/transfer_federal_agency.py | 19 +-- .../commands/utility/terminal_helper.py | 35 ++++-- 3 files changed, 145 insertions(+), 17 deletions(-) create mode 100644 docs/developer/management_script_helpers.md diff --git a/docs/developer/management_script_helpers.md b/docs/developer/management_script_helpers.md new file mode 100644 index 000000000..fd1a82839 --- /dev/null +++ b/docs/developer/management_script_helpers.md @@ -0,0 +1,108 @@ +# Terminal Helper Functions +`terminal_helper.py` contains utility functions to assist with common terminal and script operations. + +## TerminalColors +`TerminalColors` provides ANSI color codes as variables to style terminal output. Example usage: + +print(f"{TerminalColors.OKGREEN}Success!{TerminalColors.ENDC}") + +## ScriptDataHelper +### bulk_update_fields + +`bulk_update_fields` performs a memory-efficient bulk update on a Django model in batches using a Paginator. + +Usage: +bulk_update_fields(Domain, page.object_list, ["first_ready"]) + +## PopulateScriptTemplate + +`PopulateScriptTemplate` is an abstract base class that provides a template for creating generic populate scripts. It handles logging and bulk updating for repetitive scripts that update a few fields. + +**Disclaimer:** This template is intended as a shorthand for simple scripts. It is not recommended for complex operations. See `transfer_federal_agency.py` for a straightforward example of how to use this template. + +To use `PopulateScriptTemplate`, create a new class that inherits from it and implement the `update_record` method. This method defines how each record should be updated. + +The class provides the following optional configuration variables: +- `prompt_title`: The header displayed by `prompt_for_execution` when the script starts (default: "Do you wish to proceed?") +- `display_run_summary_items_as_str`: If True, runs `str(item)` on each item when printing the run summary for prettier output (default: False) +- `run_summary_header`: The header for the script run summary printed after the script finishes (default: None) + +The main method provided by `PopulateScriptTemplate` is `mass_update_records`. This method loops through each valid object (specified by `filter_conditions`) and updates the fields defined in `fields_to_update` using the `update_record` method. + +Before updating, `mass_update_records` prompts the user to confirm the proposed changes. If the user does not proceed, the script will exit. + +After processing the records, `mass_update_records` performs a bulk update on the specified fields using `ScriptDataHelper.bulk_update_fields` and logs a summary of the script run using `TerminalHelper.log_script_run_summary`. + +The class also provides helper methods: +- `get_class_name`: Returns a display-friendly class name for the terminal prompt +- `get_failure_message`: Returns the message to display if a record fails to update +- `should_skip_record`: Defines the condition for skipping a record (by default, no records are skipped) + +To create a script using `PopulateScriptTemplate`: +1. Create a new class that inherits from `PopulateScriptTemplate` +2. Implement the `update_record` method to define how each record should be updated +3. Optionally, override the configuration variables and helper methods as needed +4. Call `mass_update_records` within `handle` and run the script + +## TerminalHelper +### log_script_run_summary + +`log_script_run_summary` logs a summary of a script run, including counts of updated, skipped, and failed records. + +### print_conditional + +`print_conditional` conditionally logs a statement at a specified severity if a condition is met. + +### prompt_for_execution + +`prompt_for_execution` prompts the user to inspect a string and confirm if they wish to proceed. Returns True if proceeding, False if skipping, or exits the script. + +### get_file_line_count + +`get_file_line_count` returns the number of lines in a file. + +### print_to_file_conditional + +`print_to_file_conditional` conditionally writes content to a file if a condition is met. + +Refer to the source code for full function signatures and additional details. + +### query_yes_no + +`query_yes_no` prompts the user with a yes/no question and returns True for "yes" or False for "no". + +Usage: +```python +if query_yes_no("Do you want to proceed?"): + print("Proceeding...") +else: + print("Aborting.") +``` + +### query_yes_no_exit + +`query_yes_no_exit` is similar to `query_yes_no` but includes an "exit" option to terminate the script. + +Usage: +if query_yes_no_exit("Continue, abort, or exit?"): + print("Continuing...") +else: + print("Aborting.") + # Script will exit if user selected "e" for exit + +### array_as_string + +`array_as_string` converts a list of strings into a single string with each element on a new line. + +Usage: +```python +my_list = ["apple", "banana", "cherry"] +print(array_as_string(my_list)) +``` + +Output: +``` +apple +banana +cherry +``` diff --git a/src/registrar/management/commands/transfer_federal_agency.py b/src/registrar/management/commands/transfer_federal_agency.py index 3be7d452f..dd7b1e5db 100644 --- a/src/registrar/management/commands/transfer_federal_agency.py +++ b/src/registrar/management/commands/transfer_federal_agency.py @@ -1,30 +1,31 @@ import logging from django.core.management import BaseCommand -from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate from registrar.models import FederalAgency, DomainRequest -from registrar.models.utility.generic_helper import convert_queryset_to_dict + logger = logging.getLogger(__name__) +# This command uses the PopulateScriptTemplate. +# This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. +# It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each valid User object and updates its verification_type value" prompt_title = "Do you wish to update all Federal Agencies?" + display_run_summary_items_as_str = True def handle(self, **kwargs): """Loops through each valid User object and updates its verification_type value""" # Get all existing domain requests self.all_domain_requests = DomainRequest.objects.select_related("federal_agency").distinct() - - filter_condition = { - "agency__isnull": False, - } - updated_fields = ["federal_type"] - self.mass_update_records(FederalAgency, filter_condition, updated_fields) + self.mass_update_records( + FederalAgency, filter_conditions={"agency__isnull": False}, fields_to_update=["federal_type"] + ) def update_record(self, record: FederalAgency): - """Defines how we update the federal_type field""" + """Defines how we update the federal_type field on each record.""" request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() record.federal_type = request.federal_type diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index bc5b0cc8c..577423ba7 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -59,12 +59,29 @@ class ScriptDataHelper: model_class.objects.bulk_update(page.object_list, fields_to_update) + +# This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. +# It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. +# See the transfer_federal_agency.py file for example usage - its really quite simple! class PopulateScriptTemplate(ABC): """ Contains an ABC for generic populate scripts """ - prompt_title = "Do you wish to proceed?" + # Optional script-global config variables. For the most part, you can leave these untouched. + # Defines what prompt_for_execution displays as its header when you first start the script + prompt_title: str = "Do you wish to proceed?" + + # Runs str(item) over each item when printing. Use this for prettier run summaries. + display_run_summary_items_as_str: bool = False + + # The header when printing the script run summary (after the script finishes) + run_summary_header = None + + @abstractmethod + def update_record(self, record): + """Defines how we update each field. Must be defined before using mass_update_records.""" + raise NotImplementedError def mass_update_records(self, sender, filter_conditions, fields_to_update, debug=True): """Loops through each valid "sender" object - specified by filter_conditions - and @@ -108,7 +125,14 @@ class PopulateScriptTemplate(ABC): ScriptDataHelper.bulk_update_fields(sender, to_update, fields_to_update) # Log what happened - TerminalHelper.log_script_run_summary(to_update, failed_to_update, to_skip, debug, display_as_str=True) + TerminalHelper.log_script_run_summary( + to_update, + failed_to_update, + to_skip, + debug=debug, + log_header=self.run_summary_header, + display_as_str=self.display_run_summary_items_as_str, + ) def get_class_name(self, sender) -> str: """Returns the class name that we want to display for the terminal prompt. @@ -121,15 +145,10 @@ class PopulateScriptTemplate(ABC): return f"{TerminalColors.FAIL}" f"Failed to update {record}" f"{TerminalColors.ENDC}" def should_skip_record(self, record) -> bool: # noqa - """Defines the conditions in which we should skip updating a record.""" + """Defines the condition in which we should skip updating a record.""" # By default - don't skip return False - @abstractmethod - def update_record(self, record): - """Defines how we update each field. Must be defined before using mass_populate_field.""" - raise NotImplementedError - class TerminalHelper: From 689e42da1b416d27c10ba8cd0b693552f83f1253 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 20 Jun 2024 16:25:42 -0700 Subject: [PATCH 05/32] updated the documentation --- .../decisions/0026-django-waffle-library.md | 39 ++++++++----------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/docs/architecture/decisions/0026-django-waffle-library.md b/docs/architecture/decisions/0026-django-waffle-library.md index a8dbfccac..d04cab531 100644 --- a/docs/architecture/decisions/0026-django-waffle-library.md +++ b/docs/architecture/decisions/0026-django-waffle-library.md @@ -8,46 +8,39 @@ Approved ## Context -We release finished code twice a week allowing features to reach users fast. However, several upcoming features required a series of changes that would need to be done over a couple sprints and should not display to users until we are all done. Thus, if we followed our normal process, users would see half-finished features. +We release finished code twice weekly, allowing features to reach users quickly. However, several upcoming features require a series of changes that will need to be done over a few sprints and should only be displayed to users once we are all done. Thus, users would see half-finished features if we followed our standard process. -At the same time, some of these features should only be turned on for users upon request (and likely during user research). We would want a way for our CISA users to be able to turn this feature on and off for people without requiring a lengthy process or code changes. +At the same time, some of these features should only be turned on for users upon request (and likely during user research). We would want a way for our CISA users to turn this feature on and off for people without requiring a lengthy process or code changes. This brought us to finding solutions that could fix one or both of these problems. - ## Considered Options **Option 1:** Environment variables -Environment variables would allow implementation over multiple sprints, by allowing us to set a true or false value to the given variable. When the varaible is on (true) the feature shows, otherwise it remains hidden. ANother benefit is that it is free and we already use environment variables for other things in our code. +The environment allows developers to set a true or false value to the given variable, allowing implementation over multiple sprints when new features are encapsulated with this variable. The feature shows when the variable is on (true); otherwise, it remains hidden. Environment variables are also innate to Django, making them free to use; on top of that, we already use them for other things in our code. -The down side, is that to see what the current setting is on a sandbox you would need to go to cloud.gov or use the cf cli to see the current settings. This is very technical, meaning only developers would really be able to see what features were set and we would be the only ones to adjust them. It would also be really easy to accidentally have the feature on or off without noticing. This also would not solve the problem of being able to turn features on and off easily for a given user group. +The downside is that you would need to go to cloud.gov or use the cf CLI to see the current settings on a sandbox. This is very technical, meaning only developers would really be able to see what features were set, and we would be the only ones able to adjust them. It would also be easy to accidentally have the feature on or off without noticing. This also would not solve the problem of turning features on and off quickly for a given user group. **Option 2:** Feature branches -Like environrment variables, using feature branches would be free and allow us to iterate on development of big features over multiple sprints. We would make a feature branch that developers working on that feature would push and pull from to iterate on. This quickly brings us to the downsides of this approach. +Like environment variables, using feature branches would be free and allow us to iterate on developing big features over multiple sprints. We would make a feature branch that developers working on that feature would push and pull from to iterate on. This quickly brings us to the downsides of this approach. -WIth feature branches we do not solve the problem of being able to turn features on and off easily for a user group. More importantly, by working in a seperated branch for more than a sprint we easily run the risk of having out of sync migrations and merge conflicts that would slow down development time and cause frustration. Out of sync migrations can also cause technical issues on sandboxes as well, which further adds to development frustration. +Using feature branches, we do not solve the problem of being able to turn features on and off quickly for a user group. More importantly, by working in a separate branch for more than a sprint, we easily risk having out-of-sync migrations and merge conflicts that would slow development time and cause frustration. Out-of-sync migrations can also cause technical issues on sandboxes, further contributing to development frustration. **Option 3:** Feature flags -Feature flags are free, allow us to implement thigns over multiple sprints, and some libar +Feature flags are free, allowing us to implement features over multiple sprints, and some libraries can apply features based on UserGroups while even more come with an interface for non-developers to control turning feature flags on and off. Going with this decision would also entail picking the correct library or product. -Waffle feature flag libary +**Option 3a:** Feature flags with Waffle +The Waffle feature flag library is a highly recommended Django library for handling large features. It has clear documentation on turning on feature flags for user groups, which is one of the main problems it attempts to solve. It also provides "Samples" that can turn on flags for a certain percentage of users and "Switches" that can be used to turn features on and off holistically. The reviews from those who used it were highly favorable, some even mentioning how it beat out competitors like Gargoyl. It's also compatible with Django admin, providing a quick way to add the view of the flags in Django admin so any user with admin access can modify flags for their sandbox. +The repo has had new releases every year since its the creation and looks to be well maintained, with many issues on the repo referring to new feature requests. -**Option 3a:** Feature flags with waffle -The Waffle feature flag library is a highly reccomended django libary for handling large features. It has clear documentation on how to turn on feature flags for user groups as this is one of the main problems it attempts to solve. It also provides Samples which can turn on flags for a certain percent of users and Switches which can be used to hollistically turn features on and off. The reviews from those who used it were highly favorable, some even mentioning how it beat out competitors like gargoyl. It's also comaptible with django admin, providing a quick way to add the view of the flags in django admin so any user with admin access can modify flags for their sandbox. - - -**Option 3b:** Feature flags with gargoyl -Another feature flag libary with django, but with more forks of the libary, less consitent maintanence and reviews saying it wasn't as easy to work with as waffle. The reviews and less robust documentation were immediatly huge cons to this as an option - -**Option 3c:** Paid feature flag system with github integration- LaunchDarkly -LaunchDarkly is a Fedramp'd solution with excellent reviews for controlling feature flags straight from github to promote any team member to be in easy control of setting feature flags. However, the big con to this was that it would be a paid solution and would take time to procure thus slowing down our ability to start on these large features. - +**Option 3b:** Feature flags with Gargoyl +Gargoyl is another feature-flag library with Django, but it is no longer maintained, and reviews say it wasn't as easy to work with as Waffle. Using it would require forking the library, and many outstanding issues indicate bugs that need fixing. The mixed reviews from those who have done this and the less robust documentation were immediately huge cons to using this as an option. +**Option 3c:** Paid feature flag system with GitHub integration- LaunchDarkly +LaunchDarkly is a Fedramped solution with excellent reviews for controlling feature flags straight from GitHub to promote any team member easily controlling feature flags. However, the big con to this was that it would be a paid solution and would take time to procure, thus slowing down our ability to start on these significant features. We shouldn't consider LaunchDarkly because taking time to procure it would negatively affect our timeline, even if the budget was eventually approved. ## Decision - - +Option 3a, feature flags with the Django Waffle library ## Consequences - - +We are now reliant on the Waffle library for feature flags. As with any library, we would need to fork it if it ever became non-maintained with critical bugs. This doesn't seem likely in the near future, but if it occurred, we could complete the forking and fix any bug within a sprint without drastically impacting our timeline. \ No newline at end of file From d25d8f5d1f08d9daff595ee7a7c94b6038b47fae Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 21 Jun 2024 14:49:14 -0600 Subject: [PATCH 06/32] Doc --- docs/developer/management_script_helpers.md | 140 ++++++++++---------- 1 file changed, 72 insertions(+), 68 deletions(-) diff --git a/docs/developer/management_script_helpers.md b/docs/developer/management_script_helpers.md index fd1a82839..609015ad6 100644 --- a/docs/developer/management_script_helpers.md +++ b/docs/developer/management_script_helpers.md @@ -1,10 +1,12 @@ # Terminal Helper Functions -`terminal_helper.py` contains utility functions to assist with common terminal and script operations. +`terminal_helper.py` contains utility functions to assist with common terminal and script operations. +This file documents what they do and provides guidance on their usage. ## TerminalColors -`TerminalColors` provides ANSI color codes as variables to style terminal output. Example usage: +`TerminalColors` provides ANSI color codes as variables to style terminal output. -print(f"{TerminalColors.OKGREEN}Success!{TerminalColors.ENDC}") +Usage: +```print(f"{TerminalColors.OKGREEN}Success!{TerminalColors.ENDC}")``` ## ScriptDataHelper ### bulk_update_fields @@ -12,7 +14,72 @@ print(f"{TerminalColors.OKGREEN}Success!{TerminalColors.ENDC}") `bulk_update_fields` performs a memory-efficient bulk update on a Django model in batches using a Paginator. Usage: -bulk_update_fields(Domain, page.object_list, ["first_ready"]) +```bulk_update_fields(Domain, object_list, ["first_ready"])``` + +## TerminalHelper +### log_script_run_summary + +`log_script_run_summary` logs a summary of a script run, including counts of updated, skipped, and failed records. + +Usage: +```TerminalHelper.log_script_run_summary(self.to_update, self.failed_to_update, self.skipped, debug)``` + +### print_conditional + +`print_conditional` conditionally logs a statement at a specified severity if a condition is met. + +Usage: +```python +TerminalHelper.print_conditional( + debug_on, + f"""{TerminalColors.YELLOW}Missing Domain Information + {TerminalColors.ENDC}""", +) +``` + +### prompt_for_execution + +`prompt_for_execution` prompts the user to inspect a string and confirm if they wish to proceed. Returns True if proceeding, False if skipping, or exits the script. + +```python +TerminalHelper.prompt_for_execution( + system_exit_on_terminate=True, + info_to_inspect=f""" + ==Master data file== + domain_additional_filename: {org_args.domain_additional_filename} + + ==Organization data== + organization_adhoc_filename: {org_args.organization_adhoc_filename} + + ==Containing directory== + directory: {org_args.directory} + """, + prompt_title="Do you wish to load organization data for TransitionDomains?", +) +``` + +### query_yes_no + +`query_yes_no` prompts the user with a yes/no question and returns True for "yes" or False for "no". + +Usage: +```python +if query_yes_no("Do you want to proceed?"): + print("Proceeding...") +else: + print("Aborting.") +``` + +### query_yes_no_exit + +`query_yes_no_exit` is similar to `query_yes_no` but includes an "exit" option to terminate the script. + +Usage: +if query_yes_no_exit("Continue, abort, or exit?"): + print("Continuing...") +else: + print("Aborting.") + # Script will exit if user selected "e" for exit ## PopulateScriptTemplate @@ -42,67 +109,4 @@ To create a script using `PopulateScriptTemplate`: 1. Create a new class that inherits from `PopulateScriptTemplate` 2. Implement the `update_record` method to define how each record should be updated 3. Optionally, override the configuration variables and helper methods as needed -4. Call `mass_update_records` within `handle` and run the script - -## TerminalHelper -### log_script_run_summary - -`log_script_run_summary` logs a summary of a script run, including counts of updated, skipped, and failed records. - -### print_conditional - -`print_conditional` conditionally logs a statement at a specified severity if a condition is met. - -### prompt_for_execution - -`prompt_for_execution` prompts the user to inspect a string and confirm if they wish to proceed. Returns True if proceeding, False if skipping, or exits the script. - -### get_file_line_count - -`get_file_line_count` returns the number of lines in a file. - -### print_to_file_conditional - -`print_to_file_conditional` conditionally writes content to a file if a condition is met. - -Refer to the source code for full function signatures and additional details. - -### query_yes_no - -`query_yes_no` prompts the user with a yes/no question and returns True for "yes" or False for "no". - -Usage: -```python -if query_yes_no("Do you want to proceed?"): - print("Proceeding...") -else: - print("Aborting.") -``` - -### query_yes_no_exit - -`query_yes_no_exit` is similar to `query_yes_no` but includes an "exit" option to terminate the script. - -Usage: -if query_yes_no_exit("Continue, abort, or exit?"): - print("Continuing...") -else: - print("Aborting.") - # Script will exit if user selected "e" for exit - -### array_as_string - -`array_as_string` converts a list of strings into a single string with each element on a new line. - -Usage: -```python -my_list = ["apple", "banana", "cherry"] -print(array_as_string(my_list)) -``` - -Output: -``` -apple -banana -cherry -``` +4. Call `mass_update_records` within `handle` and run the script \ No newline at end of file From 26ddf317d5872df3bc771a65edc43343f80b9454 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 09:12:59 -0600 Subject: [PATCH 07/32] Add more documentation --- docs/operations/data_migration.md | 27 +++++++++++++++++++ ...ncy.py => transfer_federal_agency_type.py} | 10 ++++--- 2 files changed, 33 insertions(+), 4 deletions(-) rename src/registrar/management/commands/{transfer_federal_agency.py => transfer_federal_agency_type.py} (78%) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index 472362a79..bc1aa908d 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -697,3 +697,30 @@ Example: `cf ssh getgov-za` | | Parameter | Description | |:-:|:-------------------------- |:----------------------------------------------------------------------------| | 1 | **debug** | Increases logging detail. Defaults to False. | + +## Transfer federal agency script +The transfer federal agency script adds the "federal_type" field on each associated DomainRequest, and uses that to populate the "federal_type" field on each FederalAgency. + +**Important:** When running this script, note that data generated by our fixtures will be inaccurate (since we assign random data to them). Use real data on this script. +Do note that there is a check on record uniqueness. If two or more records do NOT have the same value for federal_type for any given federal agency, then the record is skipped. This protects against fixtures data when loaded with real data. + +### Running on sandboxes + +#### Step 1: Login to CloudFoundry +```cf login -a api.fr.cloud.gov --sso``` + +#### Step 2: SSH into your environment +```cf ssh getgov-{space}``` + +Example: `cf ssh getgov-za` + +#### Step 3: Create a shell instance +```/tmp/lifecycle/shell``` + +#### Step 4: Running the script +```./manage.py transfer_federal_agency_type``` + +### Running locally + +#### Step 1: Running the script +```docker-compose exec app ./manage.py transfer_federal_agency_type``` diff --git a/src/registrar/management/commands/transfer_federal_agency.py b/src/registrar/management/commands/transfer_federal_agency_type.py similarity index 78% rename from src/registrar/management/commands/transfer_federal_agency.py rename to src/registrar/management/commands/transfer_federal_agency_type.py index dd7b1e5db..e9ee6f3ce 100644 --- a/src/registrar/management/commands/transfer_federal_agency.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -18,7 +18,7 @@ class Command(BaseCommand, PopulateScriptTemplate): def handle(self, **kwargs): """Loops through each valid User object and updates its verification_type value""" - # Get all existing domain requests + # Get all existing domain requests. Select_related allows us to skip doing db queries. self.all_domain_requests = DomainRequest.objects.select_related("federal_agency").distinct() self.mass_update_records( FederalAgency, filter_conditions={"agency__isnull": False}, fields_to_update=["federal_type"] @@ -28,9 +28,11 @@ class Command(BaseCommand, PopulateScriptTemplate): """Defines how we update the federal_type field on each record.""" request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() record.federal_type = request.federal_type - + def should_skip_record(self, record) -> bool: # noqa """Defines the conditions in which we should skip updating a record.""" - request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() - return not request or not request.federal_agency + requests = self.all_domain_requests.filter(federal_agency__agency=record.agency, federal_type__isnull=False) + # Check if all federal_type values are the same. Skip the record otherwise. + distinct_federal_types = requests.values('federal_type').distinct() + return distinct_federal_types.count() != 1 From 02e0f23120b1b2af8f95271e79c2a6bf15181ed3 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 09:37:31 -0600 Subject: [PATCH 08/32] Cleanup --- .../management/commands/populate_verification_type.py | 9 ++++----- .../management/commands/transfer_federal_agency_type.py | 8 ++++---- .../management/commands/utility/terminal_helper.py | 8 ++++---- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/registrar/management/commands/populate_verification_type.py b/src/registrar/management/commands/populate_verification_type.py index b61521977..30cccfc15 100644 --- a/src/registrar/management/commands/populate_verification_type.py +++ b/src/registrar/management/commands/populate_verification_type.py @@ -12,12 +12,11 @@ class Command(BaseCommand, PopulateScriptTemplate): def handle(self, **kwargs): """Loops through each valid User object and updates its verification_type value""" filter_condition = {"verification_type__isnull": True} - self.mass_populate_field(User, filter_condition, ["verification_type"]) + self.mass_update_records(User, filter_condition, ["verification_type"]) - def populate_field(self, field_to_update): + def update_record(self, record: User): """Defines how we update the verification_type field""" - field_to_update.set_user_verification_type() + record.set_user_verification_type() logger.info( - f"{TerminalColors.OKCYAN}Updating {field_to_update} => " - f"{field_to_update.verification_type}{TerminalColors.OKCYAN}" + f"{TerminalColors.OKCYAN}Updating {record} => " f"{record.verification_type}{TerminalColors.OKCYAN}" ) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index e9ee6f3ce..c1816aeb9 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -1,6 +1,6 @@ import logging from django.core.management import BaseCommand -from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import FederalAgency, DomainRequest @@ -19,7 +19,7 @@ class Command(BaseCommand, PopulateScriptTemplate): """Loops through each valid User object and updates its verification_type value""" # Get all existing domain requests. Select_related allows us to skip doing db queries. - self.all_domain_requests = DomainRequest.objects.select_related("federal_agency").distinct() + self.all_domain_requests = DomainRequest.objects.select_related("federal_agency") self.mass_update_records( FederalAgency, filter_conditions={"agency__isnull": False}, fields_to_update=["federal_type"] ) @@ -28,11 +28,11 @@ class Command(BaseCommand, PopulateScriptTemplate): """Defines how we update the federal_type field on each record.""" request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() record.federal_type = request.federal_type + logger.info(f"{TerminalColors.OKCYAN}Updating {str(record)} => {record.federal_type}{TerminalColors.OKCYAN}") def should_skip_record(self, record) -> bool: # noqa """Defines the conditions in which we should skip updating a record.""" requests = self.all_domain_requests.filter(federal_agency__agency=record.agency, federal_type__isnull=False) # Check if all federal_type values are the same. Skip the record otherwise. - distinct_federal_types = requests.values('federal_type').distinct() + distinct_federal_types = requests.values("federal_type").distinct() return distinct_federal_types.count() != 1 - diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 577423ba7..22054ac7b 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -59,7 +59,6 @@ class ScriptDataHelper: model_class.objects.bulk_update(page.object_list, fields_to_update) - # This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. # It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. # See the transfer_federal_agency.py file for example usage - its really quite simple! @@ -143,17 +142,18 @@ class PopulateScriptTemplate(ABC): def get_failure_message(self, record) -> str: """Returns the message that we will display if a record fails to update""" return f"{TerminalColors.FAIL}" f"Failed to update {record}" f"{TerminalColors.ENDC}" - + def should_skip_record(self, record) -> bool: # noqa """Defines the condition in which we should skip updating a record.""" # By default - don't skip return False - class TerminalHelper: @staticmethod - def log_script_run_summary(to_update, failed_to_update, skipped, debug: bool, log_header=None, display_as_str=False): + def log_script_run_summary( + to_update, failed_to_update, skipped, debug: bool, log_header=None, display_as_str=False + ): """Prints success, failed, and skipped counts, as well as all affected objects.""" update_success_count = len(to_update) From 1cd69aa12f9611396d199713ba00cc961d017cb4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 10:20:49 -0600 Subject: [PATCH 09/32] Move field --- src/registrar/assets/js/get-gov-admin.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 524cfe594..27ac25206 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -405,12 +405,16 @@ function initializeWidgetOnList(list, parentId) { document.addEventListener('DOMContentLoaded', function() { let statusSelect = document.getElementById('id_status'); - function moveStatusChangelog(actionNeededReasonFormGroup, statusSelect) { - let flexContainer = actionNeededReasonFormGroup.querySelector('.flex-container'); + function moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect) { + let flexContainerActionNeeded = actionNeededReasonFormGroup.querySelector('.flex-container'); + let flexContainerRejected = rejectionReasonFormGroup.querySelector('.flex-container'); let statusChangelog = document.getElementById('dja-status-changelog'); if (statusSelect.value === "action needed") { - flexContainer.parentNode.insertBefore(statusChangelog, flexContainer.nextSibling); - } else { + flexContainerActionNeeded.parentNode.insertBefore(statusChangelog, flexContainerActionNeeded.nextSibling); + } else if (statusSelect.value === "rejected"){ + flexContainerRejected.parentNode.insertBefore(statusChangelog, flexContainerRejected.nextSibling); + } + else { // Move the changelog back to its original location let statusFlexContainer = statusSelect.closest('.flex-container'); statusFlexContainer.parentNode.insertBefore(statusChangelog, statusFlexContainer.nextSibling); @@ -418,11 +422,11 @@ function initializeWidgetOnList(list, parentId) { } // Call the function on page load - moveStatusChangelog(actionNeededReasonFormGroup, statusSelect); + moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect); // Add event listener to handle changes to the selector itself statusSelect.addEventListener('change', function() { - moveStatusChangelog(actionNeededReasonFormGroup, statusSelect); + moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect); }) }); })(); From f42090fa309003c146a1482e3fb4dcca6538910a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 11:02:25 -0600 Subject: [PATCH 10/32] Use creator rather than email --- src/registrar/models/domain_request.py | 28 ++++++++++++------- .../already_has_domains.txt | 2 +- .../emails/action_needed_reasons/bad_name.txt | 2 +- .../eligibility_unclear.txt | 2 +- .../questionable_authorizing_official.txt | 2 +- .../emails/domain_request_withdrawn.txt | 2 +- .../includes/domain_request_summary.txt | 2 +- .../emails/status_change_approved.txt | 2 +- .../emails/status_change_rejected.txt | 2 +- .../emails/submission_confirmation.txt | 2 +- 10 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 1c4725be1..72d57f155 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -7,6 +7,7 @@ from django.conf import settings from django.db import models from django_fsm import FSMField, transition # type: ignore from django.utils import timezone +from waffle import flag_is_active from registrar.models.domain import Domain from registrar.models.federal_agency import FederalAgency from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper @@ -668,34 +669,41 @@ class DomainRequest(TimeStampedModel): def _send_status_update_email( self, new_status, email_template, email_template_subject, send_email=True, bcc_address="", wrap_email=False ): - """Send a status update email to the submitter. + """Send a status update email to the creator. - The email goes to the email address that the submitter gave as their - contact information. If there is not submitter information, then do + The email goes to the email address that the creator gave as their + contact information. If there is not creator information, then do nothing. send_email: bool -> Used to bypass the send_templated_email function, in the event we just want to log that an email would have been sent, rather than actually sending one. """ - if self.submitter is None or self.submitter.email is None: - logger.warning(f"Cannot send {new_status} email, no submitter email address.") + # Send the status update to the request creator + to_address = self.creator.email if self.creator else None + if to_address is None: + logger.warning(f"Cannot send {new_status} email, no creator email address.") return None if not send_email: - logger.info(f"Email was not sent. Would send {new_status} email: {self.submitter.email}") + logger.info(f"Email was not sent. Would send {new_status} email: {to_address}") return None - + + recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter try: send_templated_email( email_template, email_template_subject, - self.submitter.email, - context={"domain_request": self}, + to_address, + context={ + "domain_request": self, + # This is the user that we refer to in the email + "recipient": recipient, + }, bcc_address=bcc_address, wrap_email=wrap_email, ) - logger.info(f"The {new_status} email sent to: {self.submitter.email}") + logger.info(f"The {new_status} email sent to: {to_address}") except EmailSendingError: logger.warning("Failed to send confirmation email", exc_info=True) diff --git a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt index 842b4ac4f..264fe265b 100644 --- a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt +++ b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. diff --git a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt index beba3521d..95967639c 100644 --- a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt +++ b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. diff --git a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt index df8db313d..280321045 100644 --- a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt +++ b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. diff --git a/src/registrar/templates/emails/action_needed_reasons/questionable_authorizing_official.txt b/src/registrar/templates/emails/action_needed_reasons/questionable_authorizing_official.txt index 1c2c5d855..259968204 100644 --- a/src/registrar/templates/emails/action_needed_reasons/questionable_authorizing_official.txt +++ b/src/registrar/templates/emails/action_needed_reasons/questionable_authorizing_official.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. diff --git a/src/registrar/templates/emails/domain_request_withdrawn.txt b/src/registrar/templates/emails/domain_request_withdrawn.txt index 5ef429541..0c061c53c 100644 --- a/src/registrar/templates/emails/domain_request_withdrawn.txt +++ b/src/registrar/templates/emails/domain_request_withdrawn.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. Your .gov domain request has been withdrawn and will not be reviewed by our team. diff --git a/src/registrar/templates/emails/includes/domain_request_summary.txt b/src/registrar/templates/emails/includes/domain_request_summary.txt index 10ec9ac2c..b3f6c0d95 100644 --- a/src/registrar/templates/emails/includes/domain_request_summary.txt +++ b/src/registrar/templates/emails/includes/domain_request_summary.txt @@ -43,7 +43,7 @@ Purpose of your domain: {{ domain_request.purpose }} Your contact information: -{% spaceless %}{% include "emails/includes/contact.txt" with contact=domain_request.submitter %}{% endspaceless %} +{% spaceless %}{% include "emails/includes/contact.txt" with contact=recipient %}{% endspaceless %} Other employees from your organization:{% for other in domain_request.other_contacts.all %} {% spaceless %}{% include "emails/includes/contact.txt" with contact=other %}{% endspaceless %} diff --git a/src/registrar/templates/emails/status_change_approved.txt b/src/registrar/templates/emails/status_change_approved.txt index cab88d973..bbef8c81a 100644 --- a/src/registrar/templates/emails/status_change_approved.txt +++ b/src/registrar/templates/emails/status_change_approved.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. Congratulations! Your .gov domain request has been approved. diff --git a/src/registrar/templates/emails/status_change_rejected.txt b/src/registrar/templates/emails/status_change_rejected.txt index 4febb4c64..e377b12ff 100644 --- a/src/registrar/templates/emails/status_change_rejected.txt +++ b/src/registrar/templates/emails/status_change_rejected.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. Your .gov domain request has been rejected. diff --git a/src/registrar/templates/emails/submission_confirmation.txt b/src/registrar/templates/emails/submission_confirmation.txt index c0ae3bb8f..2dd4387a4 100644 --- a/src/registrar/templates/emails/submission_confirmation.txt +++ b/src/registrar/templates/emails/submission_confirmation.txt @@ -1,5 +1,5 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} -Hi, {{ domain_request.submitter.first_name }}. +Hi, {{ recipient.first_name }}. We received your .gov domain request. From 8659ad7f7555797b5cc91c44a32e0d21c145b446 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 11:07:26 -0600 Subject: [PATCH 11/32] Slight refactor --- src/registrar/models/domain_request.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 72d57f155..f62e47642 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -677,24 +677,25 @@ class DomainRequest(TimeStampedModel): send_email: bool -> Used to bypass the send_templated_email function, in the event we just want to log that an email would have been sent, rather than actually sending one. + + wrap_email: bool -> Wraps emails using `wrap_text_and_preserve_paragraphs` if any given + paragraph exceeds our desired max length (for prettier display). """ - # Send the status update to the request creator - to_address = self.creator.email if self.creator else None - if to_address is None: + recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter + if recipient is None or recipient.email is None: logger.warning(f"Cannot send {new_status} email, no creator email address.") return None if not send_email: - logger.info(f"Email was not sent. Would send {new_status} email: {to_address}") + logger.info(f"Email was not sent. Would send {new_status} email to: {recipient.email}") return None - - recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter + try: send_templated_email( email_template, email_template_subject, - to_address, + recipient.email, context={ "domain_request": self, # This is the user that we refer to in the email @@ -703,7 +704,7 @@ class DomainRequest(TimeStampedModel): bcc_address=bcc_address, wrap_email=wrap_email, ) - logger.info(f"The {new_status} email sent to: {to_address}") + logger.info(f"The {new_status} email sent to: {recipient.email}") except EmailSendingError: logger.warning("Failed to send confirmation email", exc_info=True) From a0e9db74ebcfb8f2df5667a3ef77cc32355c17da Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 13:05:04 -0600 Subject: [PATCH 12/32] Minor changes --- .../commands/transfer_federal_agency_type.py | 8 +++++--- .../commands/utility/terminal_helper.py | 18 +++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index c1816aeb9..b3ec5da5e 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -7,10 +7,12 @@ from registrar.models import FederalAgency, DomainRequest logger = logging.getLogger(__name__) -# This command uses the PopulateScriptTemplate. -# This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. -# It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. class Command(BaseCommand, PopulateScriptTemplate): + """ + This command uses the PopulateScriptTemplate. + This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. + It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. + """ help = "Loops through each valid User object and updates its verification_type value" prompt_title = "Do you wish to update all Federal Agencies?" display_run_summary_items_as_str = True diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 22054ac7b..eedc09aae 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -82,15 +82,15 @@ class PopulateScriptTemplate(ABC): """Defines how we update each field. Must be defined before using mass_update_records.""" raise NotImplementedError - def mass_update_records(self, sender, filter_conditions, fields_to_update, debug=True): - """Loops through each valid "sender" object - specified by filter_conditions - and + def mass_update_records(self, object_class, filter_conditions, fields_to_update, debug=True): + """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. You must define update_record before you can use this function. """ - records = sender.objects.filter(**filter_conditions) - readable_class_name = self.get_class_name(sender) + records = object_class.objects.filter(**filter_conditions) + readable_class_name = self.get_class_name(object_class) # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( @@ -104,9 +104,9 @@ class PopulateScriptTemplate(ABC): ) logger.info("Updating...") - to_update: List[sender] = [] - to_skip: List[sender] = [] - failed_to_update: List[sender] = [] + to_update: List[object_class] = [] + to_skip: List[object_class] = [] + failed_to_update: List[object_class] = [] for record in records: try: if not self.should_skip_record(record): @@ -121,7 +121,7 @@ class PopulateScriptTemplate(ABC): logger.error(fail_message) # Do a bulk update on the desired field - ScriptDataHelper.bulk_update_fields(sender, to_update, fields_to_update) + ScriptDataHelper.bulk_update_fields(object_class, to_update, fields_to_update) # Log what happened TerminalHelper.log_script_run_summary( @@ -144,7 +144,7 @@ class PopulateScriptTemplate(ABC): return f"{TerminalColors.FAIL}" f"Failed to update {record}" f"{TerminalColors.ENDC}" def should_skip_record(self, record) -> bool: # noqa - """Defines the condition in which we should skip updating a record.""" + """Defines the condition in which we should skip updating a record. Override as needed.""" # By default - don't skip return False From 3794e9db86a7200e6a21272dc1af1d51d510d514 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 13:32:36 -0600 Subject: [PATCH 13/32] Slight rewrite for docs --- docs/developer/management_script_helpers.md | 79 +++++---------------- 1 file changed, 16 insertions(+), 63 deletions(-) diff --git a/docs/developer/management_script_helpers.md b/docs/developer/management_script_helpers.md index 609015ad6..104e4dc13 100644 --- a/docs/developer/management_script_helpers.md +++ b/docs/developer/management_script_helpers.md @@ -5,94 +5,47 @@ This file documents what they do and provides guidance on their usage. ## TerminalColors `TerminalColors` provides ANSI color codes as variables to style terminal output. -Usage: -```print(f"{TerminalColors.OKGREEN}Success!{TerminalColors.ENDC}")``` - ## ScriptDataHelper ### bulk_update_fields `bulk_update_fields` performs a memory-efficient bulk update on a Django model in batches using a Paginator. -Usage: -```bulk_update_fields(Domain, object_list, ["first_ready"])``` - ## TerminalHelper ### log_script_run_summary `log_script_run_summary` logs a summary of a script run, including counts of updated, skipped, and failed records. -Usage: -```TerminalHelper.log_script_run_summary(self.to_update, self.failed_to_update, self.skipped, debug)``` - ### print_conditional `print_conditional` conditionally logs a statement at a specified severity if a condition is met. -Usage: -```python -TerminalHelper.print_conditional( - debug_on, - f"""{TerminalColors.YELLOW}Missing Domain Information - {TerminalColors.ENDC}""", -) -``` - ### prompt_for_execution `prompt_for_execution` prompts the user to inspect a string and confirm if they wish to proceed. Returns True if proceeding, False if skipping, or exits the script. -```python -TerminalHelper.prompt_for_execution( - system_exit_on_terminate=True, - info_to_inspect=f""" - ==Master data file== - domain_additional_filename: {org_args.domain_additional_filename} - - ==Organization data== - organization_adhoc_filename: {org_args.organization_adhoc_filename} - - ==Containing directory== - directory: {org_args.directory} - """, - prompt_title="Do you wish to load organization data for TransitionDomains?", -) -``` - ### query_yes_no `query_yes_no` prompts the user with a yes/no question and returns True for "yes" or False for "no". -Usage: -```python -if query_yes_no("Do you want to proceed?"): - print("Proceeding...") -else: - print("Aborting.") -``` - ### query_yes_no_exit `query_yes_no_exit` is similar to `query_yes_no` but includes an "exit" option to terminate the script. -Usage: -if query_yes_no_exit("Continue, abort, or exit?"): - print("Continuing...") -else: - print("Aborting.") - # Script will exit if user selected "e" for exit - ## PopulateScriptTemplate `PopulateScriptTemplate` is an abstract base class that provides a template for creating generic populate scripts. It handles logging and bulk updating for repetitive scripts that update a few fields. -**Disclaimer:** This template is intended as a shorthand for simple scripts. It is not recommended for complex operations. See `transfer_federal_agency.py` for a straightforward example of how to use this template. +### **Disclaimer** +This template is intended as a shorthand for simple scripts. It is not recommended for complex operations. See `transfer_federal_agency.py` for a straightforward example of how to use this template. -To use `PopulateScriptTemplate`, create a new class that inherits from it and implement the `update_record` method. This method defines how each record should be updated. +### Step-by-step usage guide +To create a script using `PopulateScriptTemplate`: +1. Create a new class that inherits from `PopulateScriptTemplate` +2. Implement the `update_record` method to define how each record should be updated +3. Optionally, override the configuration variables and helper methods as needed +4. Call `mass_update_records` within `handle` and run the script -The class provides the following optional configuration variables: -- `prompt_title`: The header displayed by `prompt_for_execution` when the script starts (default: "Do you wish to proceed?") -- `display_run_summary_items_as_str`: If True, runs `str(item)` on each item when printing the run summary for prettier output (default: False) -- `run_summary_header`: The header for the script run summary printed after the script finishes (default: None) +#### Template explanation The main method provided by `PopulateScriptTemplate` is `mass_update_records`. This method loops through each valid object (specified by `filter_conditions`) and updates the fields defined in `fields_to_update` using the `update_record` method. @@ -100,13 +53,13 @@ Before updating, `mass_update_records` prompts the user to confirm the proposed After processing the records, `mass_update_records` performs a bulk update on the specified fields using `ScriptDataHelper.bulk_update_fields` and logs a summary of the script run using `TerminalHelper.log_script_run_summary`. +#### Config options +The class provides the following optional configuration variables: +- `prompt_title`: The header displayed by `prompt_for_execution` when the script starts (default: "Do you wish to proceed?") +- `display_run_summary_items_as_str`: If True, runs `str(item)` on each item when printing the run summary for prettier output (default: False) +- `run_summary_header`: The header for the script run summary printed after the script finishes (default: None) + The class also provides helper methods: - `get_class_name`: Returns a display-friendly class name for the terminal prompt - `get_failure_message`: Returns the message to display if a record fails to update -- `should_skip_record`: Defines the condition for skipping a record (by default, no records are skipped) - -To create a script using `PopulateScriptTemplate`: -1. Create a new class that inherits from `PopulateScriptTemplate` -2. Implement the `update_record` method to define how each record should be updated -3. Optionally, override the configuration variables and helper methods as needed -4. Call `mass_update_records` within `handle` and run the script \ No newline at end of file +- `should_skip_record`: Defines the condition for skipping a record (by default, no records are skipped) \ No newline at end of file From d018a5b9d96dace2898a5fd4e41cfcddcb324d55 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 13:56:11 -0600 Subject: [PATCH 14/32] Use flag for user --- src/registrar/models/domain_request.py | 7 ++++--- src/registrar/models/utility/generic_helper.py | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index f62e47642..5369f8285 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -10,7 +10,7 @@ from django.utils import timezone from waffle import flag_is_active from registrar.models.domain import Domain from registrar.models.federal_agency import FederalAgency -from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper +from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper, flag_is_active_for_user from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes from registrar.utility.constants import BranchChoices @@ -681,8 +681,9 @@ class DomainRequest(TimeStampedModel): wrap_email: bool -> Wraps emails using `wrap_text_and_preserve_paragraphs` if any given paragraph exceeds our desired max length (for prettier display). """ - - recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter + # This is a chicken or egg kind of problem. We don't have the request -- how do we know + # to use either the creator or the submitter in this scenario? + recipient = self.creator if flag_is_active_for_user(self.creator, "profile_feature") else self.submitter if recipient is None or recipient.email is None: logger.warning(f"Cannot send {new_status} email, no creator email address.") return None diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py index f9d4303c4..22db67721 100644 --- a/src/registrar/models/utility/generic_helper.py +++ b/src/registrar/models/utility/generic_helper.py @@ -3,6 +3,8 @@ import time import logging from urllib.parse import urlparse, urlunparse, urlencode +from registrar.models import User +from waffle import get_waffle_flag_model logger = logging.getLogger(__name__) @@ -321,3 +323,9 @@ def convert_queryset_to_dict(queryset, is_model=True, key="id"): request_dict = {value[key]: value for value in queryset} return request_dict + + +def flag_is_active_for_user(user: User, flag_name: str) -> bool | None: + """Given a user, returns if said user has the given flag permission or not""" + flag = get_waffle_flag_model().get(flag_name) + return flag.is_active_for_user(user) \ No newline at end of file From 3a0fcf2648795fc476301cbe863f57eec9982c47 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 15:08:13 -0600 Subject: [PATCH 15/32] Add test --- .../commands/transfer_federal_agency_type.py | 8 +- src/registrar/tests/common.py | 4 + .../tests/test_management_scripts.py | 113 ++++++++++++++++++ 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index b3ec5da5e..71c6c1393 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -1,7 +1,7 @@ import logging from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors -from registrar.models import FederalAgency, DomainRequest +from registrar.models import FederalAgency, DomainInformation logger = logging.getLogger(__name__) @@ -21,20 +21,20 @@ class Command(BaseCommand, PopulateScriptTemplate): """Loops through each valid User object and updates its verification_type value""" # Get all existing domain requests. Select_related allows us to skip doing db queries. - self.all_domain_requests = DomainRequest.objects.select_related("federal_agency") + self.all_domain_infos = DomainInformation.objects.select_related("federal_agency") self.mass_update_records( FederalAgency, filter_conditions={"agency__isnull": False}, fields_to_update=["federal_type"] ) def update_record(self, record: FederalAgency): """Defines how we update the federal_type field on each record.""" - request = self.all_domain_requests.filter(federal_agency__agency=record.agency).first() + request = self.all_domain_infos.filter(federal_agency__agency=record.agency).first() record.federal_type = request.federal_type logger.info(f"{TerminalColors.OKCYAN}Updating {str(record)} => {record.federal_type}{TerminalColors.OKCYAN}") def should_skip_record(self, record) -> bool: # noqa """Defines the conditions in which we should skip updating a record.""" - requests = self.all_domain_requests.filter(federal_agency__agency=record.agency, federal_type__isnull=False) + requests = self.all_domain_infos.filter(federal_agency__agency=record.agency, federal_type__isnull=False) # Check if all federal_type values are the same. Skip the record otherwise. distinct_federal_types = requests.values("federal_type").distinct() return distinct_federal_types.count() != 1 diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 923195bc1..5f5695556 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -858,6 +858,7 @@ def completed_domain_request( # noqa is_election_board=False, organization_type=None, federal_agency=None, + federal_type=None, ): """A completed domain request.""" if not user: @@ -922,6 +923,9 @@ def completed_domain_request( # noqa if organization_type: domain_request_kwargs["organization_type"] = organization_type + if federal_type: + domain_request_kwargs["federal_type"] = federal_type + domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) if has_other_contacts: diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index 500953f02..20212b245 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -2,6 +2,7 @@ import copy from datetime import date, datetime, time from django.core.management import call_command from django.test import TestCase, override_settings +from registrar.utility.constants import BranchChoices from django.utils import timezone from django.utils.module_loading import import_string import logging @@ -1108,3 +1109,115 @@ class TestImportTables(TestCase): # Check that logger.error was called with the correct message mock_logger.error.assert_called_once_with("Zip file tmp/exported_tables.zip does not exist.") + + +class TestTransferFederalAgencyType(TestCase): + """Tests for the transfer_federal_agency_type script""" + + def setUp(self): + """Creates a fake domain object""" + super().setUp() + + self.amtrak, _ = FederalAgency.objects.get_or_create(agency="AMTRAK") + self.legislative_branch, _ = FederalAgency.objects.get_or_create(agency="Legislative Branch") + self.library_of_congress, _ = FederalAgency.objects.get_or_create(agency="Library of Congress") + self.gov_admin, _ = FederalAgency.objects.get_or_create(agency="gov Administration") + + self.domain_request_1 = completed_domain_request( + name="testgov.gov", + federal_agency=self.amtrak, + federal_type=BranchChoices.EXECUTIVE, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + self.domain_request_2 = completed_domain_request( + name="cheesefactory.gov", + federal_agency=self.legislative_branch, + federal_type=BranchChoices.LEGISLATIVE, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + self.domain_request_3 = completed_domain_request( + name="meowardslaw.gov", + federal_agency=self.library_of_congress, + federal_type=BranchChoices.JUDICIAL, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + + # Duplicate fields with invalid data - we expect to skip updating these + self.domain_request_4 = completed_domain_request( + name="baddata.gov", + federal_agency=self.gov_admin, + federal_type=BranchChoices.EXECUTIVE, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + self.domain_request_5 = completed_domain_request( + name="worsedata.gov", + federal_agency=self.gov_admin, + federal_type=BranchChoices.JUDICIAL, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + + self.domain_request_1.approve() + self.domain_request_2.approve() + self.domain_request_3.approve() + self.domain_request_4.approve() + self.domain_request_5.approve() + + def tearDown(self): + """Deletes all DB objects related to migrations""" + super().tearDown() + + # Delete domains and related information + Domain.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + User.objects.all().delete() + Contact.objects.all().delete() + Website.objects.all().delete() + FederalAgency.objects.all().delete() + + def run_transfer_federal_agency_type(self): + """ + This method executes the transfer_federal_agency_type command. + + The 'call_command' function from Django's management framework is then used to + execute the populate_first_ready command with the specified arguments. + """ + with less_console_noise(): + with patch( + "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", # noqa + return_value=True, + ): + call_command("transfer_federal_agency_type") + + @less_console_noise_decorator + def test_transfer_federal_agency_type_script(self): + """ + Tests that the transfer_federal_agency_type script updates what we expect, and skips what we expect + """ + + # Before proceeding, make sure we don't have any data contamination + tested_agencies = [ + self.amtrak, + self.legislative_branch, + self.library_of_congress, + self.gov_admin, + ] + for agency in tested_agencies: + self.assertEqual(agency.federal_type, None) + + # Run the script + self.run_transfer_federal_agency_type() + + # Refresh the local db instance to reflect changes + self.amtrak.refresh_from_db() + self.legislative_branch.refresh_from_db() + self.library_of_congress.refresh_from_db() + self.gov_admin.refresh_from_db() + + # Test the values that we expect to be updated + self.assertEqual(self.amtrak.federal_type, BranchChoices.EXECUTIVE) + self.assertEqual(self.legislative_branch.federal_type, BranchChoices.LEGISLATIVE) + self.assertEqual(self.library_of_congress.federal_type, BranchChoices.JUDICIAL) + + # We don't expect this field to be updated (as it has duplicate data) + self.assertEqual(self.gov_admin.federal_type, None) From 1411186921a463417b4ae8d19f48790cd5f2f6b8 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Jun 2024 15:24:18 -0600 Subject: [PATCH 16/32] Update transfer_federal_agency_type.py --- .../management/commands/transfer_federal_agency_type.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index 71c6c1393..6ee07d7ef 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -13,6 +13,7 @@ class Command(BaseCommand, PopulateScriptTemplate): This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. """ + help = "Loops through each valid User object and updates its verification_type value" prompt_title = "Do you wish to update all Federal Agencies?" display_run_summary_items_as_str = True From 72b627e468a9521908e3297ad47b4d2ee87d264b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 08:44:56 -0600 Subject: [PATCH 17/32] Add some additional logging --- .../commands/transfer_federal_agency_type.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index 6ee07d7ef..305907351 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -31,11 +31,17 @@ class Command(BaseCommand, PopulateScriptTemplate): """Defines how we update the federal_type field on each record.""" request = self.all_domain_infos.filter(federal_agency__agency=record.agency).first() record.federal_type = request.federal_type - logger.info(f"{TerminalColors.OKCYAN}Updating {str(record)} => {record.federal_type}{TerminalColors.OKCYAN}") + logger.info(f"{TerminalColors.OKCYAN}Updating {str(record)} => {record.federal_type}{TerminalColors.ENDC}") def should_skip_record(self, record) -> bool: # noqa """Defines the conditions in which we should skip updating a record.""" requests = self.all_domain_infos.filter(federal_agency__agency=record.agency, federal_type__isnull=False) # Check if all federal_type values are the same. Skip the record otherwise. distinct_federal_types = requests.values("federal_type").distinct() - return distinct_federal_types.count() != 1 + should_skip = distinct_federal_types.count() != 1 + if should_skip: + logger.info( + f"{TerminalColors.YELLOW}Skipping update for {str(record)} => count is " + f"{distinct_federal_types.count()} and records are {distinct_federal_types}{TerminalColors.ENDC}" + ) + return should_skip From 52ae6719b59fea75724ec1dc30c78e00d690130c Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 09:11:47 -0600 Subject: [PATCH 18/32] Update generic_helper.py --- src/registrar/models/utility/generic_helper.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py index 22db67721..4fc00be02 100644 --- a/src/registrar/models/utility/generic_helper.py +++ b/src/registrar/models/utility/generic_helper.py @@ -3,7 +3,6 @@ import time import logging from urllib.parse import urlparse, urlunparse, urlencode -from registrar.models import User from waffle import get_waffle_flag_model @@ -325,7 +324,7 @@ def convert_queryset_to_dict(queryset, is_model=True, key="id"): return request_dict -def flag_is_active_for_user(user: User, flag_name: str) -> bool | None: +def flag_is_active_for_user(user, flag_name: str) -> bool | None: """Given a user, returns if said user has the given flag permission or not""" flag = get_waffle_flag_model().get(flag_name) - return flag.is_active_for_user(user) \ No newline at end of file + return flag.is_active_for_user(user) From b45d5cebbcdb5bdf5a29b569198f1f1888d93b7b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 09:58:29 -0600 Subject: [PATCH 19/32] Use regular flag_is_active --- src/registrar/models/domain_request.py | 7 +++---- src/registrar/models/utility/generic_helper.py | 7 ------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 5369f8285..f62e47642 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -10,7 +10,7 @@ from django.utils import timezone from waffle import flag_is_active from registrar.models.domain import Domain from registrar.models.federal_agency import FederalAgency -from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper, flag_is_active_for_user +from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes from registrar.utility.constants import BranchChoices @@ -681,9 +681,8 @@ class DomainRequest(TimeStampedModel): wrap_email: bool -> Wraps emails using `wrap_text_and_preserve_paragraphs` if any given paragraph exceeds our desired max length (for prettier display). """ - # This is a chicken or egg kind of problem. We don't have the request -- how do we know - # to use either the creator or the submitter in this scenario? - recipient = self.creator if flag_is_active_for_user(self.creator, "profile_feature") else self.submitter + + recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter if recipient is None or recipient.email is None: logger.warning(f"Cannot send {new_status} email, no creator email address.") return None diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py index 4fc00be02..f9d4303c4 100644 --- a/src/registrar/models/utility/generic_helper.py +++ b/src/registrar/models/utility/generic_helper.py @@ -3,7 +3,6 @@ import time import logging from urllib.parse import urlparse, urlunparse, urlencode -from waffle import get_waffle_flag_model logger = logging.getLogger(__name__) @@ -322,9 +321,3 @@ def convert_queryset_to_dict(queryset, is_model=True, key="id"): request_dict = {value[key]: value for value in queryset} return request_dict - - -def flag_is_active_for_user(user, flag_name: str) -> bool | None: - """Given a user, returns if said user has the given flag permission or not""" - flag = get_waffle_flag_model().get(flag_name) - return flag.is_active_for_user(user) From aa9a7342ccc7580cb8b9eb4fdd465e78a99dca9b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 10:44:10 -0600 Subject: [PATCH 20/32] Add unit tests --- src/registrar/tests/test_models.py | 80 ++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 20 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 53af1674e..37b9c0b04 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -23,6 +23,7 @@ from registrar.utility.constants import BranchChoices from .common import MockSESClient, less_console_noise, completed_domain_request, set_domain_request_investigators from django_fsm import TransitionNotAllowed +from waffle.testutils import override_flag # Test comment for push -- will remove @@ -31,29 +32,44 @@ from django_fsm import TransitionNotAllowed @boto3_mocking.patching class TestDomainRequest(TestCase): def setUp(self): + + self.dummy_user, _ = Contact.objects.get_or_create( + email="mayor@igorville.com", first_name="Hello", last_name="World" + ) + self.dummy_user_2, _ = User.objects.get_or_create( + username="intern@igorville.com", email="intern@igorville.com", first_name="Lava", last_name="World" + ) self.started_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.STARTED, name="started.gov" + status=DomainRequest.DomainRequestStatus.STARTED, + name="started.gov", ) self.submitted_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.SUBMITTED, name="submitted.gov" + status=DomainRequest.DomainRequestStatus.SUBMITTED, + name="submitted.gov", ) self.in_review_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.IN_REVIEW, name="in-review.gov" + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + name="in-review.gov", ) self.action_needed_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.ACTION_NEEDED, name="action-needed.gov" + status=DomainRequest.DomainRequestStatus.ACTION_NEEDED, + name="action-needed.gov", ) self.approved_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.APPROVED, name="approved.gov" + status=DomainRequest.DomainRequestStatus.APPROVED, + name="approved.gov", ) self.withdrawn_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.WITHDRAWN, name="withdrawn.gov" + status=DomainRequest.DomainRequestStatus.WITHDRAWN, + name="withdrawn.gov", ) self.rejected_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.REJECTED, name="rejected.gov" + status=DomainRequest.DomainRequestStatus.REJECTED, + name="rejected.gov", ) self.ineligible_domain_request = completed_domain_request( - status=DomainRequest.DomainRequestStatus.INELIGIBLE, name="ineligible.gov" + status=DomainRequest.DomainRequestStatus.INELIGIBLE, + name="ineligible.gov", ) # Store all domain request statuses in a variable for ease of use @@ -197,7 +213,9 @@ class TestDomainRequest(TestCase): domain_request.submit() self.assertEqual(domain_request.status, domain_request.DomainRequestStatus.SUBMITTED) - def check_email_sent(self, domain_request, msg, action, expected_count): + def check_email_sent( + self, domain_request, msg, action, expected_count, expected_content=None, expected_email="mayor@igorville.com" + ): """Check if an email was sent after performing an action.""" with self.subTest(msg=msg, action=action): @@ -211,19 +229,35 @@ class TestDomainRequest(TestCase): sent_emails = [ email for email in MockSESClient.EMAILS_SENT - if "mayor@igorville.gov" in email["kwargs"]["Destination"]["ToAddresses"] + if expected_email in email["kwargs"]["Destination"]["ToAddresses"] ] self.assertEqual(len(sent_emails), expected_count) + if expected_content: + email_content = sent_emails[0]["kwargs"]["Content"]["Simple"]["Body"]["Text"]["Data"] + self.assertIn(expected_content, email_content) + + @override_flag("profile_feature", active=False) def test_submit_from_started_sends_email(self): msg = "Create a domain request and submit it and see if email was sent." - domain_request = completed_domain_request() - self.check_email_sent(domain_request, msg, "submit", 1) + domain_request = completed_domain_request(submitter=self.dummy_user, user=self.dummy_user_2) + self.check_email_sent(domain_request, msg, "submit", 1, expected_content="Hello") + + @override_flag("profile_feature", active=True) + def test_submit_from_started_sends_email_to_creator(self): + """Tests if, when the profile feature flag is on, we send an email to the creator""" + msg = "Create a domain request and submit it and see if email was sent when the feature flag is on." + domain_request = completed_domain_request(submitter=self.dummy_user, user=self.dummy_user_2) + self.check_email_sent( + domain_request, msg, "submit", 1, expected_content="Lava", expected_email="intern@igorville.com" + ) def test_submit_from_withdrawn_sends_email(self): msg = "Create a withdrawn domain request and submit it and see if email was sent." - domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.WITHDRAWN) - self.check_email_sent(domain_request, msg, "submit", 1) + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.WITHDRAWN, submitter=self.dummy_user + ) + self.check_email_sent(domain_request, msg, "submit", 1, expected_content="Hello") def test_submit_from_action_needed_does_not_send_email(self): msg = "Create a domain request with ACTION_NEEDED status and submit it, check if email was not sent." @@ -237,18 +271,24 @@ class TestDomainRequest(TestCase): def test_approve_sends_email(self): msg = "Create a domain request and approve it and see if email was sent." - domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) - self.check_email_sent(domain_request, msg, "approve", 1) + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, submitter=self.dummy_user + ) + self.check_email_sent(domain_request, msg, "approve", 1, expected_content="Hello") def test_withdraw_sends_email(self): msg = "Create a domain request and withdraw it and see if email was sent." - domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) - self.check_email_sent(domain_request, msg, "withdraw", 1) + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, submitter=self.dummy_user + ) + self.check_email_sent(domain_request, msg, "withdraw", 1, expected_content="Hello") def test_reject_sends_email(self): msg = "Create a domain request and reject it and see if email was sent." - domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.APPROVED) - self.check_email_sent(domain_request, msg, "reject", 1) + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.APPROVED, submitter=self.dummy_user + ) + self.check_email_sent(domain_request, msg, "reject", 1, expected_content="Hello") def test_reject_with_prejudice_does_not_send_email(self): msg = "Create a domain request and reject it with prejudice and see if email was sent." From 1fae782399c1c8e6c17ccdd2d333e9d397c794ac Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 15:29:52 -0600 Subject: [PATCH 21/32] Update errors.py --- src/registrar/utility/errors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 546fe604d..47d3feff5 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -101,7 +101,7 @@ class FSMDomainRequestError(Exception): FSMErrorCodes.NO_INVESTIGATOR: ("Investigator is required for this status."), FSMErrorCodes.INVESTIGATOR_NOT_STAFF: ("Investigator is not a staff user."), FSMErrorCodes.INVESTIGATOR_NOT_SUBMITTER: ("Only the assigned investigator can make this change."), - FSMErrorCodes.NO_REJECTION_REASON: ("A rejection reason is required."), + FSMErrorCodes.NO_REJECTION_REASON: ("A reason is required for this status."), FSMErrorCodes.NO_ACTION_NEEDED_REASON: ("A reason is required for this status."), } From 084d56d32a3eef12c13b3edbb4a005e7234db15f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Jun 2024 15:31:14 -0600 Subject: [PATCH 22/32] Revert "Move field" This reverts commit 1cd69aa12f9611396d199713ba00cc961d017cb4. --- src/registrar/assets/js/get-gov-admin.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 27ac25206..524cfe594 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -405,16 +405,12 @@ function initializeWidgetOnList(list, parentId) { document.addEventListener('DOMContentLoaded', function() { let statusSelect = document.getElementById('id_status'); - function moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect) { - let flexContainerActionNeeded = actionNeededReasonFormGroup.querySelector('.flex-container'); - let flexContainerRejected = rejectionReasonFormGroup.querySelector('.flex-container'); + function moveStatusChangelog(actionNeededReasonFormGroup, statusSelect) { + let flexContainer = actionNeededReasonFormGroup.querySelector('.flex-container'); let statusChangelog = document.getElementById('dja-status-changelog'); if (statusSelect.value === "action needed") { - flexContainerActionNeeded.parentNode.insertBefore(statusChangelog, flexContainerActionNeeded.nextSibling); - } else if (statusSelect.value === "rejected"){ - flexContainerRejected.parentNode.insertBefore(statusChangelog, flexContainerRejected.nextSibling); - } - else { + flexContainer.parentNode.insertBefore(statusChangelog, flexContainer.nextSibling); + } else { // Move the changelog back to its original location let statusFlexContainer = statusSelect.closest('.flex-container'); statusFlexContainer.parentNode.insertBefore(statusChangelog, statusFlexContainer.nextSibling); @@ -422,11 +418,11 @@ function initializeWidgetOnList(list, parentId) { } // Call the function on page load - moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect); + moveStatusChangelog(actionNeededReasonFormGroup, statusSelect); // Add event listener to handle changes to the selector itself statusSelect.addEventListener('change', function() { - moveStatusChangelog(actionNeededReasonFormGroup, rejectionReasonFormGroup, statusSelect); + moveStatusChangelog(actionNeededReasonFormGroup, statusSelect); }) }); })(); From 77bd49c66184090e9fb1e165467274d8a1536a1b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Jun 2024 08:16:15 -0600 Subject: [PATCH 23/32] Fix unit test --- src/registrar/tests/test_admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 802974b6e..66a44c502 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -944,7 +944,7 @@ class TestDomainRequestAdminForm(TestCase): self.assertIn("rejection_reason", form.errors) rejection_reason = form.errors.get("rejection_reason") - self.assertEqual(rejection_reason, ["A rejection reason is required."]) + self.assertEqual(rejection_reason, ["A reason is required for this status."]) def test_form_choices_when_no_instance(self): with less_console_noise(): @@ -1911,7 +1911,7 @@ class TestDomainRequestAdmin(MockEppLib): messages.error.assert_called_once_with( request, - "A rejection reason is required.", + "A reason is required for this status.", ) domain_request.refresh_from_db() From 592b069499a78789a786935c6c5e8305bfb82bef Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Jun 2024 14:04:43 -0600 Subject: [PATCH 24/32] Update src/registrar/management/commands/transfer_federal_agency_type.py Co-authored-by: CuriousX --- .../management/commands/transfer_federal_agency_type.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index 305907351..5c58084a4 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -19,7 +19,7 @@ class Command(BaseCommand, PopulateScriptTemplate): display_run_summary_items_as_str = True def handle(self, **kwargs): - """Loops through each valid User object and updates its verification_type value""" + """Loops through each valid User object and updates the value of its verification_type field""" # Get all existing domain requests. Select_related allows us to skip doing db queries. self.all_domain_infos = DomainInformation.objects.select_related("federal_agency") From b65921c83fee3e5da1256668618ba57ac40fe464 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Jun 2024 14:50:58 -0600 Subject: [PATCH 25/32] Add data for missing records --- .../commands/transfer_federal_agency_type.py | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index 5c58084a4..5d7bb443e 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -2,6 +2,7 @@ import logging from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import FederalAgency, DomainInformation +from registrar.utility.constants import BranchChoices logger = logging.getLogger(__name__) @@ -21,6 +22,45 @@ class Command(BaseCommand, PopulateScriptTemplate): def handle(self, **kwargs): """Loops through each valid User object and updates the value of its verification_type field""" + # These are federal agencies that we don't have any data on. + # Independent agencies are considered "EXECUTIVE" here. + self.missing_records = { + "Christopher Columbus Fellowship Foundation": BranchChoices.EXECUTIVE, + "Commission for the Preservation of America's Heritage Abroad": BranchChoices.EXECUTIVE, + "Commission of Fine Arts": BranchChoices.EXECUTIVE, + "Committee for Purchase From People Who Are Blind or Severely Disabled": BranchChoices.EXECUTIVE, + "DC Court Services and Offender Supervision Agency": BranchChoices.EXECUTIVE, + "DC Pre-trial Services": BranchChoices.EXECUTIVE, + "Department of Agriculture": BranchChoices.EXECUTIVE, + "Dwight D. Eisenhower Memorial Commission": BranchChoices.LEGISLATIVE, + "Farm Credit System Insurance Corporation": BranchChoices.EXECUTIVE, + "Federal Financial Institutions Examination Council": BranchChoices.EXECUTIVE, + "Federal Judiciary": BranchChoices.JUDICIAL, + "Institute of Peace": BranchChoices.EXECUTIVE, + "International Boundary and Water Commission: United States and Mexico": BranchChoices.EXECUTIVE, + "International Boundary Commission: United States and Canada": BranchChoices.EXECUTIVE, + "International Joint Commission: United States and Canada": BranchChoices.EXECUTIVE, + "Legislative Branch": BranchChoices.LEGISLATIVE, + "National Foundation on the Arts and the Humanities": BranchChoices.EXECUTIVE, + "Nuclear Safety Oversight Committee": BranchChoices.EXECUTIVE, + "Office of Compliance": BranchChoices.LEGISLATIVE, + "Overseas Private Investment Corporation": BranchChoices.EXECUTIVE, + "Public Defender Service for the District of Columbia": BranchChoices.EXECUTIVE, + "The Executive Office of the President": BranchChoices.EXECUTIVE, + "U.S. Access Board": BranchChoices.EXECUTIVE, + "U.S. Agency for Global Media": BranchChoices.EXECUTIVE, + "U.S. China Economic and Security Review Commission": BranchChoices.LEGISLATIVE, + "U.S. Interagency Council on Homelessness": BranchChoices.EXECUTIVE, + "U.S. International Trade Commission": BranchChoices.EXECUTIVE, + "U.S. Postal Service": BranchChoices.EXECUTIVE, + "U.S. Trade and Development Agency": BranchChoices.EXECUTIVE, + "Udall Foundation": BranchChoices.EXECUTIVE, + "United States Arctic Research Commission": BranchChoices.EXECUTIVE, + "Utah Reclamation Mitigation and Conservation Commission": BranchChoices.EXECUTIVE, + "Vietnam Education Foundation": BranchChoices.EXECUTIVE, + "Woodrow Wilson International Center for Scholars": BranchChoices.EXECUTIVE, + "World War I Centennial Commission": BranchChoices.EXECUTIVE, + } # Get all existing domain requests. Select_related allows us to skip doing db queries. self.all_domain_infos = DomainInformation.objects.select_related("federal_agency") self.mass_update_records( @@ -30,7 +70,10 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: FederalAgency): """Defines how we update the federal_type field on each record.""" request = self.all_domain_infos.filter(federal_agency__agency=record.agency).first() - record.federal_type = request.federal_type + if request: + record.federal_type = request.federal_type + elif not request and record.agency in self.missing_records: + record.federal_type = self.missing_records.get(record.agency) logger.info(f"{TerminalColors.OKCYAN}Updating {str(record)} => {record.federal_type}{TerminalColors.ENDC}") def should_skip_record(self, record) -> bool: # noqa @@ -39,9 +82,15 @@ class Command(BaseCommand, PopulateScriptTemplate): # Check if all federal_type values are the same. Skip the record otherwise. distinct_federal_types = requests.values("federal_type").distinct() should_skip = distinct_federal_types.count() != 1 - if should_skip: + if should_skip and record.agency not in self.missing_records: logger.info( f"{TerminalColors.YELLOW}Skipping update for {str(record)} => count is " f"{distinct_federal_types.count()} and records are {distinct_federal_types}{TerminalColors.ENDC}" ) + elif record.agency in self.missing_records: + logger.info( + f"{TerminalColors.MAGENTA}Missing data on {str(record)} - " + f"swapping to manual mapping{TerminalColors.ENDC}" + ) + should_skip = False return should_skip From 9f29509d9217fc30af9da419be10b6106f18b5da Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Jun 2024 15:03:11 -0600 Subject: [PATCH 26/32] PR suggestions --- .../commands/transfer_federal_agency_type.py | 6 ++---- .../management/commands/utility/terminal_helper.py | 13 +++++-------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/registrar/management/commands/transfer_federal_agency_type.py b/src/registrar/management/commands/transfer_federal_agency_type.py index 5d7bb443e..ca4360527 100644 --- a/src/registrar/management/commands/transfer_federal_agency_type.py +++ b/src/registrar/management/commands/transfer_federal_agency_type.py @@ -10,14 +10,12 @@ logger = logging.getLogger(__name__) class Command(BaseCommand, PopulateScriptTemplate): """ - This command uses the PopulateScriptTemplate. - This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. - It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. + This command uses the PopulateScriptTemplate, + which provides reusable logging and bulk updating functions for mass-updating fields. """ help = "Loops through each valid User object and updates its verification_type value" prompt_title = "Do you wish to update all Federal Agencies?" - display_run_summary_items_as_str = True def handle(self, **kwargs): """Loops through each valid User object and updates the value of its verification_type field""" diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index eedc09aae..82821bd70 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -59,21 +59,18 @@ class ScriptDataHelper: model_class.objects.bulk_update(page.object_list, fields_to_update) -# This template handles logging and bulk updating for you, for repetitive scripts that update a few fields. -# It is the ultimate lazy mans shorthand. Don't use this for anything terribly complicated. -# See the transfer_federal_agency.py file for example usage - its really quite simple! class PopulateScriptTemplate(ABC): """ - Contains an ABC for generic populate scripts + Contains an ABC for generic populate scripts. + + This template provides reusable logging and bulk updating functions for + mass-updating fields. """ # Optional script-global config variables. For the most part, you can leave these untouched. # Defines what prompt_for_execution displays as its header when you first start the script prompt_title: str = "Do you wish to proceed?" - # Runs str(item) over each item when printing. Use this for prettier run summaries. - display_run_summary_items_as_str: bool = False - # The header when printing the script run summary (after the script finishes) run_summary_header = None @@ -130,7 +127,7 @@ class PopulateScriptTemplate(ABC): to_skip, debug=debug, log_header=self.run_summary_header, - display_as_str=self.display_run_summary_items_as_str, + display_as_str=True, ) def get_class_name(self, sender) -> str: From f5b93d3e10099a00e0c1ee95a92b096a379a7d1b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Jun 2024 13:10:04 -0600 Subject: [PATCH 27/32] add pk to log --- src/registrar/models/domain_request.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index f62e47642..c11fac7c7 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -684,7 +684,9 @@ class DomainRequest(TimeStampedModel): recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter if recipient is None or recipient.email is None: - logger.warning(f"Cannot send {new_status} email, no creator email address.") + logger.warning( + f"Cannot send {new_status} email, no creator email address for domain request with pk: {self.pk}." + ) return None if not send_email: From e56d80ca94cd562877dc946936b4d0018c70953b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Jun 2024 13:11:31 -0600 Subject: [PATCH 28/32] Update domain_request.py --- src/registrar/models/domain_request.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index c11fac7c7..787198146 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -686,6 +686,7 @@ class DomainRequest(TimeStampedModel): if recipient is None or recipient.email is None: logger.warning( f"Cannot send {new_status} email, no creator email address for domain request with pk: {self.pk}." + f" Name: {self.requested_domain.name}" if self.requested_domain else "" ) return None From 15c5b068c2aff15a047a01383686a154dccfe788 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Jun 2024 13:18:08 -0600 Subject: [PATCH 29/32] Add comment --- src/registrar/models/domain_request.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 787198146..cb778c90a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -675,6 +675,9 @@ class DomainRequest(TimeStampedModel): contact information. If there is not creator information, then do nothing. + If the waffle flag "profile_feature" is active, then this email will be sent to the + domain request creator rather than the submitter + send_email: bool -> Used to bypass the send_templated_email function, in the event we just want to log that an email would have been sent, rather than actually sending one. From 017ffcad888e449e4898d1ccf9f0a3fa6f093766 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:32:47 -0600 Subject: [PATCH 30/32] lint --- src/registrar/models/domain_request.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 5b01ae681..d26bb3284 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -18,8 +18,6 @@ from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError from itertools import chain -from waffle.decorators import flag_is_active - logger = logging.getLogger(__name__) @@ -696,7 +694,9 @@ class DomainRequest(TimeStampedModel): if recipient is None or recipient.email is None: logger.warning( f"Cannot send {new_status} email, no creator email address for domain request with pk: {self.pk}." - f" Name: {self.requested_domain.name}" if self.requested_domain else "" + f" Name: {self.requested_domain.name}" + if self.requested_domain + else "" ) return None From 2cde8743569a1eac8a2d8e78f366801a44b35b6e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Jun 2024 15:15:37 -0600 Subject: [PATCH 31/32] Fix weird bug --- src/registrar/assets/js/get-gov-admin.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 140421db5..94fdd8b07 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -426,6 +426,10 @@ function initializeWidgetOnList(list, parentId) { let statusSelect = document.getElementById('id_status'); function moveStatusChangelog(actionNeededReasonFormGroup, statusSelect) { + if (!actionNeededReasonFormGroup || !statusSelect) { + return; + } + let flexContainer = actionNeededReasonFormGroup.querySelector('.flex-container'); let statusChangelog = document.getElementById('dja-status-changelog'); From 04fb04279b17ec9d9441d9de7fb942bc92c16c65 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 28 Jun 2024 08:33:40 -0700 Subject: [PATCH 32/32] updated title and added newline --- docs/architecture/decisions/0026-django-waffle-library.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/architecture/decisions/0026-django-waffle-library.md b/docs/architecture/decisions/0026-django-waffle-library.md index d04cab531..256aaf5ed 100644 --- a/docs/architecture/decisions/0026-django-waffle-library.md +++ b/docs/architecture/decisions/0026-django-waffle-library.md @@ -1,4 +1,4 @@ -# 24. Production Release Cadence +# 26. Django Waffle library for Feature Flags Date: 2024-07-06 @@ -20,6 +20,7 @@ This brought us to finding solutions that could fix one or both of these problem The environment allows developers to set a true or false value to the given variable, allowing implementation over multiple sprints when new features are encapsulated with this variable. The feature shows when the variable is on (true); otherwise, it remains hidden. Environment variables are also innate to Django, making them free to use; on top of that, we already use them for other things in our code. The downside is that you would need to go to cloud.gov or use the cf CLI to see the current settings on a sandbox. This is very technical, meaning only developers would really be able to see what features were set, and we would be the only ones able to adjust them. It would also be easy to accidentally have the feature on or off without noticing. This also would not solve the problem of turning features on and off quickly for a given user group. + **Option 2:** Feature branches Like environment variables, using feature branches would be free and allow us to iterate on developing big features over multiple sprints. We would make a feature branch that developers working on that feature would push and pull from to iterate on. This quickly brings us to the downsides of this approach.