From a554c6a5507f9fec38c689e289a666f9eff8096d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 6 Jan 2025 11:35:44 -0600 Subject: [PATCH 001/132] 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 002/132] 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 569ec088f031d650a9244a4e27b6896f2c294078 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 17 Jan 2025 10:47:54 -0800 Subject: [PATCH 003/132] Link and text changes --- src/registrar/templates/domain_renewal.html | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/domain_renewal.html b/src/registrar/templates/domain_renewal.html index 32e535ed5..7c01f6718 100644 --- a/src/registrar/templates/domain_renewal.html +++ b/src/registrar/templates/domain_renewal.html @@ -38,11 +38,11 @@ {{ block.super }}

Confirm the following information for accuracy

-

Review these details below. We +

Review the details below. We require that you maintain accurate information for the domain. The details you provide will only be used to support the administration of .gov and won't be made public.

-

If you would like to retire your domain instead, please +

If you would like to retire your domain instead, please contact us.

Required fields are marked with an asterisk (*).

@@ -119,9 +119,8 @@ >
From cb462c1ab2fb8293f9f60ecf2542b0a41780a9f7 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 17 Jan 2025 10:58:36 -0800 Subject: [PATCH 004/132] Fix tool tip text and adjust the banner for expiring soon --- src/registrar/models/domain.py | 2 +- src/registrar/templates/includes/domains_table.html | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6bd8278a1..d4c48f6bc 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1586,7 +1586,7 @@ class Domain(TimeStampedModel, DomainHelper): "This domain has expired, but it is still online. " "To renew this domain, contact help@get.gov." ) elif flag_is_active(request, "domain_renewal") and self.is_expiring(): - help_text = "This domain will expire soon. Contact one of the listed domain managers to renew the domain." + help_text = "This domain will expire soon. Go to “Manage” to renew the domain." else: help_text = Domain.State.get_help_text(self.state) diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index f7e36d330..9b95e840a 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -10,9 +10,9 @@ {% if has_domain_renewal_flag and num_expiring_domains > 0 and has_any_domains_portfolio_permission %} -
+
-
+

{% if num_expiring_domains == 1%} One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. @@ -76,9 +76,9 @@ {% if has_domain_renewal_flag and num_expiring_domains > 0 and not portfolio %} -

+
-
+

