diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index d244f9931..a81f9d1e7 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -337,21 +337,21 @@ class BasePortfolioMemberForm(forms.ModelForm): UserPortfolioRoleChoices.ORGANIZATION_ADMIN in previous_roles and UserPortfolioRoleChoices.ORGANIZATION_ADMIN not in new_roles ) - + def is_change(self) -> bool: """ - Determines if the form has changed by comparing the initial data + Determines if the form has changed by comparing the initial data with the submitted cleaned data. - + Returns: bool: True if the form has changed, False otherwise. """ # Compare role values - previous_roles = set(self.initial.get("roles", [])) + previous_roles = set(self.initial.get("roles", [])) new_roles = set(self.cleaned_data.get("roles", [])) # Compare additional permissions values - previous_permissions = set(self.initial.get("additional_permissions", [])) + previous_permissions = set(self.initial.get("additional_permissions", [])) new_permissions = set(self.cleaned_data.get("additional_permissions", [])) return previous_roles != new_roles or previous_permissions != new_permissions diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index dd80f946e..045cf2de4 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -101,41 +101,41 @@ class PortfolioInvitation(TimeStampedModel): str: The display name of the user's role. """ return get_role_display(self.roles) - + @property def domains_display(self): """ Returns a string representation of the user's domain access level. - Uses the `get_domains_display` function to determine whether the user has + Uses the `get_domains_display` function to determine whether the user has "Viewer, all" access (can view all domains) or "Viewer, limited" access. Returns: str: The display name of the user's domain permissions. """ return get_domains_display(self.roles, self.additional_permissions) - + @property def domain_requests_display(self): """ Returns a string representation of the user's access to domain requests. - Uses the `get_domain_requests_display` function to determine if the user - is a "Creator" (can create and edit requests), a "Viewer" (can only view requests), + Uses the `get_domain_requests_display` function to determine if the user + is a "Creator" (can create and edit requests), a "Viewer" (can only view requests), or has "No access" to domain requests. Returns: str: The display name of the user's domain request permissions. """ return get_domain_requests_display(self.roles, self.additional_permissions) - + @property def members_display(self): """ Returns a string representation of the user's access to managing members. - Uses the `get_members_display` function to determine if the user is a - "Manager" (can edit members), a "Viewer" (can view members), or has "No access" + Uses the `get_members_display` function to determine if the user is a + "Manager" (can edit members), a "Viewer" (can view members), or has "No access" to member management. Returns: diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 372715db2..7daa3bd11 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -201,41 +201,41 @@ class UserPortfolioPermission(TimeStampedModel): str: The display name of the user's role. """ return get_role_display(self.roles) - + @property def domains_display(self): """ Returns a string representation of the user's domain access level. - Uses the `get_domains_display` function to determine whether the user has + Uses the `get_domains_display` function to determine whether the user has "Viewer, all" access (can view all domains) or "Viewer, limited" access. Returns: str: The display name of the user's domain permissions. """ return get_domains_display(self.roles, self.additional_permissions) - + @property def domain_requests_display(self): """ Returns a string representation of the user's access to domain requests. - Uses the `get_domain_requests_display` function to determine if the user - is a "Creator" (can create and edit requests), a "Viewer" (can only view requests), + Uses the `get_domain_requests_display` function to determine if the user + is a "Creator" (can create and edit requests), a "Viewer" (can only view requests), or has "No access" to domain requests. Returns: str: The display name of the user's domain request permissions. """ return get_domain_requests_display(self.roles, self.additional_permissions) - + @property def members_display(self): """ Returns a string representation of the user's access to managing members. - Uses the `get_members_display` function to determine if the user is a - "Manager" (can edit members), a "Viewer" (can view members), or has "No access" + Uses the `get_members_display` function to determine if the user is a + "Manager" (can edit members), a "Viewer" (can view members), or has "No access" to member management. Returns: diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index f1f9ef4f2..67e546db8 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -82,6 +82,7 @@ class MemberPermissionDisplay(StrEnum): VIEWER = "Viewer" NONE = "None" + def get_role_display(roles): """ Returns a user-friendly display name for a given list of user roles. @@ -103,6 +104,7 @@ def get_role_display(roles): else: return "-" + def get_domains_display(roles, permissions): """ Determines the display name for a user's domain viewing permissions. @@ -124,6 +126,7 @@ def get_domains_display(roles, permissions): else: return "Viewer, limited" + def get_domain_requests_display(roles, permissions): """ Determines the display name for a user's domain request permissions. @@ -148,6 +151,7 @@ def get_domain_requests_display(roles, permissions): else: return "No access" + def get_members_display(roles, permissions): """ Determines the display name for a user's member management permissions. @@ -172,6 +176,7 @@ def get_members_display(roles, permissions): else: return "No access" + def validate_user_portfolio_permission(user_portfolio_permission): """ Validates a UserPortfolioPermission instance. Located in portfolio_helper to avoid circular imports diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 77a8c402f..20ac4a565 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -16,6 +16,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_member_permission_update_email, ) from api.tests.common import less_console_noise_decorator @@ -522,7 +523,6 @@ class PortfolioInvitationEmailTests(unittest.TestCase): "registrar.utility.email_invitations._get_requestor_email", side_effect=MissingEmailError("Requestor has no email"), ) - @less_console_noise_decorator def test_send_portfolio_invitation_email_missing_requestor_email(self, mock_get_email): """Test when requestor has no email""" is_admin_invitation = False @@ -888,3 +888,77 @@ class SendPortfolioAdminRemovalEmailsTests(unittest.TestCase): mock_get_requestor_email.assert_called_once_with(self.requestor, portfolio=self.portfolio) mock_send_removal_emails.assert_called_once_with(self.email, self.requestor.email, self.portfolio) self.assertFalse(result) + + +class TestSendPortfolioMemberPermissionUpdateEmail(unittest.TestCase): + """Unit tests for send_portfolio_member_permission_update_email function.""" + + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations._get_requestor_email") + def test_send_email_success(self, mock_get_requestor_email, mock_send_email): + """Test that the email is sent successfully when there are no errors.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_update_email(requestor, permissions) + + # Assertions + mock_get_requestor_email.assert_called_once_with(requestor, portfolio=permissions.portfolio) + mock_send_email.assert_called_once_with( + "emails/portfolio_update.txt", + "emails/portfolio_update_subject.txt", + to_address="user@example.com", + context={ + "requested_user": permissions.user, + "portfolio": permissions.portfolio, + "requestor_email": "requestor@example.com", + "permissions": permissions, + }, + ) + self.assertTrue(result) + + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError("Email failed")) + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations.logger") + def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_send_email): + """Test that the function returns False and logs an error when email sending fails.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_update_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_called_once_with( + "Could not send email organization member update notification to %s for portfolio: %s", + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) + self.assertFalse(result) + + @patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error")) + @patch("registrar.utility.email_invitations.logger") + def test_requestor_email_retrieval_failure(self, mock_logger, mock_get_requestor_email): + """Test that an exception in retrieving requestor email is logged.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + + # Call function + with self.assertRaises(Exception): + send_portfolio_member_permission_update_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_not_called() # Function should fail before logging email failure diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index ed057a41b..d206bf279 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -225,6 +225,7 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i ) return all_admin_emails_sent + def send_portfolio_member_permission_update_email(requestor, permissions: UserPortfolioPermission): """ Sends an email notification to a portfolio member when their permissions are updated. @@ -234,7 +235,7 @@ def send_portfolio_member_permission_update_email(requestor, permissions: UserPo Args: requestor (User): The user initiating the permission update. - permissions (UserPortfolioPermission): The updated permissions object containing the affected user + permissions (UserPortfolioPermission): The updated permissions object containing the affected user and the portfolio details. Returns: @@ -254,18 +255,19 @@ def send_portfolio_member_permission_update_email(requestor, permissions: UserPo "portfolio": permissions.portfolio, "requestor_email": requestor_email, "permissions": permissions, - } + }, ) - except EmailSendingError as err: + except EmailSendingError: logger.warning( "Could not send email organization member update notification to %s " "for portfolio: %s", - permissions.user.email, - permissions.portfolio.organization_name, - exc_info=True, - ) + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) return False return True + def send_portfolio_admin_addition_emails(email: str, requestor, portfolio: Portfolio): """ Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index e2d031fca..d63b5964e 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -215,8 +215,7 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): try: if form.is_change(): if not send_portfolio_member_permission_update_email( - requestor=request.user, - permissions=form.instance + requestor=request.user, permissions=form.instance ): messages.warning(self.request, f"Could not send email notification to {user.email}.") if form.is_change_from_member_to_admin():