From a554c6a5507f9fec38c689e289a666f9eff8096d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 6 Jan 2025 11:35:44 -0600 Subject: [PATCH 01/44] 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/44] 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/44] 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/44] 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 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 05/44] 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 06/44] 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 37f52aa9d4136cdc15be5602d65109f75a270008 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 07:34:20 -0500 Subject: [PATCH 07/44] 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 08/44] 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 04b375fae1e790e1704606e25d82d89bca9bb488 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 11:01:57 -0500 Subject: [PATCH 09/44] 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 10/44] 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 11/44] 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 12/44] 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 13/44] 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 14/44] 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 15/44] 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 16/44] 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 17/44] 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 18/44] 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 5996c4df123f181ba5d7baa3ff8477956d20506d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 31 Jan 2025 13:07:36 -0500 Subject: [PATCH 19/44] 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 20/44] 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 21/44] 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 22/44] 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 23/44] 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 24/44] 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 25/44] 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 26/44] 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 27/44] 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 28/44] 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 29/44] 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 30/44] 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 31/44] 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 a8fa08acb2afeb0aa719badcd2163899714f1c60 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 06:14:15 -0500 Subject: [PATCH 32/44] 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 33/44] 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 cdbffa395c74325cc9f43a2e568bd37d51d8ca54 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 07:20:33 -0500 Subject: [PATCH 34/44] canceled and retrieved domain invitations not displayed in domain overview --- src/registrar/models/domain.py | 5 +++++ src/registrar/templates/includes/summary_item.html | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 0f0b3f112..4154c5575 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -9,6 +9,7 @@ from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignor from django.db import models, IntegrityError from django.utils import timezone from typing import Any +from registrar.models.domain_invitation import DomainInvitation from registrar.models.host import Host from registrar.models.host_ip import HostIP from registrar.utility.enums import DefaultEmail @@ -1176,6 +1177,10 @@ class Domain(TimeStampedModel, DomainHelper): elif self.state == self.State.UNKNOWN or self.state == self.State.DNS_NEEDED: return "DNS needed" return self.state.capitalize() + + def active_invitations(self): + """Returns only the active invitations (those with status 'invited').""" + return self.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED) def map_epp_contact_to_public_contact(self, contact: eppInfo.InfoContactResultData, contact_id, contact_type): """Maps the Epp contact representation to a PublicContact object. diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index 26e56fea7..d062a7b4e 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -113,10 +113,10 @@ {% endif %} {% endif %} - {% if value.invitations.all %} + {% if value.active_invitations.all %}

Invited domain managers

    - {% for item in value.invitations.all %} + {% for item in value.active_invitations.all %}
  • {{ item.email }}
    • {% endfor %}
From b1f2dddd99a0f2592d4f75536d92888447d44c96 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 12:16:28 -0500 Subject: [PATCH 35/44] fixed portfolio members table, assigned domains --- src/registrar/models/domain.py | 4 ++-- src/registrar/views/portfolio_members_json.py | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4154c5575..649b3f93d 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1177,10 +1177,10 @@ class Domain(TimeStampedModel, DomainHelper): elif self.state == self.State.UNKNOWN or self.state == self.State.DNS_NEEDED: return "DNS needed" return self.state.capitalize() - + def active_invitations(self): """Returns only the active invitations (those with status 'invited').""" - return self.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED) + return self.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED) def map_epp_contact_to_public_contact(self, contact: eppInfo.InfoContactResultData, contact_id, contact_type): """Maps the Epp contact representation to a PublicContact object. diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index a45ad66e9..29dc6a71c 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -123,7 +123,11 @@ class PortfolioMembersJson(PortfolioMembersPermission, View): # Subquery to get concatenated domain information for each email domain_invitations = ( - DomainInvitation.objects.filter(email=OuterRef("email"), domain__domain_info__portfolio=portfolio) + DomainInvitation.objects.filter( + email=OuterRef("email"), + domain__domain_info__portfolio=portfolio, + status=DomainInvitation.DomainInvitationStatus.INVITED, + ) .annotate( concatenated_info=Concat(F("domain__id"), Value(":"), F("domain__name"), output_field=CharField()) ) From 3f7d1b0524824033d6535672f225f3239989f5cb Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 12:24:46 -0500 Subject: [PATCH 36/44] unblock invitation when previously retrieved invitation --- src/registrar/models/utility/portfolio_helper.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 8c42b80c7..7c82413ae 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,10 @@ 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. " 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 37/44] 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 38/44] 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 8723e35262c9bf0868557f997e185fb4bde0b7be Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 18:35:45 -0500 Subject: [PATCH 39/44] unit test added --- .../models/utility/portfolio_helper.py | 4 +- .../tests/test_views_members_json.py | 16 +++ src/registrar/tests/test_views_portfolio.py | 119 +++++++++++++++--- 3 files changed, 120 insertions(+), 19 deletions(-) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 7c82413ae..0864bded0 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -197,8 +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.exclude(id=portfolio_invitation.id).filter( - email=portfolio_invitation.email + 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_views_members_json.py b/src/registrar/tests/test_views_members_json.py index ceae1e35f..c505421ec 100644 --- a/src/registrar/tests/test_views_members_json.py +++ b/src/registrar/tests/test_views_members_json.py @@ -372,6 +372,21 @@ class GetPortfolioMembersJsonTest(MockEppLib, WebTest): domain=domain3, ) + # create another domain in the portfolio + # but make sure the domain invitation is canceled + domain4 = Domain.objects.create( + name="somedomain4.com", + ) + DomainInformation.objects.create( + creator=self.user, + domain=domain4, + ) + DomainInvitation.objects.create( + email=self.email6, + domain=domain4, + status=DomainInvitation.DomainInvitationStatus.CANCELED, + ) + response = self.app.get(reverse("get_portfolio_members_json"), params={"portfolio": self.portfolio.id}) self.assertEqual(response.status_code, 200) data = response.json @@ -381,6 +396,7 @@ class GetPortfolioMembersJsonTest(MockEppLib, WebTest): self.assertIn("somedomain1.com", domain_names) self.assertIn("thissecondinvitetestsasubqueryinjson@lets.notbreak", domain_names) self.assertNotIn("somedomain3.com", domain_names) + self.assertNotIn("somedomain4.com", domain_names) @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 33f334f7f..a324fc822 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -22,7 +22,7 @@ from registrar.models.utility.portfolio_helper import UserPortfolioPermissionCho from registrar.tests.test_views import TestWithUser from registrar.utility.email import EmailSendingError from registrar.utility.errors import MissingEmailError -from .common import MockSESClient, completed_domain_request, create_test_user, create_user +from .common import MockEppLib, MockSESClient, completed_domain_request, create_test_user, create_user from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware import boto3_mocking # type: ignore @@ -3049,34 +3049,35 @@ class TestRequestingEntity(WebTest): self.assertContains(response, "kepler, AL") -class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): +class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): - @classmethod - def setUpClass(cls): - super().setUpClass() + def setUp(self): + super().setUp() + + self.user = create_test_user() # Create Portfolio - cls.portfolio = Portfolio.objects.create(creator=cls.user, organization_name="Test Portfolio") + self.portfolio = Portfolio.objects.create(creator=self.user, organization_name="Test Portfolio") # Add an invited member who has been invited to manage domains - cls.invited_member_email = "invited@example.com" - cls.invitation = PortfolioInvitation.objects.create( - email=cls.invited_member_email, - portfolio=cls.portfolio, + 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, ], ) - cls.new_member_email = "newmember@example.com" + self.new_member_email = "newmember@example.com" - AllowedEmail.objects.get_or_create(email=cls.new_member_email) + AllowedEmail.objects.get_or_create(email=self.new_member_email) # Assign permissions to the user making requests UserPortfolioPermission.objects.create( - user=cls.user, - portfolio=cls.portfolio, + user=self.user, + portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], additional_permissions=[ UserPortfolioPermissionChoices.VIEW_MEMBERS, @@ -3084,14 +3085,13 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): ], ) - @classmethod - def tearDownClass(cls): + def tearDown(self): PortfolioInvitation.objects.all().delete() UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() User.objects.all().delete() AllowedEmail.objects.all().delete() - super().tearDownClass() + super().tearDown() @boto3_mocking.patching @less_console_noise_decorator @@ -3136,6 +3136,91 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): # Check that an email was sent self.assertTrue(mock_client.send_email.called) + @boto3_mocking.patching + @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_member_invite_for_previously_removed_user(self, mock_send_email): + """Tests the member invitation flow for an existing member which was previously removed.""" + self.client.force_login(self.user) + + # invite, then retrieve an existing user, then remove the user from the portfolio + retrieved_member_email = "retrieved@example.com" + retrieved_user = User.objects.create( + username="retrieved_user", + first_name="Retrieved", + last_name="User", + email=retrieved_member_email, + phone="8003111234", + title="retrieved", + ) + + retrieved_invitation = PortfolioInvitation.objects.create( + email=retrieved_member_email, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, + ) + retrieved_invitation.retrieve() + retrieved_invitation.save() + upp = UserPortfolioPermission.objects.filter( + user=retrieved_user, + portfolio=self.portfolio, + ) + upp.delete() + + # Simulate a session to ensure continuity + session_id = self.client.session.session_key + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + + mock_client_class = MagicMock() + mock_client = mock_client_class.return_value + + with boto3_mocking.clients.handler_for("sesv2", mock_client_class): + # Simulate submission of member invite for previously retrieved/removed member + final_response = self.client.post( + reverse("new-member"), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, + "domain_request_permissions": UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, + "member_permissions": "no_access", + "email": retrieved_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 and retrieved + self.assertFalse( + PortfolioInvitation.objects.filter( + email=retrieved_member_email, + portfolio=self.portfolio, + status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, + ).exists() + ) + # at least one retrieved invitation + self.assertTrue( + PortfolioInvitation.objects.filter( + email=retrieved_member_email, + portfolio=self.portfolio, + status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED, + ).exists() + ) + # Ensure exactly one UserPortfolioPermission exists for the retrieved user + self.assertEqual( + UserPortfolioPermission.objects.filter(user=retrieved_user, portfolio=self.portfolio).count(), + 1, + "Expected exactly one UserPortfolioPermission for the retrieved user." + ) + + @boto3_mocking.patching @less_console_noise_decorator @override_flag("organization_feature", active=True) From 5f545daed876bbbba1dfbe5f065c6c7e8857ab3d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 19:42:23 -0500 Subject: [PATCH 40/44] lint --- src/registrar/tests/test_views_portfolio.py | 78 ++++++++++----------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index a274e777b..8d06e35da 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3148,7 +3148,6 @@ class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): # Check that an email was sent self.assertTrue(mock_client.send_email.called) - @boto3_mocking.patching @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @@ -3189,49 +3188,44 @@ class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): session_id = self.client.session.session_key self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) - mock_client_class = MagicMock() - mock_client = mock_client_class.return_value + # Simulate submission of member invite for previously retrieved/removed member + final_response = self.client.post( + reverse("new-member"), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, + "domain_request_permissions": UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, + "member_permissions": "no_access", + "email": retrieved_member_email, + }, + ) - with boto3_mocking.clients.handler_for("sesv2", mock_client_class): - # Simulate submission of member invite for previously retrieved/removed member - final_response = self.client.post( - reverse("new-member"), - { - "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, - "domain_request_permissions": UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, - "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, - "member_permissions": "no_access", - "email": retrieved_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 and retrieved - self.assertFalse( - PortfolioInvitation.objects.filter( - email=retrieved_member_email, - portfolio=self.portfolio, - status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, - ).exists() - ) - # at least one retrieved invitation - self.assertTrue( - PortfolioInvitation.objects.filter( - email=retrieved_member_email, - portfolio=self.portfolio, - status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED, - ).exists() - ) - # Ensure exactly one UserPortfolioPermission exists for the retrieved user - self.assertEqual( - UserPortfolioPermission.objects.filter(user=retrieved_user, portfolio=self.portfolio).count(), - 1, - "Expected exactly one UserPortfolioPermission for the retrieved user." - ) + # Ensure the final submission is successful + self.assertEqual(final_response.status_code, 302) # Redirects + # Validate Database Changes + # Validate that portfolio invitation was created and retrieved + self.assertFalse( + PortfolioInvitation.objects.filter( + email=retrieved_member_email, + portfolio=self.portfolio, + status=PortfolioInvitation.PortfolioInvitationStatus.INVITED, + ).exists() + ) + # at least one retrieved invitation + self.assertTrue( + PortfolioInvitation.objects.filter( + email=retrieved_member_email, + portfolio=self.portfolio, + status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED, + ).exists() + ) + # Ensure exactly one UserPortfolioPermission exists for the retrieved user + self.assertEqual( + UserPortfolioPermission.objects.filter(user=retrieved_user, portfolio=self.portfolio).count(), + 1, + "Expected exactly one UserPortfolioPermission for the retrieved user.", + ) @boto3_mocking.patching @less_console_noise_decorator From 2bdd1cf71e12758b95e436cecbaeb556ec8997d9 Mon Sep 17 00:00:00 2001 From: Cameron Dixon Date: Wed, 5 Feb 2025 14:34:34 -0500 Subject: [PATCH 41/44] 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 42/44] 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 43/44] 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 44/44] 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)