{% if num_expiring_domains == 1%} One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. From a3346d666c938b9b709c78965250ecffbe51a6df Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 21 Jan 2025 11:47:53 -0600 Subject: [PATCH 005/132] 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 61450053639b8b3bd2805f5907f9d61018233737 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 21 Jan 2025 13:37:51 -0800 Subject: [PATCH 006/132] Only if expired or expiring and domain manager can see /renewal otherwise 403 --- src/registrar/tests/test_views_domain.py | 1 - src/registrar/views/domain.py | 31 +++++++++++++++++++++++- src/registrar/views/utility/mixins.py | 3 ++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index f13490312..2be9e21fa 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -583,7 +583,6 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): reverse("domain", kwargs={"pk": self.domaintorenew.id}), ) - print("puglesss", self.domaintorenew.is_expired) # Make sure we see the link as a domain manager self.assertContains(detail_page, "Renew to maintain access") diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index f82d7005d..d0dd62210 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -311,11 +311,40 @@ class DomainView(DomainBaseView): self._update_session_with_domain() -class DomainRenewalView(DomainView): +class DomainRenewalView(DomainBaseView): """Domain detail overview page.""" template_name = "domain_renewal.html" + def get_context_data(self, **kwargs): + """Grabbing security email information for the renewal form""" + + context = super().get_context_data(**kwargs) + + default_emails = [DefaultEmail.PUBLIC_CONTACT_DEFAULT.value, DefaultEmail.LEGACY_DEFAULT.value] + + context["hidden_security_emails"] = default_emails + + security_email = self.object.get_security_email() + if security_email is None or security_email in default_emails: + context["security_email"] = None + return context + context["security_email"] = security_email + return context + + def in_editable_state(self, pk): + """Override in_editable_state from DomainPermission + Allow renewal form to be accessed""" + + requested_domain = None + if Domain.objects.filter(id=pk).exists(): + requested_domain = Domain.objects.get(id=pk) + + if requested_domain and (requested_domain.is_expiring() or requested_domain.is_expired()): + return True + + return False + def post(self, request, pk): domain = get_object_or_404(Domain, id=pk) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 11384ca09..954f18b88 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -192,7 +192,8 @@ class DomainPermission(PermissionsLoginMixin): def can_access_domain_via_portfolio(self, pk): """Most views should not allow permission to portfolio users. If particular views allow access to the domain pages, they will need to override - this function.""" + this function. + """ return False def in_editable_state(self, pk): From db97a3f715c5517c11a0b3e0f03fc235b87ef731 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 21 Jan 2025 15:57:38 -0600 Subject: [PATCH 007/132] 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 85503160f2d27bbdcb36795d0614bf2d263cc71e Mon Sep 17 00:00:00 2001 From: asaki222 Date: Wed, 22 Jan 2025 09:46:13 -0500 Subject: [PATCH 008/132] added test --- src/registrar/tests/test_views_domain.py | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 2be9e21fa..cdc6995d2 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -443,6 +443,15 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): name="domainrenewal.gov", ) + self.domainnotexpiring, _ = Domain.objects.get_or_create( + name="domainnotexpiring.gov", + expiration_date=timezone.now().date() + timedelta(days=65) + ) + + self.domainnodomainmanager, _ = Domain.objects.get_or_create( + name="domainnodomainmanager" + ) + UserDomainRole.objects.get_or_create( user=self.user, domain=self.domaintorenew, role=UserDomainRole.Roles.MANAGER ) @@ -655,7 +664,23 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): edit_page = renewal_page.click(href=edit_button_url, index=1) self.assertEqual(edit_page.status_code, 200) self.assertContains(edit_page, "Domain managers can update all information related to a domain") + + @override_flag("domain_renewal", active=True) + def test_domain_renewal_form_not_expired_or_expiring(self): + with less_console_noise(): + # Start on the Renewal page for the domain + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnotexpiring.id})) + self.assertEqual(renewal_page.status_code, 403) + @override_flag("domain_renewal", active=True) + def test_domain_renewal_form_does_not_appear_if_not_domain_manager(self): + with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( + Domain, "is_expired", self.custom_is_expired_true + ): + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnodomainmanager.id})) + self.assertEqual(renewal_page.status_code, 403) + + @override_flag("domain_renewal", active=True) def test_ack_checkbox_not_checked(self): From d2323aa9ab900b9fa4e3d4455d244549017399de Mon Sep 17 00:00:00 2001 From: asaki222 Date: Wed, 22 Jan 2025 10:00:59 -0500 Subject: [PATCH 009/132] ran linter --- src/registrar/tests/test_views_domain.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index cdc6995d2..58edea32d 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -444,13 +444,10 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): ) self.domainnotexpiring, _ = Domain.objects.get_or_create( - name="domainnotexpiring.gov", - expiration_date=timezone.now().date() + timedelta(days=65) + name="domainnotexpiring.gov", expiration_date=timezone.now().date() + timedelta(days=65) ) - self.domainnodomainmanager, _ = Domain.objects.get_or_create( - name="domainnodomainmanager" - ) + self.domainnodomainmanager, _ = Domain.objects.get_or_create(name="domainnodomainmanager") UserDomainRole.objects.get_or_create( user=self.user, domain=self.domaintorenew, role=UserDomainRole.Roles.MANAGER @@ -664,12 +661,12 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): edit_page = renewal_page.click(href=edit_button_url, index=1) self.assertEqual(edit_page.status_code, 200) self.assertContains(edit_page, "Domain managers can update all information related to a domain") - + @override_flag("domain_renewal", active=True) def test_domain_renewal_form_not_expired_or_expiring(self): with less_console_noise(): - # Start on the Renewal page for the domain - renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnotexpiring.id})) + # Start on the Renewal page for the domain + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnotexpiring.id})) self.assertEqual(renewal_page.status_code, 403) @override_flag("domain_renewal", active=True) @@ -677,10 +674,9 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( Domain, "is_expired", self.custom_is_expired_true ): - renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnodomainmanager.id})) + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domainnodomainmanager.id})) self.assertEqual(renewal_page.status_code, 403) - @override_flag("domain_renewal", active=True) def test_ack_checkbox_not_checked(self): From 0bf017f6501281850f3b9de2212c55a2b60186f9 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Wed, 22 Jan 2025 15:45:18 -0800 Subject: [PATCH 010/132] Fix link colour for renew and submit button text --- src/registrar/templates/domain_detail.html | 4 ++-- src/registrar/templates/domain_renewal.html | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 2cd3e5a5c..89685e074 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -52,11 +52,11 @@ {% if has_domain_renewal_flag and domain.is_expired and is_domain_manager %} This domain has expired, but it is still online. {% url 'domain-renewal' pk=domain.id as url %} - Renew to maintain access. + Renew to maintain access. {% elif has_domain_renewal_flag and domain.is_expiring and is_domain_manager %} This domain will expire soon. {% url 'domain-renewal' pk=domain.id as url %} - Renew to maintain access. + Renew to maintain access. {% elif has_domain_renewal_flag and domain.is_expiring and is_portfolio_user %} This domain will expire soon. Contact one of the listed domain managers to renew the domain. {% elif has_domain_renewal_flag and domain.is_expired and is_portfolio_user %} diff --git a/src/registrar/templates/domain_renewal.html b/src/registrar/templates/domain_renewal.html index 7c01f6718..fa833592c 100644 --- a/src/registrar/templates/domain_renewal.html +++ b/src/registrar/templates/domain_renewal.html @@ -130,7 +130,7 @@ name="submit_button" value="next" class="usa-button margin-top-3" - > Submit + > Submit and renew From 7bca3880f451c637d14b300df5074482fecc3ce2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 23 Jan 2025 13:48:35 -0500 Subject: [PATCH 011/132] cleanup_after_portfolio_member_deletion and unit tests --- src/registrar/models/portfolio_invitation.py | 25 ++ .../models/user_portfolio_permission.py | 11 + .../models/utility/portfolio_helper.py | 29 ++ src/registrar/tests/test_models.py | 299 ++++++++++++++++++ 4 files changed, 364 insertions(+) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 11c564c36..8feeb0794 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -8,6 +8,7 @@ from registrar.models import DomainInvitation, UserPortfolioPermission from .utility.portfolio_helper import ( UserPortfolioPermissionChoices, UserPortfolioRoleChoices, + cleanup_after_portfolio_member_deletion, validate_portfolio_invitation, ) # type: ignore from .utility.time_stamped_model import TimeStampedModel @@ -115,3 +116,27 @@ class PortfolioInvitation(TimeStampedModel): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() validate_portfolio_invitation(self) + + def delete(self, *args, **kwargs): + + User = get_user_model() + + email = self.email # Capture the email before the instance is deleted + portfolio = self.portfolio # Capture the portfolio before the instance is deleted + + # Call the superclass delete method to actually delete the instance + super().delete(*args, **kwargs) + + if self.status == self.PortfolioInvitationStatus.INVITED: + + # Query the user by email + users = User.objects.filter(email=email) + + if users.count() > 1: + # This should never happen, log an error if more than one object is returned + logger.error(f"Multiple users found with the same email: {email}") + + # Retrieve the first user, or None if no users are found + user = users.first() + + cleanup_after_portfolio_member_deletion(portfolio=portfolio, email=email, user=user) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 03a01b80d..b63579d76 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -5,6 +5,7 @@ from registrar.models.utility.portfolio_helper import ( UserPortfolioRoleChoices, DomainRequestPermissionDisplay, MemberPermissionDisplay, + cleanup_after_portfolio_member_deletion, validate_user_portfolio_permission, ) from .utility.time_stamped_model import TimeStampedModel @@ -186,3 +187,13 @@ class UserPortfolioPermission(TimeStampedModel): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() validate_user_portfolio_permission(self) + + def delete(self, *args, **kwargs): + + user = self.user # Capture the user before the instance is deleted + portfolio = self.portfolio # Capture the portfolio before the instance is deleted + + # Call the superclass delete method to actually delete the instance + super().delete(*args, **kwargs) + + cleanup_after_portfolio_member_deletion(portfolio=portfolio, email=user.email, user=user) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 4ae282f21..56216140f 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -227,3 +227,32 @@ def validate_portfolio_invitation(portfolio_invitation): "This user is already assigned to a portfolio invitation. " "Based on current waffle flag settings, users cannot be assigned to multiple portfolios." ) + + +def cleanup_after_portfolio_member_deletion(portfolio, email, user=None): + """ + Cleans up after removing a portfolio member or a portfolio invitation. + + Args: + portfolio: portfolio + user: passed when removing a portfolio member. + email: passed when removing a portfolio invitation, or passed as user.email + when removing a portfolio member. + """ + + DomainInvitation = apps.get_model("registrar.DomainInvitation") + UserDomainRole = apps.get_model("registrar.UserDomainRole") + + # Fetch domain invitations matching the criteria + invitations = DomainInvitation.objects.filter( + email=email, domain__domain_info__portfolio=portfolio, status=DomainInvitation.DomainInvitationStatus.INVITED + ) + + # Call `cancel_invitation` on each invitation + for invitation in invitations: + invitation.cancel_invitation() + invitation.save() + + if user: + # Remove user's domain roles for the current portfolio + UserDomainRole.objects.filter(user=user, domain__domain_info__portfolio=portfolio).delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index d8db0f043..04f8ae530 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -164,6 +164,7 @@ class TestPortfolioInvitations(TestCase): DomainInformation.objects.all().delete() Domain.objects.all().delete() UserPortfolioPermission.objects.all().delete() + UserDomainRole.objects.all().delete() Portfolio.objects.all().delete() PortfolioInvitation.objects.all().delete() User.objects.all().delete() @@ -442,6 +443,180 @@ class TestPortfolioInvitations(TestCase): pass + @less_console_noise_decorator + def test_delete_portfolio_invitation_deletes_portfolio_domain_invitations(self): + """Deleting a portfolio invitation causes domain invitations for the same email on the same + portfolio to be canceled.""" + + email_with_no_user = "email-with-no-user@email.gov" + + domain_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_1.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_1, portfolio=self.portfolio + ) + invite_1, _ = DomainInvitation.objects.get_or_create(email=email_with_no_user, domain=domain_in_portfolio_1) + + domain_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_2, portfolio=self.portfolio + ) + invite_2, _ = DomainInvitation.objects.get_or_create(email=email_with_no_user, domain=domain_in_portfolio_2) + + domain_not_in_portfolio, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio) + invite_3, _ = DomainInvitation.objects.get_or_create(email=email_with_no_user, domain=domain_not_in_portfolio) + + invitation_of_email_with_no_user, _ = PortfolioInvitation.objects.get_or_create( + email=email_with_no_user, + portfolio=self.portfolio, + roles=[self.portfolio_role_base, self.portfolio_role_admin], + additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + + # The domain invitations start off as INVITED + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # Delete member (invite) + invitation_of_email_with_no_user.delete() + + # Reload the objects from the database + invite_1 = DomainInvitation.objects.get(pk=invite_1.pk) + invite_2 = DomainInvitation.objects.get(pk=invite_2.pk) + invite_3 = DomainInvitation.objects.get(pk=invite_3.pk) + + # The domain invitations to the portfolio domains have been canceled + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.CANCELED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.CANCELED) + + # Invite 3 is unaffected + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + @less_console_noise_decorator + def test_delete_portfolio_invitation_deletes_user_domain_roles(self): + """Deleting a portfolio invitation causes domain invitations for the same email on the same + portfolio to be canceled, also deletes any exiting user domain roles on the portfolio for the + user if the user exists.""" + + domain_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_1.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_1, portfolio=self.portfolio + ) + invite_1, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_1) + + domain_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_2, portfolio=self.portfolio + ) + invite_2, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_2) + + domain_in_portfolio_3, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_3.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_3, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_3, role=UserDomainRole.Roles.MANAGER + ) + + domain_in_portfolio_4, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_4.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_4, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_4, role=UserDomainRole.Roles.MANAGER + ) + + domain_not_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_1) + invite_3, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_not_in_portfolio_1) + + domain_not_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_2) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_not_in_portfolio_2, role=UserDomainRole.Roles.MANAGER + ) + + # The domain invitations start off as INVITED + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # The user domain roles exist + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + + # Delete member (invite) + self.invitation.delete() + + # Reload the objects from the database + invite_1 = DomainInvitation.objects.get(pk=invite_1.pk) + invite_2 = DomainInvitation.objects.get(pk=invite_2.pk) + invite_3 = DomainInvitation.objects.get(pk=invite_3.pk) + + # The domain invitations to the portfolio domains have been canceled + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.CANCELED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.CANCELED) + + # Invite 3 is unaffected + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # The user domain roles have been deleted for the domains in portfolio + self.assertFalse( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertFalse( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + + # The user domain role on the domain not in portfolio still exists + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + class TestUserPortfolioPermission(TestCase): @less_console_noise_decorator @@ -457,6 +632,7 @@ class TestUserPortfolioPermission(TestCase): Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() + DomainInvitation.objects.all().delete() UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() User.objects.all().delete() @@ -750,6 +926,129 @@ class TestUserPortfolioPermission(TestCase): # Should return the forbidden permissions for member role self.assertEqual(member_only_permissions, set(member_forbidden)) + @less_console_noise_decorator + def test_delete_portfolio_permission_deletes_user_domain_roles(self): + """Deleting a user portfolio permission causes domain invitations for the same email on the same + portfolio to be canceled, also deletes any exiting user domain roles on the portfolio for the + user if the user exists.""" + + domain_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_1.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_1, portfolio=self.portfolio + ) + invite_1, _ = DomainInvitation.objects.get_or_create(email=self.user.email, domain=domain_in_portfolio_1) + + domain_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_2, portfolio=self.portfolio + ) + invite_2, _ = DomainInvitation.objects.get_or_create(email=self.user.email, domain=domain_in_portfolio_2) + + domain_in_portfolio_3, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_3.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_3, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_3, role=UserDomainRole.Roles.MANAGER + ) + + domain_in_portfolio_4, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_4.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_4, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_4, role=UserDomainRole.Roles.MANAGER + ) + + domain_not_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_1) + invite_3, _ = DomainInvitation.objects.get_or_create(email=self.user.email, domain=domain_not_in_portfolio_1) + + domain_not_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_2) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_not_in_portfolio_2, role=UserDomainRole.Roles.MANAGER + ) + + # Create portfolio permission + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=self.portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # The domain invitations start off as INVITED + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # The user domain roles exist + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + + # Delete member (user portfolio permission) + portfolio_permission.delete() + + # Reload the objects from the database + invite_1 = DomainInvitation.objects.get(pk=invite_1.pk) + invite_2 = DomainInvitation.objects.get(pk=invite_2.pk) + invite_3 = DomainInvitation.objects.get(pk=invite_3.pk) + + # The domain invitations to the portfolio domains have been canceled + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.CANCELED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.CANCELED) + + # Invite 3 is unaffected + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # The user domain roles have been deleted for the domains in portfolio + self.assertFalse( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertFalse( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + + # The user domain role on the domain not in portfolio still exists + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + class TestUser(TestCase): """Test actions that occur on user login, From c8665e86fca06303880fbb16a80b1b3bca1cf81d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 23 Jan 2025 14:00:03 -0500 Subject: [PATCH 012/132] Add test for retrieved invite --- src/registrar/tests/test_models.py | 114 +++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 04f8ae530..69af7ed2b 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -499,6 +499,120 @@ class TestPortfolioInvitations(TestCase): # Invite 3 is unaffected self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + @less_console_noise_decorator + def test_deleting_a_retrieved_invitation_has_no_side_effects(self): + """Deleting a retrieved portfolio invitation causes no side effects.""" + + domain_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_1.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_1, portfolio=self.portfolio + ) + invite_1, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_1) + + domain_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_2, portfolio=self.portfolio + ) + invite_2, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_2) + + domain_in_portfolio_3, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_3.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_3, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_3, role=UserDomainRole.Roles.MANAGER + ) + + domain_in_portfolio_4, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited_4.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_4, portfolio=self.portfolio + ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_in_portfolio_4, role=UserDomainRole.Roles.MANAGER + ) + + domain_not_in_portfolio_1, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_1) + invite_3, _ = DomainInvitation.objects.get_or_create(email=self.email, domain=domain_not_in_portfolio_1) + + domain_not_in_portfolio_2, _ = Domain.objects.get_or_create( + name="domain_not_in_portfolio_2.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_not_in_portfolio_2) + UserDomainRole.objects.get_or_create( + user=self.user, domain=domain_not_in_portfolio_2, role=UserDomainRole.Roles.MANAGER + ) + + # The domain invitations start off as INVITED + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + + # The user domain roles exist + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + + # retrieve the invitation + self.invitation.retrieve() + self.invitation.save() + + # Delete member (invite) + self.invitation.delete() + + # Reload the objects from the database + invite_1 = DomainInvitation.objects.get(pk=invite_1.pk) + invite_2 = DomainInvitation.objects.get(pk=invite_2.pk) + invite_3 = DomainInvitation.objects.get(pk=invite_3.pk) + + # Test that no side effects have been triggered + self.assertEqual(invite_1.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_2.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertEqual(invite_3.status, DomainInvitation.DomainInvitationStatus.INVITED) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_3, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_in_portfolio_4, + ).exists() + ) + self.assertTrue( + UserDomainRole.objects.filter( + user=self.user, + domain=domain_not_in_portfolio_2, + ).exists() + ) + @less_console_noise_decorator def test_delete_portfolio_invitation_deletes_user_domain_roles(self): """Deleting a portfolio invitation causes domain invitations for the same email on the same From a6de5293c2fdb51971e87f6ac7bc89b36061dbaf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 23 Jan 2025 15:15:24 -0700 Subject: [PATCH 013/132] Core logic --- src/registrar/forms/domain_request_wizard.py | 20 ++++++++++++++++++-- src/registrar/utility/waffle.py | 16 ++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 7c9dcb180..c1ea2dda1 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -2,7 +2,8 @@ from __future__ import annotations # allows forward references in annotations import logging from api.views import DOMAIN_API_MESSAGES from phonenumber_field.formfields import PhoneNumberField # type: ignore - +from registrar.models.portfolio import Portfolio +from registrar.utility.waffle import flag_is_active_anywhere from django import forms from django.core.validators import RegexValidator, MaxLengthValidator from django.utils.safestring import mark_safe @@ -321,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), + queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) organization_name = forms.CharField( @@ -363,6 +364,21 @@ class OrganizationContactForm(RegistrarForm): label="Urbanization (required for Puerto Rico only)", ) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # Set the queryset for federal agency. + # If the organization_requests flag is active, we hide data that exists in portfolios. + # NOTE: This function assumes that the federal_agency field was first set to None if a portfolio exists. + federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) + if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): + # Exclude both predefined agencies and those matching portfolio names in one query + federal_agency_queryset = federal_agency_queryset.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True) + ) + + self.fields["federal_agency"].queryset = federal_agency_queryset + def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" federal_agency = self.cleaned_data.get("federal_agency", None) diff --git a/src/registrar/utility/waffle.py b/src/registrar/utility/waffle.py index a78799e4c..f97a18874 100644 --- a/src/registrar/utility/waffle.py +++ b/src/registrar/utility/waffle.py @@ -1,5 +1,6 @@ from django.http import HttpRequest from waffle.decorators import flag_is_active +from waffle.models import get_waffle_flag_model def flag_is_active_for_user(user, flag_name): @@ -10,3 +11,18 @@ def flag_is_active_for_user(user, flag_name): request = HttpRequest() request.user = user return flag_is_active(request, flag_name) + + +def flag_is_active_anywhere(flag_name): + """Checks if the given flag name is active for anyone, anywhere. + More specifically, it checks on flag.everyone or flag.users.exists(). + Does not check self.superuser, self.staff or self.group. + + This function effectively behaves like a switch: + If said flag is enabled for someone, somewhere - return true. + Otherwise - return false. + """ + flag = get_waffle_flag_model().get(flag_name) + if flag.everyone is None: + return flag.users.exists() + return flag.everyone From 072738cf03113b3535894a171271334eb0b6630c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 23 Jan 2025 16:04:37 -0800 Subject: [PATCH 014/132] Add alert to user domain role delete confirmation page --- src/registrar/admin.py | 4 ++++ .../admin/user_domain_role_delete_confirmation.html | 13 +++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e89147b11..f0e435090 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1369,9 +1369,13 @@ class UserDomainRoleAdmin(ListHeaderAdmin, ImportExportModelAdmin): change_form_template = "django/admin/user_domain_role_change_form.html" + # Override for the delete confirmation page on the domain table (bulk delete action) + delete_selected_confirmation_template = "django/admin/user_domain_role_delete_confirmation.html" + # Fixes a bug where non-superusers are redirected to the main page def delete_view(self, request, object_id, extra_context=None): """Custom delete_view implementation that specifies redirect behaviour""" + self.delete_confirmation_template = "django/admin/user_domain_role_delete_confirmation.html" response = super().delete_view(request, object_id, extra_context) if isinstance(response, HttpResponseRedirect) and not request.user.has_perm("registrar.full_access_permission"): diff --git a/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html b/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html new file mode 100644 index 000000000..807502a31 --- /dev/null +++ b/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html @@ -0,0 +1,13 @@ +{% extends 'admin/delete_confirmation.html' %} +{% load i18n static %} + +{% block content %} +

