diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 41e442f2d..38fde0ced 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -21,49 +21,66 @@ class OpenIdConnectBackend(ModelBackend): """ def authenticate(self, request, **kwargs): - logger.debug("kwargs %s" % kwargs) - user = None - if not kwargs or "sub" not in kwargs.keys(): - return user + logger.debug("kwargs %s", kwargs) + + if not kwargs or "sub" not in kwargs: + return None UserModel = get_user_model() username = self.clean_username(kwargs["sub"]) + openid_data = self.extract_openid_data(kwargs) - # Some OP may actually choose to withhold some information, so we must - # test if it is present - openid_data = {"last_login": timezone.now()} - openid_data["first_name"] = kwargs.get("given_name", "") - openid_data["last_name"] = kwargs.get("family_name", "") - openid_data["email"] = kwargs.get("email", "") - openid_data["phone"] = kwargs.get("phone", "") - - # Note that this could be accomplished in one try-except clause, but - # instead we use get_or_create when creating unknown users since it has - # built-in safeguards for multiple threads. if getattr(settings, "OIDC_CREATE_UNKNOWN_USER", True): - args = { - UserModel.USERNAME_FIELD: username, - # defaults _will_ be updated, these are not fallbacks - "defaults": openid_data, - } - - user, created = UserModel.objects.get_or_create(**args) - - if not created: - # If user exists, update existing user - self.update_existing_user(user, args["defaults"]) - else: - # If user is created, configure the user - user = self.configure_user(user, **kwargs) + user = self.get_or_create_user(UserModel, username, openid_data, kwargs) else: - try: - user = UserModel.objects.get_by_natural_key(username) - except UserModel.DoesNotExist: - return None - # run this callback for a each login - user.on_each_login() + user = self.get_user_by_username(UserModel, username) + + if user: + user.on_each_login() + return user + def extract_openid_data(self, kwargs): + """Extract OpenID data from authentication kwargs.""" + return { + "last_login": timezone.now(), + "first_name": kwargs.get("given_name", ""), + "last_name": kwargs.get("family_name", ""), + "email": kwargs.get("email", ""), + "phone": kwargs.get("phone", ""), + } + + def get_or_create_user(self, UserModel, username, openid_data, kwargs): + """Retrieve user by username or email, or create a new user.""" + user = self.get_user_by_username(UserModel, username) + + if not user and openid_data["email"]: + user = self.get_user_by_email(UserModel, openid_data["email"]) + if user: + # if found by email, update the username + setattr(user, UserModel.USERNAME_FIELD, username) + + if not user: + user = UserModel.objects.create(**{UserModel.USERNAME_FIELD: username}, **openid_data) + return self.configure_user(user, **kwargs) + + self.update_existing_user(user, openid_data) + return user + + def get_user_by_username(self, UserModel, username): + """Retrieve user by username.""" + try: + return UserModel.objects.get(**{UserModel.USERNAME_FIELD: username}) + except UserModel.DoesNotExist: + return None + + def get_user_by_email(self, UserModel, email): + """Retrieve user by email.""" + try: + return UserModel.objects.get(email=email) + except UserModel.DoesNotExist: + return None + def update_existing_user(self, user, kwargs): """ Update user fields without overwriting certain fields. diff --git a/src/djangooidc/tests/test_backends.py b/src/djangooidc/tests/test_backends.py index c15106fa9..4e8f80a23 100644 --- a/src/djangooidc/tests/test_backends.py +++ b/src/djangooidc/tests/test_backends.py @@ -1,5 +1,6 @@ from django.test import TestCase from registrar.models import User +from api.tests.common import less_console_noise_decorator from ..backends import OpenIdConnectBackend # Adjust the import path based on your project structure @@ -17,6 +18,7 @@ class OpenIdConnectBackendTestCase(TestCase): def tearDown(self) -> None: User.objects.all().delete() + @less_console_noise_decorator def test_authenticate_with_create_user(self): """Test that authenticate creates a new user if it does not find existing user""" @@ -32,6 +34,7 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "123456789") + @less_console_noise_decorator def test_authenticate_with_existing_user(self): """Test that authenticate updates an existing user if it finds one. For this test, given_name and family_name are supplied""" @@ -50,6 +53,30 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "123456789") + @less_console_noise_decorator + def test_authenticate_with_existing_user_same_email_different_username(self): + """Test that authenticate updates an existing user if it finds one. + In this case, match is to an existing record with matching email but + a non-matching username. The existing record's username should be udpated. + For this test, given_name and family_name are supplied""" + # Create an existing user with the same username + User.objects.create_user(username="old_username", email="john.doe@example.com") + + # Ensure that the authenticate method updates the existing user + user = self.backend.authenticate(request=None, **self.kwargs) + self.assertIsNotNone(user) + self.assertIsInstance(user, User) + + # Verify that user fields are correctly updated + self.assertEqual(user.first_name, "John") + self.assertEqual(user.last_name, "Doe") + self.assertEqual(user.email, "john.doe@example.com") + self.assertEqual(user.phone, "123456789") + self.assertEqual(user.username, "test_user") + # Assert that a user no longer exists by the old username + self.assertFalse(User.objects.filter(username="old_username").exists()) + + @less_console_noise_decorator def test_authenticate_with_existing_user_with_existing_first_last_phone(self): """Test that authenticate updates an existing user if it finds one. For this test, given_name and family_name are not supplied. @@ -79,6 +106,7 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "9999999999") + @less_console_noise_decorator def test_authenticate_with_existing_user_different_name_phone(self): """Test that authenticate updates an existing user if it finds one. For this test, given_name and family_name are supplied and overwrite""" @@ -100,6 +128,7 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "123456789") + @less_console_noise_decorator def test_authenticate_with_unknown_user(self): """Test that authenticate returns None when no kwargs are supplied""" # Ensure that the authenticate method handles the case when the user is not found diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8ecf36f52..7dbe7abb0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1407,10 +1407,13 @@ 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 - has_errors = any(message.level_tag in ["error", "warning"] for message in storage) + # 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: # Re-render the change form if there are errors or warnings @@ -1552,13 +1555,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/assets/src/js/getgov/table-base.js b/src/registrar/assets/src/js/getgov/table-base.js index 0abfee9b6..ce4397887 100644 --- a/src/registrar/assets/src/js/getgov/table-base.js +++ b/src/registrar/assets/src/js/getgov/table-base.js @@ -129,7 +129,7 @@ export class BaseTable { this.displayName = itemName; this.sectionSelector = itemName + 's'; this.tableWrapper = document.getElementById(`${this.sectionSelector}__table-wrapper`); - this.tableHeaders = document.querySelectorAll(`#${this.sectionSelector} th[data-sortable]`); + this.tableHeaderSortButtons = document.querySelectorAll(`#${this.sectionSelector} th[data-sortable] button`); this.currentSortBy = 'id'; this.currentOrder = 'asc'; this.currentStatus = []; @@ -303,13 +303,18 @@ export class BaseTable { * A helper that resets sortable table headers * */ - unsetHeader = (header) => { - header.removeAttribute('aria-sort'); - let headerName = header.innerText; - const headerLabel = `${headerName}, sortable column, currently unsorted"`; - const headerButtonLabel = `Click to sort by ascending order.`; - header.setAttribute("aria-label", headerLabel); - header.querySelector('.usa-table__header__button').setAttribute("title", headerButtonLabel); + unsetHeader = (headerSortButton) => { + let header = headerSortButton.closest('th'); + if (header) { + header.removeAttribute('aria-sort'); + let headerName = header.innerText; + const headerLabel = `${headerName}, sortable column, currently unsorted"`; + const headerButtonLabel = `Click to sort by ascending order.`; + header.setAttribute("aria-label", headerLabel); + header.querySelector('.usa-table__header__button').setAttribute("title", headerButtonLabel); + } else { + console.warn('Issue with DOM'); + } }; /** @@ -505,24 +510,21 @@ export class BaseTable { // Add event listeners to table headers for sorting initializeTableHeaders() { - this.tableHeaders.forEach(header => { - header.addEventListener('click', event => { - let button = header.querySelector('.usa-table__header__button') - const sortBy = header.getAttribute('data-sortable'); - let order = 'asc'; - // sort order will be ascending, unless the currently sorted column is ascending, and the user - // is selecting the same column to sort in descending order - if (sortBy === this.currentSortBy) { - order = this.currentOrder === 'asc' ? 'desc' : 'asc'; - } - // load the results with the updated sort - this.loadTable(1, sortBy, order); - // If the click occurs outside of the button, need to simulate a button click in order - // for USWDS listener on the button to execute. - // Check first to see if click occurs outside of the button - if (!button.contains(event.target)) { - // Simulate a button click - button.click(); + this.tableHeaderSortButtons.forEach(tableHeader => { + tableHeader.addEventListener('click', event => { + let header = tableHeader.closest('th'); + if (header) { + const sortBy = header.getAttribute('data-sortable'); + let order = 'asc'; + // sort order will be ascending, unless the currently sorted column is ascending, and the user + // is selecting the same column to sort in descending order + if (sortBy === this.currentSortBy) { + order = this.currentOrder === 'asc' ? 'desc' : 'asc'; + } + // load the results with the updated sort + this.loadTable(1, sortBy, order); + } else { + console.warn('Issue with DOM'); } }); }); @@ -587,9 +589,9 @@ export class BaseTable { // Reset UI and accessibility resetHeaders() { - this.tableHeaders.forEach(header => { + this.tableHeaderSortButtons.forEach(headerSortButton => { // Unset sort UI in headers - this.unsetHeader(header); + this.unsetHeader(headerSortButton); }); // Reset the announcement region this.tableAnnouncementRegion.innerHTML = ''; diff --git a/src/registrar/assets/src/js/getgov/table-member-domains.js b/src/registrar/assets/src/js/getgov/table-member-domains.js index f9b789e1f..d1455c4dc 100644 --- a/src/registrar/assets/src/js/getgov/table-member-domains.js +++ b/src/registrar/assets/src/js/getgov/table-member-domains.js @@ -35,16 +35,19 @@ export class MemberDomainsTable extends BaseTable { showElement(dataWrapper); hideElement(noSearchResultsWrapper); hideElement(noDataWrapper); + this.tableAnnouncementRegion.innerHTML = ''; } else { hideElement(dataWrapper); showElement(noSearchResultsWrapper); hideElement(noDataWrapper); + this.tableAnnouncementRegion.innerHTML = this.noSearchResultsWrapper.innerHTML; } } else { hideElement(searchSection); hideElement(dataWrapper); hideElement(noSearchResultsWrapper); showElement(noDataWrapper); + this.tableAnnouncementRegion.innerHTML = this.noDataWrapper.innerHTML; } }; } diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index 75d5170e8..22069f726 100644 --- a/src/registrar/assets/src/sass/_theme/_typography.scss +++ b/src/registrar/assets/src/sass/_theme/_typography.scss @@ -11,7 +11,8 @@ address, } h1:not(.usa-alert__heading), -h2:not(.usa-alert__heading), +// .module h2 excludes headers in DJA +h2:not(.usa-alert__heading, .module h2), h3:not(.usa-alert__heading), h4:not(.usa-alert__heading), h5:not(.usa-alert__heading), 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/fixtures/fixtures_users.py b/src/registrar/fixtures/fixtures_users.py index f1623e674..977bf0858 100644 --- a/src/registrar/fixtures/fixtures_users.py +++ b/src/registrar/fixtures/fixtures_users.py @@ -352,12 +352,37 @@ class UserFixture: @staticmethod def _get_existing_users(users): + # if users match existing users in db by email address, update the users with the username + # from the db. this will prevent duplicate users (with same email) from being added to db. + # it is ok to keep the old username in the db because the username will be updated by oidc process during login + + # Extract email addresses from users + emails = [user.get("email") for user in users] + + # Fetch existing users by email + existing_users_by_email = User.objects.filter(email__in=emails).values_list("email", "username", "id") + + # Create a dictionary to map emails to existing usernames + email_to_existing_user = {user[0]: user[1] for user in existing_users_by_email} + + # Update the users list with the usernames from existing users by email + for user in users: + email = user.get("email") + if email and email in email_to_existing_user: + user["username"] = email_to_existing_user[email] # Update username with the existing one + + # Get the user identifiers (username, id) for the existing users to query the database user_identifiers = [(user.get("username"), user.get("id")) for user in users] + + # Fetch existing users by username or id existing_users = User.objects.filter( username__in=[user[0] for user in user_identifiers] + [user[1] for user in user_identifiers] ).values_list("username", "id") + + # Create sets for usernames and ids that exist existing_usernames = set(user[0] for user in existing_users) existing_user_ids = set(user[1] for user in existing_users) + return existing_usernames, existing_user_ids @staticmethod diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb481db7a..0f0b3f112 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1582,11 +1582,9 @@ class Domain(TimeStampedModel, DomainHelper): if self.is_expired() and self.state != self.State.UNKNOWN: # Given expired is not a physical state, but it is displayed as such, # We need custom logic to determine this message. - help_text = ( - "This domain has expired, but it is still online. " "To renew this domain, contact help@get.gov." - ) + help_text = "This domain has expired. Complete the online renewal process to maintain access." elif flag_is_active(request, "domain_renewal") and self.is_expiring(): - help_text = "This domain will expire soon. Contact one of the listed domain managers to renew the domain." + help_text = "This domain is expiring soon. Complete the online renewal process to maintain access." else: help_text = Domain.State.get_help_text(self.state) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 2b5b56a78..1d508f88f 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -171,11 +171,14 @@ class User(AbstractUser): now = timezone.now().date() expiration_window = 60 threshold_date = now + timedelta(days=expiration_window) + acceptable_statuses = [Domain.State.UNKNOWN, Domain.State.DNS_NEEDED, Domain.State.READY] + num_of_expiring_domains = Domain.objects.filter( id__in=domain_ids, expiration_date__isnull=False, expiration_date__lte=threshold_date, expiration_date__gt=now, + state__in=acceptable_statuses, ).count() return num_of_expiring_domains diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 1d34ef4e4..03df2d59c 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -49,11 +49,11 @@ {% if has_domain_renewal_flag and domain.is_expired and is_domain_manager %} This domain has expired, but it is still online. {% url 'domain-renewal' pk=domain.id as url %} - Renew to maintain access. + Renew to maintain access. {% elif has_domain_renewal_flag and domain.is_expiring and is_domain_manager %} This domain will expire soon. {% url 'domain-renewal' pk=domain.id as url %} - Renew to maintain access. + Renew to maintain access. {% elif has_domain_renewal_flag and domain.is_expiring and is_portfolio_user %} This domain will expire soon. Contact one of the listed domain managers to renew the domain. {% elif has_domain_renewal_flag and domain.is_expired and is_portfolio_user %} diff --git a/src/registrar/templates/domain_renewal.html b/src/registrar/templates/domain_renewal.html index 30e1be0e4..703c2358f 100644 --- a/src/registrar/templates/domain_renewal.html +++ b/src/registrar/templates/domain_renewal.html @@ -38,11 +38,11 @@ {{ block.super }}

