From 0bc6595a17321739e5748d6e828a904ff8b1b216 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 11 Mar 2025 15:16:48 -0700 Subject: [PATCH 1/6] Fix the manage url issue for action needed emails --- .../templates/emails/transition_domain_invitation.txt | 2 +- src/registrar/utility/admin_helpers.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/emails/transition_domain_invitation.txt b/src/registrar/templates/emails/transition_domain_invitation.txt index 14dd626dd..35947eb72 100644 --- a/src/registrar/templates/emails/transition_domain_invitation.txt +++ b/src/registrar/templates/emails/transition_domain_invitation.txt @@ -57,7 +57,7 @@ THANK YOU The .gov team .Gov blog -Domain management <{{ manage_url }}}> +Domain management <{{ manage_url }}> Get.gov The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency (CISA) diff --git a/src/registrar/utility/admin_helpers.py b/src/registrar/utility/admin_helpers.py index 93a0a16b5..adbc182d0 100644 --- a/src/registrar/utility/admin_helpers.py +++ b/src/registrar/utility/admin_helpers.py @@ -1,4 +1,5 @@ from registrar.models.domain_request import DomainRequest +from django.conf import settings from django.template.loader import get_template from django.utils.html import format_html from django.urls import reverse @@ -35,8 +36,13 @@ def _get_default_email(domain_request, file_path, reason, excluded_reasons=None) return None recipient = domain_request.creator + env_base_url = settings.BASE_URL + # If NOT in prod, update instances of "manage.get.gov" links to point to + # current environment, ie "getgov-rh.app.cloud.gov" + manage_url = env_base_url if not settings.IS_PRODUCTION else "https://manage.get.gov" + # Return the context of the rendered views - context = {"domain_request": domain_request, "recipient": recipient, "reason": reason} + context = {"domain_request": domain_request, "recipient": recipient, "reason": reason, "manage_url": manage_url} email_body_text = get_template(file_path).render(context=context) email_body_text_cleaned = email_body_text.strip().lstrip("\n") if email_body_text else None From 585569a006420b713f416c88a5402645b38ca514 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 12 Mar 2025 10:29:38 -0600 Subject: [PATCH 2/6] bug fix --- src/registrar/forms/portfolio.py | 10 +++- .../models/utility/portfolio_helper.py | 60 +++++++++++++++---- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index b83e718cb..2ee174050 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -22,6 +22,7 @@ from registrar.models.utility.portfolio_helper import ( get_domains_display, get_members_description_display, get_members_display, + get_portfolio_invitation_associations, ) logger = logging.getLogger(__name__) @@ -459,7 +460,14 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm): if hasattr(e, "code"): field = "email" if "email" in self.fields else None if e.code == "has_existing_permissions": - self.add_error(field, f"{self.instance.email} is already a member of another .gov organization.") + existing_permissions, existing_invitations = ( + get_portfolio_invitation_associations(self.instance) + ) + + same_portfolio_for_permissions = existing_permissions.exclude(portfolio=self.instance.portfolio) + same_portfolio_for_invitations = existing_invitations.exclude(portfolio=self.instance.portfolio) + if same_portfolio_for_permissions.exists() or same_portfolio_for_invitations.exists(): + self.add_error(field, f"{self.instance.email} is already a member of another .gov organization.") override_error = True elif e.code == "has_existing_invitations": self.add_error( diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 009ea3c26..707dfcf54 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -286,8 +286,8 @@ def validate_user_portfolio_permission(user_portfolio_permission): # == Validate the multiple_porfolios flag. == # if not flag_is_active_for_user(user_portfolio_permission.user, "multiple_portfolios"): - existing_permissions = UserPortfolioPermission.objects.exclude(id=user_portfolio_permission.id).filter( - user=user_portfolio_permission.user + existing_permissions, existing_invitations = ( + get_user_portfolio_permission_associations(user_portfolio_permission) ) if existing_permissions.exists(): raise ValidationError( @@ -296,10 +296,6 @@ def validate_user_portfolio_permission(user_portfolio_permission): code="has_existing_permissions", ) - 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. " @@ -307,6 +303,29 @@ def validate_user_portfolio_permission(user_portfolio_permission): code="has_existing_invitations", ) +def get_user_portfolio_permission_associations(user_portfolio_permission): + """ + Retrieves the associations for a user portfolio invitation. + + Returns: + A tuple: + (existing_permissions, existing_invitations) + where: + - existing_permissions: UserPortfolioPermission objects excluding the current permission. + - existing_invitations: PortfolioInvitation objects for the user email excluding + the current invitation and those with status RETRIEVED. + """ + PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") + UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") + existing_permissions = UserPortfolioPermission.objects.exclude(id=user_portfolio_permission.id).filter( + user=user_portfolio_permission.user + ) + existing_invitations = PortfolioInvitation.objects.filter(email__iexact=user_portfolio_permission.user.email).exclude( + Q(portfolio=user_portfolio_permission.portfolio) + | Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + ) + return (existing_permissions, existing_invitations) + def validate_portfolio_invitation(portfolio_invitation): """ @@ -351,17 +370,14 @@ def validate_portfolio_invitation(portfolio_invitation): ) # == Validate the multiple_porfolios flag. == # - user = User.objects.filter(email=portfolio_invitation.email).first() + user = User.objects.filter(email__iexact=portfolio_invitation.email).first() # If user returns None, then we check for global assignment of multiple_portfolios. # Otherwise we just check on the user. 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_permissions, existing_invitations = ( + get_portfolio_invitation_associations(portfolio_invitation) ) - if existing_permissions.exists(): raise ValidationError( "This user is already assigned to a portfolio. " @@ -376,6 +392,26 @@ def validate_portfolio_invitation(portfolio_invitation): code="has_existing_invitations", ) +def get_portfolio_invitation_associations(portfolio_invitation): + """ + Retrieves the associations for a portfolio invitation. + + Returns: + A tuple: + (existing_permissions, existing_invitations) + where: + - existing_permissions: UserPortfolioPermission objects matching the email. + - existing_invitations: PortfolioInvitation objects for the email excluding + the current invitation and those with status RETRIEVED. + """ + PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") + UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") + existing_permissions = UserPortfolioPermission.objects.filter(user__email__iexact=portfolio_invitation.email) + existing_invitations = PortfolioInvitation.objects.filter(email__iexact=portfolio_invitation.email).exclude( + Q(id=portfolio_invitation.id) | Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + ) + return (existing_permissions, existing_invitations) + def cleanup_after_portfolio_member_deletion(portfolio, email, user=None): """ From beb4e722b377c3297ad5f0d142e4a98597dac07d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 12 Mar 2025 15:44:57 -0600 Subject: [PATCH 3/6] Fix unrelated test failure --- src/registrar/forms/portfolio.py | 8 ++++---- .../models/utility/portfolio_helper.py | 18 ++++++++++-------- src/registrar/tests/test_views_portfolio.py | 8 +++++--- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 2ee174050..db1f58d88 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -460,14 +460,14 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm): if hasattr(e, "code"): field = "email" if "email" in self.fields else None if e.code == "has_existing_permissions": - existing_permissions, existing_invitations = ( - get_portfolio_invitation_associations(self.instance) - ) + existing_permissions, existing_invitations = get_portfolio_invitation_associations(self.instance) same_portfolio_for_permissions = existing_permissions.exclude(portfolio=self.instance.portfolio) same_portfolio_for_invitations = existing_invitations.exclude(portfolio=self.instance.portfolio) if same_portfolio_for_permissions.exists() or same_portfolio_for_invitations.exists(): - self.add_error(field, f"{self.instance.email} is already a member of another .gov organization.") + self.add_error( + field, f"{self.instance.email} is already a member of another .gov organization." + ) override_error = True elif e.code == "has_existing_invitations": self.add_error( diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 707dfcf54..98be6cc87 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -286,8 +286,8 @@ def validate_user_portfolio_permission(user_portfolio_permission): # == Validate the multiple_porfolios flag. == # if not flag_is_active_for_user(user_portfolio_permission.user, "multiple_portfolios"): - existing_permissions, existing_invitations = ( - get_user_portfolio_permission_associations(user_portfolio_permission) + existing_permissions, existing_invitations = get_user_portfolio_permission_associations( + user_portfolio_permission ) if existing_permissions.exists(): raise ValidationError( @@ -303,6 +303,7 @@ def validate_user_portfolio_permission(user_portfolio_permission): code="has_existing_invitations", ) + def get_user_portfolio_permission_associations(user_portfolio_permission): """ Retrieves the associations for a user portfolio invitation. @@ -312,7 +313,7 @@ def get_user_portfolio_permission_associations(user_portfolio_permission): (existing_permissions, existing_invitations) where: - existing_permissions: UserPortfolioPermission objects excluding the current permission. - - existing_invitations: PortfolioInvitation objects for the user email excluding + - existing_invitations: PortfolioInvitation objects for the user email excluding the current invitation and those with status RETRIEVED. """ PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") @@ -320,7 +321,9 @@ def get_user_portfolio_permission_associations(user_portfolio_permission): existing_permissions = UserPortfolioPermission.objects.exclude(id=user_portfolio_permission.id).filter( user=user_portfolio_permission.user ) - existing_invitations = PortfolioInvitation.objects.filter(email__iexact=user_portfolio_permission.user.email).exclude( + existing_invitations = PortfolioInvitation.objects.filter( + email__iexact=user_portfolio_permission.user.email + ).exclude( Q(portfolio=user_portfolio_permission.portfolio) | Q(status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) ) @@ -375,9 +378,7 @@ def validate_portfolio_invitation(portfolio_invitation): # If user returns None, then we check for global assignment of multiple_portfolios. # Otherwise we just check on the user. if not flag_is_active_for_user(user, "multiple_portfolios"): - existing_permissions, existing_invitations = ( - get_portfolio_invitation_associations(portfolio_invitation) - ) + existing_permissions, existing_invitations = get_portfolio_invitation_associations(portfolio_invitation) if existing_permissions.exists(): raise ValidationError( "This user is already assigned to a portfolio. " @@ -392,6 +393,7 @@ def validate_portfolio_invitation(portfolio_invitation): code="has_existing_invitations", ) + def get_portfolio_invitation_associations(portfolio_invitation): """ Retrieves the associations for a portfolio invitation. @@ -401,7 +403,7 @@ def get_portfolio_invitation_associations(portfolio_invitation): (existing_permissions, existing_invitations) where: - existing_permissions: UserPortfolioPermission objects matching the email. - - existing_invitations: PortfolioInvitation objects for the email excluding + - existing_invitations: PortfolioInvitation objects for the email excluding the current invitation and those with status RETRIEVED. """ PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 2065c2d35..bb99e875f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3930,17 +3930,19 @@ class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): response = self.client.post( reverse("new-member"), { - "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, - "domain_request_permission_member": UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, "email": self.user.email, }, + follow=True ) self.assertEqual(response.status_code, 200) + with open("debug_response.html", "w") as f: + f.write(response.content.decode('utf-8')) # Verify messages self.assertContains( response, - f"{self.user.email} is already a member of another .gov organization.", + "User is already a member of this portfolio.", ) # Validate Database has not changed From 98de052c2ecf748b58c54af1c5dd957a80df3063 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 13 Mar 2025 08:30:58 -0600 Subject: [PATCH 4/6] linting --- src/registrar/models/utility/portfolio_helper.py | 4 ---- src/registrar/tests/test_views_portfolio.py | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 98be6cc87..669985725 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -257,9 +257,6 @@ def validate_user_portfolio_permission(user_portfolio_permission): Raises: ValidationError: If any of the validation rules are violated. """ - PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") - UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") - has_portfolio = bool(user_portfolio_permission.portfolio_id) portfolio_permissions = set(user_portfolio_permission._get_portfolio_permissions()) @@ -346,7 +343,6 @@ def validate_portfolio_invitation(portfolio_invitation): Raises: ValidationError: If any of the validation rules are violated. """ - PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") User = get_user_model() diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index bb99e875f..13a7a3bc6 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3933,11 +3933,11 @@ class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, "email": self.user.email, }, - follow=True + follow=True, ) self.assertEqual(response.status_code, 200) with open("debug_response.html", "w") as f: - f.write(response.content.decode('utf-8')) + f.write(response.content.decode("utf-8")) # Verify messages self.assertContains( From 9362de496761fccdd92dde7dca80e3eea5d4d773 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 14 Mar 2025 08:51:03 -0600 Subject: [PATCH 5/6] Fix bug and add unit test --- src/registrar/admin.py | 4 +-- src/registrar/tests/test_views_portfolio.py | 40 +++++++++++++++++++++ src/registrar/views/portfolios.py | 2 +- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 09d0eaa81..28f5abf57 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1846,7 +1846,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): requested_user = get_requested_user(requested_email) permission_exists = UserPortfolioPermission.objects.filter( - user__email=requested_email, portfolio=portfolio, user__email__isnull=False + user__email__iexact=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 @@ -1857,7 +1857,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): is_admin_invitation=is_admin_invitation, ): messages.warning( - self.request, "Could not send email notification to existing organization admins." + request, "Could not send email notification to existing organization admins." ) # if user exists for email, immediately retrieve portfolio invitation upon creation if requested_user is not None: diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 13a7a3bc6..114c066b3 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3952,6 +3952,46 @@ class TestPortfolioInviteNewMemberView(MockEppLib, WebTest): # assert that send_portfolio_invitation_email is not called mock_send_email.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_invitation_email") + def test_member_invite_for_existing_member_uppercase(self, mock_send_email): + """Tests the member invitation flow for existing portfolio member with a different case.""" + 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) + + invite_count_before = PortfolioInvitation.objects.count() + + # Simulate submission of member invite for user who has already been invited + response = self.client.post( + reverse("new-member"), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + "email": self.user.email.upper(), + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + with open("debug_response.html", "w") as f: + f.write(response.content.decode("utf-8")) + + # Verify messages + self.assertContains( + response, + "User is already a member of this portfolio.", + ) + + # Validate Database has not changed + invite_count_after = PortfolioInvitation.objects.count() + self.assertEqual(invite_count_after, invite_count_before) + + # assert that send_portfolio_invitation_email is not called + mock_send_email.assert_not_called() + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index c2ec44b9e..7fa421eaa 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -970,7 +970,7 @@ class PortfolioAddMemberView(DetailView, FormMixin): portfolio = form.cleaned_data["portfolio"] is_admin_invitation = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in form.cleaned_data["roles"] - requested_user = User.objects.filter(email=requested_email).first() + requested_user = User.objects.filter(email__iexact=requested_email).first() permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() try: if not requested_user or not permission_exists: From 127eb38259e478032b2e0de5a36d5a37d3f637ee Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 14 Mar 2025 13:28:05 -0600 Subject: [PATCH 6/6] lint --- src/registrar/admin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 28f5abf57..343624915 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1856,9 +1856,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): portfolio=portfolio, is_admin_invitation=is_admin_invitation, ): - messages.warning( - request, "Could not send email notification to existing organization admins." - ) + messages.warning(request, "Could not send email notification to existing organization admins.") # if user exists for email, immediately retrieve portfolio invitation upon creation if requested_user is not None: obj.retrieve()