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..31c75e05e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1329,6 +1329,14 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): get_roles.short_description = "Roles" # type: ignore + 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. + 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 + class UserDomainRoleAdmin(ListHeaderAdmin, ImportExportModelAdmin): """Custom user domain role admin class.""" @@ -1407,10 +1415,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 +1563,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() @@ -1657,6 +1669,14 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): # Call the parent save method to save the object super().save_model(request, obj, form, change) + 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. + 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 + class DomainInformationResource(resources.ModelResource): """defines how each field in the referenced model should be mapped to the corresponding fields in the diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index d3422b722..c96677ebc 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -87,14 +87,6 @@ export function initAddNewMemberPageListeners() { }); }); - /* - Helper function to capitalize the first letter in a string (for display purposes) - */ - function capitalizeFirstLetter(text) { - if (!text) return ''; // Return empty string if input is falsy - return text.charAt(0).toUpperCase() + text.slice(1); - } - /* Populates contents of the "Add Member" confirmation modal */ @@ -102,10 +94,12 @@ export function initAddNewMemberPageListeners() { const permissionDetailsContainer = document.getElementById("permission_details"); permissionDetailsContainer.innerHTML = ""; // Clear previous content - // Get all permission sections (divs with h3 and radio inputs) - const permissionSections = document.querySelectorAll(`#${permission_details_div_id} > h3`); + if (permission_details_div_id == 'member-basic-permissions') { + // for basic users, display values are based on selections in the form + // Get all permission sections (divs with h3 and radio inputs) + const permissionSections = document.querySelectorAll(`#${permission_details_div_id} > h3`); - permissionSections.forEach(section => { + permissionSections.forEach(section => { // Find the
) + const mainText = Array.from(label.childNodes) + .filter(node => node.nodeType === Node.TEXT_NODE) + .map(node => node.textContent.trim()) + .join(""); // Combine and trim whitespace + selectedPermission = mainText || "No permission selected"; } - - // Create new elements for the modal content - const titleElement = document.createElement("h4"); - titleElement.textContent = sectionTitle; - titleElement.classList.add("text-primary"); - titleElement.classList.add("margin-bottom-0"); - - const permissionElement = document.createElement("p"); - permissionElement.textContent = selectedPermission; - permissionElement.classList.add("margin-top-0"); - - // Append to the modal content container - permissionDetailsContainer.appendChild(titleElement); - permissionDetailsContainer.appendChild(permissionElement); + } + appendPermissionInContainer(sectionTitle, selectedPermission, permissionDetailsContainer); } - }); + }); + } else { + // for admin users, the permissions are always the same + appendPermissionInContainer('Domains', 'Viewer, all', permissionDetailsContainer); + appendPermissionInContainer('Domain requests', 'Creator', permissionDetailsContainer); + appendPermissionInContainer('Members', 'Manager', permissionDetailsContainer); + } + } + + function appendPermissionInContainer(sectionTitle, permissionDisplay, permissionContainer) { + // Create new elements for the content + const titleElement = document.createElement("h4"); + titleElement.textContent = sectionTitle; + titleElement.classList.add("text-primary", "margin-bottom-0"); + + const permissionElement = document.createElement("p"); + permissionElement.textContent = permissionDisplay; + permissionElement.classList.add("margin-top-0"); + + // Append to the content container + permissionContainer.appendChild(titleElement); + permissionContainer.appendChild(permissionElement); } /* @@ -149,18 +158,25 @@ export function initAddNewMemberPageListeners() { let emailValue = document.getElementById('id_email').value; document.getElementById('modalEmail').textContent = emailValue; - // Get selected radio button for access level + // Get selected radio button for member access level let selectedAccess = document.querySelector('input[name="role"]:checked'); - // Set the selected permission text to 'Basic' or 'Admin' (the value of the selected radio button) - // This value does not have the first letter capitalized so let's capitalize it - let accessText = selectedAccess ? capitalizeFirstLetter(selectedAccess.value) : "No access level selected"; + // Map the access level values to user-friendly labels + const accessLevelMapping = { + organization_admin: "Admin", + organization_member: "Basic", + }; + // Determine the access text based on the selected value + let accessText = selectedAccess + ? accessLevelMapping[selectedAccess.value] || "Unknown access level" + : "No access level selected"; + // Update the modal with the appropriate member access level text document.getElementById('modalAccessLevel').textContent = accessText; // Populate permission details based on access level if (selectedAccess && selectedAccess.value === 'organization_admin') { - populatePermissionDetails('new-member-admin-permissions'); + populatePermissionDetails('admin'); } else { - populatePermissionDetails('new-member-basic-permissions'); + populatePermissionDetails('member-basic-permissions'); } //------- Show the modal @@ -177,22 +193,14 @@ export function initPortfolioMemberPageRadio() { document.addEventListener("DOMContentLoaded", () => { let memberForm = document.getElementById("member_form"); let newMemberForm = document.getElementById("add_member_form") - if (memberForm) { + if (memberForm || newMemberForm) { hookupRadioTogglerListener( 'role', { - 'organization_admin': 'member-admin-permissions', + 'organization_admin': '', 'organization_member': 'member-basic-permissions' } ); - }else if (newMemberForm){ - hookupRadioTogglerListener( - 'role', - { - 'organization_admin': 'new-member-admin-permissions', - 'organization_member': 'new-member-basic-permissions' - } - ); } }); } 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/_accordions.scss b/src/registrar/assets/src/sass/_theme/_accordions.scss index 762618415..ca9990ca9 100644 --- a/src/registrar/assets/src/sass/_theme/_accordions.scss +++ b/src/registrar/assets/src/sass/_theme/_accordions.scss @@ -49,3 +49,30 @@ tr:last-of-type .usa-accordion--more-actions .usa-accordion__content { bottom: -10px; right: 30px; } + +// A CSS only show-more/show-less based on usa-accordion +.usa-accordion--show-more { + width: auto; + .usa-accordion__button[aria-expanded=false], + .usa-accordion__button[aria-expanded=false]:hover, + .usa-accordion__button[aria-expanded=true], + .usa-accordion__button[aria-expanded=true]:hover { + background-image: none; + background-color: transparent; + padding-right: 0; + padding-left: 0; + font-weight: normal; + } + .usa-accordion__button[aria-expanded=true] .expand-more { + display: inline-block; + } + .usa-accordion__button[aria-expanded=true] .expand-less { + display: none; + } + .usa-accordion__button[aria-expanded=false] .expand-more { + display: none; + } + .usa-accordion__button[aria-expanded=false] .expand-less { + display: inline-block; + } +} diff --git a/src/registrar/assets/src/sass/_theme/_tables.scss b/src/registrar/assets/src/sass/_theme/_tables.scss index a8a829a45..37ae22b1b 100644 --- a/src/registrar/assets/src/sass/_theme/_tables.scss +++ b/src/registrar/assets/src/sass/_theme/_tables.scss @@ -105,3 +105,25 @@ th { } } } + +.dotgov-table--cell-padding-2 { + td, th { + padding: units(2); + } +} + +.usa-table--striped tbody tr:nth-child(odd) th, +.usa-table--striped tbody tr:nth-child(odd) td { + background-color: color('primary-lightest'); +} + +.usa-table--bg-transparent { + td, thead th { + background-color: transparent; + } +} + +.usa-table--full-borderless td, +.usa-table--full-borderless th { + border: none !important; +} 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 diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index 5e00bf1b4..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), @@ -45,3 +46,7 @@ h4, .h4 { padding-left: units(1); border-left: 2px solid color('base-lighter'); } + +.font-body-1 { + font-size: size('body', 1); +} 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/forms/portfolio.py b/src/registrar/forms/portfolio.py index e57b56c4f..c9ef280b0 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,7 +4,6 @@ import logging from django import forms from django.core.validators import RegexValidator from django.core.validators import MaxLengthValidator -from django.utils.safestring import mark_safe from registrar.forms.utility.combobox import ComboboxWidget from registrar.models import ( @@ -121,47 +120,47 @@ class BasePortfolioMemberForm(forms.ModelForm): widget=forms.RadioSelect, required=True, error_messages={ - "required": "Member access level is required", + "required": "Select the level of access you would like to grant this member.", }, ) - domain_request_permission_admin = forms.ChoiceField( - label=mark_safe(f"Select permission {required_star}"), # nosec + domain_permissions = forms.ChoiceField( choices=[ - (UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, "View all requests"), - (UserPortfolioPermissionChoices.EDIT_REQUESTS.value, "View all requests plus create requests"), + (UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, "Viewer, limited"), + (UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value, "Viewer, all"), ], widget=forms.RadioSelect, required=False, + initial=UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, error_messages={ - "required": "Admin domain request permission is required", + "required": "Domain permission is required.", }, ) - member_permission_admin = forms.ChoiceField( - label=mark_safe(f"Select permission {required_star}"), # nosec + domain_request_permissions = forms.ChoiceField( choices=[ - (UserPortfolioPermissionChoices.VIEW_MEMBERS.value, "View all members"), - (UserPortfolioPermissionChoices.EDIT_MEMBERS.value, "View all members plus manage members"), - ], - widget=forms.RadioSelect, - required=False, - error_messages={ - "required": "Admin member permission is required", - }, - ) - - domain_request_permission_member = forms.ChoiceField( - label=mark_safe(f"Select permission {required_star}"), # nosec - choices=[ - (UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, "View all requests"), - (UserPortfolioPermissionChoices.EDIT_REQUESTS.value, "View all requests plus create requests"), ("no_access", "No access"), + (UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value, "Viewer"), + (UserPortfolioPermissionChoices.EDIT_REQUESTS.value, "Creator"), ], widget=forms.RadioSelect, required=False, + initial="no_access", error_messages={ - "required": "Basic member permission is required", + "required": "Domain request permission is required.", + }, + ) + + member_permissions = forms.ChoiceField( + choices=[ + ("no_access", "No access"), + (UserPortfolioPermissionChoices.VIEW_MEMBERS.value, "Viewer"), + ], + widget=forms.RadioSelect, + required=False, + initial="no_access", + error_messages={ + "required": "Member permission is required.", }, ) @@ -169,12 +168,11 @@ class BasePortfolioMemberForm(forms.ModelForm): # All of the fields included here have "required=False" by default as they are conditionally required. # see def clean() for more details. ROLE_REQUIRED_FIELDS = { - UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ - "domain_request_permission_admin", - "member_permission_admin", - ], + UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [], UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ - "domain_request_permission_member", + "domain_permissions", + "member_permissions", + "domain_request_permissions", ], } @@ -190,15 +188,24 @@ class BasePortfolioMemberForm(forms.ModelForm): Update field descriptions. """ super().__init__(*args, **kwargs) - # Adds a
description beneath each role option - self.fields["role"].descriptions = { - "organization_admin": UserPortfolioRoleChoices.get_role_description( - UserPortfolioRoleChoices.ORGANIZATION_ADMIN - ), - "organization_member": UserPortfolioRoleChoices.get_role_description( - UserPortfolioRoleChoices.ORGANIZATION_MEMBER - ), + + # Adds a
description beneath each option + self.fields["domain_permissions"].descriptions = { + UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value: "Can view only the domains they manage", + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value: "Can view all domains for the organization", } + self.fields["domain_request_permissions"].descriptions = { + UserPortfolioPermissionChoices.EDIT_REQUESTS.value: ( + "Can view all domain requests for the organization and create requests" + ), + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS.value: "Can view all domain requests for the organization", + "no_access": "Cannot view or create domain requests", + } + self.fields["member_permissions"].descriptions = { + UserPortfolioPermissionChoices.VIEW_MEMBERS.value: "Can view all members permissions", + "no_access": "Cannot view member permissions", + } + # Map model instance values to custom form fields if self.instance: self.map_instance_to_initial() @@ -222,8 +229,12 @@ class BasePortfolioMemberForm(forms.ModelForm): self.add_error(field_name, self.fields.get(field_name).error_messages.get("required")) # Edgecase: Member uses a special form value for None called "no_access". - if cleaned_data.get("domain_request_permission_member") == "no_access": - cleaned_data["domain_request_permission_member"] = None + if cleaned_data.get("domain_request_permissions") == "no_access": + cleaned_data["domain_request_permissions"] = None + + # Edgecase: Member uses a special form value for None called "no_access". + if cleaned_data.get("member_permissions") == "no_access": + cleaned_data["member_permissions"] = None # Handle roles cleaned_data["roles"] = [role] @@ -253,7 +264,7 @@ class BasePortfolioMemberForm(forms.ModelForm): "role": "organization_admin" or "organization_member", "member_permission_admin": permission level if admin, "domain_request_permission_admin": permission level if admin, - "domain_request_permission_member": permission level if member + "domain_request_permissions": permission level if member } """ if self.initial is None: @@ -267,12 +278,15 @@ class BasePortfolioMemberForm(forms.ModelForm): UserPortfolioRoleChoices.ORGANIZATION_ADMIN, UserPortfolioRoleChoices.ORGANIZATION_MEMBER, ] - domain_perms = [ + domain_request_perms = [ UserPortfolioPermissionChoices.EDIT_REQUESTS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] + domain_perms = [ + UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + ] member_perms = [ - UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_MEMBERS, ] @@ -282,16 +296,21 @@ class BasePortfolioMemberForm(forms.ModelForm): roles = self.instance.roles or [] selected_role = next((role for role in roles if role in roles), None) self.initial["role"] = selected_role - is_admin = selected_role == UserPortfolioRoleChoices.ORGANIZATION_ADMIN - if is_admin: - selected_domain_permission = next((perm for perm in domain_perms if perm in perms), None) - selected_member_permission = next((perm for perm in member_perms if perm in perms), None) - self.initial["domain_request_permission_admin"] = selected_domain_permission - self.initial["member_permission_admin"] = selected_member_permission - else: - # Edgecase: Member uses a special form value for None called "no_access". This ensures a form selection. - selected_domain_permission = next((perm for perm in domain_perms if perm in perms), "no_access") - self.initial["domain_request_permission_member"] = selected_domain_permission + is_member = selected_role == UserPortfolioRoleChoices.ORGANIZATION_MEMBER + if is_member: + # Edgecase: Member and domain request use a special form value for None called "no_access". + # This ensures a form selection. + selected_domain_permission = next( + (perm for perm in domain_perms if perm in perms), + UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, + ) + selected_domain_request_permission = next( + (perm for perm in domain_request_perms if perm in perms), "no_access" + ) + selected_member_permission = next((perm for perm in member_perms if perm in perms), "no_access") + self.initial["domain_request_permissions"] = selected_domain_request_permission + self.initial["domain_permissions"] = selected_domain_permission + self.initial["member_permissions"] = selected_member_permission class PortfolioMemberForm(BasePortfolioMemberForm): @@ -320,7 +339,7 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm): """ email = forms.EmailField( - label="Enter the email of the member you'd like to invite", + label="Email", max_length=None, error_messages={ "invalid": ("Enter an email address in the required format, like name@example.com."), 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/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.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/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 03a01b80d..11d9c56e3 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 @@ -21,16 +22,18 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - # Domain: field specific permissions UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], # NOTE: Check FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS before adding roles here. UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, ], } @@ -38,9 +41,9 @@ class UserPortfolioPermission(TimeStampedModel): # Used to throw a ValidationError on clean() for UserPortfolioPermission and PortfolioInvitation. FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ - UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_MEMBERS, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], } @@ -186,3 +189,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..8c42b80c7 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -25,23 +25,6 @@ class UserPortfolioRoleChoices(models.TextChoices): logger.warning(f"Invalid portfolio role: {user_portfolio_role}") return f"Unknown ({user_portfolio_role})" - @classmethod - def get_role_description(cls, user_portfolio_role): - """Returns a detailed description for a given role.""" - descriptions = { - cls.ORGANIZATION_ADMIN: ( - "Grants this member access to the organization-wide information " - "on domains, domain requests, and members. Domain management can be assigned separately." - ), - cls.ORGANIZATION_MEMBER: ( - "Grants this member access to the organization. They can be given extra permissions to view all " - "organization domain requests and submit domain requests on behalf of the organization. Basic access " - "members can’t view all members of an organization or manage them. " - "Domain management can be assigned separately." - ), - } - return descriptions.get(user_portfolio_role) - class UserPortfolioPermissionChoices(models.TextChoices): """ """ @@ -227,3 +210,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/templates/django/forms/widgets/multiple_input.html b/src/registrar/templates/django/forms/widgets/multiple_input.html index cc0e11989..af98e898b 100644 --- a/src/registrar/templates/django/forms/widgets/multiple_input.html +++ b/src/registrar/templates/django/forms/widgets/multiple_input.html @@ -21,7 +21,7 @@ {% if field and field.field and field.field.descriptions %} {% with description=field.field.descriptions|get_dict_value:option.value %} {% if description %} -
{{ description }}
+{{ description }}
{% endif %} {% endwith %} {% endif %} 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 }}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 (*).
{% 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 ca64e1bff..8adc0929a 100644
--- a/src/registrar/templates/includes/domain_requests_table.html
+++ b/src/registrar/templates/includes/domain_requests_table.html
@@ -65,6 +65,7 @@
id="domain-requests__usa-button--filter"
aria-expanded="false"
aria-controls="filter-status"
+ aria-label="Status, list 7 items"
>
Status