From a2e6238b18ae36413962d3ab5d3bad19e4cd9084 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 12 Dec 2024 17:38:28 -0500 Subject: [PATCH 001/114] wip --- src/registrar/views/domain.py | 80 +++++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index cb3da1f83..94423f4f8 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1184,11 +1184,13 @@ class DomainAddUserView(DomainFormBaseView): return reverse("domain-users", kwargs={"pk": self.object.pk}) def _domain_abs_url(self): - """Get an absolute URL for this domain.""" + """Get an absolute URL for this domain. + Used by the email helper.""" return self.request.build_absolute_uri(reverse("domain", kwargs={"pk": self.object.id})) def _is_member_of_different_org(self, email, requestor, requested_user): - """Verifies if an email belongs to a different organization as a member or invited member.""" + """Verifies if an email belongs to a different organization as a member or invited member. + Used by the email helper.""" # Check if user is a already member of a different organization than the requestor's org requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() @@ -1225,22 +1227,27 @@ class DomainAddUserView(DomainFormBaseView): raises EmailSendingError """ - # Set a default email address to send to for staff + print('_send_domain_invitation_email') + + # FROM email for staff requestor_email = settings.DEFAULT_FROM_EMAIL - # Check if the email requestor has a valid email address - if not requestor.is_staff and requestor.email is not None and requestor.email.strip() != "": - requestor_email = requestor.email - elif not requestor.is_staff: - messages.error(self.request, "Can't send invitation email. No email is associated with your account.") - logger.error( - f"Can't send email to '{email}' on domain '{self.object}'." - f"No email exists for the requestor '{requestor.username}'.", - exc_info=True, - ) - return None + # FROM email for users + if not requestor.is_staff: + if requestor.email is not None and requestor.email.strip() != "": + requestor_email = requestor.email + else: + # The user is not staff and does not have an email + logger.error( + f"Can't send email to '{email}' on domain '{self.object}'." + f"No email exists for the requestor '{requestor.username}'.", + exc_info=True, + ) + # The None returned here will trigger a specific error alert + return None - # Check is user is a member or invited member of a different org from this domain's org + + # Check is user is a member or invited member of a different org if flag_is_active_for_user(requestor, "organization_feature") and self._is_member_of_different_org( email, requestor, requested_user ): @@ -1252,8 +1259,8 @@ class DomainAddUserView(DomainFormBaseView): invite = DomainInvitation.objects.get(email=email, domain=self.object) # check if the invite has already been accepted or has a canceled invite add_success = self._check_invite_status(invite, email) - except Exception: - logger.error("An error occured") + except Exception as exeption: + logger.error(f"Invite does not exist: {exeption}") try: send_templated_email( @@ -1275,14 +1282,25 @@ class DomainAddUserView(DomainFormBaseView): ) logger.info(exc) raise EmailSendingError("Could not send email invitation.") from exc - else: - if add_success: - messages.success(self.request, f"{email} has been invited to this domain.") + # else: + # if add_success: + # messages.success(self.request, f"{email} has been invited to this domain.") + + return add_success def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" try: - self._send_domain_invitation_email(email=email_address, requestor=requestor) + print('_make_invitation') + add_success = self._send_domain_invitation_email(email=email_address, requestor=requestor) + + if add_success is None: + messages.error(self.request, "Can't send invitation email. No email is associated with your account.") + elif add_success == True: + messages.success(self.request, f"{email_address} has been invited to this domain.") + + + except EmailSendingError: messages.warning(self.request, "Could not send email invitation.") else: @@ -1295,21 +1313,28 @@ class DomainAddUserView(DomainFormBaseView): Throws EmailSendingError.""" requested_email = form.cleaned_data["email"] requestor = self.request.user - email_success = False + should_add_user_domain_role = False # look up a user with that email try: requested_user = User.objects.get(email=requested_email) except User.DoesNotExist: # no matching user, go make an invitation - email_success = True + print('User.DoesNotExist') return self._make_invitation(requested_email, requestor) else: # if user already exists then just send an email try: - self._send_domain_invitation_email( + print('else block after User.DoesNotExist - user already exists then just send an email') + add_success = self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) - email_success = True + + if add_success is None: + messages.error(self.request, "Can't send invitation email. No email is associated with your account.") + elif add_success == True: + messages.success(self.request, f"{requested_email} has been invited to this domain.") + + should_add_user_domain_role = True except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -1317,7 +1342,7 @@ class DomainAddUserView(DomainFormBaseView): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - email_success = True + should_add_user_domain_role = True except OutsideOrgMemberError: logger.warn( "Could not send email. Can not invite member of a .gov organization to a different organization.", @@ -1335,8 +1360,9 @@ class DomainAddUserView(DomainFormBaseView): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - if email_success: + if should_add_user_domain_role: try: + print('will add domain role') UserDomainRole.objects.create( user=requested_user, domain=self.object, From d766aa9b2a384cf86e0091b04b652794d8533b3d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 12 Dec 2024 20:23:30 -0500 Subject: [PATCH 002/114] refactored _send_domain_invitation_email in domain.py view --- src/registrar/views/domain.py | 225 ++++++++++++++++------------------ 1 file changed, 108 insertions(+), 117 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 94423f4f8..526cad53f 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -69,6 +69,34 @@ from django import forms logger = logging.getLogger(__name__) +class InvitationError(Exception): + """Base exception for invitation-related errors.""" + + pass + + +class AlreadyManagerError(InvitationError): + """Raised when the user is already a manager for the domain.""" + + def __init__(self, email): + super().__init__(f"{email} is already a manager for this domain.") + + +class AlreadyInvitedError(InvitationError): + """Raised when the user has already been invited to the domain.""" + + def __init__(self, email): + super().__init__(f"{email} has already been invited to this domain.") + + +class MissingEmailError(InvitationError): + """Raised when the requestor has no email associated with their account.""" + + def __init__(self, username): + super().__init__(f"Can't send invitation email. No email is associated with the account for '{username}'.") + self.username = username + + class DomainBaseView(DomainPermissionView): """ Base View for the Domain. Handles getting and setting the domain @@ -1184,13 +1212,11 @@ class DomainAddUserView(DomainFormBaseView): return reverse("domain-users", kwargs={"pk": self.object.pk}) def _domain_abs_url(self): - """Get an absolute URL for this domain. - Used by the email helper.""" + """Get an absolute URL for this domain.""" return self.request.build_absolute_uri(reverse("domain", kwargs={"pk": self.object.id})) def _is_member_of_different_org(self, email, requestor, requested_user): - """Verifies if an email belongs to a different organization as a member or invited member. - Used by the email helper.""" + """Verifies if an email belongs to a different organization as a member or invited member.""" # Check if user is a already member of a different organization than the requestor's org requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() @@ -1200,67 +1226,53 @@ class DomainAddUserView(DomainFormBaseView): existing_org_invitation and existing_org_invitation.portfolio != requestor_org ) - def _check_invite_status(self, invite, email): - """Check if invitation status is canceled or retrieved, and gives the appropiate response""" - if invite.status == DomainInvitation.DomainInvitationStatus.RETRIEVED: - messages.warning( - self.request, - f"{email} is already a manager for this domain.", - ) - return False - elif invite.status == DomainInvitation.DomainInvitationStatus.CANCELED: - invite.update_cancellation_status() - invite.save() - return True - else: - # else if it has been sent but not accepted - messages.warning(self.request, f"{email} has already been invited to this domain") - return False + def _check_existing_invitation_status(self, email, domain): + """Check if existing invitation exists; checks its status for canceled, invited or retrieved, and gives the appropiate response. - def _send_domain_invitation_email(self, email: str, requestor: User, requested_user=None, add_success=True): + Exceptions raised for RETRIEVED or INVITED. Existing CANCELED invitations updated to INVITED. + Existing CANCELED invitations do not raise an exception.""" + try: + invite = DomainInvitation.objects.get(email=email, domain=domain) + except Exception as err: + # No existing invitation, do nothing + return + else: + if invite.status == DomainInvitation.DomainInvitationStatus.RETRIEVED: + raise AlreadyManagerError(email) + elif invite.status == DomainInvitation.DomainInvitationStatus.CANCELED: + invite.update_cancellation_status() + invite.save() + else: + # Status is INVITED + raise AlreadyInvitedError(email) + + def _send_domain_invitation_email(self, email: str, requestor: User, requested_user=None): """Performs the sending of the domain invitation email, does not make a domain information object email: string- email to send to - add_success: bool- default True indicates: - adding a success message to the view if the email sending succeeds raises EmailSendingError """ - print('_send_domain_invitation_email') - - # FROM email for staff + # Set a default email address to send to for staff requestor_email = settings.DEFAULT_FROM_EMAIL - # FROM email for users + # refactor below to raise an error + # if the user is not a staff member if not requestor.is_staff: - if requestor.email is not None and requestor.email.strip() != "": - requestor_email = requestor.email + if not requestor.email or requestor.email.strip() == "": + raise MissingEmailError(requestor.username) else: - # The user is not staff and does not have an email - logger.error( - f"Can't send email to '{email}' on domain '{self.object}'." - f"No email exists for the requestor '{requestor.username}'.", - exc_info=True, - ) - # The None returned here will trigger a specific error alert - return None + requestor_email = requestor.email - - # Check is user is a member or invited member of a different org + # Check is user is a member or invited member of a different org from this domain's org if flag_is_active_for_user(requestor, "organization_feature") and self._is_member_of_different_org( email, requestor, requested_user ): - add_success = False raise OutsideOrgMemberError # Check to see if an invite has already been sent - try: - invite = DomainInvitation.objects.get(email=email, domain=self.object) - # check if the invite has already been accepted or has a canceled invite - add_success = self._check_invite_status(invite, email) - except Exception as exeption: - logger.error(f"Invite does not exist: {exeption}") + self._check_existing_invitation_status(email=email, domain=self.object) try: send_templated_email( @@ -1282,95 +1294,74 @@ class DomainAddUserView(DomainFormBaseView): ) logger.info(exc) raise EmailSendingError("Could not send email invitation.") from exc - # else: - # if add_success: - # messages.success(self.request, f"{email} has been invited to this domain.") - - return add_success - - def _make_invitation(self, email_address: str, requestor: User): - """Make a Domain invitation for this email and redirect with a message.""" - try: - print('_make_invitation') - add_success = self._send_domain_invitation_email(email=email_address, requestor=requestor) - - if add_success is None: - messages.error(self.request, "Can't send invitation email. No email is associated with your account.") - elif add_success == True: - messages.success(self.request, f"{email_address} has been invited to this domain.") - - - - except EmailSendingError: - messages.warning(self.request, "Could not send email invitation.") - else: - # (NOTE: only create a domainInvitation if the e-mail sends correctly) - DomainInvitation.objects.get_or_create(email=email_address, domain=self.object) - return redirect(self.get_success_url()) def form_valid(self, form): """Add the specified user on this domain. Throws EmailSendingError.""" requested_email = form.cleaned_data["email"] requestor = self.request.user - should_add_user_domain_role = False + # look up a user with that email + requested_user = None try: requested_user = User.objects.get(email=requested_email) except User.DoesNotExist: - # no matching user, go make an invitation - print('User.DoesNotExist') - return self._make_invitation(requested_email, requestor) - else: - # if user already exists then just send an email - try: - print('else block after User.DoesNotExist - user already exists then just send an email') - add_success = self._send_domain_invitation_email( - requested_email, requestor, requested_user=requested_user, add_success=False - ) + pass - if add_success is None: - messages.error(self.request, "Can't send invitation email. No email is associated with your account.") - elif add_success == True: - messages.success(self.request, f"{requested_email} has been invited to this domain.") - - should_add_user_domain_role = True - except EmailSendingError: - logger.warn( - "Could not send email invitation (EmailSendingError)", - self.object, - exc_info=True, + try: + if requested_user is None: + # no user exists, send an email and make an invitation + self._send_domain_invitation_email(email=requested_email, requestor=requestor) + DomainInvitation.objects.get_or_create(email=requested_email, domain=self.object) + messages.success(self.request, f"{requested_email} has been invited to this domain.") + else: + # user exists, send email and make user domain role + self._send_domain_invitation_email( + email=requested_email, requestor=requestor, requested_user=requested_user ) - messages.warning(self.request, "Could not send email invitation.") - should_add_user_domain_role = True - except OutsideOrgMemberError: - logger.warn( - "Could not send email. Can not invite member of a .gov organization to a different organization.", - self.object, - exc_info=True, - ) - messages.error( - self.request, - f"{requested_email} is already a member of another .gov organization.", - ) - except Exception: - logger.warn( - "Could not send email invitation (Other Exception)", - self.object, - exc_info=True, - ) - messages.warning(self.request, "Could not send email invitation.") - if should_add_user_domain_role: - try: - print('will add domain role') UserDomainRole.objects.create( user=requested_user, domain=self.object, role=UserDomainRole.Roles.MANAGER, ) messages.success(self.request, f"Added user {requested_email}.") - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") + except EmailSendingError: + logger.warn( + "Could not send email invitation (EmailSendingError)", + self.object, + exc_info=True, + ) + messages.warning(self.request, "Could not send email invitation.") + except OutsideOrgMemberError: + logger.warn( + "Could not send email. Can not invite member of a .gov organization to a different organization.", + self.object, + exc_info=True, + ) + messages.error( + self.request, + f"{requested_email} is already a member of another .gov organization.", + ) + except AlreadyManagerError as e: + messages.warning(self.request, str(e)) + except AlreadyInvitedError as e: + messages.warning(self.request, str(e)) + except MissingEmailError as e: + messages.error(self.request, str(e)) + logger.error( + f"Can't send email to '{requested_email}' on domain '{self.object}'. " + f"No email exists for the requestor '{e.username}'.", + exc_info=True, + ) + except Exception: + logger.warn( + "Could not send email invitation (Other Exception)", + self.object, + exc_info=True, + ) + messages.warning(self.request, "Could not send email invitation.") + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") return redirect(self.get_success_url()) From d2d44bfea149127c746e3a12c5b1fda0b2c7aa83 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 13 Dec 2024 06:48:47 -0500 Subject: [PATCH 003/114] first refactor attempt, moving send_domain_invitation_email to a helper --- src/registrar/models/domain.py | 8 ++ src/registrar/utility/email_invitations.py | 97 +++++++++++++++ src/registrar/utility/errors.py | 28 +++++ src/registrar/views/domain.py | 137 +++------------------ 4 files changed, 150 insertions(+), 120 deletions(-) create mode 100644 src/registrar/utility/email_invitations.py diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cc600e1ce..bc3700409 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -7,6 +7,7 @@ from typing import Optional from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore from django.db import models +from django.urls import reverse from django.utils import timezone from typing import Any from registrar.models.host import Host @@ -1087,6 +1088,13 @@ class Domain(TimeStampedModel, DomainHelper): help_text="Record of the last change event for ds data", ) + def get_absolute_url(self): + """ + Returns the absolute URL for the domain instance. + This is the standard implementation for Django models. + """ + return reverse("domain", kwargs={"pk": self.pk}) + def isActive(self): return self.state == Domain.State.CREATED diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py new file mode 100644 index 000000000..06a99bd08 --- /dev/null +++ b/src/registrar/utility/email_invitations.py @@ -0,0 +1,97 @@ +from django.conf import settings +from registrar.models import DomainInvitation +from registrar.models.portfolio_invitation import PortfolioInvitation +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.utility.errors import ( + MissingEmailError, + AlreadyManagerError, + AlreadyInvitedError, + OutsideOrgMemberError, +) +from registrar.utility.waffle import flag_is_active_for_user +from registrar.utility.email import send_templated_email, EmailSendingError +import logging + +logger = logging.getLogger(__name__) + + +def _is_member_of_different_org(email, requestor, requested_user): + """Verifies if an email belongs to a different organization as a member or invited member.""" + # Check if requested_user is a already member of a different organization than the requestor's org + requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio + existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() + existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() + + return (existing_org_permission and existing_org_permission.portfolio != requestor_org) or ( + existing_org_invitation and existing_org_invitation.portfolio != requestor_org + ) + + +def send_domain_invitation_email(email: str, requestor, domain, requested_user=None): + """ + Sends a domain invitation email to the specified address. + + Raises exceptions for validation or email-sending issues. + + Args: + email (str): Email address of the recipient. + requestor (User): The user initiating the invitation. + domain (Domain): The domain object for which the invitation is being sent. + requested_user (User): The user of the recipient, if exists; defaults to None + + Raises: + MissingEmailError: If the requestor has no email associated with their account. + AlreadyManagerError: If the email corresponds to an existing domain manager. + AlreadyInvitedError: If an invitation has already been sent. + OutsideOrgMemberError: If the requested_user is part of a different organization. + EmailSendingError: If there is an error while sending the email. + """ + # Default email address for staff + requestor_email = settings.DEFAULT_FROM_EMAIL + + # Check if the requestor is staff and has an email + if not requestor.is_staff: + if not requestor.email or requestor.email.strip() == "": + raise MissingEmailError(requestor.username) + else: + requestor_email = requestor.email + + # Check if the recipient is part of a different organization + if flag_is_active_for_user(requestor, "organization_feature") and _is_member_of_different_org( + email, requestor, requested_user + ): + raise OutsideOrgMemberError + + # Check for an existing invitation + try: + invite = DomainInvitation.objects.get(email=email, domain=domain) + if invite.status == DomainInvitation.DomainInvitationStatus.RETRIEVED: + raise AlreadyManagerError(email) + elif invite.status == DomainInvitation.DomainInvitationStatus.CANCELED: + invite.update_cancellation_status() + invite.save() + else: + raise AlreadyInvitedError(email) + except DomainInvitation.DoesNotExist: + pass + + # Send the email + try: + send_templated_email( + "emails/domain_invitation.txt", + "emails/domain_invitation_subject.txt", + to_address=email, + context={ + "domain_url": domain.get_absolute_url(), + "domain": domain, + "requestor_email": requestor_email, + }, + ) + except EmailSendingError as exc: + logger.warning( + "Could not send email invitation to %s for domain %s", + email, + domain, + exc_info=True, + ) + raise EmailSendingError("Could not send email invitation.") from exc diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index e70f06d1e..3ba936c41 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -23,6 +23,34 @@ class InvalidDomainError(ValueError): pass +class InvitationError(Exception): + """Base exception for invitation-related errors.""" + + pass + + +class AlreadyManagerError(InvitationError): + """Raised when the user is already a manager for the domain.""" + + def __init__(self, email): + super().__init__(f"{email} is already a manager for this domain.") + + +class AlreadyInvitedError(InvitationError): + """Raised when the user has already been invited to the domain.""" + + def __init__(self, email): + super().__init__(f"{email} has already been invited to this domain.") + + +class MissingEmailError(InvitationError): + """Raised when the requestor has no email associated with their account.""" + + def __init__(self, username): + super().__init__(f"Can't send invitation email. No email is associated with the account for '{username}'.") + self.username = username + + class OutsideOrgMemberError(ValueError): """ Error raised when an org member tries adding a user from a different .gov org. diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 526cad53f..0d038d55b 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -25,14 +25,16 @@ from registrar.models import ( PortfolioInvitation, User, UserDomainRole, - UserPortfolioPermission, PublicContact, ) from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.enums import DefaultEmail from registrar.utility.errors import ( + AlreadyInvitedError, + AlreadyManagerError, GenericError, GenericErrorCodes, + MissingEmailError, NameserverError, NameserverErrorCodes as nsErrorCodes, DsDataError, @@ -63,40 +65,13 @@ from epplibwrapper import ( ) from ..utility.email import send_templated_email, EmailSendingError +from ..utility.email_invitations import send_domain_invitation_email from .utility import DomainPermissionView, DomainInvitationPermissionCancelView from django import forms logger = logging.getLogger(__name__) -class InvitationError(Exception): - """Base exception for invitation-related errors.""" - - pass - - -class AlreadyManagerError(InvitationError): - """Raised when the user is already a manager for the domain.""" - - def __init__(self, email): - super().__init__(f"{email} is already a manager for this domain.") - - -class AlreadyInvitedError(InvitationError): - """Raised when the user has already been invited to the domain.""" - - def __init__(self, email): - super().__init__(f"{email} has already been invited to this domain.") - - -class MissingEmailError(InvitationError): - """Raised when the requestor has no email associated with their account.""" - - def __init__(self, username): - super().__init__(f"Can't send invitation email. No email is associated with the account for '{username}'.") - self.username = username - - class DomainBaseView(DomainPermissionView): """ Base View for the Domain. Handles getting and setting the domain @@ -1211,93 +1186,8 @@ class DomainAddUserView(DomainFormBaseView): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.pk}) - def _domain_abs_url(self): - """Get an absolute URL for this domain.""" - return self.request.build_absolute_uri(reverse("domain", kwargs={"pk": self.object.id})) - - def _is_member_of_different_org(self, email, requestor, requested_user): - """Verifies if an email belongs to a different organization as a member or invited member.""" - # Check if user is a already member of a different organization than the requestor's org - requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio - existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() - existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() - - return (existing_org_permission and existing_org_permission.portfolio != requestor_org) or ( - existing_org_invitation and existing_org_invitation.portfolio != requestor_org - ) - - def _check_existing_invitation_status(self, email, domain): - """Check if existing invitation exists; checks its status for canceled, invited or retrieved, and gives the appropiate response. - - Exceptions raised for RETRIEVED or INVITED. Existing CANCELED invitations updated to INVITED. - Existing CANCELED invitations do not raise an exception.""" - try: - invite = DomainInvitation.objects.get(email=email, domain=domain) - except Exception as err: - # No existing invitation, do nothing - return - else: - if invite.status == DomainInvitation.DomainInvitationStatus.RETRIEVED: - raise AlreadyManagerError(email) - elif invite.status == DomainInvitation.DomainInvitationStatus.CANCELED: - invite.update_cancellation_status() - invite.save() - else: - # Status is INVITED - raise AlreadyInvitedError(email) - - def _send_domain_invitation_email(self, email: str, requestor: User, requested_user=None): - """Performs the sending of the domain invitation email, - does not make a domain information object - email: string- email to send to - - raises EmailSendingError - """ - - # Set a default email address to send to for staff - requestor_email = settings.DEFAULT_FROM_EMAIL - - # refactor below to raise an error - # if the user is not a staff member - if not requestor.is_staff: - if not requestor.email or requestor.email.strip() == "": - raise MissingEmailError(requestor.username) - else: - requestor_email = requestor.email - - # Check is user is a member or invited member of a different org from this domain's org - if flag_is_active_for_user(requestor, "organization_feature") and self._is_member_of_different_org( - email, requestor, requested_user - ): - raise OutsideOrgMemberError - - # Check to see if an invite has already been sent - self._check_existing_invitation_status(email=email, domain=self.object) - - try: - send_templated_email( - "emails/domain_invitation.txt", - "emails/domain_invitation_subject.txt", - to_address=email, - context={ - "domain_url": self._domain_abs_url(), - "domain": self.object, - "requestor_email": requestor_email, - }, - ) - except EmailSendingError as exc: - logger.warn( - "Could not sent email invitation to %s for domain %s", - email, - self.object, - exc_info=True, - ) - logger.info(exc) - raise EmailSendingError("Could not send email invitation.") from exc - def form_valid(self, form): - """Add the specified user on this domain. - Throws EmailSendingError.""" + """Add the specified user on this domain.""" requested_email = form.cleaned_data["email"] requestor = self.request.user @@ -1311,13 +1201,20 @@ class DomainAddUserView(DomainFormBaseView): try: if requested_user is None: # no user exists, send an email and make an invitation - self._send_domain_invitation_email(email=requested_email, requestor=requestor) + send_domain_invitation_email( + email=requested_email, + requestor=requestor, + domain=self.object, + ) DomainInvitation.objects.get_or_create(email=requested_email, domain=self.object) messages.success(self.request, f"{requested_email} has been invited to this domain.") else: # user exists, send email and make user domain role - self._send_domain_invitation_email( - email=requested_email, requestor=requestor, requested_user=requested_user + send_domain_invitation_email( + email=requested_email, + requestor=requestor, + requested_user=requested_user, + domain=self.object, ) UserDomainRole.objects.create( user=requested_user, @@ -1353,6 +1250,8 @@ class DomainAddUserView(DomainFormBaseView): f"No email exists for the requestor '{e.username}'.", exc_info=True, ) + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") except Exception: logger.warn( "Could not send email invitation (Other Exception)", @@ -1360,8 +1259,6 @@ class DomainAddUserView(DomainFormBaseView): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") return redirect(self.get_success_url()) From b4826ea18310333d5f3231d476ecb1a5cb04e022 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 13 Dec 2024 07:05:00 -0500 Subject: [PATCH 004/114] refactored form_valid to break into smaller chunks (to satisfy linter) --- src/registrar/views/domain.py | 113 ++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 54 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 0d038d55b..1f86bef27 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -1187,49 +1187,61 @@ class DomainAddUserView(DomainFormBaseView): return reverse("domain-users", kwargs={"pk": self.object.pk}) def form_valid(self, form): - """Add the specified user on this domain.""" + """Add the specified user to this domain.""" requested_email = form.cleaned_data["email"] requestor = self.request.user - # look up a user with that email - requested_user = None - try: - requested_user = User.objects.get(email=requested_email) - except User.DoesNotExist: - pass + # Look up a user with that email + requested_user = self._get_requested_user(requested_email) try: if requested_user is None: - # no user exists, send an email and make an invitation - send_domain_invitation_email( - email=requested_email, - requestor=requestor, - domain=self.object, - ) - DomainInvitation.objects.get_or_create(email=requested_email, domain=self.object) - messages.success(self.request, f"{requested_email} has been invited to this domain.") + self._handle_new_user_invitation(requested_email, requestor) else: - # user exists, send email and make user domain role - send_domain_invitation_email( - email=requested_email, - requestor=requestor, - requested_user=requested_user, - domain=self.object, - ) - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - messages.success(self.request, f"Added user {requested_email}.") - except EmailSendingError: - logger.warn( - "Could not send email invitation (EmailSendingError)", - self.object, - exc_info=True, - ) + self._handle_existing_user(requested_email, requestor, requested_user) + except Exception as e: + self._handle_exceptions(e, requested_email) + + return redirect(self.get_success_url()) + + def _get_requested_user(self, email): + """Retrieve a user by email or return None if the user doesn't exist.""" + try: + return User.objects.get(email=email) + except User.DoesNotExist: + return None + + def _handle_new_user_invitation(self, email, requestor): + """Handle invitation for a new user who does not exist in the system.""" + send_domain_invitation_email( + email=email, + requestor=requestor, + domain=self.object, + ) + DomainInvitation.objects.get_or_create(email=email, domain=self.object) + messages.success(self.request, f"{email} has been invited to this domain.") + + def _handle_existing_user(self, email, requestor, requested_user): + """Handle adding an existing user to the domain.""" + send_domain_invitation_email( + email=email, + requestor=requestor, + requested_user=requested_user, + domain=self.object, + ) + UserDomainRole.objects.create( + user=requested_user, + domain=self.object, + role=UserDomainRole.Roles.MANAGER, + ) + messages.success(self.request, f"Added user {email}.") + + def _handle_exceptions(self, exception, email): + """Handle exceptions raised during the process.""" + if isinstance(exception, EmailSendingError): + logger.warn("Could not send email invitation (EmailSendingError)", self.object, exc_info=True) messages.warning(self.request, "Could not send email invitation.") - except OutsideOrgMemberError: + elif isinstance(exception, OutsideOrgMemberError): logger.warn( "Could not send email. Can not invite member of a .gov organization to a different organization.", self.object, @@ -1237,31 +1249,24 @@ class DomainAddUserView(DomainFormBaseView): ) messages.error( self.request, - f"{requested_email} is already a member of another .gov organization.", + f"{email} is already a member of another .gov organization.", ) - except AlreadyManagerError as e: - messages.warning(self.request, str(e)) - except AlreadyInvitedError as e: - messages.warning(self.request, str(e)) - except MissingEmailError as e: - messages.error(self.request, str(e)) + elif isinstance(exception, AlreadyManagerError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, AlreadyInvitedError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, MissingEmailError): + messages.error(self.request, str(exception)) logger.error( - f"Can't send email to '{requested_email}' on domain '{self.object}'. " - f"No email exists for the requestor '{e.username}'.", - exc_info=True, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - except Exception: - logger.warn( - "Could not send email invitation (Other Exception)", - self.object, + f"Can't send email to '{email}' on domain '{self.object}'. No email exists for the requestor.", exc_info=True, ) + elif isinstance(exception, IntegrityError): + messages.warning(self.request, f"{email} is already a manager for this domain") + else: + logger.warn("Could not send email invitation (Other Exception)", self.object, exc_info=True) messages.warning(self.request, "Could not send email invitation.") - return redirect(self.get_success_url()) - class DomainInvitationCancelView(SuccessMessageMixin, DomainInvitationPermissionCancelView): object: DomainInvitation From dd32cb9e5d91545544b5206fa4a693c1534b4419 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 13 Dec 2024 09:46:43 -0500 Subject: [PATCH 005/114] portfolio invitation emails refactored and implemented in helper --- src/registrar/models/domain.py | 7 - .../templates/emails/portfolio_invitation.txt | 34 +++++ .../emails/portfolio_invitation_subject.txt | 1 + src/registrar/utility/email_invitations.py | 75 +++++++++- src/registrar/utility/errors.py | 18 ++- src/registrar/views/domain.py | 8 +- src/registrar/views/portfolios.py | 131 +++++------------- 7 files changed, 156 insertions(+), 118 deletions(-) create mode 100644 src/registrar/templates/emails/portfolio_invitation.txt create mode 100644 src/registrar/templates/emails/portfolio_invitation_subject.txt diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index bc3700409..aa2e5f32b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1088,13 +1088,6 @@ class Domain(TimeStampedModel, DomainHelper): help_text="Record of the last change event for ds data", ) - def get_absolute_url(self): - """ - Returns the absolute URL for the domain instance. - This is the standard implementation for Django models. - """ - return reverse("domain", kwargs={"pk": self.pk}) - def isActive(self): return self.state == Domain.State.CREATED diff --git a/src/registrar/templates/emails/portfolio_invitation.txt b/src/registrar/templates/emails/portfolio_invitation.txt new file mode 100644 index 000000000..775b74c7c --- /dev/null +++ b/src/registrar/templates/emails/portfolio_invitation.txt @@ -0,0 +1,34 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi. + +{{ requestor_email }} has invited you to {{ portfolio.organization_name }}. + +You can view this organization on the .gov registrar . + +---------------------------------------------------------------- + +YOU NEED A LOGIN.GOV ACCOUNT +You’ll need a Login.gov account to access this .gov organization. That account +needs to be associated with the following email address: {{ email }} + +Login.gov provides a simple and secure process for signing in to many government +services with one account. If you don’t already have one, follow these steps to +create your Login.gov account . + + +SOMETHING WRONG? +If you’re not affiliated with {{ portfolio.organization_name }} or think you received this +message in error, reply to this email. + + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Learn about .gov + +The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency (CISA) +{% endautoescape %} diff --git a/src/registrar/templates/emails/portfolio_invitation_subject.txt b/src/registrar/templates/emails/portfolio_invitation_subject.txt new file mode 100644 index 000000000..552bb2bec --- /dev/null +++ b/src/registrar/templates/emails/portfolio_invitation_subject.txt @@ -0,0 +1 @@ +You’ve been invited to a .gov organization \ No newline at end of file diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 06a99bd08..af5a8998e 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -3,9 +3,11 @@ from registrar.models import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.utility.errors import ( + AlreadyDomainInvitedError, + AlreadyDomainManagerError, + AlreadyPortfolioInvitedError, + AlreadyPortfolioMemberError, MissingEmailError, - AlreadyManagerError, - AlreadyInvitedError, OutsideOrgMemberError, ) from registrar.utility.waffle import flag_is_active_for_user @@ -41,8 +43,8 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N Raises: MissingEmailError: If the requestor has no email associated with their account. - AlreadyManagerError: If the email corresponds to an existing domain manager. - AlreadyInvitedError: If an invitation has already been sent. + AlreadyDomainManagerError: If the email corresponds to an existing domain manager. + AlreadyDomainInvitedError: If an invitation has already been sent. OutsideOrgMemberError: If the requested_user is part of a different organization. EmailSendingError: If there is an error while sending the email. """ @@ -66,12 +68,12 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N try: invite = DomainInvitation.objects.get(email=email, domain=domain) if invite.status == DomainInvitation.DomainInvitationStatus.RETRIEVED: - raise AlreadyManagerError(email) + raise AlreadyDomainManagerError(email) elif invite.status == DomainInvitation.DomainInvitationStatus.CANCELED: invite.update_cancellation_status() invite.save() else: - raise AlreadyInvitedError(email) + raise AlreadyDomainInvitedError(email) except DomainInvitation.DoesNotExist: pass @@ -82,7 +84,6 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N "emails/domain_invitation_subject.txt", to_address=email, context={ - "domain_url": domain.get_absolute_url(), "domain": domain, "requestor_email": requestor_email, }, @@ -95,3 +96,63 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N exc_info=True, ) raise EmailSendingError("Could not send email invitation.") from exc + + +def send_portfolio_invitation_email(email: str, requestor, portfolio): + """ + Sends a portfolio member invitation email to the specified address. + + Raises exceptions for validation or email-sending issues. + + Args: + email (str): Email address of the recipient + requestor (User): The user initiating the invitation. + portfolio (Portfolio): The portfolio object for which the invitation is being sent. + + Raises: + MissingEmailError: If the requestor has no email associated with their account. + AlreadyPortfolioMemberError: If the email corresponds to an existing portfolio member. + AlreadyPortfolioInvitedError: If an invitation has already been sent. + EmailSendingError: If there is an error while sending the email. + """ + + # Default email address for staff + requestor_email = settings.DEFAULT_FROM_EMAIL + + # Check if the requestor is staff and has an email + if not requestor.is_staff: + if not requestor.email or requestor.email.strip() == "": + raise MissingEmailError(requestor.username) + else: + requestor_email = requestor.email + + # Check to see if an invite has already been sent + try: + invite = PortfolioInvitation.objects.get(email=email, portfolio=portfolio) + if invite.status == PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED: + raise AlreadyPortfolioMemberError(email) + else: + raise AlreadyPortfolioInvitedError(email) + except PortfolioInvitation.DoesNotExist: + pass + + try: + send_templated_email( + "emails/portfolio_invitation.txt", + "emails/portfolio_invitation_subject.txt", + to_address=email, + context={ + "portfolio": portfolio, + "requestor_email": requestor_email, + "email": email, + }, + ) + except EmailSendingError as exc: + logger.warning( + "Could not sent email invitation to %s for portfolio %s", + email, + portfolio, + exc_info=True, + ) + raise EmailSendingError("Could not send email invitation.") from exc + diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 3ba936c41..2ad95a99c 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -29,20 +29,34 @@ class InvitationError(Exception): pass -class AlreadyManagerError(InvitationError): +class AlreadyDomainManagerError(InvitationError): """Raised when the user is already a manager for the domain.""" def __init__(self, email): super().__init__(f"{email} is already a manager for this domain.") -class AlreadyInvitedError(InvitationError): +class AlreadyDomainInvitedError(InvitationError): """Raised when the user has already been invited to the domain.""" def __init__(self, email): super().__init__(f"{email} has already been invited to this domain.") +class AlreadyPortfolioMemberError(InvitationError): + """Raised when the user is already a member of the portfolio.""" + + def __init__(self, email): + super().__init__(f"{email} is already a manager for this portfolio.") + + +class AlreadyPortfolioInvitedError(InvitationError): + """Raised when the user has already been invited to the portfolio.""" + + def __init__(self, email): + super().__init__(f"{email} has already been invited to this portfolio.") + + class MissingEmailError(InvitationError): """Raised when the requestor has no email associated with their account.""" diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1f86bef27..eccc49aad 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -30,8 +30,8 @@ from registrar.models import ( from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.enums import DefaultEmail from registrar.utility.errors import ( - AlreadyInvitedError, - AlreadyManagerError, + AlreadyDomainInvitedError, + AlreadyDomainManagerError, GenericError, GenericErrorCodes, MissingEmailError, @@ -1251,9 +1251,9 @@ class DomainAddUserView(DomainFormBaseView): self.request, f"{email} is already a member of another .gov organization.", ) - elif isinstance(exception, AlreadyManagerError): + elif isinstance(exception, AlreadyDomainManagerError): messages.warning(self.request, str(exception)) - elif isinstance(exception, AlreadyInvitedError): + elif isinstance(exception, AlreadyDomainInvitedError): messages.warning(self.request, str(exception)) elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 880472509..c1a45208e 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -13,6 +13,8 @@ from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError +from registrar.utility.email_invitations import send_portfolio_invitation_email +from registrar.utility.errors import AlreadyPortfolioInvitedError, AlreadyPortfolioMemberError, MissingEmailError from registrar.views.utility.mixins import PortfolioMemberPermission from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, @@ -521,108 +523,41 @@ class NewMemberView(PortfolioMembersPermissionView, FormMixin): """Redirect to members table.""" return reverse("members") - def _send_portfolio_invitation_email(self, email: str, requestor: User, add_success=True): - """Performs the sending of the member invitation email - email: string- email to send to - add_success: bool- default True indicates: - adding a success message to the view if the email sending succeeds - - raises EmailSendingError - """ - - # Set a default email address to send to for staff - requestor_email = settings.DEFAULT_FROM_EMAIL - - # Check if the email requestor has a valid email address - if not requestor.is_staff and requestor.email is not None and requestor.email.strip() != "": - requestor_email = requestor.email - elif not requestor.is_staff: - messages.error(self.request, "Can't send invitation email. No email is associated with your account.") - logger.error( - f"Can't send email to '{email}' on domain '{self.object}'." - f"No email exists for the requestor '{requestor.username}'.", - exc_info=True, - ) - return None - - # Check to see if an invite has already been sent - try: - invite = PortfolioInvitation.objects.get(email=email, portfolio=self.object) - if invite: # We have an existin invite - # check if the invite has already been accepted - if invite.status == PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED: - add_success = False - messages.warning( - self.request, - f"{email} is already a manager for this portfolio.", - ) - else: - add_success = False - # it has been sent but not accepted - messages.warning(self.request, f"{email} has already been invited to this portfolio") - return - except Exception as err: - logger.error(f"_send_portfolio_invitation_email() => An error occured: {err}") - - try: - logger.debug("requestor email: " + requestor_email) - - # send_templated_email( - # "emails/portfolio_invitation.txt", - # "emails/portfolio_invitation_subject.txt", - # to_address=email, - # context={ - # "portfolio": self.object, - # "requestor_email": requestor_email, - # }, - # ) - except EmailSendingError as exc: - logger.warn( - "Could not sent email invitation to %s for domain %s", - email, - self.object, - exc_info=True, - ) - raise EmailSendingError("Could not send email invitation.") from exc - else: - if add_success: - messages.success(self.request, f"{email} has been invited.") - - def _make_invitation(self, email_address: str, requestor: User, add_success=True): - """Make a Member invitation for this email and redirect with a message.""" - try: - self._send_portfolio_invitation_email(email=email_address, requestor=requestor, add_success=add_success) - except EmailSendingError: - logger.warn( - "Could not send email invitation (EmailSendingError)", - self.object, - exc_info=True, - ) - messages.warning(self.request, "Could not send email invitation.") - except Exception: - logger.warn( - "Could not send email invitation (Other Exception)", - self.object, - exc_info=True, - ) - messages.warning(self.request, "Could not send email invitation.") - else: - # (NOTE: only create a MemberInvitation if the e-mail sends correctly) - PortfolioInvitation.objects.get_or_create(email=email_address, portfolio=self.object) - return redirect(self.get_success_url()) - def submit_new_member(self, form): - """Add the specified user as a member - for this portfolio. - Throws EmailSendingError.""" + """Add the specified user as a member for this portfolio.""" requested_email = form.cleaned_data["email"] requestor = self.request.user requested_user = User.objects.filter(email=requested_email).first() permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=self.object).exists() - if not requested_user or not permission_exists: - return self._make_invitation(requested_email, requestor) - else: - if permission_exists: - messages.warning(self.request, "User is already a member of this portfolio.") + # invitation_exists = PortfolioInvitation.objects.filter(email=requested_email, portfolio=self.object).exists() + try: + if not requested_user or not permission_exists: + send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=self.object) + PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=self.object) + messages.success(self.request, f"{requested_email} has been invited.") + else: + if permission_exists: + messages.warning(self.request, "User is already a member of this portfolio.") + except Exception as e: + self._handle_exceptions(e, requested_email) return redirect(self.get_success_url()) + + def _handle_exceptions(self, exception, email): + """Handle exceptions raised during the process.""" + if isinstance(exception, EmailSendingError): + logger.warning("Could not send email invitation (EmailSendingError)", self.object, exc_info=True) + messages.warning(self.request, "Could not send email invitation.") + elif isinstance(exception, AlreadyPortfolioMemberError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, AlreadyPortfolioInvitedError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, MissingEmailError): + messages.error(self.request, str(exception)) + logger.error( + f"Can't send email to '{email}' for portfolio '{self.object}'. No email exists for the requestor.", + exc_info=True, + ) + else: + logger.warning("Could not send email invitation (Other Exception)", self.object, exc_info=True) + messages.warning(self.request, "Could not send email invitation.") From ddf7d92b4b9434718dc7c8c3e32b50efadeeb0fe Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 10:59:58 -0500 Subject: [PATCH 006/114] moving around logic, prepping for refactor --- src/registrar/utility/email_invitations.py | 80 ++++++------------ src/registrar/views/domain.py | 98 ++++++++++++++++++++-- src/registrar/views/portfolios.py | 4 +- 3 files changed, 117 insertions(+), 65 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index af5a8998e..1c076493a 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -17,19 +17,7 @@ import logging logger = logging.getLogger(__name__) -def _is_member_of_different_org(email, requestor, requested_user): - """Verifies if an email belongs to a different organization as a member or invited member.""" - # Check if requested_user is a already member of a different organization than the requestor's org - requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio - existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() - existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() - - return (existing_org_permission and existing_org_permission.portfolio != requestor_org) or ( - existing_org_invitation and existing_org_invitation.portfolio != requestor_org - ) - - -def send_domain_invitation_email(email: str, requestor, domain, requested_user=None): +def send_domain_invitation_email(email: str, requestor, domain, is_member_of_different_org): """ Sends a domain invitation email to the specified address. @@ -39,7 +27,7 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N email (str): Email address of the recipient. requestor (User): The user initiating the invitation. domain (Domain): The domain object for which the invitation is being sent. - requested_user (User): The user of the recipient, if exists; defaults to None + is_member_of_different_org (bool): if an email belongs to a different org Raises: MissingEmailError: If the requestor has no email associated with their account. @@ -59,8 +47,11 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N requestor_email = requestor.email # Check if the recipient is part of a different organization - if flag_is_active_for_user(requestor, "organization_feature") and _is_member_of_different_org( - email, requestor, requested_user + # COMMENT: this does not account for multiple_portfolios flag being active + if ( + flag_is_active_for_user(requestor, "organization_feature") + and not flag_is_active_for_user(requestor, "multiple_portfolios") + and is_member_of_different_org ): raise OutsideOrgMemberError @@ -78,24 +69,15 @@ def send_domain_invitation_email(email: str, requestor, domain, requested_user=N pass # Send the email - try: - send_templated_email( - "emails/domain_invitation.txt", - "emails/domain_invitation_subject.txt", - to_address=email, - context={ - "domain": domain, - "requestor_email": requestor_email, - }, - ) - except EmailSendingError as exc: - logger.warning( - "Could not send email invitation to %s for domain %s", - email, - domain, - exc_info=True, - ) - raise EmailSendingError("Could not send email invitation.") from exc + send_templated_email( + "emails/domain_invitation.txt", + "emails/domain_invitation_subject.txt", + to_address=email, + context={ + "domain": domain, + "requestor_email": requestor_email, + }, + ) def send_portfolio_invitation_email(email: str, requestor, portfolio): @@ -136,23 +118,13 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): except PortfolioInvitation.DoesNotExist: pass - try: - send_templated_email( - "emails/portfolio_invitation.txt", - "emails/portfolio_invitation_subject.txt", - to_address=email, - context={ - "portfolio": portfolio, - "requestor_email": requestor_email, - "email": email, - }, - ) - except EmailSendingError as exc: - logger.warning( - "Could not sent email invitation to %s for portfolio %s", - email, - portfolio, - exc_info=True, - ) - raise EmailSendingError("Could not send email invitation.") from exc - + send_templated_email( + "emails/portfolio_invitation.txt", + "emails/portfolio_invitation_subject.txt", + to_address=email, + context={ + "portfolio": portfolio, + "requestor_email": requestor_email, + "email": email, + }, + ) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index eccc49aad..f0b0d5085 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -27,11 +27,14 @@ from registrar.models import ( UserDomainRole, PublicContact, ) +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.enums import DefaultEmail from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, + AlreadyPortfolioInvitedError, + AlreadyPortfolioMemberError, GenericError, GenericErrorCodes, MissingEmailError, @@ -65,7 +68,7 @@ from epplibwrapper import ( ) from ..utility.email import send_templated_email, EmailSendingError -from ..utility.email_invitations import send_domain_invitation_email +from ..utility.email_invitations import send_domain_invitation_email, send_portfolio_invitation_email from .utility import DomainPermissionView, DomainInvitationPermissionCancelView from django import forms @@ -1117,6 +1120,7 @@ class DomainUsersView(DomainBaseView): # Check if there are any PortfolioInvitations linked to the same portfolio with the ORGANIZATION_ADMIN role has_admin_flag = False + logger.info(domain_invitation) # Query PortfolioInvitations linked to the same portfolio and check roles portfolio_invitations = PortfolioInvitation.objects.filter( portfolio=portfolio, email=domain_invitation.email @@ -1124,7 +1128,9 @@ class DomainUsersView(DomainBaseView): # If any of the PortfolioInvitations have the ORGANIZATION_ADMIN role, set the flag to True for portfolio_invitation in portfolio_invitations: - if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_invitation.roles: + logger.info(portfolio_invitation) + logger.info(portfolio_invitation.roles) + if portfolio_invitation.roles and UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_invitation.roles: has_admin_flag = True break # Once we find one match, no need to check further @@ -1186,6 +1192,38 @@ class DomainAddUserView(DomainFormBaseView): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.pk}) + def _get_org_membership(self, requestor_org, requested_email, requested_user): + """ + Verifies if an email belongs to a different organization as a member or invited member. + Verifies if an email belongs to this organization as a member or invited member. + User does not belong to any org can be deduced from the tuple returned. + + Returns a tuple (member_of_a_different_org, member_of_this_org). + """ + + # COMMENT: this code does not take into account multiple portfolios flag + + # COMMENT: shouldn't this code be based on the organization of the domain, not the org + # of the requestor? requestor could have multiple portfolios + + # Check for existing permissions or invitations for the requested user + existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() + existing_org_invitation = PortfolioInvitation.objects.filter(email=requested_email).first() + + # Determine membership in a different organization + member_of_a_different_org = ( + (existing_org_permission and existing_org_permission.portfolio != requestor_org) or + (existing_org_invitation and existing_org_invitation.portfolio != requestor_org) + ) + + # Determine membership in the same organization + member_of_this_org = ( + (existing_org_permission and existing_org_permission.portfolio == requestor_org) or + (existing_org_invitation and existing_org_invitation.portfolio == requestor_org) + ) + + return member_of_a_different_org, member_of_this_org + def form_valid(self, form): """Add the specified user to this domain.""" requested_email = form.cleaned_data["email"] @@ -1193,12 +1231,34 @@ class DomainAddUserView(DomainFormBaseView): # Look up a user with that email requested_user = self._get_requested_user(requested_email) + # Get the requestor's organization + requestor_org = UserPortfolioPermission.objects.filter(user=requestor).first().portfolio + + member_of_a_different_org, member_of_this_org = self._get_org_membership(requestor_org, requested_email, requested_user) + + # determine portfolio of the domain (code currently is looking at requestor's portfolio) + # if requested_email/user is not member or invited member of this portfolio + # COMMENT: this code does not take into account multiple portfolios flag + # send portfolio invitation email + # create portfolio invitation + # create message to view + if ( + flag_is_active_for_user(requestor, "organization_feature") + and not flag_is_active_for_user(requestor, "multiple_portfolios") + and not member_of_this_org + ): + try: + send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=requestor_org) + PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=requestor_org) + messages.success(self.request, f"{requested_email} has been invited.") + except Exception as e: + self._handle_portfolio_exceptions(e, requested_email, requestor_org) try: if requested_user is None: - self._handle_new_user_invitation(requested_email, requestor) + self._handle_new_user_invitation(requested_email, requestor, member_of_a_different_org) else: - self._handle_existing_user(requested_email, requestor, requested_user) + self._handle_existing_user(requested_email, requestor, requested_user, member_of_a_different_org) except Exception as e: self._handle_exceptions(e, requested_email) @@ -1211,23 +1271,25 @@ class DomainAddUserView(DomainFormBaseView): except User.DoesNotExist: return None - def _handle_new_user_invitation(self, email, requestor): + 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( email=email, requestor=requestor, domain=self.object, + is_member_of_different_org=member_of_different_org, ) DomainInvitation.objects.get_or_create(email=email, domain=self.object) messages.success(self.request, f"{email} has been invited to this domain.") - def _handle_existing_user(self, email, requestor, requested_user): + 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( email=email, requestor=requestor, requested_user=requested_user, domain=self.object, + is_member_of_different_org=member_of_different_org, ) UserDomainRole.objects.create( user=requested_user, @@ -1239,10 +1301,10 @@ class DomainAddUserView(DomainFormBaseView): def _handle_exceptions(self, exception, email): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): - logger.warn("Could not send email invitation (EmailSendingError)", self.object, exc_info=True) + logger.warning("Could not send email invitation to %s for domain %s (EmailSendingError)", email, self.object, exc_info=True) messages.warning(self.request, "Could not send email invitation.") elif isinstance(exception, OutsideOrgMemberError): - logger.warn( + logger.warning( "Could not send email. Can not invite member of a .gov organization to a different organization.", self.object, exc_info=True, @@ -1264,9 +1326,27 @@ class DomainAddUserView(DomainFormBaseView): elif isinstance(exception, IntegrityError): messages.warning(self.request, f"{email} is already a manager for this domain") else: - logger.warn("Could not send email invitation (Other Exception)", self.object, exc_info=True) + logger.warning("Could not send email invitation (Other Exception)", self.object, exc_info=True) messages.warning(self.request, "Could not send email invitation.") + def _handle_portfolio_exceptions(self, exception, email, portfolio): + """Handle exceptions raised during the process.""" + if isinstance(exception, EmailSendingError): + logger.warning("Could not send email invitation (EmailSendingError)", portfolio, exc_info=True) + messages.warning(self.request, "Could not send email invitation.") + elif isinstance(exception, AlreadyPortfolioMemberError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, AlreadyPortfolioInvitedError): + messages.warning(self.request, str(exception)) + elif isinstance(exception, MissingEmailError): + messages.error(self.request, str(exception)) + logger.error( + f"Can't send email to '{email}' for portfolio '{portfolio}'. No email exists for the requestor.", + exc_info=True, + ) + else: + logger.warning("Could not send email invitation (Other Exception)", portfolio, exc_info=True) + messages.warning(self.request, "Could not send email invitation.") class DomainInvitationCancelView(SuccessMessageMixin, DomainInvitationPermissionCancelView): object: DomainInvitation diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index c1a45208e..dae2cc9fe 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -530,10 +530,10 @@ class NewMemberView(PortfolioMembersPermissionView, FormMixin): requested_user = User.objects.filter(email=requested_email).first() permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=self.object).exists() - # invitation_exists = PortfolioInvitation.objects.filter(email=requested_email, portfolio=self.object).exists() try: if not requested_user or not permission_exists: send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=self.object) + ## NOTE : this is not yet accounting properly for roles and permissions PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=self.object) messages.success(self.request, f"{requested_email} has been invited.") else: @@ -546,7 +546,7 @@ class NewMemberView(PortfolioMembersPermissionView, FormMixin): def _handle_exceptions(self, exception, email): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): - logger.warning("Could not send email invitation (EmailSendingError)", self.object, exc_info=True) + logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", email, self.object, exc_info=True) messages.warning(self.request, "Could not send email invitation.") elif isinstance(exception, AlreadyPortfolioMemberError): messages.warning(self.request, str(exception)) From 36f3d3e8a909588b06ea5efc1e78e6e1c2e4ee0a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 16 Dec 2024 10:25:25 -0600 Subject: [PATCH 007/114] add warning to domain requests when status cannot be changed --- src/registrar/admin.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0e8e4847a..5e8148664 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2543,6 +2543,31 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Further filter the queryset by the portfolio qs = qs.filter(portfolio=portfolio_id) return qs + + def change_view(self, request, object_id, form_url="", extra_context=None): + """Extend the change_view for DomainRequest objects in django admin. + Customize to display notification that statu cannot be changed from 'Approved'.""" + + # Fetch the Contact instance + domain_request: models.DomainRequest = models.DomainRequest.objects.get(pk=object_id) + if domain_request.approved_domain and domain_request.approved_domain.state == models.Domain.State.READY: + domain = domain_request.approved_domain + # get change url for domain + app_label = domain_request.approved_domain._meta.app_label + model_name = domain._meta.model_name + obj_id = domain.id + change_url = reverse("admin:%s_%s_change" % (app_label, model_name), args=[obj_id]) + + message += f"