+ {{ block.super }} +{% endblock %} From ea31249ef80a0bfd7db00e15578720a4a0598a0c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 23 Jan 2025 16:06:15 -0800 Subject: [PATCH 015/132] Move alert to content subtitle --- .../django/admin/user_domain_role_delete_confirmation.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html b/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html index 807502a31..171f4c3ea 100644 --- a/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html +++ b/src/registrar/templates/django/admin/user_domain_role_delete_confirmation.html @@ -1,7 +1,7 @@ {% extends 'admin/delete_confirmation.html' %} {% load i18n static %} -{% block content %} +{% block content_subtitle %}
-
- {% endif %} +
{% if portfolio %} diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index ee2c079f3..e1a4a7d81 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,16 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response -class ExportDataTypeRequests(DomainAndRequestsReportsPermission, View): - """Returns a domain requests report for a given user on the request""" - - def get(self, request, *args, **kwargs): - response = HttpResponse(content_type="text/csv") - response["Content-Disposition"] = 'attachment; filename="domain-requests.csv"' - csv_export.DomainRequestDataType.export_data_to_csv(response, request=request) - - return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): From 0834a9701ee6756e46555387718d32744fef9299 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 30 Jan 2025 15:33:55 -0800 Subject: [PATCH 077/132] Update link to message in domain invitation delete confirmation --- .../django/admin/domain_invitation_delete_confirmation.html | 2 +- .../admin/domain_invitation_delete_selected_confirmation.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html index abd0721de..215bf5ada 100644 --- a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html +++ b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html @@ -7,7 +7,7 @@

If you cancel the domain invitation here, it won't trigger any emails. It also won't remove their domain management privileges if they already have that role assigned. Go to the - User Domain Roles table + User Domain Roles table if you want to remove the user from a domain.

diff --git a/src/registrar/templates/django/admin/domain_invitation_delete_selected_confirmation.html b/src/registrar/templates/django/admin/domain_invitation_delete_selected_confirmation.html index 86a4f7134..2e15347c1 100644 --- a/src/registrar/templates/django/admin/domain_invitation_delete_selected_confirmation.html +++ b/src/registrar/templates/django/admin/domain_invitation_delete_selected_confirmation.html @@ -7,7 +7,7 @@

If you cancel the domain invitation here, it won't trigger any emails. It also won't remove their domain management privileges if they already have that role assigned. Go to the - User Domain Roles table + User Domain Roles table if you want to remove the user from a domain.

