From a554c6a5507f9fec38c689e289a666f9eff8096d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 6 Jan 2025 11:35:44 -0600 Subject: [PATCH 01/57] new logging logic to separate out error logs --- src/registrar/config/settings.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 050950c9b..b6b87c6eb 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -486,14 +486,6 @@ class JsonServerFormatter(ServerFormatter): log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) - -# default to json formatted logs -server_formatter, console_formatter = "json.server", "json" - -# don't use json format locally, it makes logs hard to read in console -if "localhost" in env_base_url: - server_formatter, console_formatter = "django.server", "verbose" - LOGGING = { "version": 1, # Don't import Django's existing loggers @@ -526,23 +518,35 @@ LOGGING = { "console": { "level": env_log_level, "class": "logging.StreamHandler", - "formatter": console_formatter, + "formatter": "verbose", + "filters": ["below_error"], }, "django.server": { "level": "INFO", "class": "logging.StreamHandler", - "formatter": server_formatter, + "formatter": "django.server", + }, + "json": { + "level": "ERROR", + "class": "logging.StreamHandler", + "formatter": "json", }, # No file logger is configured, # because containerized apps # do not log to the file system. }, + "filters": { + "below_error": { + "()": "django.utils.log.CallbackFilter", + "callback": lambda record: record.levelno < logging.ERROR, + } + }, # define loggers: these are "sinks" into which # messages are sent for processing "loggers": { # Django's generic logger "django": { - "handlers": ["console"], + "handlers": ["console", "json"], "level": "INFO", "propagate": False, }, From 1cc2bd55f815441d1f719b5872bf4e8f12dcbb07 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 6 Jan 2025 15:40:03 -0600 Subject: [PATCH 02/57] test logging without json --- src/registrar/config/settings.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index b6b87c6eb..e0d32274d 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -519,13 +519,16 @@ LOGGING = { "level": env_log_level, "class": "logging.StreamHandler", "formatter": "verbose", - "filters": ["below_error"], + # "filters": ["below_error"], }, "django.server": { "level": "INFO", "class": "logging.StreamHandler", "formatter": "django.server", }, + # log all messages at ERROR level or higher using json formatter + # We do this because error logs often comprise many lines, + # and json formatting makes them easier to parse. "json": { "level": "ERROR", "class": "logging.StreamHandler", @@ -535,7 +538,9 @@ LOGGING = { # because containerized apps # do not log to the file system. }, + # filters are used to filter messages based on a callback function "filters": { + # filter for messages below ERROR level "below_error": { "()": "django.utils.log.CallbackFilter", "callback": lambda record: record.levelno < logging.ERROR, @@ -546,7 +551,7 @@ LOGGING = { "loggers": { # Django's generic logger "django": { - "handlers": ["console", "json"], + "handlers": ["console"], "level": "INFO", "propagate": False, }, From a3346d666c938b9b709c78965250ecffbe51a6df Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 21 Jan 2025 11:47:53 -0600 Subject: [PATCH 03/57] Revert "test logging without json" This reverts commit 1cc2bd55f815441d1f719b5872bf4e8f12dcbb07. --- src/registrar/config/settings.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index e0d32274d..b6b87c6eb 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -519,16 +519,13 @@ LOGGING = { "level": env_log_level, "class": "logging.StreamHandler", "formatter": "verbose", - # "filters": ["below_error"], + "filters": ["below_error"], }, "django.server": { "level": "INFO", "class": "logging.StreamHandler", "formatter": "django.server", }, - # log all messages at ERROR level or higher using json formatter - # We do this because error logs often comprise many lines, - # and json formatting makes them easier to parse. "json": { "level": "ERROR", "class": "logging.StreamHandler", @@ -538,9 +535,7 @@ LOGGING = { # because containerized apps # do not log to the file system. }, - # filters are used to filter messages based on a callback function "filters": { - # filter for messages below ERROR level "below_error": { "()": "django.utils.log.CallbackFilter", "callback": lambda record: record.levelno < logging.ERROR, @@ -551,7 +546,7 @@ LOGGING = { "loggers": { # Django's generic logger "django": { - "handlers": ["console"], + "handlers": ["console", "json"], "level": "INFO", "propagate": False, }, From db97a3f715c5517c11a0b3e0f03fc235b87ef731 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 21 Jan 2025 15:57:38 -0600 Subject: [PATCH 04/57] add logic to conditionally set log level --- ops/manifests/manifest-ab.yaml | 2 ++ ops/manifests/manifest-ad.yaml | 2 ++ ops/manifests/manifest-ag.yaml | 2 ++ ops/manifests/manifest-backup.yaml | 2 ++ ops/manifests/manifest-bob.yaml | 2 ++ ops/manifests/manifest-cb.yaml | 2 ++ ops/manifests/manifest-development.yaml | 2 ++ ops/manifests/manifest-dk.yaml | 2 ++ ops/manifests/manifest-el.yaml | 2 ++ ops/manifests/manifest-es.yaml | 2 ++ ops/manifests/manifest-gd.yaml | 2 ++ ops/manifests/manifest-hotgov.yaml | 2 ++ ops/manifests/manifest-ko.yaml | 2 ++ ops/manifests/manifest-ky.yaml | 2 ++ ops/manifests/manifest-litterbox.yaml | 2 ++ ops/manifests/manifest-meoward.yaml | 2 ++ ops/manifests/manifest-ms.yaml | 2 ++ ops/manifests/manifest-nl.yaml | 2 ++ ops/manifests/manifest-rb.yaml | 2 ++ ops/manifests/manifest-rh.yaml | 2 ++ ops/manifests/manifest-rjm.yaml | 2 ++ ops/manifests/manifest-stable.yaml | 2 ++ ops/manifests/manifest-staging.yaml | 2 ++ src/registrar/config/settings.py | 28 ++++++++++++++++++------- 24 files changed, 67 insertions(+), 7 deletions(-) diff --git a/ops/manifests/manifest-ab.yaml b/ops/manifests/manifest-ab.yaml index 3ca800392..f5a3e2c3c 100644 --- a/ops/manifests/manifest-ab.yaml +++ b/ops/manifests/manifest-ab.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ab.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-ad.yaml b/ops/manifests/manifest-ad.yaml index 73d6f96ff..6975f9f50 100644 --- a/ops/manifests/manifest-ad.yaml +++ b/ops/manifests/manifest-ad.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ad.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-ag.yaml b/ops/manifests/manifest-ag.yaml index 68d630f3e..192b58edb 100644 --- a/ops/manifests/manifest-ag.yaml +++ b/ops/manifests/manifest-ag.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ag.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-backup.yaml b/ops/manifests/manifest-backup.yaml index ab9e36d68..194b6e91c 100644 --- a/ops/manifests/manifest-backup.yaml +++ b/ops/manifests/manifest-backup.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-backup.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-bob.yaml b/ops/manifests/manifest-bob.yaml index f39d9e145..7af7e1df5 100644 --- a/ops/manifests/manifest-bob.yaml +++ b/ops/manifests/manifest-bob.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-bob.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-cb.yaml b/ops/manifests/manifest-cb.yaml index b9be98d27..e08f800fa 100644 --- a/ops/manifests/manifest-cb.yaml +++ b/ops/manifests/manifest-cb.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-cb.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-development.yaml b/ops/manifests/manifest-development.yaml index 23558ba4c..957cb0227 100644 --- a/ops/manifests/manifest-development.yaml +++ b/ops/manifests/manifest-development.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-development.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-dk.yaml b/ops/manifests/manifest-dk.yaml index 071efb416..6afbe9321 100644 --- a/ops/manifests/manifest-dk.yaml +++ b/ops/manifests/manifest-dk.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-dk.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-el.yaml b/ops/manifests/manifest-el.yaml index 4c7d4d4e4..ee5673700 100644 --- a/ops/manifests/manifest-el.yaml +++ b/ops/manifests/manifest-el.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-el.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-es.yaml b/ops/manifests/manifest-es.yaml index 7fd19b7a0..f0fc73d7e 100644 --- a/ops/manifests/manifest-es.yaml +++ b/ops/manifests/manifest-es.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-es.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-gd.yaml b/ops/manifests/manifest-gd.yaml index 89a7c2169..5c4f83cc5 100644 --- a/ops/manifests/manifest-gd.yaml +++ b/ops/manifests/manifest-gd.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-gd.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-hotgov.yaml b/ops/manifests/manifest-hotgov.yaml index 70cc97ee7..2aa37817a 100644 --- a/ops/manifests/manifest-hotgov.yaml +++ b/ops/manifests/manifest-hotgov.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-hotgov.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-ko.yaml b/ops/manifests/manifest-ko.yaml index a69493f9b..adc3dcc89 100644 --- a/ops/manifests/manifest-ko.yaml +++ b/ops/manifests/manifest-ko.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ko.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-ky.yaml b/ops/manifests/manifest-ky.yaml index f416d7385..292b0575c 100644 --- a/ops/manifests/manifest-ky.yaml +++ b/ops/manifests/manifest-ky.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ky.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-litterbox.yaml b/ops/manifests/manifest-litterbox.yaml index ae899ef3a..e2ab5489c 100644 --- a/ops/manifests/manifest-litterbox.yaml +++ b/ops/manifests/manifest-litterbox.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-litterbox.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-meoward.yaml b/ops/manifests/manifest-meoward.yaml index c47d9529d..ba452684e 100644 --- a/ops/manifests/manifest-meoward.yaml +++ b/ops/manifests/manifest-meoward.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-meoward.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-ms.yaml b/ops/manifests/manifest-ms.yaml index ac46f5d92..0068dfa02 100644 --- a/ops/manifests/manifest-ms.yaml +++ b/ops/manifests/manifest-ms.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ms.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: DEBUG + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-nl.yaml b/ops/manifests/manifest-nl.yaml index d74174e7d..fbf3b0f5f 100644 --- a/ops/manifests/manifest-nl.yaml +++ b/ops/manifests/manifest-nl.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-nl.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-rb.yaml b/ops/manifests/manifest-rb.yaml index 570b49dde..02b099bdd 100644 --- a/ops/manifests/manifest-rb.yaml +++ b/ops/manifests/manifest-rb.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-rb.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-rh.yaml b/ops/manifests/manifest-rh.yaml index f44894ce8..abce35140 100644 --- a/ops/manifests/manifest-rh.yaml +++ b/ops/manifests/manifest-rh.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-rh.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-rjm.yaml b/ops/manifests/manifest-rjm.yaml index 048b44e95..b51db1b95 100644 --- a/ops/manifests/manifest-rjm.yaml +++ b/ops/manifests/manifest-rjm.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-rjm.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/ops/manifests/manifest-stable.yaml b/ops/manifests/manifest-stable.yaml index 80c97339f..438a012a9 100644 --- a/ops/manifests/manifest-stable.yaml +++ b/ops/manifests/manifest-stable.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://manage.get.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: json # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Which OIDC provider to use diff --git a/ops/manifests/manifest-staging.yaml b/ops/manifests/manifest-staging.yaml index 38099cf17..7679b7248 100644 --- a/ops/manifests/manifest-staging.yaml +++ b/ops/manifests/manifest-staging.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-staging.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: json # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index b6b87c6eb..f2ccf5d93 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -60,6 +60,7 @@ env_db_url = env.dj_db_url("DATABASE_URL") env_debug = env.bool("DJANGO_DEBUG", default=False) env_is_production = env.bool("IS_PRODUCTION", default=False) env_log_level = env.str("DJANGO_LOG_LEVEL", "DEBUG") +env_log_format = env.str("DJANGO_LOG_FORMAT", "console") env_base_url: str = env.str("DJANGO_BASE_URL") env_getgov_public_site_url = env.str("GETGOV_PUBLIC_SITE_URL", "") env_oidc_active_provider = env.str("OIDC_ACTIVE_PROVIDER", "identity sandbox") @@ -485,6 +486,24 @@ class JsonServerFormatter(ServerFormatter): log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) + +# Define console handler outside LOGGING so we can conditionally enablefilters +console_handler = { + "level": env_log_level, + "class": "logging.StreamHandler", + "formatter": "verbose", +} + +if env_log_format == "json": + # in production we need everything to be logged as json so that log levels are parsed correctly + django_handlers = ["json"] +else: + # for non-production environments, send non-error messages to console handler + # we do this because json clutters logs when debugging + django_handlers = ["console", "json"] + # Only add below_error filter for non-production environments + console_handler["filters"] = ["below_error"] + LOGGING = { "version": 1, @@ -515,12 +534,7 @@ LOGGING = { # define where log messages will be sent; # each logger can have one or more handlers "handlers": { - "console": { - "level": env_log_level, - "class": "logging.StreamHandler", - "formatter": "verbose", - "filters": ["below_error"], - }, + "console": console_handler, "django.server": { "level": "INFO", "class": "logging.StreamHandler", @@ -546,7 +560,7 @@ LOGGING = { "loggers": { # Django's generic logger "django": { - "handlers": ["console", "json"], + "handlers": django_handlers, "level": "INFO", "propagate": False, }, From a6de5293c2fdb51971e87f6ac7bc89b36061dbaf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 23 Jan 2025 15:15:24 -0700 Subject: [PATCH 05/57] Core logic --- src/registrar/forms/domain_request_wizard.py | 20 ++++++++++++++++++-- src/registrar/utility/waffle.py | 16 ++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 7c9dcb180..c1ea2dda1 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -2,7 +2,8 @@ from __future__ import annotations # allows forward references in annotations import logging from api.views import DOMAIN_API_MESSAGES from phonenumber_field.formfields import PhoneNumberField # type: ignore - +from registrar.models.portfolio import Portfolio +from registrar.utility.waffle import flag_is_active_anywhere from django import forms from django.core.validators import RegexValidator, MaxLengthValidator from django.utils.safestring import mark_safe @@ -321,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), + queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) organization_name = forms.CharField( @@ -363,6 +364,21 @@ class OrganizationContactForm(RegistrarForm): label="Urbanization (required for Puerto Rico only)", ) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # Set the queryset for federal agency. + # If the organization_requests flag is active, we hide data that exists in portfolios. + # NOTE: This function assumes that the federal_agency field was first set to None if a portfolio exists. + federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) + if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): + # Exclude both predefined agencies and those matching portfolio names in one query + federal_agency_queryset = federal_agency_queryset.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True) + ) + + self.fields["federal_agency"].queryset = federal_agency_queryset + def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" federal_agency = self.cleaned_data.get("federal_agency", None) diff --git a/src/registrar/utility/waffle.py b/src/registrar/utility/waffle.py index a78799e4c..f97a18874 100644 --- a/src/registrar/utility/waffle.py +++ b/src/registrar/utility/waffle.py @@ -1,5 +1,6 @@ from django.http import HttpRequest from waffle.decorators import flag_is_active +from waffle.models import get_waffle_flag_model def flag_is_active_for_user(user, flag_name): @@ -10,3 +11,18 @@ def flag_is_active_for_user(user, flag_name): request = HttpRequest() request.user = user return flag_is_active(request, flag_name) + + +def flag_is_active_anywhere(flag_name): + """Checks if the given flag name is active for anyone, anywhere. + More specifically, it checks on flag.everyone or flag.users.exists(). + Does not check self.superuser, self.staff or self.group. + + This function effectively behaves like a switch: + If said flag is enabled for someone, somewhere - return true. + Otherwise - return false. + """ + flag = get_waffle_flag_model().get(flag_name) + if flag.everyone is None: + return flag.users.exists() + return flag.everyone From a4f1abc0011556e8557a17909edeb58e1e3cd693 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 24 Jan 2025 08:12:10 -0700 Subject: [PATCH 06/57] add check --- src/registrar/models/domain_request.py | 26 ++++++++++++++++++++++++-- src/registrar/views/domain_request.py | 10 +--------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 3d3aac769..49fddbe18 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -14,8 +14,7 @@ from registrar.utility.constants import BranchChoices from auditlog.models import LogEntry from django.core.exceptions import ValidationError -from registrar.utility.waffle import flag_is_active_for_user - +from registrar.utility.waffle import flag_is_active_for_user, flag_is_active_anywhere from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError from itertools import chain @@ -1289,6 +1288,29 @@ class DomainRequest(TimeStampedModel): return True return False + def unlock_organization_contact(self) -> bool: + """Unlocks the organization_contact step.""" + + # NOTE: This check may struggle with a high number of portfolios, consider something else then. + if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): + # Check if the current federal agency is an outlawed one + Portfolio = apps.get_model("registrar.Portfolio") + return FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ).filter(agency=self.federal_agency).exists() + + # NOTE: Shouldn't this be an AND on all required fields? + return ( + self.domain_request.federal_agency is not None + or self.domain_request.organization_name is not None + or self.domain_request.address_line1 is not None + or self.domain_request.city is not None + or self.domain_request.state_territory is not None + or self.domain_request.zipcode is not None + or self.domain_request.urbanization is not None + ) + + # ## Form policies ## # # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 9754b0d0c..b530dc7ae 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -107,15 +107,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): Step.TRIBAL_GOVERNMENT: lambda self: self.domain_request.tribe_name is not None, Step.ORGANIZATION_FEDERAL: lambda self: self.domain_request.federal_type is not None, Step.ORGANIZATION_ELECTION: lambda self: self.domain_request.is_election_board is not None, - Step.ORGANIZATION_CONTACT: lambda self: ( - self.domain_request.federal_agency is not None - or self.domain_request.organization_name is not None - or self.domain_request.address_line1 is not None - or self.domain_request.city is not None - or self.domain_request.state_territory is not None - or self.domain_request.zipcode is not None - or self.domain_request.urbanization is not None - ), + Step.ORGANIZATION_CONTACT: lambda self: self.from_model("unlock_organization_contact", False), Step.ABOUT_YOUR_ORGANIZATION: lambda self: self.domain_request.about_your_organization is not None, Step.SENIOR_OFFICIAL: lambda self: self.domain_request.senior_official is not None, Step.CURRENT_SITES: lambda self: ( From 297029a447504749af194fab2065240634f21a6a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 11:14:37 -0700 Subject: [PATCH 07/57] Add UI logic --- src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 151 +----------------- .../includes/request_review_steps.html | 2 +- src/registrar/views/domain_request.py | 9 +- 4 files changed, 12 insertions(+), 151 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb481db7a..c869dfa67 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,6 +244,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 49fddbe18..1456742dd 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1290,8 +1290,6 @@ class DomainRequest(TimeStampedModel): def unlock_organization_contact(self) -> bool: """Unlocks the organization_contact step.""" - - # NOTE: This check may struggle with a high number of portfolios, consider something else then. if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one Portfolio = apps.get_model("registrar.Portfolio") @@ -1301,16 +1299,15 @@ class DomainRequest(TimeStampedModel): # NOTE: Shouldn't this be an AND on all required fields? return ( - self.domain_request.federal_agency is not None - or self.domain_request.organization_name is not None - or self.domain_request.address_line1 is not None - or self.domain_request.city is not None - or self.domain_request.state_territory is not None - or self.domain_request.zipcode is not None - or self.domain_request.urbanization is not None + self.federal_agency is not None + or self.organization_name is not None + or self.address_line1 is not None + or self.city is not None + or self.state_territory is not None + or self.zipcode is not None + or self.urbanization is not None ) - # ## Form policies ## # # # These methods control what questions need to be answered by applicants @@ -1408,140 +1405,6 @@ class DomainRequest(TimeStampedModel): names = [n for n in [self.cisa_representative_first_name, self.cisa_representative_last_name] if n] return " ".join(names) if names else "Unknown" - def _is_federal_complete(self): - # Federal -> "Federal government branch" page can't be empty + Federal Agency selection can't be None - return not (self.federal_type is None or self.federal_agency is None) - - def _is_interstate_complete(self): - # Interstate -> "About your organization" page can't be empty - return self.about_your_organization is not None - - def _is_state_or_territory_complete(self): - # State -> ""Election office" page can't be empty - return self.is_election_board is not None - - def _is_tribal_complete(self): - # Tribal -> "Tribal name" and "Election office" page can't be empty - return self.tribe_name is not None and self.is_election_board is not None - - def _is_county_complete(self): - # County -> "Election office" page can't be empty - return self.is_election_board is not None - - def _is_city_complete(self): - # City -> "Election office" page can't be empty - return self.is_election_board is not None - - def _is_special_district_complete(self): - # Special District -> "Election office" and "About your organization" page can't be empty - return self.is_election_board is not None and self.about_your_organization is not None - - # Do we still want to test this after creator is autogenerated? Currently it went back to being selectable - def _is_creator_complete(self): - return self.creator is not None - - def _is_organization_name_and_address_complete(self): - return not ( - self.organization_name is None - and self.address_line1 is None - and self.city is None - and self.state_territory is None - and self.zipcode is None - ) - - def _is_senior_official_complete(self): - return self.senior_official is not None - - def _is_requested_domain_complete(self): - return self.requested_domain is not None - - def _is_purpose_complete(self): - return self.purpose is not None - - def _has_other_contacts_and_filled(self): - # Other Contacts Radio button is Yes and if all required fields are filled - return ( - self.has_other_contacts() - and self.other_contacts.filter( - first_name__isnull=False, - last_name__isnull=False, - title__isnull=False, - email__isnull=False, - phone__isnull=False, - ).exists() - ) - - def _has_no_other_contacts_gives_rationale(self): - # Other Contacts Radio button is No and a rationale is provided - return self.has_other_contacts() is False and self.no_other_contacts_rationale is not None - - def _is_other_contacts_complete(self): - if self._has_other_contacts_and_filled() or self._has_no_other_contacts_gives_rationale(): - return True - return False - - def _cisa_rep_check(self): - # Either does not have a CISA rep, OR has a CISA rep + both first name - # and last name are NOT empty and are NOT an empty string - to_return = ( - self.has_cisa_representative is True - and self.cisa_representative_first_name is not None - and self.cisa_representative_first_name != "" - and self.cisa_representative_last_name is not None - and self.cisa_representative_last_name != "" - ) or self.has_cisa_representative is False - - return to_return - - def _anything_else_radio_button_and_text_field_check(self): - # Anything else boolean is True + filled text field and it's not an empty string OR the boolean is No - return ( - self.has_anything_else_text is True and self.anything_else is not None and self.anything_else != "" - ) or self.has_anything_else_text is False - - def _is_additional_details_complete(self): - return self._cisa_rep_check() and self._anything_else_radio_button_and_text_field_check() - - def _is_policy_acknowledgement_complete(self): - return self.is_policy_acknowledged is not None - - def _is_general_form_complete(self, request): - return ( - self._is_creator_complete() - and self._is_organization_name_and_address_complete() - and self._is_senior_official_complete() - and self._is_requested_domain_complete() - and self._is_purpose_complete() - and self._is_other_contacts_complete() - and self._is_additional_details_complete() - and self._is_policy_acknowledgement_complete() - ) - - def _form_complete(self, request): - match self.generic_org_type: - case DomainRequest.OrganizationChoices.FEDERAL: - is_complete = self._is_federal_complete() - case DomainRequest.OrganizationChoices.INTERSTATE: - is_complete = self._is_interstate_complete() - case DomainRequest.OrganizationChoices.STATE_OR_TERRITORY: - is_complete = self._is_state_or_territory_complete() - case DomainRequest.OrganizationChoices.TRIBAL: - is_complete = self._is_tribal_complete() - case DomainRequest.OrganizationChoices.COUNTY: - is_complete = self._is_county_complete() - case DomainRequest.OrganizationChoices.CITY: - is_complete = self._is_city_complete() - case DomainRequest.OrganizationChoices.SPECIAL_DISTRICT: - is_complete = self._is_special_district_complete() - case DomainRequest.OrganizationChoices.SCHOOL_DISTRICT: - is_complete = True - case _: - # NOTE: Shouldn't happen, this is only if somehow they didn't choose an org type - is_complete = False - if not is_complete or not self._is_general_form_complete(request): - return False - return True - """The following converted_ property methods get field data from this domain request's portfolio, if there is an associated portfolio. If not, they return data from the domain request model.""" diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index 6151d01a8..96ba7f151 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -41,7 +41,7 @@ {% endif %} {% if step == Step.ORGANIZATION_CONTACT %} - {% if domain_request.organization_name %} + {% if domain_request.unlock_organization_contact %} {% with title=form_titles|get_item:step value=domain_request %} {% include "includes/summary_item.html" with title=title value=value heading_level=heading_level editable=is_editable edit_link=domain_request_url address='true' %} {% endwith %} diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index b530dc7ae..f200cc3ae 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -434,12 +434,8 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): requested_domain_name = self.domain_request.requested_domain.name context = {} - - # Note: we will want to consolidate the non_org_steps_complete check into the same check that - # org_steps_complete is using at some point. - non_org_steps_complete = DomainRequest._form_complete(self.domain_request, self.request) org_steps_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) - if (not self.is_portfolio and non_org_steps_complete) or (self.is_portfolio and org_steps_complete): + if org_steps_complete: context = { "form_titles": self.titles, "steps": self.steps, @@ -774,7 +770,8 @@ class Review(DomainRequestWizard): forms = [] # type: ignore def get_context_data(self): - if DomainRequest._form_complete(self.domain_request, self.request) is False: + form_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + if form_complete is False: logger.warning("User arrived at review page with an incomplete form.") context = super().get_context_data() context["Step"] = self.get_step_enum().__members__ From 1f9efb7fc0baebd0c1c0d34bf4f835ae4e992a1e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 12:56:08 -0700 Subject: [PATCH 08/57] Add unit test for waffle utility --- src/registrar/forms/domain_request_wizard.py | 2 +- src/registrar/tests/test_utilities.py | 40 +++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 5668b3757..d27ccde89 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -322,6 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, + # We populate this queryset in init. We want to exclude agencies with a portfolio. queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) @@ -369,7 +370,6 @@ class OrganizationContactForm(RegistrarForm): # Set the queryset for federal agency. # If the organization_requests flag is active, we hide data that exists in portfolios. - # NOTE: This function assumes that the federal_agency field was first set to None if a portfolio exists. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Exclude both predefined agencies and those matching portfolio names in one query diff --git a/src/registrar/tests/test_utilities.py b/src/registrar/tests/test_utilities.py index 5a2234d66..60b74cd60 100644 --- a/src/registrar/tests/test_utilities.py +++ b/src/registrar/tests/test_utilities.py @@ -1,7 +1,8 @@ from django.test import TestCase from registrar.models import User from waffle.testutils import override_flag -from registrar.utility.waffle import flag_is_active_for_user +from waffle.models import get_waffle_flag_model +from registrar.utility.waffle import flag_is_active_for_user, flag_is_active_anywhere class FlagIsActiveForUserTest(TestCase): @@ -21,3 +22,40 @@ class FlagIsActiveForUserTest(TestCase): # Test that the flag is inactive for the user is_active = flag_is_active_for_user(self.user, "test_flag") self.assertFalse(is_active) + + +class TestFlagIsActiveAnywhere(TestCase): + def setUp(self): + self.user = User.objects.create_user(username="testuser") + self.flag_name = "test_flag" + + @override_flag("test_flag", active=True) + def test_flag_active_for_everyone(self): + """Test when flag is active for everyone""" + is_active = flag_is_active_anywhere("test_flag") + self.assertTrue(is_active) + + @override_flag("test_flag", active=False) + def test_flag_inactive_for_everyone(self): + """Test when flag is inactive for everyone""" + is_active = flag_is_active_anywhere("test_flag") + self.assertFalse(is_active) + + def test_flag_active_for_some_users(self): + """Test when flag is active for specific users""" + flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") + flag.everyone = None + flag.save() + flag.users.add(self.user) + + is_active = flag_is_active_anywhere("test_flag") + self.assertTrue(is_active) + + def test_flag_inactive_with_no_users(self): + """Test when flag has no users and everyone is None""" + flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") + flag.everyone = None + flag.save() + + is_active = flag_is_active_anywhere("test_flag") + self.assertFalse(is_active) From bdb3875f1210671b9379ecccfdf0249b645ab382 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 14:07:16 -0700 Subject: [PATCH 09/57] Add tests for unlock_organization_contact --- src/registrar/tests/test_views_request.py | 41 +++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 81beba604..8e4871f79 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3221,6 +3221,47 @@ class TestDomainRequestWizard(TestWithUser, WebTest): federal_agency.delete() domain_request.delete() + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=True) + @less_console_noise_decorator + def test_unlock_organization_contact_flags_enabled(self): + """Tests unlock_organization_contact when agency exists in a portfolio""" + # Create a federal agency + federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") + + # Create a portfolio with matching organization name + Portfolio.objects.create( + creator=self.user, + organization_name=federal_agency.agency + ) + + # Create domain request with the portfolio agency + domain_request = completed_domain_request( + federal_agency=federal_agency, + user=self.user + ) + self.assertFalse(domain_request.unlock_organization_contact()) + + @override_flag("organization_feature", active=False) + @override_flag("organization_requests", active=False) + @less_console_noise_decorator + def test_unlock_organization_contact_flags_disabled(self): + """Tests unlock_organization_contact when organization flags are disabled""" + # Create a federal agency + federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") + + # Create a portfolio with matching organization name + Portfolio.objects.create( + creator=self.user, + organization_name=federal_agency.agency + ) + + domain_request = completed_domain_request( + federal_agency=federal_agency, + user=self.user + ) + self.assertTrue(domain_request.unlock_organization_contact()) + class TestPortfolioDomainRequestViewonly(TestWithUser, WebTest): From ad79557a55598cc4f67c33d11b01acf76a7024be Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 09:58:41 -0700 Subject: [PATCH 10/57] lint and fix test --- src/registrar/forms/domain_request_wizard.py | 4 +-- src/registrar/models/domain.py | 1 - src/registrar/models/domain_request.py | 10 +++++-- src/registrar/tests/test_utilities.py | 10 +++---- src/registrar/tests/test_views_request.py | 30 ++++++-------------- 5 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index d27ccde89..0cf82558b 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -367,7 +367,7 @@ class OrganizationContactForm(RegistrarForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - + # Set the queryset for federal agency. # If the organization_requests flag is active, we hide data that exists in portfolios. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) @@ -376,7 +376,7 @@ class OrganizationContactForm(RegistrarForm): federal_agency_queryset = federal_agency_queryset.exclude( agency__in=Portfolio.objects.values_list("organization_name", flat=True) ) - + self.fields["federal_agency"].queryset = federal_agency_queryset def clean_federal_agency(self): diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c869dfa67..cb481db7a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,7 +244,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 1456742dd..2d0dd50e5 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1293,9 +1293,13 @@ class DomainRequest(TimeStampedModel): if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one Portfolio = apps.get_model("registrar.Portfolio") - return FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), - ).filter(agency=self.federal_agency).exists() + return ( + FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ) + .filter(agency=self.federal_agency) + .exists() + ) # NOTE: Shouldn't this be an AND on all required fields? return ( diff --git a/src/registrar/tests/test_utilities.py b/src/registrar/tests/test_utilities.py index 60b74cd60..d882fdedd 100644 --- a/src/registrar/tests/test_utilities.py +++ b/src/registrar/tests/test_utilities.py @@ -28,29 +28,29 @@ class TestFlagIsActiveAnywhere(TestCase): def setUp(self): self.user = User.objects.create_user(username="testuser") self.flag_name = "test_flag" - + @override_flag("test_flag", active=True) def test_flag_active_for_everyone(self): """Test when flag is active for everyone""" is_active = flag_is_active_anywhere("test_flag") self.assertTrue(is_active) - + @override_flag("test_flag", active=False) def test_flag_inactive_for_everyone(self): """Test when flag is inactive for everyone""" is_active = flag_is_active_anywhere("test_flag") self.assertFalse(is_active) - + def test_flag_active_for_some_users(self): """Test when flag is active for specific users""" flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") flag.everyone = None flag.save() flag.users.add(self.user) - + is_active = flag_is_active_anywhere("test_flag") self.assertTrue(is_active) - + def test_flag_inactive_with_no_users(self): """Test when flag has no users and everyone is None""" flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 8e4871f79..c559a73c7 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3228,18 +3228,12 @@ class TestDomainRequestWizard(TestWithUser, WebTest): """Tests unlock_organization_contact when agency exists in a portfolio""" # Create a federal agency federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") - + # Create a portfolio with matching organization name - Portfolio.objects.create( - creator=self.user, - organization_name=federal_agency.agency - ) - + Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + # Create domain request with the portfolio agency - domain_request = completed_domain_request( - federal_agency=federal_agency, - user=self.user - ) + domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) self.assertFalse(domain_request.unlock_organization_contact()) @override_flag("organization_feature", active=False) @@ -3247,19 +3241,13 @@ class TestDomainRequestWizard(TestWithUser, WebTest): @less_console_noise_decorator def test_unlock_organization_contact_flags_disabled(self): """Tests unlock_organization_contact when organization flags are disabled""" - # Create a federal agency + # Create a federal agency federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") - - # Create a portfolio with matching organization name - Portfolio.objects.create( - creator=self.user, - organization_name=federal_agency.agency - ) - domain_request = completed_domain_request( - federal_agency=federal_agency, - user=self.user - ) + # Create a portfolio with matching organization name + Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + + domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) self.assertTrue(domain_request.unlock_organization_contact()) From 0478d2bee6078859d5b96e1076de0608fe2459f4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 14:36:40 -0700 Subject: [PATCH 11/57] Fix unit tests --- src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 13 ++- .../includes/request_review_steps.html | 2 +- src/registrar/tests/test_models.py | 91 +++++++++++-------- src/registrar/utility/waffle.py | 11 ++- src/registrar/views/domain_request.py | 32 +++++-- 6 files changed, 97 insertions(+), 53 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb481db7a..c869dfa67 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,6 +244,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 2d0dd50e5..c56d42c1e 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1300,8 +1300,6 @@ class DomainRequest(TimeStampedModel): .filter(agency=self.federal_agency) .exists() ) - - # NOTE: Shouldn't this be an AND on all required fields? return ( self.federal_agency is not None or self.organization_name is not None @@ -1312,6 +1310,17 @@ class DomainRequest(TimeStampedModel): or self.urbanization is not None ) + def unlock_other_contacts(self) -> bool: + """Unlocks the other contacts step""" + other_contacts_filled_out = self.other_contacts.filter( + first_name__isnull=False, + last_name__isnull=False, + title__isnull=False, + email__isnull=False, + phone__isnull=False, + ).exists() + return (self.has_other_contacts() and other_contacts_filled_out) or self.no_other_contacts_rationale is not None + # ## Form policies ## # # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index 96ba7f151..f1b13f890 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -116,7 +116,7 @@ {% endif %} {% if step == Step.OTHER_CONTACTS %} - {% if domain_request.other_contacts.all %} + {% if domain_request.unlock_other_contacts %} {% with title=form_titles|get_item:step value=domain_request.other_contacts.all %} {% include "includes/summary_item.html" with title=title value=value heading_level=heading_level editable=is_editable edit_link=domain_request_url contact='true' list='true' %} {% endwith %} diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index d8db0f043..45f8a15f0 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1,9 +1,10 @@ from django.forms import ValidationError from django.test import TestCase from unittest.mock import patch - +from unittest.mock import Mock from django.test import RequestFactory - +from waffle.models import get_waffle_flag_model +from registrar.views.domain_request import DomainRequestWizard from registrar.models import ( Contact, DomainRequest, @@ -1692,11 +1693,20 @@ class TestDomainRequestIncomplete(TestCase): anything_else="Anything else", is_policy_acknowledged=True, creator=self.user, + city="fake", ) - self.domain_request.other_contacts.add(other) self.domain_request.current_websites.add(current) self.domain_request.alternative_domains.add(alt) + self.wizard = DomainRequestWizard() + self.wizard._domain_request = self.domain_request + self.wizard.request = Mock(user=self.user, session={}) + self.wizard.kwargs = {"id": self.domain_request.id} + + # We use both of these flags in the test. In the normal app these are generated normally. + # The alternative syntax is adding the decorator to each test. + get_waffle_flag_model().objects.get_or_create(name="organization_feature") + get_waffle_flag_model().objects.get_or_create(name="organization_requests") def tearDown(self): super().tearDown() @@ -1709,66 +1719,67 @@ class TestDomainRequestIncomplete(TestCase): super().tearDownClass() cls.user.delete() - @less_console_noise_decorator + # @less_console_noise_decorator def test_is_federal_complete(self): - self.assertTrue(self.domain_request._is_federal_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.federal_type = None self.domain_request.save() - self.assertFalse(self.domain_request._is_federal_complete()) + self.domain_request.refresh_from_db() + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_interstate_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.INTERSTATE self.domain_request.about_your_organization = "Something something about your organization" self.domain_request.save() - self.assertTrue(self.domain_request._is_interstate_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.about_your_organization = None self.domain_request.save() - self.assertFalse(self.domain_request._is_interstate_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_state_or_territory_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.STATE_OR_TERRITORY self.domain_request.is_election_board = True self.domain_request.save() - self.assertTrue(self.domain_request._is_state_or_territory_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_state_or_territory_complete()) + self.assertFalse(self.wizard.form_is_complete()) - @less_console_noise_decorator + # @less_console_noise_decorator def test_is_tribal_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.TRIBAL self.domain_request.tribe_name = "Tribe Name" self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_tribal_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_tribal_complete()) + self.assertFalse(self.wizard.form_is_complete()) self.domain_request.tribe_name = None self.domain_request.save() - self.assertFalse(self.domain_request._is_tribal_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_county_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.COUNTY self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_county_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_county_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_city_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.CITY self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_city_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_city_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_special_district_complete(self): @@ -1776,55 +1787,55 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.about_your_organization = "Something something about your organization" self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_special_district_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_special_district_complete()) + self.assertFalse(self.wizard.form_is_complete()) self.domain_request.about_your_organization = None self.domain_request.save() - self.assertFalse(self.domain_request._is_special_district_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_organization_name_and_address_complete(self): - self.assertTrue(self.domain_request._is_organization_name_and_address_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.organization_name = None self.domain_request.address_line1 = None self.domain_request.save() - self.assertTrue(self.domain_request._is_organization_name_and_address_complete()) + self.assertTrue(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_senior_official_complete(self): - self.assertTrue(self.domain_request._is_senior_official_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.senior_official = None self.domain_request.save() - self.assertFalse(self.domain_request._is_senior_official_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_requested_domain_complete(self): - self.assertTrue(self.domain_request._is_requested_domain_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.requested_domain = None self.domain_request.save() - self.assertFalse(self.domain_request._is_requested_domain_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_purpose_complete(self): - self.assertTrue(self.domain_request._is_purpose_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.purpose = None self.domain_request.save() - self.assertFalse(self.domain_request._is_purpose_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_complete_missing_one_field(self): - self.assertTrue(self.domain_request._is_other_contacts_complete()) + self.assertTrue(self.wizard.form_is_complete()) contact = self.domain_request.other_contacts.first() contact.first_name = None contact.save() - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_complete_all_none(self): self.domain_request.other_contacts.clear() - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_False_and_has_rationale(self): @@ -1832,7 +1843,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.other_contacts.clear() self.domain_request.other_contacts.exists = False self.domain_request.no_other_contacts_rationale = "Some rationale" - self.assertTrue(self.domain_request._is_other_contacts_complete()) + self.assertTrue(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_False_and_NO_rationale(self): @@ -1840,7 +1851,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.other_contacts.clear() self.domain_request.other_contacts.exists = False self.domain_request.no_other_contacts_rationale = None - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_additional_details_complete(self): @@ -2044,28 +2055,28 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.save() self.domain_request.refresh_from_db() self.assertEqual( - self.domain_request._is_additional_details_complete(), + self.wizard.form_is_complete(), case["expected"], msg=f"Failed for case: {case}", ) @less_console_noise_decorator def test_is_policy_acknowledgement_complete(self): - self.assertTrue(self.domain_request._is_policy_acknowledgement_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_policy_acknowledged = False - self.assertTrue(self.domain_request._is_policy_acknowledgement_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_policy_acknowledged = None - self.assertFalse(self.domain_request._is_policy_acknowledgement_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_form_complete(self): request = self.factory.get("/") request.user = self.user - self.assertTrue(self.domain_request._form_complete(request)) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.generic_org_type = None self.domain_request.save() - self.assertFalse(self.domain_request._form_complete(request)) + self.assertFalse(self.wizard.form_is_complete()) class TestPortfolio(TestCase): diff --git a/src/registrar/utility/waffle.py b/src/registrar/utility/waffle.py index f97a18874..3071fbed9 100644 --- a/src/registrar/utility/waffle.py +++ b/src/registrar/utility/waffle.py @@ -22,7 +22,10 @@ def flag_is_active_anywhere(flag_name): If said flag is enabled for someone, somewhere - return true. Otherwise - return false. """ - flag = get_waffle_flag_model().get(flag_name) - if flag.everyone is None: - return flag.users.exists() - return flag.everyone + try: + flag = get_waffle_flag_model().get(flag_name) + if flag.everyone is None: + return flag.users.exists() + return flag.everyone + except get_waffle_flag_model().DoesNotExist: + return False diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index f200cc3ae..45d802764 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -115,9 +115,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): ), Step.DOTGOV_DOMAIN: lambda self: self.domain_request.requested_domain is not None, Step.PURPOSE: lambda self: self.domain_request.purpose is not None, - Step.OTHER_CONTACTS: lambda self: ( - self.domain_request.other_contacts.exists() or self.domain_request.no_other_contacts_rationale is not None - ), + Step.OTHER_CONTACTS: lambda self: self.from_model("unlock_other_contacts", False), Step.ADDITIONAL_DETAILS: lambda self: ( # Additional details is complete as long as "has anything else" and "has cisa rep" are not None ( @@ -425,16 +423,38 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): """Helper for get_context_data. Queries the DB for a domain request and returns a list of unlocked steps.""" return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] + + def form_is_complete(self): + """ + Determines if all required steps in the domain request form are complete. + + This method: + 1. Gets a list of all steps that have been completed (unlocked_steps) + 2. Filters the full step list to only include steps that should be shown based on + the wizard conditions. For example, federal-specific steps are only required + if the organization type is federal. + 3. Compares the number of completed steps to required steps to determine if + the form is complete. + + Returns: + bool: True if all required steps are complete, False otherwise + """ + unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + required_steps = set(self.steps.all) + unlocked_steps = set() + for step in required_steps: + if step in unlockable_steps: + unlocked_steps.add(step) + return required_steps == unlocked_steps def get_context_data(self): """Define context for access on all wizard pages.""" - requested_domain_name = None if self.domain_request.requested_domain is not None: requested_domain_name = self.domain_request.requested_domain.name context = {} - org_steps_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + org_steps_complete = self.form_is_complete() if org_steps_complete: context = { "form_titles": self.titles, @@ -770,7 +790,7 @@ class Review(DomainRequestWizard): forms = [] # type: ignore def get_context_data(self): - form_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + form_complete = self.form_is_complete() if form_complete is False: logger.warning("User arrived at review page with an incomplete form.") context = super().get_context_data() From 4dba0a31c35faf1eab2fdc19c34ca1b372728666 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 15:15:56 -0700 Subject: [PATCH 12/57] remove test code and lint --- src/registrar/models/domain.py | 1 - src/registrar/views/domain_request.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c869dfa67..cb481db7a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,7 +244,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 45d802764..e1f94391e 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -423,11 +423,11 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): """Helper for get_context_data. Queries the DB for a domain request and returns a list of unlocked steps.""" return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] - + def form_is_complete(self): """ Determines if all required steps in the domain request form are complete. - + This method: 1. Gets a list of all steps that have been completed (unlocked_steps) 2. Filters the full step list to only include steps that should be shown based on From b5935a36d690bfda88f7ec277f6e2313b28ca309 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:10:12 -0700 Subject: [PATCH 13/57] lint --- src/registrar/tests/test_models.py | 4 ++-- src/registrar/views/domain_request.py | 22 ++++++---------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 45f8a15f0..2a44f7765 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1719,7 +1719,7 @@ class TestDomainRequestIncomplete(TestCase): super().tearDownClass() cls.user.delete() - # @less_console_noise_decorator + @less_console_noise_decorator def test_is_federal_complete(self): self.assertTrue(self.wizard.form_is_complete()) self.domain_request.federal_type = None @@ -1747,7 +1747,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.save() self.assertFalse(self.wizard.form_is_complete()) - # @less_console_noise_decorator + @less_console_noise_decorator def test_is_tribal_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.TRIBAL self.domain_request.tribe_name = "Tribe Name" diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index e1f94391e..3248c1368 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -425,26 +425,16 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] def form_is_complete(self): - """ - Determines if all required steps in the domain request form are complete. - - This method: - 1. Gets a list of all steps that have been completed (unlocked_steps) - 2. Filters the full step list to only include steps that should be shown based on - the wizard conditions. For example, federal-specific steps are only required - if the organization type is federal. - 3. Compares the number of completed steps to required steps to determine if - the form is complete. - + """Determines if all required steps in the domain request form are complete. Returns: bool: True if all required steps are complete, False otherwise """ - unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + # 1. Get all steps visibly present to the user (required steps) + # 2. Return every possible step that is "unlocked" (even hidden, conditional ones) + # 3. Narrows down the list to remove hidden conditional steps required_steps = set(self.steps.all) - unlocked_steps = set() - for step in required_steps: - if step in unlockable_steps: - unlocked_steps.add(step) + unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + unlocked_steps = {step for step in required_steps if step in unlockable_steps} return required_steps == unlocked_steps def get_context_data(self): From 992e86f7bb4ec2519f65ab694fd89b326476888b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:57:16 -0700 Subject: [PATCH 14/57] First pass --- src/registrar/fixtures/fixtures_domains.py | 14 +-- src/registrar/fixtures/fixtures_portfolios.py | 69 +++++++-------- src/registrar/fixtures/fixtures_requests.py | 12 +-- .../fixtures_user_portfolio_permissions.py | 85 +++++++++---------- .../commands/remove_unused_portfolios.py | 22 ++--- src/registrar/utility/db_helpers.py | 17 ++-- 6 files changed, 110 insertions(+), 109 deletions(-) diff --git a/src/registrar/fixtures/fixtures_domains.py b/src/registrar/fixtures/fixtures_domains.py index 4606024d0..366789dfd 100644 --- a/src/registrar/fixtures/fixtures_domains.py +++ b/src/registrar/fixtures/fixtures_domains.py @@ -29,19 +29,19 @@ class DomainFixture(DomainRequestFixture): def load(cls): # Lumped under .atomic to ensure we don't make redundant DB calls. # This bundles them all together, and then saves it in a single call. - with transaction.atomic(): - try: + try: + with transaction.atomic(): # Get the usernames of users created in the UserFixture created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] # Filter users to only include those created by the fixture users = list(User.objects.filter(username__in=created_usernames)) - except Exception as e: - logger.warning(e) - return + except Exception as e: + logger.warning(e) + return - # Approve each user associated with `in review` status domains - cls._approve_domain_requests(users) + # Approve each user associated with `in review` status domains + cls._approve_domain_requests(users) @staticmethod def _generate_fake_expiration_date(days_in_future=365): diff --git a/src/registrar/fixtures/fixtures_portfolios.py b/src/registrar/fixtures/fixtures_portfolios.py index 2a391fb16..efb90e475 100644 --- a/src/registrar/fixtures/fixtures_portfolios.py +++ b/src/registrar/fixtures/fixtures_portfolios.py @@ -87,39 +87,40 @@ class PortfolioFixture: # Lumped under .atomic to ensure we don't make redundant DB calls. # This bundles them all together, and then saves it in a single call. - with transaction.atomic(): - try: + try: + with transaction.atomic(): user = User.objects.all().last() + except Exception as e: + logger.warning(e) + return + + portfolios_to_create = [] + for portfolio_data in cls.PORTFOLIOS: + organization_name = portfolio_data["organization_name"] + + # Check if portfolio with the organization name already exists + if Portfolio.objects.filter(organization_name=organization_name).exists(): + logger.info( + f"Portfolio with organization name '{organization_name}' already exists, skipping creation." + ) + continue + + try: + with transaction.atomic(): + portfolio = Portfolio( + creator=user, + organization_name=portfolio_data["organization_name"], + ) + cls._set_non_foreign_key_fields(portfolio, portfolio_data) + cls._set_foreign_key_fields(portfolio, portfolio_data, user) + portfolios_to_create.append(portfolio) + except Exception as e: + logger.warning(e) + + # Bulk create domain requests + if len(portfolios_to_create) > 0: + try: + Portfolio.objects.bulk_create(portfolios_to_create) + logger.info(f"Successfully created {len(portfolios_to_create)} portfolios") except Exception as e: - logger.warning(e) - return - - portfolios_to_create = [] - for portfolio_data in cls.PORTFOLIOS: - organization_name = portfolio_data["organization_name"] - - # Check if portfolio with the organization name already exists - if Portfolio.objects.filter(organization_name=organization_name).exists(): - logger.info( - f"Portfolio with organization name '{organization_name}' already exists, skipping creation." - ) - continue - - try: - portfolio = Portfolio( - creator=user, - organization_name=portfolio_data["organization_name"], - ) - cls._set_non_foreign_key_fields(portfolio, portfolio_data) - cls._set_foreign_key_fields(portfolio, portfolio_data, user) - portfolios_to_create.append(portfolio) - except Exception as e: - logger.warning(e) - - # Bulk create domain requests - if len(portfolios_to_create) > 0: - try: - Portfolio.objects.bulk_create(portfolios_to_create) - logger.info(f"Successfully created {len(portfolios_to_create)} portfolios") - except Exception as e: - logger.warning(f"Error bulk creating portfolios: {e}") + logger.warning(f"Error bulk creating portfolios: {e}") diff --git a/src/registrar/fixtures/fixtures_requests.py b/src/registrar/fixtures/fixtures_requests.py index c4d824b37..142c7f5a9 100644 --- a/src/registrar/fixtures/fixtures_requests.py +++ b/src/registrar/fixtures/fixtures_requests.py @@ -309,18 +309,18 @@ class DomainRequestFixture: # The atomic block will cause the code to stop executing if one instance in the # nested iteration fails, which will cause an early exit and make it hard to debug. # Comment out with transaction.atomic() when debugging. - with transaction.atomic(): - try: + try: + with transaction.atomic(): # Get the usernames of users created in the UserFixture created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] # Filter users to only include those created by the fixture users = list(User.objects.filter(username__in=created_usernames)) - except Exception as e: - logger.warning(e) - return + except Exception as e: + logger.warning(e) + return - cls._create_domain_requests(users) + cls._create_domain_requests(users) @classmethod def _create_domain_requests(cls, users): # noqa: C901 diff --git a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py index 5f9fd64ef..1f547b231 100644 --- a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py +++ b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py @@ -26,56 +26,55 @@ class UserPortfolioPermissionFixture: # Lumped under .atomic to ensure we don't make redundant DB calls. # This bundles them all together, and then saves it in a single call. - with transaction.atomic(): - try: - # Get the usernames of users created in the UserFixture - created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] + try: + # Get the usernames of users created in the UserFixture + created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] - # Filter users to only include those created by the fixture - users = list(User.objects.filter(username__in=created_usernames)) + # Filter users to only include those created by the fixture + users = list(User.objects.filter(username__in=created_usernames)) - organization_names = [portfolio["organization_name"] for portfolio in PortfolioFixture.PORTFOLIOS] + organization_names = [portfolio["organization_name"] for portfolio in PortfolioFixture.PORTFOLIOS] - portfolios = list(Portfolio.objects.filter(organization_name__in=organization_names)) + portfolios = list(Portfolio.objects.filter(organization_name__in=organization_names)) - if not users: - logger.warning("User fixtures missing.") - return - - if not portfolios: - logger.warning("Portfolio fixtures missing.") - return - - except Exception as e: - logger.warning(e) + if not users: + logger.warning("User fixtures missing.") return - user_portfolio_permissions_to_create = [] - for user in users: - # Assign a random portfolio to a user - portfolio = random.choice(portfolios) # nosec - try: - if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists(): - user_portfolio_permission = UserPortfolioPermission( - user=user, - portfolio=portfolio, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - additional_permissions=[ - UserPortfolioPermissionChoices.EDIT_MEMBERS, - UserPortfolioPermissionChoices.EDIT_REQUESTS, - ], - ) - user_portfolio_permissions_to_create.append(user_portfolio_permission) - else: - logger.info( - f"Permission exists for user '{user.username}' " - f"on portfolio '{portfolio.organization_name}'." - ) - except Exception as e: - logger.warning(e) + if not portfolios: + logger.warning("Portfolio fixtures missing.") + return - # Bulk create permissions - cls._bulk_create_permissions(user_portfolio_permissions_to_create) + except Exception as e: + logger.warning(e) + return + + user_portfolio_permissions_to_create = [] + for user in users: + # Assign a random portfolio to a user + portfolio = random.choice(portfolios) # nosec + try: + if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists(): + user_portfolio_permission = UserPortfolioPermission( + user=user, + portfolio=portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.EDIT_MEMBERS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + user_portfolio_permissions_to_create.append(user_portfolio_permission) + else: + logger.info( + f"Permission exists for user '{user.username}' " + f"on portfolio '{portfolio.organization_name}'." + ) + except Exception as e: + logger.warning(e) + + # Bulk create permissions + cls._bulk_create_permissions(user_portfolio_permissions_to_create) @classmethod def _bulk_create_permissions(cls, user_portfolio_permissions_to_create): diff --git a/src/registrar/management/commands/remove_unused_portfolios.py b/src/registrar/management/commands/remove_unused_portfolios.py index 4940cc16f..859318a45 100644 --- a/src/registrar/management/commands/remove_unused_portfolios.py +++ b/src/registrar/management/commands/remove_unused_portfolios.py @@ -149,9 +149,9 @@ class Command(BaseCommand): ) return - with transaction.atomic(): - # Try to delete the portfolios - try: + # Try to delete the portfolios + try: + with transaction.atomic(): summary = [] for portfolio in portfolios_to_delete: portfolio_summary = [f"---- CASCADE SUMMARY for {portfolio.organization_name} -----"] @@ -222,14 +222,14 @@ class Command(BaseCommand): """ ) - except IntegrityError as e: - logger.info( - f"""{TerminalColors.FAIL} - Could not delete some portfolios due to integrity constraints: - {e} - {TerminalColors.ENDC} - """ - ) + except IntegrityError as e: + logger.info( + f"""{TerminalColors.FAIL} + Could not delete some portfolios due to integrity constraints: + {e} + {TerminalColors.ENDC} + """ + ) def handle(self, *args, **options): # Get all Portfolio entries not in the allowed portfolios list diff --git a/src/registrar/utility/db_helpers.py b/src/registrar/utility/db_helpers.py index 5b7e0392c..b6af059c1 100644 --- a/src/registrar/utility/db_helpers.py +++ b/src/registrar/utility/db_helpers.py @@ -9,12 +9,13 @@ def ignore_unique_violation(): Execute within an atomic transaction so that if a unique constraint violation occurs, the individual transaction is rolled back without invalidating any larger transaction. """ - with transaction.atomic(): - try: + try: + # NOTE - is transaction doing anything here?? + with transaction.atomic(): yield - except IntegrityError as e: - if e.__cause__.pgcode == errorcodes.UNIQUE_VIOLATION: - # roll back to the savepoint, effectively ignoring this transaction - pass - else: - raise e + except IntegrityError as e: + if e.__cause__.pgcode == errorcodes.UNIQUE_VIOLATION: + # roll back to the savepoint, effectively ignoring this transaction + pass + else: + raise e From a52e4aabf0f4358897ff2c8726944a751eea4d38 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 29 Jan 2025 15:19:34 -0600 Subject: [PATCH 15/57] fix issue where error logs were missing --- src/registrar/config/settings.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index f2ccf5d93..11c128cbe 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -494,16 +494,19 @@ console_handler = { "formatter": "verbose", } -if env_log_format == "json": +# If we're running locally we don't want json formatting +if 'localhost' in env_base_url: + django_handlers = ["console"] + console_filter = [] +elif env_log_format == "json": # in production we need everything to be logged as json so that log levels are parsed correctly django_handlers = ["json"] + console_filter = [] else: - # for non-production environments, send non-error messages to console handler + # for non-production non-local environments, send non-error messages to console handler # we do this because json clutters logs when debugging django_handlers = ["console", "json"] - # Only add below_error filter for non-production environments - console_handler["filters"] = ["below_error"] - + console_filter = ["below_error"] LOGGING = { "version": 1, @@ -533,15 +536,20 @@ LOGGING = { }, # define where log messages will be sent; # each logger can have one or more handlers - "handlers": { - "console": console_handler, + "handlers": { + "console": { + "level": env_log_level, + "class": "logging.StreamHandler", + "formatter": "verbose", + "filters": console_filter, + }, "django.server": { "level": "INFO", "class": "logging.StreamHandler", "formatter": "django.server", }, "json": { - "level": "ERROR", + "level": "ERROR" if env_log_format == "console" else env_log_level, "class": "logging.StreamHandler", "formatter": "json", }, From cc0c53006c6696a119d310bb76bbc762ba2b2e9f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 29 Jan 2025 15:43:53 -0700 Subject: [PATCH 16/57] Update test_forms.py --- src/registrar/tests/test_forms.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/tests/test_forms.py b/src/registrar/tests/test_forms.py index a2960deac..0589d8dec 100644 --- a/src/registrar/tests/test_forms.py +++ b/src/registrar/tests/test_forms.py @@ -23,6 +23,7 @@ from registrar.forms.portfolio import ( PortfolioMemberForm, PortfolioNewMemberForm, ) +from waffle.models import get_waffle_flag_model from registrar.models.portfolio import Portfolio from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user import User @@ -38,6 +39,10 @@ class TestFormValidation(MockEppLib): self.API_BASE_PATH = "/api/v1/available/?domain=" self.user = get_user_model().objects.create(username="username") self.factory = RequestFactory() + # We use both of these flags in the test. In the normal app these are generated normally. + # The alternative syntax is adding the decorator to each test. + get_waffle_flag_model().objects.get_or_create(name="organization_feature") + get_waffle_flag_model().objects.get_or_create(name="organization_requests") def test_org_contact_zip_invalid(self): form = OrganizationContactForm(data={"zipcode": "nah"}) From 37f52aa9d4136cdc15be5602d65109f75a270008 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 07:34:20 -0500 Subject: [PATCH 17/57] portfolio addition email send to admins on registrar invite --- .../portfolio_admin_addition_notification.txt | 40 +++++++++++++ ...io_admin_addition_notification_subject.txt | 1 + src/registrar/utility/email_invitations.py | 58 ++++++++++++++++++- src/registrar/views/portfolios.py | 11 +++- 4 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 src/registrar/templates/emails/portfolio_admin_addition_notification.txt create mode 100644 src/registrar/templates/emails/portfolio_admin_addition_notification_subject.txt diff --git a/src/registrar/templates/emails/portfolio_admin_addition_notification.txt b/src/registrar/templates/emails/portfolio_admin_addition_notification.txt new file mode 100644 index 000000000..b8953aa67 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_admin_addition_notification.txt @@ -0,0 +1,40 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi,{% if portfolio_admin and portfolio_admin.first_name %} {{ portfolio_admin.first_name }}.{% endif %} + +An admin was invited to your .gov organization. + +ORGANIZATION: {{ portfolio.organization_name }} +INVITED BY: {{ requestor_email }} +INVITED ON: {{date}} +ADMIN INVITED: {{ invited_email_address }} + +---------------------------------------------------------------- + +NEXT STEPS +The person who received the invitation will become an admin once they log in to the +.gov registrar. They'll need to access the registrar using a Login.gov account that's +associated with the invited email address. + +If you need to cancel this invitation or remove the admin, you can do that by going to +the Members section for your organization . + + +WHY DID YOU RECEIVE THIS EMAIL? +Youโ€™re listed as an admin for {{ portfolio.organization_name }}. That means you'll receive a notification +whenever a new admin is invited to that organization. + +If you have questions or concerns, reach out to the person who sent the invitation or reply to this email. + + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Learn about .gov + +The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency +(CISA) +{% endautoescape %} diff --git a/src/registrar/templates/emails/portfolio_admin_addition_notification_subject.txt b/src/registrar/templates/emails/portfolio_admin_addition_notification_subject.txt new file mode 100644 index 000000000..3d6b2a140 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_admin_addition_notification_subject.txt @@ -0,0 +1 @@ +An admin was invited to your .gov organization \ No newline at end of file diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index f9c3b89b2..b1377d09a 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -1,6 +1,8 @@ from datetime import date from django.conf import settings from registrar.models import Domain, DomainInvitation, UserDomainRole +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, @@ -169,7 +171,7 @@ def send_invitation_email(email, requestor_email, domains, requested_user): raise EmailSendingError(f"Could not send email invitation to {email} for domains: {domain_names}") from err -def send_portfolio_invitation_email(email: str, requestor, portfolio): +def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_invitation): """ Sends a portfolio member invitation email to the specified address. @@ -179,6 +181,10 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): email (str): Email address of the recipient requestor (User): The user initiating the invitation. portfolio (Portfolio): The portfolio object for which the invitation is being sent. + is_admin_invitation (boolean): boolean indicating if the invitation is an admin invitation + + Returns: + Boolean indicating if all messages were sent successfully. Raises: MissingEmailError: If the requestor has no email associated with their account. @@ -210,3 +216,53 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): raise EmailSendingError( f"Could not sent email invitation to {email} for portfolio {portfolio}. Portfolio invitation not saved." ) from err + + all_admin_emails_sent = True + # send emails to portfolio admins + if is_admin_invitation: + all_admin_emails_sent = send_portfolio_admin_addition_emails_to_portfolio_admins( + email=email, + requestor_email=requestor_email, + portfolio=portfolio, + requested_user=None, + ) + return all_admin_emails_sent + + +def send_portfolio_admin_addition_emails_to_portfolio_admins( + email: str, requestor_email, portfolio: Domain, requested_user=None +): + """ + Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin + + Returns: + Boolean indicating if all messages were sent successfully. + """ + all_emails_sent = True + # Get each portfolio admin from list + user_portfolio_permissions = UserPortfolioPermission.objects.filter( + portfolio=portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ).exclude(user__email=email) + for user_portfolio_permission in user_portfolio_permissions: + # Send email to each portfolio_admin + user = user_portfolio_permission.user + try: + send_templated_email( + "emails/portfolio_admin_addition_notification.txt", + "emails/portfolio_admin_addition_notification_subject.txt", + to_address=user.email, + context={ + "portfolio": portfolio, + "requestor_email": requestor_email, + "invited_email_address": email, + "portfolio_admin": user, + "date": date.today(), + }, + ) + except EmailSendingError: + logger.warning( + f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", + exc_info=True, + ) + all_emails_sent = False + return all_emails_sent diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 8ea135cfd..e2743b864 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -773,12 +773,19 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): requested_email = form.cleaned_data["email"] requestor = self.request.user portfolio = form.cleaned_data["portfolio"] + is_admin_invitation = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in form.cleaned_data["roles"] requested_user = User.objects.filter(email=requested_email).first() permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() try: if not requested_user or not permission_exists: - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) + if not send_portfolio_invitation_email( + email=requested_email, + requestor=requestor, + portfolio=portfolio, + is_admin_invitation=is_admin_invitation, + ): + messages.warning(self.request, "Could not send email notification to existing organization admins.") portfolio_invitation = form.save() # if user exists for email, immediately retrieve portfolio invitation upon creation if requested_user is not None: @@ -801,7 +808,7 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): portfolio, exc_info=True, ) - messages.warning(self.request, "Could not send portfolio email invitation.") + messages.error(self.request, "Could not send organization invitation email.") elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) logger.error( From 1bd73b6794a97ab538a1f8389e1b026d17a3ace7 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 08:35:22 -0500 Subject: [PATCH 18/57] added support in forms and views for sending email on change from member to admin --- src/registrar/forms/portfolio.py | 26 +++++++++++++++++++++ src/registrar/utility/email_invitations.py | 23 ++++++++---------- src/registrar/views/portfolios.py | 27 +++++++++++++++++++++- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index e57b56c4f..b1e6bad8e 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -293,6 +293,32 @@ class BasePortfolioMemberForm(forms.ModelForm): selected_domain_permission = next((perm for perm in domain_perms if perm in perms), "no_access") self.initial["domain_request_permission_member"] = selected_domain_permission + def is_change_from_member_to_admin(self) -> bool: + """ + Checks if the roles have changed from not containing ORGANIZATION_ADMIN + to containing ORGANIZATION_ADMIN. + """ + previous_roles = set(self.initial.get("roles", [])) # Initial roles before change + new_roles = set(self.cleaned_data.get("roles", [])) # New roles after change + + return ( + UserPortfolioRoleChoices.ORGANIZATION_ADMIN not in previous_roles + and UserPortfolioRoleChoices.ORGANIZATION_ADMIN in new_roles + ) + + def is_change_from_admin_to_member(self) -> bool: + """ + Checks if the roles have changed from containing ORGANIZATION_ADMIN + to not containing ORGANIZATION_ADMIN. + """ + previous_roles = set(self.initial.get("roles", [])) # Initial roles before change + new_roles = set(self.cleaned_data.get("roles", [])) # New roles after change + + return ( + UserPortfolioRoleChoices.ORGANIZATION_ADMIN in previous_roles + and UserPortfolioRoleChoices.ORGANIZATION_ADMIN not in new_roles + ) + class PortfolioMemberForm(BasePortfolioMemberForm): """ diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index b1377d09a..4e315cdda 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -40,7 +40,7 @@ def send_domain_invitation_email( EmailSendingError: If there is an error while sending the email. """ domains = normalize_domains(domains) - requestor_email = get_requestor_email(requestor, domains) + requestor_email = get_requestor_email(requestor, domains=domains) _validate_invitation(email, requested_user, domains, requestor, is_member_of_different_org) @@ -99,17 +99,22 @@ def normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: return [domains] if isinstance(domains, Domain) else domains -def get_requestor_email(requestor, domains): +def get_requestor_email(requestor, domains=None, portfolio=None): """Get the requestor's email or raise an error if it's missing. If the requestor is staff, default email is returned. + + Raises: + MissingEmailError """ if requestor.is_staff: return settings.DEFAULT_FROM_EMAIL if not requestor.email or requestor.email.strip() == "": - domain_names = ", ".join([domain.name for domain in domains]) - raise MissingEmailError(email=requestor.email, domain=domain_names) + domain_names = None + if domains: + domain_names = ", ".join([domain.name for domain in domains]) + raise MissingEmailError(email=requestor.email, domain=domain_names, portfolio=portfolio) return requestor.email @@ -191,15 +196,7 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i EmailSendingError: If there is an error while sending the email. """ - # Default email address for staff - requestor_email = settings.DEFAULT_FROM_EMAIL - - # Check if the requestor is staff and has an email - if not requestor.is_staff: - if not requestor.email or requestor.email.strip() == "": - raise MissingEmailError(email=email, portfolio=portfolio) - else: - requestor_email = requestor.email + requestor_email = get_requestor_email(requestor, portfolio=portfolio) try: send_templated_email( diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index e2743b864..a629309cc 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -15,7 +15,7 @@ from registrar.models.user_domain_role import UserDomainRole from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError -from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_invitation_email +from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_admin_addition_emails, send_portfolio_invitation_email from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues from registrar.views.utility.mixins import PortfolioMemberPermission @@ -405,6 +405,19 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) form = self.form_class(request.POST, instance=portfolio_invitation) if form.is_valid(): + try: + if form.is_change_from_member_to_admin(): + if not send_portfolio_admin_addition_emails( + email=portfolio_invitation.email, + requestor=request.user, + portfolio=portfolio_invitation.portfolio + ): + messages.warning(self.request, "Could not send email notification to existing organization admins.") + elif form.is_change_from_admin_to_member(): + # NOTE: need to add portfolio_admin_removal_emails when ready + pass + except Exception as e: + self._handle_exceptions(e) form.save() messages.success(self.request, "The member access and permission changes have been saved.") return redirect("invitedmember", pk=pk) @@ -418,6 +431,18 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): }, ) + def _handle_exceptions(self, exception): + """Handle exceptions raised during the process.""" + if isinstance(exception, MissingEmailError): + messages.warning(self.request, "Could not send email notification to existing organization admins.") + logger.warning( + f"Could not send email notification to existing organization admins.", + exc_info=True, + ) + else: + logger.warning("Could not send email notification to existing organization admins.", exc_info=True) + messages.warning(self.request, "Could not send email notification to existing organization admins.") + class PortfolioInvitedMemberDomainsView(PortfolioMemberDomainsPermissionView, View): From 59d1301fd72be651e49b0a39ad7d08b133783ee5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 30 Jan 2025 08:40:24 -0700 Subject: [PATCH 19/57] Update test_views_request.py --- src/registrar/tests/test_views_request.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index c559a73c7..9638ff669 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3081,17 +3081,26 @@ class TestDomainRequestWizard(TestWithUser, WebTest): contact = Contact.objects.create( first_name="Henry", last_name="Mcfakerson", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) # Create two non-orphaned contacts contact_2 = Contact.objects.create( first_name="Saturn", last_name="Mars", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) # Attach a user object to a contact (should not be deleted) contact_user, _ = Contact.objects.get_or_create( first_name="Hank", last_name="McFakey", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) site = DraftDomain.objects.create(name="igorville.gov") From 714a7232311be3659eb2c79819bf476c3386decc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 30 Jan 2025 08:58:37 -0700 Subject: [PATCH 20/57] lint --- src/registrar/tests/test_views_request.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 9638ff669..d33d7e6ad 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3079,28 +3079,16 @@ class TestDomainRequestWizard(TestWithUser, WebTest): # Create the site and contacts to delete (orphaned) contact = Contact.objects.create( - first_name="Henry", - last_name="Mcfakerson", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Henry", last_name="Mcfakerson", title="test", email="moar@igorville.gov", phone="1234567890" ) # Create two non-orphaned contacts contact_2 = Contact.objects.create( - first_name="Saturn", - last_name="Mars", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Saturn", last_name="Mars", title="test", email="moar@igorville.gov", phone="1234567890" ) # Attach a user object to a contact (should not be deleted) contact_user, _ = Contact.objects.get_or_create( - first_name="Hank", - last_name="McFakey", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Hank", last_name="McFakey", title="test", email="moar@igorville.gov", phone="1234567890" ) site = DraftDomain.objects.create(name="igorville.gov") From 04b375fae1e790e1704606e25d82d89bca9bb488 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 11:01:57 -0500 Subject: [PATCH 21/57] removal email template --- .../portfolio_admin_removal_notification.txt | 33 +++++++++++++++++++ ...lio_admin_removal_notification_subject.txt | 1 + 2 files changed, 34 insertions(+) create mode 100644 src/registrar/templates/emails/portfolio_admin_removal_notification.txt create mode 100644 src/registrar/templates/emails/portfolio_admin_removal_notification_subject.txt diff --git a/src/registrar/templates/emails/portfolio_admin_removal_notification.txt b/src/registrar/templates/emails/portfolio_admin_removal_notification.txt new file mode 100644 index 000000000..6a536aa49 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_admin_removal_notification.txt @@ -0,0 +1,33 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi,{% if portfolio_admin and portfolio_admin.first_name %} {{ portfolio_admin.first_name }}.{% endif %} + +An admin was removed from your .gov organization. + +ORGANIZATION: {{ portfolio.organization_name }} +REMOVED BY: {{ requestor_email }} +REMOVED ON: {{date}} +ADMIN REMOVED: {{ removed_email_address }} + +You can view this update by going to the Members section for your .gov organization . + +---------------------------------------------------------------- + +WHY DID YOU RECEIVE THIS EMAIL? +Youโ€™re listed as an admin for {{ portfolio.organization_name }}. That means you'll receive a notification +whenever an admin is removed from that organization. + +If you have questions or concerns, reach out to the person who removed the admin or reply to this email. + + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Learn about .gov + +The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency +(CISA) +{% endautoescape %} diff --git a/src/registrar/templates/emails/portfolio_admin_removal_notification_subject.txt b/src/registrar/templates/emails/portfolio_admin_removal_notification_subject.txt new file mode 100644 index 000000000..e250b17f8 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_admin_removal_notification_subject.txt @@ -0,0 +1 @@ +An admin was removed from your .gov organization \ No newline at end of file From 344d1f1c7f3567094d151eee16da8056247202ac Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 30 Jan 2025 10:01:32 -0700 Subject: [PATCH 22/57] Update fixtures_user_portfolio_permissions.py --- src/registrar/fixtures/fixtures_user_portfolio_permissions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py index 1f547b231..e2c84f817 100644 --- a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py +++ b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py @@ -1,7 +1,6 @@ import logging import random from faker import Faker -from django.db import transaction from registrar.fixtures.fixtures_portfolios import PortfolioFixture from registrar.fixtures.fixtures_users import UserFixture From 87c5a13a202d66e35808bf2ac662c5f48031beea Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 30 Jan 2025 11:04:54 -0600 Subject: [PATCH 23/57] change how json log levels are handled --- src/registrar/config/settings.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 11c128cbe..8af259ad1 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -498,16 +498,18 @@ console_handler = { if 'localhost' in env_base_url: django_handlers = ["console"] console_filter = [] + json_log_level = "ERROR" elif env_log_format == "json": # in production we need everything to be logged as json so that log levels are parsed correctly django_handlers = ["json"] console_filter = [] + json_log_level = env_log_level else: # for non-production non-local environments, send non-error messages to console handler # we do this because json clutters logs when debugging django_handlers = ["console", "json"] console_filter = ["below_error"] - + json_log_level = "ERROR" LOGGING = { "version": 1, # Don't import Django's existing loggers @@ -549,7 +551,7 @@ LOGGING = { "formatter": "django.server", }, "json": { - "level": "ERROR" if env_log_format == "console" else env_log_level, + "level": json_log_level, "class": "logging.StreamHandler", "formatter": "json", }, From 0fd76d040969e0685152dcdb41218e15f056272e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 30 Jan 2025 11:51:11 -0600 Subject: [PATCH 24/57] another attempt at fixing split level logging --- src/registrar/config/settings.py | 37 ++++++++++++++++---------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 8af259ad1..13c4d8e69 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -487,29 +487,19 @@ class JsonServerFormatter(ServerFormatter): log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) -# Define console handler outside LOGGING so we can conditionally enablefilters -console_handler = { - "level": env_log_level, - "class": "logging.StreamHandler", - "formatter": "verbose", -} - # If we're running locally we don't want json formatting if 'localhost' in env_base_url: django_handlers = ["console"] - console_filter = [] - json_log_level = "ERROR" elif env_log_format == "json": # in production we need everything to be logged as json so that log levels are parsed correctly django_handlers = ["json"] - console_filter = [] - json_log_level = env_log_level else: - # for non-production non-local environments, send non-error messages to console handler - # we do this because json clutters logs when debugging - django_handlers = ["console", "json"] - console_filter = ["below_error"] - json_log_level = "ERROR" + # for non-production non-local environments: + # - send ERROR and above to json handler + # - send below ERROR to console handler with verbose formatting + # yes this is janky but it's the best we can do for now + django_handlers = ["split_console", "split_json"] + LOGGING = { "version": 1, # Don't import Django's existing loggers @@ -543,7 +533,18 @@ LOGGING = { "level": env_log_level, "class": "logging.StreamHandler", "formatter": "verbose", - "filters": console_filter, + }, + # Special handlers for split logging case + "split_console": { + "level": env_log_level, + "class": "logging.StreamHandler", + "formatter": "verbose", + "filters": ["below_error"], + }, + "split_json": { + "level": "ERROR", + "class": "logging.StreamHandler", + "formatter": "json", }, "django.server": { "level": "INFO", @@ -551,7 +552,7 @@ LOGGING = { "formatter": "django.server", }, "json": { - "level": json_log_level, + "level": env_log_level, "class": "logging.StreamHandler", "formatter": "json", }, From 4ce9efffcdfc1783bab23f90a16ef39248d10f45 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 15:06:00 -0500 Subject: [PATCH 25/57] updates to complete initial implementation of invitation emails to admins --- src/registrar/admin.py | 15 ++++++++-- src/registrar/tests/test_email_invitations.py | 10 +++---- src/registrar/utility/email_invitations.py | 28 +++++++++++++------ src/registrar/views/domain.py | 4 ++- src/registrar/views/portfolios.py | 12 ++++++-- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7dbe7abb0..a9944c4c0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1543,7 +1543,9 @@ class DomainInvitationAdmin(BaseInvitationAdmin): and not member_of_this_org and not member_of_a_different_org ): - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=domain_org) + send_portfolio_invitation_email( + email=requested_email, requestor=requestor, portfolio=domain_org, is_admin_invitation=False + ) portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create( email=requested_email, portfolio=domain_org, @@ -1638,6 +1640,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): portfolio = obj.portfolio requested_email = obj.email requestor = request.user + is_admin_invitation = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in obj.roles # Look up a user with that email requested_user = get_requested_user(requested_email) @@ -1647,7 +1650,15 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): try: if not permission_exists: # if permission does not exist for a user with requested_email, send email - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) + if not send_portfolio_invitation_email( + email=requested_email, + requestor=requestor, + portfolio=portfolio, + is_admin_invitation=is_admin_invitation, + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) # if user exists for email, immediately retrieve portfolio invitation upon creation if requested_user is not None: obj.retrieve() diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 1377dec42..1914e73bd 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -16,7 +16,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations.send_templated_email") @patch("registrar.utility.email_invitations.UserDomainRole.objects.filter") @patch("registrar.utility.email_invitations._validate_invitation") - @patch("registrar.utility.email_invitations.get_requestor_email") + @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") @patch("registrar.utility.email_invitations.normalize_domains") def test_send_domain_invitation_email( @@ -81,7 +81,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations.send_templated_email") @patch("registrar.utility.email_invitations.UserDomainRole.objects.filter") @patch("registrar.utility.email_invitations._validate_invitation") - @patch("registrar.utility.email_invitations.get_requestor_email") + @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") @patch("registrar.utility.email_invitations.normalize_domains") def test_send_domain_invitation_email_multiple_domains( @@ -197,7 +197,7 @@ class DomainInvitationEmail(unittest.TestCase): mock_validate_invitation.assert_called_once() @less_console_noise_decorator - @patch("registrar.utility.email_invitations.get_requestor_email") + @patch("registrar.utility.email_invitations._get_requestor_email") def test_send_domain_invitation_email_raises_get_requestor_email_exception(self, mock_get_requestor_email): """Test sending domain invitation email for one domain and assert exception when get_requestor_email fails. @@ -217,7 +217,7 @@ class DomainInvitationEmail(unittest.TestCase): @less_console_noise_decorator @patch("registrar.utility.email_invitations._validate_invitation") - @patch("registrar.utility.email_invitations.get_requestor_email") + @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") @patch("registrar.utility.email_invitations.normalize_domains") def test_send_domain_invitation_email_raises_sending_email_exception( @@ -267,7 +267,7 @@ class DomainInvitationEmail(unittest.TestCase): @less_console_noise_decorator @patch("registrar.utility.email_invitations.send_emails_to_domain_managers") @patch("registrar.utility.email_invitations._validate_invitation") - @patch("registrar.utility.email_invitations.get_requestor_email") + @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") @patch("registrar.utility.email_invitations.normalize_domains") def test_send_domain_invitation_email_manager_emails_send_mail_exception( diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 4e315cdda..209c8b392 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -1,6 +1,7 @@ from datetime import date from django.conf import settings from registrar.models import Domain, DomainInvitation, UserDomainRole +from registrar.models.portfolio import Portfolio from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.errors import ( @@ -40,7 +41,7 @@ def send_domain_invitation_email( EmailSendingError: If there is an error while sending the email. """ domains = normalize_domains(domains) - requestor_email = get_requestor_email(requestor, domains=domains) + requestor_email = _get_requestor_email(requestor, domains=domains) _validate_invitation(email, requested_user, domains, requestor, is_member_of_different_org) @@ -99,7 +100,7 @@ def normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: return [domains] if isinstance(domains, Domain) else domains -def get_requestor_email(requestor, domains=None, portfolio=None): +def _get_requestor_email(requestor, domains=None, portfolio=None): """Get the requestor's email or raise an error if it's missing. If the requestor is staff, default email is returned. @@ -196,7 +197,7 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i EmailSendingError: If there is an error while sending the email. """ - requestor_email = get_requestor_email(requestor, portfolio=portfolio) + requestor_email = _get_requestor_email(requestor, portfolio=portfolio) try: send_templated_email( @@ -217,18 +218,29 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i all_admin_emails_sent = True # send emails to portfolio admins if is_admin_invitation: - all_admin_emails_sent = send_portfolio_admin_addition_emails_to_portfolio_admins( + all_admin_emails_sent = _send_portfolio_admin_addition_emails_to_portfolio_admins( email=email, requestor_email=requestor_email, portfolio=portfolio, - requested_user=None, ) return all_admin_emails_sent -def send_portfolio_admin_addition_emails_to_portfolio_admins( - email: str, requestor_email, portfolio: Domain, requested_user=None -): +def send_portfolio_admin_addition_emails(email: str, requestor, portfolio: Portfolio): + """ + Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin + + Returns: + Boolean indicating if all messages were sent successfully. + + Raises: + MissingEmailError + """ + requestor_email = _get_requestor_email(requestor, portfolio=portfolio) + return _send_portfolio_admin_addition_emails_to_portfolio_admins(email, requestor_email, portfolio) + + +def _send_portfolio_admin_addition_emails_to_portfolio_admins(email: str, requestor_email, portfolio: Portfolio): """ Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index a1d2b8081..464e0d2a1 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1206,7 +1206,9 @@ class DomainAddUserView(DomainFormBaseView): and requestor_can_update_portfolio and not member_of_this_org ): - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=domain_org) + send_portfolio_invitation_email( + email=requested_email, requestor=requestor, portfolio=domain_org, is_admin_invitation=False + ) portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create( email=requested_email, portfolio=domain_org, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 28933ec65..b8003b622 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -15,7 +15,11 @@ from registrar.models.user_domain_role import UserDomainRole from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError -from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_admin_addition_emails, send_portfolio_invitation_email +from registrar.utility.email_invitations import ( + send_domain_invitation_email, + send_portfolio_admin_addition_emails, + send_portfolio_invitation_email, +) from registrar.utility.errors import MissingEmailError from registrar.utility.enums import DefaultUserValues from registrar.views.utility.mixins import PortfolioMemberPermission @@ -418,9 +422,11 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): if not send_portfolio_admin_addition_emails( email=portfolio_invitation.email, requestor=request.user, - portfolio=portfolio_invitation.portfolio + portfolio=portfolio_invitation.portfolio, ): - messages.warning(self.request, "Could not send email notification to existing organization admins.") + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) elif form.is_change_from_admin_to_member(): # NOTE: need to add portfolio_admin_removal_emails when ready pass From f786d13cdf70e1a03ab9ef97d022e5047e101471 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 15:45:30 -0500 Subject: [PATCH 26/57] handle demoted member --- src/registrar/views/portfolios.py | 53 ++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index b8003b622..74e53d8c1 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -18,6 +18,7 @@ from registrar.utility.email import EmailSendingError from registrar.utility.email_invitations import ( send_domain_invitation_email, send_portfolio_admin_addition_emails, + send_portfolio_admin_removal_emails, send_portfolio_invitation_email, ) from registrar.utility.errors import MissingEmailError @@ -185,12 +186,29 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): user = portfolio_permission.user form = self.form_class(request.POST, instance=portfolio_permission) if form.is_valid(): - # Check if user is removing their own admin or edit role - removing_admin_role_on_self = ( - request.user == user - and user_initially_is_admin - and UserPortfolioRoleChoices.ORGANIZATION_ADMIN not in form.cleaned_data.get("role", []) - ) + try: + if form.is_change_from_member_to_admin(): + if not send_portfolio_admin_addition_emails( + email=portfolio_permission.user.email, + requestor=request.user, + portfolio=portfolio_permission.portfolio + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) + elif form.is_change_from_admin_to_member(): + if not send_portfolio_admin_removal_emails( + email=portfolio_permission.user.email, + requestor=request.user, + portfolio=portfolio_permission.portfolio + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) + # Check if user is removing their own admin or edit role + removing_admin_role_on_self = (request.user == user) + except Exception as e: + self._handle_exceptions(e) form.save() messages.success(self.request, "The member access and permission changes have been saved.") return redirect("member", pk=pk) if not removing_admin_role_on_self else redirect("home") @@ -203,6 +221,19 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): "member": user, # Pass the user object again to the template }, ) + + def _handle_exceptions(self, exception): + """Handle exceptions raised during the process.""" + if isinstance(exception, MissingEmailError): + messages.warning(self.request, "Could not send email notification to existing organization admins.") + logger.warning( + f"Could not send email notification to existing organization admins.", + exc_info=True, + ) + else: + logger.warning("Could not send email notification to existing organization admins.", exc_info=True) + messages.warning(self.request, "Could not send email notification to existing organization admins.") + class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): @@ -428,8 +459,14 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): self.request, "Could not send email notification to existing organization admins." ) elif form.is_change_from_admin_to_member(): - # NOTE: need to add portfolio_admin_removal_emails when ready - pass + if not send_portfolio_admin_removal_emails( + email=portfolio_invitation.email, + requestor=request.user, + portfolio=portfolio_invitation.portfolio + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) except Exception as e: self._handle_exceptions(e) form.save() From b1f528b9a25671cde2dd7832bc147f700b80bb0a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 16:30:18 -0500 Subject: [PATCH 27/57] handle removals --- src/registrar/utility/email_invitations.py | 51 ++++++++++++++++++++ src/registrar/views/portfolios.py | 55 +++++++++++++++++++++- 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 209c8b392..8f6cdd35a 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -275,3 +275,54 @@ def _send_portfolio_admin_addition_emails_to_portfolio_admins(email: str, reques ) all_emails_sent = False return all_emails_sent + + +def send_portfolio_admin_removal_emails(email: str, requestor, portfolio: Portfolio): + """ + Notifies all portfolio admins of the provided portfolio of a removed portfolio admin + + Returns: + Boolean indicating if all messages were sent successfully. + + Raises: + MissingEmailError + """ + requestor_email = _get_requestor_email(requestor, portfolio=portfolio) + return _send_portfolio_admin_removal_emails_to_portfolio_admins(email, requestor_email, portfolio) + + +def _send_portfolio_admin_removal_emails_to_portfolio_admins(email: str, requestor_email, portfolio: Portfolio): + """ + Notifies all portfolio admins of the provided portfolio of a removed portfolio admin + + Returns: + Boolean indicating if all messages were sent successfully. + """ + all_emails_sent = True + # Get each portfolio admin from list + user_portfolio_permissions = UserPortfolioPermission.objects.filter( + portfolio=portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ).exclude(user__email=email) + for user_portfolio_permission in user_portfolio_permissions: + # Send email to each portfolio_admin + user = user_portfolio_permission.user + try: + send_templated_email( + "emails/portfolio_admin_removal_notification.txt", + "emails/portfolio_admin_removal_notification_subject.txt", + to_address=user.email, + context={ + "portfolio": portfolio, + "requestor_email": requestor_email, + "removed_email_address": email, + "portfolio_admin": user, + "date": date.today(), + }, + ) + except EmailSendingError: + logger.warning( + f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", + exc_info=True, + ) + all_emails_sent = False + return all_emails_sent diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 74e53d8c1..c79072537 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -148,6 +148,21 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.error(request, error_message) return redirect(reverse("member", kwargs={"pk": pk})) + # if member being removed is an admin + if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: + try: + # attempt to send notification emails of the removal to other portfolio admins + if not send_portfolio_admin_removal_emails( + email=portfolio_member_permission.user.email, + requestor=request.user, + portfolio=portfolio_member_permission.portfolio + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) + except Exception as e: + self._handle_exceptions(e) + # passed all error conditions portfolio_member_permission.delete() @@ -159,6 +174,18 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.success(request, success_message) return redirect(reverse("members")) + def _handle_exceptions(self, exception): + """Handle exceptions raised during the process.""" + if isinstance(exception, MissingEmailError): + messages.warning(self.request, "Could not send email notification to existing organization admins.") + logger.warning( + f"Could not send email notification to existing organization admins.", + exc_info=True, + ) + else: + logger.warning("Could not send email notification to existing organization admins.", exc_info=True) + messages.warning(self.request, "Could not send email notification to existing organization admins.") + class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): @@ -182,7 +209,6 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): def post(self, request, pk): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) - user_initially_is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_permission.roles user = portfolio_permission.user form = self.form_class(request.POST, instance=portfolio_permission) if form.is_valid(): @@ -415,6 +441,21 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): """ portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) + # if invitation being removed is an admin + if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_invitation.roles: + try: + # attempt to send notification emails of the removal to portfolio admins + if not send_portfolio_admin_removal_emails( + email=portfolio_invitation.email, + requestor=request.user, + portfolio=portfolio_invitation.portfolio + ): + messages.warning( + self.request, "Could not send email notification to existing organization admins." + ) + except Exception as e: + self._handle_exceptions(e) + portfolio_invitation.delete() success_message = f"You've removed {portfolio_invitation.email} from the organization." @@ -425,6 +466,18 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): messages.success(request, success_message) return redirect(reverse("members")) + def _handle_exceptions(self, exception): + """Handle exceptions raised during the process.""" + if isinstance(exception, MissingEmailError): + messages.warning(self.request, "Could not send email notification to existing organization admins.") + logger.warning( + f"Could not send email notification to existing organization admins.", + exc_info=True, + ) + else: + logger.warning("Could not send email notification to existing organization admins.", exc_info=True) + messages.warning(self.request, "Could not send email notification to existing organization admins.") + class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): From 25b95d6794d55084ab79d3add278da97b4ae88df Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 30 Jan 2025 15:31:30 -0600 Subject: [PATCH 28/57] one more try --- src/registrar/config/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 13c4d8e69..970a10e00 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -607,7 +607,7 @@ LOGGING = { }, # Our app! "registrar": { - "handlers": ["console"], + "handlers": django_handlers, "level": "DEBUG", "propagate": False, }, From 5523d0b28f01bbc8add956b52709e52dac1b38ea Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 30 Jan 2025 15:37:26 -0600 Subject: [PATCH 29/57] add more logger handling --- src/registrar/config/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 970a10e00..1db44a0a7 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -577,7 +577,7 @@ LOGGING = { }, # Django's template processor "django.template": { - "handlers": ["console"], + "handlers": django_handlers, "level": "INFO", "propagate": False, }, From 4cc6452c6a1cd94ea71018653ccb2340c2916bc4 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 30 Jan 2025 16:31:50 -0600 Subject: [PATCH 30/57] fix oidc logging --- src/registrar/config/settings.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 1db44a0a7..2bf9ac70a 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -595,13 +595,13 @@ LOGGING = { }, # OpenID Connect logger "oic": { - "handlers": ["console"], + "handlers": django_handlers, "level": "INFO", "propagate": False, }, # Django wrapper for OpenID Connect "djangooidc": { - "handlers": ["console"], + "handlers": django_handlers, "level": "INFO", "propagate": False, }, @@ -615,7 +615,7 @@ LOGGING = { # root logger catches anything, unless # defined by a more specific logger "root": { - "handlers": ["console"], + "handlers": django_handlers, "level": "INFO", }, } From 3a473ccb505caf2ab224d538786aedbed9987536 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 30 Jan 2025 15:32:40 -0800 Subject: [PATCH 31/57] remove the domain request export --- src/registrar/config/urls.py | 6 ------ .../templates/includes/domain_requests_table.html | 15 +-------------- src/registrar/views/report_views.py | 10 ---------- 3 files changed, 1 insertion(+), 30 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index beb38e104..eb095c5ca 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -20,7 +20,6 @@ from registrar.views.report_views import ( AnalyticsView, ExportDomainRequestDataFull, ExportDataTypeUser, - ExportDataTypeRequests, ExportMembersPortfolio, ) @@ -260,11 +259,6 @@ urlpatterns = [ ExportDataTypeUser.as_view(), name="export_data_type_user", ), - path( - "reports/export_data_type_requests/", - ExportDataTypeRequests.as_view(), - name="export_data_type_requests", - ), path( "domain-request//edit/", views.DomainRequestWizard.as_view(), diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index b026a7a6b..ca64e1bff 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -51,20 +51,7 @@ - {% if portfolio %} -
-
- - -
-
- {% endif %} + {% if portfolio %} diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index ee2c079f3..e1a4a7d81 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,16 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response -class ExportDataTypeRequests(DomainAndRequestsReportsPermission, View): - """Returns a domain requests report for a given user on the request""" - - def get(self, request, *args, **kwargs): - response = HttpResponse(content_type="text/csv") - response["Content-Disposition"] = 'attachment; filename="domain-requests.csv"' - csv_export.DomainRequestDataType.export_data_to_csv(response, request=request) - - return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): From cbf23d80e7960646277efd2b40a755b2950c7174 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 30 Jan 2025 15:49:47 -0800 Subject: [PATCH 32/57] added lint --- src/registrar/views/report_views.py | 1 - src/registrar/views/utility/mixins.py | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index e1a4a7d81..1b9198c79 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,7 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): def get(self, request, *args, **kwargs): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 2d121849e..a05334169 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -189,9 +189,7 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ): + if not self.request.user.has_view_members_portfolio_permission(portfolio): return False return self.request.user.is_org_user(self.request) From 5996c4df123f181ba5d7baa3ff8477956d20506d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 31 Jan 2025 13:07:36 -0500 Subject: [PATCH 33/57] wip --- src/registrar/models/utility/portfolio_helper.py | 15 +++++++++++---- src/registrar/utility/email_invitations.py | 4 ++-- src/registrar/views/portfolios.py | 3 ++- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index b3bb07c3d..d0a369c56 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -1,5 +1,6 @@ from registrar.utility import StrEnum from django.db import models +from django.db.models import Q from django.apps import apps from django.forms import ValidationError from registrar.utility.waffle import flag_is_active_for_user @@ -136,9 +137,12 @@ def validate_user_portfolio_permission(user_portfolio_permission): "Based on current waffle flag settings, users cannot be assigned to multiple portfolios." ) - existing_invitations = PortfolioInvitation.objects.exclude( - portfolio=user_portfolio_permission.portfolio - ).filter(email=user_portfolio_permission.user.email) + existing_invitations = PortfolioInvitation.objects.filter( + email=user_portfolio_permission.user.email + ).exclude( + Q(portfolio=user_portfolio_permission.portfolio) | + Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + ) if existing_invitations.exists(): raise ValidationError( "This user is already assigned to a portfolio invitation. " @@ -195,8 +199,11 @@ def validate_portfolio_invitation(portfolio_invitation): if not flag_is_active_for_user(user, "multiple_portfolios"): existing_permissions = UserPortfolioPermission.objects.filter(user=user) - existing_invitations = PortfolioInvitation.objects.exclude(id=portfolio_invitation.id).filter( + existing_invitations = PortfolioInvitation.objects.filter( email=portfolio_invitation.email + ).exclude( + Q(id=portfolio_invitation.id) | + Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) ) if existing_permissions.exists(): diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 8f6cdd35a..7d3a015c1 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -273,7 +273,7 @@ def _send_portfolio_admin_addition_emails_to_portfolio_admins(email: str, reques f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", exc_info=True, ) - all_emails_sent = False + all_emails_sent = False return all_emails_sent @@ -324,5 +324,5 @@ def _send_portfolio_admin_removal_emails_to_portfolio_admins(email: str, request f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", exc_info=True, ) - all_emails_sent = False + all_emails_sent = False return all_emails_sent diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index c79072537..41f37baee 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -211,6 +211,7 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) user = portfolio_permission.user form = self.form_class(request.POST, instance=portfolio_permission) + removing_admin_role_on_self = False if form.is_valid(): try: if form.is_change_from_member_to_admin(): @@ -477,7 +478,7 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): else: logger.warning("Could not send email notification to existing organization admins.", exc_info=True) messages.warning(self.request, "Could not send email notification to existing organization admins.") - + class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): From 041df217b5a6b5073dfcc27e1795e13a75943bde Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 07:02:22 -0500 Subject: [PATCH 34/57] handle changes to PortfolioInvitation in DJA --- src/registrar/admin.py | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a9944c4c0..1e7d411a6 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -28,7 +28,7 @@ from django.shortcuts import redirect from django_fsm import get_available_FIELD_transitions, FSMField from registrar.models import DomainInformation, Portfolio, UserPortfolioPermission, DomainInvitation from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_invitation_email +from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_admin_addition_emails, send_portfolio_invitation_email from registrar.views.utility.invitation_helper import ( get_org_membership, get_requested_user, @@ -1636,18 +1636,18 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): Emails sent to requested user / email. When exceptions are raised, return without saving model. """ - if not change: # Only send email if this is a new PortfolioInvitation (creation) + try: portfolio = obj.portfolio requested_email = obj.email requestor = request.user is_admin_invitation = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in obj.roles - # Look up a user with that email - requested_user = get_requested_user(requested_email) + if not change: # Only send email if this is a new PortfolioInvitation (creation) + # Look up a user with that email + requested_user = get_requested_user(requested_email) - permission_exists = UserPortfolioPermission.objects.filter( - user__email=requested_email, portfolio=portfolio, user__email__isnull=False - ).exists() - try: + permission_exists = UserPortfolioPermission.objects.filter( + user__email=requested_email, portfolio=portfolio, user__email__isnull=False + ).exists() if not permission_exists: # if permission does not exist for a user with requested_email, send email if not send_portfolio_invitation_email( @@ -1665,10 +1665,28 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): messages.success(request, f"{requested_email} has been invited.") else: messages.warning(request, "User is already a member of this portfolio.") - except Exception as e: - # when exception is raised, handle and do not save the model - handle_invitation_exceptions(request, e, requested_email) - return + else: # Handle the case when updating an existing PortfolioInvitation + # Retrieve the existing object from the database + existing_obj = PortfolioInvitation.objects.get(pk=obj.pk) + + # Check if the previous roles did NOT include ORGANIZATION_ADMIN + # and the new roles DO include ORGANIZATION_ADMIN + was_not_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN not in existing_obj.roles + # Check also if status is INVITED, ignore role changes for other statuses + is_invited = obj.status == PortfolioInvitation.PortfolioInvitationStatus.INVITED + + if was_not_admin and is_admin_invitation and is_invited: + # send email to existing portfolio admins if new admin + if not send_portfolio_admin_addition_emails( + email=requested_email, + requestor=requestor, + portfolio=portfolio, + ): + messages.warning(request, "Could not send email notification to existing organization admins.") + except Exception as e: + # when exception is raised, handle and do not save the model + handle_invitation_exceptions(request, e, requested_email) + return # Call the parent save method to save the object super().save_model(request, obj, form, change) From 6671545c55690df7161d1800b7356553c9fc6e0c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 08:10:10 -0500 Subject: [PATCH 35/57] fixed existing tests and lint errors --- src/registrar/admin.py | 6 +++- .../models/utility/portfolio_helper.py | 15 +++----- src/registrar/tests/test_admin.py | 6 ++++ src/registrar/tests/test_email_invitations.py | 8 ++--- src/registrar/tests/test_views_domain.py | 15 ++++++-- src/registrar/tests/test_views_portfolio.py | 7 ++-- src/registrar/utility/email_invitations.py | 8 +++-- src/registrar/views/portfolios.py | 35 ++++++++----------- 8 files changed, 57 insertions(+), 43 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 1e7d411a6..e5c5cafe2 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -28,7 +28,11 @@ from django.shortcuts import redirect from django_fsm import get_available_FIELD_transitions, FSMField from registrar.models import DomainInformation, Portfolio, UserPortfolioPermission, DomainInvitation from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from registrar.utility.email_invitations import send_domain_invitation_email, send_portfolio_admin_addition_emails, send_portfolio_invitation_email +from registrar.utility.email_invitations import ( + send_domain_invitation_email, + send_portfolio_admin_addition_emails, + send_portfolio_invitation_email, +) from registrar.views.utility.invitation_helper import ( get_org_membership, get_requested_user, diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index d0a369c56..49c2cc1dc 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -137,11 +137,9 @@ def validate_user_portfolio_permission(user_portfolio_permission): "Based on current waffle flag settings, users cannot be assigned to multiple portfolios." ) - existing_invitations = PortfolioInvitation.objects.filter( - email=user_portfolio_permission.user.email - ).exclude( - Q(portfolio=user_portfolio_permission.portfolio) | - Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + existing_invitations = PortfolioInvitation.objects.filter(email=user_portfolio_permission.user.email).exclude( + Q(portfolio=user_portfolio_permission.portfolio) + | Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) ) if existing_invitations.exists(): raise ValidationError( @@ -199,11 +197,8 @@ def validate_portfolio_invitation(portfolio_invitation): if not flag_is_active_for_user(user, "multiple_portfolios"): existing_permissions = UserPortfolioPermission.objects.filter(user=user) - existing_invitations = PortfolioInvitation.objects.filter( - email=portfolio_invitation.email - ).exclude( - Q(id=portfolio_invitation.id) | - Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + existing_invitations = PortfolioInvitation.objects.filter(email=portfolio_invitation.email).exclude( + Q(id=portfolio_invitation.id) | Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) ) if existing_permissions.exists(): diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 387319663..1025cf369 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -254,6 +254,7 @@ class TestDomainInvitationAdmin(TestCase): email="test@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert success message @@ -504,6 +505,7 @@ class TestDomainInvitationAdmin(TestCase): email="test@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert retrieve on domain invite only was called @@ -567,6 +569,7 @@ class TestDomainInvitationAdmin(TestCase): email="test@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert retrieve on domain invite only was called @@ -693,6 +696,7 @@ class TestDomainInvitationAdmin(TestCase): email="nonexistent@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert retrieve was not called @@ -918,6 +922,7 @@ class TestDomainInvitationAdmin(TestCase): email="nonexistent@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert retrieve on domain invite only was called @@ -979,6 +984,7 @@ class TestDomainInvitationAdmin(TestCase): email="nonexistent@example.com", requestor=self.superuser, portfolio=self.portfolio, + is_admin_invitation=False, ) # Assert retrieve on domain invite only was called diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 1914e73bd..f5b12ff22 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -58,7 +58,7 @@ class DomainInvitationEmail(unittest.TestCase): # Assertions mock_normalize_domains.assert_called_once_with(mock_domain) - mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) + mock_get_requestor_email.assert_called_once_with(mock_requestor, domains=[mock_domain]) mock_validate_invitation.assert_called_once_with( email, None, [mock_domain], mock_requestor, is_member_of_different_org ) @@ -137,7 +137,7 @@ class DomainInvitationEmail(unittest.TestCase): # Assertions mock_normalize_domains.assert_called_once_with([mock_domain1, mock_domain2]) - mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain1, mock_domain2]) + mock_get_requestor_email.assert_called_once_with(mock_requestor, domains=[mock_domain1, mock_domain2]) mock_validate_invitation.assert_called_once_with( email, None, [mock_domain1, mock_domain2], mock_requestor, is_member_of_different_org ) @@ -258,7 +258,7 @@ class DomainInvitationEmail(unittest.TestCase): # Assertions mock_normalize_domains.assert_called_once_with(mock_domain) - mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) + mock_get_requestor_email.assert_called_once_with(mock_requestor, domains=[mock_domain]) mock_validate_invitation.assert_called_once_with( email, None, [mock_domain], mock_requestor, is_member_of_different_org ) @@ -306,7 +306,7 @@ class DomainInvitationEmail(unittest.TestCase): # Assertions mock_normalize_domains.assert_called_once_with(mock_domain) - mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) + mock_get_requestor_email.assert_called_once_with(mock_requestor, domains=[mock_domain]) mock_validate_invitation.assert_called_once_with( email, None, [mock_domain], mock_requestor, is_member_of_different_org ) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 7035e0bd0..ba4d4485f 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -810,7 +810,10 @@ class TestDomainManagers(TestDomainOverview): # Verify that the invitation emails were sent mock_send_portfolio_email.assert_called_once_with( - email="mayor@igorville.gov", requestor=self.user, portfolio=self.portfolio + email="mayor@igorville.gov", + requestor=self.user, + portfolio=self.portfolio, + is_admin_invitation=False, ) mock_send_domain_email.assert_called_once() call_args = mock_send_domain_email.call_args.kwargs @@ -864,7 +867,10 @@ class TestDomainManagers(TestDomainOverview): # Verify that the invitation emails were sent mock_send_portfolio_email.assert_called_once_with( - email="notauser@igorville.gov", requestor=self.user, portfolio=self.portfolio + email="notauser@igorville.gov", + requestor=self.user, + portfolio=self.portfolio, + is_admin_invitation=False, ) mock_send_domain_email.assert_called_once() call_args = mock_send_domain_email.call_args.kwargs @@ -999,7 +1005,10 @@ class TestDomainManagers(TestDomainOverview): # Verify that the invitation emails were sent mock_send_portfolio_email.assert_called_once_with( - email="mayor@igorville.gov", requestor=self.user, portfolio=self.portfolio + email="mayor@igorville.gov", + requestor=self.user, + portfolio=self.portfolio, + is_admin_invitation=False, ) mock_send_domain_email.assert_not_called() diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 33f334f7f..876583a39 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3290,7 +3290,7 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): # Assert # assert that the send_portfolio_invitation_email called mock_send_email.assert_called_once_with( - email=self.new_member_email, requestor=self.user, portfolio=self.portfolio + email=self.new_member_email, requestor=self.user, portfolio=self.portfolio, is_admin_invitation=False ) # assert that response is a redirect to reverse("members") self.assertRedirects(response, reverse("members")) @@ -3334,7 +3334,10 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): # Assert # assert that the send_portfolio_invitation_email called mock_send_email.assert_called_once_with( - email=self.new_member_email, requestor=self.user, portfolio=self.portfolio + email=self.new_member_email, + requestor=self.user, + portfolio=self.portfolio, + is_admin_invitation=False, ) # assert that response is a redirect to reverse("members") self.assertRedirects(response, reverse("members")) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 7d3a015c1..f1c366502 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -270,7 +270,9 @@ def _send_portfolio_admin_addition_emails_to_portfolio_admins(email: str, reques ) except EmailSendingError: logger.warning( - f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", + "Could not send email organization admin notification to %s " "for portfolio: %s", + user.email, + portfolio.organization_name, exc_info=True, ) all_emails_sent = False @@ -321,7 +323,9 @@ def _send_portfolio_admin_removal_emails_to_portfolio_admins(email: str, request ) except EmailSendingError: logger.warning( - f"Could not send email organization admin notification to {user.email} for portfolio: {portfolio.name}", + "Could not send email organization admin notification to %s " "for portfolio: %s", + user.email, + portfolio.organization_name, exc_info=True, ) all_emails_sent = False diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 41f37baee..c5cc72c59 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -155,11 +155,9 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): if not send_portfolio_admin_removal_emails( email=portfolio_member_permission.user.email, requestor=request.user, - portfolio=portfolio_member_permission.portfolio + portfolio=portfolio_member_permission.portfolio, ): - messages.warning( - self.request, "Could not send email notification to existing organization admins." - ) + messages.warning(self.request, "Could not send email notification to existing organization admins.") except Exception as e: self._handle_exceptions(e) @@ -179,7 +177,7 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): if isinstance(exception, MissingEmailError): messages.warning(self.request, "Could not send email notification to existing organization admins.") logger.warning( - f"Could not send email notification to existing organization admins.", + "Could not send email notification to existing organization admins.", exc_info=True, ) else: @@ -218,7 +216,7 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): if not send_portfolio_admin_addition_emails( email=portfolio_permission.user.email, requestor=request.user, - portfolio=portfolio_permission.portfolio + portfolio=portfolio_permission.portfolio, ): messages.warning( self.request, "Could not send email notification to existing organization admins." @@ -227,15 +225,15 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): if not send_portfolio_admin_removal_emails( email=portfolio_permission.user.email, requestor=request.user, - portfolio=portfolio_permission.portfolio + portfolio=portfolio_permission.portfolio, ): messages.warning( self.request, "Could not send email notification to existing organization admins." ) # Check if user is removing their own admin or edit role - removing_admin_role_on_self = (request.user == user) + removing_admin_role_on_self = request.user == user except Exception as e: - self._handle_exceptions(e) + self._handle_exceptions(e) form.save() messages.success(self.request, "The member access and permission changes have been saved.") return redirect("member", pk=pk) if not removing_admin_role_on_self else redirect("home") @@ -248,13 +246,13 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): "member": user, # Pass the user object again to the template }, ) - + def _handle_exceptions(self, exception): """Handle exceptions raised during the process.""" if isinstance(exception, MissingEmailError): messages.warning(self.request, "Could not send email notification to existing organization admins.") logger.warning( - f"Could not send email notification to existing organization admins.", + "Could not send email notification to existing organization admins.", exc_info=True, ) else: @@ -262,7 +260,6 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): messages.warning(self.request, "Could not send email notification to existing organization admins.") - class PortfolioMemberDomainsView(PortfolioMemberDomainsPermissionView, View): template_name = "portfolio_member_domains.html" @@ -447,13 +444,9 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): try: # attempt to send notification emails of the removal to portfolio admins if not send_portfolio_admin_removal_emails( - email=portfolio_invitation.email, - requestor=request.user, - portfolio=portfolio_invitation.portfolio + email=portfolio_invitation.email, requestor=request.user, portfolio=portfolio_invitation.portfolio ): - messages.warning( - self.request, "Could not send email notification to existing organization admins." - ) + messages.warning(self.request, "Could not send email notification to existing organization admins.") except Exception as e: self._handle_exceptions(e) @@ -472,7 +465,7 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): if isinstance(exception, MissingEmailError): messages.warning(self.request, "Could not send email notification to existing organization admins.") logger.warning( - f"Could not send email notification to existing organization admins.", + "Could not send email notification to existing organization admins.", exc_info=True, ) else: @@ -516,7 +509,7 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): if not send_portfolio_admin_removal_emails( email=portfolio_invitation.email, requestor=request.user, - portfolio=portfolio_invitation.portfolio + portfolio=portfolio_invitation.portfolio, ): messages.warning( self.request, "Could not send email notification to existing organization admins." @@ -541,7 +534,7 @@ class PortfolioInvitedMemberEditView(PortfolioMemberEditPermissionView, View): if isinstance(exception, MissingEmailError): messages.warning(self.request, "Could not send email notification to existing organization admins.") logger.warning( - f"Could not send email notification to existing organization admins.", + "Could not send email notification to existing organization admins.", exc_info=True, ) else: From 1e280ed86ea2c12883ec47a4f046391d0812e803 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 08:35:40 -0500 Subject: [PATCH 36/57] fix remaining broken test --- src/registrar/tests/test_views_portfolio.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 876583a39..679de98ed 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3243,18 +3243,21 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): } # Act - with patch("django.contrib.messages.warning") as mock_warning: + with patch("django.contrib.messages.error") as mock_error: response = self.client.post(reverse("new-member"), data=form_data) # Assert # assert that the send_portfolio_invitation_email called mock_send_email.assert_called_once_with( - email=self.new_member_email, requestor=self.user, portfolio=self.portfolio + email=self.new_member_email, + requestor=self.user, + portfolio=self.portfolio, + is_admin_invitation=False, ) # assert that response is a redirect to reverse("members") self.assertRedirects(response, reverse("members")) # assert that messages contains message, "Could not send email invitation" - mock_warning.assert_called_once_with(response.wsgi_request, "Could not send portfolio email invitation.") + mock_error.assert_called_once_with(response.wsgi_request, "Could not send organization invitation email.") # assert that portfolio invitation is not created self.assertFalse( PortfolioInvitation.objects.filter(email=self.new_member_email, portfolio=self.portfolio).exists(), From be0c55db2757356d1dd83af34469ad0669c61b67 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 11:17:21 -0500 Subject: [PATCH 37/57] added tests for test_email_invitations --- src/registrar/tests/test_email_invitations.py | 429 +++++++++++++++++- src/registrar/utility/email_invitations.py | 4 +- 2 files changed, 426 insertions(+), 7 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index f5b12ff22..5c7f1a0c1 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -2,12 +2,24 @@ import unittest from unittest.mock import patch, MagicMock from datetime import date from registrar.models.domain import Domain +from registrar.models.portfolio import Portfolio from registrar.models.user import User from registrar.models.user_domain_role import UserDomainRole +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError -from registrar.utility.email_invitations import send_domain_invitation_email, send_emails_to_domain_managers +from registrar.utility.email_invitations import ( + _send_portfolio_admin_addition_emails_to_portfolio_admins, + _send_portfolio_admin_removal_emails_to_portfolio_admins, + send_domain_invitation_email, + send_emails_to_domain_managers, + send_portfolio_admin_addition_emails, + send_portfolio_admin_removal_emails, + send_portfolio_invitation_email, +) from api.tests.common import less_console_noise_decorator +from registrar.utility.errors import MissingEmailError class DomainInvitationEmail(unittest.TestCase): @@ -18,7 +30,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations._validate_invitation") @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") - @patch("registrar.utility.email_invitations.normalize_domains") + @patch("registrar.utility.email_invitations._normalize_domains") def test_send_domain_invitation_email( self, mock_normalize_domains, @@ -83,7 +95,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations._validate_invitation") @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") - @patch("registrar.utility.email_invitations.normalize_domains") + @patch("registrar.utility.email_invitations._normalize_domains") def test_send_domain_invitation_email_multiple_domains( self, mock_normalize_domains, @@ -219,7 +231,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations._validate_invitation") @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") - @patch("registrar.utility.email_invitations.normalize_domains") + @patch("registrar.utility.email_invitations._normalize_domains") def test_send_domain_invitation_email_raises_sending_email_exception( self, mock_normalize_domains, @@ -269,7 +281,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.utility.email_invitations._validate_invitation") @patch("registrar.utility.email_invitations._get_requestor_email") @patch("registrar.utility.email_invitations.send_invitation_email") - @patch("registrar.utility.email_invitations.normalize_domains") + @patch("registrar.utility.email_invitations._normalize_domains") def test_send_domain_invitation_email_manager_emails_send_mail_exception( self, mock_normalize_domains, @@ -469,3 +481,410 @@ class DomainInvitationEmail(unittest.TestCase): "date": date.today(), }, ) + + +class PortfolioInvitationEmailTests(unittest.TestCase): + + def setUp(self): + """Setup common test data for all test cases""" + self.email = "invitee@example.com" + self.requestor = MagicMock(name="User") + self.requestor.email = "requestor@example.com" + self.portfolio = MagicMock(name="Portfolio") + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + def test_send_portfolio_invitation_email_success(self, mock_send_templated_email): + """Test successful email sending""" + is_admin_invitation = False + + result = send_portfolio_invitation_email(self.email, self.requestor, self.portfolio, is_admin_invitation) + + self.assertTrue(result) + mock_send_templated_email.assert_called_once() + + @less_console_noise_decorator + @patch( + "registrar.utility.email_invitations.send_templated_email", + side_effect=EmailSendingError("Failed to send email"), + ) + def test_send_portfolio_invitation_email_failure(self, mock_send_templated_email): + """Test failure when sending email""" + is_admin_invitation = False + + with self.assertRaises(EmailSendingError) as context: + send_portfolio_invitation_email(self.email, self.requestor, self.portfolio, is_admin_invitation) + + self.assertIn("Could not sent email invitation to", str(context.exception)) + + @less_console_noise_decorator + @patch( + "registrar.utility.email_invitations._get_requestor_email", + side_effect=MissingEmailError("Requestor has no email"), + ) + @less_console_noise_decorator + def test_send_portfolio_invitation_email_missing_requestor_email(self, mock_get_email): + """Test when requestor has no email""" + is_admin_invitation = False + + with self.assertRaises(MissingEmailError) as context: + send_portfolio_invitation_email(self.email, self.requestor, self.portfolio, is_admin_invitation) + + self.assertIn( + "Can't send invitation email. No email is associated with your user account.", str(context.exception) + ) + + @less_console_noise_decorator + @patch( + "registrar.utility.email_invitations._send_portfolio_admin_addition_emails_to_portfolio_admins", + return_value=False, + ) + @patch("registrar.utility.email_invitations.send_templated_email") + def test_send_portfolio_invitation_email_admin_invitation(self, mock_send_templated_email, mock_admin_email): + """Test admin invitation email logic""" + is_admin_invitation = True + + result = send_portfolio_invitation_email(self.email, self.requestor, self.portfolio, is_admin_invitation) + + self.assertFalse(result) # Admin email sending failed + mock_send_templated_email.assert_called_once() + mock_admin_email.assert_called_once() + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations._send_portfolio_admin_addition_emails_to_portfolio_admins") + def test_send_email_success(self, mock_send_admin_emails, mock_get_requestor_email): + """Test successful sending of admin addition emails.""" + mock_get_requestor_email.return_value = "requestor@example.com" + mock_send_admin_emails.return_value = True + + result = send_portfolio_admin_addition_emails(self.email, self.requestor, self.portfolio) + + mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) + mock_send_admin_emails.assert_called_once_with(self.email, "requestor@example.com", self.portfolio) + self.assertTrue(result) + + @less_console_noise_decorator + @patch( + "registrar.utility.email_invitations._get_requestor_email", + side_effect=MissingEmailError("Requestor email missing"), + ) + def test_missing_requestor_email_raises_exception(self, mock_get_requestor_email): + """Test exception raised if requestor email is missing.""" + with self.assertRaises(MissingEmailError): + send_portfolio_admin_addition_emails(self.email, self.requestor, self.portfolio) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations._send_portfolio_admin_addition_emails_to_portfolio_admins") + def test_send_email_failure(self, mock_send_admin_emails, mock_get_requestor_email): + """Test handling of failure in sending admin addition emails.""" + mock_get_requestor_email.return_value = "requestor@example.com" + mock_send_admin_emails.return_value = False # Simulate failure + + result = send_portfolio_admin_addition_emails(self.email, self.requestor, self.portfolio) + + self.assertFalse(result) + mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) + mock_send_admin_emails.assert_called_once_with(self.email, "requestor@example.com", self.portfolio) + + +class SendPortfolioAdminAdditionEmailsTests(unittest.TestCase): + """Unit tests for _send_portfolio_admin_addition_emails_to_portfolio_admins function.""" + + def setUp(self): + """Set up test data.""" + self.email = "new.admin@example.com" + self.requestor_email = "requestor@example.com" + self.portfolio = MagicMock(spec=Portfolio) + self.portfolio.organization_name = "Test Organization" + + # Mock portfolio admin users + self.admin_user1 = MagicMock(spec=User) + self.admin_user1.email = "admin1@example.com" + + self.admin_user2 = MagicMock(spec=User) + self.admin_user2.email = "admin2@example.com" + + self.portfolio_admin1 = MagicMock(spec=UserPortfolioPermission) + self.portfolio_admin1.user = self.admin_user1 + self.portfolio_admin1.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + self.portfolio_admin2 = MagicMock(spec=UserPortfolioPermission) + self.portfolio_admin2.user = self.admin_user2 + self.portfolio_admin2.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_send_email_success(self, mock_filter, mock_send_templated_email): + """Test successful sending of admin addition emails.""" + mock_filter.return_value.exclude.return_value = [self.portfolio_admin1, self.portfolio_admin2] + mock_send_templated_email.return_value = None # No exception means success + + result = _send_portfolio_admin_addition_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_addition_notification.txt", + "emails/portfolio_admin_addition_notification_subject.txt", + to_address=self.admin_user1.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "invited_email_address": self.email, + "portfolio_admin": self.admin_user1, + "date": date.today(), + }, + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_addition_notification.txt", + "emails/portfolio_admin_addition_notification_subject.txt", + to_address=self.admin_user2.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "invited_email_address": self.email, + "portfolio_admin": self.admin_user2, + "date": date.today(), + }, + ) + self.assertTrue(result) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError) + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_send_email_failure(self, mock_filter, mock_send_templated_email): + """Test handling of failure in sending admin addition emails.""" + mock_filter.return_value.exclude.return_value = [self.portfolio_admin1, self.portfolio_admin2] + + result = _send_portfolio_admin_addition_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + self.assertFalse(result) + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_addition_notification.txt", + "emails/portfolio_admin_addition_notification_subject.txt", + to_address=self.admin_user1.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "invited_email_address": self.email, + "portfolio_admin": self.admin_user1, + "date": date.today(), + }, + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_addition_notification.txt", + "emails/portfolio_admin_addition_notification_subject.txt", + to_address=self.admin_user2.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "invited_email_address": self.email, + "portfolio_admin": self.admin_user2, + "date": date.today(), + }, + ) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_no_admins_to_notify(self, mock_filter): + """Test case where there are no portfolio admins to notify.""" + mock_filter.return_value.exclude.return_value = [] # No admins + + result = _send_portfolio_admin_addition_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + self.assertTrue(result) # No emails sent, but also no failures + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + +class SendPortfolioAdminRemovalEmailsTests(unittest.TestCase): + """Unit tests for _send_portfolio_admin_removal_emails_to_portfolio_admins function.""" + + def setUp(self): + """Set up test data.""" + self.email = "removed.admin@example.com" + self.requestor_email = "requestor@example.com" + self.portfolio = MagicMock(spec=Portfolio) + self.portfolio.organization_name = "Test Organization" + + # Mock portfolio admin users + self.admin_user1 = MagicMock(spec=User) + self.admin_user1.email = "admin1@example.com" + + self.admin_user2 = MagicMock(spec=User) + self.admin_user2.email = "admin2@example.com" + + self.portfolio_admin1 = MagicMock(spec=UserPortfolioPermission) + self.portfolio_admin1.user = self.admin_user1 + self.portfolio_admin1.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + self.portfolio_admin2 = MagicMock(spec=UserPortfolioPermission) + self.portfolio_admin2.user = self.admin_user2 + self.portfolio_admin2.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_send_email_success(self, mock_filter, mock_send_templated_email): + """Test successful sending of admin removal emails.""" + mock_filter.return_value.exclude.return_value = [self.portfolio_admin1, self.portfolio_admin2] + mock_send_templated_email.return_value = None # No exception means success + + result = _send_portfolio_admin_removal_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_removal_notification.txt", + "emails/portfolio_admin_removal_notification_subject.txt", + to_address=self.admin_user1.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "removed_email_address": self.email, + "portfolio_admin": self.admin_user1, + "date": date.today(), + }, + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_removal_notification.txt", + "emails/portfolio_admin_removal_notification_subject.txt", + to_address=self.admin_user2.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "removed_email_address": self.email, + "portfolio_admin": self.admin_user2, + "date": date.today(), + }, + ) + self.assertTrue(result) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError) + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_send_email_failure(self, mock_filter, mock_send_templated_email): + """Test handling of failure in sending admin removal emails.""" + mock_filter.return_value.exclude.return_value = [self.portfolio_admin1, self.portfolio_admin2] + + result = _send_portfolio_admin_removal_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + self.assertFalse(result) + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_removal_notification.txt", + "emails/portfolio_admin_removal_notification_subject.txt", + to_address=self.admin_user1.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "removed_email_address": self.email, + "portfolio_admin": self.admin_user1, + "date": date.today(), + }, + ) + mock_send_templated_email.assert_any_call( + "emails/portfolio_admin_removal_notification.txt", + "emails/portfolio_admin_removal_notification_subject.txt", + to_address=self.admin_user2.email, + context={ + "portfolio": self.portfolio, + "requestor_email": self.requestor_email, + "removed_email_address": self.email, + "portfolio_admin": self.admin_user2, + "date": date.today(), + }, + ) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.UserPortfolioPermission.objects.filter") + def test_no_admins_to_notify(self, mock_filter): + """Test case where there are no portfolio admins to notify.""" + mock_filter.return_value.exclude.return_value = [] # No admins + + result = _send_portfolio_admin_removal_emails_to_portfolio_admins( + self.email, self.requestor_email, self.portfolio + ) + + self.assertTrue(result) # No emails sent, but also no failures + mock_filter.assert_called_once_with( + portfolio=self.portfolio, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + +class SendPortfolioAdminRemovalEmailsTests(unittest.TestCase): + """Unit tests for send_portfolio_admin_removal_emails function.""" + + def setUp(self): + """Set up test data.""" + self.email = "removed.admin@example.com" + self.requestor = MagicMock(spec=User) + self.requestor.email = "requestor@example.com" + self.portfolio = MagicMock(spec=Portfolio) + self.portfolio.organization_name = "Test Organization" + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations._send_portfolio_admin_removal_emails_to_portfolio_admins") + def test_send_email_success(self, mock_send_removal_emails, mock_get_requestor_email): + """Test successful execution of send_portfolio_admin_removal_emails.""" + mock_get_requestor_email.return_value = self.requestor.email + mock_send_removal_emails.return_value = True # Simulating success + + result = send_portfolio_admin_removal_emails(self.email, self.requestor, self.portfolio) + + mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) + mock_send_removal_emails.assert_called_once_with(self.email, self.requestor.email, self.portfolio) + self.assertTrue(result) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations._get_requestor_email", side_effect=MissingEmailError("No email found")) + @patch("registrar.utility.email_invitations._send_portfolio_admin_removal_emails_to_portfolio_admins") + def test_missing_email_error(self, mock_send_removal_emails, mock_get_requestor_email): + """Test handling of MissingEmailError when requestor has no email.""" + with self.assertRaises(MissingEmailError) as context: + send_portfolio_admin_removal_emails(self.email, self.requestor, self.portfolio) + + mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) + mock_send_removal_emails.assert_not_called() # Should not proceed if email retrieval fails + self.assertEqual( + str(context.exception), "Can't send invitation email. No email is associated with your user account." + ) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch( + "registrar.utility.email_invitations._send_portfolio_admin_removal_emails_to_portfolio_admins", + return_value=False, + ) + def test_send_email_failure(self, mock_send_removal_emails, mock_get_requestor_email): + """Test handling of failure when admin removal emails fail to send.""" + mock_get_requestor_email.return_value = self.requestor.email + mock_send_removal_emails.return_value = False # Simulating failure + + result = send_portfolio_admin_removal_emails(self.email, self.requestor, self.portfolio) + + mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) + mock_send_removal_emails.assert_called_once_with(self.email, self.requestor.email, self.portfolio) + self.assertFalse(result) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index f1c366502..de21b2a61 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -40,7 +40,7 @@ def send_domain_invitation_email( OutsideOrgMemberError: If the requested_user is part of a different organization. EmailSendingError: If there is an error while sending the email. """ - domains = normalize_domains(domains) + domains = _normalize_domains(domains) requestor_email = _get_requestor_email(requestor, domains=domains) _validate_invitation(email, requested_user, domains, requestor, is_member_of_different_org) @@ -95,7 +95,7 @@ def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, return all_emails_sent -def normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: +def _normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: """Ensures domains is always a list.""" return [domains] if isinstance(domains, Domain) else domains From c47122ac190e1e0a7e71301e38f6773e12026f74 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 12:38:05 -0500 Subject: [PATCH 38/57] testing PortfolioInvitationAdmin --- src/registrar/tests/test_admin.py | 90 ++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 1025cf369..28a407036 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1210,7 +1210,7 @@ class TestPortfolioInvitationAdmin(TestCase): @less_console_noise_decorator @patch("registrar.admin.send_portfolio_invitation_email") - @patch("django.contrib.messages.success") # Mock the `messages.warning` call + @patch("django.contrib.messages.success") # Mock the `messages.success` call def test_save_sends_email(self, mock_messages_success, mock_send_email): """On save_model, an email is sent if an invitation already exists.""" @@ -1461,6 +1461,94 @@ class TestPortfolioInvitationAdmin(TestCase): # Assert that messages.error was called with the correct message mock_messages_error.assert_called_once_with(request, "Could not send email invitation.") + @less_console_noise_decorator + @patch("registrar.admin.send_portfolio_admin_addition_emails") + def test_save_existing_sends_email_notification(self, mock_send_email): + """On save_model to an existing invitation, an email is set to notify existing + admins, if the invitation changes from member to admin.""" + + # Create an instance of the admin class + admin_instance = PortfolioInvitationAdmin(PortfolioInvitation, admin_site=None) + + # Mock the response value of the email send + mock_send_email.return_value = True + + # Create and save a PortfolioInvitation instance + portfolio_invitation = PortfolioInvitation.objects.create( + email="james.gordon@gotham.gov", + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], # Initially NOT an admin + status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, # Must be "INVITED" + ) + + # Create a request object + request = self.factory.post(f"/admin/registrar/PortfolioInvitation/{portfolio_invitation.pk}/change/") + request.user = self.superuser + + # Change roles from MEMBER to ADMIN + portfolio_invitation.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + # Call the save_model method + admin_instance.save_model(request, portfolio_invitation, None, True) + + # Assert that send_portfolio_admin_addition_emails is called + mock_send_email.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_email.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], "james.gordon@gotham.gov") + self.assertEqual(called_kwargs["requestor"], self.superuser) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + @less_console_noise_decorator + @patch("registrar.admin.send_portfolio_admin_addition_emails") + @patch("django.contrib.messages.warning") # Mock the `messages.warning` call + def test_save_existing_email_notification_warning(self, mock_messages_warning, mock_send_email): + """On save_model for an existing invitation, a warning is displayed if method to + send email to notify admins returns False.""" + + # Create an instance of the admin class + admin_instance = PortfolioInvitationAdmin(PortfolioInvitation, admin_site=None) + + # Mock the response value of the email send + mock_send_email.return_value = False + + # Create and save a PortfolioInvitation instance + portfolio_invitation = PortfolioInvitation.objects.create( + email="james.gordon@gotham.gov", + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], # Initially NOT an admin + status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, # Must be "INVITED" + ) + + # Create a request object + request = self.factory.post(f"/admin/registrar/PortfolioInvitation/{portfolio_invitation.pk}/change/") + request.user = self.superuser + + # Change roles from MEMBER to ADMIN + portfolio_invitation.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + + # Call the save_model method + admin_instance.save_model(request, portfolio_invitation, None, True) + + # Assert that send_portfolio_admin_addition_emails is called + mock_send_email.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_email.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], "james.gordon@gotham.gov") + self.assertEqual(called_kwargs["requestor"], self.superuser) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Assert that messages.error was called with the correct message + mock_messages_warning.assert_called_once_with( + request, "Could not send email notification to existing organization admins." + ) + class TestHostAdmin(TestCase): """Tests for the HostAdmin class as super user From ff9c402c408292b59daea0c0ea31ca8fd93d2d7c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 13:28:17 -0500 Subject: [PATCH 39/57] additional portfolio member tests --- src/registrar/tests/test_views_portfolio.py | 49 +++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 679de98ed..c0f81363c 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3484,6 +3484,55 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): self.assertEqual(call_args["requestor"], self.user) self.assertIsNone(call_args.get("is_member_of_different_org")) + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_invitation_email") + def test_admin_invite_for_new_users(self, mock_send_email): + """Tests the member invitation flow for new admin.""" + self.client.force_login(self.user) + + # Simulate a session to ensure continuity + session_id = self.client.session.session_key + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + + mock_send_email.return_value = True + + # Simulate submission of member invite for new admin + final_response = self.client.post( + reverse("new-member"), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, + "domain_request_permissions": UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, + "member_permissions": "no_access", + "email": self.new_member_email, + }, + ) + + # Ensure the final submission is successful + self.assertEqual(final_response.status_code, 302) # Redirects + + # Validate Database Changes + # Validate that portfolio invitation was created but not retrieved + portfolio_invite = PortfolioInvitation.objects.filter( + email=self.new_member_email, portfolio=self.portfolio + ).first() + self.assertIsNotNone(portfolio_invite) + self.assertEqual(portfolio_invite.email, self.new_member_email) + self.assertEqual(portfolio_invite.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + + # Check that an email was sent + mock_send_email.assert_called() + + # Get the arguments passed to send_portfolio_invitation_email + _, called_kwargs = mock_send_email.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], self.new_member_email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + class TestEditPortfolioMemberView(WebTest): """Tests for the edit member page on portfolios""" From d29205f75e15289ddb2cd3c59b4acd86a2782532 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 16:02:49 -0500 Subject: [PATCH 40/57] tests for PortfolioMemberDeleteView PortfolioInvitedMemberDeleteView --- src/registrar/tests/test_email_invitations.py | 2 +- src/registrar/tests/test_views_portfolio.py | 312 +++++++++++++++++- 2 files changed, 309 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 5c7f1a0c1..77a8c402f 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -711,7 +711,7 @@ class SendPortfolioAdminAdditionEmailsTests(unittest.TestCase): ) -class SendPortfolioAdminRemovalEmailsTests(unittest.TestCase): +class SendPortfolioAdminRemovalEmailsToAdminsTests(unittest.TestCase): """Unit tests for _send_portfolio_admin_removal_emails_to_portfolio_admins function.""" def setUp(self): diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c0f81363c..cafcd127b 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1631,10 +1631,33 @@ class TestPortfolio(WebTest): # Assert that the toggleable alert ID exists self.assertContains(response, '
Date: Sat, 1 Feb 2025 16:59:17 -0500 Subject: [PATCH 41/57] updated tests for PortfolioMemberEditView --- src/registrar/tests/test_views_portfolio.py | 243 +++++++++++++++++++- 1 file changed, 241 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index cafcd127b..02d5a828d 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3838,7 +3838,7 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): self.assertEqual(called_kwargs["portfolio"], self.portfolio) -class TestEditPortfolioMemberView(WebTest): +class TestPortfolioMemberEditView(WebTest): """Tests for the edit member page on portfolios""" def setUp(self): @@ -3877,7 +3877,9 @@ class TestEditPortfolioMemberView(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) - def test_edit_member_permissions_basic_to_admin(self): + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_basic_to_admin(self, mock_send_removal_emails, mock_send_addition_emails): """Tests converting a basic member to admin with full permissions.""" self.client.force_login(self.user) @@ -3890,6 +3892,8 @@ class TestEditPortfolioMemberView(WebTest): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], ) + mock_send_addition_emails.return_value = True + response = self.client.post( reverse("member-permissions", kwargs={"pk": basic_permission.id}), { @@ -3904,6 +3908,241 @@ class TestEditPortfolioMemberView(WebTest): basic_permission.refresh_from_db() self.assertEqual(basic_permission.roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + # assert addition emails are sent to portfolio admins + mock_send_addition_emails.assert_called_once() + mock_send_removal_emails.assert_not_called() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_addition_emails.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], basic_member.email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("django.contrib.messages.warning") + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_basic_to_admin_notification_fails( + self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning + ): + """Tests converting a basic member to admin with full permissions.""" + self.client.force_login(self.user) + + # Create a basic member to edit + basic_member = create_test_user() + basic_permission = UserPortfolioPermission.objects.create( + user=basic_member, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], + ) + + mock_send_addition_emails.return_value = False + + response = self.client.post( + reverse("member-permissions", kwargs={"pk": basic_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + # Verify redirect and success message + self.assertEqual(response.status_code, 302) + + # Verify database changes + basic_permission.refresh_from_db() + self.assertEqual(basic_permission.roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + + # assert addition emails are sent to portfolio admins + mock_send_addition_emails.assert_called_once() + mock_send_removal_emails.assert_not_called() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_addition_emails.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], basic_member.email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Assert warning message is called correctly + mock_messages_warning.assert_called_once() + warning_args, _ = mock_messages_warning.call_args + self.assertIsInstance(warning_args[0], WSGIRequest) + self.assertEqual(warning_args[1], "Could not send email notification to existing organization admins.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_admin_to_admin(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests updating an admin without changing permissions.""" + self.client.force_login(self.user) + + # Create an admin member to edit + admin_member = create_test_user() + admin_permission = UserPortfolioPermission.objects.create( + user=admin_member, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + response = self.client.post( + reverse("member-permissions", kwargs={"pk": admin_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + # Verify redirect and success message + self.assertEqual(response.status_code, 302) + + # assert addition emails are not sent to portfolio admins + mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_not_called() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + def test_edit_member_permissions_basic_to_basic(self, mock_send_addition_emails): + """Tests updating an admin without changing permissions.""" + self.client.force_login(self.user) + + # Create an admin member to edit + admin_member = create_test_user() + admin_permission = UserPortfolioPermission.objects.create( + user=admin_member, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + response = self.client.post( + reverse("member-permissions", kwargs={"pk": admin_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + # Verify redirect and success message + self.assertEqual(response.status_code, 302) + + # assert addition emails are not sent to portfolio admins + mock_send_addition_emails.assert_not_called() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_admin_to_basic(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests converting an admin to basic member.""" + self.client.force_login(self.user) + + # Create an admin member to edit + admin_member = create_test_user() + admin_permission = UserPortfolioPermission.objects.create( + user=admin_member, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], + ) + + mock_send_removal_emails.return_value = True + + response = self.client.post( + reverse("member-permissions", kwargs={"pk": admin_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + # Verify redirect and success message + self.assertEqual(response.status_code, 302) + + # Verify database changes + admin_permission.refresh_from_db() + self.assertEqual(admin_permission.roles, [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) + + # assert removal emails are sent to portfolio admins + mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_removal_emails + _, called_kwargs = mock_send_removal_emails.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], admin_member.email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("django.contrib.messages.warning") + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_admin_to_basic_notification_fails( + self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning + ): + """Tests converting an admin to basic member.""" + self.client.force_login(self.user) + + # Create an admin member to edit + admin_member = create_test_user() + admin_permission = UserPortfolioPermission.objects.create( + user=admin_member, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], + ) + + # False return indicates that at least one notification email failed to send + mock_send_removal_emails.return_value = False + + response = self.client.post( + reverse("member-permissions", kwargs={"pk": admin_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + # Verify redirect and success message + self.assertEqual(response.status_code, 302) + + # Verify database changes + admin_permission.refresh_from_db() + self.assertEqual(admin_permission.roles, [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) + + # assert removal emails are sent to portfolio admins + mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_removal_emails + _, called_kwargs = mock_send_removal_emails.call_args + + # Assert the email content + self.assertEqual(called_kwargs["email"], admin_member.email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Assert warning message is called correctly + mock_messages_warning.assert_called_once() + warning_args, _ = mock_messages_warning.call_args + self.assertIsInstance(warning_args[0], WSGIRequest) + self.assertEqual(warning_args[1], "Could not send email notification to existing organization admins.") + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) From f405ae84570f703411690781f58cbae84cd1deb5 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 17:51:40 -0500 Subject: [PATCH 42/57] tests for PortfolioInvitedMemberEditView --- src/registrar/tests/test_views_portfolio.py | 338 ++++++++++++++++++-- 1 file changed, 304 insertions(+), 34 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 02d5a828d..74a2f4399 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3892,6 +3892,7 @@ class TestPortfolioMemberEditView(WebTest): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], ) + # return indicator that notification emails sent successfully mock_send_addition_emails.return_value = True response = self.client.post( @@ -3910,12 +3911,13 @@ class TestPortfolioMemberEditView(WebTest): # assert addition emails are sent to portfolio admins mock_send_addition_emails.assert_called_once() + # assert removal emails are not sent mock_send_removal_emails.assert_not_called() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_addition_emails.call_args - # Assert the email content + # Assert the notification email content self.assertEqual(called_kwargs["email"], basic_member.email) self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) @@ -3929,7 +3931,8 @@ class TestPortfolioMemberEditView(WebTest): def test_edit_member_permissions_basic_to_admin_notification_fails( self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning ): - """Tests converting a basic member to admin with full permissions.""" + """Tests converting a basic member to admin with full permissions. + Handle when notification emails fail to send.""" self.client.force_login(self.user) # Create a basic member to edit @@ -3941,6 +3944,7 @@ class TestPortfolioMemberEditView(WebTest): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], ) + # At least one notification email failed to send mock_send_addition_emails.return_value = False response = self.client.post( @@ -3959,6 +3963,7 @@ class TestPortfolioMemberEditView(WebTest): # assert addition emails are sent to portfolio admins mock_send_addition_emails.assert_called_once() + # assert no removal emails are sent mock_send_removal_emails.assert_not_called() # Get the arguments passed to send_portfolio_admin_addition_emails @@ -4002,7 +4007,7 @@ class TestPortfolioMemberEditView(WebTest): # Verify redirect and success message self.assertEqual(response.status_code, 302) - # assert addition emails are not sent to portfolio admins + # assert addition and removal emails are not sent to portfolio admins mock_send_addition_emails.assert_not_called() mock_send_removal_emails.assert_not_called() @@ -4010,30 +4015,36 @@ class TestPortfolioMemberEditView(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") - def test_edit_member_permissions_basic_to_basic(self, mock_send_addition_emails): + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_member_permissions_basic_to_basic(self, mock_send_removal_emails, mock_send_addition_emails): """Tests updating an admin without changing permissions.""" self.client.force_login(self.user) - # Create an admin member to edit - admin_member = create_test_user() - admin_permission = UserPortfolioPermission.objects.create( - user=admin_member, + # Create a basic member to edit + basic_member = create_test_user() + basic_permission = UserPortfolioPermission.objects.create( + user=basic_member, portfolio=self.portfolio, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], ) response = self.client.post( - reverse("member-permissions", kwargs={"pk": admin_permission.id}), + reverse("member-permissions", kwargs={"pk": basic_permission.id}), { - "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", }, ) # Verify redirect and success message self.assertEqual(response.status_code, 302) - # assert addition emails are not sent to portfolio admins + # assert addition and removal emails are not sent to portfolio admins mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_not_called() @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -4050,7 +4061,6 @@ class TestPortfolioMemberEditView(WebTest): user=admin_member, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS], ) mock_send_removal_emails.return_value = True @@ -4172,27 +4182,6 @@ class TestPortfolioMemberEditView(WebTest): self.assertEqual(response.context["form"].errors["member_permissions"][0], "Member permission is required.") self.assertEqual(response.context["form"].errors["domain_permissions"][0], "Domain permission is required.") - @less_console_noise_decorator - @override_flag("organization_feature", active=True) - @override_flag("organization_members", active=True) - def test_edit_invited_member_permissions(self): - """Tests editing permissions for an invited (but not yet joined) member.""" - self.client.force_login(self.user) - - # Test updating invitation permissions - response = self.client.post( - reverse("invitedmember-permissions", kwargs={"pk": self.invitation.id}), - { - "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - }, - ) - - self.assertEqual(response.status_code, 302) - - # Verify invitation was updated - updated_invitation = PortfolioInvitation.objects.get(pk=self.invitation.id) - self.assertEqual(updated_invitation.roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) - @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @@ -4221,3 +4210,284 @@ class TestPortfolioMemberEditView(WebTest): self.assertEqual(response.status_code, 302) self.assertEqual(response["Location"], reverse("home")) + + +class TestPortfolioInvitedMemberEditView(WebTest): + """Tests for the edit invited member page on portfolios""" + + def setUp(self): + self.user = create_user() + # Create Portfolio + self.portfolio = Portfolio.objects.create(creator=self.user, organization_name="Test Portfolio") + + # Add an invited member who has been invited to manage domains + self.invited_member_email = "invited@example.com" + self.invitation = PortfolioInvitation.objects.create( + email=self.invited_member_email, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + ) + + # Add an invited admin who has been invited to manage domains + self.invited_admin_email = "invitedadmin@example.com" + self.admin_invitation = PortfolioInvitation.objects.create( + email=self.invited_admin_email, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[], + ) + + # Assign permissions to the user making requests + UserPortfolioPermission.objects.create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_MEMBERS, + ], + ) + + def tearDown(self): + PortfolioInvitation.objects.all().delete() + UserPortfolioPermission.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_basic_to_admin(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests editing permissions for an invited (but not yet joined) member. + Update basic member to admin.""" + self.client.force_login(self.user) + + # email notifications send successfully + mock_send_addition_emails.return_value = True + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + self.assertEqual(response.status_code, 302) + + # Verify invitation was updated + updated_invitation = PortfolioInvitation.objects.get(pk=self.invitation.id) + self.assertEqual(updated_invitation.roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + + # Assert that addition emails are sent + mock_send_addition_emails.assert_called_once() + # Assert that removal emails are not sent + mock_send_removal_emails.assert_not_called() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_addition_emails.call_args + + # Assert the notification email content + self.assertEqual(called_kwargs["email"], self.invited_member_email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("django.contrib.messages.warning") + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_basic_to_admin_notification_fails(self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning): + """Tests editing permissions for an invited (but not yet joined) member. + Update basic member to admin.""" + self.client.force_login(self.user) + + # at least one email notification not sent successfully + mock_send_addition_emails.return_value = False + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + self.assertEqual(response.status_code, 302) + + # Verify invitation was updated + updated_invitation = PortfolioInvitation.objects.get(pk=self.invitation.id) + self.assertEqual(updated_invitation.roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + + # Assert that addition emails are sent + mock_send_addition_emails.assert_called_once() + # Assert that removal emails are not sent + mock_send_removal_emails.assert_not_called() + + # Get the arguments passed to send_portfolio_admin_addition_emails + _, called_kwargs = mock_send_addition_emails.call_args + + # Assert the notification email content + self.assertEqual(called_kwargs["email"], self.invited_member_email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Assert warning message is called correctly + mock_messages_warning.assert_called_once() + warning_args, _ = mock_messages_warning.call_args + self.assertIsInstance(warning_args[0], WSGIRequest) + self.assertEqual(warning_args[1], "Could not send email notification to existing organization admins.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_admin_to_basic(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests editing permissions for an invited (but not yet joined) admin. + Update admin to basic member.""" + self.client.force_login(self.user) + + # email notifications send successfully + mock_send_addition_emails.return_value = True + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.admin_invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + self.assertEqual(response.status_code, 302) + + # Verify invitation was updated + updated_invitation = PortfolioInvitation.objects.get(pk=self.admin_invitation.id) + self.assertEqual(updated_invitation.roles, [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) + + # Assert that addition emails are not sent + mock_send_addition_emails.assert_not_called() + # Assert that removal emails are sent + mock_send_removal_emails.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_removal_emails + _, called_kwargs = mock_send_removal_emails.call_args + + # Assert the notification email content + self.assertEqual(called_kwargs["email"], self.invited_admin_email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("django.contrib.messages.warning") + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_admin_to_basic_notification_fails(self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning): + """Tests editing permissions for an invited (but not yet joined) admin. + Update basic member to admin. At least one notification email fails.""" + self.client.force_login(self.user) + + # at least one email notification not sent successfully + mock_send_removal_emails.return_value = False + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.admin_invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + self.assertEqual(response.status_code, 302) + + # Verify invitation was updated + updated_invitation = PortfolioInvitation.objects.get(pk=self.admin_invitation.id) + self.assertEqual(updated_invitation.roles, [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) + + # Assert that addition emails are not sent + mock_send_addition_emails.assert_not_called() + # Assert that removal emails are sent + mock_send_removal_emails.assert_called_once() + + # Get the arguments passed to send_portfolio_admin_removal_emails + _, called_kwargs = mock_send_removal_emails.call_args + + # Assert the notification email content + self.assertEqual(called_kwargs["email"], self.invited_admin_email) + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Assert warning message is called correctly + mock_messages_warning.assert_called_once() + warning_args, _ = mock_messages_warning.call_args + self.assertIsInstance(warning_args[0], WSGIRequest) + self.assertEqual(warning_args[1], "Could not send email notification to existing organization admins.") + + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_basic_to_basic(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests editing permissions for an invited (but not yet joined) member. + Update basic member without changing role.""" + self.client.force_login(self.user) + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + self.assertEqual(response.status_code, 302) + + # Assert that addition and removal emails are not sent + mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_not_called() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") + @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") + def test_edit_invited_member_permissions_admin_to_admin(self, mock_send_removal_emails, mock_send_addition_emails): + """Tests editing permissions for an invited (but not yet joined) admin. + Update admin member without changing role.""" + self.client.force_login(self.user) + + # Test updating invitation permissions + response = self.client.post( + reverse("invitedmember-permissions", kwargs={"pk": self.admin_invitation.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + }, + ) + + self.assertEqual(response.status_code, 302) + + # Assert that addition and removal emails are not sent + mock_send_addition_emails.assert_not_called() + mock_send_removal_emails.assert_not_called() + + + From 2ab3b04d4e4c9de7bcce93f71f5ffba2b0980d0e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 17:58:36 -0500 Subject: [PATCH 43/57] lint --- src/registrar/tests/test_views_portfolio.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 74a2f4399..43ab8f7a2 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -4303,7 +4303,9 @@ class TestPortfolioInvitedMemberEditView(WebTest): @patch("django.contrib.messages.warning") @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_edit_invited_member_permissions_basic_to_admin_notification_fails(self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning): + def test_edit_invited_member_permissions_basic_to_admin_notification_fails( + self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning + ): """Tests editing permissions for an invited (but not yet joined) member. Update basic member to admin.""" self.client.force_login(self.user) @@ -4393,7 +4395,9 @@ class TestPortfolioInvitedMemberEditView(WebTest): @patch("django.contrib.messages.warning") @patch("registrar.views.portfolios.send_portfolio_admin_addition_emails") @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_edit_invited_member_permissions_admin_to_basic_notification_fails(self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning): + def test_edit_invited_member_permissions_admin_to_basic_notification_fails( + self, mock_send_removal_emails, mock_send_addition_emails, mock_messages_warning + ): """Tests editing permissions for an invited (but not yet joined) admin. Update basic member to admin. At least one notification email fails.""" self.client.force_login(self.user) @@ -4437,7 +4441,6 @@ class TestPortfolioInvitedMemberEditView(WebTest): self.assertIsInstance(warning_args[0], WSGIRequest) self.assertEqual(warning_args[1], "Could not send email notification to existing organization admins.") - @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @@ -4488,6 +4491,3 @@ class TestPortfolioInvitedMemberEditView(WebTest): # Assert that addition and removal emails are not sent mock_send_addition_emails.assert_not_called() mock_send_removal_emails.assert_not_called() - - - From a9f208116702fcbd917e85a5b98dad3e1d310c2c Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 3 Feb 2025 14:23:06 -0600 Subject: [PATCH 44/57] linter fixes --- src/registrar/config/settings.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 4a80c528e..78439188e 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -491,9 +491,10 @@ class JsonServerFormatter(ServerFormatter): log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) - + + # If we're running locally we don't want json formatting -if 'localhost' in env_base_url: +if "localhost" in env_base_url: django_handlers = ["console"] elif env_log_format == "json": # in production we need everything to be logged as json so that log levels are parsed correctly @@ -533,7 +534,7 @@ LOGGING = { }, # define where log messages will be sent # each logger can have one or more handlers - "handlers": { + "handlers": { "console": { "level": env_log_level, "class": "logging.StreamHandler", From 095f1abbf8af2cf20d378c14935f3acd60891954 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 3 Feb 2025 15:04:13 -0600 Subject: [PATCH 45/57] add log format to sandbox template --- ops/scripts/manifest-sandbox-template.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ops/scripts/manifest-sandbox-template.yaml b/ops/scripts/manifest-sandbox-template.yaml index f0aee9664..ddd1860e1 100644 --- a/ops/scripts/manifest-sandbox-template.yaml +++ b/ops/scripts/manifest-sandbox-template.yaml @@ -21,6 +21,8 @@ applications: DJANGO_BASE_URL: https://getgov-ENVIRONMENT.app.cloud.gov # Tell Django how much stuff to log DJANGO_LOG_LEVEL: INFO + # tell django what log format to use: console or json. See settings.py for more details. + DJANGO_LOG_FORMAT: console # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments From d14a23621e7f76891bde5bdff5f8246dc342c288 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:36:45 -0700 Subject: [PATCH 46/57] fix bug --- 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 f58c6c6a2..fdeebaad6 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1302,15 +1302,16 @@ class DomainRequest(TimeStampedModel): """Unlocks the organization_contact step.""" if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one - Portfolio = apps.get_model("registrar.Portfolio") - return ( - FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + if self.organization_type == self.OrganizationChoices.FEDERAL and self.federal_agency: + Portfolio = apps.get_model("registrar.Portfolio") + return ( + FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ) + .filter(agency=self.federal_agency) + .exists() ) - .filter(agency=self.federal_agency) - .exists() - ) - return ( + return bool( self.federal_agency is not None or self.organization_name is not None or self.address_line1 is not None From 8a82256206d7259d1a22441a32fa5b49218a7e1d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 05:13:58 -0500 Subject: [PATCH 47/57] update header and test --- .../templates/includes/header_extended.html | 2 ++ src/registrar/tests/test_views_portfolio.py | 22 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 1e40a508d..83b71c3ab 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -92,11 +92,13 @@ {% endif %} {% if has_organization_members_flag %} + {% if has_view_members_portfolio_permission %}
  • Members
    • + {% endif %} {% endif %}
  • diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 33f334f7f..b50c9a36f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1097,8 +1097,10 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_no_permissions(self): - """Test the nav contains a link to the no requests page""" + """Test the nav contains a link to the no requests page + Also test that members link not present""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) @@ -1118,20 +1120,23 @@ class TestPortfolio(WebTest): self.assertNotContains(portfolio_landing_page, "basic-nav-section-two") # link to requests self.assertNotContains(portfolio_landing_page, 'href="/requests/') - # link to create + # link to create request self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertNotContains(portfolio_landing_page, 'href="/members/') @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_all_permissions(self): """Test the nav contains a dropdown with a link to create and another link to view requests - Also test for the existence of the Create a new request btn on the requests page""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of the members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS], ) self.client.force_login(self.user) # create and submit a domain request @@ -1151,6 +1156,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) @@ -1160,15 +1167,18 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_view_but_not_edit_permissions(self): """Test the nav contains a simple link to view requests - Also test for the existence of the Create a new request btn on the requests page""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.VIEW_MEMBERS, ], ) self.client.force_login(self.user) @@ -1189,6 +1199,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) From a8fa08acb2afeb0aa719badcd2163899714f1c60 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 06:14:15 -0500 Subject: [PATCH 48/57] combined suborg and portfolio permissions --- src/registrar/context_processors.py | 9 ++------- src/registrar/models/user.py | 9 +-------- .../models/user_portfolio_permission.py | 4 ---- .../models/utility/portfolio_helper.py | 4 ---- src/registrar/templates/domain_detail.html | 8 ++++---- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/domain_suborganization.html | 2 +- .../templates/includes/domains_table.html | 2 +- src/registrar/tests/test_models.py | 18 ++---------------- src/registrar/tests/test_reports.py | 2 +- src/registrar/tests/test_views_domain.py | 2 +- 11 files changed, 14 insertions(+), 48 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index b3d9c3727..b22729563 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -57,11 +57,10 @@ def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" portfolio_context = { "has_base_portfolio_permission": False, + "has_edit_org_portfolio_permission": False, "has_any_domains_portfolio_permission": False, "has_any_requests_portfolio_permission": False, "has_edit_request_portfolio_permission": False, - "has_view_suborganization_portfolio_permission": False, - "has_edit_suborganization_portfolio_permission": False, "has_view_members_portfolio_permission": False, "has_edit_members_portfolio_permission": False, "portfolio": None, @@ -82,15 +81,11 @@ def portfolio_permissions(request): } ) - # Linting: line too long - view_suborg = request.user.has_view_suborganization_portfolio_permission(portfolio) - edit_suborg = request.user.has_edit_suborganization_portfolio_permission(portfolio) if portfolio: return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), + "has_edit_org_portfolio_permission": request.user.has_edit_org_portfolio_permission(portfolio), "has_edit_request_portfolio_permission": request.user.has_edit_request_portfolio_permission(portfolio), - "has_view_suborganization_portfolio_permission": view_suborg, - "has_edit_suborganization_portfolio_permission": edit_suborg, "has_any_domains_portfolio_permission": request.user.has_any_domains_portfolio_permission(portfolio), "has_any_requests_portfolio_permission": request.user.has_any_requests_portfolio_permission(portfolio), "has_view_members_portfolio_permission": request.user.has_view_members_portfolio_permission(portfolio), diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 1d508f88f..7e0790c5b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -268,13 +268,6 @@ class User(AbstractUser): def has_edit_request_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS) - # Field specific permission checks - def has_view_suborganization_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - - def has_edit_suborganization_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) - def is_portfolio_admin(self, portfolio): return "Admin" in self.portfolio_role_summary(portfolio) @@ -293,7 +286,7 @@ class User(AbstractUser): # Define the conditions and their corresponding roles conditions_roles = [ - (self.has_edit_suborganization_portfolio_permission(portfolio), ["Admin"]), + (self.has_edit_org_portfolio_permission(portfolio), ["Admin"]), ( self.has_view_all_domains_portfolio_permission(portfolio) and self.has_any_requests_portfolio_permission(portfolio) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 11d9c56e3..5378dc185 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -27,13 +27,10 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, - UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], # NOTE: Check FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS before adding roles here. UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, ], } @@ -43,7 +40,6 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.EDIT_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_MEMBERS, - UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], } diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 8c42b80c7..2c7b733d5 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -41,10 +41,6 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_PORTFOLIO = "view_portfolio", "View organization" EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" - # Domain: field specific permissions - VIEW_SUBORGANIZATION = "view_suborganization", "View suborganization" - EDIT_SUBORGANIZATION = "edit_suborganization", "Edit suborganization" - @classmethod def get_user_portfolio_permission_label(cls, user_portfolio_permission): return cls(user_portfolio_permission).label if user_portfolio_permission else None diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 03df2d59c..489d6fdf9 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -103,12 +103,12 @@ {% endif %} {% if portfolio %} - {% if has_any_domains_portfolio_permission and has_edit_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_suborganization_portfolio_permission %} - {% elif has_any_domains_portfolio_permission and has_view_suborganization_portfolio_permission %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_org_portfolio_permission %} + {% elif has_any_domains_portfolio_permission and has_base_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_view_suborganization_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_base_portfolio_permission view_button=True %} {% endif %} {% else %} {% url 'domain-org-name-address' pk=domain.id as url %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index ca3802720..a87a611cd 100644 --- a/src/registrar/templates/domain_sidebar.html +++ b/src/registrar/templates/domain_sidebar.html @@ -61,7 +61,7 @@ {% if portfolio %} {% comment %} Only show this menu option if the user has the perms to do so {% endcomment %} - {% if has_any_domains_portfolio_permission and has_view_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_base_portfolio_permission %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index e050690c8..89ce4e79d 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -39,7 +39,7 @@ please contact help@get.gov.

    - {% if has_any_domains_portfolio_permission and has_edit_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %}
    {% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index de3d15eb0..9a49e46f9 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -208,7 +208,7 @@ Domain name Expires Status - {% if portfolio and has_view_suborganization_portfolio_permission %} + {% if portfolio and has_base_portfolio_permission %} Suborganization {% endif %} Date: Tue, 4 Feb 2025 06:19:19 -0500 Subject: [PATCH 49/57] added migration --- ...itation_additional_permissions_and_more.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py diff --git a/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py b/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py new file mode 100644 index 000000000..8360d7a72 --- /dev/null +++ b/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py @@ -0,0 +1,60 @@ +# Generated by Django 4.2.10 on 2025-02-04 11:18 + +import django.contrib.postgres.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0139_alter_domainrequest_action_needed_reason"), + ] + + operations = [ + migrations.AlterField( + model_name="portfolioinvitation", + name="additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + migrations.AlterField( + model_name="userportfoliopermission", + name="additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + ] From 367b2b804316237b6d1dffa7d8263e92211a0e45 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:58:17 -0700 Subject: [PATCH 50/57] Requested changes --- src/registrar/forms/domain_request_wizard.py | 8 ++++---- src/registrar/models/domain_request.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 0cf82558b..d7a02b124 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -322,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - # We populate this queryset in init. We want to exclude agencies with a portfolio. + # We populate this queryset in init. queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) @@ -369,12 +369,12 @@ class OrganizationContactForm(RegistrarForm): super().__init__(*args, **kwargs) # Set the queryset for federal agency. - # If the organization_requests flag is active, we hide data that exists in portfolios. + # If the organization_requests flag is active, We want to exclude agencies with a portfolio. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): - # Exclude both predefined agencies and those matching portfolio names in one query + # Exclude both predefined agencies and those matching portfolio records in one query federal_agency_queryset = federal_agency_queryset.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True) + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True) ) self.fields["federal_agency"].queryset = federal_agency_queryset diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index fdeebaad6..3071d40d8 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1306,9 +1306,9 @@ class DomainRequest(TimeStampedModel): Portfolio = apps.get_model("registrar.Portfolio") return ( FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True), ) - .filter(agency=self.federal_agency) + .filter(id=self.federal_agency.id) .exists() ) return bool( From 9bf152a8fc9abd5b1c6e07314daaa6bb08a06571 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:18:24 -0700 Subject: [PATCH 51/57] unit test --- src/registrar/tests/test_views_request.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index d33d7e6ad..f6eba2a56 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3227,7 +3227,9 @@ class TestDomainRequestWizard(TestWithUser, WebTest): federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") # Create a portfolio with matching organization name - Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + Portfolio.objects.create( + creator=self.user, organization_name=federal_agency.agency, federal_agency=federal_agency + ) # Create domain request with the portfolio agency domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) From 7c613a01f855fe18f9c3aeb99edf0057b4f002e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:52:40 -0700 Subject: [PATCH 52/57] revert db_helpers change --- src/registrar/utility/db_helpers.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/registrar/utility/db_helpers.py b/src/registrar/utility/db_helpers.py index b6af059c1..5b7e0392c 100644 --- a/src/registrar/utility/db_helpers.py +++ b/src/registrar/utility/db_helpers.py @@ -9,13 +9,12 @@ def ignore_unique_violation(): Execute within an atomic transaction so that if a unique constraint violation occurs, the individual transaction is rolled back without invalidating any larger transaction. """ - try: - # NOTE - is transaction doing anything here?? - with transaction.atomic(): + with transaction.atomic(): + try: yield - except IntegrityError as e: - if e.__cause__.pgcode == errorcodes.UNIQUE_VIOLATION: - # roll back to the savepoint, effectively ignoring this transaction - pass - else: - raise e + except IntegrityError as e: + if e.__cause__.pgcode == errorcodes.UNIQUE_VIOLATION: + # roll back to the savepoint, effectively ignoring this transaction + pass + else: + raise e From c2eacbfcdbe0af38249c7ff257a912adb2a7b3c7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 15:15:02 -0700 Subject: [PATCH 53/57] Remove transaction.atomic() from fixtures --- src/registrar/fixtures/fixtures_domains.py | 9 ++++---- src/registrar/fixtures/fixtures_portfolios.py | 21 +++++++------------ src/registrar/fixtures/fixtures_requests.py | 15 ++++--------- .../fixtures/fixtures_suborganizations.py | 12 +++++------ src/registrar/fixtures/fixtures_users.py | 11 +++++----- 5 files changed, 26 insertions(+), 42 deletions(-) diff --git a/src/registrar/fixtures/fixtures_domains.py b/src/registrar/fixtures/fixtures_domains.py index 366789dfd..6566465c1 100644 --- a/src/registrar/fixtures/fixtures_domains.py +++ b/src/registrar/fixtures/fixtures_domains.py @@ -30,12 +30,11 @@ class DomainFixture(DomainRequestFixture): # Lumped under .atomic to ensure we don't make redundant DB calls. # This bundles them all together, and then saves it in a single call. try: - with transaction.atomic(): - # Get the usernames of users created in the UserFixture - created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] + # Get the usernames of users created in the UserFixture + created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] - # Filter users to only include those created by the fixture - users = list(User.objects.filter(username__in=created_usernames)) + # Filter users to only include those created by the fixture + users = list(User.objects.filter(username__in=created_usernames)) except Exception as e: logger.warning(e) return diff --git a/src/registrar/fixtures/fixtures_portfolios.py b/src/registrar/fixtures/fixtures_portfolios.py index efb90e475..a302de95b 100644 --- a/src/registrar/fixtures/fixtures_portfolios.py +++ b/src/registrar/fixtures/fixtures_portfolios.py @@ -84,12 +84,8 @@ class PortfolioFixture: def load(cls): """Creates portfolios.""" logger.info("Going to load %s portfolios" % len(cls.PORTFOLIOS)) - - # Lumped under .atomic to ensure we don't make redundant DB calls. - # This bundles them all together, and then saves it in a single call. try: - with transaction.atomic(): - user = User.objects.all().last() + user = User.objects.all().last() except Exception as e: logger.warning(e) return @@ -106,14 +102,13 @@ class PortfolioFixture: continue try: - with transaction.atomic(): - portfolio = Portfolio( - creator=user, - organization_name=portfolio_data["organization_name"], - ) - cls._set_non_foreign_key_fields(portfolio, portfolio_data) - cls._set_foreign_key_fields(portfolio, portfolio_data, user) - portfolios_to_create.append(portfolio) + portfolio = Portfolio( + creator=user, + organization_name=portfolio_data["organization_name"], + ) + cls._set_non_foreign_key_fields(portfolio, portfolio_data) + cls._set_foreign_key_fields(portfolio, portfolio_data, user) + portfolios_to_create.append(portfolio) except Exception as e: logger.warning(e) diff --git a/src/registrar/fixtures/fixtures_requests.py b/src/registrar/fixtures/fixtures_requests.py index 142c7f5a9..5c7ab6798 100644 --- a/src/registrar/fixtures/fixtures_requests.py +++ b/src/registrar/fixtures/fixtures_requests.py @@ -303,19 +303,12 @@ class DomainRequestFixture: def load(cls): """Creates domain requests for each user in the database.""" logger.info("Going to load %s domain requests" % len(cls.DOMAINREQUESTS)) - - # Lumped under .atomic to ensure we don't make redundant DB calls. - # This bundles them all together, and then saves it in a single call. - # The atomic block will cause the code to stop executing if one instance in the - # nested iteration fails, which will cause an early exit and make it hard to debug. - # Comment out with transaction.atomic() when debugging. try: - with transaction.atomic(): - # Get the usernames of users created in the UserFixture - created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] + # Get the usernames of users created in the UserFixture + created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF] - # Filter users to only include those created by the fixture - users = list(User.objects.filter(username__in=created_usernames)) + # Filter users to only include those created by the fixture + users = list(User.objects.filter(username__in=created_usernames)) except Exception as e: logger.warning(e) return diff --git a/src/registrar/fixtures/fixtures_suborganizations.py b/src/registrar/fixtures/fixtures_suborganizations.py index af7e02804..a1bf6a043 100644 --- a/src/registrar/fixtures/fixtures_suborganizations.py +++ b/src/registrar/fixtures/fixtures_suborganizations.py @@ -34,14 +34,12 @@ class SuborganizationFixture: def load(cls): """Creates suborganizations.""" logger.info(f"Going to load {len(cls.SUBORGS)} suborgs") + portfolios = cls._get_portfolios() + if not portfolios: + return - with transaction.atomic(): - portfolios = cls._get_portfolios() - if not portfolios: - return - - suborgs_to_create = cls._prepare_suborgs_to_create(portfolios) - cls._bulk_create_suborgs(suborgs_to_create) + suborgs_to_create = cls._prepare_suborgs_to_create(portfolios) + cls._bulk_create_suborgs(suborgs_to_create) @classmethod def _get_portfolios(cls): diff --git a/src/registrar/fixtures/fixtures_users.py b/src/registrar/fixtures/fixtures_users.py index f1623e674..29e67027a 100644 --- a/src/registrar/fixtures/fixtures_users.py +++ b/src/registrar/fixtures/fixtures_users.py @@ -430,10 +430,9 @@ class UserFixture: @classmethod def load(cls): - with transaction.atomic(): - cls.load_users(cls.ADMINS, "full_access_group", are_superusers=True) - cls.load_users(cls.STAFF, "cisa_analysts_group") + cls.load_users(cls.ADMINS, "full_access_group", are_superusers=True) + cls.load_users(cls.STAFF, "cisa_analysts_group") - # Combine ADMINS and STAFF lists - all_users = cls.ADMINS + cls.STAFF - cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS) + # Combine ADMINS and STAFF lists + all_users = cls.ADMINS + cls.STAFF + cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS) From 2bdd1cf71e12758b95e436cecbaeb556ec8997d9 Mon Sep 17 00:00:00 2001 From: Cameron Dixon Date: Wed, 5 Feb 2025 14:34:34 -0500 Subject: [PATCH 54/57] Delete story.yml --- .github/ISSUE_TEMPLATE/story.yml | 61 -------------------------------- 1 file changed, 61 deletions(-) delete mode 100644 .github/ISSUE_TEMPLATE/story.yml diff --git a/.github/ISSUE_TEMPLATE/story.yml b/.github/ISSUE_TEMPLATE/story.yml deleted file mode 100644 index e7d81ad3a..000000000 --- a/.github/ISSUE_TEMPLATE/story.yml +++ /dev/null @@ -1,61 +0,0 @@ -name: Story -description: Capture actionable sprint work -labels: ["story"] - -body: - - type: markdown - id: help - attributes: - value: | - > **Note** - > GitHub Issues use [GitHub Flavored Markdown](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax) for formatting. - - type: textarea - id: story - attributes: - label: Story - description: | - Please add the "as a, I want, so that" details that describe the story. - If more than one "as a, I want, so that" describes the story, add multiple. - - Example: - As an analyst - I want the ability to approve a domain request - so that a request can be fulfilled and a new .gov domain can be provisioned - value: | - As a - I want - so that - validations: - required: true - - type: textarea - id: acceptance-criteria - attributes: - label: Acceptance Criteria - description: | - Please add the acceptance criteria that best describe the desired outcomes when this work is completed - - Example: - - Application sends an email when analysts approve domain requests - - Domain request status is "approved" - - Example ("given, when, then" format): - Given that I am an analyst who has finished reviewing a domain request - When I click to approve a domain request - Then the domain provisioning process should be initiated, and the applicant should receive an email update. - validations: - required: true - - type: textarea - id: additional-context - attributes: - label: Additional Context - description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" - - type: textarea - id: issue-links - attributes: - label: Issue Links - description: | - What other issues does this story relate to and how? - - Example: - - ๐Ÿšง Blocked by: #123 - - ๐Ÿ”„ Relates to: #234 From 120650895a5bdf8b8507f80945e9178602a94657 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 5 Feb 2025 16:42:56 -0500 Subject: [PATCH 55/57] normalized more portfolio permission names --- src/registrar/context_processors.py | 8 ++++---- src/registrar/models/domain_request.py | 2 +- src/registrar/models/user.py | 16 ++++++++-------- src/registrar/templates/domain_detail.html | 8 ++++---- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/domain_suborganization.html | 2 +- .../templates/includes/domains_table.html | 2 +- .../templates/portfolio_organization.html | 2 +- src/registrar/tests/test_models.py | 18 +++++++++--------- src/registrar/views/portfolios.py | 2 +- 10 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index b22729563..a078c81ac 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -56,8 +56,8 @@ def add_path_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" portfolio_context = { - "has_base_portfolio_permission": False, - "has_edit_org_portfolio_permission": False, + "has_view_portfolio_permission": False, + "has_edit_portfolio_permission": False, "has_any_domains_portfolio_permission": False, "has_any_requests_portfolio_permission": False, "has_edit_request_portfolio_permission": False, @@ -83,8 +83,8 @@ def portfolio_permissions(request): if portfolio: return { - "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), - "has_edit_org_portfolio_permission": request.user.has_edit_org_portfolio_permission(portfolio), + "has_view_portfolio_permission": request.user.has_view_portfolio_permission(portfolio), + "has_edit_portfolio_permission": request.user.has_edit_portfolio_permission(portfolio), "has_edit_request_portfolio_permission": request.user.has_edit_request_portfolio_permission(portfolio), "has_any_domains_portfolio_permission": request.user.has_any_domains_portfolio_permission(portfolio), "has_any_requests_portfolio_permission": request.user.has_any_requests_portfolio_permission(portfolio), diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index c5a0926ad..598fe7a3d 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -947,7 +947,7 @@ class DomainRequest(TimeStampedModel): try: if not context: has_organization_feature_flag = flag_is_active_for_user(recipient, "organization_feature") - is_org_user = has_organization_feature_flag and recipient.has_base_portfolio_permission(self.portfolio) + is_org_user = has_organization_feature_flag and recipient.has_view_portfolio_permission(self.portfolio) context = { "domain_request": self, # This is the user that we refer to in the email diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 7e0790c5b..6f8ee499b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -210,10 +210,10 @@ class User(AbstractUser): return portfolio_permission in user_portfolio_perms._get_portfolio_permissions() - def has_base_portfolio_permission(self, portfolio): + def has_view_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - def has_edit_org_portfolio_permission(self, portfolio): + def has_edit_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) def has_any_domains_portfolio_permission(self, portfolio): @@ -286,7 +286,7 @@ class User(AbstractUser): # Define the conditions and their corresponding roles conditions_roles = [ - (self.has_edit_org_portfolio_permission(portfolio), ["Admin"]), + (self.has_edit_portfolio_permission(portfolio), ["Admin"]), ( self.has_view_all_domains_portfolio_permission(portfolio) and self.has_any_requests_portfolio_permission(portfolio) @@ -299,20 +299,20 @@ class User(AbstractUser): ["View-only admin"], ), ( - self.has_base_portfolio_permission(portfolio) + self.has_view_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), ["Domain requestor", "Domain manager"], ), ( - self.has_base_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio), + self.has_view_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio), ["Domain requestor"], ), ( - self.has_base_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), + self.has_view_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), ["Domain manager"], ), - (self.has_base_portfolio_permission(portfolio), ["Member"]), + (self.has_view_portfolio_permission(portfolio), ["Member"]), ] # Evaluate conditions and add roles @@ -470,7 +470,7 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") portfolio = request.session.get("portfolio") - return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) + return has_organization_feature_flag and self.has_view_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 489d6fdf9..758c43366 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -103,12 +103,12 @@ {% endif %} {% if portfolio %} - {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_org_portfolio_permission %} - {% elif has_any_domains_portfolio_permission and has_base_portfolio_permission %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_portfolio_permission %} + {% elif has_any_domains_portfolio_permission and has_view_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_base_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_view_portfolio_permission view_button=True %} {% endif %} {% else %} {% url 'domain-org-name-address' pk=domain.id as url %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index a87a611cd..5946b6859 100644 --- a/src/registrar/templates/domain_sidebar.html +++ b/src/registrar/templates/domain_sidebar.html @@ -61,7 +61,7 @@ {% if portfolio %} {% comment %} Only show this menu option if the user has the perms to do so {% endcomment %} - {% if has_any_domains_portfolio_permission and has_base_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_view_portfolio_permission %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index 89ce4e79d..1c3b8e588 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -39,7 +39,7 @@ please contact help@get.gov.

    - {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_portfolio_permission %} {% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 9a49e46f9..94cb4ea6d 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -208,7 +208,7 @@ Domain name Expires Status - {% if portfolio and has_base_portfolio_permission %} + {% if portfolio and has_view_portfolio_permission %} Suborganization {% endif %} The name of your organization will be publicly listed as the domain registrant.

    - {% if has_edit_org_portfolio_permission %} + {% if has_edit_portfolio_permission %}

    Your organization name canโ€™t be updated here. To suggest an update, email help@get.gov. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 4cd353d36..0d708671e 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1190,7 +1190,7 @@ class TestUser(TestCase): User.objects.all().delete() UserDomainRole.objects.all().delete() - @patch.object(User, "has_edit_org_portfolio_permission", return_value=True) + @patch.object(User, "has_edit_portfolio_permission", return_value=True) def test_portfolio_role_summary_admin(self, mock_edit_org): # Test if the user is recognized as an Admin self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Admin"]) @@ -1216,7 +1216,7 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, has_any_domains_portfolio_permission=lambda self, portfolio: True, ) @@ -1226,7 +1226,7 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_member_domain_requestor(self): @@ -1235,14 +1235,14 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_portfolio_permission=lambda self, portfolio: True, has_any_domains_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_member_domain_manager(self): # Test if the user has 'Member' and 'Domain manager' roles self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain manager"]) - @patch.multiple(User, has_base_portfolio_permission=lambda self, portfolio: True) + @patch.multiple(User, has_view_portfolio_permission=lambda self, portfolio: True) def test_portfolio_role_summary_member(self): # Test if the user is recognized as a Member self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Member"]) @@ -1252,17 +1252,17 @@ class TestUser(TestCase): self.assertEqual(self.user.portfolio_role_summary(self.portfolio), []) @patch("registrar.models.User._has_portfolio_permission") - def test_has_base_portfolio_permission(self, mock_has_permission): + def test_has_view_portfolio_permission(self, mock_has_permission): mock_has_permission.return_value = True - self.assertTrue(self.user.has_base_portfolio_permission(self.portfolio)) + self.assertTrue(self.user.has_view_portfolio_permission(self.portfolio)) mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) @patch("registrar.models.User._has_portfolio_permission") - def test_has_edit_org_portfolio_permission(self, mock_has_permission): + def test_has_edit_portfolio_permission(self, mock_has_permission): mock_has_permission.return_value = True - self.assertTrue(self.user.has_edit_org_portfolio_permission(self.portfolio)) + self.assertTrue(self.user.has_edit_portfolio_permission(self.portfolio)) mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) @patch("registrar.models.User._has_portfolio_permission") diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 212ce089d..beb04d2c7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -641,7 +641,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) portfolio = self.request.session.get("portfolio") - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(portfolio) + context["has_edit_portfolio_permission"] = self.request.user.has_edit_portfolio_permission(portfolio) return context def get_object(self, queryset=None): From c8217d7f53b9f1870466bb72cffa90ee75d8f5f8 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:39:54 -0700 Subject: [PATCH 56/57] remove unused imports --- src/registrar/fixtures/fixtures_domains.py | 1 - src/registrar/fixtures/fixtures_portfolios.py | 1 - src/registrar/fixtures/fixtures_requests.py | 1 - src/registrar/fixtures/fixtures_suborganizations.py | 1 - src/registrar/fixtures/fixtures_users.py | 1 - 5 files changed, 5 deletions(-) diff --git a/src/registrar/fixtures/fixtures_domains.py b/src/registrar/fixtures/fixtures_domains.py index 6566465c1..8855194f8 100644 --- a/src/registrar/fixtures/fixtures_domains.py +++ b/src/registrar/fixtures/fixtures_domains.py @@ -3,7 +3,6 @@ from django.utils import timezone import logging import random from faker import Faker -from django.db import transaction from registrar.fixtures.fixtures_requests import DomainRequestFixture from registrar.fixtures.fixtures_users import UserFixture diff --git a/src/registrar/fixtures/fixtures_portfolios.py b/src/registrar/fixtures/fixtures_portfolios.py index a302de95b..e4262c7c1 100644 --- a/src/registrar/fixtures/fixtures_portfolios.py +++ b/src/registrar/fixtures/fixtures_portfolios.py @@ -1,7 +1,6 @@ import logging import random from faker import Faker -from django.db import transaction from registrar.models import User, DomainRequest, FederalAgency from registrar.models.portfolio import Portfolio diff --git a/src/registrar/fixtures/fixtures_requests.py b/src/registrar/fixtures/fixtures_requests.py index 5c7ab6798..6eee6438f 100644 --- a/src/registrar/fixtures/fixtures_requests.py +++ b/src/registrar/fixtures/fixtures_requests.py @@ -3,7 +3,6 @@ from django.utils import timezone import logging import random from faker import Faker -from django.db import transaction from registrar.fixtures.fixtures_portfolios import PortfolioFixture from registrar.fixtures.fixtures_suborganizations import SuborganizationFixture diff --git a/src/registrar/fixtures/fixtures_suborganizations.py b/src/registrar/fixtures/fixtures_suborganizations.py index a1bf6a043..787ce8f75 100644 --- a/src/registrar/fixtures/fixtures_suborganizations.py +++ b/src/registrar/fixtures/fixtures_suborganizations.py @@ -1,6 +1,5 @@ import logging from faker import Faker -from django.db import transaction from registrar.models.portfolio import Portfolio from registrar.models.suborganization import Suborganization diff --git a/src/registrar/fixtures/fixtures_users.py b/src/registrar/fixtures/fixtures_users.py index 29e67027a..e09d41cce 100644 --- a/src/registrar/fixtures/fixtures_users.py +++ b/src/registrar/fixtures/fixtures_users.py @@ -1,6 +1,5 @@ import logging from faker import Faker -from django.db import transaction from registrar.models import ( User, From c80348b1c891f1b6fa2308778f70bbf7a44922ce Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 6 Feb 2025 11:33:20 -0700 Subject: [PATCH 57/57] Update src/registrar/fixtures/fixtures_portfolios.py --- src/registrar/fixtures/fixtures_portfolios.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/fixtures/fixtures_portfolios.py b/src/registrar/fixtures/fixtures_portfolios.py index e4262c7c1..b93b9efdd 100644 --- a/src/registrar/fixtures/fixtures_portfolios.py +++ b/src/registrar/fixtures/fixtures_portfolios.py @@ -111,7 +111,7 @@ class PortfolioFixture: except Exception as e: logger.warning(e) - # Bulk create domain requests + # Bulk create portfolios if len(portfolios_to_create) > 0: try: Portfolio.objects.bulk_create(portfolios_to_create)