Confirm the following information for accuracy

-

Review these details below. We +

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

-

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

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

Required fields are marked with an asterisk (*).

@@ -98,7 +98,7 @@ {% if form.is_policy_acknowledged.errors %} {% for error in form.is_policy_acknowledged.errors %} @@ -131,7 +129,7 @@ name="submit_button" value="next" class="usa-button margin-top-3" - > Submit + > Submit and renew diff --git a/src/registrar/templates/includes/domain_dates.html b/src/registrar/templates/includes/domain_dates.html index b14c091d0..339d75c44 100644 --- a/src/registrar/templates/includes/domain_dates.html +++ b/src/registrar/templates/includes/domain_dates.html @@ -1,7 +1,7 @@ {% if domain.expiration_date or domain.created_at %}

{% if domain.expiration_date %} - Expires: + Date of expiration: {{ domain.expiration_date|date }} {% if domain.is_expired %} (expired){% endif %}
diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index b026a7a6b..e411f1494 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -57,7 +57,7 @@ - {% if has_domain_renewal_flag and num_expiring_domains > 0 and has_any_domains_portfolio_permission %} -

+
-
+

{% if num_expiring_domains == 1%} - One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. + One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. {% else%} - Multiple domains will expire soon. Go to "Manage" to renew the domains. Show expiring domains. + Multiple domains will expire soon. Go to "Manage" to renew the domains. Show expiring domains. {% endif %}

