From 1c3a53a192e522332d857c0a0566dea24482c233 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 20 Jan 2025 08:05:37 -0500 Subject: [PATCH 1/7] update portfolio members page to not include retrieved invitations --- src/registrar/tests/test_views_members_json.py | 13 ++++++++++++- src/registrar/views/portfolio_members_json.py | 4 +++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_members_json.py b/src/registrar/tests/test_views_members_json.py index 8082a1a30..ceae1e35f 100644 --- a/src/registrar/tests/test_views_members_json.py +++ b/src/registrar/tests/test_views_members_json.py @@ -54,6 +54,7 @@ class GetPortfolioMembersJsonTest(MockEppLib, WebTest): title="Admin", ) self.email6 = "fifth@example.com" + self.email7 = "sixth@example.com" # Create Portfolio self.portfolio = Portfolio.objects.create(creator=self.user, organization_name="Test Portfolio") @@ -302,7 +303,7 @@ class GetPortfolioMembersJsonTest(MockEppLib, WebTest): @override_flag("organization_members", active=True) def test_get_portfolio_invited_json_with_domains(self): """Test that portfolio invited members are returned properly for an authenticated user and the response includes - the domains that the member manages..""" + the domains that the member manages. Test also verifies that retrieved invitations are not included.""" UserPortfolioPermission.objects.create( user=self.user, portfolio=self.portfolio, @@ -319,6 +320,16 @@ class GetPortfolioMembersJsonTest(MockEppLib, WebTest): UserPortfolioPermissionChoices.EDIT_MEMBERS, ], ) + PortfolioInvitation.objects.create( + email=self.email7, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_MEMBERS, + ], + status=PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED, + ) # create a domain in the portfolio domain = Domain.objects.create( diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 11e58e112..a45ad66e9 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -137,7 +137,9 @@ class PortfolioMembersJson(PortfolioMembersPermission, View): ) # PortfolioInvitation query - invitations = PortfolioInvitation.objects.filter(portfolio=portfolio) + invitations = PortfolioInvitation.objects.filter( + portfolio=portfolio, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED + ) invitations = invitations.annotate( first_name=Value(None, output_field=CharField()), last_name=Value(None, output_field=CharField()), From 2c5c465c971c8cd3b88de7c8f65349cbd7c0d080 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 20 Jan 2025 08:22:55 -0500 Subject: [PATCH 2/7] prevent email invite from being sent to existing domain manager --- src/registrar/utility/email_invitations.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 48c796340..0b7b03f79 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -1,6 +1,7 @@ from django.conf import settings from registrar.models import DomainInvitation from registrar.models.domain import Domain +from registrar.models.user_domain_role import UserDomainRole from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, @@ -37,7 +38,7 @@ def send_domain_invitation_email( domains = normalize_domains(domains) requestor_email = get_requestor_email(requestor, domains) - validate_invitation(email, domains, requestor, is_member_of_different_org) + _validate_invitation(email, requested_user, domains, requestor, is_member_of_different_org) send_invitation_email(email, requestor_email, domains, requested_user) @@ -62,12 +63,12 @@ def get_requestor_email(requestor, domains): return requestor.email -def validate_invitation(email, domains, requestor, is_member_of_different_org): +def _validate_invitation(email, user, domains, requestor, is_member_of_different_org): """Validate the invitation conditions.""" check_outside_org_membership(email, requestor, is_member_of_different_org) for domain in domains: - validate_existing_invitation(email, domain) + _validate_existing_invitation(email, user, domain) def check_outside_org_membership(email, requestor, is_member_of_different_org): @@ -80,7 +81,7 @@ def check_outside_org_membership(email, requestor, is_member_of_different_org): raise OutsideOrgMemberError(email=email) -def validate_existing_invitation(email, domain): +def _validate_existing_invitation(email, user, domain): """Check for existing invitations and handle their status.""" try: invite = DomainInvitation.objects.get(email=email, domain=domain) @@ -91,6 +92,9 @@ def validate_existing_invitation(email, domain): invite.save() else: raise AlreadyDomainInvitedError(email) + if user: + if UserDomainRole.objects.exists(user=user, domain=domain): + raise AlreadyDomainManagerError(email) except DomainInvitation.DoesNotExist: pass From decd9f7e7165d3579603f3e89aedb3f88dc6053d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 22 Jan 2025 16:57:17 -0500 Subject: [PATCH 3/7] fixed tests --- src/registrar/tests/test_email_invitations.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 87384d3be..af4d5e53d 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -12,7 +12,7 @@ class DomainInvitationEmail(unittest.TestCase): @less_console_noise_decorator @patch("registrar.utility.email_invitations.send_templated_email") @patch("registrar.utility.email_invitations.UserDomainRole.objects.filter") - @patch("registrar.utility.email_invitations.validate_invitation") + @patch("registrar.utility.email_invitations._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") @@ -77,7 +77,7 @@ class DomainInvitationEmail(unittest.TestCase): @less_console_noise_decorator @patch("registrar.utility.email_invitations.send_templated_email") @patch("registrar.utility.email_invitations.UserDomainRole.objects.filter") - @patch("registrar.utility.email_invitations.validate_invitation") + @patch("registrar.utility.email_invitations._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") @@ -175,7 +175,7 @@ class DomainInvitationEmail(unittest.TestCase): self.assertEqual(mock_send_templated_email.call_count, 2) @less_console_noise_decorator - @patch("registrar.utility.email_invitations.validate_invitation") + @patch("registrar.utility.email_invitations._validate_invitation") def test_send_domain_invitation_email_raises_invite_validation_exception(self, mock_validate_invitation): """Test sending domain invitation email for one domain and assert exception when invite validation fails. @@ -213,7 +213,7 @@ class DomainInvitationEmail(unittest.TestCase): mock_get_requestor_email.assert_called_once() @less_console_noise_decorator - @patch("registrar.utility.email_invitations.validate_invitation") + @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") @@ -263,7 +263,7 @@ class DomainInvitationEmail(unittest.TestCase): @less_console_noise_decorator @patch("registrar.utility.email_invitations.send_emails_to_domain_managers") - @patch("registrar.utility.email_invitations.validate_invitation") + @patch("registrar.utility.email_invitations._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") From 4f9866056838c4129900366b47a8fa856e3389e4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 22 Jan 2025 17:10:57 -0500 Subject: [PATCH 4/7] fixed tests --- src/registrar/tests/test_email_invitations.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index af4d5e53d..b9fef1bf8 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -57,7 +57,7 @@ class DomainInvitationEmail(unittest.TestCase): mock_normalize_domains.assert_called_once_with(mock_domain) mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) mock_validate_invitation.assert_called_once_with( - email, [mock_domain], mock_requestor, is_member_of_different_org + email, None, [mock_domain], mock_requestor, is_member_of_different_org ) mock_send_invitation_email.assert_called_once_with(email, mock_requestor_email, [mock_domain], None) mock_user_domain_role_filter.assert_called_once_with(domain=mock_domain) @@ -136,7 +136,7 @@ class DomainInvitationEmail(unittest.TestCase): 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_validate_invitation.assert_called_once_with( - email, [mock_domain1, mock_domain2], mock_requestor, is_member_of_different_org + email, None, [mock_domain1, mock_domain2], mock_requestor, is_member_of_different_org ) mock_send_invitation_email.assert_called_once_with( email, mock_requestor_email, [mock_domain1, mock_domain2], None @@ -257,7 +257,7 @@ class DomainInvitationEmail(unittest.TestCase): mock_normalize_domains.assert_called_once_with(mock_domain) mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) mock_validate_invitation.assert_called_once_with( - email, [mock_domain], mock_requestor, is_member_of_different_org + email, None, [mock_domain], mock_requestor, is_member_of_different_org ) self.assertEqual(str(context.exception), "Error sending email") @@ -305,7 +305,7 @@ class DomainInvitationEmail(unittest.TestCase): mock_normalize_domains.assert_called_once_with(mock_domain) mock_get_requestor_email.assert_called_once_with(mock_requestor, [mock_domain]) mock_validate_invitation.assert_called_once_with( - email, [mock_domain], mock_requestor, is_member_of_different_org + email, None, [mock_domain], mock_requestor, is_member_of_different_org ) mock_send_invitation_email.assert_called_once_with(email, mock_requestor_email, [mock_domain], None) self.assertEqual(str(context.exception), "Error sending email") From 662e4603db57e3f47d409e3fa041516acf54ff7e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 23 Jan 2025 13:04:46 -0500 Subject: [PATCH 5/7] small fix and removed rogue log info message --- src/registrar/utility/email_invitations.py | 6 +++--- src/registrar/views/utility/permission_views.py | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index c18161590..d589f814f 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -132,11 +132,11 @@ def _validate_existing_invitation(email, user, domain): invite.save() else: raise AlreadyDomainInvitedError(email) - if user: - if UserDomainRole.objects.exists(user=user, domain=domain): - raise AlreadyDomainManagerError(email) except DomainInvitation.DoesNotExist: pass + if user: + if UserDomainRole.objects.filter(user=user, domain=domain).exists(): + raise AlreadyDomainManagerError(email) def send_invitation_email(email, requestor_email, domains, requested_user): diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index a3067d3a2..3234ea701 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -65,7 +65,6 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC): def is_editable(self): """Returns whether domain is editable in the context of the view""" - logger.info("checking if is_editable") domain_editable = self.object.is_editable() if not domain_editable: return False From aae6617f3dedad43040400662fc758e42c2224a0 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 23 Jan 2025 13:56:15 -0500 Subject: [PATCH 6/7] fixed small bug with handling of OutsideOrgMemberError --- src/registrar/utility/email_invitations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index d589f814f..61cc753f0 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -118,7 +118,7 @@ def check_outside_org_membership(email, requestor, is_member_of_different_org): and not flag_is_active_for_user(requestor, "multiple_portfolios") and is_member_of_different_org ): - raise OutsideOrgMemberError(email=email) + raise OutsideOrgMemberError() def _validate_existing_invitation(email, user, domain): From 46d6cab4c7dde714ae8d9d9f873532be7865d8f4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 23 Jan 2025 14:25:50 -0500 Subject: [PATCH 7/7] cleaned up error handling of OutsideOrgMemberError --- src/registrar/utility/email_invitations.py | 2 +- src/registrar/utility/errors.py | 9 +++++++-- src/registrar/views/utility/invitation_helper.py | 10 ++-------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 61cc753f0..d589f814f 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -118,7 +118,7 @@ def check_outside_org_membership(email, requestor, is_member_of_different_org): and not flag_is_active_for_user(requestor, "multiple_portfolios") and is_member_of_different_org ): - raise OutsideOrgMemberError() + raise OutsideOrgMemberError(email=email) def _validate_existing_invitation(email, user, domain): diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index cc18d7269..0a6f00c36 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -59,13 +59,18 @@ class MissingEmailError(InvitationError): super().__init__(message) -class OutsideOrgMemberError(ValueError): +class OutsideOrgMemberError(InvitationError): """ Error raised when an org member tries adding a user from a different .gov org. To be deleted when users can be members of multiple orgs. """ - pass + def __init__(self, email=None): + # Default message if no additional info is provided + message = "Can not invite member of a .gov organization to a different organization." + if email: + message = f"{email} is already a member of another .gov organization." + super().__init__(message) class ActionNotAllowed(Exception): diff --git a/src/registrar/views/utility/invitation_helper.py b/src/registrar/views/utility/invitation_helper.py index 5c730d0c3..98c36b308 100644 --- a/src/registrar/views/utility/invitation_helper.py +++ b/src/registrar/views/utility/invitation_helper.py @@ -67,14 +67,8 @@ def handle_invitation_exceptions(request, exception, email): messages.error(request, str(exception)) logger.error(str(exception), exc_info=True) elif isinstance(exception, OutsideOrgMemberError): - logger.warning( - "Could not send email. Can not invite member of a .gov organization to a different organization.", - exc_info=True, - ) - messages.error( - request, - f"{email} is already a member of another .gov organization.", - ) + messages.error(request, str(exception)) + logger.warning(str(exception), exc_info=True) elif isinstance(exception, AlreadyDomainManagerError): messages.warning(request, str(exception)) elif isinstance(exception, AlreadyDomainInvitedError):