From 3dfb8d328f7f8edc5c7f1eccd41cb2f5ef951f7a Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 30 Jan 2025 15:35:16 -0800 Subject: [PATCH 078/132] Add comments for tests --- src/registrar/templates/domain_renewal.html | 3 +-- src/registrar/tests/test_views_domain.py | 25 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/domain_renewal.html b/src/registrar/templates/domain_renewal.html index 6848bf32e..703c2358f 100644 --- a/src/registrar/templates/domain_renewal.html +++ b/src/registrar/templates/domain_renewal.html @@ -120,8 +120,7 @@ diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index f2e3e1f5b..7d1e69783 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -479,6 +479,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_expiring_domain_on_detail_page_as_domain_manager(self): + """If a user is a domain manager and their domain is expiring soon, + user should be able to see the "Renew to maintain access" link domain overview detail box.""" self.client.force_login(self.user) with patch.object(Domain, "is_expiring", self.custom_is_expiring), patch.object( Domain, "is_expired", self.custom_is_expired_false @@ -497,6 +499,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) @override_flag("organization_feature", active=True) def test_expiring_domain_on_detail_page_in_org_model_as_a_non_domain_manager(self): + """In org model: If a user is NOT a domain manager and their domain is expiring soon, + user be notified to contact a domain manager in the domain overview detail box.""" portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user) non_dom_manage_user = get_user_model().objects.create( first_name="Non Domain", @@ -533,6 +537,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) @override_flag("organization_feature", active=True) def test_expiring_domain_on_detail_page_in_org_model_as_a_domain_manager(self): + """Inorg model: If a user is a domain manager and their domain is expiring soon, + user should be able to see the "Renew to maintain access" link domain overview detail box.""" portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org2", creator=self.user) domain_to_renew3, _ = Domain.objects.get_or_create(name="bogusdomain3.gov") @@ -551,6 +557,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_and_sidebar_expiring(self): + """If a user is a domain manager and their domain is expiring soon, + user should be able to see Renewal Form on the sidebar.""" self.client.force_login(self.user) with patch.object(Domain, "is_expiring", self.custom_is_expiring), patch.object( Domain, "is_expiring", self.custom_is_expiring @@ -578,7 +586,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_and_sidebar_expired(self): - + """If a user is a domain manager and their domain is expired, + user should be able to see Renewal Form on the sidebar.""" self.client.force_login(self.user) with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( @@ -607,6 +616,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_your_contact_info_edit(self): + """Checking that if a user is a domain manager they can edit the + Your Profile portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -625,6 +636,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_security_email_edit(self): + """Checking that if a user is a domain manager they can edit the + Security Email portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -646,6 +659,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_domain_manager_edit(self): + """Checking that if a user is a domain manager they can edit the + Domain Manager portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -664,6 +679,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_not_expired_or_expiring(self): + """Checking that if the user's domain is not expired or expiring that user should not be able + to access /renewal and that it should receive a 403.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domain_not_expiring.id})) @@ -671,6 +688,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_does_not_appear_if_not_domain_manager(self): + """If user is not a domain manager and tries to access /renewal, user should receive a 403.""" with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( Domain, "is_expired", self.custom_is_expired_true ): @@ -679,7 +697,7 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_ack_checkbox_not_checked(self): - + """If user don't check the checkbox, user should receive an error message.""" # Grab the renewal URL renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) @@ -691,7 +709,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_ack_checkbox_checked(self): - + """If user check the checkbox and submits the form, + user should be redirected Domain Over page with an updated by 1 year expiration date""" # Grab the renewal URL with patch.object(Domain, "renew_domain", self.custom_renew_domain): renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) From cbf23d80e7960646277efd2b40a755b2950c7174 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 30 Jan 2025 15:49:47 -0800 Subject: [PATCH 079/132] added lint --- src/registrar/views/report_views.py | 1 - src/registrar/views/utility/mixins.py | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index e1a4a7d81..1b9198c79 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,7 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): def get(self, request, *args, **kwargs): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 2d121849e..a05334169 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -189,9 +189,7 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ): + if not self.request.user.has_view_members_portfolio_permission(portfolio): return False return self.request.user.is_org_user(self.request) From 15b1311e461fe65954805b1b5ff9367678e5ce0e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 31 Jan 2025 12:38:26 -0500 Subject: [PATCH 080/132] lint --- src/registrar/views/utility/mixins.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 2d121849e..a05334169 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -189,9 +189,7 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ): + if not self.request.user.has_view_members_portfolio_permission(portfolio): return False return self.request.user.is_org_user(self.request) From 5996c4df123f181ba5d7baa3ff8477956d20506d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 31 Jan 2025 13:07:36 -0500 Subject: [PATCH 081/132] 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 8de354f31e76cd7047fbe6e35e59d526e0c3521f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 31 Jan 2025 16:07:41 -0500 Subject: [PATCH 082/132] testing traceboack --- src/registrar/views/utility/invitation_helper.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/views/utility/invitation_helper.py b/src/registrar/views/utility/invitation_helper.py index 7e6e53dba..ce679f6f2 100644 --- a/src/registrar/views/utility/invitation_helper.py +++ b/src/registrar/views/utility/invitation_helper.py @@ -3,7 +3,7 @@ from django.db import IntegrityError from registrar.models import PortfolioInvitation, User, UserPortfolioPermission from registrar.utility.email import EmailSendingError import logging - +import traceback from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, @@ -11,7 +11,7 @@ from registrar.utility.errors import ( OutsideOrgMemberError, ) -logger = logging.getLogger(__name__) +logger = logging.getLogger(__name__) # These methods are used by multiple views which share similar logic and function # when creating invitations and sending associated emails. These can be reused in @@ -61,11 +61,11 @@ def get_requested_user(email): def handle_invitation_exceptions(request, exception, email): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): - logger.warning(str(exception), exc_info=True) + logger.warning(exception, exc_info=True) messages.error(request, str(exception)) elif isinstance(exception, MissingEmailError): messages.error(request, str(exception)) - logger.error(str(exception), exc_info=True) + logger.error(exception, exc_info=True) elif isinstance(exception, OutsideOrgMemberError): messages.error(request, str(exception)) elif isinstance(exception, AlreadyDomainManagerError): From 117bfa9b169a07876354323f1b6b3ba1c5b0ee97 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 31 Jan 2025 14:07:16 -0800 Subject: [PATCH 083/132] Add space to domain manager deleted email template --- .../templates/emails/domain_manager_deleted_notification.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/emails/domain_manager_deleted_notification.txt b/src/registrar/templates/emails/domain_manager_deleted_notification.txt index b16b74dec..af0b92a8c 100644 --- a/src/registrar/templates/emails/domain_manager_deleted_notification.txt +++ b/src/registrar/templates/emails/domain_manager_deleted_notification.txt @@ -1,7 +1,7 @@ {% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} Hi,{% if domain_manager and domain_manager.first_name %} {{ domain_manager.first_name }}.{% endif %} -A domain manager was removed from {{ domain.name }}. +A domain manager was removed from {{ domain.name }}. REMOVED BY: {{ removed_by.email }} REMOVED ON: {{ date }} MANAGER REMOVED: {{ manager_removed.email }} From 001cb2273b53291852cb4b8f218b77ab1f4852e3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 05:50:03 -0500 Subject: [PATCH 084/132] fixing formatting of traceback --- src/registrar/config/settings.py | 7 ++++++- src/registrar/views/utility/invitation_helper.py | 3 +-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index a58e3e2f9..58250e85c 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -25,6 +25,7 @@ from typing import Final from botocore.config import Config import json import logging +import traceback from django.utils.log import ServerFormatter # # # ### @@ -471,7 +472,11 @@ class JsonFormatter(logging.Formatter): "lineno": record.lineno, "message": record.getMessage(), } - return json.dumps(log_record) + # Capture exception info if it exists + if record.exc_info: + log_record["exception"] = "".join(traceback.format_exception(*record.exc_info)) + + return json.dumps(log_record, ensure_ascii=False) class JsonServerFormatter(ServerFormatter): diff --git a/src/registrar/views/utility/invitation_helper.py b/src/registrar/views/utility/invitation_helper.py index ce679f6f2..18c427940 100644 --- a/src/registrar/views/utility/invitation_helper.py +++ b/src/registrar/views/utility/invitation_helper.py @@ -3,7 +3,6 @@ from django.db import IntegrityError from registrar.models import PortfolioInvitation, User, UserPortfolioPermission from registrar.utility.email import EmailSendingError import logging -import traceback from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, @@ -11,7 +10,7 @@ from registrar.utility.errors import ( OutsideOrgMemberError, ) -logger = logging.getLogger(__name__) +logger = logging.getLogger(__name__) # These methods are used by multiple views which share similar logic and function # when creating invitations and sending associated emails. These can be reused in From 041df217b5a6b5073dfcc27e1795e13a75943bde Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 07:02:22 -0500 Subject: [PATCH 085/132] 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 086/132] 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 087/132] 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 088/132] 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 089/132] 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 090/132] 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 091/132] 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 092/132] 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 093/132] 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 094/132] 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 095/132] 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 fa27f26942814a308e6f571685ee5aaf20f0e437 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Mon, 3 Feb 2025 16:03:18 -0500 Subject: [PATCH 096/132] yaml file and docs --- .github/workflows/delete-and-recreate-db.yaml | 91 +++++++++++++++++++ docs/developer/workflows/README.md | 7 ++ .../workflows/delete-and-recreate-db.md | 13 +++ 3 files changed, 111 insertions(+) create mode 100644 .github/workflows/delete-and-recreate-db.yaml create mode 100644 docs/developer/workflows/README.md create mode 100644 docs/developer/workflows/delete-and-recreate-db.md diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml new file mode 100644 index 000000000..96a698392 --- /dev/null +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -0,0 +1,91 @@ +# This workflow can be run from the CLI +# gh workflow run reset-db.yaml -f environment=ENVIRONMENT + +name: Reset database +run-name: Reset database for ${{ github.event.inputs.environment }} + +on: + workflow_dispatch: + inputs: + environment: + type: choice + description: Which environment should we flush and re-load data for? + options: + - el + - ad + - ms + - ag + - litterbox + - hotgov + - cb + - bob + - meoward + - backup + - ky + - es + - nl + - rh + - za + - gd + - rb + - ko + - ab + - rjm + - dk + +jobs: + reset-db: + runs-on: ubuntu-latest + env: + CF_USERNAME: CF_${{ github.event.inputs.environment }}_USERNAME + CF_PASSWORD: CF_${{ github.event.inputs.environment }}_PASSWORD + DESTINATION_ENVIRONMENT: ${{ github.event.inputs.environment}} + steps: + - name: Dekete and Recreate Database + env: + cf_username: ${{ secrets[env.CF_USERNAME] }} + cf_password: ${{ secrets[env.CF_PASSWORD] }} + run: | + # install cf cli and other tools + wget -q -O - https://packages.cloudfoundry.org/debian/cli.cloudfoundry.org.key | sudo gpg --dearmor -o /usr/share/keyrings/cli.cloudfoundry.org.gpg + echo "deb [signed-by=/usr/share/keyrings/cli.cloudfoundry.org.gpg] https://packages.cloudfoundry.org/debian stable main" | sudo tee /etc/apt/sources.list.d/cloudfoundry-cli.list + + sudo apt-get update + sudo apt-get install cf8-cli + cf api api.fr.cloud.gov + cf auth "$CF_USERNAME" "$CF_PASSWORD" + cf target -o cisa-dotgov -s $DESTINATION_ENVIRONMENT + + + + #unbind the service + cf unbind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database + #delete the service key + yes Y | cf delete-service-key getgov-$DESTINATION_ENVIRONMENT-database SERVICE_CONNECT + #delete the service + yes Y | cf delete-service getgov-$DESTINATION_ENVIRONMENT-database + #create it again + cf create-service aws-rds micro-psql getgov-$DESTINATION_ENVIRONMENT-database + # wait for it be created (up to 5 mins) + # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database + # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail + + until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + do + echo "Database not up yet, waiting..." + sleep 30 + done + + #rebind the service + cf bind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database + #restage the app or it will not connect to the database right for the next commands + cf restage getgov-$DESTINATION_ENVIRONMENT + #wait for the above command to finish + #if it is taking way to long and the annoying “instance starting” line that keeps repeating, then run following two commands in a separate window. This will interrupt the death loop where it keeps hitting an error with it failing health checks + #create the cache table and run migrations + cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py createcachetable' --name createcachetable + cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py migrate' --name migrate + + #check that your cloud.gov logs show this is done before you run the following command (or be like me and you have to run the command again because you were impatient. Running this before the migrate finishes will cause an error) + #load fixtures + cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py load' --name loaddata diff --git a/docs/developer/workflows/README.md b/docs/developer/workflows/README.md new file mode 100644 index 000000000..ea06a8a31 --- /dev/null +++ b/docs/developer/workflows/README.md @@ -0,0 +1,7 @@ +# Workflow + +======================== + +This directory contains files related to workflows + +Delete And Recreate Database is in [docs/ops](../workflows/delete-and-recreate-db.md/). \ No newline at end of file diff --git a/docs/developer/workflows/delete-and-recreate-db.md b/docs/developer/workflows/delete-and-recreate-db.md new file mode 100644 index 000000000..667ea6fe4 --- /dev/null +++ b/docs/developer/workflows/delete-and-recreate-db.md @@ -0,0 +1,13 @@ +## Delete And Recreate Database + +This script destroys recreates a database. This is another troubleshooting tool for issues with the database. It + +1. unbinds the database +2. deletes it +3. recreates it +4. binds it back to the sandbox +5. runs migrations + +Addition Info in this slack thread: + +[Slack thread](https://cisa-corp.slack.com/archives/C05BGB4L5NF/p1725495150772119) From 095f1abbf8af2cf20d378c14935f3acd60891954 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 3 Feb 2025 15:04:13 -0600 Subject: [PATCH 097/132] 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 9b4ae79170f6098384b352ab354de83e9705580c Mon Sep 17 00:00:00 2001 From: asaki222 Date: Mon, 3 Feb 2025 16:04:18 -0500 Subject: [PATCH 098/132] fixed text --- docs/developer/workflows/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developer/workflows/README.md b/docs/developer/workflows/README.md index ea06a8a31..6cff81add 100644 --- a/docs/developer/workflows/README.md +++ b/docs/developer/workflows/README.md @@ -1,4 +1,4 @@ -# Workflow +# Workflows Docs ======================== From ffa564b21ec865e0f90d1a855c359184a4f17ed5 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 3 Feb 2025 13:38:45 -0800 Subject: [PATCH 099/132] Add requested whitespace --- .../templates/emails/domain_manager_deleted_notification.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/templates/emails/domain_manager_deleted_notification.txt b/src/registrar/templates/emails/domain_manager_deleted_notification.txt index af0b92a8c..fbb1e47cc 100644 --- a/src/registrar/templates/emails/domain_manager_deleted_notification.txt +++ b/src/registrar/templates/emails/domain_manager_deleted_notification.txt @@ -2,6 +2,7 @@ Hi,{% if domain_manager and domain_manager.first_name %} {{ domain_manager.first_name }}.{% endif %} A domain manager was removed from {{ domain.name }}. + REMOVED BY: {{ removed_by.email }} REMOVED ON: {{ date }} MANAGER REMOVED: {{ manager_removed.email }} From d14a23621e7f76891bde5bdff5f8246dc342c288 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:36:45 -0700 Subject: [PATCH 100/132] fix bug --- src/registrar/models/domain_request.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index f58c6c6a2..fdeebaad6 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1302,15 +1302,16 @@ class DomainRequest(TimeStampedModel): """Unlocks the organization_contact step.""" if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one - Portfolio = apps.get_model("registrar.Portfolio") - return ( - FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + if self.organization_type == self.OrganizationChoices.FEDERAL and self.federal_agency: + Portfolio = apps.get_model("registrar.Portfolio") + return ( + FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ) + .filter(agency=self.federal_agency) + .exists() ) - .filter(agency=self.federal_agency) - .exists() - ) - return ( + return bool( self.federal_agency is not None or self.organization_name is not None or self.address_line1 is not None From 4736165d448dc8ed2bd8a3918e462fc462f9f85d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:48:06 -0800 Subject: [PATCH 101/132] Add tests for custome delete page content --- .../tests/test_admin_domain_invitation.py | 96 +++++++++++++++++++ .../tests/test_admin_userdomainrole.py | 91 ++++++++++++++++++ src/registrar/views/domain.py | 2 +- 3 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 src/registrar/tests/test_admin_domain_invitation.py create mode 100644 src/registrar/tests/test_admin_userdomainrole.py diff --git a/src/registrar/tests/test_admin_domain_invitation.py b/src/registrar/tests/test_admin_domain_invitation.py new file mode 100644 index 000000000..03215b9cd --- /dev/null +++ b/src/registrar/tests/test_admin_domain_invitation.py @@ -0,0 +1,96 @@ +from django.contrib.admin.sites import AdminSite +from django.test import RequestFactory, Client +from api.tests.common import less_console_noise_decorator +from django.urls import reverse +from django_webtest import WebTest # type: ignore +from registrar.admin import ( + DomainAdmin, +) +from registrar.models import ( + Domain, + DomainInvitation, + User, + Host, + DomainInformation, +) +from .common import create_superuser, MockEppLib, GenericTestHelper + + +class TestDomainInvitationAdminAsStaff(MockEppLib, WebTest): + """Test DomainInvitationAdmin class as staff user. + + Notes: + all tests share staffuser; do not change staffuser model in tests + tests have available staffuser, client, and admin + """ + + # csrf checks do not work with WebTest. + # We disable them here. TODO for another ticket. + csrf_checks = False + + @classmethod + def setUpClass(self): + super().setUpClass() + self.site = AdminSite() + self.admin = DomainAdmin(model=DomainInvitation, admin_site=self.site) + self.factory = RequestFactory() + self.superuser = create_superuser() + + def setUp(self): + self.client = Client(HTTP_HOST="localhost:8080") + self.client.force_login(self.superuser) + super().setUp() + self.app.set_user(self.superuser.username) + + def tearDown(self): + super().tearDown() + DomainInformation.objects.all().delete() + Host.objects.all().delete() + DomainInvitation.objects.all().delete() + + @classmethod + def tearDownClass(self): + User.objects.all().delete() + super().tearDownClass() + + @less_console_noise_decorator + def test_custom_delete_confirmation_page(self): + """Tests if custom alerts display on Domain Invitation delete page""" + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) + + domain_invitation_change_page = self.app.get( + reverse("admin:registrar_domaininvitation_change", args=[domain_invitation.pk]) + ) + + self.assertContains(domain_invitation_change_page, "domain-invitation-test.gov") + # click the "Delete" link + confirmation_page = domain_invitation_change_page.click("Delete", index=0) + + custom_alert_content = "If you cancel the domain invitation here" + self.assertContains(confirmation_page, custom_alert_content) + + @less_console_noise_decorator + def test_custom_selected_delete_confirmation_page(self): + """Tests if custom alerts display on Domain Invitation selected delete page from Domain Invitation table""" + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) + + # Get the index. The post expects the index to be encoded as a string + index = f"{domain_invitation.id}" + + test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url=reverse("admin:registrar_domaininvitation_changelist"), + model=Domain, + client=self.client, + ) + + # Simulate selecting a single record, then clicking "Delete selected domains" + response = test_helper.get_table_delete_confirmation_page("0", index) + + # Check for custom alert message + custom_alert_content = "If you cancel the domain invitation here" + self.assertContains(response, custom_alert_content) diff --git a/src/registrar/tests/test_admin_userdomainrole.py b/src/registrar/tests/test_admin_userdomainrole.py new file mode 100644 index 000000000..19c58f9b2 --- /dev/null +++ b/src/registrar/tests/test_admin_userdomainrole.py @@ -0,0 +1,91 @@ +from django.contrib.admin.sites import AdminSite +from django.test import RequestFactory, Client +from api.tests.common import less_console_noise_decorator +from django.urls import reverse +from django_webtest import WebTest # type: ignore +from registrar.admin import ( + DomainAdmin, +) +from registrar.models import Domain, DomainInvitation, User, Host, DomainInformation, UserDomainRole +from .common import create_superuser, MockEppLib, GenericTestHelper + + +class TestUserDomainRoleAsStaff(MockEppLib, WebTest): + """Test UserDomainRole class as staff user. + + Notes: + all tests share staffuser; do not change staffuser model in tests + tests have available staffuser, client, and admin + """ + + # csrf checks do not work with WebTest. + # We disable them here. TODO for another ticket. + csrf_checks = False + + @classmethod + def setUpClass(self): + super().setUpClass() + self.site = AdminSite() + self.admin = DomainAdmin(model=DomainInvitation, admin_site=self.site) + self.factory = RequestFactory() + self.superuser = create_superuser() + + def setUp(self): + self.client = Client(HTTP_HOST="localhost:8080") + self.client.force_login(self.superuser) + super().setUp() + self.app.set_user(self.superuser.username) + + def tearDown(self): + super().tearDown() + UserDomainRole.objects.all().delete() + DomainInformation.objects.all().delete() + Host.objects.all().delete() + DomainInvitation.objects.all().delete() + + @classmethod + def tearDownClass(self): + User.objects.all().delete() + super().tearDownClass() + + @less_console_noise_decorator + def test_custom_delete_confirmation_page(self): + """Tests if custom alerts display on User Domain Role delete page""" + domain, _ = Domain.objects.get_or_create(name="user-domain-role-test.gov", state=Domain.State.READY) + domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) + + domain_invitation_change_page = self.app.get( + reverse("admin:registrar_userdomainrole_change", args=[domain_role.pk]) + ) + + self.assertContains(domain_invitation_change_page, "user-domain-role-test.gov") + # click the "Delete" link + confirmation_page = domain_invitation_change_page.click("Delete", index=0) + + custom_alert_content = "If you remove someone from a domain here" + self.assertContains(confirmation_page, custom_alert_content) + + @less_console_noise_decorator + def test_custom_selected_delete_confirmation_page(self): + """Tests if custom alerts display on selected delete page from User Domain Roles table""" + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) + + # Get the index. The post expects the index to be encoded as a string + index = f"{domain_role.id}" + + test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url=reverse("admin:registrar_userdomainrole_changelist"), + model=Domain, + client=self.client, + ) + + # Simulate selecting a single record, then clicking "Delete selected domains" + response = test_helper.get_table_delete_confirmation_page("0", index) + + # Check for custom alert message + custom_alert_content = "If you remove someone from a domain here" + self.assertContains(response, custom_alert_content) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 43931a31d..bcdddb1ee 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1353,7 +1353,7 @@ class DomainDeleteUserView(UserDomainRolePermissionDeleteView): ) except EmailSendingError: logger.warning( - "Could not sent notification email to %s for domain %s", + "Could not send notification email to %s for domain %s", email, domain.name, exc_info=True, From a80ab45401b5d0fef6ad9630931de985879ba233 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 3 Feb 2025 18:22:51 -0500 Subject: [PATCH 102/132] lint --- src/registrar/admin.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4375268aa..31c75e05e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1331,8 +1331,8 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): def delete_queryset(self, request, queryset): """We override the delete method in the model. - When deleting in DJA, if you select multiple items in a table using checkboxes and apply a delete action, the model - delete does not get called. This method gets called instead. + When deleting in DJA, if you select multiple items in a table using checkboxes and apply a delete action + the model delete does not get called. This method gets called instead. This override makes sure our code in the model gets executed in these situations.""" for obj in queryset: obj.delete() # Calls the overridden delete method on each instance @@ -1671,8 +1671,8 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): def delete_queryset(self, request, queryset): """We override the delete method in the model. - When deleting in DJA, if you select multiple items in a table using checkboxes and apply a delete action, the model - delete does not get called. This method gets called instead. + When deleting in DJA, if you select multiple items in a table using checkboxes and apply a delete action, + the model delete does not get called. This method gets called instead. This override makes sure our code in the model gets executed in these situations.""" for obj in queryset: obj.delete() # Calls the overridden delete method on each instance From 8a82256206d7259d1a22441a32fa5b49218a7e1d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 05:13:58 -0500 Subject: [PATCH 103/132] update header and test --- .../templates/includes/header_extended.html | 2 ++ src/registrar/tests/test_views_portfolio.py | 22 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 1e40a508d..83b71c3ab 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -92,11 +92,13 @@ {% endif %} {% if has_organization_members_flag %} + {% if has_view_members_portfolio_permission %}
  • Members
    • + {% endif %} {% endif %}
  • diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 33f334f7f..b50c9a36f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1097,8 +1097,10 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_no_permissions(self): - """Test the nav contains a link to the no requests page""" + """Test the nav contains a link to the no requests page + Also test that members link not present""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) @@ -1118,20 +1120,23 @@ class TestPortfolio(WebTest): self.assertNotContains(portfolio_landing_page, "basic-nav-section-two") # link to requests self.assertNotContains(portfolio_landing_page, 'href="/requests/') - # link to create + # link to create request self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertNotContains(portfolio_landing_page, 'href="/members/') @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_all_permissions(self): """Test the nav contains a dropdown with a link to create and another link to view requests - Also test for the existence of the Create a new request btn on the requests page""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of the members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS], ) self.client.force_login(self.user) # create and submit a domain request @@ -1151,6 +1156,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) @@ -1160,15 +1167,18 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_view_but_not_edit_permissions(self): """Test the nav contains a simple link to view requests - Also test for the existence of the Create a new request btn on the requests page""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.VIEW_MEMBERS, ], ) self.client.force_login(self.user) @@ -1189,6 +1199,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) From a8fa08acb2afeb0aa719badcd2163899714f1c60 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 06:14:15 -0500 Subject: [PATCH 104/132] 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 105/132] 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 106/132] 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 394fc265d7294ef8eb472d7a5cdad5391820c5d9 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Tue, 4 Feb 2025 09:45:53 -0500 Subject: [PATCH 107/132] updated script --- .github/workflows/delete-and-recreate-db.yaml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index 96a698392..e52397d32 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -84,8 +84,7 @@ jobs: #if it is taking way to long and the annoying “instance starting” line that keeps repeating, then run following two commands in a separate window. This will interrupt the death loop where it keeps hitting an error with it failing health checks #create the cache table and run migrations cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py createcachetable' --name createcachetable - cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py migrate' --name migrate + cf run-task getgov-$DESTINATION_ENVIRONMENT --wait --command 'python manage.py migrate' --name migrate - #check that your cloud.gov logs show this is done before you run the following command (or be like me and you have to run the command again because you were impatient. Running this before the migrate finishes will cause an error) #load fixtures - cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py load' --name loaddata + cf run-task getgov-$DESTINATION_ENVIRONMENT --wait --command 'python manage.py load' --name loaddata From 62e7b0914425960787885f9205ec036eab01e7c9 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Tue, 4 Feb 2025 09:49:42 -0500 Subject: [PATCH 108/132] fixed typos --- docs/developer/workflows/delete-and-recreate-db.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/developer/workflows/delete-and-recreate-db.md b/docs/developer/workflows/delete-and-recreate-db.md index 667ea6fe4..98a2d445a 100644 --- a/docs/developer/workflows/delete-and-recreate-db.md +++ b/docs/developer/workflows/delete-and-recreate-db.md @@ -1,6 +1,6 @@ ## Delete And Recreate Database -This script destroys recreates a database. This is another troubleshooting tool for issues with the database. It +This script destroys and recreates a database. This is another troubleshooting tool for issues with the database. 1. unbinds the database 2. deletes it @@ -11,3 +11,4 @@ This script destroys recreates a database. This is another troubleshooting tool Addition Info in this slack thread: [Slack thread](https://cisa-corp.slack.com/archives/C05BGB4L5NF/p1725495150772119) +[Script](../../../../manage.get.gov/.github/workflows/delete-and-recreate-db.yaml) From 5bd8ec6761ea011c1b98b63ed8e115fc4cc5245f Mon Sep 17 00:00:00 2001 From: asaki222 Date: Tue, 4 Feb 2025 10:46:23 -0500 Subject: [PATCH 109/132] changed the file path --- docs/developer/workflows/delete-and-recreate-db.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/developer/workflows/delete-and-recreate-db.md b/docs/developer/workflows/delete-and-recreate-db.md index 98a2d445a..970449aa6 100644 --- a/docs/developer/workflows/delete-and-recreate-db.md +++ b/docs/developer/workflows/delete-and-recreate-db.md @@ -10,5 +10,5 @@ This script destroys and recreates a database. This is another troubleshooting t Addition Info in this slack thread: -[Slack thread](https://cisa-corp.slack.com/archives/C05BGB4L5NF/p1725495150772119) -[Script](../../../../manage.get.gov/.github/workflows/delete-and-recreate-db.yaml) +- [Slack thread](https://cisa-corp.slack.com/archives/C05BGB4L5NF/p1725495150772119) +- [Script](.github/workflows/delete-and-recreate-db.yaml) From 1a3e83b507d0e5f1bcb8779950fb158539d6d61d Mon Sep 17 00:00:00 2001 From: asaki222 Date: Tue, 4 Feb 2025 10:49:12 -0500 Subject: [PATCH 110/132] removed script link --- docs/developer/workflows/delete-and-recreate-db.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/developer/workflows/delete-and-recreate-db.md b/docs/developer/workflows/delete-and-recreate-db.md index 970449aa6..7b378ce47 100644 --- a/docs/developer/workflows/delete-and-recreate-db.md +++ b/docs/developer/workflows/delete-and-recreate-db.md @@ -11,4 +11,3 @@ This script destroys and recreates a database. This is another troubleshooting t Addition Info in this slack thread: - [Slack thread](https://cisa-corp.slack.com/archives/C05BGB4L5NF/p1725495150772119) -- [Script](.github/workflows/delete-and-recreate-db.yaml) From 367b2b804316237b6d1dffa7d8263e92211a0e45 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:58:17 -0700 Subject: [PATCH 111/132] Requested changes --- src/registrar/forms/domain_request_wizard.py | 8 ++++---- src/registrar/models/domain_request.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 0cf82558b..d7a02b124 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -322,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - # We populate this queryset in init. We want to exclude agencies with a portfolio. + # We populate this queryset in init. queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) @@ -369,12 +369,12 @@ class OrganizationContactForm(RegistrarForm): super().__init__(*args, **kwargs) # Set the queryset for federal agency. - # If the organization_requests flag is active, we hide data that exists in portfolios. + # If the organization_requests flag is active, We want to exclude agencies with a portfolio. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): - # Exclude both predefined agencies and those matching portfolio names in one query + # Exclude both predefined agencies and those matching portfolio records in one query federal_agency_queryset = federal_agency_queryset.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True) + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True) ) self.fields["federal_agency"].queryset = federal_agency_queryset diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index fdeebaad6..3071d40d8 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1306,9 +1306,9 @@ class DomainRequest(TimeStampedModel): Portfolio = apps.get_model("registrar.Portfolio") return ( FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True), ) - .filter(agency=self.federal_agency) + .filter(id=self.federal_agency.id) .exists() ) return bool( From b1f2dddd99a0f2592d4f75536d92888447d44c96 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 12:16:28 -0500 Subject: [PATCH 112/132] 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 113/132] 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 9bf152a8fc9abd5b1c6e07314daaa6bb08a06571 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:18:24 -0700 Subject: [PATCH 114/132] unit test --- src/registrar/tests/test_views_request.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index d33d7e6ad..f6eba2a56 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3227,7 +3227,9 @@ class TestDomainRequestWizard(TestWithUser, WebTest): federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") # Create a portfolio with matching organization name - Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + Portfolio.objects.create( + creator=self.user, organization_name=federal_agency.agency, federal_agency=federal_agency + ) # Create domain request with the portfolio agency domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) From 7c613a01f855fe18f9c3aeb99edf0057b4f002e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:52:40 -0700 Subject: [PATCH 115/132] 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 116/132] 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 117/132] 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 118/132] 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 119/132] 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 120/132] 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 121/132] 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 1bba542a557290056166b424e7df4bd66fca0808 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 12:36:41 -0500 Subject: [PATCH 122/132] changes --- .github/workflows/delete-and-recreate-db.yaml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index e52397d32..caf6a1996 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -41,7 +41,7 @@ jobs: CF_PASSWORD: CF_${{ github.event.inputs.environment }}_PASSWORD DESTINATION_ENVIRONMENT: ${{ github.event.inputs.environment}} steps: - - name: Dekete and Recreate Database + - name: Delete and Recreate Database env: cf_username: ${{ secrets[env.CF_USERNAME] }} cf_password: ${{ secrets[env.CF_PASSWORD] }} @@ -58,33 +58,33 @@ jobs: - #unbind the service + # unbind the service cf unbind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database #delete the service key yes Y | cf delete-service-key getgov-$DESTINATION_ENVIRONMENT-database SERVICE_CONNECT #delete the service yes Y | cf delete-service getgov-$DESTINATION_ENVIRONMENT-database - #create it again + # create it again cf create-service aws-rds micro-psql getgov-$DESTINATION_ENVIRONMENT-database # wait for it be created (up to 5 mins) # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail - until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + timeout 30 until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' do echo "Database not up yet, waiting..." sleep 30 done - #rebind the service + # rebind the service cf bind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database #restage the app or it will not connect to the database right for the next commands cf restage getgov-$DESTINATION_ENVIRONMENT - #wait for the above command to finish - #if it is taking way to long and the annoying “instance starting” line that keeps repeating, then run following two commands in a separate window. This will interrupt the death loop where it keeps hitting an error with it failing health checks - #create the cache table and run migrations + # wait for the above command to finish + # if it is taking way to long and the annoying “instance starting” line that keeps repeating, then run following two commands in a separate window. This will interrupt the death loop where it keeps hitting an error with it failing health checks + # create the cache table and run migrations cf run-task getgov-$DESTINATION_ENVIRONMENT --command 'python manage.py createcachetable' --name createcachetable cf run-task getgov-$DESTINATION_ENVIRONMENT --wait --command 'python manage.py migrate' --name migrate - #load fixtures + # load fixtures cf run-task getgov-$DESTINATION_ENVIRONMENT --wait --command 'python manage.py load' --name loaddata From a2151f4b0375ae2e745fe59e2465343dc741142e Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 12:43:13 -0500 Subject: [PATCH 123/132] updates --- .github/workflows/delete-and-recreate-db.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index caf6a1996..a24690f8e 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -70,11 +70,11 @@ jobs: # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail - timeout 30 until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + timeout 10 bash -c 'until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' do echo "Database not up yet, waiting..." sleep 30 - done + done' # rebind the service cf bind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database From 75ae3d3114fd8606e9f2d14999c6654556db65ab Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 12:46:27 -0500 Subject: [PATCH 124/132] trying again --- .github/workflows/delete-and-recreate-db.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index a24690f8e..6a0b7be94 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -70,11 +70,11 @@ jobs: # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail - timeout 10 bash -c 'until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + timeout 10 bash -c "until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' do - echo "Database not up yet, waiting..." + echo 'Database not up yet, waiting...' sleep 30 - done' + done" # rebind the service cf bind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database 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 125/132] 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) From 640805833b0073b4fcd52c7fe9a5a0e4aeaaa69f Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 13:50:09 -0500 Subject: [PATCH 126/132] increased timeout --- .github/workflows/delete-and-recreate-db.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index 6a0b7be94..7d19dad09 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -70,7 +70,7 @@ jobs: # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail - timeout 10 bash -c "until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + timeout 480 bash -c "until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' do echo 'Database not up yet, waiting...' sleep 30 From 8c35346a58c18f88826b8dae6734f0e43812db5b Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 13:51:19 -0500 Subject: [PATCH 127/132] increased timeout and fixed spacing --- .github/workflows/delete-and-recreate-db.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index 7d19dad09..a61ee626e 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -70,11 +70,11 @@ jobs: # this checks the creation cf service getgov-$DESTINATION_ENVIRONMENT-database # the below command with check “status” line using cf service command mentioned above. if it says “create in progress” it will keep waiting otherwise the next steps fail - timeout 480 bash -c "until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' - do - echo 'Database not up yet, waiting...' - sleep 30 - done" + timeout 480 bash -c "until cf service getgov-$DESTINATION_ENVIRONMENT-database | grep -q 'The service instance status is succeeded' + do + echo 'Database not up yet, waiting...' + sleep 30 + done" # rebind the service cf bind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database From 5247bc3c8701efa5136dc515a15e2624e40c2ca6 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 6 Feb 2025 14:33:58 -0500 Subject: [PATCH 128/132] missed a comment space --- .github/workflows/delete-and-recreate-db.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/delete-and-recreate-db.yaml b/.github/workflows/delete-and-recreate-db.yaml index a61ee626e..ecdf54bbc 100644 --- a/.github/workflows/delete-and-recreate-db.yaml +++ b/.github/workflows/delete-and-recreate-db.yaml @@ -62,7 +62,7 @@ jobs: cf unbind-service getgov-$DESTINATION_ENVIRONMENT getgov-$DESTINATION_ENVIRONMENT-database #delete the service key yes Y | cf delete-service-key getgov-$DESTINATION_ENVIRONMENT-database SERVICE_CONNECT - #delete the service + # delete the service yes Y | cf delete-service getgov-$DESTINATION_ENVIRONMENT-database # create it again cf create-service aws-rds micro-psql getgov-$DESTINATION_ENVIRONMENT-database From ba9deb7a9353d71edea884add37ff92076ecbf08 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 6 Feb 2025 12:00:08 -0800 Subject: [PATCH 129/132] Move admin tests to admin.py --- src/registrar/tests/test_admin.py | 108 +++++++++++++++++- .../tests/test_admin_domain_invitation.py | 96 ---------------- .../tests/test_admin_userdomainrole.py | 91 --------------- 3 files changed, 104 insertions(+), 191 deletions(-) delete mode 100644 src/registrar/tests/test_admin_domain_invitation.py delete mode 100644 src/registrar/tests/test_admin_userdomainrole.py diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 387319663..9c840fb99 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -54,6 +54,7 @@ from registrar.models import ( from .common import ( MockDbForSharedTests, AuditedAdminMockData, + MockEppLib, completed_domain_request, generic_domain_object, less_console_noise, @@ -120,23 +121,35 @@ class TestFsmModelResource(TestCase): fsm_field_mock.save.assert_not_called() -class TestDomainInvitationAdmin(TestCase): +class TestDomainInvitationAdmin(MockEppLib, WebTest): """Tests for the DomainInvitationAdmin class as super user Notes: all tests share superuser; do not change this model in tests tests have available superuser, client, and admin """ + # csrf checks do not work with WebTest. + # We disable them here. TODO for another ticket. + csrf_checks = False + + @classmethod + def setUpClass(self): + super().setUpClass() + self.site = AdminSite() + self.factory = RequestFactory() + self.superuser = create_superuser() def setUp(self): - self.factory = RequestFactory() + super().setUp() self.admin = ListHeaderAdmin(model=DomainInvitationAdmin, admin_site=AdminSite()) - self.superuser = create_superuser() self.domain = Domain.objects.create(name="example.com") self.portfolio = Portfolio.objects.create(organization_name="new portfolio", creator=self.superuser) DomainInformation.objects.create(domain=self.domain, portfolio=self.portfolio, creator=self.superuser) """Create a client object""" self.client = Client(HTTP_HOST="localhost:8080") + self.client.force_login(self.superuser) + self.app.set_user(self.superuser.username) + def tearDown(self): """Delete all DomainInvitation objects""" @@ -1065,6 +1078,49 @@ class TestDomainInvitationAdmin(TestCase): self.assertEqual(DomainInvitation.objects.count(), 0) self.assertEqual(PortfolioInvitation.objects.count(), 1) + @less_console_noise_decorator + def test_custom_delete_confirmation_page(self): + """Tests if custom alerts display on Domain Invitation delete page""" + self.client.force_login(self.superuser) + self.app.set_user(self.superuser.username) + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) + + domain_invitation_change_page = self.app.get( + reverse("admin:registrar_domaininvitation_change", args=[domain_invitation.pk]) + ) + + self.assertContains(domain_invitation_change_page, "domain-invitation-test.gov") + # click the "Delete" link + confirmation_page = domain_invitation_change_page.click("Delete", index=0) + + custom_alert_content = "If you cancel the domain invitation here" + self.assertContains(confirmation_page, custom_alert_content) + + @less_console_noise_decorator + def test_custom_selected_delete_confirmation_page(self): + """Tests if custom alerts display on Domain Invitation selected delete page from Domain Invitation table""" + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) + + # Get the index. The post expects the index to be encoded as a string + index = f"{domain_invitation.id}" + + test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url=reverse("admin:registrar_domaininvitation_changelist"), + model=Domain, + client=self.client, + ) + + # Simulate selecting a single record, then clicking "Delete selected domains" + response = test_helper.get_table_delete_confirmation_page("0", index) + + # Check for custom alert message + custom_alert_content = "If you cancel the domain invitation here" + self.assertContains(response, custom_alert_content) class TestUserPortfolioPermissionAdmin(TestCase): """Tests for the PortfolioInivtationAdmin class""" @@ -1922,7 +1978,7 @@ class TestDomainInformationAdmin(TestCase): self.test_helper.assert_table_sorted("-4", ("-creator__first_name", "-creator__last_name")) -class TestUserDomainRoleAdmin(TestCase): +class TestUserDomainRoleAdmin(MockEppLib, WebTest): """Tests for the UserDomainRoleAdmin class as super user Notes: @@ -1949,6 +2005,8 @@ class TestUserDomainRoleAdmin(TestCase): """Setup environment for a mock admin user""" super().setUp() self.client = Client(HTTP_HOST="localhost:8080") + self.client.force_login(self.superuser) + self.app.set_user(self.superuser.username) def tearDown(self): """Delete all Users, Domains, and UserDomainRoles""" @@ -2111,6 +2169,48 @@ class TestUserDomainRoleAdmin(TestCase): # We only need to check for the end of the HTML string self.assertContains(response, "Joe Jones AntarcticPolarBears@example.com", count=1) + @less_console_noise_decorator + def test_custom_delete_confirmation_page(self): + """Tests if custom alerts display on User Domain Role delete page""" + domain, _ = Domain.objects.get_or_create(name="user-domain-role-test.gov", state=Domain.State.READY) + domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) + + domain_invitation_change_page = self.app.get( + reverse("admin:registrar_userdomainrole_change", args=[domain_role.pk]) + ) + + self.assertContains(domain_invitation_change_page, "user-domain-role-test.gov") + # click the "Delete" link + confirmation_page = domain_invitation_change_page.click("Delete", index=0) + + custom_alert_content = "If you remove someone from a domain here" + self.assertContains(confirmation_page, custom_alert_content) + + @less_console_noise_decorator + def test_custom_selected_delete_confirmation_page(self): + """Tests if custom alerts display on selected delete page from User Domain Roles table""" + domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) + domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) + + # Get the index. The post expects the index to be encoded as a string + index = f"{domain_role.id}" + + test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url=reverse("admin:registrar_userdomainrole_changelist"), + model=Domain, + client=self.client, + ) + + # Simulate selecting a single record, then clicking "Delete selected domains" + response = test_helper.get_table_delete_confirmation_page("0", index) + + # Check for custom alert message + custom_alert_content = "If you remove someone from a domain here" + self.assertContains(response, custom_alert_content) + class TestListHeaderAdmin(TestCase): """Tests for the ListHeaderAdmin class as super user diff --git a/src/registrar/tests/test_admin_domain_invitation.py b/src/registrar/tests/test_admin_domain_invitation.py deleted file mode 100644 index 03215b9cd..000000000 --- a/src/registrar/tests/test_admin_domain_invitation.py +++ /dev/null @@ -1,96 +0,0 @@ -from django.contrib.admin.sites import AdminSite -from django.test import RequestFactory, Client -from api.tests.common import less_console_noise_decorator -from django.urls import reverse -from django_webtest import WebTest # type: ignore -from registrar.admin import ( - DomainAdmin, -) -from registrar.models import ( - Domain, - DomainInvitation, - User, - Host, - DomainInformation, -) -from .common import create_superuser, MockEppLib, GenericTestHelper - - -class TestDomainInvitationAdminAsStaff(MockEppLib, WebTest): - """Test DomainInvitationAdmin class as staff user. - - Notes: - all tests share staffuser; do not change staffuser model in tests - tests have available staffuser, client, and admin - """ - - # csrf checks do not work with WebTest. - # We disable them here. TODO for another ticket. - csrf_checks = False - - @classmethod - def setUpClass(self): - super().setUpClass() - self.site = AdminSite() - self.admin = DomainAdmin(model=DomainInvitation, admin_site=self.site) - self.factory = RequestFactory() - self.superuser = create_superuser() - - def setUp(self): - self.client = Client(HTTP_HOST="localhost:8080") - self.client.force_login(self.superuser) - super().setUp() - self.app.set_user(self.superuser.username) - - def tearDown(self): - super().tearDown() - DomainInformation.objects.all().delete() - Host.objects.all().delete() - DomainInvitation.objects.all().delete() - - @classmethod - def tearDownClass(self): - User.objects.all().delete() - super().tearDownClass() - - @less_console_noise_decorator - def test_custom_delete_confirmation_page(self): - """Tests if custom alerts display on Domain Invitation delete page""" - domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) - domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) - - domain_invitation_change_page = self.app.get( - reverse("admin:registrar_domaininvitation_change", args=[domain_invitation.pk]) - ) - - self.assertContains(domain_invitation_change_page, "domain-invitation-test.gov") - # click the "Delete" link - confirmation_page = domain_invitation_change_page.click("Delete", index=0) - - custom_alert_content = "If you cancel the domain invitation here" - self.assertContains(confirmation_page, custom_alert_content) - - @less_console_noise_decorator - def test_custom_selected_delete_confirmation_page(self): - """Tests if custom alerts display on Domain Invitation selected delete page from Domain Invitation table""" - domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) - domain_invitation, _ = DomainInvitation.objects.get_or_create(domain=domain) - - # Get the index. The post expects the index to be encoded as a string - index = f"{domain_invitation.id}" - - test_helper = GenericTestHelper( - factory=self.factory, - user=self.superuser, - admin=self.admin, - url=reverse("admin:registrar_domaininvitation_changelist"), - model=Domain, - client=self.client, - ) - - # Simulate selecting a single record, then clicking "Delete selected domains" - response = test_helper.get_table_delete_confirmation_page("0", index) - - # Check for custom alert message - custom_alert_content = "If you cancel the domain invitation here" - self.assertContains(response, custom_alert_content) diff --git a/src/registrar/tests/test_admin_userdomainrole.py b/src/registrar/tests/test_admin_userdomainrole.py deleted file mode 100644 index 19c58f9b2..000000000 --- a/src/registrar/tests/test_admin_userdomainrole.py +++ /dev/null @@ -1,91 +0,0 @@ -from django.contrib.admin.sites import AdminSite -from django.test import RequestFactory, Client -from api.tests.common import less_console_noise_decorator -from django.urls import reverse -from django_webtest import WebTest # type: ignore -from registrar.admin import ( - DomainAdmin, -) -from registrar.models import Domain, DomainInvitation, User, Host, DomainInformation, UserDomainRole -from .common import create_superuser, MockEppLib, GenericTestHelper - - -class TestUserDomainRoleAsStaff(MockEppLib, WebTest): - """Test UserDomainRole class as staff user. - - Notes: - all tests share staffuser; do not change staffuser model in tests - tests have available staffuser, client, and admin - """ - - # csrf checks do not work with WebTest. - # We disable them here. TODO for another ticket. - csrf_checks = False - - @classmethod - def setUpClass(self): - super().setUpClass() - self.site = AdminSite() - self.admin = DomainAdmin(model=DomainInvitation, admin_site=self.site) - self.factory = RequestFactory() - self.superuser = create_superuser() - - def setUp(self): - self.client = Client(HTTP_HOST="localhost:8080") - self.client.force_login(self.superuser) - super().setUp() - self.app.set_user(self.superuser.username) - - def tearDown(self): - super().tearDown() - UserDomainRole.objects.all().delete() - DomainInformation.objects.all().delete() - Host.objects.all().delete() - DomainInvitation.objects.all().delete() - - @classmethod - def tearDownClass(self): - User.objects.all().delete() - super().tearDownClass() - - @less_console_noise_decorator - def test_custom_delete_confirmation_page(self): - """Tests if custom alerts display on User Domain Role delete page""" - domain, _ = Domain.objects.get_or_create(name="user-domain-role-test.gov", state=Domain.State.READY) - domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) - - domain_invitation_change_page = self.app.get( - reverse("admin:registrar_userdomainrole_change", args=[domain_role.pk]) - ) - - self.assertContains(domain_invitation_change_page, "user-domain-role-test.gov") - # click the "Delete" link - confirmation_page = domain_invitation_change_page.click("Delete", index=0) - - custom_alert_content = "If you remove someone from a domain here" - self.assertContains(confirmation_page, custom_alert_content) - - @less_console_noise_decorator - def test_custom_selected_delete_confirmation_page(self): - """Tests if custom alerts display on selected delete page from User Domain Roles table""" - domain, _ = Domain.objects.get_or_create(name="domain-invitation-test.gov", state=Domain.State.READY) - domain_role, _ = UserDomainRole.objects.get_or_create(domain=domain, user=self.superuser) - - # Get the index. The post expects the index to be encoded as a string - index = f"{domain_role.id}" - - test_helper = GenericTestHelper( - factory=self.factory, - user=self.superuser, - admin=self.admin, - url=reverse("admin:registrar_userdomainrole_changelist"), - model=Domain, - client=self.client, - ) - - # Simulate selecting a single record, then clicking "Delete selected domains" - response = test_helper.get_table_delete_confirmation_page("0", index) - - # Check for custom alert message - custom_alert_content = "If you remove someone from a domain here" - self.assertContains(response, custom_alert_content) From bcf2f0946c2f7f45f50ccea440797250b8ab4fd3 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 6 Feb 2025 13:07:30 -0800 Subject: [PATCH 130/132] Fix linting --- src/registrar/tests/test_admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index adec70aeb..b488ee655 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -128,6 +128,7 @@ class TestDomainInvitationAdmin(MockEppLib, WebTest): all tests share superuser; do not change this model in tests tests have available superuser, client, and admin """ + # csrf checks do not work with WebTest. # We disable them here. TODO for another ticket. csrf_checks = False @@ -150,7 +151,6 @@ class TestDomainInvitationAdmin(MockEppLib, WebTest): self.client.force_login(self.superuser) self.app.set_user(self.superuser.username) - def tearDown(self): """Delete all DomainInvitation objects""" PortfolioInvitation.objects.all().delete() @@ -1128,6 +1128,7 @@ class TestDomainInvitationAdmin(MockEppLib, WebTest): custom_alert_content = "If you cancel the domain invitation here" self.assertContains(response, custom_alert_content) + class TestUserPortfolioPermissionAdmin(TestCase): """Tests for the PortfolioInivtationAdmin class""" From 40f21b86ee279f1ace236aca37204b57127b3bd6 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 7 Feb 2025 09:47:56 -0800 Subject: [PATCH 131/132] Remove unused library --- src/registrar/tests/test_admin.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index b488ee655..9447d211f 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -54,7 +54,6 @@ from registrar.models import ( from .common import ( MockDbForSharedTests, AuditedAdminMockData, - MockEppLib, completed_domain_request, generic_domain_object, less_console_noise, @@ -121,7 +120,7 @@ class TestFsmModelResource(TestCase): fsm_field_mock.save.assert_not_called() -class TestDomainInvitationAdmin(MockEppLib, WebTest): +class TestDomainInvitationAdmin(WebTest): """Tests for the DomainInvitationAdmin class as super user Notes: @@ -2073,7 +2072,7 @@ class TestDomainInformationAdmin(TestCase): self.test_helper.assert_table_sorted("-4", ("-creator__first_name", "-creator__last_name")) -class TestUserDomainRoleAdmin(MockEppLib, WebTest): +class TestUserDomainRoleAdmin(WebTest): """Tests for the UserDomainRoleAdmin class as super user Notes: From 88d3a90521e1441e56e404b7fc93e2c91d22e771 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 10 Feb 2025 11:56:01 -0500 Subject: [PATCH 132/132] linter errors --- .../management/commands/create_federal_portfolio.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index 75c5d6acc..d753d0ce8 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -204,7 +204,9 @@ class Command(BaseCommand): # Fetch all users with manager roles for the domains # select_related means that a db query will not be occur when you do user_domain_role.user # Its similar to a set or dict in that it costs slightly more upfront in exchange for perf later - user_domain_roles = UserDomainRole.objects.select_related("user").filter(domain__in=domains, role=UserDomainRole.Roles.MANAGER) + user_domain_roles = UserDomainRole.objects.select_related("user").filter( + domain__in=domains, role=UserDomainRole.Roles.MANAGER + ) domain_managers.update(user_domain_roles) invited_managers: set[str] = set() @@ -243,9 +245,10 @@ class Command(BaseCommand): _, created = PortfolioInvitation.objects.get_or_create( portfolio=portfolio, email=email, - defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.INVITED, - "roles": [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - }, + defaults={ + "status": PortfolioInvitation.PortfolioInvitationStatus.INVITED, + "roles": [UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + }, ) if created: self.added_invitations.add(email)