@@ -64,7 +64,7 @@ {% if user_domain_count and user_domain_count > 0 %}
- + Export as CSV @@ -76,14 +76,14 @@ {% if has_domain_renewal_flag and num_expiring_domains > 0 and not portfolio %} -
+
-
+

{% if num_expiring_domains == 1%} - One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. + One domain will expire soon. Go to "Manage" to renew the domain. Show expiring domain. {% else%} - Multiple domains will expire soon. Go to "Manage" to renew the domains. Show expiring domains. + Multiple domains will expire soon. Go to "Manage" to renew the domains. Show expiring domains. {% endif %}

@@ -101,6 +101,7 @@ id="domains__usa-button--filter" aria-expanded="false" aria-controls="filter-status" + aria-label="Status, list 5 items" > Status
- + Export as CSV 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..1377dec42 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(), + }, + ) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index f46e417be..2dfead13f 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -214,7 +214,7 @@ class HomeTests(TestWithUser): @less_console_noise_decorator def test_state_help_text_expired(self): """Tests if each domain state has help text when expired""" - expired_text = "This domain has expired, but it is still online. " + expired_text = "This domain has expired. " test_domain, _ = Domain.objects.get_or_create(name="expired.gov", state=Domain.State.READY) test_domain.expiration_date = date(2011, 10, 10) test_domain.save() @@ -240,7 +240,7 @@ class HomeTests(TestWithUser): """Tests if each domain state has help text when expiration date is None""" # == Test a expiration of None for state ready. This should be expired. == # - expired_text = "This domain has expired, but it is still online. " + expired_text = "This domain has expired. " test_domain, _ = Domain.objects.get_or_create(name="imexpired.gov", state=Domain.State.READY) test_domain.expiration_date = None test_domain.save() diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index d4766b474..02b61fda9 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -439,15 +439,21 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): username="usertest", ) - self.domaintorenew, _ = Domain.objects.get_or_create( + self.domain_to_renew, _ = Domain.objects.get_or_create( name="domainrenewal.gov", ) - UserDomainRole.objects.get_or_create( - user=self.user, domain=self.domaintorenew, role=UserDomainRole.Roles.MANAGER + self.domain_not_expiring, _ = Domain.objects.get_or_create( + name="domainnotexpiring.gov", expiration_date=timezone.now().date() + timedelta(days=65) ) - DomainInformation.objects.get_or_create(creator=self.user, domain=self.domaintorenew) + self.domain_no_domain_manager, _ = Domain.objects.get_or_create(name="domainnodomainmanager.gov") + + UserDomainRole.objects.get_or_create( + user=self.user, domain=self.domain_to_renew, role=UserDomainRole.Roles.MANAGER + ) + + DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_to_renew) self.portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user) @@ -473,13 +479,15 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_expiring_domain_on_detail_page_as_domain_manager(self): + """If a user is a domain manager and their domain is expiring soon, + user should be able to see the "Renew to maintain access" link domain overview detail box.""" self.client.force_login(self.user) with patch.object(Domain, "is_expiring", self.custom_is_expiring), patch.object( Domain, "is_expired", self.custom_is_expired_false ): - self.assertEquals(self.domaintorenew.state, Domain.State.UNKNOWN) + self.assertEquals(self.domain_to_renew.state, Domain.State.UNKNOWN) detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domaintorenew.id}), + reverse("domain", kwargs={"pk": self.domain_to_renew.id}), ) self.assertContains(detail_page, "Expiring soon") @@ -491,6 +499,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) @override_flag("organization_feature", active=True) def test_expiring_domain_on_detail_page_in_org_model_as_a_non_domain_manager(self): + """In org model: If a user is NOT a domain manager and their domain is expiring soon, + user be notified to contact a domain manager in the domain overview detail box.""" portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user) non_dom_manage_user = get_user_model().objects.create( first_name="Non Domain", @@ -510,9 +520,9 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, ], ) - domaintorenew2, _ = Domain.objects.get_or_create(name="bogusdomain2.gov") + domain_to_renew2, _ = Domain.objects.get_or_create(name="bogusdomain2.gov") DomainInformation.objects.get_or_create( - creator=non_dom_manage_user, domain=domaintorenew2, portfolio=self.portfolio + creator=non_dom_manage_user, domain=domain_to_renew2, portfolio=self.portfolio ) non_dom_manage_user.refresh_from_db() self.client.force_login(non_dom_manage_user) @@ -520,38 +530,42 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): Domain, "is_expired", self.custom_is_expired_false ): detail_page = self.client.get( - reverse("domain", kwargs={"pk": domaintorenew2.id}), + reverse("domain", kwargs={"pk": domain_to_renew2.id}), ) self.assertContains(detail_page, "Contact one of the listed domain managers to renew the domain.") @override_flag("domain_renewal", active=True) @override_flag("organization_feature", active=True) def test_expiring_domain_on_detail_page_in_org_model_as_a_domain_manager(self): + """Inorg model: If a user is a domain manager and their domain is expiring soon, + user should be able to see the "Renew to maintain access" link domain overview detail box.""" portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org2", creator=self.user) - domaintorenew3, _ = Domain.objects.get_or_create(name="bogusdomain3.gov") + domain_to_renew3, _ = Domain.objects.get_or_create(name="bogusdomain3.gov") - UserDomainRole.objects.get_or_create(user=self.user, domain=domaintorenew3, role=UserDomainRole.Roles.MANAGER) - DomainInformation.objects.get_or_create(creator=self.user, domain=domaintorenew3, portfolio=portfolio) + UserDomainRole.objects.get_or_create(user=self.user, domain=domain_to_renew3, role=UserDomainRole.Roles.MANAGER) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_to_renew3, portfolio=portfolio) self.user.refresh_from_db() self.client.force_login(self.user) with patch.object(Domain, "is_expiring", self.custom_is_expiring), patch.object( Domain, "is_expired", self.custom_is_expired_false ): detail_page = self.client.get( - reverse("domain", kwargs={"pk": domaintorenew3.id}), + reverse("domain", kwargs={"pk": domain_to_renew3.id}), ) self.assertContains(detail_page, "Renew to maintain access") @override_flag("domain_renewal", active=True) def test_domain_renewal_form_and_sidebar_expiring(self): + """If a user is a domain manager and their domain is expiring soon, + user should be able to see Renewal Form on the sidebar.""" self.client.force_login(self.user) with patch.object(Domain, "is_expiring", self.custom_is_expiring), patch.object( Domain, "is_expiring", self.custom_is_expiring ): # Grab the detail page detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domaintorenew.id}), + reverse("domain", kwargs={"pk": self.domain_to_renew.id}), ) # Make sure we see the link as a domain manager @@ -561,18 +575,19 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(detail_page, "Renewal form") # Grab link to the renewal page - renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domaintorenew.id}) + renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domain_to_renew.id}) self.assertContains(detail_page, f'href="{renewal_form_url}"') # Simulate clicking the link response = self.client.get(renewal_form_url) self.assertEqual(response.status_code, 200) - self.assertContains(response, f"Renew {self.domaintorenew.name}") + self.assertContains(response, f"Renew {self.domain_to_renew.name}") @override_flag("domain_renewal", active=True) def test_domain_renewal_form_and_sidebar_expired(self): - + """If a user is a domain manager and their domain is expired, + user should be able to see Renewal Form on the sidebar.""" self.client.force_login(self.user) with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( @@ -580,10 +595,9 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): ): # Grab the detail page detail_page = self.client.get( - reverse("domain", kwargs={"pk": self.domaintorenew.id}), + reverse("domain", kwargs={"pk": self.domain_to_renew.id}), ) - print("puglesss", self.domaintorenew.is_expired) # Make sure we see the link as a domain manager self.assertContains(detail_page, "Renew to maintain access") @@ -591,17 +605,19 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(detail_page, "Renewal form") # Grab link to the renewal page - renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domaintorenew.id}) + renewal_form_url = reverse("domain-renewal", kwargs={"pk": self.domain_to_renew.id}) self.assertContains(detail_page, f'href="{renewal_form_url}"') # Simulate clicking the link response = self.client.get(renewal_form_url) self.assertEqual(response.status_code, 200) - self.assertContains(response, f"Renew {self.domaintorenew.name}") + self.assertContains(response, f"Renew {self.domain_to_renew.name}") @override_flag("domain_renewal", active=True) def test_domain_renewal_form_your_contact_info_edit(self): + """Checking that if a user is a domain manager they can edit the + Your Profile portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -620,6 +636,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_security_email_edit(self): + """Checking that if a user is a domain manager they can edit the + Security Email portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -641,6 +659,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_domain_renewal_form_domain_manager_edit(self): + """Checking that if a user is a domain manager they can edit the + Domain Manager portion of the Renewal Form.""" with less_console_noise(): # Start on the Renewal page for the domain renewal_page = self.app.get(reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id})) @@ -658,8 +678,26 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): self.assertContains(edit_page, "Domain managers can update all information related to a domain") @override_flag("domain_renewal", active=True) - def test_ack_checkbox_not_checked(self): + def test_domain_renewal_form_not_expired_or_expiring(self): + """Checking that if the user's domain is not expired or expiring that user should not be able + to access /renewal and that it should receive a 403.""" + with less_console_noise(): + # Start on the Renewal page for the domain + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domain_not_expiring.id})) + self.assertEqual(renewal_page.status_code, 403) + @override_flag("domain_renewal", active=True) + def test_domain_renewal_form_does_not_appear_if_not_domain_manager(self): + """If user is not a domain manager and tries to access /renewal, user should receive a 403.""" + with patch.object(Domain, "is_expired", self.custom_is_expired_true), patch.object( + Domain, "is_expired", self.custom_is_expired_true + ): + renewal_page = self.client.get(reverse("domain-renewal", kwargs={"pk": self.domain_no_domain_manager.id})) + self.assertEqual(renewal_page.status_code, 403) + + @override_flag("domain_renewal", active=True) + def test_ack_checkbox_not_checked(self): + """If user don't check the checkbox, user should receive an error message.""" # Grab the renewal URL renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) @@ -671,7 +709,8 @@ class TestDomainDetailDomainRenewal(TestDomainOverview): @override_flag("domain_renewal", active=True) def test_ack_checkbox_checked(self): - + """If user check the checkbox and submits the form, + user should be redirected Domain Over page with an updated by 1 year expiration date""" # Grab the renewal URL with patch.object(Domain, "renew_domain", self.custom_renew_domain): renewal_url = reverse("domain-renewal", kwargs={"pk": self.domain_with_ip.id}) @@ -886,6 +925,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 @@ -2866,11 +2939,11 @@ class TestDomainRenewal(TestWithUser): name="igorville.gov", expiration_date=expiring_date ) self.domain_with_expired_date, _ = Domain.objects.get_or_create( - name="domainwithexpireddate.com", expiration_date=expired_date + name="domainwithexpireddate.gov", expiration_date=expired_date ) self.domain_with_current_date, _ = Domain.objects.get_or_create( - name="domainwithfarexpireddate.com", expiration_date=expiring_date_current + name="domainwithfarexpireddate.gov", expiration_date=expiring_date_current ) UserDomainRole.objects.get_or_create( @@ -2916,7 +2989,7 @@ class TestDomainRenewal(TestWithUser): today = datetime.now() expiring_date = (today + timedelta(days=30)).strftime("%Y-%m-%d") self.domain_with_another_expiring, _ = Domain.objects.get_or_create( - name="domainwithanotherexpiringdate.com", expiration_date=expiring_date + name="domainwithanotherexpiringdate.gov", expiration_date=expiring_date ) UserDomainRole.objects.get_or_create( @@ -2952,7 +3025,7 @@ class TestDomainRenewal(TestWithUser): today = datetime.now() expiring_date = (today + timedelta(days=31)).strftime("%Y-%m-%d") self.domain_with_another_expiring_org_model, _ = Domain.objects.get_or_create( - name="domainwithanotherexpiringdate_orgmodel.com", expiration_date=expiring_date + name="domainwithanotherexpiringdate_orgmodel.gov", expiration_date=expiring_date ) UserDomainRole.objects.get_or_create( diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 3afbb2a7b..33f334f7f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -3254,7 +3254,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(), @@ -3339,7 +3339,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(), diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index d589f814f..f9c3b89b2 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: @@ -75,10 +84,12 @@ def send_emails_to_domain_managers(email: str, requestor_email, domain: Domain, "date": date.today(), }, ) - except EmailSendingError as err: - raise EmailSendingError( - f"Could not send email manager notification to {user.email} for domain: {domain.name}" - ) from err + 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 def normalize_domains(domains: Domain | list[Domain]) -> list[Domain]: diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 83898934c..297cb689a 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -311,11 +311,39 @@ class DomainView(DomainBaseView): self._update_session_with_domain() -class DomainRenewalView(DomainView): +class DomainRenewalView(DomainBaseView): """Domain detail overview page.""" template_name = "domain_renewal.html" + def get_context_data(self, **kwargs): + """Grabs the security email information and adds security_email to the renewal form context + sets it to None if it uses a default email""" + + context = super().get_context_data(**kwargs) + + default_emails = [DefaultEmail.PUBLIC_CONTACT_DEFAULT.value, DefaultEmail.LEGACY_DEFAULT.value] + + context["hidden_security_emails"] = default_emails + + security_email = self.object.get_security_email() + context["security_email"] = security_email + return context + + def in_editable_state(self, pk): + """Override in_editable_state from DomainPermission + Allow renewal form to be accessed + returns boolean""" + requested_domain = None + if Domain.objects.filter(id=pk).exists(): + requested_domain = Domain.objects.get(id=pk) + + return ( + requested_domain + and requested_domain.is_editable() + and (requested_domain.is_expiring() or requested_domain.is_expired()) + ) + def post(self, request, pk): domain = get_object_or_404(Domain, id=pk) @@ -1227,24 +1255,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 f5db165d1..212ce089d 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -303,13 +303,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( [ @@ -525,12 +526,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) @@ -807,7 +809,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( @@ -816,4 +818,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 98c36b308..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 - from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, @@ -61,20 +60,19 @@ 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)) - 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.") diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 236ef8696..08212088b 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -192,7 +192,8 @@ class DomainPermission(PermissionsLoginMixin): def can_access_domain_via_portfolio(self, pk): """Most views should not allow permission to portfolio users. If particular views allow access to the domain pages, they will need to override - this function.""" + this function. + """ return False def in_editable_state(self, pk):