From a2e6238b18ae36413962d3ab5d3bad19e4cd9084 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 12 Dec 2024 17:38:28 -0500 Subject: [PATCH 01/52] 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 02/52] 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 03/52] 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 04/52] 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 05/52] 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 06/52] 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 d888c616b8f8a87ca1ce54bb7e13b945ae06b5a1 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Dec 2024 13:13:41 -0500 Subject: [PATCH 07/52] 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 08/52] 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 060f2d5c8acca4a733311970d4057e33b1576e32 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 16 Dec 2024 17:13:35 -0500 Subject: [PATCH 09/52] 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 10/52] 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 11/52] 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 12/52] 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 d2343b710e73fe0b639f94659846d1727796ef07 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 17 Dec 2024 16:19:25 -0500 Subject: [PATCH 13/52] 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 14/52] 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 15/52] 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 16/52] 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 17/52] 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 18/52] 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 19/52] 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 08b985d637acaf7b5681a26b76201ce55b3f7261 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 19 Dec 2024 15:26:27 -0500 Subject: [PATCH 20/52] 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 21/52] 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 4e4e2c0d8eba85553883cd8392086db7e9f07f3d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 19 Dec 2024 16:57:48 -0500 Subject: [PATCH 22/52] 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 %} -