The status of this domain request cannot be changed because it has been joined to a domain in Ready status: " # noqa + message += f"{domain}

" + + message_html = mark_safe(message) # nosec + messages.warning( + request, + message_html, + ) + + return super().change_view(request, object_id, form_url, extra_context=extra_context) class TransitionDomainAdmin(ListHeaderAdmin): From d888c616b8f8a87ca1ce54bb7e13b945ae06b5a1 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 13:13:41 -0500 Subject: [PATCH 008/114] wip --- src/registrar/admin.py | 105 ++++++++++++++++++++++++++++++- src/registrar/forms/portfolio.py | 2 +- 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0e8e4847a..e7bab8405 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -14,6 +14,7 @@ from django.db.models import ( from django.db.models.functions import Concat, Coalesce from django.http import HttpResponseRedirect from registrar.models.federal_agency import FederalAgency +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.utility.admin_helpers import ( AutocompleteSelectWithPlaceholder, get_action_needed_reason_default_email, @@ -21,10 +22,14 @@ from registrar.utility.admin_helpers import ( get_field_links_as_list, ) from django.conf import settings +from django.contrib.messages import get_messages +from django.contrib.admin.helpers import AdminForm from django.shortcuts import redirect from django_fsm import get_available_FIELD_transitions, FSMField from registrar.models import DomainInformation, Portfolio, UserPortfolioPermission, DomainInvitation from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.utility.email import EmailSendingError +from registrar.utility.email_invitations import send_portfolio_invitation_email from waffle.decorators import flag_is_active from django.contrib import admin, messages from django.contrib.auth.admin import UserAdmin as BaseUserAdmin @@ -37,7 +42,7 @@ from waffle.admin import FlagAdmin from waffle.models import Sample, Switch from registrar.models import Contact, Domain, DomainRequest, DraftDomain, User, Website, SeniorOfficial from registrar.utility.constants import BranchChoices -from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes +from registrar.utility.errors import AlreadyPortfolioInvitedError, AlreadyPortfolioMemberError, FSMDomainRequestError, FSMErrorCodes, MissingEmailError from registrar.utility.waffle import flag_is_active_for_user from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR @@ -1461,6 +1466,104 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): extra_context["tabtitle"] = "Portfolio invitations" # Get the filtered values return super().changelist_view(request, extra_context=extra_context) + + def save_model(self, request, obj, form, change): + """ + Override the save_model method to send an email only on creation of the PortfolioInvitation object. + """ + if not change: # Only send email if this is a new PortfolioInvitation(creation) + portfolio = obj.portfolio + requested_email = obj.email + requestor = request.user + + requested_user = User.objects.filter(email=requested_email).first() + permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() + try: + if not requested_user or not permission_exists: + send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) + messages.success(request, f"{requested_email} has been invited.") + else: + if permission_exists: + messages.warning(request, "User is already a member of this portfolio.") + except Exception as e: + self._handle_exceptions(e, request, obj) + return + # Call the parent save method to save the object + super().save_model(request, obj, form, change) + + def _handle_exceptions(self, exception, request, obj): + """Handle exceptions raised during the process.""" + if isinstance(exception, EmailSendingError): + logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", obj.email, obj.portfolio, exc_info=True) + messages.warning(request, "Could not send email invitation.") + elif isinstance(exception, AlreadyPortfolioMemberError): + messages.warning(request, str(exception)) + elif isinstance(exception, AlreadyPortfolioInvitedError): + messages.warning(request, str(exception)) + elif isinstance(exception, MissingEmailError): + messages.error(request, str(exception)) + logger.error( + f"Can't send email to '{obj.email}' for portfolio '{obj.portfolio}'. No email exists for the requestor.", + exc_info=True, + ) + else: + logger.warning("Could not send email invitation (Other Exception)", obj.portfolio, exc_info=True) + messages.warning(request, "Could not send email invitation.") + + def response_add(self, request, obj, post_url_continue=None): + """ + Override response_add to handle redirection when exceptions are raised. + """ + # Check if there are any error or warning messages in the `messages` framework + storage = get_messages(request) + has_errors = any(message.level_tag in ["error", "warning"] for message in storage) + + if has_errors: + # Re-render the change form if there are errors or warnings + # Prepare context for rendering the change form + opts = self.model._meta + app_label = opts.app_label + + # Get the model form + ModelForm = self.get_form(request, obj=obj) + form = ModelForm(instance=obj) + + # Create an AdminForm instance + admin_form = AdminForm( + form, + list(self.get_fieldsets(request, obj)), + self.prepopulated_fields, + self.get_readonly_fields(request, obj), + model_admin=self, + ) + + opts = obj._meta + change_form_context = { + **self.admin_site.each_context(request), # Add admin context + "title": f"Change {opts.verbose_name}", + "opts": opts, + "original": obj, + "save_as": self.save_as, + "has_change_permission": self.has_change_permission(request, obj), + "add": False, # Indicate this is not an "Add" form + "change": True, # Indicate this is a "Change" form + "is_popup": False, + "inline_admin_formsets": [], + "save_on_top": self.save_on_top, + "show_delete": self.has_delete_permission(request, obj), + "obj": obj, + "adminform": admin_form, # Pass the AdminForm instance + "errors": None, # You can use this to pass custom form errors + } + return self.render_change_form( + request, + context=change_form_context, + add=False, + change=True, + obj=obj, + ) + + return super().response_add(request, obj, post_url_continue) class DomainInformationResource(resources.ModelResource): diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 5309f7263..5dd6fdb3e 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -223,7 +223,7 @@ class NewMemberForm(forms.ModelForm): ) class Meta: - model = User + model = PortfolioInvitation fields = ["email"] def clean(self): From f09627bca6268c05d4515cb97335a2cd57ce2f1d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 14:28:17 -0500 Subject: [PATCH 009/114] work in progress tracing current form implementation --- src/registrar/config/urls.py | 2 +- src/registrar/forms/portfolio.py | 6 +- src/registrar/models/portfolio.py | 6 ++ src/registrar/models/portfolio_invitation.py | 4 + .../models/utility/portfolio_helper.py | 3 + src/registrar/views/portfolios.py | 74 ++++++++++--------- 6 files changed, 57 insertions(+), 38 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index caf51cc36..d71645dee 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -136,7 +136,7 @@ urlpatterns = [ # ), path( "members/new-member/", - views.NewMemberView.as_view(), + views.PortfolioNewMemberView.as_view(), name="new-member", ), path( diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 5dd6fdb3e..14b00529f 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -164,7 +164,7 @@ class PortfolioInvitedMemberForm(forms.ModelForm): ] -class NewMemberForm(forms.ModelForm): +class PortfolioNewMemberForm(forms.ModelForm): member_access_level = forms.ChoiceField( label="Select permission", choices=[("admin", "Admin Access"), ("basic", "Basic Access")], @@ -226,6 +226,10 @@ class NewMemberForm(forms.ModelForm): model = PortfolioInvitation fields = ["email"] + def _post_clean(self): + logger.info("in _post_clean") + super()._post_clean() + def clean(self): cleaned_data = super().clean() diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index 82afcd4d6..633f27126 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -6,7 +6,9 @@ from registrar.models.user import User from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel +import logging +logger = logging.getLogger(__name__) class Portfolio(TimeStampedModel): """ @@ -163,3 +165,7 @@ class Portfolio(TimeStampedModel): def get_suborganizations(self): """Returns all suborganizations associated with this portfolio""" return self.portfolio_suborganizations.all() + + def full_clean(self, exclude=None, validate_unique=True): + logger.info("portfolio full clean") + super().full_clean(exclude, validate_unique) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 11c564c36..92f527377 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -111,6 +111,10 @@ class PortfolioInvitation(TimeStampedModel): user_portfolio_permission.additional_permissions = self.additional_permissions user_portfolio_permission.save() + def full_clean(self, exclude=None, validate_unique=True): + logger.info("portfolio invitation full clean") + super().full_clean(exclude, validate_unique) + def clean(self): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 3768aa77a..396e6ab69 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -4,7 +4,9 @@ from django.apps import apps from django.forms import ValidationError from registrar.utility.waffle import flag_is_active_for_user from django.contrib.auth import get_user_model +import logging +logger = logging.getLogger(__name__) class UserPortfolioRoleChoices(models.TextChoices): """ @@ -153,6 +155,7 @@ def validate_portfolio_invitation(portfolio_invitation): Raises: ValidationError: If any of the validation rules are violated. """ + logger.info("portfolio invitataion validation") PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") User = get_user_model() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index dae2cc9fe..84fa9682b 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -469,33 +469,34 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): return render(request, "portfolio_members.html") -class NewMemberView(PortfolioMembersPermissionView, FormMixin): +class PortfolioNewMemberView(PortfolioMembersPermissionView, FormMixin): template_name = "portfolio_members_add_new.html" - form_class = portfolioForms.NewMemberForm + form_class = portfolioForms.PortfolioNewMemberForm - def get_object(self, queryset=None): - """Get the portfolio object based on the session.""" - portfolio = self.request.session.get("portfolio") - if portfolio is None: - raise Http404("No organization found for this user") - return portfolio + # def get_object(self, queryset=None): + # """Get the portfolio object based on the session.""" + # portfolio = self.request.session.get("portfolio") + # if portfolio is None: + # raise Http404("No organization found for this user") + # return portfolio - def get_form_kwargs(self): - """Include the instance in the form kwargs.""" - kwargs = super().get_form_kwargs() - kwargs["instance"] = self.get_object() - return kwargs + # def get_form_kwargs(self): + # """Include the instance in the form kwargs.""" + # kwargs = super().get_form_kwargs() + # kwargs["instance"] = self.get_object() + # return kwargs def get(self, request, *args, **kwargs): """Handle GET requests to display the form.""" - self.object = self.get_object() + self.object = self.request.session.get("portfolio") form = self.get_form() return self.render_to_response(self.get_context_data(form=form)) def post(self, request, *args, **kwargs): """Handle POST requests to process form submission.""" - self.object = self.get_object() + + # self.object = self.get_object() form = self.get_form() if form.is_valid(): @@ -503,50 +504,51 @@ class NewMemberView(PortfolioMembersPermissionView, FormMixin): else: return self.form_invalid(form) - def is_ajax(self): - return self.request.headers.get("X-Requested-With") == "XMLHttpRequest" + # def is_ajax(self): + # return self.request.headers.get("X-Requested-With") == "XMLHttpRequest" - def form_invalid(self, form): - if self.is_ajax(): - return JsonResponse({"is_valid": False}) # Return a JSON response - else: - return super().form_invalid(form) # Handle non-AJAX requests normally + # def form_invalid(self, form): + # if self.is_ajax(): + # return JsonResponse({"is_valid": False}) # Return a JSON response + # else: + # return super().form_invalid(form) # Handle non-AJAX requests normally - def form_valid(self, form): + # def form_valid(self, form): - if self.is_ajax(): - return JsonResponse({"is_valid": True}) # Return a JSON response - else: - return self.submit_new_member(form) + # if self.is_ajax(): + # return JsonResponse({"is_valid": True}) # Return a JSON response + # else: + # return self.submit_new_member(form) def get_success_url(self): """Redirect to members table.""" return reverse("members") - def submit_new_member(self, form): + def form_valid(self, form): """Add the specified user as a member for this portfolio.""" requested_email = form.cleaned_data["email"] requestor = self.request.user + portfolio = self.request.session.get("portfolio") requested_user = User.objects.filter(email=requested_email).first() - permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=self.object).exists() + permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() try: if not requested_user or not permission_exists: - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=self.object) + send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) ## NOTE : this is not yet accounting properly for roles and permissions - PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=self.object) + PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=portfolio) messages.success(self.request, f"{requested_email} has been invited.") else: if permission_exists: messages.warning(self.request, "User is already a member of this portfolio.") except Exception as e: - self._handle_exceptions(e, requested_email) + self._handle_exceptions(e, portfolio, requested_email) return redirect(self.get_success_url()) - def _handle_exceptions(self, exception, email): + def _handle_exceptions(self, exception, portfolio, email): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): - logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", email, self.object, exc_info=True) + logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", email, portfolio, exc_info=True) messages.warning(self.request, "Could not send email invitation.") elif isinstance(exception, AlreadyPortfolioMemberError): messages.warning(self.request, str(exception)) @@ -555,9 +557,9 @@ class NewMemberView(PortfolioMembersPermissionView, FormMixin): elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) logger.error( - f"Can't send email to '{email}' for portfolio '{self.object}'. No email exists for the requestor.", + f"Can't send email to '{email}' for portfolio '{portfolio}'. No email exists for the requestor.", exc_info=True, ) else: - logger.warning("Could not send email invitation (Other Exception)", self.object, exc_info=True) + logger.warning("Could not send email invitation (Other Exception)", portfolio, exc_info=True) messages.warning(self.request, "Could not send email invitation.") From ead90c15411ac2f9d1cadc478bc85992642b4e7a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 16 Dec 2024 14:11:42 -0600 Subject: [PATCH 010/114] tests for approved domain warning --- src/registrar/admin.py | 50 ++++++++++------------- src/registrar/tests/test_admin_request.py | 38 ++++++++++++++++- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5e8148664..ce3b0220c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2429,8 +2429,28 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): return response def change_view(self, request, object_id, form_url="", extra_context=None): - """Display restricted warning, - Setup the auditlog trail and pass it in extra context.""" + """Display restricted warning, setup the auditlog trail and pass it in extra context, + display warning that status cannot be changed from 'Approved' if domain is in Ready state""" + + # Fetch the Contact instance + domain_request: models.DomainRequest = models.DomainRequest.objects.get(pk=object_id) + if domain_request.approved_domain and domain_request.approved_domain.state == models.Domain.State.READY: + domain = domain_request.approved_domain + # get change url for domain + app_label = domain_request.approved_domain._meta.app_label + model_name = domain._meta.model_name + obj_id = domain.id + change_url = reverse("admin:%s_%s_change" % (app_label, model_name), args=[obj_id]) + + message = f"
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: " # noqa + message += f"{domain}
    • " + + message_html = mark_safe(message) # nosec + messages.warning( + request, + message_html, + ) + obj = self.get_object(request, object_id) self.display_restricted_warning(request, obj) @@ -2543,32 +2563,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Further filter the queryset by the portfolio qs = qs.filter(portfolio=portfolio_id) return qs - - def change_view(self, request, object_id, form_url="", extra_context=None): - """Extend the change_view for DomainRequest objects in django admin. - Customize to display notification that statu cannot be changed from 'Approved'.""" - - # Fetch the Contact instance - domain_request: models.DomainRequest = models.DomainRequest.objects.get(pk=object_id) - if domain_request.approved_domain and domain_request.approved_domain.state == models.Domain.State.READY: - domain = domain_request.approved_domain - # get change url for domain - app_label = domain_request.approved_domain._meta.app_label - model_name = domain._meta.model_name - obj_id = domain.id - change_url = reverse("admin:%s_%s_change" % (app_label, model_name), args=[obj_id]) - - message += f"

    The status of this domain request cannot be changed because it has been joined to a domain in Ready status: " # noqa - message += f"{domain}

    " - - message_html = mark_safe(message) # nosec - messages.warning( - request, - message_html, - ) - - return super().change_view(request, object_id, form_url, extra_context=extra_context) - class TransitionDomainAdmin(ListHeaderAdmin): """Custom transition domain admin class.""" diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index df0902719..d744dd00a 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -25,6 +25,8 @@ from registrar.models import ( Portfolio, AllowedEmail, ) +from registrar.models.host import Host +from registrar.models.public_contact import PublicContact from .common import ( MockSESClient, completed_domain_request, @@ -36,7 +38,7 @@ from .common import ( MockEppLib, GenericTestHelper, ) -from unittest.mock import patch +from unittest.mock import ANY, patch from django.conf import settings import boto3_mocking # type: ignore @@ -76,6 +78,8 @@ class TestDomainRequestAdmin(MockEppLib): def tearDown(self): super().tearDown() + Host.objects.all().delete() + PublicContact.objects.all().delete() Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() @@ -91,6 +95,7 @@ class TestDomainRequestAdmin(MockEppLib): User.objects.all().delete() AllowedEmail.objects.all().delete() + @less_console_noise_decorator def test_domain_request_senior_official_is_alphabetically_sorted(self): """Tests if the senior offical dropdown is alphanetically sorted in the django admin display""" @@ -1810,6 +1815,37 @@ class TestDomainRequestAdmin(MockEppLib): request, "Cannot edit a domain request with a restricted creator.", ) + + # @less_console_noise_decorator + def test_approved_domain_request_with_ready_domain_has_warning_message(self): + """Tests if the domain request has a warning message when the approved domain is in Ready state""" + # Create an instance of the model + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) + # Approve the domain request + domain_request.approve() + domain_request.save() + + # Add nameservers to get to Ready state + domain_request.approved_domain.nameservers = [ + ("ns1.city.gov", ["1.1.1.1"]), + ("ns2.city.gov", ["1.1.1.2"]), + ] + domain_request.approved_domain.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client): + with patch("django.contrib.messages.warning") as mock_warning: + # Create a request object + self.client.force_login(self.superuser) + self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Assert that the error message was called with the correct argument + mock_warning.assert_called_once_with( + ANY, # don't care about the request argument + "
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: city.gov
    • " # care about this message + ) def trigger_saving_approved_to_another_state(self, domain_is_active, another_state, rejection_reason=None): """Helper method that triggers domain request state changes from approved to another state, From ec39b159ecfffb7786b2530ce127037084edee68 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 16 Dec 2024 14:19:12 -0600 Subject: [PATCH 011/114] make test less brittle --- src/registrar/tests/test_admin_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index d744dd00a..bbf6c1923 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1844,7 +1844,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that the error message was called with the correct argument mock_warning.assert_called_once_with( ANY, # don't care about the request argument - "
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: city.gov
    • " # care about this message + f"
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: {domain_request.approved_domain.name}
    • ", # noqa ) def trigger_saving_approved_to_another_state(self, domain_is_active, another_state, rejection_reason=None): From 79077cce31efa1e469ba426507e9e7d44c263890 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 16 Dec 2024 13:23:14 -0700 Subject: [PATCH 012/114] basic logic --- .../js/getgov-admin/domain-request-form.js | 5 +++ ...0_alter_suborganization_unique_together.py | 17 +++++++++ src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 37 ++++++++++++++++++- src/registrar/models/suborganization.py | 3 ++ 5 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/registrar/migrations/0140_alter_suborganization_unique_together.py diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index a815a59a1..6b79a2419 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -629,6 +629,10 @@ export function initRejectedEmail() { }); } +function handleSuborganizationSelection() { + console.log("cats are cool") +} + /** * A function for dynamic DomainRequest fields */ @@ -636,5 +640,6 @@ export function initDynamicDomainRequestFields(){ const domainRequestPage = document.getElementById("domainrequest_form"); if (domainRequestPage) { handlePortfolioSelection(); + handleSuborganizationSelection(); } } diff --git a/src/registrar/migrations/0140_alter_suborganization_unique_together.py b/src/registrar/migrations/0140_alter_suborganization_unique_together.py new file mode 100644 index 000000000..e59ecdf2b --- /dev/null +++ b/src/registrar/migrations/0140_alter_suborganization_unique_together.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.10 on 2024-12-16 17:02 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0139_alter_domainrequest_action_needed_reason"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="suborganization", + unique_together={("name", "portfolio")}, + ), + ] diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cc600e1ce..2ea78ff10 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -237,6 +237,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 44d8511b0..46e188a0a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -690,6 +690,18 @@ class DomainRequest(TimeStampedModel): # Update the cached values after saving self._cache_status_and_status_reasons() + def create_requested_suborganization(self): + """Creates the requested suborganization. + Adds the name, portfolio, city, and state_territory fields. + Returns the created suborganization.""" + Suborganization = apps.get_model("registrar.Suborganization") + return Suborganization.objects.create( + name=self.requested_suborganization, + portfolio=self.portfolio, + city=self.suborganization_city, + state_territory=self.suborganization_state_territory, + ) + def send_custom_status_update_email(self, status): """Helper function to send out a second status email when the status remains the same, but the reason has changed.""" @@ -785,6 +797,7 @@ class DomainRequest(TimeStampedModel): def delete_and_clean_up_domain(self, called_from): try: + # Clean up the approved domain domain_state = self.approved_domain.state # Only reject if it exists on EPP if domain_state != Domain.State.UNKNOWN: @@ -796,6 +809,19 @@ class DomainRequest(TimeStampedModel): logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") + # Clean up any created suborgs, assuming its for this record only + if self.sub_organization is not None: + request_suborg_count = self.request_sub_organization.count() + domain_suborgs = self.DomainRequest_info.filter( + sub_organization=self.sub_organization + ).count() + if request_suborg_count == 1 and domain_suborgs.count() <= 1: + # if domain_suborgs.count() == 1: + # domain_info = domain_suborgs.first() + # domain_info.sub_organization = None + # domain_info.save() + self.sub_organization.delete() + def _send_status_update_email( self, new_status, @@ -984,6 +1010,7 @@ class DomainRequest(TimeStampedModel): if self.status == self.DomainRequestStatus.APPROVED: self.delete_and_clean_up_domain("action_needed") + elif self.status == self.DomainRequestStatus.REJECTED: self.rejection_reason = None @@ -1014,8 +1041,16 @@ class DomainRequest(TimeStampedModel): domain request into an admin on that domain. It also triggers an email notification.""" + should_save = False if self.federal_agency is None: self.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() + should_save = True + + if self.is_requesting_new_suborganization(): + self.sub_organization = self.create_requested_suborganization() + should_save = True + + if should_save: self.save() # create the domain @@ -1148,7 +1183,7 @@ class DomainRequest(TimeStampedModel): def is_requesting_new_suborganization(self) -> bool: """Determines if a user is trying to request a new suborganization using the domain request form, rather than one that already exists. - Used for the RequestingEntity page. + Used for the RequestingEntity page and on DomainInformation.create_from_da(). Returns True if a sub_organization does not exist and if requested_suborganization, suborganization_city, and suborganization_state_territory all exist. diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index 087490244..fb32ad48a 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -9,6 +9,9 @@ class Suborganization(TimeStampedModel): Suborganization under an organization (portfolio) """ + class Meta: + unique_together = ["name", "portfolio"] + name = models.CharField( unique=True, max_length=1000, From 86960b3068a44ace232e3d00b3e2ea42872e8081 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 16 Dec 2024 14:27:04 -0600 Subject: [PATCH 013/114] remove conditional third buller on dotgov domain step of domain request --- src/registrar/templates/domain_request_dotgov_domain.html | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/registrar/templates/domain_request_dotgov_domain.html b/src/registrar/templates/domain_request_dotgov_domain.html index 6c62c6497..38347ad96 100644 --- a/src/registrar/templates/domain_request_dotgov_domain.html +++ b/src/registrar/templates/domain_request_dotgov_domain.html @@ -6,11 +6,7 @@
    • Be available
    • Relate to your organization’s name, location, and/or services
      • - {% if portfolio %} -
    • Be clear to the general public. Your domain name must not be easily confused with other organizations.
      • - {% else %}
    • Be unlikely to mislead or confuse the general public (even if your domain is only intended for a specific audience)
      • - {% endif %}

    From 060f2d5c8acca4a733311970d4057e33b1576e32 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 16 Dec 2024 17:13:35 -0500 Subject: [PATCH 014/114] refactor form and view for add portfolio member --- src/registrar/assets/src/js/getgov/main.js | 4 +- .../src/js/getgov/portfolio-member-page.js | 4 +- src/registrar/config/urls.py | 2 +- src/registrar/forms/portfolio.py | 94 +++++++++--------- src/registrar/models/portfolio_invitation.py | 5 +- .../models/utility/portfolio_helper.py | 11 ++- .../templates/portfolio_members_add_new.html | 1 - src/registrar/utility/email_invitations.py | 2 +- src/registrar/utility/errors.py | 4 +- src/registrar/views/portfolios.py | 98 +++++++++---------- 10 files changed, 115 insertions(+), 110 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/main.js b/src/registrar/assets/src/js/getgov/main.js index 5de02f35a..0e019dd22 100644 --- a/src/registrar/assets/src/js/getgov/main.js +++ b/src/registrar/assets/src/js/getgov/main.js @@ -23,8 +23,8 @@ hookupYesNoListener("additional_details-has_anything_else_text",'anything-else', hookupRadioTogglerListener( 'member_access_level', { - 'admin': 'new-member-admin-permissions', - 'basic': 'new-member-basic-permissions' + 'organization_admin': 'new-member-admin-permissions', + 'organization_member': 'new-member-basic-permissions' } ); hookupYesNoListener("additional_details-has_cisa_representative",'cisa-representative', null); 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 ac0b7cffe..fb5bd46f7 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -49,7 +49,7 @@ export function initPortfolioMemberPageToggle() { * on the Add New Member page. */ export function initAddNewMemberPageListeners() { - add_member_form = document.getElementById("add_member_form") + let add_member_form = document.getElementById("add_member_form") if (!add_member_form){ return; } @@ -156,7 +156,7 @@ export function initAddNewMemberPageListeners() { document.getElementById('modalAccessLevel').textContent = accessText; // Populate permission details based on access level - if (selectedAccess && selectedAccess.value === 'admin') { + if (selectedAccess && selectedAccess.value === 'organization_admin') { populatePermissionDetails('new-member-admin-permissions'); } else { populatePermissionDetails('new-member-basic-permissions'); diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index d71645dee..e92742984 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -136,7 +136,7 @@ urlpatterns = [ # ), path( "members/new-member/", - views.PortfolioNewMemberView.as_view(), + views.PortfolioAddMemberView.as_view(), name="new-member", ), path( diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 14b00529f..bb3da7f7d 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -167,7 +167,7 @@ class PortfolioInvitedMemberForm(forms.ModelForm): class PortfolioNewMemberForm(forms.ModelForm): member_access_level = forms.ChoiceField( label="Select permission", - choices=[("admin", "Admin Access"), ("basic", "Basic Access")], + choices=[("organization_admin", "Admin Access"), ("organization_member", "Basic Access")], widget=forms.RadioSelect(attrs={"class": "usa-radio__input usa-radio__input--tile"}), required=True, error_messages={ @@ -176,7 +176,7 @@ class PortfolioNewMemberForm(forms.ModelForm): ) admin_org_domain_request_permissions = forms.ChoiceField( label="Select permission", - choices=[("view_only", "View all requests"), ("view_and_create", "View all requests plus create requests")], + choices=[("view_all_requests", "View all requests"), ("edit_requests", "View all requests plus create requests")], widget=forms.RadioSelect, required=True, error_messages={ @@ -185,7 +185,7 @@ class PortfolioNewMemberForm(forms.ModelForm): ) admin_org_members_permissions = forms.ChoiceField( label="Select permission", - choices=[("view_only", "View all members"), ("view_and_create", "View all members plus manage members")], + choices=[("view_members", "View all members"), ("edit_members", "View all members plus manage members")], widget=forms.RadioSelect, required=True, error_messages={ @@ -195,9 +195,9 @@ class PortfolioNewMemberForm(forms.ModelForm): basic_org_domain_request_permissions = forms.ChoiceField( label="Select permission", choices=[ - ("view_only", "View all requests"), - ("view_and_create", "View all requests plus create requests"), - ("no_access", "No access"), + ("view_all_requests", "View all requests"), + ("edit_requests", "View all requests plus create requests"), + ("", "No access"), ], widget=forms.RadioSelect, required=True, @@ -226,52 +226,52 @@ class PortfolioNewMemberForm(forms.ModelForm): model = PortfolioInvitation fields = ["email"] - def _post_clean(self): - logger.info("in _post_clean") - super()._post_clean() - def clean(self): - cleaned_data = super().clean() - # Lowercase the value of the 'email' field - email_value = cleaned_data.get("email") + email_value = self.cleaned_data.get("email") if email_value: - cleaned_data["email"] = email_value.lower() + self.cleaned_data["email"] = email_value.lower() - ########################################## - # TODO: future ticket - # (invite new member) - ########################################## - # Check for an existing user (if there isn't any, send an invite) - # if email_value: - # try: - # existingUser = User.objects.get(email=email_value) - # except User.DoesNotExist: - # raise forms.ValidationError("User with this email does not exist.") + # Get the selected member access level + member_access_level = self.cleaned_data.get("member_access_level") - member_access_level = cleaned_data.get("member_access_level") - - # Intercept the error messages so that we don't validate hidden inputs + # If no member access level is selected, remove errors for hidden inputs if not member_access_level: - # If no member access level has been selected, delete error messages - # for all hidden inputs (which is everything except the e-mail input - # and member access selection) - for field in self.fields: - if field in self.errors and field != "email" and field != "member_access_level": - del self.errors[field] - return cleaned_data + self._remove_hidden_field_errors(exclude_fields=["email", "member_access_level"]) + return self.cleaned_data - basic_dom_req_error = "basic_org_domain_request_permissions" - admin_dom_req_error = "admin_org_domain_request_permissions" - admin_member_error = "admin_org_members_permissions" + # Define field names for validation cleanup + field_error_map = { + "organization_admin": ["basic_org_domain_request_permissions"], # Fields irrelevant to "admin" + "organization_member": ["admin_org_domain_request_permissions", "admin_org_members_permissions"], # Fields irrelevant to "basic" + } + + # Remove errors for irrelevant fields based on the selected access level + irrelevant_fields = field_error_map.get(member_access_level, []) + for field in irrelevant_fields: + if field in self.errors: + del self.errors[field] + + # Map roles and additional permissions to cleaned_data + self.cleaned_data["roles"] = [member_access_level] + additional_permissions = [ + self.cleaned_data.get("admin_org_domain_request_permissions"), + self.cleaned_data.get("basic_org_domain_request_permissions"), + self.cleaned_data.get("admin_org_members_permissions"), + ] + # Filter out None values + self.cleaned_data["additional_permissions"] = [perm for perm in additional_permissions if perm] + + return super().clean() + + def _remove_hidden_field_errors(self, exclude_fields=None): + """ + Helper method to remove errors for fields that are not relevant + (e.g., hidden inputs), except for explicitly excluded fields. + """ + exclude_fields = exclude_fields or [] + hidden_fields = [field for field in self.fields if field not in exclude_fields] + for field in hidden_fields: + if field in self.errors: + del self.errors[field] - if member_access_level == "admin" and basic_dom_req_error in self.errors: - # remove the error messages pertaining to basic permission inputs - del self.errors[basic_dom_req_error] - elif member_access_level == "basic": - # remove the error messages pertaining to admin permission inputs - if admin_dom_req_error in self.errors: - del self.errors[admin_dom_req_error] - if admin_member_error in self.errors: - del self.errors[admin_member_error] - return cleaned_data diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 92f527377..3a5103b17 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -111,11 +111,8 @@ class PortfolioInvitation(TimeStampedModel): user_portfolio_permission.additional_permissions = self.additional_permissions user_portfolio_permission.save() - def full_clean(self, exclude=None, validate_unique=True): - logger.info("portfolio invitation full clean") - super().full_clean(exclude, validate_unique) - def clean(self): """Extends clean method to perform additional validation, which can raise errors in django admin.""" + print(f'portfolio invitation model clean') super().clean() validate_portfolio_invitation(self) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 396e6ab69..1ec9f2109 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -155,7 +155,6 @@ def validate_portfolio_invitation(portfolio_invitation): Raises: ValidationError: If any of the validation rules are violated. """ - logger.info("portfolio invitataion validation") PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation") UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission") User = get_user_model() @@ -163,11 +162,21 @@ def validate_portfolio_invitation(portfolio_invitation): has_portfolio = bool(portfolio_invitation.portfolio_id) portfolio_permissions = set(portfolio_invitation.get_portfolio_permissions()) + print(f"has_portfolio {has_portfolio}") + + print(f"portfolio_permissions {portfolio_permissions}") + + print(f"roles {portfolio_invitation.roles}") + + print(f"additional permissions {portfolio_invitation.additional_permissions}") + # == Validate required fields == # if not has_portfolio and portfolio_permissions: + print(f"not has_portfolio and portfolio_permissions {portfolio_permissions}") raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") if has_portfolio and not portfolio_permissions: + print(f"has_portfolio and not portfolio_permissions {portfolio_permissions}") raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") # == Validate role permissions. Compares existing permissions to forbidden ones. == # diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index 655b01852..a441c15b1 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -134,7 +134,6 @@ id="invite-member-modal" aria-labelledby="invite-member-heading" aria-describedby="confirm-invite-description" - style="display: none;" >
    diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 1c076493a..a32c8092f 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -114,7 +114,7 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): if invite.status == PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED: raise AlreadyPortfolioMemberError(email) else: - raise AlreadyPortfolioInvitedError(email) + raise AlreadyPortfolioInvitedError(email, portfolio) except PortfolioInvitation.DoesNotExist: pass diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 2ad95a99c..ea825b367 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -53,8 +53,8 @@ class AlreadyPortfolioMemberError(InvitationError): class AlreadyPortfolioInvitedError(InvitationError): """Raised when the user has already been invited to the portfolio.""" - def __init__(self, email): - super().__init__(f"{email} has already been invited to this portfolio.") + def __init__(self, email, portfolio): + super().__init__(f"{email} has already been invited to {portfolio}.") class MissingEmailError(InvitationError): diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 84fa9682b..62320364e 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -469,82 +469,82 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): return render(request, "portfolio_members.html") -class PortfolioNewMemberView(PortfolioMembersPermissionView, FormMixin): +class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): template_name = "portfolio_members_add_new.html" form_class = portfolioForms.PortfolioNewMemberForm - # def get_object(self, queryset=None): - # """Get the portfolio object based on the session.""" - # portfolio = self.request.session.get("portfolio") - # if portfolio is None: - # raise Http404("No organization found for this user") - # return portfolio - - # def get_form_kwargs(self): - # """Include the instance in the form kwargs.""" - # kwargs = super().get_form_kwargs() - # kwargs["instance"] = self.get_object() - # return kwargs - def get(self, request, *args, **kwargs): """Handle GET requests to display the form.""" - self.object = self.request.session.get("portfolio") + self.object = None # No existing PortfolioInvitation instance form = self.get_form() return self.render_to_response(self.get_context_data(form=form)) + + def is_ajax(self): + return self.request.headers.get("X-Requested-With") == "XMLHttpRequest" + + def form_invalid(self, form): + if self.is_ajax(): + return JsonResponse({"is_valid": False}) # Return a JSON response + else: + return super().form_invalid(form) # Handle non-AJAX requests normally + + def form_valid(self, form): + + if self.is_ajax(): + return JsonResponse({"is_valid": True}) # Return a JSON response + else: + return self.submit_new_member(form) def post(self, request, *args, **kwargs): """Handle POST requests to process form submission.""" - - # self.object = self.get_object() + self.object = None # For a new invitation, there's no existing model instance form = self.get_form() + print('before is_valid') if form.is_valid(): + print('form is_valid') return self.form_valid(form) else: + print('form NOT is_valid') return self.form_invalid(form) - # def is_ajax(self): - # return self.request.headers.get("X-Requested-With") == "XMLHttpRequest" - - # def form_invalid(self, form): - # if self.is_ajax(): - # return JsonResponse({"is_valid": False}) # Return a JSON response - # else: - # return super().form_invalid(form) # Handle non-AJAX requests normally - - # def form_valid(self, form): - - # if self.is_ajax(): - # return JsonResponse({"is_valid": True}) # Return a JSON response - # else: - # return self.submit_new_member(form) - def get_success_url(self): """Redirect to members table.""" return reverse("members") - def form_valid(self, form): + def submit_new_member(self, form): """Add the specified user as a member for this portfolio.""" - requested_email = form.cleaned_data["email"] - requestor = self.request.user + # Retrieve the portfolio from the session portfolio = self.request.session.get("portfolio") + if not portfolio: + messages.error(self.request, "No portfolio found in session.") + return self.form_invalid(form) + + # Save the invitation instance + invitation = form.save(commit=False) + invitation.portfolio = portfolio + + # Send invitation email and show a success message + send_portfolio_invitation_email( + email=invitation.email, + requestor=self.request.user, + portfolio=portfolio, + ) + + # Use processed data from the form + invitation.roles = form.cleaned_data["roles"] + invitation.additional_permissions = form.cleaned_data["additional_permissions"] + invitation.save() + + messages.success(self.request, f"{invitation.email} has been invited.") - requested_user = User.objects.filter(email=requested_email).first() - permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() - try: - if not requested_user or not permission_exists: - send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) - ## NOTE : this is not yet accounting properly for roles and permissions - PortfolioInvitation.objects.get_or_create(email=requested_email, portfolio=portfolio) - messages.success(self.request, f"{requested_email} has been invited.") - else: - if permission_exists: - messages.warning(self.request, "User is already a member of this portfolio.") - except Exception as e: - self._handle_exceptions(e, portfolio, requested_email) return redirect(self.get_success_url()) + def get_success_url(self): + """Redirect to the members page.""" + return reverse("members") + def _handle_exceptions(self, exception, portfolio, email): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): From ae3ce1f9cd9d8c934f5dfad39a4979bdf4dfec27 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 18:02:35 -0500 Subject: [PATCH 015/114] wip --- src/registrar/forms/portfolio.py | 23 +++++++++++++++++++- src/registrar/models/portfolio_invitation.py | 4 ++++ src/registrar/views/portfolios.py | 12 +++++++++- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index bb3da7f7d..26619ecf9 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -224,9 +224,30 @@ class PortfolioNewMemberForm(forms.ModelForm): class Meta: model = PortfolioInvitation - fields = ["email"] + fields = ["portfolio", "email", "roles", "additional_permissions"] + + def is_valid(self): + logger.info("is valid()") + return super().is_valid() + + def full_clean(self): + logger.info("full_clean()") + super().full_clean() + + def _clean_fields(self): + logger.info("clean fields") + logger.info(self.fields) + super()._clean_fields() + + def _post_clean(self): + logger.info("post clean") + logger.info(self.cleaned_data) + super()._post_clean() + logger.info(self.instance) def clean(self): + logger.info(self.cleaned_data) + logger.info(self.initial) # Lowercase the value of the 'email' field email_value = self.cleaned_data.get("email") if email_value: diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 3a5103b17..7988dcd88 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -111,6 +111,10 @@ class PortfolioInvitation(TimeStampedModel): user_portfolio_permission.additional_permissions = self.additional_permissions user_portfolio_permission.save() + def full_clean(self, exclude=True, validate_unique=False): + logger.info("full clean") + super().full_clean(exclude=exclude, validate_unique=validate_unique) + def clean(self): """Extends clean method to perform additional validation, which can raise errors in django admin.""" print(f'portfolio invitation model clean') diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 62320364e..6f88f8d21 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -499,7 +499,17 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): def post(self, request, *args, **kwargs): """Handle POST requests to process form submission.""" self.object = None # For a new invitation, there's no existing model instance - form = self.get_form() + + data = request.POST.copy() + + # Override the 'portfolio' field value + if not data.get("portfolio"): + data["portfolio"] = self.request.session.get("portfolio").id + + # Pass the modified data to the form + form = portfolioForms.PortfolioNewMemberForm(data) + #form = self.get_form() + #logger.info(form.fields["portfolio"]) print('before is_valid') if form.is_valid(): From 7bbee19bfe2f6748720df461e78468bf99b32988 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 22:53:49 -0500 Subject: [PATCH 016/114] portfolio invitations working in admin view as well as user view --- src/registrar/admin.py | 27 +++--- src/registrar/forms/portfolio.py | 21 ----- src/registrar/models/portfolio_invitation.py | 5 -- .../models/utility/portfolio_helper.py | 10 --- src/registrar/utility/email_invitations.py | 12 --- src/registrar/views/portfolios.py | 85 +++++++------------ 6 files changed, 44 insertions(+), 116 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e7bab8405..dddbb96a4 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -14,7 +14,6 @@ from django.db.models import ( from django.db.models.functions import Concat, Coalesce from django.http import HttpResponseRedirect from registrar.models.federal_agency import FederalAgency -from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.utility.admin_helpers import ( AutocompleteSelectWithPlaceholder, get_action_needed_reason_default_email, @@ -42,7 +41,7 @@ from waffle.admin import FlagAdmin from waffle.models import Sample, Switch from registrar.models import Contact, Domain, DomainRequest, DraftDomain, User, Website, SeniorOfficial from registrar.utility.constants import BranchChoices -from registrar.utility.errors import AlreadyPortfolioInvitedError, AlreadyPortfolioMemberError, FSMDomainRequestError, FSMErrorCodes, MissingEmailError +from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes, MissingEmailError from registrar.utility.waffle import flag_is_active_for_user from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR @@ -1495,11 +1494,7 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): """Handle exceptions raised during the process.""" if isinstance(exception, EmailSendingError): logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", obj.email, obj.portfolio, exc_info=True) - messages.warning(request, "Could not send email invitation.") - elif isinstance(exception, AlreadyPortfolioMemberError): - messages.warning(request, str(exception)) - elif isinstance(exception, AlreadyPortfolioInvitedError): - messages.warning(request, str(exception)) + messages.error(request, "Could not send email invitation. Portfolio invitation not saved.") elif isinstance(exception, MissingEmailError): messages.error(request, str(exception)) logger.error( @@ -1508,7 +1503,7 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): ) else: logger.warning("Could not send email invitation (Other Exception)", obj.portfolio, exc_info=True) - messages.warning(request, "Could not send email invitation.") + messages.error(request, "Could not send email invitation. Portfolio invitation not saved.") def response_add(self, request, obj, post_url_continue=None): """ @@ -1522,7 +1517,6 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): # Re-render the change form if there are errors or warnings # Prepare context for rendering the change form opts = self.model._meta - app_label = opts.app_label # Get the model form ModelForm = self.get_form(request, obj=obj) @@ -1532,37 +1526,38 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): admin_form = AdminForm( form, list(self.get_fieldsets(request, obj)), - self.prepopulated_fields, + self.get_prepopulated_fields(request, obj), self.get_readonly_fields(request, obj), model_admin=self, ) + media = self.media + form.media opts = obj._meta change_form_context = { **self.admin_site.each_context(request), # Add admin context - "title": f"Change {opts.verbose_name}", + "title": f"Add {opts.verbose_name}", "opts": opts, "original": obj, "save_as": self.save_as, "has_change_permission": self.has_change_permission(request, obj), - "add": False, # Indicate this is not an "Add" form - "change": True, # Indicate this is a "Change" form + "add": True, # Indicate this is an "Add" form + "change": False, # Indicate this is not a "Change" form "is_popup": False, "inline_admin_formsets": [], "save_on_top": self.save_on_top, "show_delete": self.has_delete_permission(request, obj), "obj": obj, "adminform": admin_form, # Pass the AdminForm instance + "media": media, "errors": None, # You can use this to pass custom form errors } return self.render_change_form( request, context=change_form_context, - add=False, - change=True, + add=True, + change=False, obj=obj, ) - return super().response_add(request, obj, post_url_continue) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 26619ecf9..b879aacb7 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -226,28 +226,7 @@ class PortfolioNewMemberForm(forms.ModelForm): model = PortfolioInvitation fields = ["portfolio", "email", "roles", "additional_permissions"] - def is_valid(self): - logger.info("is valid()") - return super().is_valid() - - def full_clean(self): - logger.info("full_clean()") - super().full_clean() - - def _clean_fields(self): - logger.info("clean fields") - logger.info(self.fields) - super()._clean_fields() - - def _post_clean(self): - logger.info("post clean") - logger.info(self.cleaned_data) - super()._post_clean() - logger.info(self.instance) - def clean(self): - logger.info(self.cleaned_data) - logger.info(self.initial) # Lowercase the value of the 'email' field email_value = self.cleaned_data.get("email") if email_value: diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 7988dcd88..11c564c36 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -111,12 +111,7 @@ class PortfolioInvitation(TimeStampedModel): user_portfolio_permission.additional_permissions = self.additional_permissions user_portfolio_permission.save() - def full_clean(self, exclude=True, validate_unique=False): - logger.info("full clean") - super().full_clean(exclude=exclude, validate_unique=validate_unique) - def clean(self): """Extends clean method to perform additional validation, which can raise errors in django admin.""" - print(f'portfolio invitation model clean') super().clean() validate_portfolio_invitation(self) diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 1ec9f2109..f50b14384 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -162,21 +162,11 @@ def validate_portfolio_invitation(portfolio_invitation): has_portfolio = bool(portfolio_invitation.portfolio_id) portfolio_permissions = set(portfolio_invitation.get_portfolio_permissions()) - print(f"has_portfolio {has_portfolio}") - - print(f"portfolio_permissions {portfolio_permissions}") - - print(f"roles {portfolio_invitation.roles}") - - print(f"additional permissions {portfolio_invitation.additional_permissions}") - # == Validate required fields == # if not has_portfolio and portfolio_permissions: - print(f"not has_portfolio and portfolio_permissions {portfolio_permissions}") raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") if has_portfolio and not portfolio_permissions: - print(f"has_portfolio and not portfolio_permissions {portfolio_permissions}") raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") # == Validate role permissions. Compares existing permissions to forbidden ones. == # diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index a32c8092f..1b3b47876 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -93,8 +93,6 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): Raises: MissingEmailError: If the requestor has no email associated with their account. - AlreadyPortfolioMemberError: If the email corresponds to an existing portfolio member. - AlreadyPortfolioInvitedError: If an invitation has already been sent. EmailSendingError: If there is an error while sending the email. """ @@ -108,16 +106,6 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio): else: requestor_email = requestor.email - # Check to see if an invite has already been sent - try: - invite = PortfolioInvitation.objects.get(email=email, portfolio=portfolio) - if invite.status == PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED: - raise AlreadyPortfolioMemberError(email) - else: - raise AlreadyPortfolioInvitedError(email, portfolio) - except PortfolioInvitation.DoesNotExist: - pass - send_templated_email( "emails/portfolio_invitation.txt", "emails/portfolio_invitation_subject.txt", diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 6f88f8d21..d8e4872d9 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -14,7 +14,7 @@ from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError from registrar.utility.email_invitations import send_portfolio_invitation_email -from registrar.utility.errors import AlreadyPortfolioInvitedError, AlreadyPortfolioMemberError, MissingEmailError +from registrar.utility.errors import MissingEmailError from registrar.views.utility.mixins import PortfolioMemberPermission from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, @@ -480,6 +480,22 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): form = self.get_form() return self.render_to_response(self.get_context_data(form=form)) + def post(self, request, *args, **kwargs): + """Handle POST requests to process form submission.""" + self.object = None # For a new invitation, there's no existing model instance + + # portfolio not submitted with form, so override the value + data = request.POST.copy() + if not data.get("portfolio"): + data["portfolio"] = self.request.session.get("portfolio").id + # Pass the modified data to the form + form = portfolioForms.PortfolioNewMemberForm(data) + + if form.is_valid(): + return self.form_valid(form) + else: + return self.form_invalid(form) + def is_ajax(self): return self.request.headers.get("X-Requested-With") == "XMLHttpRequest" @@ -490,65 +506,34 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): return super().form_invalid(form) # Handle non-AJAX requests normally def form_valid(self, form): - + super().form_valid(form) if self.is_ajax(): return JsonResponse({"is_valid": True}) # Return a JSON response else: return self.submit_new_member(form) - def post(self, request, *args, **kwargs): - """Handle POST requests to process form submission.""" - self.object = None # For a new invitation, there's no existing model instance - - data = request.POST.copy() - - # Override the 'portfolio' field value - if not data.get("portfolio"): - data["portfolio"] = self.request.session.get("portfolio").id - - # Pass the modified data to the form - form = portfolioForms.PortfolioNewMemberForm(data) - #form = self.get_form() - #logger.info(form.fields["portfolio"]) - - print('before is_valid') - if form.is_valid(): - print('form is_valid') - return self.form_valid(form) - else: - print('form NOT is_valid') - return self.form_invalid(form) - def get_success_url(self): """Redirect to members table.""" return reverse("members") def submit_new_member(self, form): """Add the specified user as a member for this portfolio.""" - # Retrieve the portfolio from the session - portfolio = self.request.session.get("portfolio") - if not portfolio: - messages.error(self.request, "No portfolio found in session.") - return self.form_invalid(form) - - # Save the invitation instance - invitation = form.save(commit=False) - invitation.portfolio = portfolio - - # Send invitation email and show a success message - send_portfolio_invitation_email( - email=invitation.email, - requestor=self.request.user, - portfolio=portfolio, - ) - - # Use processed data from the form - invitation.roles = form.cleaned_data["roles"] - invitation.additional_permissions = form.cleaned_data["additional_permissions"] - invitation.save() - - messages.success(self.request, f"{invitation.email} has been invited.") + requested_email = form.cleaned_data["email"] + requestor = self.request.user + portfolio = form.cleaned_data["portfolio"] + requested_user = User.objects.filter(email=requested_email).first() + permission_exists = UserPortfolioPermission.objects.filter(user=requested_user, portfolio=portfolio).exists() + try: + if not requested_user or not permission_exists: + send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio) + form.save() + messages.success(self.request, f"{requested_email} has been invited.") + else: + if permission_exists: + messages.warning(self.request, "User is already a member of this portfolio.") + except Exception as e: + self._handle_exceptions(e, portfolio, requested_email) return redirect(self.get_success_url()) def get_success_url(self): @@ -560,10 +545,6 @@ class PortfolioAddMemberView(PortfolioMembersPermissionView, FormMixin): if isinstance(exception, EmailSendingError): logger.warning("Could not sent email invitation to %s for portfolio %s (EmailSendingError)", email, portfolio, exc_info=True) messages.warning(self.request, "Could not send email invitation.") - elif isinstance(exception, AlreadyPortfolioMemberError): - messages.warning(self.request, str(exception)) - elif isinstance(exception, AlreadyPortfolioInvitedError): - messages.warning(self.request, str(exception)) elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) logger.error( From 5cb6c3f0fb6624490ca8fdd5c70958750eb1e7c3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 22:59:26 -0500 Subject: [PATCH 017/114] error cleanup --- src/registrar/utility/email_invitations.py | 2 -- src/registrar/utility/errors.py | 14 -------------- src/registrar/views/domain.py | 6 ------ 3 files changed, 22 deletions(-) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 1b3b47876..c0077c196 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -5,8 +5,6 @@ from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, - AlreadyPortfolioInvitedError, - AlreadyPortfolioMemberError, MissingEmailError, OutsideOrgMemberError, ) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index ea825b367..210d13081 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -43,20 +43,6 @@ class AlreadyDomainInvitedError(InvitationError): super().__init__(f"{email} has already been invited to this domain.") -class AlreadyPortfolioMemberError(InvitationError): - """Raised when the user is already a member of the portfolio.""" - - def __init__(self, email): - super().__init__(f"{email} is already a manager for this portfolio.") - - -class AlreadyPortfolioInvitedError(InvitationError): - """Raised when the user has already been invited to the portfolio.""" - - def __init__(self, email, portfolio): - super().__init__(f"{email} has already been invited to {portfolio}.") - - class MissingEmailError(InvitationError): """Raised when the requestor has no email associated with their account.""" diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index f0b0d5085..f52688e1a 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -33,8 +33,6 @@ from registrar.utility.enums import DefaultEmail from registrar.utility.errors import ( AlreadyDomainInvitedError, AlreadyDomainManagerError, - AlreadyPortfolioInvitedError, - AlreadyPortfolioMemberError, GenericError, GenericErrorCodes, MissingEmailError, @@ -1334,10 +1332,6 @@ class DomainAddUserView(DomainFormBaseView): if isinstance(exception, EmailSendingError): logger.warning("Could not send email invitation (EmailSendingError)", portfolio, exc_info=True) messages.warning(self.request, "Could not send email invitation.") - elif isinstance(exception, AlreadyPortfolioMemberError): - messages.warning(self.request, str(exception)) - elif isinstance(exception, AlreadyPortfolioInvitedError): - messages.warning(self.request, str(exception)) elif isinstance(exception, MissingEmailError): messages.error(self.request, str(exception)) logger.error( From 4572b52aad52154d000e0d9399d0987dbcfd48e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 17 Dec 2024 08:34:58 -0700 Subject: [PATCH 018/114] Removal logic --- src/registrar/forms/domain_request_wizard.py | 17 ++++++- ...0_alter_suborganization_unique_together.py | 17 ------- src/registrar/models/domain_request.py | 46 ++++++++++++++----- src/registrar/models/suborganization.py | 3 -- 4 files changed, 50 insertions(+), 33 deletions(-) delete mode 100644 src/registrar/migrations/0140_alter_suborganization_unique_together.py diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 572ef6399..eed0866f3 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -17,6 +17,7 @@ from registrar.models import Contact, DomainRequest, DraftDomain, Domain, Federa from registrar.templatetags.url_helpers import public_site_url from registrar.utility.enums import ValidationReturnType from registrar.utility.constants import BranchChoices +from django.core.exceptions import ValidationError logger = logging.getLogger(__name__) @@ -78,6 +79,20 @@ class RequestingEntityForm(RegistrarForm): # Otherwise just return the suborg as normal return self.cleaned_data.get("sub_organization") + def clean_requested_suborganization(self): + name = self.cleaned_data.get('requested_suborganization') + if name and Suborganization.objects.filter( + name__iexact=name, + portfolio=self.domain_request.portfolio, + name__isnull=False, + portfolio__isnull=False + ).exists(): + raise ValidationError( + "This suborganization already exists. " + "Choose a new name, or select it directly if you would like to use it." + ) + return name + def full_clean(self): """Validation logic to remove the custom suborganization value before clean is triggered. Without this override, the form will throw an 'invalid option' error.""" @@ -114,7 +129,7 @@ class RequestingEntityForm(RegistrarForm): if requesting_entity_is_suborganization == "True": if is_requesting_new_suborganization: # Validate custom suborganization fields - if not cleaned_data.get("requested_suborganization"): + if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: self.add_error("requested_suborganization", "Enter the name of your suborganization.") if not cleaned_data.get("suborganization_city"): self.add_error("suborganization_city", "Enter the city where your suborganization is located.") diff --git a/src/registrar/migrations/0140_alter_suborganization_unique_together.py b/src/registrar/migrations/0140_alter_suborganization_unique_together.py deleted file mode 100644 index e59ecdf2b..000000000 --- a/src/registrar/migrations/0140_alter_suborganization_unique_together.py +++ /dev/null @@ -1,17 +0,0 @@ -# Generated by Django 4.2.10 on 2024-12-16 17:02 - -from django.db import migrations - - -class Migration(migrations.Migration): - - dependencies = [ - ("registrar", "0139_alter_domainrequest_action_needed_reason"), - ] - - operations = [ - migrations.AlterUniqueTogether( - name="suborganization", - unique_together={("name", "portfolio")}, - ), - ] diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 46e188a0a..d78cd587f 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -12,6 +12,7 @@ from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTy from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes from registrar.utility.constants import BranchChoices from auditlog.models import LogEntry +from django.core.exceptions import ValidationError from registrar.utility.waffle import flag_is_active_for_user @@ -796,6 +797,7 @@ class DomainRequest(TimeStampedModel): return True def delete_and_clean_up_domain(self, called_from): + # Delete the approved domain try: # Clean up the approved domain domain_state = self.approved_domain.state @@ -808,19 +810,39 @@ class DomainRequest(TimeStampedModel): except Exception as err: logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") + + # Delete the suborg as long as this is the only place it is used + self._cleanup_dangling_suborg() - # Clean up any created suborgs, assuming its for this record only - if self.sub_organization is not None: - request_suborg_count = self.request_sub_organization.count() - domain_suborgs = self.DomainRequest_info.filter( - sub_organization=self.sub_organization - ).count() - if request_suborg_count == 1 and domain_suborgs.count() <= 1: - # if domain_suborgs.count() == 1: - # domain_info = domain_suborgs.first() - # domain_info.sub_organization = None - # domain_info.save() - self.sub_organization.delete() + def _cleanup_dangling_suborg(self): + """Deletes the existing suborg if its only being used by the deleted record""" + # Nothing to delete, so we just smile and walk away + if self.sub_organization is None: + return + + Suborganization = apps.get_model("registrar.Suborganization") + + # Stored as so because we need to set the reference to none first, + # so we can't just use the self.sub_organization property + suborg = Suborganization.objects.get(id=self.sub_organization.id) + requests = suborg.request_sub_organization + domain_infos = suborg.information_sub_organization + + # Check if this is the only reference to the suborganization + if requests.count() != 1 or domain_infos.count() > 1: + return + + # Remove the suborganization reference from request. + self.sub_organization = None + self.save() + + # Remove the suborganization reference from domain if it exists. + if domain_infos.count() == 1: + domain_infos.update(sub_organization=None) + + # Delete the now-orphaned suborganization + logger.info(f"_cleanup_dangling_suborg() -> Deleting orphan suborganization: {suborg}") + suborg.delete() def _send_status_update_email( self, diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index fb32ad48a..087490244 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -9,9 +9,6 @@ class Suborganization(TimeStampedModel): Suborganization under an organization (portfolio) """ - class Meta: - unique_together = ["name", "portfolio"] - name = models.CharField( unique=True, max_length=1000, From 24354fd7f12ee89db24c73162c3f979c31f653d3 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 17 Dec 2024 13:03:22 -0600 Subject: [PATCH 019/114] linter fixes --- src/registrar/admin.py | 5 +++-- src/registrar/tests/test_admin_request.py | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ce3b0220c..c04975cb9 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2442,7 +2442,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): obj_id = domain.id change_url = reverse("admin:%s_%s_change" % (app_label, model_name), args=[obj_id]) - message = f"
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: " # noqa + message = f"
  • The status of this domain request cannot be changed because it has been joined to a domain in Ready status: " # noqa message += f"{domain}
    • " message_html = mark_safe(message) # nosec @@ -2450,7 +2450,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): request, message_html, ) - + obj = self.get_object(request, object_id) self.display_restricted_warning(request, obj) @@ -2564,6 +2564,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): qs = qs.filter(portfolio=portfolio_id) return qs + class TransitionDomainAdmin(ListHeaderAdmin): """Custom transition domain admin class.""" diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index bbf6c1923..e109d3f96 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -95,7 +95,6 @@ class TestDomainRequestAdmin(MockEppLib): User.objects.all().delete() AllowedEmail.objects.all().delete() - @less_console_noise_decorator def test_domain_request_senior_official_is_alphabetically_sorted(self): """Tests if the senior offical dropdown is alphanetically sorted in the django admin display""" @@ -1815,7 +1814,7 @@ class TestDomainRequestAdmin(MockEppLib): request, "Cannot edit a domain request with a restricted creator.", ) - + # @less_console_noise_decorator def test_approved_domain_request_with_ready_domain_has_warning_message(self): """Tests if the domain request has a warning message when the approved domain is in Ready state""" @@ -1834,7 +1833,7 @@ class TestDomainRequestAdmin(MockEppLib): with boto3_mocking.clients.handler_for("sesv2", self.mock_client): with patch("django.contrib.messages.warning") as mock_warning: - # Create a request object + # Create a request object self.client.force_login(self.superuser) self.client.get( "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), From d2343b710e73fe0b639f94659846d1727796ef07 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 17 Dec 2024 16:19:25 -0500 Subject: [PATCH 020/114] static content on invitation and permissions change forms --- src/registrar/admin.py | 4 +++- .../assets/src/sass/_theme/_alerts.scss | 5 +++++ .../portfolio_invitation_change_form.html | 14 ++++++++++++ ...user_portfolio_permission_change_form.html | 22 +++++++++---------- 4 files changed, 32 insertions(+), 13 deletions(-) create mode 100644 src/registrar/templates/django/admin/portfolio_invitation_change_form.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index dddbb96a4..6cc52de85 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1300,6 +1300,8 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): search_fields = ["user__first_name", "user__last_name", "user__email", "portfolio__organization_name"] search_help_text = "Search by first name, last name, email, or portfolio." + change_form_template = "django/admin/user_portfolio_permission_change_form.html" + def get_roles(self, obj): readable_roles = obj.get_readable_roles() return ", ".join(readable_roles) @@ -1456,7 +1458,7 @@ class PortfolioInvitationAdmin(ListHeaderAdmin): autocomplete_fields = ["portfolio"] - change_form_template = "django/admin/email_clipboard_change_form.html" + change_form_template = "django/admin/portfolio_invitation_change_form.html" # Select portfolio invitations to change -> Portfolio invitations def changelist_view(self, request, extra_context=None): diff --git a/src/registrar/assets/src/sass/_theme/_alerts.scss b/src/registrar/assets/src/sass/_theme/_alerts.scss index 9579cc057..37e2b6b1c 100644 --- a/src/registrar/assets/src/sass/_theme/_alerts.scss +++ b/src/registrar/assets/src/sass/_theme/_alerts.scss @@ -47,3 +47,8 @@ background-color: color('base-darkest'); } } + +// Override the specificity of USWDS css to enable no max width on admin alerts +.usa-alert__body.maxw-none { + max-width: none; +} diff --git a/src/registrar/templates/django/admin/portfolio_invitation_change_form.html b/src/registrar/templates/django/admin/portfolio_invitation_change_form.html new file mode 100644 index 000000000..959e8f8bf --- /dev/null +++ b/src/registrar/templates/django/admin/portfolio_invitation_change_form.html @@ -0,0 +1,14 @@ +{% extends 'django/admin/email_clipboard_change_form.html' %} +{% load custom_filters %} +{% load i18n static %} + +{% block content_subtitle %} +
    +
    +

    + If you add someone to a portfolio here, it will trigger an invitation email when you click "save." If you don't want to trigger an email, use the User portfolio permissions table instead. +

    +
    +
    + {{ block.super }} +{% endblock %} diff --git a/src/registrar/templates/django/admin/user_portfolio_permission_change_form.html b/src/registrar/templates/django/admin/user_portfolio_permission_change_form.html index 1249a486c..489d67bc5 100644 --- a/src/registrar/templates/django/admin/user_portfolio_permission_change_form.html +++ b/src/registrar/templates/django/admin/user_portfolio_permission_change_form.html @@ -2,15 +2,13 @@ {% load custom_filters %} {% load i18n static %} -{% block field_sets %} - {% for fieldset in adminform %} - {% comment %} - This is a placeholder for now. - - Disclaimer: - When extending the fieldset view consider whether you need to make a new one that extends from detail_table_fieldset. - detail_table_fieldset is used on multiple admin pages, so a change there can have unintended consequences. - {% endcomment %} - {% include "django/admin/includes/user_portfolio_permission_fieldset.html" with original_object=original %} - {% endfor %} -{% endblock %} \ No newline at end of file +{% block content_subtitle %} +
    +
    +

    + If you add someone to a portfolio here, it will not trigger an invitation email. To trigger an email, use the Portfolio invitations table instead. +

    +
    +
    + {{ block.super }} +{% endblock %} From fb4ea590667ea0a913b99956f6c3bde44c7916b8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 17:07:46 -0500 Subject: [PATCH 021/114] member domains submit --- .../js/getgov/table-edit-member-domains.js | 139 +++++++++++++++++- .../src/sass/_theme/_register-form.scss | 11 +- .../assets/src/sass/_theme/_typography.scss | 8 + .../domain_request_status_manage.html | 4 +- .../portfolio_request_review_steps.html | 2 +- .../includes/request_review_steps.html | 6 +- .../includes/request_status_manage.html | 4 +- .../templates/includes/summary_item.html | 2 +- .../portfolio_member_domains_edit.html | 137 +++++++++++------ src/registrar/views/member_domains_json.py | 2 +- src/registrar/views/portfolios.py | 128 ++++++++++++++++ 11 files changed, 377 insertions(+), 66 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-edit-member-domains.js b/src/registrar/assets/src/js/getgov/table-edit-member-domains.js index 95492d46f..567387bd5 100644 --- a/src/registrar/assets/src/js/getgov/table-edit-member-domains.js +++ b/src/registrar/assets/src/js/getgov/table-edit-member-domains.js @@ -1,5 +1,6 @@ import { BaseTable } from './table-base.js'; +import { hideElement, showElement } from './helpers.js'; /** * EditMemberDomainsTable is used for PortfolioMember and PortfolioInvitedMember @@ -18,8 +19,14 @@ export class EditMemberDomainsTable extends BaseTable { this.initialDomainAssignmentsOnlyMember = []; // list of initially assigned domains which are readonly this.addedDomains = []; // list of domains added to member this.removedDomains = []; // list of domains removed from member + this.editModeContainer = document.getElementById('domain-assignments-edit-view'); + this.readonlyModeContainer = document.getElementById('domain-assignments-readonly-view'); + this.reviewButton = document.getElementById('review-domain-assignments'); + this.backButton = document.querySelectorAll('.back-to-edit-domain-assignments'); + this.saveButton = document.getElementById('save-domain-assignments'); this.initializeDomainAssignments(); this.initCancelEditDomainAssignmentButton(); + this.initEventListeners(); } getBaseUrl() { return document.getElementById("get_member_domains_json_url"); @@ -55,6 +62,14 @@ export class EditMemberDomainsTable extends BaseTable { getSearchParams(page, sortBy, order, searchTerm, status, portfolio) { let searchParams = super.getSearchParams(page, sortBy, order, searchTerm, status, portfolio); // Add checkedDomains to searchParams + let checkedDomains = this.getCheckedDomains(); + // Append updated checkedDomain IDs to searchParams + if (checkedDomains.length > 0) { + searchParams.append("checkedDomainIds", checkedDomains.join(",")); + } + return searchParams; + } + getCheckedDomains() { // Clone the initial domains to avoid mutating them let checkedDomains = [...this.initialDomainAssignments]; // Add IDs from addedDomains that are not already in checkedDomains @@ -70,11 +85,7 @@ export class EditMemberDomainsTable extends BaseTable { checkedDomains.splice(index, 1); } }); - // Append updated checkedDomain IDs to searchParams - if (checkedDomains.length > 0) { - searchParams.append("checkedDomainIds", checkedDomains.join(",")); - } - return searchParams; + return checkedDomains } addRow(dataObject, tbody, customTableOptions) { const domain = dataObject; @@ -217,7 +228,125 @@ export class EditMemberDomainsTable extends BaseTable { } }); } + + updateReadonlyDisplay() { + let totalAssignedDomains = this.getCheckedDomains().length; + + // Create unassigned domains list + const unassignedDomainsList = document.createElement('ul'); + unassignedDomainsList.classList.add('usa-list', 'usa-list--unstyled'); + this.removedDomains.forEach(removedDomain => { + const removedDomainListItem = document.createElement('li'); + removedDomainListItem.textContent = removedDomain.name; // Use textContent for security + unassignedDomainsList.appendChild(removedDomainListItem); + }); + + // Create assigned domains list + const assignedDomainsList = document.createElement('ul'); + assignedDomainsList.classList.add('usa-list', 'usa-list--unstyled'); + this.addedDomains.forEach(addedDomain => { + const addedDomainListItem = document.createElement('li'); + addedDomainListItem.textContent = addedDomain.name; // Use textContent for security + assignedDomainsList.appendChild(addedDomainListItem); + }); + + // Get the summary container + const domainAssignmentSummary = document.getElementById('domain-assignments-summary'); + // Clear existing content + domainAssignmentSummary.innerHTML = ''; + + // Append unassigned domains section + if (this.removedDomains.length) { + const unassignedHeader = document.createElement('h3'); + unassignedHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + unassignedHeader.textContent = 'Unassigned domains'; + domainAssignmentSummary.appendChild(unassignedHeader); + domainAssignmentSummary.appendChild(unassignedDomainsList); + } + + // Append assigned domains section + if (this.addedDomains.length) { + const assignedHeader = document.createElement('h3'); + assignedHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + assignedHeader.textContent = 'Assigned domains'; + domainAssignmentSummary.appendChild(assignedHeader); + domainAssignmentSummary.appendChild(assignedDomainsList); + } + + // Append total assigned domains section + const totalHeader = document.createElement('h3'); + totalHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + totalHeader.textContent = 'Total assigned domains'; + domainAssignmentSummary.appendChild(totalHeader); + const totalCount = document.createElement('p'); + totalCount.classList.add('margin-y-0'); + totalCount.textContent = totalAssignedDomains; + domainAssignmentSummary.appendChild(totalCount); + } + + showReadonlyMode() { + this.updateReadonlyDisplay(); + hideElement(this.editModeContainer); + showElement(this.readonlyModeContainer); + } + + showEditMode() { + hideElement(this.readonlyModeContainer); + showElement(this.editModeContainer); + } + + submitChanges() { + let memberDomainsEditForm = document.getElementById("member-domains-edit-form"); + if (memberDomainsEditForm) { + // Serialize data to send + const addedDomainIds = this.addedDomains.map(domain => domain.id); + const addedDomainsInput = document.createElement('input'); + addedDomainsInput.type = 'hidden'; + addedDomainsInput.name = 'added_domains'; // Backend will use this key to retrieve data + addedDomainsInput.value = JSON.stringify(addedDomainIds); // Stringify the array + + const removedDomainsIds = this.removedDomains.map(domain => domain.id); + const removedDomainsInput = document.createElement('input'); + removedDomainsInput.type = 'hidden'; + removedDomainsInput.name = 'removed_domains'; // Backend will use this key to retrieve data + removedDomainsInput.value = JSON.stringify(removedDomainsIds); // Stringify the array + + // Append input to the form + memberDomainsEditForm.appendChild(addedDomainsInput); + memberDomainsEditForm.appendChild(removedDomainsInput); + + memberDomainsEditForm.submit(); + } + } + + initEventListeners() { + if (this.reviewButton) { + this.reviewButton.addEventListener('click', () => { + this.showReadonlyMode(); + }); + } else { + console.warn('Missing DOM element. Expected element with id review-domain-assignments') + } + + if (this.backButton.length == 2) { + this.backButton.forEach(backbutton => { + backbutton.addEventListener('click', () => { + this.showEditMode(); + }); + }); + } else { + console.warn('Missing one or more DOM element. Expected 2 elements with class back-to-edit-domain-assignments') + } + + if (this.saveButton) { + this.saveButton.addEventListener('click', () => { + this.submitChanges(); + }); + } else { + console.warn('Missing DOM element. Expected element with id save-domain-assignments') + } + } } export function initEditMemberDomainsTable() { diff --git a/src/registrar/assets/src/sass/_theme/_register-form.scss b/src/registrar/assets/src/sass/_theme/_register-form.scss index 41d2980e3..fcc5b5ae6 100644 --- a/src/registrar/assets/src/sass/_theme/_register-form.scss +++ b/src/registrar/assets/src/sass/_theme/_register-form.scss @@ -12,7 +12,7 @@ margin-top: units(1); } -// register-form-review-header is used on the summary page and +// header--body is used on the summary page and // should not be styled like the register form headers .register-form-step h3 { color: color('primary-dark'); @@ -25,15 +25,6 @@ } } -.register-form-review-header { - color: color('primary-dark'); - margin-top: units(2); - margin-bottom: 0; - font-weight: font-weight('semibold'); - // The units mixin can only get us close, so it's between - // hardcoding the value and using in markup - font-size: 16.96px; -} .register-form-step h4 { margin-bottom: 0; diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index 466b6f975..4d89b245d 100644 --- a/src/registrar/assets/src/sass/_theme/_typography.scss +++ b/src/registrar/assets/src/sass/_theme/_typography.scss @@ -23,6 +23,14 @@ h2 { color: color('primary-darker'); } +.header--body { + margin-top: units(2); + font-weight: font-weight('semibold'); + // The units mixin can only get us close, so it's between + // hardcoding the value and using in markup + font-size: 16.96px; +} + .h4--sm-05 { font-size: size('body', 'sm'); font-weight: normal; diff --git a/src/registrar/templates/includes/domain_request_status_manage.html b/src/registrar/templates/includes/domain_request_status_manage.html index 2a254df4b..382574b31 100644 --- a/src/registrar/templates/includes/domain_request_status_manage.html +++ b/src/registrar/templates/includes/domain_request_status_manage.html @@ -213,7 +213,7 @@ {# We always show this field even if None #} {% if DomainRequest %} -

    CISA Regional Representative

    +

    CISA Regional Representative

      {% if DomainRequest.cisa_representative_first_name %} {{ DomainRequest.get_formatted_cisa_rep_name }} @@ -221,7 +221,7 @@ No {% endif %}
    -

    Anything else

    +

    Anything else

      {% if DomainRequest.anything_else %} {{DomainRequest.anything_else}} diff --git a/src/registrar/templates/includes/portfolio_request_review_steps.html b/src/registrar/templates/includes/portfolio_request_review_steps.html index fcb087090..eecc5005a 100644 --- a/src/registrar/templates/includes/portfolio_request_review_steps.html +++ b/src/registrar/templates/includes/portfolio_request_review_steps.html @@ -46,7 +46,7 @@ {% endwith %} {% if domain_request.alternative_domains.all %} -

      Alternative domains

      +

      Alternative domains

        {% for site in domain_request.alternative_domains.all %}
      • {{ site.website }}
        • diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index 73b71d536..dada2dffb 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -88,7 +88,7 @@ {% endwith %} {% if domain_request.alternative_domains.all %} -

        Alternative domains

        +

        Alternative domains

          {% for site in domain_request.alternative_domains.all %}
        • {{ site.website }}
          • @@ -132,7 +132,7 @@ {% with title=form_titles|get_item:step %} {% if domain_request.has_additional_details %} {% include "includes/summary_item.html" with title="Additional Details" value=" " heading_level=heading_level editable=is_editable edit_link=domain_request_url %} -

          CISA Regional Representative

          +

          CISA Regional Representative

            {% if domain_request.cisa_representative_first_name %}
          • {{domain_request.cisa_representative_first_name}} {{domain_request.cisa_representative_last_name}}
            • @@ -144,7 +144,7 @@ {% endif %}
          -

          Anything else

          +

          Anything else

            {% if domain_request.anything_else %} {{domain_request.anything_else}} diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index fc2fd8f12..71c6b1321 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -216,7 +216,7 @@ {# We always show this field even if None #} {% if DomainRequest %} -

            CISA Regional Representative

            +

            CISA Regional Representative

              {% if DomainRequest.cisa_representative_first_name %} {{ DomainRequest.get_formatted_cisa_rep_name }} @@ -224,7 +224,7 @@ No {% endif %}
            -

            Anything else

            +

            Anything else

              {% if DomainRequest.anything_else %} {{DomainRequest.anything_else}} diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index 15cc0f67f..bbdfc8dee 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -22,7 +22,7 @@ {% endif %} {% if sub_header_text %} -

              {{ sub_header_text }}

              +

              {{ sub_header_text }}

              {% endif %} {% if permissions %} {% include "includes/member_permissions.html" with permissions=value %} diff --git a/src/registrar/templates/portfolio_member_domains_edit.html b/src/registrar/templates/portfolio_member_domains_edit.html index d430e7572..49aff41c8 100644 --- a/src/registrar/templates/portfolio_member_domains_edit.html +++ b/src/registrar/templates/portfolio_member_domains_edit.html @@ -17,53 +17,108 @@ {% url 'invitedmember-domains' pk=portfolio_invitation.id as url3 %} {% endif %} -

              Edit domain assignments

              +
              +

              Edit domain assignments

              -

              - A domain manager can be assigned to any domain across the organization. Domain managers can change domain information, adjust DNS settings, and invite or assign other domain managers to their assigned domains. -

              -

              - When you save this form the member will get an email to notify them of any changes. -

              +

              + A domain manager can be assigned to any domain across the organization. Domain managers can change domain information, adjust DNS settings, and invite or assign other domain managers to their assigned domains. +

              +

              + When you save this form the member will get an email to notify them of any changes. +

              - {% include "includes/member_domains_edit_table.html" %} + {% include "includes/member_domains_edit_table.html" %} -
                -
              • - +
                  +
                • + -
                  • -
                • - -
                  • -
                +
                • +
              • + +
                • +
              +
              + + + +
              {% csrf_token %}
    {% endblock %} diff --git a/src/registrar/views/member_domains_json.py b/src/registrar/views/member_domains_json.py index 125059692..007730166 100644 --- a/src/registrar/views/member_domains_json.py +++ b/src/registrar/views/member_domains_json.py @@ -90,7 +90,7 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): domain_info_ids = DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True ) - domain_invitations = DomainInvitation.objects.filter(email=email).values_list("domain_id", flat=True) + domain_invitations = DomainInvitation.objects.filter(email=email, status=DomainInvitation.DomainInvitationStatus.INVITED).values_list("domain_id", flat=True) return domain_info_ids.intersection(domain_invitations) else: domain_infos = DomainInformation.objects.filter(portfolio=portfolio) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 90313339b..8482f2b01 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -1,3 +1,4 @@ +import json import logging from django.conf import settings @@ -9,7 +10,9 @@ from django.contrib import messages from registrar.forms import portfolio as portfolioForms from registrar.models import Portfolio, User +from registrar.models.domain_invitation import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation +from registrar.models.user_domain_role import UserDomainRole from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.utility.email import EmailSendingError @@ -27,6 +30,7 @@ from registrar.views.utility.permission_views import ( ) from django.views.generic import View from django.views.generic.edit import FormMixin +from django.db import IntegrityError logger = logging.getLogger(__name__) @@ -215,6 +219,58 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V "member": member, }, ) + + def post(self, request, pk): + """ + Handles adding and removing domains for a portfolio member. + """ + added_domains = request.POST.get("added_domains") + removed_domains = request.POST.get("removed_domains") + portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) + member = portfolio_permission.user + + try: + added_domain_ids = json.loads(added_domains) if added_domains else [] + except json.JSONDecodeError: + messages.error(request, "Invalid data for added domains.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + + try: + removed_domain_ids = json.loads(removed_domains) if removed_domains else [] + except json.JSONDecodeError: + messages.error(request, "Invalid data for removed domains.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + + if added_domain_ids or removed_domain_ids: + try: + if added_domain_ids: + # Bulk create UserDomainRole instances for added domains + UserDomainRole.objects.bulk_create( + [ + UserDomainRole(domain_id=domain_id, user=member) + for domain_id in added_domain_ids + ], + ignore_conflicts=True, # Avoid duplicate entries + ) + + if removed_domain_ids: + # Delete UserDomainRole instances for removed domains + UserDomainRole.objects.filter(domain_id__in=removed_domain_ids, user=member).delete() + + messages.success(request, "The domain assignment changes have been saved.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + + except IntegrityError: + messages.error(request, "A database error occurred while saving changes.") + return redirect(reverse("member-domains-edit", kwargs={"pk": pk})) + + except Exception as e: + messages.error(request, f"An unexpected error occurred: {str(e)}") + return redirect(reverse("member-domains-edit", kwargs={"pk": pk})) + else: + messages.info(request, "No changes detected.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): @@ -340,6 +396,78 @@ class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermission "portfolio_invitation": portfolio_invitation, }, ) + + def post(self, request, pk): + """ + Handles adding and removing domains for a portfolio invitee. + + Instead of deleting DomainInvitations, we move their status to CANCELED. + Instead of adding DomainIncitations, we first do a lookup and if the invite exists + we change its status to INVITED, otherwise we do a create. + """ + added_domains = request.POST.get("added_domains") + removed_domains = request.POST.get("removed_domains") + portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) + email = portfolio_invitation.email + + try: + added_domain_ids = json.loads(added_domains) if added_domains else [] + except json.JSONDecodeError: + messages.error(request, "Invalid data for added domains.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + + try: + removed_domain_ids = json.loads(removed_domains) if removed_domains else [] + except json.JSONDecodeError: + messages.error(request, "Invalid data for removed domains.") + return redirect(reverse("member-domains", kwargs={"pk": pk})) + + if added_domain_ids or removed_domain_ids: + try: + if added_domain_ids: + # Get existing invitations for the added domains + existing_invitations = DomainInvitation.objects.filter( + domain_id__in=added_domain_ids, email=email + ) + + # Update existing invitations from CANCELED to INVITED + existing_invitations.update(status=DomainInvitation.DomainInvitationStatus.INVITED) + + # Determine which domains need new invitations + existing_domain_ids = existing_invitations.values_list("domain_id", flat=True) + new_domain_ids = set(added_domain_ids) - set(existing_domain_ids) + + # Bulk create new invitations for domains without existing invitations + DomainInvitation.objects.bulk_create( + [ + DomainInvitation( + domain_id=domain_id, + email=email, + status=DomainInvitation.DomainInvitationStatus.INVITED, + ) + for domain_id in new_domain_ids + ] + ) + + if removed_domain_ids: + # Bulk update invitations for removed domains from INVITED to CANCELED + DomainInvitation.objects.filter( + domain_id__in=removed_domain_ids, email=email, status=DomainInvitation.DomainInvitationStatus.INVITED + ).update(status=DomainInvitation.DomainInvitationStatus.CANCELED) + + messages.success(request, "The domain assignment changes have been saved.") + return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) + + except IntegrityError: + messages.error(request, "A database error occurred while saving changes.") + return redirect(reverse("invitedmember-domains-edit", kwargs={"pk": pk})) + + except Exception as e: + messages.error(request, f"An unexpected error occurred: {str(e)}") + return redirect(reverse("invitedmember-domains-edit", kwargs={"pk": pk})) + else: + messages.info(request, "No changes detected.") + return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): From 14d6196b4ab10fd62ce84560e146b9639c930a5e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 18:07:43 -0500 Subject: [PATCH 022/114] lint --- .../tests/test_views_member_domains_json.py | 6 + src/registrar/tests/test_views_portfolio.py | 291 +++++++++++++++++- src/registrar/views/member_domains_json.py | 4 +- src/registrar/views/portfolios.py | 169 +++++----- 4 files changed, 395 insertions(+), 75 deletions(-) diff --git a/src/registrar/tests/test_views_member_domains_json.py b/src/registrar/tests/test_views_member_domains_json.py index c9f1e38cc..45b115842 100644 --- a/src/registrar/tests/test_views_member_domains_json.py +++ b/src/registrar/tests/test_views_member_domains_json.py @@ -94,6 +94,12 @@ class GetPortfolioMemberDomainsJsonTest(TestWithUser, WebTest): DomainInvitation.objects.create( email=cls.invited_member_email, domain=cls.domain2, status=DomainInvitation.DomainInvitationStatus.INVITED ) + DomainInvitation.objects.create( + email=cls.invited_member_email, domain=cls.domain3, status=DomainInvitation.DomainInvitationStatus.CANCELED + ) + DomainInvitation.objects.create( + email=cls.invited_member_email, domain=cls.domain4, status=DomainInvitation.DomainInvitationStatus.RETRIEVED + ) @classmethod def tearDownClass(cls): diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index de27b7059..0ebb485c9 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -14,6 +14,7 @@ from registrar.models import ( Suborganization, AllowedEmail, ) +from registrar.models.domain_invitation import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_group import UserGroup from registrar.models.user_portfolio_permission import UserPortfolioPermission @@ -25,6 +26,7 @@ from django.contrib.sessions.middleware import SessionMiddleware import boto3_mocking # type: ignore from django.test import Client import logging +import json logger = logging.getLogger(__name__) @@ -1927,7 +1929,7 @@ class TestPortfolioMemberDomainsView(TestWithUser, WebTest): cls.portfolio = Portfolio.objects.create(creator=cls.user, organization_name="Test Portfolio") # Assign permissions to the user making requests - UserPortfolioPermission.objects.create( + cls.portfolio_permission = UserPortfolioPermission.objects.create( user=cls.user, portfolio=cls.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], @@ -2106,10 +2108,15 @@ class TestPortfolioMemberDomainsEditView(TestPortfolioMemberDomainsView): @classmethod def setUpClass(cls): super().setUpClass() + cls.url = reverse("member-domains-edit", kwargs={"pk": cls.portfolio_permission.pk}) + names = ["1.gov", "2.gov", "3.gov"] + Domain.objects.bulk_create([Domain(name=name) for name in names]) @classmethod def tearDownClass(cls): super().tearDownClass() + UserDomainRole.objects.all().delete() + Domain.objects.all().delete() @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -2162,15 +2169,132 @@ class TestPortfolioMemberDomainsEditView(TestPortfolioMemberDomainsView): # Make sure the response is not found self.assertEqual(response.status_code, 404) + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_valid_added_domains(self): + """Test that domains can be successfully added.""" + self.client.force_login(self.user) + + data = { + "added_domains": json.dumps([1, 2, 3]), # Mock domain IDs + } + response = self.client.post(self.url, data) + + # Check that the UserDomainRole objects were created + self.assertEqual(UserDomainRole.objects.filter(user=self.user).count(), 3) + + # Check for a success message and a redirect + self.assertRedirects(response, reverse("member-domains", kwargs={"pk": self.portfolio_permission.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "The domain assignment changes have been saved.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_valid_removed_domains(self): + """Test that domains can be successfully removed.""" + self.client.force_login(self.user) + + # Create some UserDomainRole objects + domains = [1, 2, 3] + UserDomainRole.objects.bulk_create([UserDomainRole(domain_id=domain, user=self.user) for domain in domains]) + + data = { + "removed_domains": json.dumps([1, 2]), + } + response = self.client.post(self.url, data) + + # Check that the UserDomainRole objects were deleted + self.assertEqual(UserDomainRole.objects.filter(user=self.user).count(), 1) + self.assertEqual(UserDomainRole.objects.filter(domain_id=3, user=self.user).count(), 1) + + # Check for a success message and a redirect + self.assertRedirects(response, reverse("member-domains", kwargs={"pk": self.portfolio_permission.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "The domain assignment changes have been saved.") + + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_invalid_added_domains_data(self): + """Test that an error is returned for invalid added domains data.""" + self.client.force_login(self.user) + + data = { + "added_domains": "json-statham", + } + response = self.client.post(self.url, data) + + # Check that no UserDomainRole objects were created + self.assertEqual(UserDomainRole.objects.filter(user=self.user).count(), 0) + + # Check for an error message and a redirect + self.assertRedirects(response, reverse("member-domains", kwargs={"pk": self.portfolio_permission.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Invalid data for added domains.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_invalid_removed_domains_data(self): + """Test that an error is returned for invalid removed domains data.""" + self.client.force_login(self.user) + + data = { + "removed_domains": "not-a-json", + } + response = self.client.post(self.url, data) + + # Check that no UserDomainRole objects were deleted + self.assertEqual(UserDomainRole.objects.filter(user=self.user).count(), 0) + + # Check for an error message and a redirect + self.assertRedirects(response, reverse("member-domains", kwargs={"pk": self.portfolio_permission.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Invalid data for removed domains.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_no_changes(self): + """Test that no changes message is displayed when no changes are made.""" + self.client.force_login(self.user) + + response = self.client.post(self.url, {}) + + # Check that no UserDomainRole objects were created or deleted + self.assertEqual(UserDomainRole.objects.filter(user=self.user).count(), 0) + + # Check for an info message and a redirect + self.assertRedirects(response, reverse("member-domains", kwargs={"pk": self.portfolio_permission.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "No changes detected.") + class TestPortfolioInvitedMemberEditDomainsView(TestPortfolioInvitedMemberDomainsView): @classmethod def setUpClass(cls): super().setUpClass() + cls.url = reverse("invitedmember-domains-edit", kwargs={"pk": cls.invitation.pk}) + names = ["1.gov", "2.gov", "3.gov"] + Domain.objects.bulk_create([Domain(name=name) for name in names]) @classmethod def tearDownClass(cls): super().tearDownClass() + Domain.objects.all().delete() + + def tearDown(self): + return super().tearDown() + DomainInvitation.objects.all().delete() @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -2222,6 +2346,171 @@ class TestPortfolioInvitedMemberEditDomainsView(TestPortfolioInvitedMemberDomain # Make sure the response is not found self.assertEqual(response.status_code, 404) + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_valid_added_domains(self): + """Test adding new domains successfully.""" + self.client.force_login(self.user) + + data = { + "added_domains": json.dumps([1, 2, 3]), # Mock domain IDs + } + response = self.client.post(self.url, data) + + # Check that the DomainInvitation objects were created + self.assertEqual( + DomainInvitation.objects.filter( + email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ).count(), + 3, + ) + + # Check for a success message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "The domain assignment changes have been saved.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_existing_and_new_added_domains(self): + """Test updating existing and adding new invitations.""" + self.client.force_login(self.user) + + # Create existing invitations + DomainInvitation.objects.bulk_create( + [ + DomainInvitation( + domain_id=1, email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.CANCELED + ), + DomainInvitation( + domain_id=2, email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ), + ] + ) + + data = { + "added_domains": json.dumps([1, 2, 3]), + } + response = self.client.post(self.url, data) + + # Check that status for domain_id=1 was updated to INVITED + self.assertEqual( + DomainInvitation.objects.get(domain_id=1, email="invited@example.com").status, + DomainInvitation.DomainInvitationStatus.INVITED, + ) + + # Check that domain_id=3 was created as INVITED + self.assertTrue( + DomainInvitation.objects.filter( + domain_id=3, email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ).exists() + ) + + # Check for a success message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_valid_removed_domains(self): + """Test removing domains successfully.""" + self.client.force_login(self.user) + + # Create existing invitations + DomainInvitation.objects.bulk_create( + [ + DomainInvitation( + domain_id=1, email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ), + DomainInvitation( + domain_id=2, email="invited@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ), + ] + ) + + data = { + "removed_domains": json.dumps([1]), + } + response = self.client.post(self.url, data) + + # Check that the status for domain_id=1 was updated to CANCELED + self.assertEqual( + DomainInvitation.objects.get(domain_id=1, email="invited@example.com").status, + DomainInvitation.DomainInvitationStatus.CANCELED, + ) + + # Check that domain_id=2 remains INVITED + self.assertEqual( + DomainInvitation.objects.get(domain_id=2, email="invited@example.com").status, + DomainInvitation.DomainInvitationStatus.INVITED, + ) + + # Check for a success message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_invalid_added_domains_data(self): + """Test handling of invalid JSON for added domains.""" + self.client.force_login(self.user) + + data = { + "added_domains": "not-a-json", + } + response = self.client.post(self.url, data) + + # Check that no DomainInvitation objects were created + self.assertEqual(DomainInvitation.objects.count(), 0) + + # Check for an error message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Invalid data for added domains.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_invalid_removed_domains_data(self): + """Test handling of invalid JSON for removed domains.""" + self.client.force_login(self.user) + + data = { + "removed_domains": "json-sudeikis", + } + response = self.client.post(self.url, data) + + # Check that no DomainInvitation objects were updated + self.assertEqual(DomainInvitation.objects.count(), 0) + + # Check for an error message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Invalid data for removed domains.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_post_with_no_changes(self): + """Test the case where no changes are made.""" + self.client.force_login(self.user) + + response = self.client.post(self.url, {}) + + # Check that no DomainInvitation objects were created or updated + self.assertEqual(DomainInvitation.objects.count(), 0) + + # Check for an info message and a redirect + self.assertRedirects(response, reverse("invitedmember-domains", kwargs={"pk": self.invitation.pk})) + messages = list(response.wsgi_request._messages) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "No changes detected.") + class TestRequestingEntity(WebTest): """The requesting entity page is a domain request form that only exists diff --git a/src/registrar/views/member_domains_json.py b/src/registrar/views/member_domains_json.py index 007730166..3d24336bb 100644 --- a/src/registrar/views/member_domains_json.py +++ b/src/registrar/views/member_domains_json.py @@ -90,7 +90,9 @@ class PortfolioMemberDomainsJson(PortfolioMemberDomainsPermission, View): domain_info_ids = DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True ) - domain_invitations = DomainInvitation.objects.filter(email=email, status=DomainInvitation.DomainInvitationStatus.INVITED).values_list("domain_id", flat=True) + domain_invitations = DomainInvitation.objects.filter( + email=email, status=DomainInvitation.DomainInvitationStatus.INVITED + ).values_list("domain_id", flat=True) return domain_info_ids.intersection(domain_invitations) else: domain_infos = DomainInformation.objects.filter(portfolio=portfolio) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 8482f2b01..043d1e4a3 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -219,7 +219,7 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V "member": member, }, ) - + def post(self, request, pk): """ Handles adding and removing domains for a portfolio member. @@ -229,41 +229,23 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) member = portfolio_permission.user - try: - added_domain_ids = json.loads(added_domains) if added_domains else [] - except json.JSONDecodeError: - messages.error(request, "Invalid data for added domains.") + added_domain_ids = self._parse_domain_ids(added_domains, "added domains") + if added_domain_ids is None: return redirect(reverse("member-domains", kwargs={"pk": pk})) - try: - removed_domain_ids = json.loads(removed_domains) if removed_domains else [] - except json.JSONDecodeError: - messages.error(request, "Invalid data for removed domains.") + removed_domain_ids = self._parse_domain_ids(removed_domains, "removed domains") + if removed_domain_ids is None: return redirect(reverse("member-domains", kwargs={"pk": pk})) if added_domain_ids or removed_domain_ids: try: - if added_domain_ids: - # Bulk create UserDomainRole instances for added domains - UserDomainRole.objects.bulk_create( - [ - UserDomainRole(domain_id=domain_id, user=member) - for domain_id in added_domain_ids - ], - ignore_conflicts=True, # Avoid duplicate entries - ) - - if removed_domain_ids: - # Delete UserDomainRole instances for removed domains - UserDomainRole.objects.filter(domain_id__in=removed_domain_ids, user=member).delete() - + self._process_added_domains(added_domain_ids, member) + self._process_removed_domains(removed_domain_ids, member) messages.success(request, "The domain assignment changes have been saved.") return redirect(reverse("member-domains", kwargs={"pk": pk})) - except IntegrityError: messages.error(request, "A database error occurred while saving changes.") return redirect(reverse("member-domains-edit", kwargs={"pk": pk})) - except Exception as e: messages.error(request, f"An unexpected error occurred: {str(e)}") return redirect(reverse("member-domains-edit", kwargs={"pk": pk})) @@ -271,6 +253,34 @@ class PortfolioMemberDomainsEditView(PortfolioMemberDomainsEditPermissionView, V messages.info(request, "No changes detected.") return redirect(reverse("member-domains", kwargs={"pk": pk})) + def _parse_domain_ids(self, domain_data, domain_type): + """ + Parses the domain IDs from the request and handles JSON errors. + """ + try: + return json.loads(domain_data) if domain_data else [] + except json.JSONDecodeError: + messages.error(self.request, f"Invalid data for {domain_type}.") + return None + + def _process_added_domains(self, added_domain_ids, member): + """ + Processes added domains by bulk creating UserDomainRole instances. + """ + if added_domain_ids: + # Bulk create UserDomainRole instances for added domains + UserDomainRole.objects.bulk_create( + [UserDomainRole(domain_id=domain_id, user=member) for domain_id in added_domain_ids], + ignore_conflicts=True, # Avoid duplicate entries + ) + + def _process_removed_domains(self, removed_domain_ids, member): + """ + Processes removed domains by deleting corresponding UserDomainRole instances. + """ + if removed_domain_ids: + # Delete UserDomainRole instances for removed domains + UserDomainRole.objects.filter(domain_id__in=removed_domain_ids, user=member).delete() class PortfolioInvitedMemberView(PortfolioMemberPermissionView, View): @@ -396,72 +406,33 @@ class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermission "portfolio_invitation": portfolio_invitation, }, ) - + def post(self, request, pk): """ Handles adding and removing domains for a portfolio invitee. - - Instead of deleting DomainInvitations, we move their status to CANCELED. - Instead of adding DomainIncitations, we first do a lookup and if the invite exists - we change its status to INVITED, otherwise we do a create. """ added_domains = request.POST.get("added_domains") removed_domains = request.POST.get("removed_domains") portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) email = portfolio_invitation.email - try: - added_domain_ids = json.loads(added_domains) if added_domains else [] - except json.JSONDecodeError: - messages.error(request, "Invalid data for added domains.") - return redirect(reverse("member-domains", kwargs={"pk": pk})) + added_domain_ids = self._parse_domain_ids(added_domains, "added domains") + if added_domain_ids is None: + return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) - try: - removed_domain_ids = json.loads(removed_domains) if removed_domains else [] - except json.JSONDecodeError: - messages.error(request, "Invalid data for removed domains.") - return redirect(reverse("member-domains", kwargs={"pk": pk})) + removed_domain_ids = self._parse_domain_ids(removed_domains, "removed domains") + if removed_domain_ids is None: + return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) if added_domain_ids or removed_domain_ids: try: - if added_domain_ids: - # Get existing invitations for the added domains - existing_invitations = DomainInvitation.objects.filter( - domain_id__in=added_domain_ids, email=email - ) - - # Update existing invitations from CANCELED to INVITED - existing_invitations.update(status=DomainInvitation.DomainInvitationStatus.INVITED) - - # Determine which domains need new invitations - existing_domain_ids = existing_invitations.values_list("domain_id", flat=True) - new_domain_ids = set(added_domain_ids) - set(existing_domain_ids) - - # Bulk create new invitations for domains without existing invitations - DomainInvitation.objects.bulk_create( - [ - DomainInvitation( - domain_id=domain_id, - email=email, - status=DomainInvitation.DomainInvitationStatus.INVITED, - ) - for domain_id in new_domain_ids - ] - ) - - if removed_domain_ids: - # Bulk update invitations for removed domains from INVITED to CANCELED - DomainInvitation.objects.filter( - domain_id__in=removed_domain_ids, email=email, status=DomainInvitation.DomainInvitationStatus.INVITED - ).update(status=DomainInvitation.DomainInvitationStatus.CANCELED) - + self._process_added_domains(added_domain_ids, email) + self._process_removed_domains(removed_domain_ids, email) messages.success(request, "The domain assignment changes have been saved.") return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) - except IntegrityError: messages.error(request, "A database error occurred while saving changes.") return redirect(reverse("invitedmember-domains-edit", kwargs={"pk": pk})) - except Exception as e: messages.error(request, f"An unexpected error occurred: {str(e)}") return redirect(reverse("invitedmember-domains-edit", kwargs={"pk": pk})) @@ -469,6 +440,58 @@ class PortfolioInvitedMemberDomainsEditView(PortfolioMemberDomainsEditPermission messages.info(request, "No changes detected.") return redirect(reverse("invitedmember-domains", kwargs={"pk": pk})) + def _parse_domain_ids(self, domain_data, domain_type): + """ + Parses the domain IDs from the request and handles JSON errors. + """ + try: + return json.loads(domain_data) if domain_data else [] + except json.JSONDecodeError: + messages.error(self.request, f"Invalid data for {domain_type}.") + return None + + def _process_added_domains(self, added_domain_ids, email): + """ + Processes added domain invitations by updating existing invitations + or creating new ones. + """ + if not added_domain_ids: + return + + # Update existing invitations from CANCELED to INVITED + existing_invitations = DomainInvitation.objects.filter(domain_id__in=added_domain_ids, email=email) + existing_invitations.update(status=DomainInvitation.DomainInvitationStatus.INVITED) + + # Determine which domains need new invitations + existing_domain_ids = existing_invitations.values_list("domain_id", flat=True) + new_domain_ids = set(added_domain_ids) - set(existing_domain_ids) + + # Bulk create new invitations + DomainInvitation.objects.bulk_create( + [ + DomainInvitation( + domain_id=domain_id, + email=email, + status=DomainInvitation.DomainInvitationStatus.INVITED, + ) + for domain_id in new_domain_ids + ] + ) + + def _process_removed_domains(self, removed_domain_ids, email): + """ + Processes removed domain invitations by updating their status to CANCELED. + """ + if not removed_domain_ids: + return + + # Update invitations from INVITED to CANCELED + DomainInvitation.objects.filter( + domain_id__in=removed_domain_ids, + email=email, + status=DomainInvitation.DomainInvitationStatus.INVITED, + ).update(status=DomainInvitation.DomainInvitationStatus.CANCELED) + class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): """Some users have access to the underlying portfolio, but not any domains. From 4a73133cb7836883785b7ac46ea64488203f5fb9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 18:25:34 -0500 Subject: [PATCH 023/114] test setup revision --- .../tests/test_views_member_domains_json.py | 3 ++- src/registrar/tests/test_views_portfolio.py | 24 ++++++++++++------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_views_member_domains_json.py b/src/registrar/tests/test_views_member_domains_json.py index 45b115842..091ad6151 100644 --- a/src/registrar/tests/test_views_member_domains_json.py +++ b/src/registrar/tests/test_views_member_domains_json.py @@ -144,7 +144,8 @@ class GetPortfolioMemberDomainsJsonTest(TestWithUser, WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) def test_get_portfolio_invitedmember_domains_json_authenticated(self): - """Test that portfolio invitedmember's domains are returned properly for an authenticated user.""" + """Test that portfolio invitedmember's domains are returned properly for an authenticated user. + CANCELED and RETRIEVED invites should be ignored.""" response = self.app.get( reverse("get_member_domains_json"), params={"portfolio": self.portfolio.id, "email": self.invited_member_email, "member_only": "true"}, diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 0ebb485c9..8a1171baf 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2109,12 +2109,18 @@ class TestPortfolioMemberDomainsEditView(TestPortfolioMemberDomainsView): def setUpClass(cls): super().setUpClass() cls.url = reverse("member-domains-edit", kwargs={"pk": cls.portfolio_permission.pk}) - names = ["1.gov", "2.gov", "3.gov"] - Domain.objects.bulk_create([Domain(name=name) for name in names]) @classmethod def tearDownClass(cls): super().tearDownClass() + + def setUp(self): + super().setUp() + names = ["1.gov", "2.gov", "3.gov"] + Domain.objects.bulk_create([Domain(name=name) for name in names]) + + def tearDown(self): + super().tearDown() UserDomainRole.objects.all().delete() Domain.objects.all().delete() @@ -2284,17 +2290,19 @@ class TestPortfolioInvitedMemberEditDomainsView(TestPortfolioInvitedMemberDomain def setUpClass(cls): super().setUpClass() cls.url = reverse("invitedmember-domains-edit", kwargs={"pk": cls.invitation.pk}) - names = ["1.gov", "2.gov", "3.gov"] - Domain.objects.bulk_create([Domain(name=name) for name in names]) @classmethod def tearDownClass(cls): super().tearDownClass() - Domain.objects.all().delete() - + + def setUp(self): + super().setUp() + names = ["1.gov", "2.gov", "3.gov"] + Domain.objects.bulk_create([Domain(name=name) for name in names]) + def tearDown(self): - return super().tearDown() - DomainInvitation.objects.all().delete() + super().tearDown() + Domain.objects.all().delete() @less_console_noise_decorator @override_flag("organization_feature", active=True) From 0cb2ddb468a4b6785a4f618d082674b03274dbdd Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 18:32:47 -0500 Subject: [PATCH 024/114] teak test setup --- src/registrar/tests/test_views_portfolio.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 8a1171baf..18ceb34a6 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2303,6 +2303,7 @@ class TestPortfolioInvitedMemberEditDomainsView(TestPortfolioInvitedMemberDomain def tearDown(self): super().tearDown() Domain.objects.all().delete() + DomainInvitation.objects.all().delete() @less_console_noise_decorator @override_flag("organization_feature", active=True) From ff769df1bdbe56fd32f3574b8835aefe62b11b6e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 18:34:28 -0500 Subject: [PATCH 025/114] lint --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 18ceb34a6..a60c7adee 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2294,12 +2294,12 @@ class TestPortfolioInvitedMemberEditDomainsView(TestPortfolioInvitedMemberDomain @classmethod def tearDownClass(cls): super().tearDownClass() - + def setUp(self): super().setUp() names = ["1.gov", "2.gov", "3.gov"] Domain.objects.bulk_create([Domain(name=name) for name in names]) - + def tearDown(self): super().tearDown() Domain.objects.all().delete() From 420e64bd9efa237691a9b8564050a6e98abd91e3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 18 Dec 2024 19:04:31 -0500 Subject: [PATCH 026/114] remove bad class, fix css selector --- src/registrar/assets/src/sass/_theme/_accordions.scss | 3 ++- src/registrar/templates/portfolio_requests.html | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_accordions.scss b/src/registrar/assets/src/sass/_theme/_accordions.scss index df4f686d8..261492dd5 100644 --- a/src/registrar/assets/src/sass/_theme/_accordions.scss +++ b/src/registrar/assets/src/sass/_theme/_accordions.scss @@ -40,7 +40,8 @@ top: 30px; } -tr:last-child .usa-accordion--more-actions .usa-accordion__content { +// Special positioning for the kabob menu popup in the last row on a given page +tr:last-of-type .usa-accordion--more-actions .usa-accordion__content { top: auto; bottom: -10px; right: 30px; diff --git a/src/registrar/templates/portfolio_requests.html b/src/registrar/templates/portfolio_requests.html index 467141077..5a9197139 100644 --- a/src/registrar/templates/portfolio_requests.html +++ b/src/registrar/templates/portfolio_requests.html @@ -19,7 +19,7 @@ {% if has_edit_request_portfolio_permission %}
    -

    Domain requests can only be modified by the person who created the request.

    +

    Domain requests can only be modified by the person who created the request.

    From 682594263cb9ce6d2832e728a554fa38f47f867a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:00:15 -0700 Subject: [PATCH 027/114] Clear button --- src/registrar/admin.py | 7 ++++ .../js/getgov-admin/domain-request-form.js | 39 ++++++++++++++++++- .../helpers-portfolio-dynamic-fields.js | 17 +++++++- .../admin/domain_request_change_form.html | 11 +----- .../includes/domain_request_fieldset.html | 14 +++++++ 5 files changed, 76 insertions(+), 12 deletions(-) create mode 100644 src/registrar/templates/django/admin/includes/domain_request_fieldset.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0e8e4847a..6cbdd4878 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1817,6 +1817,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): if self.value() == "0": return queryset.filter(Q(is_election_board=False) | Q(is_election_board=None)) + @admin.display(description="Suborganization options") + def reject_suborganization_button(self, obj): + """Custom field to display the reject suborganization button""" + return "" + @admin.display(description=_("Generic Org Type")) def converted_generic_org_type(self, obj): return obj.converted_generic_org_type_display @@ -1985,6 +1990,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "requested_suborganization", "suborganization_city", "suborganization_state_territory", + "reject_suborganization_button", "creator", ] }, @@ -2106,6 +2112,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "alternative_domains", "is_election_board", "status_history", + "reject_suborganization_button", ) # Read only that we'll leverage for CISA Analysts diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 6b79a2419..2f98684f5 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -630,7 +630,44 @@ export function initRejectedEmail() { } function handleSuborganizationSelection() { - console.log("cats are cool") + const requestedSuborganizationField = document.getElementById("id_requested_suborganization"); + const suborganizationCity = document.getElementById("id_suborganization_city"); + const suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); + // The reject button is wrapped in a fieldset with a label + const rejectButtonFieldset = document.querySelector(".field-reject_suborganization_button"); + const rejectButton = document.querySelector("#clear-requested-suborganization"); + + // Ensure that every variable is present before proceeding + if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory || !rejectButtonFieldset || !rejectButton) { + console.warn("handleSuborganizationSelection() => Could not find required fields.") + return; + } + console.log("Test") + function updateRejectButtonFieldset() { + if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { + showElement(rejectButtonFieldset); + }else { + console.log("Hiding") + console.log(requestedSuborganizationField) + console.log(requestedSuborganizationField.value) + console.log(suborganizationCity.value) + console.log(suborganizationStateTerritory.value) + hideElement(rejectButtonFieldset) + } + } + + function handleRejectButton() { + // Clear the text fields + requestedSuborganizationField.value = ""; + suborganizationCity.value = ""; + suborganizationStateTerritory.value = ""; + // Update button visibility after clearing + updateRejectButtonFieldset(); + } + rejectButton.addEventListener("click", handleRejectButton) + requestedSuborganizationField.addEventListener("blur", updateRejectButtonFieldset); + suborganizationCity.addEventListener("blur", updateRejectButtonFieldset); + suborganizationStateTerritory.addEventListener("change", updateRejectButtonFieldset); } /** diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 39f30b87f..924176c31 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -87,6 +87,7 @@ export function handlePortfolioSelection() { const portfolioUrbanizationField = document.querySelector(".field-portfolio_urbanization"); const portfolioUrbanization = portfolioUrbanizationField.querySelector(".readonly"); const portfolioJsonUrl = document.getElementById("portfolio_json_url")?.value || null; + const rejectRequestedSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); let isPageLoading = true; /** @@ -507,11 +508,25 @@ export function handlePortfolioSelection() { showElement(requestedSuborganizationField); showElement(suborganizationCity); showElement(suborganizationStateTerritory); + + let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); + let suborganizationCity = document.getElementById("id_suborganization_city"); + let suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); + if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory) { + return; + } + + if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { + showElement(rejectRequestedSuborganizationButtonFieldset); + }else { + hideElement(rejectRequestedSuborganizationButtonFieldset); + } } else { // Hide suborganization request fields if suborganization is selected hideElement(requestedSuborganizationField); hideElement(suborganizationCity); - hideElement(suborganizationStateTerritory); + hideElement(suborganizationStateTerritory); + hideElement(rejectRequestedSuborganizationButtonFieldset); } } diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 46965f236..ee79aaeef 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -20,16 +20,7 @@ {% url 'get-rejection-email-for-user-json' as url_2 %} {% for fieldset in adminform %} - {% comment %} - TODO: this will eventually need to be changed to something like this - if we ever want to customize this file: - {% include "django/admin/includes/domain_information_fieldset.html" %} - - Use detail_table_fieldset as an example, or just extend it. - - original_object is just a variable name for "DomainInformation" or "DomainRequest" - {% endcomment %} - {% include "django/admin/includes/detail_table_fieldset.html" with original_object=original %} + {% include "django/admin/includes/domain_request_fieldset.html" with original_object=original %} {% endfor %} {% endblock %} diff --git a/src/registrar/templates/django/admin/includes/domain_request_fieldset.html b/src/registrar/templates/django/admin/includes/domain_request_fieldset.html new file mode 100644 index 000000000..3186fff1d --- /dev/null +++ b/src/registrar/templates/django/admin/includes/domain_request_fieldset.html @@ -0,0 +1,14 @@ +{% extends "django/admin/includes/email_clipboard_fieldset.html" %} +{% load custom_filters %} +{% load static url_helpers %} +{% load i18n static %} + + +{% block field_readonly %} + {{ block.super }} + {% if field.field.name == "reject_suborganization_button" %} + + {% endif %} +{% endblock field_readonly %} \ No newline at end of file From 9e339c2cd9a29cfb91615f3aaa77356d84d070ac Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:19:37 -0700 Subject: [PATCH 028/114] cleanup --- .../assets/src/js/getgov-admin/domain-request-form.js | 7 +------ .../getgov-admin/helpers-portfolio-dynamic-fields.js | 11 ++++++----- src/registrar/models/domain.py | 1 - 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 2f98684f5..242ae25c8 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -642,16 +642,11 @@ function handleSuborganizationSelection() { console.warn("handleSuborganizationSelection() => Could not find required fields.") return; } - console.log("Test") + function updateRejectButtonFieldset() { if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { showElement(rejectButtonFieldset); }else { - console.log("Hiding") - console.log(requestedSuborganizationField) - console.log(requestedSuborganizationField.value) - console.log(suborganizationCity.value) - console.log(suborganizationStateTerritory.value) hideElement(rejectButtonFieldset) } } diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 924176c31..5b67f8fad 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -87,7 +87,7 @@ export function handlePortfolioSelection() { const portfolioUrbanizationField = document.querySelector(".field-portfolio_urbanization"); const portfolioUrbanization = portfolioUrbanizationField.querySelector(".readonly"); const portfolioJsonUrl = document.getElementById("portfolio_json_url")?.value || null; - const rejectRequestedSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); + const rejectSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); let isPageLoading = true; /** @@ -508,7 +508,8 @@ export function handlePortfolioSelection() { showElement(requestedSuborganizationField); showElement(suborganizationCity); showElement(suborganizationStateTerritory); - + + // Initially show / hide the clear button only if there is data to clear let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); let suborganizationCity = document.getElementById("id_suborganization_city"); let suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); @@ -517,16 +518,16 @@ export function handlePortfolioSelection() { } if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { - showElement(rejectRequestedSuborganizationButtonFieldset); + showElement(rejectSuborganizationButtonFieldset); }else { - hideElement(rejectRequestedSuborganizationButtonFieldset); + hideElement(rejectSuborganizationButtonFieldset); } } else { // Hide suborganization request fields if suborganization is selected hideElement(requestedSuborganizationField); hideElement(suborganizationCity); hideElement(suborganizationStateTerritory); - hideElement(rejectRequestedSuborganizationButtonFieldset); + hideElement(rejectSuborganizationButtonFieldset); } } diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2ea78ff10..cc600e1ce 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -237,7 +237,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in From 08b985d637acaf7b5681a26b76201ce55b3f7261 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 19 Dec 2024 15:26:27 -0500 Subject: [PATCH 029/114] PortfolioMemberForm inheriting from BasePortfolioMemberForm --- src/registrar/forms/portfolio.py | 686 ++++++++++++++----------------- 1 file changed, 298 insertions(+), 388 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index c7482b224..23c9106c0 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -112,309 +112,74 @@ class PortfolioSeniorOfficialForm(forms.ModelForm): -class BasePortfolioMemberForm(forms.Form): +class BasePortfolioMemberForm(forms.ModelForm): """Base form for the PortfolioMemberForm and PortfolioInvitedMemberForm""" + required_star = '*' + role = forms.ChoiceField( + choices=[ + # Uses .value because the choice has a different label (on /admin) + (UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, "Admin access"), + (UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, "Basic access"), + ], + widget=forms.RadioSelect, + required=True, + error_messages={ + "required": "Member access level is required", + }, + ) + + domain_request_permission_admin = 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"), + ], + widget=forms.RadioSelect, + required=False, + error_messages={ + "required": "Admin domain request permission is required", + }, + ) + + member_permission_admin = forms.ChoiceField( + label=mark_safe(f"Select permission {required_star}"), # nosec + 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"), + ], + widget=forms.RadioSelect, + required=False, + error_messages={ + "required": "Basic member permission is required", + }, + ) + + ROLE_REQUIRED_FIELDS = { + UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ + "domain_request_permission_admin", + "member_permission_admin", + ], + UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ + "domain_request_permission_member", + ], + } class Meta: model = None - fields = ["portfolio", "roles", "additional_permissions"] - - # The label for each of these has a red "required" star. We can just embed that here for simplicity. - required_star = '*' - role = forms.ChoiceField( - choices=[ - # Uses .value because the choice has a different label (on /admin) - (UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, "Admin access"), - (UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, "Basic access"), - ], - widget=forms.RadioSelect, - required=True, - error_messages={ - "required": "Member access level is required", - }, - ) - - domain_request_permission_admin = 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"), - ], - widget=forms.RadioSelect, - required=False, - error_messages={ - "required": "Admin domain request permission is required", - }, - ) - - member_permission_admin = forms.ChoiceField( - label=mark_safe(f"Select permission {required_star}"), # nosec - 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"), - ], - widget=forms.RadioSelect, - required=False, - error_messages={ - "required": "Basic member permission is required", - }, - ) - - # Tracks what form elements are required for a given role choice. - # 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_MEMBER: [ - "domain_request_permission_member", - ], - } - - # def __init__(self, *args, instance=None, **kwargs): - # """Initialize self.instance, self.initial, and descriptions under each radio button. - # Uses map_instance_to_initial to set the initial dictionary.""" - # super().__init__(*args, **kwargs) - # if instance: - # self.instance = instance - # self.initial = self.map_instance_to_initial(self.instance) - # # 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 - # ), - # } - def __init__(self, *args, **kwargs): - """Initialize self.instance, self.initial, and descriptions under each radio button. - Uses map_instance_to_initial to set the initial dictionary.""" - super().__init__(*args, **kwargs) - self.fields["role"].descriptions = { - "organization_admin": UserPortfolioRoleChoices.get_role_description( - UserPortfolioRoleChoices.ORGANIZATION_ADMIN - ), - "organization_member": UserPortfolioRoleChoices.get_role_description( - UserPortfolioRoleChoices.ORGANIZATION_MEMBER - ), - } - - def save(self): - """Saves self.instance by grabbing data from self.cleaned_data. - Uses map_cleaned_data_to_instance. - """ - self.instance = self.map_cleaned_data_to_instance(self.cleaned_data, self.instance) - self.instance.save() - return self.instance - - def clean(self): - """Validates form data based on selected role and its required fields.""" - cleaned_data = super().clean() - role = cleaned_data.get("role") - - # Get required fields for the selected role. Then validate all required fields for the role. - required_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) - for field_name in required_fields: - # Helpful error for if this breaks - if field_name not in self.fields: - raise ValueError(f"ROLE_REQUIRED_FIELDS referenced a non-existent field: {field_name}.") - - if not cleaned_data.get(field_name): - 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 - - return cleaned_data - - # Explanation of how map_instance_to_initial / map_cleaned_data_to_instance work: - # map_instance_to_initial => called on init to set self.initial. - # Converts the incoming object (usually PortfolioInvitation or UserPortfolioPermission) - # into a dictionary representation for the form to use automatically. - - # map_cleaned_data_to_instance => called on save() to save the instance to the db. - # Takes the self.cleaned_data dict, and converts this dict back to the object. - - def map_instance_to_initial(self, instance): - """ - Maps self.instance to self.initial, handling roles and permissions. - Returns form data dictionary with appropriate permission levels based on user role: - { - "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 - } - """ - # Function variables - form_data = {} - perms = UserPortfolioPermission.get_portfolio_permissions( - instance.roles, instance.additional_permissions, get_list=False - ) - - # Get the available options for roles, domains, and member. - roles = [ - UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - UserPortfolioRoleChoices.ORGANIZATION_MEMBER, - ] - domain_perms = [ - UserPortfolioPermissionChoices.EDIT_REQUESTS, - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, - ] - member_perms = [ - UserPortfolioPermissionChoices.EDIT_MEMBERS, - UserPortfolioPermissionChoices.VIEW_MEMBERS, - ] - - # Build form data based on role (which options are available). - # Get which one should be "selected" by assuming that EDIT takes precedence over view, - # and ADMIN takes precedence over MEMBER. - roles = instance.roles or [] - selected_role = next((role for role in roles if role in roles), None) - form_data = {"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) - form_data["domain_request_permission_admin"] = selected_domain_permission - form_data["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") - form_data["domain_request_permission_member"] = selected_domain_permission - - return form_data - - def map_cleaned_data_to_instance(self, cleaned_data, instance): - """ - Maps self.cleaned_data to self.instance, setting roles and permissions. - Args: - cleaned_data (dict): Cleaned data containing role and permission choices - instance: Instance to update - - Returns: - instance: Updated instance - """ - role = cleaned_data.get("role") - - # Handle roles - instance.roles = [role] - - # Handle additional_permissions - valid_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) - additional_permissions = {cleaned_data.get(field) for field in valid_fields if cleaned_data.get(field)} - - # Handle EDIT permissions (should be accompanied with a view permission) - if UserPortfolioPermissionChoices.EDIT_MEMBERS in additional_permissions: - additional_permissions.add(UserPortfolioPermissionChoices.VIEW_MEMBERS) - - if UserPortfolioPermissionChoices.EDIT_REQUESTS in additional_permissions: - additional_permissions.add(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) - - # Only set unique permissions not already defined in the base role - role_permissions = UserPortfolioPermission.get_portfolio_permissions(instance.roles, [], get_list=False) - instance.additional_permissions = list(additional_permissions - role_permissions) - return instance - - -class PortfolioMemberForm(BasePortfolioMemberForm): - """ - Form for updating a portfolio member. - """ - - class Meta: - model = UserPortfolioPermission - fields = BasePortfolioMemberForm.Meta.fields + ["user"] - - -class PortfolioInvitedMemberForm(forms.ModelForm): - """ - Form for updating a portfolio invited member. - """ - required_star = '*' - role = forms.ChoiceField( - choices=[ - # Uses .value because the choice has a different label (on /admin) - (UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, "Admin access"), - (UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, "Basic access"), - ], - widget=forms.RadioSelect, - required=True, - error_messages={ - "required": "Member access level is required", - }, - ) - - domain_request_permission_admin = 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"), - ], - widget=forms.RadioSelect, - required=False, - error_messages={ - "required": "Admin domain request permission is required", - }, - ) - - member_permission_admin = forms.ChoiceField( - label=mark_safe(f"Select permission {required_star}"), # nosec - 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"), - ], - widget=forms.RadioSelect, - required=False, - error_messages={ - "required": "Basic member permission is required", - }, - ) - - ROLE_REQUIRED_FIELDS = { - UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ - "domain_request_permission_admin", - "member_permission_admin", - ], - UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ - "domain_request_permission_member", - ], - } - - class Meta: - model = PortfolioInvitation fields = ["roles", "additional_permissions" ] def __init__(self, *args, **kwargs): @@ -437,63 +202,6 @@ class PortfolioInvitedMemberForm(forms.ModelForm): if self.instance: self.map_instance_to_initial() - # def clean(self): - # logger.info(self.cleaned_data) - # # Lowercase the value of the 'email' field - # email_value = self.cleaned_data.get("email") - # if email_value: - # self.cleaned_data["email"] = email_value.lower() - - # # Get the selected role - # role = self.cleaned_data.get("role") - - # # If no member access level is selected, remove errors for hidden inputs - # if not role: - # self._remove_hidden_field_errors(exclude_fields=["email", "role"]) - # return self.cleaned_data - - # # Define field names for validation cleanup - # field_error_map = { - # "organization_admin": ["domain_request_permission_member"], # Fields irrelevant to "admin" - # "organization_member": ["domain_request_permission_admin", "member_permission_admin"], # Fields irrelevant to "basic" - # } - - # # Remove errors for irrelevant fields based on the selected access level - # irrelevant_fields = field_error_map.get(role, []) - # for field in irrelevant_fields: - # if field in self.errors: - # del self.errors[field] - - # # Map roles and additional permissions to cleaned_data - # self.cleaned_data["roles"] = [role] - # additional_permissions = [ - # self.cleaned_data.get("domain_request_permission_member"), - # self.cleaned_data.get("domain_request_permission_admin"), - # self.cleaned_data.get("member_permission_admin"), - # ] - # # Filter out None values - # self.cleaned_data["additional_permissions"] = [perm for perm in additional_permissions if perm] - # logger.info(self.cleaned_data) - # return super().clean() - - # def _remove_hidden_field_errors(self, exclude_fields=None): - # """ - # Helper method to remove errors for fields that are not relevant - # (e.g., hidden inputs), except for explicitly excluded fields. - # """ - # exclude_fields = exclude_fields or [] - # hidden_fields = [field for field in self.fields if field not in exclude_fields] - # for field in hidden_fields: - # if field in self.errors: - # del self.errors[field] - # def save(self): - # """Saves self.instance by grabbing data from self.cleaned_data. - # Uses map_cleaned_data_to_instance. - # """ - # self.instance = self.map_cleaned_data_to_instance(self.cleaned_data, self.instance) - # self.instance.save() - # return self.instance - def clean(self): """Validates form data based on selected role and its required fields.""" logger.info("clean") @@ -514,11 +222,6 @@ class PortfolioInvitedMemberForm(forms.ModelForm): if cleaned_data.get("domain_request_permission_member") == "no_access": cleaned_data["domain_request_permission_member"] = None - - logger.info("map_cleaned_data_to_instance") - logger.info(self.cleaned_data) - role = cleaned_data.get("role") - # Handle roles cleaned_data["roles"] = [role] @@ -590,39 +293,246 @@ class PortfolioInvitedMemberForm(forms.ModelForm): self.initial["domain_request_permission_member"] = selected_domain_permission logger.info(self.initial) - def map_cleaned_data_to_instance(self, cleaned_data, instance): - """ - Maps self.cleaned_data to self.instance, setting roles and permissions. - Args: - cleaned_data (dict): Cleaned data containing role and permission choices - instance: Instance to update - Returns: - instance: Updated instance - """ - logger.info("map_cleaned_data_to_instance") - logger.info(self.cleaned_data) - role = cleaned_data.get("role") + # class Meta: + # model = None + # fields = ["portfolio", "roles", "additional_permissions"] - # Handle roles - instance.roles = [role] + # # The label for each of these has a red "required" star. We can just embed that here for simplicity. + # required_star = '*' + # role = forms.ChoiceField( + # choices=[ + # # Uses .value because the choice has a different label (on /admin) + # (UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, "Admin access"), + # (UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, "Basic access"), + # ], + # widget=forms.RadioSelect, + # required=True, + # error_messages={ + # "required": "Member access level is required", + # }, + # ) - # Handle additional_permissions - valid_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) - additional_permissions = {cleaned_data.get(field) for field in valid_fields if cleaned_data.get(field)} + # domain_request_permission_admin = 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"), + # ], + # widget=forms.RadioSelect, + # required=False, + # error_messages={ + # "required": "Admin domain request permission is required", + # }, + # ) - # Handle EDIT permissions (should be accompanied with a view permission) - if UserPortfolioPermissionChoices.EDIT_MEMBERS in additional_permissions: - additional_permissions.add(UserPortfolioPermissionChoices.VIEW_MEMBERS) + # member_permission_admin = forms.ChoiceField( + # label=mark_safe(f"Select permission {required_star}"), # nosec + # 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", + # }, + # ) - if UserPortfolioPermissionChoices.EDIT_REQUESTS in additional_permissions: - additional_permissions.add(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) + # 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"), + # ], + # widget=forms.RadioSelect, + # required=False, + # error_messages={ + # "required": "Basic member permission is required", + # }, + # ) + + # # Tracks what form elements are required for a given role choice. + # # 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_MEMBER: [ + # "domain_request_permission_member", + # ], + # } + + # # def __init__(self, *args, instance=None, **kwargs): + # # """Initialize self.instance, self.initial, and descriptions under each radio button. + # # Uses map_instance_to_initial to set the initial dictionary.""" + # # super().__init__(*args, **kwargs) + # # if instance: + # # self.instance = instance + # # self.initial = self.map_instance_to_initial(self.instance) + # # # 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 + # # ), + # # } + # def __init__(self, *args, **kwargs): + # """Initialize self.instance, self.initial, and descriptions under each radio button. + # Uses map_instance_to_initial to set the initial dictionary.""" + # super().__init__(*args, **kwargs) + # self.fields["role"].descriptions = { + # "organization_admin": UserPortfolioRoleChoices.get_role_description( + # UserPortfolioRoleChoices.ORGANIZATION_ADMIN + # ), + # "organization_member": UserPortfolioRoleChoices.get_role_description( + # UserPortfolioRoleChoices.ORGANIZATION_MEMBER + # ), + # } + + # def save(self): + # """Saves self.instance by grabbing data from self.cleaned_data. + # Uses map_cleaned_data_to_instance. + # """ + # self.instance = self.map_cleaned_data_to_instance(self.cleaned_data, self.instance) + # self.instance.save() + # return self.instance + + # def clean(self): + # """Validates form data based on selected role and its required fields.""" + # cleaned_data = super().clean() + # role = cleaned_data.get("role") + + # # Get required fields for the selected role. Then validate all required fields for the role. + # required_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) + # for field_name in required_fields: + # # Helpful error for if this breaks + # if field_name not in self.fields: + # raise ValueError(f"ROLE_REQUIRED_FIELDS referenced a non-existent field: {field_name}.") + + # if not cleaned_data.get(field_name): + # 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 + + # return cleaned_data + + # # Explanation of how map_instance_to_initial / map_cleaned_data_to_instance work: + # # map_instance_to_initial => called on init to set self.initial. + # # Converts the incoming object (usually PortfolioInvitation or UserPortfolioPermission) + # # into a dictionary representation for the form to use automatically. + + # # map_cleaned_data_to_instance => called on save() to save the instance to the db. + # # Takes the self.cleaned_data dict, and converts this dict back to the object. + + # def map_instance_to_initial(self, instance): + # """ + # Maps self.instance to self.initial, handling roles and permissions. + # Returns form data dictionary with appropriate permission levels based on user role: + # { + # "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 + # } + # """ + # # Function variables + # form_data = {} + # perms = UserPortfolioPermission.get_portfolio_permissions( + # instance.roles, instance.additional_permissions, get_list=False + # ) + + # # Get the available options for roles, domains, and member. + # roles = [ + # UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + # UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + # ] + # domain_perms = [ + # UserPortfolioPermissionChoices.EDIT_REQUESTS, + # UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + # ] + # member_perms = [ + # UserPortfolioPermissionChoices.EDIT_MEMBERS, + # UserPortfolioPermissionChoices.VIEW_MEMBERS, + # ] + + # # Build form data based on role (which options are available). + # # Get which one should be "selected" by assuming that EDIT takes precedence over view, + # # and ADMIN takes precedence over MEMBER. + # roles = instance.roles or [] + # selected_role = next((role for role in roles if role in roles), None) + # form_data = {"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) + # form_data["domain_request_permission_admin"] = selected_domain_permission + # form_data["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") + # form_data["domain_request_permission_member"] = selected_domain_permission + + # return form_data + + # def map_cleaned_data_to_instance(self, cleaned_data, instance): + # """ + # Maps self.cleaned_data to self.instance, setting roles and permissions. + # Args: + # cleaned_data (dict): Cleaned data containing role and permission choices + # instance: Instance to update + + # Returns: + # instance: Updated instance + # """ + # role = cleaned_data.get("role") + + # # Handle roles + # instance.roles = [role] + + # # Handle additional_permissions + # valid_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) + # additional_permissions = {cleaned_data.get(field) for field in valid_fields if cleaned_data.get(field)} + + # # Handle EDIT permissions (should be accompanied with a view permission) + # if UserPortfolioPermissionChoices.EDIT_MEMBERS in additional_permissions: + # additional_permissions.add(UserPortfolioPermissionChoices.VIEW_MEMBERS) + + # if UserPortfolioPermissionChoices.EDIT_REQUESTS in additional_permissions: + # additional_permissions.add(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) + + # # Only set unique permissions not already defined in the base role + # role_permissions = UserPortfolioPermission.get_portfolio_permissions(instance.roles, [], get_list=False) + # instance.additional_permissions = list(additional_permissions - role_permissions) + # return instance + + +class PortfolioMemberForm(BasePortfolioMemberForm): + """ + Form for updating a portfolio member. + """ + + class Meta: + model = UserPortfolioPermission + fields = ["roles", "additional_permissions" ] + + +class PortfolioInvitedMemberForm(BasePortfolioMemberForm): + """ + Form for updating a portfolio invited member. + """ + + class Meta: + model = PortfolioInvitation + fields = ["roles", "additional_permissions" ] - # Only set unique permissions not already defined in the base role - role_permissions = UserPortfolioPermission.get_portfolio_permissions(instance.roles, [], get_list=False) - instance.additional_permissions = list(additional_permissions - role_permissions) - return instance - class PortfolioNewMemberForm(forms.ModelForm): From be657e188b89c10431063e163e103068ab978dc0 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 19 Dec 2024 15:57:01 -0500 Subject: [PATCH 030/114] new portfolio member form inheriting from base --- .../src/js/getgov/portfolio-member-page.js | 7 +- src/registrar/forms/portfolio.py | 310 +----------------- .../templates/portfolio_members_add_new.html | 24 +- 3 files changed, 8 insertions(+), 333 deletions(-) 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 d8e8b6a9b..cfb83badc 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -7,7 +7,6 @@ import { hookupRadioTogglerListener } from './radios.js'; // This is specifically for the Member Profile (Manage Member) Page member/invitation removal export function initPortfolioNewMemberPageToggle() { document.addEventListener("DOMContentLoaded", () => { - console.log("initPortfolioNewMemberPageToggle"); const wrapperDeleteAction = document.getElementById("wrapper-delete-action") if (wrapperDeleteAction) { const member_type = wrapperDeleteAction.getAttribute("data-member-type"); @@ -51,7 +50,6 @@ export function initPortfolioNewMemberPageToggle() { * on the Add New Member page. */ export function initAddNewMemberPageListeners() { - console.log("initializing add new member page listeners"); let add_member_form = document.getElementById("add_member_form"); if (!add_member_form){ return; @@ -152,7 +150,7 @@ export function initAddNewMemberPageListeners() { document.getElementById('modalEmail').textContent = emailValue; // Get selected radio button for access level - let selectedAccess = document.querySelector('input[name="member_access_level"]:checked'); + 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"; @@ -188,9 +186,8 @@ export function initPortfolioMemberPageRadio() { } ); }else if (newMemberForm){ - console.log("initializing listeners") hookupRadioTogglerListener( - 'member_access_level', + 'role', { 'organization_admin': 'new-member-admin-permissions', 'organization_member': 'new-member-basic-permissions' diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 23c9106c0..888340d40 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -294,226 +294,6 @@ class BasePortfolioMemberForm(forms.ModelForm): logger.info(self.initial) - # class Meta: - # model = None - # fields = ["portfolio", "roles", "additional_permissions"] - - # # The label for each of these has a red "required" star. We can just embed that here for simplicity. - # required_star = '*' - # role = forms.ChoiceField( - # choices=[ - # # Uses .value because the choice has a different label (on /admin) - # (UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, "Admin access"), - # (UserPortfolioRoleChoices.ORGANIZATION_MEMBER.value, "Basic access"), - # ], - # widget=forms.RadioSelect, - # required=True, - # error_messages={ - # "required": "Member access level is required", - # }, - # ) - - # domain_request_permission_admin = 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"), - # ], - # widget=forms.RadioSelect, - # required=False, - # error_messages={ - # "required": "Admin domain request permission is required", - # }, - # ) - - # member_permission_admin = forms.ChoiceField( - # label=mark_safe(f"Select permission {required_star}"), # nosec - # 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"), - # ], - # widget=forms.RadioSelect, - # required=False, - # error_messages={ - # "required": "Basic member permission is required", - # }, - # ) - - # # Tracks what form elements are required for a given role choice. - # # 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_MEMBER: [ - # "domain_request_permission_member", - # ], - # } - - # # def __init__(self, *args, instance=None, **kwargs): - # # """Initialize self.instance, self.initial, and descriptions under each radio button. - # # Uses map_instance_to_initial to set the initial dictionary.""" - # # super().__init__(*args, **kwargs) - # # if instance: - # # self.instance = instance - # # self.initial = self.map_instance_to_initial(self.instance) - # # # 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 - # # ), - # # } - # def __init__(self, *args, **kwargs): - # """Initialize self.instance, self.initial, and descriptions under each radio button. - # Uses map_instance_to_initial to set the initial dictionary.""" - # super().__init__(*args, **kwargs) - # self.fields["role"].descriptions = { - # "organization_admin": UserPortfolioRoleChoices.get_role_description( - # UserPortfolioRoleChoices.ORGANIZATION_ADMIN - # ), - # "organization_member": UserPortfolioRoleChoices.get_role_description( - # UserPortfolioRoleChoices.ORGANIZATION_MEMBER - # ), - # } - - # def save(self): - # """Saves self.instance by grabbing data from self.cleaned_data. - # Uses map_cleaned_data_to_instance. - # """ - # self.instance = self.map_cleaned_data_to_instance(self.cleaned_data, self.instance) - # self.instance.save() - # return self.instance - - # def clean(self): - # """Validates form data based on selected role and its required fields.""" - # cleaned_data = super().clean() - # role = cleaned_data.get("role") - - # # Get required fields for the selected role. Then validate all required fields for the role. - # required_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) - # for field_name in required_fields: - # # Helpful error for if this breaks - # if field_name not in self.fields: - # raise ValueError(f"ROLE_REQUIRED_FIELDS referenced a non-existent field: {field_name}.") - - # if not cleaned_data.get(field_name): - # 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 - - # return cleaned_data - - # # Explanation of how map_instance_to_initial / map_cleaned_data_to_instance work: - # # map_instance_to_initial => called on init to set self.initial. - # # Converts the incoming object (usually PortfolioInvitation or UserPortfolioPermission) - # # into a dictionary representation for the form to use automatically. - - # # map_cleaned_data_to_instance => called on save() to save the instance to the db. - # # Takes the self.cleaned_data dict, and converts this dict back to the object. - - # def map_instance_to_initial(self, instance): - # """ - # Maps self.instance to self.initial, handling roles and permissions. - # Returns form data dictionary with appropriate permission levels based on user role: - # { - # "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 - # } - # """ - # # Function variables - # form_data = {} - # perms = UserPortfolioPermission.get_portfolio_permissions( - # instance.roles, instance.additional_permissions, get_list=False - # ) - - # # Get the available options for roles, domains, and member. - # roles = [ - # UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - # UserPortfolioRoleChoices.ORGANIZATION_MEMBER, - # ] - # domain_perms = [ - # UserPortfolioPermissionChoices.EDIT_REQUESTS, - # UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, - # ] - # member_perms = [ - # UserPortfolioPermissionChoices.EDIT_MEMBERS, - # UserPortfolioPermissionChoices.VIEW_MEMBERS, - # ] - - # # Build form data based on role (which options are available). - # # Get which one should be "selected" by assuming that EDIT takes precedence over view, - # # and ADMIN takes precedence over MEMBER. - # roles = instance.roles or [] - # selected_role = next((role for role in roles if role in roles), None) - # form_data = {"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) - # form_data["domain_request_permission_admin"] = selected_domain_permission - # form_data["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") - # form_data["domain_request_permission_member"] = selected_domain_permission - - # return form_data - - # def map_cleaned_data_to_instance(self, cleaned_data, instance): - # """ - # Maps self.cleaned_data to self.instance, setting roles and permissions. - # Args: - # cleaned_data (dict): Cleaned data containing role and permission choices - # instance: Instance to update - - # Returns: - # instance: Updated instance - # """ - # role = cleaned_data.get("role") - - # # Handle roles - # instance.roles = [role] - - # # Handle additional_permissions - # valid_fields = self.ROLE_REQUIRED_FIELDS.get(role, []) - # additional_permissions = {cleaned_data.get(field) for field in valid_fields if cleaned_data.get(field)} - - # # Handle EDIT permissions (should be accompanied with a view permission) - # if UserPortfolioPermissionChoices.EDIT_MEMBERS in additional_permissions: - # additional_permissions.add(UserPortfolioPermissionChoices.VIEW_MEMBERS) - - # if UserPortfolioPermissionChoices.EDIT_REQUESTS in additional_permissions: - # additional_permissions.add(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) - - # # Only set unique permissions not already defined in the base role - # role_permissions = UserPortfolioPermission.get_portfolio_permissions(instance.roles, [], get_list=False) - # instance.additional_permissions = list(additional_permissions - role_permissions) - # return instance - - class PortfolioMemberForm(BasePortfolioMemberForm): """ Form for updating a portfolio member. @@ -535,47 +315,7 @@ class PortfolioInvitedMemberForm(BasePortfolioMemberForm): -class PortfolioNewMemberForm(forms.ModelForm): - member_access_level = forms.ChoiceField( - label="Select permission", - choices=[("organization_admin", "Admin Access"), ("organization_member", "Basic Access")], - widget=forms.RadioSelect(attrs={"class": "usa-radio__input usa-radio__input--tile"}), - required=True, - error_messages={ - "required": "Member access level is required", - }, - ) - admin_org_domain_request_permissions = forms.ChoiceField( - label="Select permission", - choices=[("view_all_requests", "View all requests"), ("edit_requests", "View all requests plus create requests")], - widget=forms.RadioSelect, - required=True, - error_messages={ - "required": "Admin domain request permission is required", - }, - ) - admin_org_members_permissions = forms.ChoiceField( - label="Select permission", - choices=[("view_members", "View all members"), ("edit_members", "View all members plus manage members")], - widget=forms.RadioSelect, - required=True, - error_messages={ - "required": "Admin member permission is required", - }, - ) - basic_org_domain_request_permissions = forms.ChoiceField( - label="Select permission", - choices=[ - ("view_all_requests", "View all requests"), - ("edit_requests", "View all requests plus create requests"), - ("", "No access"), - ], - widget=forms.RadioSelect, - required=True, - error_messages={ - "required": "Basic member permission is required", - }, - ) +class PortfolioNewMemberForm(BasePortfolioMemberForm): email = forms.EmailField( label="Enter the email of the member you'd like to invite", @@ -597,51 +337,3 @@ class PortfolioNewMemberForm(forms.ModelForm): model = PortfolioInvitation fields = ["portfolio", "email", "roles", "additional_permissions"] - def clean(self): - # Lowercase the value of the 'email' field - email_value = self.cleaned_data.get("email") - if email_value: - self.cleaned_data["email"] = email_value.lower() - - # Get the selected member access level - member_access_level = self.cleaned_data.get("member_access_level") - - # If no member access level is selected, remove errors for hidden inputs - if not member_access_level: - self._remove_hidden_field_errors(exclude_fields=["email", "member_access_level"]) - return self.cleaned_data - - # Define field names for validation cleanup - field_error_map = { - "organization_admin": ["basic_org_domain_request_permissions"], # Fields irrelevant to "admin" - "organization_member": ["admin_org_domain_request_permissions", "admin_org_members_permissions"], # Fields irrelevant to "basic" - } - - # Remove errors for irrelevant fields based on the selected access level - irrelevant_fields = field_error_map.get(member_access_level, []) - for field in irrelevant_fields: - if field in self.errors: - del self.errors[field] - - # Map roles and additional permissions to cleaned_data - self.cleaned_data["roles"] = [member_access_level] - additional_permissions = [ - self.cleaned_data.get("admin_org_domain_request_permissions"), - self.cleaned_data.get("basic_org_domain_request_permissions"), - self.cleaned_data.get("admin_org_members_permissions"), - ] - # Filter out None values - self.cleaned_data["additional_permissions"] = [perm for perm in additional_permissions if perm] - - return super().clean() - - def _remove_hidden_field_errors(self, exclude_fields=None): - """ - Helper method to remove errors for fields that are not relevant - (e.g., hidden inputs), except for explicitly excluded fields. - """ - exclude_fields = exclude_fields or [] - hidden_fields = [field for field in self.fields if field not in exclude_fields] - for field in hidden_fields: - if field in self.errors: - del self.errors[field] diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index a441c15b1..792ef3ce2 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -56,22 +56,8 @@ Select the level of access for this member. * - {% with group_classes="usa-form-editable usa-form-editable--no-border padding-top-0" %} -

    - {% for radio in form.member_access_level %} - {{ radio.tag }} - - {% endfor %} -
    + {% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %} + {% input_with_errors form.role %} {% endwith %} @@ -85,7 +71,7 @@ text-primary-dark margin-bottom-0">Organization domain requests {% with group_classes="usa-form-editable usa-form-editable--no-border padding-top-0" %} - {% input_with_errors form.admin_org_domain_request_permissions %} + {% input_with_errors form.domain_request_permission_admin %} {% endwith %}

    Organization members

    {% with group_classes="usa-form-editable usa-form-editable--no-border padding-top-0" %} - {% input_with_errors form.admin_org_members_permissions %} + {% input_with_errors form.member_permission_admin %} {% endwith %}
    @@ -104,7 +90,7 @@

    Organization domain requests

    {% with group_classes="usa-form-editable usa-form-editable--no-border padding-top-0" %} - {% input_with_errors form.basic_org_domain_request_permissions %} + {% input_with_errors form.domain_request_permission_member %} {% endwith %}
    From de8d252136abd911dd3d4439f0f086dacc0a9660 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 14:05:06 -0700 Subject: [PATCH 031/114] unit tests --- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/common.py | 16 +++ src/registrar/tests/test_models_requests.py | 142 ++++++++++++++++++++ 3 files changed, 159 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index d78cd587f..c0d4c7e57 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -810,7 +810,7 @@ class DomainRequest(TimeStampedModel): except Exception as err: logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") - + # Delete the suborg as long as this is the only place it is used self._cleanup_dangling_suborg() diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index e1f4f5a27..1c345b83b 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -1034,6 +1034,10 @@ def completed_domain_request( # noqa action_needed_reason=None, portfolio=None, organization_name=None, + sub_organization=None, + requested_suborganization=None, + suborganization_city=None, + suborganization_state_territory=None, ): """A completed domain request.""" if not user: @@ -1098,6 +1102,18 @@ def completed_domain_request( # noqa if portfolio: domain_request_kwargs["portfolio"] = portfolio + if sub_organization: + domain_request_kwargs["sub_organization"] = sub_organization + + if requested_suborganization: + domain_request_kwargs["requested_suborganization"] = requested_suborganization + + if suborganization_city: + domain_request_kwargs["suborganization_city"] = suborganization_city + + if suborganization_state_territory: + domain_request_kwargs["suborganization_state_territory"] = suborganization_state_territory + domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) if has_other_contacts: diff --git a/src/registrar/tests/test_models_requests.py b/src/registrar/tests/test_models_requests.py index da474224c..983a12b3c 100644 --- a/src/registrar/tests/test_models_requests.py +++ b/src/registrar/tests/test_models_requests.py @@ -15,6 +15,7 @@ from registrar.models import ( FederalAgency, AllowedEmail, Portfolio, + Suborganization, ) import boto3_mocking @@ -23,6 +24,8 @@ from registrar.utility.errors import FSMDomainRequestError from .common import ( MockSESClient, + create_user, + create_superuser, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1070,3 +1073,142 @@ class TestDomainRequest(TestCase): ) self.assertEqual(domain_request2.generic_org_type, domain_request2.converted_generic_org_type) self.assertEqual(domain_request2.federal_agency, domain_request2.converted_federal_agency) + + +class TestDomainRequestSuborganization(TestCase): + """Tests for the suborganization fields on domain requests""" + + def setUp(self): + super().setUp() + self.user = create_user() + self.superuser = create_superuser() + + def tearDown(self): + super().tearDown() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + Domain.objects.all().delete() + Suborganization.objects.all().delete() + Portfolio.objects.all().delete() + + @less_console_noise_decorator + def test_approve_creates_requested_suborganization(self): + """Test that approving a domain request with a requested suborganization creates it""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + requested_suborganization="Boom", + suborganization_city="Explody town", + suborganization_state_territory=DomainRequest.StateTerritoryChoices.OHIO, + ) + domain_request.investigator = self.superuser + domain_request.save() + + domain_request.approve() + + created_suborg = Suborganization.objects.filter( + name="Boom", + city="Explody town", + state_territory=DomainRequest.StateTerritoryChoices.OHIO, + portfolio=portfolio, + ).first() + + self.assertIsNotNone(created_suborg) + self.assertEqual(domain_request.sub_organization, created_suborg) + + @less_console_noise_decorator + def test_approve_without_requested_suborganization_makes_no_changes(self): + """Test that approving without a requested suborganization doesn't create one""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + domain_request.investigator = self.superuser + domain_request.save() + + initial_suborg_count = Suborganization.objects.count() + domain_request.approve() + + self.assertEqual(Suborganization.objects.count(), initial_suborg_count) + self.assertIsNone(domain_request.sub_organization) + + @less_console_noise_decorator + def test_approve_with_existing_suborganization_makes_no_changes(self): + """Test that approving with an existing suborganization doesn't create a new one""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + existing_suborg = Suborganization.objects.create(name="Existing Division", portfolio=portfolio) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=existing_suborg, + ) + domain_request.investigator = self.superuser + domain_request.save() + + initial_suborg_count = Suborganization.objects.count() + domain_request.approve() + + self.assertEqual(Suborganization.objects.count(), initial_suborg_count) + self.assertEqual(domain_request.sub_organization, existing_suborg) + + @less_console_noise_decorator + def test_cleanup_dangling_suborg_with_single_reference(self): + """Test that a suborganization is deleted when it's only referenced once""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + suborg = Suborganization.objects.create(name="Test Division", portfolio=portfolio) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + domain_request.approve() + + # set it back to in review + domain_request.in_review() + domain_request.refresh_from_db() + + # Verify the suborganization was deleted + self.assertFalse(Suborganization.objects.filter(id=suborg.id).exists()) + self.assertIsNone(domain_request.sub_organization) + + @less_console_noise_decorator + def test_cleanup_dangling_suborg_with_multiple_references(self): + """Test that a suborganization is preserved when it has multiple references""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + suborg = Suborganization.objects.create(name="Test Division", portfolio=portfolio) + + # Create two domain requests using the same suborganization + domain_request1 = completed_domain_request( + name="test1.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + domain_request2 = completed_domain_request( + name="test2.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + + domain_request1.approve() + domain_request2.approve() + + # set one back to in review + domain_request1.in_review() + domain_request1.refresh_from_db() + + # Verify the suborganization still exists + self.assertTrue(Suborganization.objects.filter(id=suborg.id).exists()) + self.assertEqual(domain_request1.sub_organization, suborg) + self.assertEqual(domain_request2.sub_organization, suborg) From 4e4e2c0d8eba85553883cd8392086db7e9f07f3d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 19 Dec 2024 16:57:48 -0500 Subject: [PATCH 032/114] Fix background color bug --- src/registrar/assets/src/sass/_theme/_base.scss | 5 +++-- src/registrar/assets/src/sass/_theme/_forms.scss | 4 ++++ src/registrar/templates/portfolio_member_permissions.html | 8 ++++---- src/registrar/templates/portfolio_members_add_new.html | 8 ++++---- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index 8d475270b..193b71c58 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -39,7 +39,8 @@ body { padding-top: units(5)!important; } -#wrapper.dashboard--grey-1 { +#wrapper.dashboard--grey-1, +.bg-gray-1 { background-color: color('gray-1'); } @@ -260,4 +261,4 @@ abbr[title] { margin: 0; height: 1.5em; width: 1.5em; -} \ No newline at end of file +} diff --git a/src/registrar/assets/src/sass/_theme/_forms.scss b/src/registrar/assets/src/sass/_theme/_forms.scss index 9158de174..4138c5878 100644 --- a/src/registrar/assets/src/sass/_theme/_forms.scss +++ b/src/registrar/assets/src/sass/_theme/_forms.scss @@ -78,3 +78,7 @@ legend.float-left-tablet + button.float-right-tablet { .read-only-value { margin-top: units(0); } + +.bg-gray-1 .usa-radio { + background: color('gray-1'); +} diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index 66becaa9e..f0983b4f7 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -12,7 +12,7 @@ {% include "includes/form_errors.html" with form=form %} -