From 7bca3880f451c637d14b300df5074482fecc3ce2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 23 Jan 2025 13:48:35 -0500 Subject: [PATCH 01/17] 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 02/17] 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 9c61d9f23973317191d03ac81f80d5d5720a2820 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Tue, 28 Jan 2025 18:50:47 -0700 Subject: [PATCH 03/17] Fixed tooltip sizing (needed to re-apply css. For some reason old classes were stripped out of the code) --- src/registrar/assets/src/sass/_theme/_tooltips.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/src/sass/_theme/_tooltips.scss b/src/registrar/assets/src/sass/_theme/_tooltips.scss index 22b5cf534..e1e31cbec 100644 --- a/src/registrar/assets/src/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/src/sass/_theme/_tooltips.scss @@ -29,7 +29,7 @@ font-weight: 400 !important; } -.domains__table { +.domains__table, .usa-table { /* Trick tooltips in the domains table to do 2 things... 1 - Shrink itself to a padded viewport window From 6634e9bad8166352632a26613d94577b5a336d7b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 15:02:46 -0500 Subject: [PATCH 04/17] handling of failed email sends to domain managers --- src/registrar/admin.py | 7 +++--- src/registrar/utility/email_invitations.py | 23 +++++++++++++------ src/registrar/views/domain.py | 10 ++++---- src/registrar/views/portfolios.py | 10 ++++---- .../views/utility/invitation_helper.py | 8 +++---- 5 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8ecf36f52..f4e888713 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1410,7 +1410,7 @@ class BaseInvitationAdmin(ListHeaderAdmin): # store current messages from request so that they are preserved throughout the method storage = get_messages(request) # Check if there are any error or warning messages in the `messages` framework - has_errors = any(message.level_tag in ["error", "warning"] for message in storage) + has_errors = any(message.level_tag in ["error"] for message in storage) if has_errors: # Re-render the change form if there are errors or warnings @@ -1552,13 +1552,14 @@ class DomainInvitationAdmin(BaseInvitationAdmin): portfolio_invitation.save() messages.success(request, f"{requested_email} has been invited to the organization: {domain_org}") - send_domain_invitation_email( + if not send_domain_invitation_email( email=requested_email, requestor=requestor, domains=domain, is_member_of_different_org=member_of_a_different_org, requested_user=requested_user, - ) + ): + messages.warning(request, "Could not send email confirmation to existing domain managers.") if requested_user is not None: # Domain Invitation creation for an existing User obj.retrieve() diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index d589f814f..8b46589ba 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -27,6 +27,9 @@ def send_domain_invitation_email( is_member_of_different_org (bool): if an email belongs to a different org requested_user (User | None): The recipient if the email belongs to a user in the registrar + Returns: + Boolean indicating if all messages were sent successfully. + Raises: MissingEmailError: If the requestor has no email associated with their account. AlreadyDomainManagerError: If the email corresponds to an existing domain manager. @@ -41,22 +44,28 @@ def send_domain_invitation_email( send_invitation_email(email, requestor_email, domains, requested_user) + all_manager_emails_sent = True # send emails to domain managers for domain in domains: - send_emails_to_domain_managers( + if not send_emails_to_domain_managers( email=email, requestor_email=requestor_email, domain=domain, requested_user=requested_user, - ) + ): + all_manager_emails_sent = False + + return all_manager_emails_sent def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, requested_user=None): """ Notifies all domain managers of the provided domain of a change - Raises: - EmailSendingError + + Returns: + Boolean indicating if all messages were sent successfully. """ + all_emails_sent = True # Get each domain manager from list user_domain_roles = UserDomainRole.objects.filter(domain=domain) for user_domain_role in user_domain_roles: @@ -76,9 +85,9 @@ def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, }, ) except EmailSendingError as err: - raise EmailSendingError( - f"Could not send email manager notification to {user.email} for domain: {domain.name}" - ) from err + logger.warning(f"Could not send email manager notification to {user.email} for domain: {domain.name}") + all_emails_sent = False + return all_emails_sent def normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 5cac3c667..d7e157b25 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1244,24 +1244,26 @@ class DomainAddUserView(DomainFormBaseView): def _handle_new_user_invitation(self, email, requestor, member_of_different_org): """Handle invitation for a new user who does not exist in the system.""" - send_domain_invitation_email( + if not send_domain_invitation_email( email=email, requestor=requestor, domains=self.object, is_member_of_different_org=member_of_different_org, - ) + ): + messages.warning(self.request, "Could not send email confirmation to existing domain managers.") DomainInvitation.objects.get_or_create(email=email, domain=self.object) messages.success(self.request, f"{email} has been invited to the domain: {self.object}") def _handle_existing_user(self, email, requestor, requested_user, member_of_different_org): """Handle adding an existing user to the domain.""" - send_domain_invitation_email( + if not send_domain_invitation_email( email=email, requestor=requestor, domains=self.object, is_member_of_different_org=member_of_different_org, requested_user=requested_user, - ) + ): + messages.warning(self.request, "Could not send email confirmation to existing domain managers.") UserDomainRole.objects.create( user=requested_user, domain=self.object, diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 5bc16a396..1f4a99e9f 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -299,13 +299,14 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V # get added_domains from ids to pass to send email method and bulk create added_domains = Domain.objects.filter(id__in=added_domain_ids) member_of_a_different_org, _ = get_org_membership(portfolio, member.email, member) - send_domain_invitation_email( + if not send_domain_invitation_email( email=member.email, requestor=requestor, domains=added_domains, is_member_of_different_org=member_of_a_different_org, requested_user=member, - ) + ): + messages.warning(self.request, "Could not send email confirmation to existing domain managers.") # Bulk create UserDomainRole instances for added domains UserDomainRole.objects.bulk_create( [ @@ -517,12 +518,13 @@ class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermission # get added_domains from ids to pass to send email method and bulk create added_domains = Domain.objects.filter(id__in=added_domain_ids) member_of_a_different_org, _ = get_org_membership(portfolio, email, None) - send_domain_invitation_email( + if not send_domain_invitation_email( email=email, requestor=requestor, domains=added_domains, is_member_of_different_org=member_of_a_different_org, - ) + ): + messages.warning(self.request, "Could not send email confirmation to existing domain managers.") # Update existing invitations from CANCELED to INVITED existing_invitations = DomainInvitation.objects.filter(domain__in=added_domains, email=email) diff --git a/src/registrar/views/utility/invitation_helper.py b/src/registrar/views/utility/invitation_helper.py index 98c36b308..68e87294f 100644 --- a/src/registrar/views/utility/invitation_helper.py +++ b/src/registrar/views/utility/invitation_helper.py @@ -70,11 +70,11 @@ def handle_invitation_exceptions(request, exception, email): messages.error(request, str(exception)) logger.warning(str(exception), exc_info=True) elif isinstance(exception, AlreadyDomainManagerError): - messages.warning(request, str(exception)) + messages.error(request, str(exception)) elif isinstance(exception, AlreadyDomainInvitedError): - messages.warning(request, str(exception)) + messages.error(request, str(exception)) elif isinstance(exception, IntegrityError): - messages.warning(request, f"{email} is already a manager for this domain") + messages.error(request, f"{email} is already a manager for this domain") else: logger.warning("Could not send email invitation (Other Exception)", exc_info=True) - messages.warning(request, "Could not send email invitation.") + messages.error(request, "Could not send email invitation.") From dfb7c6d87558dbe2c9631dde802acfb6b199a116 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 16:00:02 -0500 Subject: [PATCH 05/17] more updates and fixes to invitation logging and exception handling --- src/registrar/utility/email_invitations.py | 2 +- src/registrar/views/portfolios.py | 4 ++-- src/registrar/views/utility/invitation_helper.py | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 8b46589ba..ad64ab23a 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -85,7 +85,7 @@ def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, }, ) except EmailSendingError as err: - logger.warning(f"Could not send email manager notification to {user.email} for domain: {domain.name}") + logger.warning(f"Could not send email manager notification to {user.email} for domain: {domain.name}", exc_info=True) all_emails_sent = False return all_emails_sent diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 1f4a99e9f..8ea135cfd 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -801,7 +801,7 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): portfolio, exc_info=True, ) - messages.warning(self.request, "Could not send email invitation.") + messages.warning(self.request, "Could not send portfolio email invitation.") elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) logger.error( @@ -810,4 +810,4 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): ) else: logger.warning("Could not send email invitation (Other Exception)", exc_info=True) - messages.warning(self.request, "Could not send email invitation.") + messages.warning(self.request, "Could not send portfolio email invitation.") diff --git a/src/registrar/views/utility/invitation_helper.py b/src/registrar/views/utility/invitation_helper.py index 68e87294f..7e6e53dba 100644 --- a/src/registrar/views/utility/invitation_helper.py +++ b/src/registrar/views/utility/invitation_helper.py @@ -68,7 +68,6 @@ def handle_invitation_exceptions(request, exception, email): logger.error(str(exception), exc_info=True) elif isinstance(exception, OutsideOrgMemberError): messages.error(request, str(exception)) - logger.warning(str(exception), exc_info=True) elif isinstance(exception, AlreadyDomainManagerError): messages.error(request, str(exception)) elif isinstance(exception, AlreadyDomainInvitedError): From fbab137d0255e142b711fde06325bec081b1d88f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 16:49:13 -0500 Subject: [PATCH 06/17] added tests for sending domain manager emails --- src/registrar/tests/test_admin.py | 2 +- src/registrar/tests/test_email_invitations.py | 182 ++++++++++++++++-- 2 files changed, 172 insertions(+), 12 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 036e35a50..387319663 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1427,7 +1427,7 @@ class TestPortfolioInvitationAdmin(TestCase): @less_console_noise_decorator @patch("registrar.admin.send_portfolio_invitation_email") - @patch("django.contrib.messages.warning") # Mock the `messages.error` call + @patch("django.contrib.messages.error") # Mock the `messages.error` call def test_save_exception_generic_error(self, mock_messages_error, mock_send_email): """Handle generic exceptions correctly during portfolio invitation.""" self.client.force_login(self.superuser) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index b9fef1bf8..cffebbe37 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -1,8 +1,11 @@ import unittest from unittest.mock import patch, MagicMock from datetime import date +from registrar.models.domain import Domain +from registrar.models.user import User +from registrar.models.user_domain_role import UserDomainRole from registrar.utility.email import EmailSendingError -from registrar.utility.email_invitations import send_domain_invitation_email +from registrar.utility.email_invitations import send_domain_invitation_email, send_emails_to_domain_managers from api.tests.common import less_console_noise_decorator @@ -290,16 +293,16 @@ class DomainInvitationEmail(unittest.TestCase): email = "invitee@example.com" is_member_of_different_org = False - mock_send_domain_manager_emails.side_effect = EmailSendingError("Error sending email") + # Change the return value to False for mock_send_domain_manager_emails + mock_send_domain_manager_emails.return_value = False - # Call and assert exception - with self.assertRaises(EmailSendingError) as context: - send_domain_invitation_email( - email=email, - requestor=mock_requestor, - domains=mock_domain, - is_member_of_different_org=is_member_of_different_org, - ) + # Call and assert that send_domain_invitation_email returns False + result = send_domain_invitation_email( + email=email, + requestor=mock_requestor, + domains=mock_domain, + is_member_of_different_org=is_member_of_different_org, + ) # Assertions mock_normalize_domains.assert_called_once_with(mock_domain) @@ -308,4 +311,161 @@ class DomainInvitationEmail(unittest.TestCase): 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") + + # Assert that the result is False + self.assertFalse(result) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.models.UserDomainRole.objects.filter") + def test_send_emails_to_domain_managers_all_emails_sent_successfully(self, mock_filter, mock_send_templated_email): + """Test when all emails are sent successfully.""" + + # Setup mocks + mock_domain = MagicMock(spec=Domain) + mock_requestor_email = "requestor@example.com" + mock_email = "invitee@example.com" + + # Create mock user and UserDomainRole + mock_user = MagicMock(spec=User) + mock_user.email = "manager@example.com" + mock_user_domain_role = MagicMock(spec=UserDomainRole, user=mock_user) + + # Mock the filter method to return a list of mock UserDomainRole objects + mock_filter.return_value = [mock_user_domain_role] + + # Mock successful email sending + mock_send_templated_email.return_value = None # No exception means success + + # Call function + result = send_emails_to_domain_managers(mock_email, mock_requestor_email, mock_domain) + + # Assertions + self.assertTrue(result) # All emails should be successfully sent + mock_send_templated_email.assert_called_once_with( + "emails/domain_manager_notification.txt", + "emails/domain_manager_notification_subject.txt", + to_address="manager@example.com", + context={ + "domain": mock_domain, + "requestor_email": mock_requestor_email, + "invited_email_address": mock_email, + "domain_manager": mock_user, + "date": date.today(), + }, + ) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.models.UserDomainRole.objects.filter") + def test_send_emails_to_domain_managers_email_send_fails(self, mock_filter, mock_send_templated_email): + """Test when sending an email fails (raises EmailSendingError).""" + + # Setup mocks + mock_domain = MagicMock(spec=Domain) + mock_requestor_email = "requestor@example.com" + mock_email = "invitee@example.com" + + # Create mock user and UserDomainRole + mock_user = MagicMock(spec=User) + mock_user.email = "manager@example.com" + mock_user_domain_role = MagicMock(spec=UserDomainRole, user=mock_user) + + # Mock the filter method to return a list of mock UserDomainRole objects + mock_filter.return_value = [mock_user_domain_role] + + # Mock sending email to raise an EmailSendingError + mock_send_templated_email.side_effect = EmailSendingError("Email sending failed") + + # Call function + result = send_emails_to_domain_managers(mock_email, mock_requestor_email, mock_domain) + + # Assertions + self.assertFalse(result) # The result should be False as email sending failed + mock_send_templated_email.assert_called_once_with( + "emails/domain_manager_notification.txt", + "emails/domain_manager_notification_subject.txt", + to_address="manager@example.com", + context={ + "domain": mock_domain, + "requestor_email": mock_requestor_email, + "invited_email_address": mock_email, + "domain_manager": mock_user, + "date": date.today(), + }, + ) + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.models.UserDomainRole.objects.filter") + def test_send_emails_to_domain_managers_no_domain_managers(self, mock_filter, mock_send_templated_email): + """Test when there are no domain managers.""" + + # Setup mocks + mock_domain = MagicMock(spec=Domain) + mock_requestor_email = "requestor@example.com" + mock_email = "invitee@example.com" + + # Mock no domain managers (empty UserDomainRole queryset) + mock_filter.return_value = [] + + # Call function + result = send_emails_to_domain_managers(mock_email, mock_requestor_email, mock_domain) + + # Assertions + self.assertTrue(result) # No emails to send, so it should return True + mock_send_templated_email.assert_not_called() # No emails should be sent + + @less_console_noise_decorator + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.models.UserDomainRole.objects.filter") + def test_send_emails_to_domain_managers_some_emails_fail(self, mock_filter, mock_send_templated_email): + """Test when some emails fail to send.""" + + # Setup mocks + mock_domain = MagicMock(spec=Domain) + mock_requestor_email = "requestor@example.com" + mock_email = "invitee@example.com" + + # Create mock users and UserDomainRoles + mock_user_1 = MagicMock(spec=User) + mock_user_1.email = "manager1@example.com" + mock_user_2 = MagicMock(spec=User) + mock_user_2.email = "manager2@example.com" + + mock_user_domain_role_1 = MagicMock(spec=UserDomainRole, user=mock_user_1) + mock_user_domain_role_2 = MagicMock(spec=UserDomainRole, user=mock_user_2) + mock_filter.return_value = [mock_user_domain_role_1, mock_user_domain_role_2] + + # Mock first email success and second email failure + mock_send_templated_email.side_effect = [None, EmailSendingError("Failed to send email")] + + # Call function + result = send_emails_to_domain_managers(mock_email, mock_requestor_email, mock_domain) + + # Assertions + self.assertFalse(result) # One email failed, so result should be False + mock_send_templated_email.assert_any_call( + "emails/domain_manager_notification.txt", + "emails/domain_manager_notification_subject.txt", + to_address="manager1@example.com", + context={ + "domain": mock_domain, + "requestor_email": mock_requestor_email, + "invited_email_address": mock_email, + "domain_manager": mock_user_1, + "date": date.today(), + }, + ) + mock_send_templated_email.assert_any_call( + "emails/domain_manager_notification.txt", + "emails/domain_manager_notification_subject.txt", + to_address="manager2@example.com", + context={ + "domain": mock_domain, + "requestor_email": mock_requestor_email, + "invited_email_address": mock_email, + "domain_manager": mock_user_2, + "date": date.today(), + }, + ) From 79636aad14e4b94e31b2fb6a6eb32e9c47f18543 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 17:09:06 -0500 Subject: [PATCH 07/17] added domain add manager email send failure test --- src/registrar/tests/test_views_domain.py | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 45758e502..17eef71b4 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -885,6 +885,40 @@ class TestDomainManagers(TestDomainOverview): success_page = success_result.follow() self.assertContains(success_page, "notauser@igorville.gov") + @override_flag("organization_feature", active=True) + @less_console_noise_decorator + @patch("registrar.views.domain.send_portfolio_invitation_email") + @patch("registrar.views.domain.send_domain_invitation_email") + def test_domain_user_add_form_fails_to_send_to_some_managers( + self, mock_send_domain_email, mock_send_portfolio_email + ): + """Adding an email not associated with a user works and sends portfolio invitation, + and when domain managers email(s) fail to send, assert proper warning displayed.""" + add_page = self.app.get(reverse("domain-users-add", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + add_page.form["email"] = "notauser@igorville.gov" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + + mock_send_domain_email.return_value = False + + success_result = add_page.form.submit() + + self.assertEqual(success_result.status_code, 302) + self.assertEqual( + success_result["Location"], + reverse("domain-users", kwargs={"pk": self.domain.id}), + ) + + # Verify that the invitation emails were sent + mock_send_portfolio_email.assert_called_once() + mock_send_domain_email.assert_called_once() + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + success_page = success_result.follow() + self.assertContains(success_page, "Could not send email confirmation to existing domain managers.") + @boto3_mocking.patching @override_flag("organization_feature", active=True) @less_console_noise_decorator From f00a4c837c9ab9c9e982c311376e7f3ada657ff6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 17:23:08 -0500 Subject: [PATCH 08/17] lint --- src/registrar/tests/test_email_invitations.py | 10 +++++----- src/registrar/utility/email_invitations.py | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index cffebbe37..1377dec42 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -311,7 +311,7 @@ class DomainInvitationEmail(unittest.TestCase): 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) - + # Assert that the result is False self.assertFalse(result) @@ -320,7 +320,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.models.UserDomainRole.objects.filter") def test_send_emails_to_domain_managers_all_emails_sent_successfully(self, mock_filter, mock_send_templated_email): """Test when all emails are sent successfully.""" - + # Setup mocks mock_domain = MagicMock(spec=Domain) mock_requestor_email = "requestor@example.com" @@ -360,7 +360,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.models.UserDomainRole.objects.filter") def test_send_emails_to_domain_managers_email_send_fails(self, mock_filter, mock_send_templated_email): """Test when sending an email fails (raises EmailSendingError).""" - + # Setup mocks mock_domain = MagicMock(spec=Domain) mock_requestor_email = "requestor@example.com" @@ -400,7 +400,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.models.UserDomainRole.objects.filter") def test_send_emails_to_domain_managers_no_domain_managers(self, mock_filter, mock_send_templated_email): """Test when there are no domain managers.""" - + # Setup mocks mock_domain = MagicMock(spec=Domain) mock_requestor_email = "requestor@example.com" @@ -421,7 +421,7 @@ class DomainInvitationEmail(unittest.TestCase): @patch("registrar.models.UserDomainRole.objects.filter") def test_send_emails_to_domain_managers_some_emails_fail(self, mock_filter, mock_send_templated_email): """Test when some emails fail to send.""" - + # Setup mocks mock_domain = MagicMock(spec=Domain) mock_requestor_email = "requestor@example.com" diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index ad64ab23a..f9c3b89b2 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -84,8 +84,10 @@ def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, "date": date.today(), }, ) - except EmailSendingError as err: - logger.warning(f"Could not send email manager notification to {user.email} for domain: {domain.name}", exc_info=True) + except EmailSendingError: + logger.warning( + f"Could not send email manager notification to {user.email} for domain: {domain.name}", exc_info=True + ) all_emails_sent = False return all_emails_sent From 3fa9e8ec6118e65c52756af11636798764588dcb Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 29 Jan 2025 17:36:37 -0500 Subject: [PATCH 09/17] fixed broken tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index a42283a7d..a0a8fab5d 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3262,7 +3262,7 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): # 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 email invitation.") + mock_warning.assert_called_once_with(response.wsgi_request, "Could not send portfolio email invitation.") # assert that portfolio invitation is not created self.assertFalse( PortfolioInvitation.objects.filter(email=self.new_member_email, portfolio=self.portfolio).exists(), @@ -3343,7 +3343,7 @@ class TestPortfolioInviteNewMemberView(TestWithUser, WebTest): # 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 email invitation.") + mock_warning.assert_called_once_with(response.wsgi_request, "Could not send portfolio email invitation.") # assert that portfolio invitation is not created self.assertFalse( PortfolioInvitation.objects.filter(email=self.new_member_email, portfolio=self.portfolio).exists(), From 5e89d4f652bfa3c35d39e648e2279e490d723068 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 30 Jan 2025 14:19:42 -0500 Subject: [PATCH 10/17] cleaned up comment --- src/registrar/admin.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f4e888713..7dbe7abb0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1407,9 +1407,12 @@ class BaseInvitationAdmin(ListHeaderAdmin): Normal flow on successful save_model on add is to redirect to changelist_view. If there are errors, flow is modified to instead render change form. """ - # store current messages from request so that they are preserved throughout the method + # store current messages from request in storage so that they are preserved throughout the + # method, as some flows remove and replace all messages, and so we store here to retrieve + # later storage = get_messages(request) - # Check if there are any error or warning messages in the `messages` framework + # Check if there are any error messages in the `messages` framework + # error messages stop the workflow; other message levels allow flow to continue as normal has_errors = any(message.level_tag in ["error"] for message in storage) if has_errors: From 3192107205e87f97120965a437708afabd1ec2bc Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 30 Jan 2025 17:22:14 -0500 Subject: [PATCH 11/17] Use method_decorator and mixins on report views --- src/registrar/views/report_views.py | 21 ++++++++++--- src/registrar/views/utility/mixins.py | 44 +++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index 694d1e205..ee2c079f3 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -5,17 +5,20 @@ from django.views import View from django.shortcuts import render from django.contrib import admin from django.db.models import Avg, F + +from registrar.views.utility.mixins import DomainAndRequestsReportsPermission, PortfolioReportsPermission from .. import models import datetime from django.utils import timezone - +from django.contrib.admin.views.decorators import staff_member_required +from django.utils.decorators import method_decorator from registrar.utility import csv_export - import logging logger = logging.getLogger(__name__) +@method_decorator(staff_member_required, name="dispatch") class AnalyticsView(View): def get(self, request): thirty_days_ago = datetime.datetime.today() - datetime.timedelta(days=30) @@ -149,6 +152,7 @@ class AnalyticsView(View): return render(request, "admin/analytics.html", context) +@method_decorator(staff_member_required, name="dispatch") class ExportDataType(View): def get(self, request, *args, **kwargs): # match the CSV example with all the fields @@ -158,7 +162,7 @@ class ExportDataType(View): return response -class ExportDataTypeUser(View): +class ExportDataTypeUser(DomainAndRequestsReportsPermission, View): """Returns a domain report for a given user on the request""" def get(self, request, *args, **kwargs): @@ -169,7 +173,7 @@ class ExportDataTypeUser(View): return response -class ExportMembersPortfolio(View): +class ExportMembersPortfolio(PortfolioReportsPermission, View): """Returns a members report for a given portfolio""" def get(self, request, *args, **kwargs): @@ -197,7 +201,7 @@ class ExportMembersPortfolio(View): return response -class ExportDataTypeRequests(View): +class ExportDataTypeRequests(DomainAndRequestsReportsPermission, View): """Returns a domain requests report for a given user on the request""" def get(self, request, *args, **kwargs): @@ -208,6 +212,7 @@ class ExportDataTypeRequests(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): def get(self, request, *args, **kwargs): # Smaller export based on 1 @@ -217,6 +222,7 @@ class ExportDataFull(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataFederal(View): def get(self, request, *args, **kwargs): # Federal only @@ -226,6 +232,7 @@ class ExportDataFederal(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDomainRequestDataFull(View): """Generates a downloaded report containing all Domain Requests (except started)""" @@ -237,6 +244,7 @@ class ExportDomainRequestDataFull(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataDomainsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -249,6 +257,7 @@ class ExportDataDomainsGrowth(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataRequestsGrowth(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -261,6 +270,7 @@ class ExportDataRequestsGrowth(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataManagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") @@ -272,6 +282,7 @@ class ExportDataManagedDomains(View): return response +@method_decorator(staff_member_required, name="dispatch") class ExportDataUnmanagedDomains(View): def get(self, request, *args, **kwargs): start_date = request.GET.get("start_date", "") diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 236ef8696..e9e03274f 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -153,6 +153,50 @@ class PermissionsLoginMixin(PermissionRequiredMixin): return super().handle_no_permission() +class DomainAndRequestsReportsPermission(PermissionsLoginMixin): + """Permission mixin for domain and requests csv downloads""" + + def has_permission(self): + """Check if this user has access to this domain. + + The user is in self.request.user and the domain needs to be looked + up from the domain's primary key in self.kwargs["pk"] + """ + + if not self.request.user.is_authenticated: + return False + + if self.request.user.is_restricted(): + return False + + return True + + +class PortfolioReportsPermission(PermissionsLoginMixin): + """Permission mixin for portfolio csv downloads""" + + def has_permission(self): + """Check if this user has access to this domain. + + The user is in self.request.user and the domain needs to be looked + up from the domain's primary key in self.kwargs["pk"] + """ + + if not self.request.user.is_authenticated: + return False + + if self.request.user.is_restricted(): + return False + + portfolio = self.request.session.get("portfolio") + if not self.request.user.has_view_members_portfolio_permission( + portfolio + ) and not self.request.user.has_edit_members_portfolio_permission(portfolio): + return False + + return self.request.user.is_org_user(self.request) + + class DomainPermission(PermissionsLoginMixin): """Permission mixin that redirects to domain if user has access, otherwise 403""" From 6054b72d72bcdaca8423fc64deed1f245f248a81 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 30 Jan 2025 17:29:11 -0500 Subject: [PATCH 12/17] remove check against portfolio edit permissions from portfolio report perm mixin --- src/registrar/views/utility/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index e9e03274f..2d121849e 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -191,7 +191,7 @@ class PortfolioReportsPermission(PermissionsLoginMixin): portfolio = self.request.session.get("portfolio") if not self.request.user.has_view_members_portfolio_permission( portfolio - ) and not self.request.user.has_edit_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 13/17] 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 8de354f31e76cd7047fbe6e35e59d526e0c3521f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 31 Jan 2025 16:07:41 -0500 Subject: [PATCH 14/17] 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 001cb2273b53291852cb4b8f218b77ab1f4852e3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 1 Feb 2025 05:50:03 -0500 Subject: [PATCH 15/17] 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 a80ab45401b5d0fef6ad9630931de985879ba233 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 3 Feb 2025 18:22:51 -0500 Subject: [PATCH 16/17] 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 17/17] 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"))