From caad00df188983273e8628d7e677e0e162cf1720 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 10:24:29 -0700 Subject: [PATCH 001/103] Block users invited to other orgs from being domain managers --- src/registrar/views/domain.py | 56 +++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index db0572bb30..dae3b60cd9 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -21,8 +21,10 @@ DomainRequest, DomainInformation, DomainInvitation, + PortfolioInvitation, User, UserDomainRole, + UserPortfolioPermission, PublicContact, ) from registrar.utility.enums import DefaultEmail @@ -38,6 +40,7 @@ ) from registrar.models.utility.contact_error import ContactError from registrar.views.utility.permission_views import UserDomainRolePermissionDeleteView +from registrar.utility.waffle import flag_is_active_for_user from ..forms import ( SeniorOfficialContactForm, @@ -778,7 +781,14 @@ 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 _send_domain_invitation_email(self, email: str, requestor: User, add_success=True): + def _is_member_of_different_org(self, email, org): + """Verifies if an email belongs to a different organization as a member or invited member.""" + # Check if user is a member of a different organization + existing_org_permission = UserPortfolioPermission.objects.get(email=email) + print("Existing org permission: ", existing_org_permission) + return True + + def _send_domain_invitation_email(self, email: str, requestor: User, requested_user=None, add_success=True): """Performs the sending of the domain invitation email, does not make a domain information object email: string- email to send to @@ -803,6 +813,26 @@ def _send_domain_invitation_email(self, email: str, requestor: User, add_success ) return None + # Check is user is a member or invited member of a different org from this domain's org + print("org feature flag is active: ", flag_is_active_for_user(requestor, "organization_feature")) + if flag_is_active_for_user(requestor, "organization_feature"): + # Check if invited user is a member from a different org from this domain's org + existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() + print("Existing org permission for requested email: ", existing_org_permission) + + existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() + requestor_org = UserPortfolioPermission.objects.get(user=requestor).portfolio + print("Requestor org: ", requestor_org) + if (existing_org_permission and existing_org_permission.portfolio != requestor_org) or \ + (existing_org_invitation and existing_org_invitation.portfolio != requestor_org): + add_success=False + messages.error( + self.request, + f"That email is already a member of another .gov organization.", + ) + raise Exception + + # Check to see if an invite has already been sent try: invite = DomainInvitation.objects.get(email=email, domain=self.object) @@ -868,7 +898,7 @@ def form_valid(self, form): else: # if user already exists then just send an email try: - self._send_domain_invitation_email(requested_email, requestor, add_success=False) + self._send_domain_invitation_email(requested_email, requestor, requested_user=requested_user, add_success=False) except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -883,17 +913,17 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - - try: - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - else: - messages.success(self.request, f"Added user {requested_email}.") + else: + try: + UserDomainRole.objects.create( + user=requested_user, + domain=self.object, + role=UserDomainRole.Roles.MANAGER, + ) + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") + else: + messages.success(self.request, f"Added user {requested_email}.") return redirect(self.get_success_url()) From 42de7f2bb79e76b9efcb242da853ed036c3959bc Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 10:40:36 -0700 Subject: [PATCH 002/103] Add domain manager breadcrumb nav --- src/registrar/templates/domain_add_user.html | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index b2f9fef24d..320404fa98 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -4,6 +4,19 @@ {% block title %}Add a domain manager | {% endblock %} {% block domain_content %} + {% block breadcrumb %} + {% url 'domain-users' pk=domain.id as url %} + + {% endblock breadcrumb %}

Add a domain manager

You can add another user to help manage your domain. They will need to sign From 8b61eb1275f2d340c458898d22baccb1a7c53b4a Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 10:43:32 -0700 Subject: [PATCH 003/103] Update add domain manager page content --- src/registrar/templates/domain_add_user.html | 4 ++-- src/registrar/views/domain.py | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index 320404fa98..e95bacd76f 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -19,8 +19,8 @@ {% endblock breadcrumb %}

Add a domain manager

-

You can add another user to help manage your domain. They will need to sign - in to the .gov registrar with their Login.gov account. +

You can add another user to help manage your domain. If they aren't an organization member they will + need to sign in to the .gov registrar with their Login.gov account.

diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index dae3b60cd9..c3bbe037ab 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -823,15 +823,15 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() requestor_org = UserPortfolioPermission.objects.get(user=requestor).portfolio print("Requestor org: ", requestor_org) - if (existing_org_permission and existing_org_permission.portfolio != requestor_org) or \ - (existing_org_invitation and existing_org_invitation.portfolio != requestor_org): - add_success=False + if (existing_org_permission and existing_org_permission.portfolio != requestor_org) or ( + existing_org_invitation and existing_org_invitation.portfolio != requestor_org + ): + add_success = False messages.error( self.request, f"That email is already a member of another .gov organization.", ) raise Exception - # Check to see if an invite has already been sent try: @@ -898,7 +898,9 @@ def form_valid(self, form): else: # if user already exists then just send an email try: - self._send_domain_invitation_email(requested_email, requestor, requested_user=requested_user, add_success=False) + self._send_domain_invitation_email( + requested_email, requestor, requested_user=requested_user, add_success=False + ) except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", From b0fe698af2e1d6ae5bf586ff2345b48f7a75f98c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 14:45:10 -0700 Subject: [PATCH 004/103] Add error code for outside org members being added --- src/registrar/utility/errors.py | 8 ++++ src/registrar/views/domain.py | 66 ++++++++++++++++----------------- 2 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 8cb83c0ee4..6a75091a69 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -23,6 +23,14 @@ class InvalidDomainError(ValueError): pass +class OutsideOrgMemberError(ValueError): + """ + Error raised when an org member tries adding a user from a different .gov org. + To be deleted when users can be members of multiple orgs. + """ + + pass + class ActionNotAllowed(Exception): """User accessed an action that is not allowed by the current state""" diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index c3bbe037ab..92da57e067 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -35,6 +35,7 @@ NameserverErrorCodes as nsErrorCodes, DsDataError, DsDataErrorCodes, + OutsideOrgMemberError, SecurityEmailError, SecurityEmailErrorCodes, ) @@ -781,12 +782,15 @@ 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, org): + def _is_member_of_different_org(self, email, requested_user, org): """Verifies if an email belongs to a different organization as a member or invited member.""" - # Check if user is a member of a different organization - existing_org_permission = UserPortfolioPermission.objects.get(email=email) - print("Existing org permission: ", existing_org_permission) - return True + # Check if user is a already member of a different organization than the given org + 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 != org) or ( + existing_org_invitation and existing_org_invitation.portfolio != org + ) def _send_domain_invitation_email(self, email: str, requestor: User, requested_user=None, add_success=True): """Performs the sending of the domain invitation email, @@ -814,24 +818,16 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u return None # Check is user is a member or invited member of a different org from this domain's org - print("org feature flag is active: ", flag_is_active_for_user(requestor, "organization_feature")) if flag_is_active_for_user(requestor, "organization_feature"): # Check if invited user is a member from a different org from this domain's org - existing_org_permission = UserPortfolioPermission.objects.filter(user=requested_user).first() - print("Existing org permission for requested email: ", existing_org_permission) - - existing_org_invitation = PortfolioInvitation.objects.filter(email=email).first() requestor_org = UserPortfolioPermission.objects.get(user=requestor).portfolio - print("Requestor org: ", requestor_org) - if (existing_org_permission and existing_org_permission.portfolio != requestor_org) or ( - existing_org_invitation and existing_org_invitation.portfolio != requestor_org - ): + if self._is_member_of_different_org(email, requested_user, requestor_org): add_success = False messages.error( self.request, - f"That email is already a member of another .gov organization.", + "That email is already a member of another .gov organization.", ) - raise Exception + raise OutsideOrgMemberError # Check to see if an invite has already been sent try: @@ -847,10 +843,7 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u add_success = False # else if it has been sent but not accepted messages.warning(self.request, f"{email} has already been invited to this domain") - except Exception: - logger.error("An error occured") - try: send_templated_email( "emails/domain_invitation.txt", "emails/domain_invitation_subject.txt", @@ -861,6 +854,11 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u "requestor_email": requestor_email, }, ) + + if add_success: + messages.success(self.request, f"{email} has been invited to this domain.") + except Exception: + logger.error("An error occured") except EmailSendingError as exc: logger.warn( "Could not sent email invitation to %s for domain %s", @@ -869,9 +867,6 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u 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 to this domain.") def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" @@ -901,6 +896,14 @@ def form_valid(self, form): self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) + + 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)", @@ -908,6 +911,12 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") + except OutsideOrgMemberError: + logger.warn( + "Could not send email invitation to a user in a different org.", + self.object, + exc_info=True, + ) except Exception: logger.warn( "Could not send email invitation (Other Exception)", @@ -915,17 +924,8 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - else: - try: - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - else: - 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") return redirect(self.get_success_url()) From d71069606b77bfb5e593d2f01659c89ffb732f7c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 14:51:50 -0700 Subject: [PATCH 005/103] Fix linting. Revert to original email sending logic --- src/registrar/utility/errors.py | 1 + src/registrar/views/domain.py | 56 +++++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 6a75091a69..f12aba2213 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -31,6 +31,7 @@ class OutsideOrgMemberError(ValueError): pass + class ActionNotAllowed(Exception): """User accessed an action that is not allowed by the current state""" diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 92da57e067..77c02d990e 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -35,9 +35,9 @@ NameserverErrorCodes as nsErrorCodes, DsDataError, DsDataErrorCodes, - OutsideOrgMemberError, SecurityEmailError, SecurityEmailErrorCodes, + OutsideOrgMemberError, ) from registrar.models.utility.contact_error import ContactError from registrar.views.utility.permission_views import UserDomainRolePermissionDeleteView @@ -859,6 +859,30 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u messages.success(self.request, f"{email} has been invited to this domain.") except Exception: logger.error("An error occured") + except OutsideOrgMemberError: + logger.error( + "Could not send email. Can not invite member of a .gov organization to a different organization." + ) + 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 + + 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", @@ -867,6 +891,9 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u 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 to this domain.") def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" @@ -896,14 +923,6 @@ def form_valid(self, form): self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) - - 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)", @@ -911,12 +930,6 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - except OutsideOrgMemberError: - logger.warn( - "Could not send email invitation to a user in a different org.", - self.object, - exc_info=True, - ) except Exception: logger.warn( "Could not send email invitation (Other Exception)", @@ -924,8 +937,17 @@ def form_valid(self, form): 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") + else: + try: + UserDomainRole.objects.create( + user=requested_user, + domain=self.object, + role=UserDomainRole.Roles.MANAGER, + ) + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") + else: + messages.success(self.request, f"Added user {requested_email}.") return redirect(self.get_success_url()) From d66ff330572280fd7b6f935dcca32317e23ca2db Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 14:52:59 -0700 Subject: [PATCH 006/103] Readd outside org member error handling --- src/registrar/views/domain.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 77c02d990e..02019c6014 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -930,6 +930,12 @@ def form_valid(self, form): 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, + ) except Exception: logger.warn( "Could not send email invitation (Other Exception)", From 3d1781c4f657aa1152ad88b0df15a1c7a99c34d0 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 26 Sep 2024 15:07:50 -0700 Subject: [PATCH 007/103] Fix linting --- src/registrar/views/domain.py | 59 ++++++++++------------------------- 1 file changed, 16 insertions(+), 43 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 02019c6014..c2ca65bab7 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -782,14 +782,15 @@ 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, requested_user, org): + 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 given org + # Check if user is a already member of a different organization than the requestor's org + requestor_org = UserPortfolioPermission.objects.get(user=requestor).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 != org) or ( - existing_org_invitation and existing_org_invitation.portfolio != org + 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(self, email: str, requestor: User, requested_user=None, add_success=True): @@ -818,16 +819,15 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u return None # 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"): - # Check if invited user is a member from a different org from this domain's org - requestor_org = UserPortfolioPermission.objects.get(user=requestor).portfolio - if self._is_member_of_different_org(email, requested_user, requestor_org): - add_success = False - messages.error( - self.request, - "That email is already a member of another .gov organization.", - ) - raise OutsideOrgMemberError + if flag_is_active_for_user(requestor, "organization_feature") and self._is_member_of_different_org( + email, requestor, requested_user + ): + add_success = False + messages.error( + self.request, + "That email is already a member of another .gov organization.", + ) + raise OutsideOrgMemberError # Check to see if an invite has already been sent try: @@ -843,34 +843,8 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u add_success = False # else if it has been sent but not accepted messages.warning(self.request, f"{email} has already been invited to this domain") - - 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, - }, - ) - - if add_success: - messages.success(self.request, f"{email} has been invited to this domain.") except Exception: logger.error("An error occured") - except OutsideOrgMemberError: - logger.error( - "Could not send email. Can not invite member of a .gov organization to a different organization." - ) - 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 try: send_templated_email( @@ -883,6 +857,8 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u "requestor_email": requestor_email, }, ) + if add_success: + messages.success(self.request, f"{email} has been invited to this domain.") except EmailSendingError as exc: logger.warn( "Could not sent email invitation to %s for domain %s", @@ -891,9 +867,6 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u 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 to this domain.") def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" From 117900cfb9f12d4ca65f007d212d5337acc4d2ce Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 30 Sep 2024 11:24:58 -0700 Subject: [PATCH 008/103] Revert to try else catch --- src/registrar/views/domain.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index c2ca65bab7..5d7a840c78 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -857,8 +857,6 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u "requestor_email": requestor_email, }, ) - if add_success: - messages.success(self.request, f"{email} has been invited to this domain.") except EmailSendingError as exc: logger.warn( "Could not sent email invitation to %s for domain %s", @@ -867,6 +865,9 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u 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 to this domain.") def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" From ee59c6a074c67b8686154b2a8d1a58b18c79ecbe Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 3 Oct 2024 02:03:12 -0600 Subject: [PATCH 009/103] Updated view only verbage. Tooltip mods --- src/registrar/assets/js/uswds-edited.js | 24 ++++++++++++++---- .../assets/sass/_theme/_tooltips.scss | 25 +++++++++++++++++++ src/registrar/templates/domain_detail.html | 3 ++- src/registrar/tests/test_views_domain.py | 2 +- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index e73f3b6c0b..c01aabea38 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5938,6 +5938,10 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { return offset; }; + const element_is_fixed_positioned = false + const parentRect = tooltipTrigger.getBoundingClientRect(); + const element_left = element_is_fixed_positioned ? parentRect.left + parentRect.width : `50%` + /** * Positions tooltip at the top * @param {HTMLElement} e - this is the tooltip body @@ -5949,7 +5953,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const topMargin = calculateMarginOffset("top", e.offsetHeight, tooltipTrigger); const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("top"); - e.style.left = `50%`; // center the element + e.style.left = element_left; // center the element e.style.top = `-${TRIANGLE_SIZE}px`; // consider the pseudo element // apply our margins based on the offset e.style.margin = `-${topMargin}px 0 0 -${leftMargin / 2}px`; @@ -5963,7 +5967,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { resetPositionStyles(e); const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("bottom"); - e.style.left = `50%`; + e.style.left = element_left; e.style.margin = `${TRIANGLE_SIZE}px 0 0 -${leftMargin / 2}px`; }; @@ -5975,7 +5979,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { resetPositionStyles(e); const topMargin = calculateMarginOffset("top", e.offsetHeight, tooltipTrigger); setPositionClass("right"); - e.style.top = `50%`; + e.style.top = element_left; e.style.left = `${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; e.style.margin = `-${topMargin / 2}px 0 0 0`; }; @@ -5991,7 +5995,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { // we have to check for some utility margins const leftMargin = calculateMarginOffset("left", tooltipTrigger.offsetLeft > e.offsetWidth ? tooltipTrigger.offsetLeft - e.offsetWidth : e.offsetWidth, tooltipTrigger); setPositionClass("left"); - e.style.top = `50%`; + e.style.top = element_left; e.style.left = `-${TRIANGLE_SIZE}px`; e.style.margin = `-${topMargin / 2}px 0 0 ${tooltipTrigger.offsetLeft > e.offsetWidth ? leftMargin : -leftMargin}px`; // adjust the margin }; @@ -6017,6 +6021,10 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { if (i < positions.length) { const pos = positions[i]; pos(element); + + const rect = element.getBoundingClientRect(); + console.log("***RECTANGLE**** "+rect.width); + if (!isElementInViewport(element)) { // eslint-disable-next-line no-param-reassign tryPositions(i += 1); @@ -6128,7 +6136,13 @@ const setUpAttributes = tooltipTrigger => { tooltipBody.setAttribute("aria-hidden", "true"); // place the text in the tooltip - tooltipBody.textContent = tooltipContent; + // DOTGOV: nest elements for tooltip to prevent clipping (works around viewport calcs) + tooltipBody.innerHTML = ` +

+ ${tooltipContent} +

` + // tooltipBody.textContent = tooltipContent; + return { tooltipBody, position, diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss index 3ab630dc0c..dc1309ad22 100644 --- a/src/registrar/assets/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -28,3 +28,28 @@ #extended-logo .usa-tooltip__body { font-weight: 400 !important; } + +.usa-tooltip__body > p { + margin-top: 0; + width: 40vw; + text-wrap: wrap; + text-align: center; + font-size: .9rem; + margin-block-start: 0em; + margin-block-end: 0em; + max-width: fit-content; + @include at-media('tablet') { + width: 70vw; + } +} + +.usa-tooltip__body { + white-space: inherit; + max-width: fit-content; + // position: fixed; +} + +.usa-tooltip__body--wrap { + min-width: inherit; + width: inherit; +} \ No newline at end of file diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index dca68f6ef0..5cb559a5ae 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -45,7 +45,8 @@

{{ domain.name }}

- To manage information for this domain, you must add yourself as a domain manager. + You don't have access to manage {{domain.name}}. If you need to make updates, contact one of the listed domain managers. + Alternatively, an admin for your organization can assign this domain to you by updating your member permissions.

diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 8fb92df721..aa2fc10c06 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -340,7 +340,7 @@ def test_domain_readonly_on_detail_page(self): detail_page = self.client.get(f"/domain/{domain.id}") # Check that alert message displays properly self.assertContains( - detail_page, "To manage information for this domain, you must add yourself as a domain manager." + detail_page, "You don't have access to manage "+domain.name+". If you need to make updates, contact one of the listed domain managers." ) # Check that user does not have option to Edit domain self.assertNotContains(detail_page, "Edit") From 64b7a5a953a5715ac5dd7ef4a4c261b3c542f7db Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 3 Oct 2024 02:45:52 -0600 Subject: [PATCH 010/103] Fixed position mode (works around overflow constraints with scrollable containers) --- src/registrar/assets/js/uswds-edited.js | 26 ++++++++++++++----- .../assets/sass/_theme/_tooltips.scss | 2 +- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index c01aabea38..928e3fc2b2 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5938,9 +5938,17 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { return offset; }; - const element_is_fixed_positioned = false + const style = window.getComputedStyle(tooltipBody); + // Check if the position property is 'fixed' + if (style.position === 'fixed') { + console.log('The element has a fixed position.'); + } else { + console.log('The element does not have a fixed position.'); + } + const element_is_fixed_positioned = style.position === 'fixed'; const parentRect = tooltipTrigger.getBoundingClientRect(); - const element_left = element_is_fixed_positioned ? parentRect.left + parentRect.width : `50%` + const element_left = element_is_fixed_positioned ? parentRect.left + parentRect.width/2 + 'px': `50%` + const element_top = element_is_fixed_positioned ? parentRect.top + parentRect.height/2 + 'px': `50%` /** * Positions tooltip at the top @@ -5954,7 +5962,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("top"); e.style.left = element_left; // center the element - e.style.top = `-${TRIANGLE_SIZE}px`; // consider the pseudo element + e.style.top = element_is_fixed_positioned ?`${parentRect.top-TRIANGLE_SIZE}px`:`-${TRIANGLE_SIZE}px`; // consider the pseudo element // apply our margins based on the offset e.style.margin = `-${topMargin}px 0 0 -${leftMargin / 2}px`; }; @@ -5967,6 +5975,9 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { resetPositionStyles(e); const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("bottom"); + if (element_is_fixed_positioned){ + e.style.top = parentRect.bottom+'px'; + } e.style.left = element_left; e.style.margin = `${TRIANGLE_SIZE}px 0 0 -${leftMargin / 2}px`; }; @@ -5979,8 +5990,9 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { resetPositionStyles(e); const topMargin = calculateMarginOffset("top", e.offsetHeight, tooltipTrigger); setPositionClass("right"); - e.style.top = element_left; - e.style.left = `${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; + e.style.top = element_top; + e.style.left = element_is_fixed_positioned ? `${parentRect.right + TRIANGLE_SIZE}px`:`${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; +; e.style.margin = `-${topMargin / 2}px 0 0 0`; }; @@ -5995,8 +6007,8 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { // we have to check for some utility margins const leftMargin = calculateMarginOffset("left", tooltipTrigger.offsetLeft > e.offsetWidth ? tooltipTrigger.offsetLeft - e.offsetWidth : e.offsetWidth, tooltipTrigger); setPositionClass("left"); - e.style.top = element_left; - e.style.left = `-${TRIANGLE_SIZE}px`; + e.style.top = element_top; + e.style.left = element_is_fixed_positioned ? `${parentRect.left-TRIANGLE_SIZE}px` : `-${TRIANGLE_SIZE}px`; e.style.margin = `-${topMargin / 2}px 0 0 ${tooltipTrigger.offsetLeft > e.offsetWidth ? leftMargin : -leftMargin}px`; // adjust the margin }; diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss index dc1309ad22..66706ccb44 100644 --- a/src/registrar/assets/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -46,7 +46,7 @@ .usa-tooltip__body { white-space: inherit; max-width: fit-content; - // position: fixed; + position: fixed; } .usa-tooltip__body--wrap { From e57cf6e39a33f1a44838b66dcfeb26ade346dbf2 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 3 Oct 2024 02:53:05 -0600 Subject: [PATCH 011/103] some cleanup --- src/registrar/assets/js/uswds-edited.js | 26 +++++++------------ .../assets/sass/_theme/_tooltips.scss | 2 +- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 928e3fc2b2..a1eff9a12d 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5938,17 +5938,11 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { return offset; }; - const style = window.getComputedStyle(tooltipBody); - // Check if the position property is 'fixed' - if (style.position === 'fixed') { - console.log('The element has a fixed position.'); - } else { - console.log('The element does not have a fixed position.'); - } - const element_is_fixed_positioned = style.position === 'fixed'; - const parentRect = tooltipTrigger.getBoundingClientRect(); - const element_left = element_is_fixed_positioned ? parentRect.left + parentRect.width/2 + 'px': `50%` - const element_top = element_is_fixed_positioned ? parentRect.top + parentRect.height/2 + 'px': `50%` + const tooltipStyle = window.getComputedStyle(tooltipBody); + const tooltipIsFixedPositioned = tooltipStyle.position === 'fixed'; + const triggerRect = tooltipTrigger.getBoundingClientRect(); + const element_left = tooltipIsFixedPositioned ? triggerRect.left + triggerRect.width/2 + 'px': `50%` + const element_top = tooltipIsFixedPositioned ? triggerRect.top + triggerRect.height/2 + 'px': `50%` /** * Positions tooltip at the top @@ -5962,7 +5956,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("top"); e.style.left = element_left; // center the element - e.style.top = element_is_fixed_positioned ?`${parentRect.top-TRIANGLE_SIZE}px`:`-${TRIANGLE_SIZE}px`; // consider the pseudo element + e.style.top = tooltipIsFixedPositioned ?`${triggerRect.top-TRIANGLE_SIZE}px`:`-${TRIANGLE_SIZE}px`; // consider the pseudo element // apply our margins based on the offset e.style.margin = `-${topMargin}px 0 0 -${leftMargin / 2}px`; }; @@ -5975,8 +5969,8 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { resetPositionStyles(e); const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("bottom"); - if (element_is_fixed_positioned){ - e.style.top = parentRect.bottom+'px'; + if (tooltipIsFixedPositioned){ + e.style.top = triggerRect.bottom+'px'; } e.style.left = element_left; e.style.margin = `${TRIANGLE_SIZE}px 0 0 -${leftMargin / 2}px`; @@ -5991,7 +5985,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const topMargin = calculateMarginOffset("top", e.offsetHeight, tooltipTrigger); setPositionClass("right"); e.style.top = element_top; - e.style.left = element_is_fixed_positioned ? `${parentRect.right + TRIANGLE_SIZE}px`:`${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; + e.style.left = tooltipIsFixedPositioned ? `${triggerRect.right + TRIANGLE_SIZE}px`:`${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; ; e.style.margin = `-${topMargin / 2}px 0 0 0`; }; @@ -6008,7 +6002,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", tooltipTrigger.offsetLeft > e.offsetWidth ? tooltipTrigger.offsetLeft - e.offsetWidth : e.offsetWidth, tooltipTrigger); setPositionClass("left"); e.style.top = element_top; - e.style.left = element_is_fixed_positioned ? `${parentRect.left-TRIANGLE_SIZE}px` : `-${TRIANGLE_SIZE}px`; + e.style.left = tooltipIsFixedPositioned ? `${triggerRect.left-TRIANGLE_SIZE}px` : `-${TRIANGLE_SIZE}px`; e.style.margin = `-${topMargin / 2}px 0 0 ${tooltipTrigger.offsetLeft > e.offsetWidth ? leftMargin : -leftMargin}px`; // adjust the margin }; diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss index 66706ccb44..20834f2aad 100644 --- a/src/registrar/assets/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -31,7 +31,7 @@ .usa-tooltip__body > p { margin-top: 0; - width: 40vw; + width: 50vw; text-wrap: wrap; text-align: center; font-size: .9rem; From f10b4d82855ba961ae7ba773f39ee7945bed669b Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 10:33:45 -0700 Subject: [PATCH 012/103] Create code review guide --- .github/pull_request_template.md | 2 +- docs/dev-practices/code_review.md | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 docs/dev-practices/code_review.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index dec0b9fac8..4f2349204f 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,6 +1,6 @@ ## Ticket -Resolves #00 +Resolves #001 ## Changes @@ -45,15 +40,10 @@ All other changes require just a single approving review.--> - [ ] Met the acceptance criteria, or will meet them in a subsequent PR - [ ] Created/modified automated tests -- [ ] Added at least 2 developers as PR reviewers (only 1 will need to approve) -- [ ] Messaged on Slack or in standup to notify the team that a PR is ready for review -- [ ] Changes to “how we do things” are documented in READMEs and or onboarding guide -- [ ] If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited. +- [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) -- [ ] All new functions and methods are commented using plain language -- [ ] Did dependency updates in Pipfile also get changed in requirements.txt? - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values @@ -62,24 +52,16 @@ All other changes require just a single approving review.--> - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) -- [ ] Add at least 1 designer as PR reviewer ### As a code reviewer, I have #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Reviewed this code and left comments +- [ ] Verified code meets code standards and comments if any standards above are not satisfied +- [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged. - [ ] Checked that all code is adequately covered by tests -- [ ] Made it clear which comments need to be addressed before this work is merged -- [ ] If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited. - -#### Ensured code standards are met (Code reviewer) - -- [ ] All new functions and methods are commented using plain language -- [ ] Interactions with external systems are wrapped in try/except -- [ ] Error handling exists for unusual or missing values -- [ ] (Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt? +- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. #### Validated user-facing changes as a developer @@ -88,12 +70,6 @@ All other changes require just a single approving review.--> - [ ] Meets all designs and user flows provided by design/product - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) -- [ ] Tested with multiple browsers, the suggestion is to use ones that the developer didn't (check off which ones were used) - - [ ] Chrome - - [ ] Microsoft Edge - - [ ] FireFox - - [ ] Safari - - [ ] (Rarely needed) Tested as both an analyst and applicant user **Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist @@ -103,10 +79,9 @@ All other changes require just a single approving review.--> #### Verified that the changes match the design intention - [ ] Checked that the design translated visually -- [ ] Checked behavior +- [ ] Checked behavior. Comment any found issues or broken flows. - [ ] Checked different states (empty, one, some, error) - [ ] Checked for landmarks, page heading structure, and links -- [ ] Tried to break the intended flow #### Validated user-facing changes as a designer diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 56d4db3948..38ed832323 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,6 +1,21 @@ -# Code Review +## Code Review After creating a pull request, pull request submitters should: -- Add at least 2 developers as PR reviewers (only 1 will need to approve) -- Message on Slack or in standup to notify the team that a PR is ready for review -- If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. \ No newline at end of file +- Add at least 2 developers as PR reviewers (only 1 will need to approve). +- Message on Slack or in standup to notify the team that a PR is ready for review. +- If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. +- If any updated dependencies on Pipfile, also update dependencies in requirements.txt. + +## Pull Requests for User-facing changes +Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. + +When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. + +## Coding standards +(The Coding standards section may be moved to a new code standards file in a future ticket. +For now we're simply moving PR template content into the code review document for consolidation) + +### Plain language +All functions and methods should use plain language. + +TODO: Description and examples in code standards ticket. \ No newline at end of file From 93ee3b0b8f3901bb14889585ef90906adac83692 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:46:13 -0700 Subject: [PATCH 017/103] Refactor spacing --- .github/pull_request_template.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e493e0a921..e2340bebeb 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -43,7 +43,8 @@ Resolves #001 - [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) - + +N/A - no external systems or errors, this is just docs refactoring. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values @@ -58,7 +59,7 @@ Resolves #001 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets code standards and comments if any standards above are not satisfied +- [ ] Verified code meets above code standards and user-facing checks. Addresses any checks that are not satisfied - [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged. - [ ] Checked that all code is adequately covered by tests - [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. @@ -85,7 +86,7 @@ Resolves #001 #### Validated user-facing changes as a designer -- [ ] Checked keyboard navigability +- [ ] Checked different states (empty, one, some, error) - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - [ ] Tested with multiple browsers (check off which ones were used) From 2bdef1e01ff802855fcd92ade0a6bd0cd705764f Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:47:13 -0700 Subject: [PATCH 018/103] Fix spacing --- .github/pull_request_template.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e2340bebeb..351ce579bd 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -70,7 +70,6 @@ N/A - no external systems or errors, this is just docs refactoring. - [ ] Checked keyboard navigability - [ ] Meets all designs and user flows provided by design/product - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - - [ ] (Rarely needed) Tested as both an analyst and applicant user **Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist From de4161dde44f3dce9d96329540e82d8be1d14285 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:47:49 -0700 Subject: [PATCH 019/103] Remove unused content --- .github/pull_request_template.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 351ce579bd..4d3b767462 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -44,7 +44,6 @@ Resolves #001 #### Ensured code standards are met (Original Developer) -N/A - no external systems or errors, this is just docs refactoring. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values From 3997251eb19c9c77748f1afbe3b875ecfe92329e Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:49:19 -0700 Subject: [PATCH 020/103] Add browser section --- .github/pull_request_template.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4d3b767462..a3646c40a2 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -87,11 +87,11 @@ Resolves #001 - [ ] Checked different states (empty, one, some, error) - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) -- [ ] Tested with multiple browsers (check off which ones were used) - - [ ] Chrome - - [ ] Microsoft Edge - - [ ] FireFox - - [ ] Safari +#### Test support on multiple browsers. Check the browser(s) tested. +- [ ] Chrome +- [ ] Microsoft Edge +- [ ] FireFox +- [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user From 7ced4b73e0f38fb3ee6a5ec9d1756bef68df130a Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 3 Oct 2024 15:52:26 -0600 Subject: [PATCH 021/103] linted + remove test content from tooltip --- src/registrar/assets/js/uswds-edited.js | 5 +---- src/registrar/tests/test_views_domain.py | 5 ++++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 49a87c4bd9..11a71b0dfd 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -6161,10 +6161,7 @@ const setUpAttributes = tooltipTrigger => { // DOTGOV: nest the text element to allow us creater control over width and wrapping behavior tooltipBody.innerHTML = `
- ${tooltipContent} - - n oainef aoieiu aw eghr hilabiuyabewisofuha libfasuiybefiae ruhawioeufh aiwfh iahf iuahefailusef aiwsfbali wefbaiue fbaliuefbalieuwfhauiowera jhfasiuf aiuwenail ewfasdn fiausfn iuafia ewfn ia fisfn iuf niuwnf iwenfailuhfiauefn aliefnaifnialsudnf aiufnailufnailefialenf ailefia fa filanf ilaefiunaifalfn ailfnialuefn ialuefnailf lifniasn filsa fnialn fila fi af ai fniaufn ilaufn ial fia fnila fiua fnilaefn ialuefn ial efailf ia fnial fia fniu ialf nailf a fal f Before this domain can be used, you’ll need to add name server addresses. - + ${tooltipContent}
` // tooltipBody.textContent = tooltipContent; diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index aa2fc10c06..928cea43e1 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -340,7 +340,10 @@ def test_domain_readonly_on_detail_page(self): detail_page = self.client.get(f"/domain/{domain.id}") # Check that alert message displays properly self.assertContains( - detail_page, "You don't have access to manage "+domain.name+". If you need to make updates, contact one of the listed domain managers." + detail_page, + "You don't have access to manage " + + domain.name + + ". If you need to make updates, contact one of the listed domain managers.", ) # Check that user does not have option to Edit domain self.assertNotContains(detail_page, "Edit") From fc421ce0578621e47bd34f33c38913ddcae7fcd7 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:56:38 -0700 Subject: [PATCH 022/103] Update code review doc --- docs/dev-practices/code_review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 38ed832323..aa3d134048 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -18,4 +18,4 @@ For now we're simply moving PR template content into the code review document fo ### Plain language All functions and methods should use plain language. -TODO: Description and examples in code standards ticket. \ No newline at end of file +TODO: Plain language description and examples in code standards ticket. \ No newline at end of file From c553ebb773df1e1544f406f9004fe89fd6ba9732 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:57:57 -0700 Subject: [PATCH 023/103] Fix punctuation --- .github/pull_request_template.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index a3646c40a2..ecf117f15a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -59,9 +59,9 @@ Resolves #001 - [ ] Pulled this branch locally and tested it - [ ] Verified code meets above code standards and user-facing checks. Addresses any checks that are not satisfied -- [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged. +- [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged - [ ] Checked that all code is adequately covered by tests -- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. +- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` #### Validated user-facing changes as a developer From d933da326cb30f6c40cc735cbc13b676c0a586f7 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 3 Oct 2024 18:44:34 -0400 Subject: [PATCH 024/103] wip --- src/registrar/forms/__init__.py | 1 + src/registrar/forms/portfolio.py | 31 ++++ .../templates/includes/header_extended.html | 14 +- src/registrar/templates/portfolio_member.html | 25 ++-- src/registrar/views/portfolio_members_json.py | 139 +++++++++++------- src/registrar/views/portfolios.py | 36 ++++- 6 files changed, 169 insertions(+), 77 deletions(-) diff --git a/src/registrar/forms/__init__.py b/src/registrar/forms/__init__.py index 033e955ed9..121e2b3f7d 100644 --- a/src/registrar/forms/__init__.py +++ b/src/registrar/forms/__init__.py @@ -13,4 +13,5 @@ ) from .portfolio import ( PortfolioOrgAddressForm, + PortfolioMemberForm, ) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 14a45f6ae4..2b669c50cc 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,6 +4,9 @@ from django import forms from django.core.validators import RegexValidator +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices + from ..models import DomainInformation, Portfolio, SeniorOfficial logger = logging.getLogger(__name__) @@ -95,3 +98,31 @@ def clean(self): cleaned_data = super().clean() cleaned_data.pop("full_name", None) return cleaned_data + + +class PortfolioMemberForm(forms.ModelForm): + """ + Form for updating a portfolio member. + """ + + roles = forms.MultipleChoiceField( + choices=UserPortfolioRoleChoices.choices, + widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + required=False, + label="Roles", + ) + + additional_permissions = forms.MultipleChoiceField( + choices=UserPortfolioPermissionChoices.choices, + widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + required=False, + label="Additional Permissions", + ) + + class Meta: + model = UserPortfolioPermission + fields = [ + "roles", + "additional_permissions", + ] + diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index a3b2364a9d..43467602e8 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -91,12 +91,14 @@ {% endif %} - {% if has_organization_members_flag and has_view_members_portfolio_permission %} -
  • - - Members - -
    • + {% if has_organization_members_flag %} + {% if has_view_members_portfolio_permission or has_edit_members_portfolio_permission %} +
  • + + Members + +
    • + {% endif %} {% endif %}
  • diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 55435d3b17..3f089ebe10 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -7,14 +7,6 @@ {% block portfolio_content %}
    -
    -

    - Portfolio name: {{ portfolio }} -

    -
    -
    {% block messages %} @@ -23,7 +15,22 @@

    Member

    -

    The name of your organization will be publicly listed as the domain registrant.

    +

    {{ user.first_name }}

    + + +
    + + + {% csrf_token %} + + {% input_with_errors form.roles %} + {% input_with_errors form.additional_permissions %} + + + diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 34cc08ee74..f3a1759803 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -8,6 +8,7 @@ from registrar.models.user import User from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices +from operator import itemgetter @login_required @@ -15,31 +16,60 @@ def get_portfolio_members_json(request): """Given the current request, get all members that are associated with the given portfolio""" portfolio = request.GET.get("portfolio") - member_ids = get_member_ids_from_request(request, portfolio) - objects = User.objects.filter(id__in=member_ids) - - admin_ids = UserPortfolioPermission.objects.filter( - portfolio=portfolio, - roles__overlap=[ - UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ], - ).values_list("user__id", flat=True) - portfolio_invitation_emails = PortfolioInvitation.objects.filter(portfolio=portfolio).values_list( - "email", flat=True + # member_ids = get_member_ids_from_request(request, portfolio) + # members = User.objects.filter(id__in=member_ids) + + permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio).select_related("user").values_list("pk", "user__first_name", "user__last_name", "user__email", "user__last_login", "roles") + invitations = PortfolioInvitation.objects.filter(portfolio=portfolio).values_list( + 'pk', 'email', 'portfolio_roles', 'portfolio_additional_permissions', 'status' ) - unfiltered_total = objects.count() + # Convert the permissions queryset into a list of dictionaries + permission_list = [ + { + 'id': perm[0], + 'first_name': perm[1], + 'last_name': perm[2], + 'email': perm[3], + 'last_active': perm[4], + 'roles': perm[5], + 'source': 'permission' # Mark the source as permissions + } + for perm in permissions + ] + + # Convert the invitations queryset into a list of dictionaries + invitation_list = [ + { + 'id': invite[0], + 'first_name': None, # No first name in invitations + 'last_name': None, # No last name in invitations + 'email': invite[1], + 'roles': invite[2], + 'additional_permissions': invite[3], + 'status': invite[4], + 'last_active': 'Invited', + 'source': 'invitation' # Mark the source as invitations + } + for invite in invitations + ] + + # Combine both lists into one unified list + combined_list = permission_list + invitation_list + + unfiltered_total = len(combined_list) - objects = apply_search(objects, request) - # objects = apply_status_filter(objects, request) - objects = apply_sorting(objects, request) + combined_list = apply_search(combined_list, request) + combined_list = apply_sorting(combined_list, request) - paginator = Paginator(objects, 10) + paginator = Paginator(combined_list, 10) page_number = request.GET.get("page", 1) page_obj = paginator.get_page(page_number) + + members = [ - serialize_members(request, portfolio, member, request.user, admin_ids, portfolio_invitation_emails) - for member in page_obj.object_list + serialize_members(request, portfolio, item, request.user) + for item in page_obj.object_list ] return JsonResponse( @@ -55,44 +85,43 @@ def get_portfolio_members_json(request): ) -def get_member_ids_from_request(request, portfolio): - """Given the current request, - get all members that are associated with the given portfolio""" - member_ids = [] - if portfolio: - member_ids = UserPortfolioPermission.objects.filter(portfolio=portfolio).values_list("user__id", flat=True) - return member_ids - +# def get_member_ids_from_request(request, portfolio): +# """Given the current request, +# get all members that are associated with the given portfolio""" +# member_ids = [] +# if portfolio: +# member_ids = UserPortfolioPermission.objects.filter(portfolio=portfolio).values_list("user__id", flat=True) +# return member_ids -def apply_search(queryset, request): - search_term = request.GET.get("search_term") +def apply_search(data_list, request): + search_term = request.GET.get("search_term", "").lower() if search_term: - queryset = queryset.filter( - Q(username__icontains=search_term) - | Q(first_name__icontains=search_term) - | Q(last_name__icontains=search_term) - | Q(email__icontains=search_term) - ) - return queryset - - -def apply_sorting(queryset, request): + # Filter the list based on the search term (case-insensitive) + data_list = [ + item for item in data_list + if search_term in (item.get('first_name', '') or '').lower() + or search_term in (item.get('last_name', '') or '').lower() + or search_term in (item.get('email', '') or '').lower() + ] + + return data_list + + +def apply_sorting(data_list, request): sort_by = request.GET.get("sort_by", "id") # Default to 'id' order = request.GET.get("order", "asc") # Default to 'asc' if sort_by == "member": - sort_by = ["email", "first_name", "middle_name", "last_name"] - else: - sort_by = [sort_by] + sort_by = "email" - if order == "desc": - sort_by = [f"-{field}" for field in sort_by] + # Sort the list + data_list = sorted(data_list, key=itemgetter(sort_by), reverse=(order == "desc")) - return queryset.order_by(*sort_by) + return data_list -def serialize_members(request, portfolio, member, user, admin_ids, portfolio_invitation_emails): +def serialize_members(request, portfolio, item, user): # ------- VIEW ONLY # If not view_only (the user has permissions to edit/manage users), show the gear icon with "Manage" link. # If view_only (the user only has view user permissions), show the "View" link (no gear icon). @@ -106,20 +135,20 @@ def serialize_members(request, portfolio, member, user, admin_ids, portfolio_inv view_only = not user.has_edit_members_portfolio_permission(portfolio) or not user_can_edit_other_users # ------- USER STATUSES - is_invited = member.email in portfolio_invitation_emails - last_active = "Invited" if is_invited else "Unknown" - if member.last_login: - last_active = member.last_login.strftime("%b. %d, %Y") - is_admin = member.id in admin_ids + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item['roles'] + + action_url = '#' + if item['source'] == 'permission': + action_url = reverse("member", kwargs={"pk": item['id']}) # ------- SERIALIZE member_json = { - "id": member.id, - "name": member.get_formatted_name(), - "email": member.email, + "id": item['id'], + "name": (item['first_name'] or '') + ' ' + (item['last_name'] or ''), + "email": item['email'], "is_admin": is_admin, - "last_active": last_active, - "action_url": reverse("member", kwargs={"pk": member.id}), # TODO: Future ticket? + "last_active": item['last_active'], + "action_url": action_url, "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), } diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index fe15ccd272..c4a19e2cde 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -3,7 +3,7 @@ from django.shortcuts import render from django.urls import reverse from django.contrib import messages -from registrar.forms.portfolio import PortfolioOrgAddressForm, PortfolioSeniorOfficialForm +from registrar.forms.portfolio import PortfolioMemberForm, PortfolioOrgAddressForm, PortfolioSeniorOfficialForm from registrar.models import Portfolio, User from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices @@ -17,7 +17,7 @@ ) from django.views.generic import View from django.views.generic.edit import FormMixin - +from django.shortcuts import get_object_or_404, redirect logger = logging.getLogger(__name__) @@ -55,11 +55,33 @@ def get(self, request): class PortfolioMemberView(PortfolioMemberPermissionView, View): template_name = "portfolio_member.html" - model = User - - # def get(self, request): - # """Add additional context data to the template.""" - # return render(request, self.template_name, context=self.get_context_data()) + form_class = PortfolioMemberForm + + def get(self, request, pk): + portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) + user = portfolio_permission.user + + form = self.form_class(instance=portfolio_permission) + + return render(request, self.template_name, { + 'form': form, + 'user': user, + }) + + def post(self, request, pk): + portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) + user = portfolio_permission.user + + form = self.form_class(request.POST, instance=portfolio_permission) + + if form.is_valid(): + form.save() + return redirect('home') + + return render(request, self.template_name, { + 'form': form, + 'user': user, # Pass the user object again to the template + }) From 993ae06b6abc8b76c752aeca1f46b875fd823cea Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 4 Oct 2024 10:56:28 -0400 Subject: [PATCH 025/103] fixed sorting for last_active --- src/registrar/views/portfolio_members_json.py | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index f3a1759803..ed8fb9c3f5 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -1,3 +1,4 @@ +from datetime import datetime from django.http import JsonResponse from django.core.paginator import Paginator from django.contrib.auth.decorators import login_required @@ -115,8 +116,25 @@ def apply_sorting(data_list, request): if sort_by == "member": sort_by = "email" - # Sort the list - data_list = sorted(data_list, key=itemgetter(sort_by), reverse=(order == "desc")) + # Custom key function that handles None, 'Invited', and datetime values for last_active + def sort_key(item): + value = item.get(sort_by) + if sort_by == "last_active": + # Return a tuple to ensure consistent data types for comparison + # First element: ordering value (0 for valid datetime, 1 for 'Invited', 2 for None) + # Second element: the actual value to sort by + if value is None: + return (2, value) # Position None last + if value == 'Invited': + return (1, value) # Position 'Invited' before None but after valid datetimes + if isinstance(value, datetime): + return (0, value) # Position valid datetime values first + + # Default case: return the value as is for comparison + return value + + # Sort the list using the custom key function + data_list = sorted(data_list, key=sort_key, reverse=(order == "desc")) return data_list From c1c060cb363df9f2aec9c7b2164fab0142d5dfe3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 4 Oct 2024 11:40:29 -0400 Subject: [PATCH 026/103] added support for invited member --- src/registrar/config/urls.py | 5 +++ src/registrar/forms/portfolio.py | 28 +++++++++++++++ src/registrar/templates/portfolio_member.html | 15 +++++--- src/registrar/views/portfolio_members_json.py | 10 ++---- src/registrar/views/portfolios.py | 36 ++++++++++++++++--- src/registrar/views/utility/mixins.py | 19 ++++++++++ .../views/utility/permission_views.py | 9 +++++ 7 files changed, 105 insertions(+), 17 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 754edca1cb..da115d4713 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -86,6 +86,11 @@ views.PortfolioMemberView.as_view(), name="member", ), + path( + "invitedmember/", + views.PortfolioInvitedMemberView.as_view(), + name="invitedmember", + ), # path( # "no-organization-members/", # views.PortfolioNoMembersView.as_view(), diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 2b669c50cc..cdf00c6250 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,6 +4,7 @@ from django import forms from django.core.validators import RegexValidator +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices @@ -126,3 +127,30 @@ class Meta: "additional_permissions", ] + +class PortfolioInvitedMemberForm(forms.ModelForm): + """ + Form for updating a portfolio invited member. + """ + + portfolio_roles = forms.MultipleChoiceField( + choices=UserPortfolioRoleChoices.choices, + widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + required=False, + label="Roles", + ) + + portfolio_additional_permissions = forms.MultipleChoiceField( + choices=UserPortfolioPermissionChoices.choices, + widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + required=False, + label="Additional Permissions", + ) + + class Meta: + model = PortfolioInvitation + fields = [ + "portfolio_roles", + "portfolio_additional_permissions", + ] + diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 3f089ebe10..284a795eb1 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -15,17 +15,22 @@

    Member

    -

    {{ user.first_name }}

    +

    {{ member.first_name }}

    {% csrf_token %} - - {% input_with_errors form.roles %} - {% input_with_errors form.additional_permissions %} - + {% if form.roles %} + {% comment - handling form fields for member %} + {% input_with_errors form.roles %} + {% input_with_errors form.additional_permissions %} + {% elif form.portfolio_roles %} + {% comment - handling form fields for invited member %} + {% input_with_errors form.portfolio_roles %} + {% input_with_errors form.portfolio_additional_permissions %} + {% endif %} +
    + + + +
    +
    +{% endblock %} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 98402fb952..d33c327f87 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -13,7 +13,9 @@ PortfolioDomainsPermissionView, PortfolioBasePermissionView, NoPortfolioDomainsPermissionView, + PortfolioInvitedMemberEditPermissionView, PortfolioInvitedMemberPermissionView, + PortfolioMemberEditPermissionView, PortfolioMemberPermissionView, PortfolioMembersPermissionView, ) @@ -57,6 +59,20 @@ def get(self, request): class PortfolioMemberView(PortfolioMemberPermissionView, View): template_name = "portfolio_member.html" + + def get(self, request, pk): + portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) + user = portfolio_permission.user + + return render(request, self.template_name, { + 'portfolio_permission': portfolio_permission, + 'member': user, + }) + + +class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): + + template_name = "portfolio_member_permissions.html" form_class = PortfolioMemberForm def get(self, request, pk): @@ -78,7 +94,7 @@ def post(self, request, pk): if form.is_valid(): form.save() - return redirect('members') + return redirect('member',pk=pk) return render(request, self.template_name, { 'form': form, @@ -86,10 +102,23 @@ def post(self, request, pk): }) - class PortfolioInvitedMemberView(PortfolioInvitedMemberPermissionView, View): template_name = "portfolio_member.html" + # form_class = PortfolioInvitedMemberForm + + def get(self, request, pk): + portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) + # form = self.form_class(instance=portfolio_invitation) + + return render(request, self.template_name, { + 'portfolio_invitation': portfolio_invitation, + }) + + +class PortfolioInvitedMemberEditView(PortfolioInvitedMemberEditPermissionView, View): + + template_name = "portfolio_member_permissions.html" form_class = PortfolioInvitedMemberForm def get(self, request, pk): @@ -106,7 +135,7 @@ def post(self, request, pk): form = self.form_class(request.POST, instance=portfolio_invitation) if form.is_valid(): form.save() - return redirect('members') + return redirect('invitedmember', pk=pk) return render(request, self.template_name, { 'form': form, diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 10e8fe3c60..94fc0b1c56 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -515,6 +515,23 @@ def has_permission(self): return False return super().has_permission() + + +class PortfolioMemberEditPermission(PortfolioBasePermission): + """Permission mixin that allows access to portfolio member pages if user + has access to edit, otherwise 403""" + + def has_permission(self): + """Check if this user has access to members for this portfolio. + + The user is in self.request.user and the portfolio can be looked + up from the portfolio's primary key in self.kwargs["pk"]""" + + portfolio = self.request.session.get("portfolio") + if not self.request.user.has_edit_members_portfolio_permission(portfolio): + return False + + return super().has_permission() class PortfolioInvitedMemberPermission(PortfolioBasePermission): @@ -534,3 +551,20 @@ def has_permission(self): return False return super().has_permission() + + +class PortfolioInvitedMemberEditPermission(PortfolioBasePermission): + """Permission mixin that allows access to portfolio invited member pages if user + has access to edit, otherwise 403""" + + def has_permission(self): + """Check if this user has access to members for this portfolio. + + The user is in self.request.user and the portfolio can be looked + up from the portfolio's primary key in self.kwargs["pk"]""" + + portfolio = self.request.session.get("portfolio") + if not self.request.user.has_edit_members_portfolio_permission(portfolio): + return False + + return super().has_permission() diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 65af99a141..6fb7fee502 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -15,7 +15,9 @@ DomainRequestWizardPermission, PortfolioDomainRequestsPermission, PortfolioDomainsPermission, + PortfolioInvitedMemberEditPermission, PortfolioInvitedMemberPermission, + PortfolioMemberEditPermission, UserDeleteDomainRolePermission, UserProfilePermission, PortfolioBasePermission, @@ -270,9 +272,25 @@ class PortfolioMemberPermissionView(PortfolioMemberPermission, PortfolioBasePerm """ +class PortfolioMemberEditPermissionView(PortfolioMemberEditPermission, PortfolioBasePermissionView, abc.ABC): + """Abstract base view for portfolio member edit views that enforces permissions. + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ + + class PortfolioInvitedMemberPermissionView(PortfolioInvitedMemberPermission, PortfolioBasePermissionView, abc.ABC): """Abstract base view for portfolio member views that enforces permissions. This abstract view cannot be instantiated. Actual views must specify `template_name`. """ + + +class PortfolioInvitedMemberEditPermissionView(PortfolioInvitedMemberEditPermission, PortfolioBasePermissionView, abc.ABC): + """Abstract base view for portfolio member edit views that enforces permissions. + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ From 02839026153c57f7688daf242c4c959a155a2427 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:17:39 -0700 Subject: [PATCH 030/103] Add more copy changes --- .github/pull_request_template.md | 4 ++-- docs/dev-practices/code_review.md | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ecf117f15a..c4e63bccdc 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -65,14 +65,14 @@ Resolves #001 #### Validated user-facing changes as a developer +**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist + - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability - [ ] Meets all designs and user flows provided by design/product - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - [ ] (Rarely needed) Tested as both an analyst and applicant user -**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist - ### As a designer reviewer, I have #### Verified that the changes match the design intention diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index aa3d134048..f30eec64e0 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -6,9 +6,10 @@ After creating a pull request, pull request submitters should: - If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. -## Pull Requests for User-facing changes Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. +All other changes require just a single approving review. +## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. ## Coding standards From 980f997dbcea3cf32ba4ccf68b9880552db3b83d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:29:10 -0700 Subject: [PATCH 031/103] Add PR naming conventions --- .github/pull_request_template.md | 13 +++++-------- docs/dev-practices/code_review.md | 7 ++++++- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c4e63bccdc..20571b3050 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -64,7 +64,6 @@ Resolves #001 - [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` #### Validated user-facing changes as a developer - **Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing @@ -86,13 +85,11 @@ Resolves #001 - [ ] Checked different states (empty, one, some, error) - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - -#### Test support on multiple browsers. Check the browser(s) tested. -- [ ] Chrome -- [ ] Microsoft Edge -- [ ] FireFox -- [ ] Safari - +- [ ] Tested with multiple browsers (check off which ones were used) + - [ ] Chrome + - [ ] Microsoft Edge + - [ ] FireFox + - [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user ## Screenshots diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index f30eec64e0..09e6e0c1cc 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,5 +1,8 @@ ## Code Review +Pull requests should be titled in the format of `#issue_number: Descriptive name ideally matching ticket name - [sandbox]` +Any pull requests including a migration should be suffixed with ` - MIGRATION` + After creating a pull request, pull request submitters should: - Add at least 2 developers as PR reviewers (only 1 will need to approve). - Message on Slack or in standup to notify the team that a PR is ready for review. @@ -7,11 +10,13 @@ After creating a pull request, pull request submitters should: - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. -All other changes require just a single approving review. +All other changes require a single approving review. ## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. +Add new pages to the .pa11yci file so they are included in our automated accessibility testing. + ## Coding standards (The Coding standards section may be moved to a new code standards file in a future ticket. For now we're simply moving PR template content into the code review document for consolidation) From c20f7e66791906b88a30e19130efe44642108400 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:49:27 -0700 Subject: [PATCH 032/103] Updating branch naming standards in contributing.md --- CONTRIBUTING.md | 21 +-------------------- docs/dev-practices/code_review.md | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ab15c660fb..5e1c01be9b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,17 +14,6 @@ There are a handful of things we do not commit to the repository: For developers, you can auto-deploy your code to your sandbox (if applicable) by naming your branch thusly: jsd/123-feature-description Where 'jsd' stands for your initials and sandbox environment name (if you were called John Smith Doe), and 123 matches the ticket number if applicable. -## Approvals - -When a code change is made that is not user facing, then the following is required: -- a developer approves the PR - -When a code change is made that is user facing, beyond content updates, then the following are required: -- a developer approves the PR -- a designer approves the PR or checks off all relevant items in this checklist - -Content or document updates require a single person to approve. - ## Project Management We use [Github Projects](https://docs.github.com/en/issues/planning-and-tracking-with-projects/learning-about-projects/about-projects) for project management and tracking. @@ -39,14 +28,6 @@ Every issue in this respository and on the project board should be appropriately We also have labels for each discipline and for research and project management related tasks. While this repository and project board track development work, we try to document all work related to the project here as well. -## Pull request etiquette - -- The submitter is in charge of merging their PRs unless the approver is given explicit permission. -- Do not commit to another person's branch unless given explicit permission. -- Keep pull requests as small as possible. This makes them easier to review and track changes. -- Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. -- Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. - ## Branch Naming -Our branch naming convention is `name/topic-or-feature`, for example: `lmm/add-contributing-doc`. +Our branch naming convention is `name/issue_no-description`, for example: `lmm/0000-add-contributing-doc`. diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 09e6e0c1cc..1cea4aa048 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,7 +1,7 @@ ## Code Review Pull requests should be titled in the format of `#issue_number: Descriptive name ideally matching ticket name - [sandbox]` -Any pull requests including a migration should be suffixed with ` - MIGRATION` +Pull requests including a migration should be suffixed with ` - MIGRATION` After creating a pull request, pull request submitters should: - Add at least 2 developers as PR reviewers (only 1 will need to approve). @@ -9,19 +9,27 @@ After creating a pull request, pull request submitters should: - If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. +## Pull request approvals Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. All other changes require a single approving review. +The submitter is responsible for merging their PR unless the approver is given explcit permission. Similarly, do not commit to another person's branch unless given explicit permission. + +Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. + ## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. Add new pages to the .pa11yci file so they are included in our automated accessibility testing. +## Other Pull request norms +- Keep pull requests as small as possible. This makes them easier to review and track changes. +- Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. + +[comment]: The Coding standards section will be moved to a new code standards file in #2898. For now we're simply moving PR template content into the code review document for consolidation ## Coding standards -(The Coding standards section may be moved to a new code standards file in a future ticket. -For now we're simply moving PR template content into the code review document for consolidation) ### Plain language All functions and methods should use plain language. -TODO: Plain language description and examples in code standards ticket. \ No newline at end of file +TODO: Plain language description and examples in code standards ticket. From a4a83986388a9f15f3690b7eeee6d0052a21faa9 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Fri, 4 Oct 2024 13:32:47 -0600 Subject: [PATCH 033/103] Design updated implemented --- src/registrar/templates/domain_detail.html | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 5cb559a5ae..0fb29d2eb0 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -46,7 +46,6 @@

    {{ domain.name }}

    You don't have access to manage {{domain.name}}. If you need to make updates, contact one of the listed domain managers. - Alternatively, an admin for your organization can assign this domain to you by updating your member permissions.

    From 9a28c8c404fdc83fef958b5e09875387f237ee76 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 4 Oct 2024 16:21:59 -0400 Subject: [PATCH 034/103] wip member edit ui --- .../includes/member_permissions.html | 44 +++++++ .../templates/includes/summary_item.html | 8 +- src/registrar/templates/portfolio_member.html | 121 +++++++++--------- src/registrar/views/portfolio_members_json.py | 2 - src/registrar/views/portfolios.py | 4 +- 5 files changed, 113 insertions(+), 66 deletions(-) create mode 100644 src/registrar/templates/includes/member_permissions.html diff --git a/src/registrar/templates/includes/member_permissions.html b/src/registrar/templates/includes/member_permissions.html new file mode 100644 index 0000000000..d2e8d5392b --- /dev/null +++ b/src/registrar/templates/includes/member_permissions.html @@ -0,0 +1,44 @@ +

    Member access

    +{% if permissions.roles and 'organization_admin' in permissions.roles %} +

    Admin access

    +{% elif permissions.portfolio_roles and 'organization_admin' in permissions.portfolio_roles %} +

    Admin access

    + +{% elif permissions.roles and 'organization_member' in permissions.roles %} +

    Basic access

    +{% elif permissions.portfolio_roles and 'organization_member' in permissions.portfolio_roles %} +

    Basic access

    + +{% else %} +

    ⎯

    +{% endif %} + +

    Organization domain requests

    +{% if permissions.roles and 'organization_admin' in permissions.roles or 'edit_requests' in permissions.additional_permissions %} +

    View all requests plus create requests

    +{% elif permissions.portfolio_roles and 'organization_admin' in permissions.portfolio_roles or 'edit_requests' in permissions.portfolio_additional_permissions %} +

    View all requests plus create requests

    + +{% elif permissions.additional_permissions and 'view_all_requests' in permissions.additional_permissions %} +

    View all requests

    +{% elif permissions.portfolio_additional_permissions and 'view_all_requests' in permissions.portfolio_additional_permissions %} +

    View all requests

    + +{% else %} +

    No access

    +{% endif %} + +

    Organization members

    +{% if permissions.additional_permissions and 'edit_members' in permissions.additional_permissions %} +

    View all members plus manage members

    +{% elif permissions.portfolio_additional_permissions and 'edit_members' in permissions.portfolio_additional_permissions %} +

    View all members plus manage members

    + +{% elif permissions.additional_permissions and 'view_members' in permissions.additional_permissions %} +

    View all members

    +{% elif permissions.portfolio_additional_permissions and 'view_members' in permissions.portfolio_additional_permissions %} +

    View all members

    + +{% else %} +

    No access

    +{% endif %} \ No newline at end of file diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index d4c68395f9..fbe392c4d4 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -24,7 +24,9 @@ {% if sub_header_text %}

    {{ sub_header_text }}

    {% endif %} - {% if address %} + {% if permissions %} + {% include "includes/member_permissions.html" with permissions=value %} + {% elif address %} {% include "includes/organization_address.html" with organization=value %} {% elif contact %} {% if list %} @@ -122,9 +124,9 @@

    {{ sub_header_text }}

    class="usa-link usa-link--icon font-sans-sm line-height-sans-5" > - Edit {{ title }} + {% if manage_button %}Manage{% elif view_button %}View{% else %}Edit{% endif %} {{ title }} {% endif %} diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index c68f561d2c..718970818a 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -6,74 +6,75 @@ {% load static %} {% block portfolio_content %} -
    -
    +
    - {% block messages %} - {% include "includes/form_messages.html" %} - {% endblock %} + {% url 'members' as url %} + + -

    Manage member

    - -

    - {% if member %} - {{ member.email }} - {% elif portfolio_invitation %} - {{ portfolio_invitation.email }} - {% endif %} -

    - -

    Last active: - {% if member and member.last_login %} - {{ member.last_login }} - {% elif portfolio_invitation %} - Invited - {% else %} - -- - {% endif %} -

    +

    Manage member

    -

    Full name: - {% if member %} - {% if member.first_name or member.last_name %} - {{ member.get_formatted_name }} - {% else %} - -- - {% endif %} - {% else %} - -- - {% endif %} -

    +

    + {% if member %} + {{ member.email }} + {% elif portfolio_invitation %} + {{ portfolio_invitation.email }} + {% endif %} +

    -

    Title or organization role: - {% if member and member.title %} - {{ member.title }} - {% else %} - -- - {% endif %} -

    - +
    + Last active: + {% if member and member.last_login %} + {{ member.last_login }} + {% elif portfolio_invitation %} + Invited + {% else %} + -- + {% endif %} +
    + Full name: + {% if member %} + {% if member.first_name or member.last_name %} + {{ member.get_formatted_name }} + {% else %} + -- + {% endif %} + {% else %} + -- + {% endif %} +
    -
    + Title or organization role: + {% if member and member.title %} + {{ member.title }} + {% else %} + -- + {% endif %} +
    -
    - {% csrf_token %} - {% if form.roles %} - {% input_with_errors form.roles %} - {% input_with_errors form.additional_permissions %} - {% elif form.portfolio_roles %} - {% input_with_errors form.portfolio_roles %} - {% input_with_errors form.portfolio_additional_permissions %} - {% endif %} - -
    + {% if portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% elif portfolio_invitation %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% endif %} + {% if has_any_domains_portfolio_permission %} + {% if has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Domain management' value="Asdasd" edit_link='#' editable='true' manage_button='true' %} + {% else %} + {% include "includes/summary_item.html" with title='Domain management' value="Asdasd" edit_link='#' editable='true' view_button='true' %} + {% endif %} + {% endif %} - -
    {% endblock %} diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 56904988ea..ec00046545 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -17,8 +17,6 @@ def get_portfolio_members_json(request): """Given the current request, get all members that are associated with the given portfolio""" portfolio = request.GET.get("portfolio") - # member_ids = get_member_ids_from_request(request, portfolio) - # members = User.objects.filter(id__in=member_ids) permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio).select_related("user").values_list("pk", "user__first_name", "user__last_name", "user__email", "user__last_login", "roles") invitations = PortfolioInvitation.objects.filter(portfolio=portfolio).values_list( diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d33c327f87..2b231a0b9e 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -65,6 +65,7 @@ def get(self, request, pk): user = portfolio_permission.user return render(request, self.template_name, { + 'edit_url': reverse('member-permissions', args=[pk]), 'portfolio_permission': portfolio_permission, 'member': user, }) @@ -94,7 +95,7 @@ def post(self, request, pk): if form.is_valid(): form.save() - return redirect('member',pk=pk) + return redirect('member', pk=pk) return render(request, self.template_name, { 'form': form, @@ -112,6 +113,7 @@ def get(self, request, pk): # form = self.form_class(instance=portfolio_invitation) return render(request, self.template_name, { + 'edit_url': reverse('invitedmember-permissions', args=[pk]), 'portfolio_invitation': portfolio_invitation, }) From 57c89497b29e1779c23ec645b789866c126b0e2e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 7 Oct 2024 12:52:42 -0400 Subject: [PATCH 035/103] formatting of last active date --- src/registrar/assets/js/get-gov.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index 8dd0fcf146..6e8ca61fa1 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1914,10 +1914,12 @@ class MembersTable extends LoadTableBase { memberList.innerHTML = ''; data.members.forEach(member => { - // const actionUrl = domain.action_url; const member_name = member.name; const member_email = member.email; - const last_active = member.last_active; + const options = { year: 'numeric', month: 'short', day: 'numeric' }; + const last_active = member.last_active ? member.last_active != 'Invited' ? new Date(member.last_active) : 'Invited' : null; + const last_active_formatted = last_active ? last_active != 'Invited' ? last_active.toLocaleDateString('en-Us', options) : 'Invited' : ''; + const last_active_sort_value = last_active ? last_active != 'Invited' ? last_active.getTime() : 'Invited' : ''; const action_url = member.action_url; const action_label = member.action_label; const svg_icon = member.svg_icon; @@ -1932,8 +1934,8 @@ class MembersTable extends LoadTableBase { ${member_email ? member_email : member_name} ${admin_tagHTML} - - ${last_active} + + ${last_active_formatted} From 58002de5e01bcac397d1934859b07aa5440fc75e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 7 Oct 2024 13:18:13 -0400 Subject: [PATCH 036/103] updated model --- src/registrar/admin.py | 12 +++++----- src/registrar/forms/portfolio.py | 8 +++---- ...itation_additional_permissions_and_more.py | 23 ++++++++++++++++++ src/registrar/models/portfolio_invitation.py | 12 +++++----- .../includes/member_permissions.html | 18 -------------- .../portfolio_member_permissions.html | 9 ++----- src/registrar/tests/test_models.py | 12 +++++----- .../tests/test_views_members_json.py | 4 ++-- src/registrar/tests/test_views_portfolio.py | 24 +++++++++---------- src/registrar/views/portfolio_members_json.py | 2 +- 10 files changed, 62 insertions(+), 62 deletions(-) create mode 100644 src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8718da9bad..9201585d71 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -188,11 +188,11 @@ class Meta: model = models.PortfolioInvitation fields = "__all__" widgets = { - "portfolio_roles": FilteredSelectMultipleArrayWidget( - "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices + "roles": FilteredSelectMultipleArrayWidget( + "roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices ), - "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( - "portfolio_additional_permissions", + "additional_permissions": FilteredSelectMultipleArrayWidget( + "additional_permissions", is_stacked=False, choices=UserPortfolioPermissionChoices.choices, ), @@ -1387,8 +1387,8 @@ class Meta: list_display = [ "email", "portfolio", - "portfolio_roles", - "portfolio_additional_permissions", + "roles", + "additional_permissions", "status", ] diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index cdf00c6250..51f53340c5 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -133,14 +133,14 @@ class PortfolioInvitedMemberForm(forms.ModelForm): Form for updating a portfolio invited member. """ - portfolio_roles = forms.MultipleChoiceField( + roles = forms.MultipleChoiceField( choices=UserPortfolioRoleChoices.choices, widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), required=False, label="Roles", ) - portfolio_additional_permissions = forms.MultipleChoiceField( + additional_permissions = forms.MultipleChoiceField( choices=UserPortfolioPermissionChoices.choices, widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), required=False, @@ -150,7 +150,7 @@ class PortfolioInvitedMemberForm(forms.ModelForm): class Meta: model = PortfolioInvitation fields = [ - "portfolio_roles", - "portfolio_additional_permissions", + "roles", + "additional_permissions", ] diff --git a/src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py b/src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py new file mode 100644 index 0000000000..338a204933 --- /dev/null +++ b/src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.10 on 2024-10-07 17:12 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0129_alter_portfolioinvitation_portfolio_roles_and_more"), + ] + + operations = [ + migrations.RenameField( + model_name="portfolioinvitation", + old_name="portfolio_additional_permissions", + new_name="additional_permissions", + ), + migrations.RenameField( + model_name="portfolioinvitation", + old_name="portfolio_roles", + new_name="roles", + ), + ] diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 46d7bf124a..392d8264b4 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -38,7 +38,7 @@ class PortfolioInvitationStatus(models.TextChoices): related_name="portfolios", ) - portfolio_roles = ArrayField( + roles = ArrayField( models.CharField( max_length=50, choices=UserPortfolioRoleChoices.choices, @@ -48,7 +48,7 @@ class PortfolioInvitationStatus(models.TextChoices): help_text="Select one or more roles.", ) - portfolio_additional_permissions = ArrayField( + additional_permissions = ArrayField( models.CharField( max_length=50, choices=UserPortfolioPermissionChoices.choices, @@ -88,8 +88,8 @@ def retrieve(self): user_portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( portfolio=self.portfolio, user=user ) - if self.portfolio_roles and len(self.portfolio_roles) > 0: - user_portfolio_permission.roles = self.portfolio_roles - if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: - user_portfolio_permission.additional_permissions = self.portfolio_additional_permissions + if self.roles and len(self.roles) > 0: + user_portfolio_permission.roles = self.roles + if self.additional_permissions and len(self.additional_permissions) > 0: + user_portfolio_permission.additional_permissions = self.additional_permissions user_portfolio_permission.save() diff --git a/src/registrar/templates/includes/member_permissions.html b/src/registrar/templates/includes/member_permissions.html index d2e8d5392b..0d38e20734 100644 --- a/src/registrar/templates/includes/member_permissions.html +++ b/src/registrar/templates/includes/member_permissions.html @@ -1,14 +1,8 @@

    Member access

    {% if permissions.roles and 'organization_admin' in permissions.roles %}

    Admin access

    -{% elif permissions.portfolio_roles and 'organization_admin' in permissions.portfolio_roles %} -

    Admin access

    - {% elif permissions.roles and 'organization_member' in permissions.roles %}

    Basic access

    -{% elif permissions.portfolio_roles and 'organization_member' in permissions.portfolio_roles %} -

    Basic access

    - {% else %}

    ⎯

    {% endif %} @@ -16,14 +10,8 @@

    Member access

    Organization domain requests

    {% if permissions.roles and 'organization_admin' in permissions.roles or 'edit_requests' in permissions.additional_permissions %}

    View all requests plus create requests

    -{% elif permissions.portfolio_roles and 'organization_admin' in permissions.portfolio_roles or 'edit_requests' in permissions.portfolio_additional_permissions %} -

    View all requests plus create requests

    - {% elif permissions.additional_permissions and 'view_all_requests' in permissions.additional_permissions %}

    View all requests

    -{% elif permissions.portfolio_additional_permissions and 'view_all_requests' in permissions.portfolio_additional_permissions %} -

    View all requests

    - {% else %}

    No access

    {% endif %} @@ -31,14 +19,8 @@

    Organization domain requests

    Organization members

    {% if permissions.additional_permissions and 'edit_members' in permissions.additional_permissions %}

    View all members plus manage members

    -{% elif permissions.portfolio_additional_permissions and 'edit_members' in permissions.portfolio_additional_permissions %} -

    View all members plus manage members

    - {% elif permissions.additional_permissions and 'view_members' in permissions.additional_permissions %}

    View all members

    -{% elif permissions.portfolio_additional_permissions and 'view_members' in permissions.portfolio_additional_permissions %} -

    View all members

    - {% else %}

    No access

    {% endif %} \ No newline at end of file diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index ca7b30b443..02d120360d 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -27,13 +27,8 @@

    Manage member

    {% csrf_token %} - {% if form.roles %} - {% input_with_errors form.roles %} - {% input_with_errors form.additional_permissions %} - {% elif form.portfolio_roles %} - {% input_with_errors form.portfolio_roles %} - {% input_with_errors form.portfolio_additional_permissions %} - {% endif %} + {% input_with_errors form.roles %} + {% input_with_errors form.additional_permissions %} +
    + + {% endif %} - +
    Last active: @@ -38,7 +84,7 @@

    {% elif portfolio_invitation %} Invited {% else %} - -- + ⎯ {% endif %}
    @@ -47,10 +93,10 @@

    {% if member.first_name or member.last_name %} {{ member.get_formatted_name }} {% else %} - -- + ⎯ {% endif %} {% else %} - -- + ⎯ {% endif %}
    @@ -58,7 +104,7 @@

    {% if member and member.title %} {{ member.title }} {% else %} - -- + ⎯ {% endif %}

    From 15db31b3dcea93f0f5cb22426054cacddbf26c1c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 7 Oct 2024 15:16:40 -0400 Subject: [PATCH 041/103] assigned domains --- src/registrar/models/user_portfolio_permission.py | 12 +++++++++++- .../templates/includes/member_domain_mgmt.html | 6 ++++++ src/registrar/templates/includes/summary_item.html | 2 ++ src/registrar/templates/portfolio_member.html | 10 ++++------ 4 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 src/registrar/templates/includes/member_domain_mgmt.html diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 6acd651dbc..847df78579 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -1,11 +1,11 @@ from django.db import models from django.forms import ValidationError +from registrar.models.user_domain_role import UserDomainRole from registrar.utility.waffle import flag_is_active_for_user from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel from django.contrib.postgres.fields import ArrayField - class UserPortfolioPermission(TimeStampedModel): """This is a linking table that connects a user with a role on a portfolio.""" @@ -67,6 +67,16 @@ class Meta: def __str__(self): return f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" if self.roles else "" + def get_managed_domains_count(self): + """Return the count of domains managed by the user for this portfolio.""" + # Filter the UserDomainRole model to get domains where the user has a manager role + managed_domains = UserDomainRole.objects.filter( + user=self.user, + role=UserDomainRole.Roles.MANAGER, + domain__domain_info__portfolio=self.portfolio + ).count() + return managed_domains + def _get_portfolio_permissions(self): """ Retrieve the permissions for the user's portfolio roles. diff --git a/src/registrar/templates/includes/member_domain_mgmt.html b/src/registrar/templates/includes/member_domain_mgmt.html new file mode 100644 index 0000000000..34f8bf5e6f --- /dev/null +++ b/src/registrar/templates/includes/member_domain_mgmt.html @@ -0,0 +1,6 @@ +

    Assigned domains

    +{% if domain_count > 0 %} +

    {{domain_count}}

    +{% else %} +

    This member does not manage any domains.{% if editable %} To assign this member a domain, click "Manage".{% endif %}

    +{% endif %} diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index fbe392c4d4..aae7b8cf97 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -26,6 +26,8 @@

    {{ sub_header_text }}

    {% endif %} {% if permissions %} {% include "includes/member_permissions.html" with permissions=value %} + {% elif domain_mgmt %} + {% include "includes/member_domain_mgmt.html" with domain_count=value %} {% elif address %} {% include "includes/organization_address.html" with organization=value %} {% elif contact %} diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 718970818a..4cb846b5a7 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -68,12 +68,10 @@

    {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} - {% if has_any_domains_portfolio_permission %} - {% if has_edit_members_portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain management' value="Asdasd" edit_link='#' editable='true' manage_button='true' %} - {% else %} - {% include "includes/summary_item.html" with title='Domain management' value="Asdasd" edit_link='#' editable='true' view_button='true' %} - {% endif %} + {% if portfolio_permission %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} + {% else %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=0 edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} {% endif %} From 91355dc7a458ea94bb2d1263833a2e82d84aeaf0 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 7 Oct 2024 16:27:11 -0400 Subject: [PATCH 042/103] Assigned domains for portfolio invitations --- src/registrar/models/portfolio_invitation.py | 10 ++++++++++ src/registrar/templates/portfolio_member.html | 2 ++ 2 files changed, 12 insertions(+) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 392d8264b4..22c4c881e0 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -4,6 +4,7 @@ from django.contrib.auth import get_user_model from django.db import models from django_fsm import FSMField, transition +from registrar.models.domain_invitation import DomainInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices # type: ignore from .utility.time_stamped_model import TimeStampedModel @@ -67,6 +68,15 @@ class PortfolioInvitationStatus(models.TextChoices): def __str__(self): return f"Invitation for {self.email} on {self.portfolio} is {self.status}" + def get_managed_domains_count(self): + """Return the count of domain invitations managed by the invited user for this portfolio.""" + # Filter the UserDomainRole model to get domains where the user has a manager role + managed_domains = DomainInvitation.objects.filter( + email=self.email, + domain__domain_info__portfolio=self.portfolio + ).count() + return managed_domains + @transition(field="status", source=PortfolioInvitationStatus.INVITED, target=PortfolioInvitationStatus.RETRIEVED) def retrieve(self): """When an invitation is retrieved, create the corresponding permission. diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 2ce1872a03..fe30399672 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -116,6 +116,8 @@

    More options

    {% if portfolio_permission %} {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} + {% elif portfolio_invitation %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_invitation.get_managed_domains_count edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} {% else %} {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=0 edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} {% endif %} From 42a25b0d683c2bf67c4d829e1451050b135b6eaa Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 7 Oct 2024 18:16:13 -0400 Subject: [PATCH 043/103] move the logic from templates into the views when testing for permissions --- src/registrar/models/portfolio_invitation.py | 17 +++++++++++ .../includes/member_permissions.html | 8 +++--- src/registrar/templates/portfolio_member.html | 4 +-- src/registrar/views/portfolios.py | 28 ++++++++++++++++--- 4 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 22c4c881e0..2c8caaee38 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -76,6 +76,23 @@ def get_managed_domains_count(self): domain__domain_info__portfolio=self.portfolio ).count() return managed_domains + + def get_portfolio_permissions(self): + """ + Retrieve the permissions for the user's portfolio roles from the invite. + This is similar logic to _get_portfolio_permissions in user_portfolio_permission + """ + # Use a set to avoid duplicate permissions + portfolio_permissions = set() + + if self.roles: + for role in self.roles: + portfolio_permissions.update(UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) + + if self.additional_permissions: + portfolio_permissions.update(self.additional_permissions) + + return list(portfolio_permissions) @transition(field="status", source=PortfolioInvitationStatus.INVITED, target=PortfolioInvitationStatus.RETRIEVED) def retrieve(self): diff --git a/src/registrar/templates/includes/member_permissions.html b/src/registrar/templates/includes/member_permissions.html index 0d38e20734..8cf75cfbfd 100644 --- a/src/registrar/templates/includes/member_permissions.html +++ b/src/registrar/templates/includes/member_permissions.html @@ -8,18 +8,18 @@

    Member access

    {% endif %}

    Organization domain requests

    -{% if permissions.roles and 'organization_admin' in permissions.roles or 'edit_requests' in permissions.additional_permissions %} +{% if member_has_edit_request_portfolio_permission %}

    View all requests plus create requests

    -{% elif permissions.additional_permissions and 'view_all_requests' in permissions.additional_permissions %} +{% elif member_has_view_all_requests_portfolio_permission %}

    View all requests

    {% else %}

    No access

    {% endif %}

    Organization members

    -{% if permissions.additional_permissions and 'edit_members' in permissions.additional_permissions %} +{% if member_has_edit_members_portfolio_permission %}

    View all members plus manage members

    -{% elif permissions.additional_permissions and 'view_members' in permissions.additional_permissions %} +{% elif member_has_view_members_portfolio_permission %}

    View all members

    {% else %}

    No access

    diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index fe30399672..727b144978 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -109,9 +109,9 @@

    More options

    {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_permission member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} {% if portfolio_permission %} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 2b231a0b9e..cd79236685 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -7,7 +7,7 @@ from registrar.models import Portfolio, User from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission -from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, PortfolioDomainsPermissionView, @@ -62,12 +62,22 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): def get(self, request, pk): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) - user = portfolio_permission.user - + member = portfolio_permission.user + + # We have to explicitely name these with member_ otherwise we'll have conflicts with context preprocessors + member_has_view_all_requests_portfolio_permission = member.has_view_all_requests_portfolio_permission(portfolio_permission.portfolio) + member_has_edit_request_portfolio_permission = member.has_edit_request_portfolio_permission(portfolio_permission.portfolio) + member_has_view_members_portfolio_permission = member.has_view_members_portfolio_permission(portfolio_permission.portfolio) + member_has_edit_members_portfolio_permission = member.has_edit_members_portfolio_permission(portfolio_permission.portfolio) + return render(request, self.template_name, { 'edit_url': reverse('member-permissions', args=[pk]), 'portfolio_permission': portfolio_permission, - 'member': user, + 'member': member, + 'member_has_view_all_requests_portfolio_permission': member_has_view_all_requests_portfolio_permission, + 'member_has_edit_request_portfolio_permission': member_has_edit_request_portfolio_permission, + 'member_has_view_members_portfolio_permission': member_has_view_members_portfolio_permission, + 'member_has_edit_members_portfolio_permission': member_has_edit_members_portfolio_permission }) @@ -112,9 +122,19 @@ def get(self, request, pk): portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) # form = self.form_class(instance=portfolio_invitation) + # We have to explicitely name these with member_ otherwise we'll have conflicts with context preprocessors + member_has_view_all_requests_portfolio_permission = UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS in portfolio_invitation.get_portfolio_permissions() + member_has_edit_request_portfolio_permission = UserPortfolioPermissionChoices.EDIT_REQUESTS in portfolio_invitation.get_portfolio_permissions() + member_has_view_members_portfolio_permission = UserPortfolioPermissionChoices.VIEW_MEMBERS in portfolio_invitation.get_portfolio_permissions() + member_has_edit_members_portfolio_permission = UserPortfolioPermissionChoices.EDIT_MEMBERS in portfolio_invitation.get_portfolio_permissions() + return render(request, self.template_name, { 'edit_url': reverse('invitedmember-permissions', args=[pk]), 'portfolio_invitation': portfolio_invitation, + 'member_has_view_all_requests_portfolio_permission': member_has_view_all_requests_portfolio_permission, + 'member_has_edit_request_portfolio_permission': member_has_edit_request_portfolio_permission, + 'member_has_view_members_portfolio_permission': member_has_view_members_portfolio_permission, + 'member_has_edit_members_portfolio_permission': member_has_edit_members_portfolio_permission }) From d9a79cabad6959b3ac6920492df438d72f4c2141 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 12:52:14 -0400 Subject: [PATCH 044/103] test models --- src/registrar/tests/test_models.py | 69 ++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index b5fb381aca..905c0a06b3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1182,6 +1182,9 @@ def setUp(self): def tearDown(self): super().tearDown() + DomainInvitation.objects.all().delete() + DomainInformation.objects.all().delete() + Domain.objects.all().delete() UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() PortfolioInvitation.objects.all().delete() @@ -1269,6 +1272,44 @@ def test_retrieve_user_multiple_invitations_when_multiple_portfolios_inactive(se updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + @less_console_noise_decorator + def test_get_managed_domains_count(self): + """Test that the correct number of domains, which are associated with the portfolio and + have invited the email of the portfolio invitation, are returned.""" + # Add three domains, one which is in the portfolio and email is invited to, + # one which is in the portfolio and email is not invited to, + # and one which is email is invited to and not in the portfolio. + # Arrange + # domain_in_portfolio should not be included in the count + domain_in_portfolio, _ = Domain.objects.get_or_create(name="domain_in_portfolio.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio, portfolio=self.portfolio) + # domain_in_portfolio_and_invited should be included in the count + domain_in_portfolio_and_invited, _ = Domain.objects.get_or_create(name="domain_in_portfolio_and_invited.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio_and_invited, portfolio=self.portfolio) + DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_and_invited) + # domain_invited should not be included in the count + domain_invited, _ = Domain.objects.get_or_create(name="domain_invited.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_invited) + DomainInvitation.objects.get_or_create(email=self.email, domain=domain_invited) + + # Assert + self.assertEqual(self.invitation.get_managed_domains_count(), 1) + + @less_console_noise_decorator + def test_get_portfolio_permissions(self): + """Test that get_portfolio_permissions returns the expected list of permissions, + based on the roles and permissions assigned to the invitation.""" + # Arrange + test_permission_list = set() + # add the arrays that are defined in UserPortfolioPermission for member and admin + test_permission_list.update(UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, [])) + test_permission_list.update(UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_ADMIN, [])) + # add the permissions that are added to the invitation as additional_permissions + test_permission_list.update([self.portfolio_permission_1, self.portfolio_permission_2]) + perm_list = list(test_permission_list) + # Verify + self.assertEquals(self.invitation.get_portfolio_permissions(), perm_list) + class TestUserPortfolioPermission(TestCase): @less_console_noise_decorator @@ -1335,6 +1376,34 @@ def test_clean_on_creates_multiple_portfolios(self): "Only one portfolio permission is allowed per user when multiple portfolios are disabled.", ) + @less_console_noise_decorator + def test_get_managed_domains_count(self): + """Test that the correct number of managed domains associated with the portfolio + are returned.""" + # Add three domains, one which is in the portfolio and managed by the user, + # one which is in the portfolio and not managed by the user, + # and one which is managed by the user and not in the portfolio. + # Arrange + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + test_user = create_test_user() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=test_user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + # domain_in_portfolio should not be included in the count + domain_in_portfolio, _ = Domain.objects.get_or_create(name="domain_in_portfolio.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio, portfolio=portfolio) + # domain_in_portfolio_and_managed should be included in the count + domain_in_portfolio_and_managed, _ = Domain.objects.get_or_create(name="domain_in_portfolio_and_managed.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio_and_managed, portfolio=portfolio) + UserDomainRole.objects.get_or_create(user=test_user, domain=domain_in_portfolio_and_managed, role=UserDomainRole.Roles.MANAGER) + # domain_managed should not be included in the count + domain_managed, _ = Domain.objects.get_or_create(name="domain_managed.gov", state=Domain.State.READY) + DomainInformation.objects.get_or_create(creator=self.user, domain=domain_managed) + UserDomainRole.objects.get_or_create(user=test_user, domain=domain_managed, role=UserDomainRole.Roles.MANAGER) + + # Assert + self.assertEqual(portfolio_permission.get_managed_domains_count(), 1) + class TestUser(TestCase): """Test actions that occur on user login, From dc76782a4327b3c86898dc47da40c43ccbebfbf9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 8 Oct 2024 13:36:05 -0400 Subject: [PATCH 045/103] remove checks on members nav link --- .../templates/includes/header_extended.html | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 43467602e8..79c6aacb73 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -92,13 +92,11 @@ {% endif %} {% if has_organization_members_flag %} - {% if has_view_members_portfolio_permission or has_edit_members_portfolio_permission %} -
  • - - Members - -
    • - {% endif %} +
  • + + Members + +
    • {% endif %}
  • From de9736ec9541cb3139f8f3f173392c729a935c64 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 8 Oct 2024 13:49:30 -0400 Subject: [PATCH 046/103] acc cancel invitation button on mobile view --- src/registrar/templates/portfolio_member.html | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 727b144978..dedeffc804 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -32,13 +32,23 @@

    {% endif %}

    {% if has_edit_members_portfolio_permission %} - - Remove member - + {% if member %} + + Remove member + + {% else %} + + Cancel invitation + + {% endif %}
    From 09944e4ce00155104f0214ba14f122bad3c83e47 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:10:51 -0700 Subject: [PATCH 047/103] Fix form valid logic --- .../fixtures_user_portfolio_permissions.py | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py index 3c64eb6b59..6b6e137cd4 100644 --- a/src/registrar/fixtures/fixtures_user_portfolio_permissions.py +++ b/src/registrar/fixtures/fixtures_user_portfolio_permissions.py @@ -1,4 +1,5 @@ import logging +import random from faker import Faker from django.db import transaction @@ -51,22 +52,23 @@ def load(cls): user_portfolio_permissions_to_create = [] for user in users: - for portfolio in portfolios: - try: - if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists(): - user_portfolio_permission = UserPortfolioPermission( - user=user, - portfolio=portfolio, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - ) - user_portfolio_permissions_to_create.append(user_portfolio_permission) - else: - logger.info( - f"Permission exists for user '{user.username}' " - f"on portfolio '{portfolio.organization_name}'." - ) - except Exception as e: - logger.warning(e) + # Assign a random portfolio to a user + portfolio = random.choice(portfolios) # nosec + try: + if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists(): + user_portfolio_permission = UserPortfolioPermission( + user=user, + portfolio=portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + user_portfolio_permissions_to_create.append(user_portfolio_permission) + else: + logger.info( + f"Permission exists for user '{user.username}' " + f"on portfolio '{portfolio.organization_name}'." + ) + except Exception as e: + logger.warning(e) # Bulk create permissions cls._bulk_create_permissions(user_portfolio_permissions_to_create) From 69a98a2f7db4617efcced27e8b1d23ae557dd4ef Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 14:44:20 -0400 Subject: [PATCH 048/103] fixed tests and view on domains --- .../includes/member_domain_mgmt.html | 2 +- src/registrar/templates/portfolio_member.html | 6 +- src/registrar/tests/test_views_portfolio.py | 221 ++++++++++++++++++ 3 files changed, 225 insertions(+), 4 deletions(-) diff --git a/src/registrar/templates/includes/member_domain_mgmt.html b/src/registrar/templates/includes/member_domain_mgmt.html index 34f8bf5e6f..6bf3f13207 100644 --- a/src/registrar/templates/includes/member_domain_mgmt.html +++ b/src/registrar/templates/includes/member_domain_mgmt.html @@ -2,5 +2,5 @@

    Assigned domains

    {% if domain_count > 0 %}

    {{domain_count}}

    {% else %} -

    This member does not manage any domains.{% if editable %} To assign this member a domain, click "Manage".{% endif %}

    +

    This member does not manage any domains.{% if manage_button %} To assign this member a domain, click "Manage".{% endif %}

    {% endif %} diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 727b144978..4e104e618b 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -115,11 +115,11 @@

    More options

    {% endif %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_invitation.get_managed_domains_count edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_invitation.get_managed_domains_count edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} {% else %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=0 edit_link='#' editable=has_edit_members_portfolio_permission manage_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=0 edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} {% endif %}
    diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 13a189a8e8..f2dc784f71 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -10,6 +10,7 @@ UserDomainRole, User, ) +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_group import UserGroup from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices @@ -855,6 +856,226 @@ def test_members_admin_detection(self): # TerminalHelper.colorful_logger(logger.info, TerminalColors.OKCYAN, f"{response.content}") self.assertContains(response, '"is_admin": true') + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_cannot_view_member_page_when_flag_is_off(self): + """Test that user cannot access the member page when waffle flag is off""" + + # Verify that the user cannot access the member page + self.client.force_login(self.user) + response = self.client.get(reverse("member", kwargs={"pk": 1}), follow=True) + # Make sure the page is denied + self.assertEqual(response.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_cannot_view_member_page_when_user_has_no_permission(self): + """Test that user cannot access the member page without proper permission""" + + # give user base permissions + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + ) + + # Verify that the user cannot access the member page + self.client.force_login(self.user) + response = self.client.get(reverse("member", kwargs={"pk": 1}), follow=True) + # Make sure the page is denied + self.assertEqual(response.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_can_view_member_page_when_user_has_view_members(self): + """Test that user can access the member page with view_members permission""" + + # Arrange + # give user permissions to view members + permission_obj, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + ) + + # Verify the page can be accessed + self.client.force_login(self.user) + response = self.client.get(reverse("member", kwargs={"pk": permission_obj.pk}), follow=True) + self.assertEqual(response.status_code, 200) + + # Assert text within the page is correct + self.assertContains(response, "First Last") + self.assertContains(response, self.user.email) + self.assertContains(response, "Basic access") + self.assertContains(response, "No access") + self.assertContains(response, "View all members") + self.assertContains(response, "This member does not manage any domains.") + + # Assert buttons and links within the page are correct + self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present + self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present + self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present + self.assertContains(response, "sprite.svg#visibility") # test that View link is present + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_can_view_member_page_when_user_has_edit_members(self): + """Test that user can access the member page with edit_members permission""" + + # Arrange + # give user permissions to view AND manage members + permission_obj, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.EDIT_MEMBERS, + ], + ) + + # Verify the page can be accessed + self.client.force_login(self.user) + response = self.client.get(reverse("member", kwargs={"pk": permission_obj.pk}), follow=True) + self.assertEqual(response.status_code, 200) + + # Assert text within the page is correct + self.assertContains(response, "First Last") + self.assertContains(response, self.user.email) + self.assertContains(response, "Admin access") + self.assertContains(response, "View all requests plus create requests") + self.assertContains(response, "View all members plus manage members") + self.assertContains(response, "This member does not manage any domains. To assign this member a domain, click \"Manage\"") + + # Assert buttons and links within the page are correct + self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present + self.assertContains(response, "sprite.svg#edit") # test that Edit link is present + self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_cannot_view_invitedmember_page_when_flag_is_off(self): + """Test that user cannot access the invitedmember page when waffle flag is off""" + + # Verify that the user cannot access the member page + self.client.force_login(self.user) + response = self.client.get(reverse("invitedmember", kwargs={"pk": 1}), follow=True) + # Make sure the page is denied + self.assertEqual(response.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_cannot_view_invitedmember_page_when_user_has_no_permission(self): + """Test that user cannot access the invitedmember page without proper permission""" + + # give user base permissions + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + ) + + # Verify that the user cannot access the member page + self.client.force_login(self.user) + response = self.client.get(reverse("invitedmember", kwargs={"pk": 1}), follow=True) + # Make sure the page is denied + self.assertEqual(response.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_can_view_invitedmember_page_when_user_has_view_members(self): + """Test that user can access the invitedmember page with view_members permission""" + + # Arrange + # give user permissions to view members + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + ) + portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create( + email="info@example.com", + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_MEMBERS, + ], + ) + + # Verify the page can be accessed + self.client.force_login(self.user) + response = self.client.get(reverse("invitedmember", kwargs={"pk": portfolio_invitation.pk}), follow=True) + self.assertEqual(response.status_code, 200) + + # Assert text within the page is correct + self.assertContains(response, "Invited") + self.assertContains(response, portfolio_invitation.email) + self.assertContains(response, "Basic access") + self.assertContains(response, "No access") + self.assertContains(response, "View all members") + self.assertContains(response, "This member does not manage any domains.") + + # Assert buttons and links within the page are correct + self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present + self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present + self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present + self.assertContains(response, "sprite.svg#visibility") # test that View link is present + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_can_view_invitedmember_page_when_user_has_edit_members(self): + """Test that user can access the invitedmember page with edit_members permission""" + + # Arrange + # give user permissions to view AND manage members + permission_obj, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.EDIT_MEMBERS, + ], + ) + portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create( + email="info@example.com", + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + additional_permissions=[ + UserPortfolioPermissionChoices.EDIT_MEMBERS, + ], + ) + + # Verify the page can be accessed + self.client.force_login(self.user) + response = self.client.get(reverse("invitedmember", kwargs={"pk": portfolio_invitation.pk}), follow=True) + self.assertEqual(response.status_code, 200) + + # Assert text within the page is correct + self.assertContains(response, "Invited") + self.assertContains(response, portfolio_invitation.email) + self.assertContains(response, "Admin access") + self.assertContains(response, "View all requests plus create requests") + self.assertContains(response, "View all members plus manage members") + self.assertContains(response, "This member does not manage any domains. To assign this member a domain, click \"Manage\"") + + # Assert buttons and links within the page are correct + self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present + self.assertContains(response, "sprite.svg#edit") # test that Edit link is present + self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present + @less_console_noise_decorator @override_flag("organization_feature", active=True) def test_portfolio_domain_requests_page_when_user_has_no_permissions(self): From 08a273c296c85f907022c55dee2a573f7e6f4c61 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 14:48:11 -0400 Subject: [PATCH 049/103] linted --- src/registrar/forms/portfolio.py | 15 +- src/registrar/models/portfolio_invitation.py | 5 +- .../models/user_portfolio_permission.py | 7 +- src/registrar/tests/test_models.py | 28 +++- .../tests/test_views_members_json.py | 9 +- src/registrar/tests/test_views_portfolio.py | 44 ++--- src/registrar/views/portfolio_members_json.py | 88 +++++----- src/registrar/views/portfolios.py | 156 +++++++++++------- src/registrar/views/utility/mixins.py | 4 +- .../views/utility/permission_views.py | 4 +- 10 files changed, 213 insertions(+), 147 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 51f53340c5..b8984023aa 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -99,7 +99,7 @@ def clean(self): cleaned_data = super().clean() cleaned_data.pop("full_name", None) return cleaned_data - + class PortfolioMemberForm(forms.ModelForm): """ @@ -108,18 +108,18 @@ class PortfolioMemberForm(forms.ModelForm): roles = forms.MultipleChoiceField( choices=UserPortfolioRoleChoices.choices, - widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + widget=forms.SelectMultiple(attrs={"class": "usa-select"}), required=False, label="Roles", ) additional_permissions = forms.MultipleChoiceField( choices=UserPortfolioPermissionChoices.choices, - widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + widget=forms.SelectMultiple(attrs={"class": "usa-select"}), required=False, label="Additional Permissions", ) - + class Meta: model = UserPortfolioPermission fields = [ @@ -135,22 +135,21 @@ class PortfolioInvitedMemberForm(forms.ModelForm): roles = forms.MultipleChoiceField( choices=UserPortfolioRoleChoices.choices, - widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + widget=forms.SelectMultiple(attrs={"class": "usa-select"}), required=False, label="Roles", ) additional_permissions = forms.MultipleChoiceField( choices=UserPortfolioPermissionChoices.choices, - widget=forms.SelectMultiple(attrs={'class': 'usa-select'}), + widget=forms.SelectMultiple(attrs={"class": "usa-select"}), required=False, label="Additional Permissions", ) - + class Meta: model = PortfolioInvitation fields = [ "roles", "additional_permissions", ] - diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 2c8caaee38..b1f22ae83d 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -72,11 +72,10 @@ def get_managed_domains_count(self): """Return the count of domain invitations managed by the invited user for this portfolio.""" # Filter the UserDomainRole model to get domains where the user has a manager role managed_domains = DomainInvitation.objects.filter( - email=self.email, - domain__domain_info__portfolio=self.portfolio + email=self.email, domain__domain_info__portfolio=self.portfolio ).count() return managed_domains - + def get_portfolio_permissions(self): """ Retrieve the permissions for the user's portfolio roles from the invite. diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 847df78579..80771ca4b3 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -6,6 +6,7 @@ from .utility.time_stamped_model import TimeStampedModel from django.contrib.postgres.fields import ArrayField + class UserPortfolioPermission(TimeStampedModel): """This is a linking table that connects a user with a role on a portfolio.""" @@ -71,12 +72,10 @@ def get_managed_domains_count(self): """Return the count of domains managed by the user for this portfolio.""" # Filter the UserDomainRole model to get domains where the user has a manager role managed_domains = UserDomainRole.objects.filter( - user=self.user, - role=UserDomainRole.Roles.MANAGER, - domain__domain_info__portfolio=self.portfolio + user=self.user, role=UserDomainRole.Roles.MANAGER, domain__domain_info__portfolio=self.portfolio ).count() return managed_domains - + def _get_portfolio_permissions(self): """ Retrieve the permissions for the user's portfolio roles. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 905c0a06b3..f565c001e9 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1284,8 +1284,12 @@ def test_get_managed_domains_count(self): domain_in_portfolio, _ = Domain.objects.get_or_create(name="domain_in_portfolio.gov", state=Domain.State.READY) DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio, portfolio=self.portfolio) # domain_in_portfolio_and_invited should be included in the count - domain_in_portfolio_and_invited, _ = Domain.objects.get_or_create(name="domain_in_portfolio_and_invited.gov", state=Domain.State.READY) - DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio_and_invited, portfolio=self.portfolio) + domain_in_portfolio_and_invited, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_invited.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_and_invited, portfolio=self.portfolio + ) DomainInvitation.objects.get_or_create(email=self.email, domain=domain_in_portfolio_and_invited) # domain_invited should not be included in the count domain_invited, _ = Domain.objects.get_or_create(name="domain_invited.gov", state=Domain.State.READY) @@ -1302,8 +1306,12 @@ def test_get_portfolio_permissions(self): # Arrange test_permission_list = set() # add the arrays that are defined in UserPortfolioPermission for member and admin - test_permission_list.update(UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, [])) - test_permission_list.update(UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_ADMIN, [])) + test_permission_list.update( + UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, []) + ) + test_permission_list.update( + UserPortfolioPermission.PORTFOLIO_ROLE_PERMISSIONS.get(UserPortfolioRoleChoices.ORGANIZATION_ADMIN, []) + ) # add the permissions that are added to the invitation as additional_permissions test_permission_list.update([self.portfolio_permission_1, self.portfolio_permission_2]) perm_list = list(test_permission_list) @@ -1393,9 +1401,15 @@ def test_get_managed_domains_count(self): domain_in_portfolio, _ = Domain.objects.get_or_create(name="domain_in_portfolio.gov", state=Domain.State.READY) DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio, portfolio=portfolio) # domain_in_portfolio_and_managed should be included in the count - domain_in_portfolio_and_managed, _ = Domain.objects.get_or_create(name="domain_in_portfolio_and_managed.gov", state=Domain.State.READY) - DomainInformation.objects.get_or_create(creator=self.user, domain=domain_in_portfolio_and_managed, portfolio=portfolio) - UserDomainRole.objects.get_or_create(user=test_user, domain=domain_in_portfolio_and_managed, role=UserDomainRole.Roles.MANAGER) + domain_in_portfolio_and_managed, _ = Domain.objects.get_or_create( + name="domain_in_portfolio_and_managed.gov", state=Domain.State.READY + ) + DomainInformation.objects.get_or_create( + creator=self.user, domain=domain_in_portfolio_and_managed, portfolio=portfolio + ) + UserDomainRole.objects.get_or_create( + user=test_user, domain=domain_in_portfolio_and_managed, role=UserDomainRole.Roles.MANAGER + ) # domain_managed should not be included in the count domain_managed, _ = Domain.objects.get_or_create(name="domain_managed.gov", state=Domain.State.READY) DomainInformation.objects.get_or_create(creator=self.user, domain=domain_managed) diff --git a/src/registrar/tests/test_views_members_json.py b/src/registrar/tests/test_views_members_json.py index a79d812744..9cd4e823c3 100644 --- a/src/registrar/tests/test_views_members_json.py +++ b/src/registrar/tests/test_views_members_json.py @@ -109,7 +109,14 @@ def test_get_portfolio_members_json_authenticated(self): self.assertEqual(len(data["members"]), 5) # Check member fields - expected_emails = {self.user.email, self.user2.email, self.user3.email, self.user4.email, self.user4.email, self.email5} + expected_emails = { + self.user.email, + self.user2.email, + self.user3.email, + self.user4.email, + self.user4.email, + self.email5, + } actual_emails = {member["email"] for member in data["members"]} self.assertEqual(expected_emails, actual_emails) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f2dc784f71..13173565c5 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -885,7 +885,7 @@ def test_cannot_view_member_page_when_user_has_no_permission(self): response = self.client.get(reverse("member", kwargs={"pk": 1}), follow=True) # Make sure the page is denied self.assertEqual(response.status_code, 403) - + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @@ -917,10 +917,10 @@ def test_can_view_member_page_when_user_has_view_members(self): self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct - self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present - self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present - self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present - self.assertContains(response, "sprite.svg#visibility") # test that View link is present + self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present + self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present + self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present + self.assertContains(response, "sprite.svg#visibility") # test that View link is present @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -950,13 +950,15 @@ def test_can_view_member_page_when_user_has_edit_members(self): self.assertContains(response, "Admin access") self.assertContains(response, "View all requests plus create requests") self.assertContains(response, "View all members plus manage members") - self.assertContains(response, "This member does not manage any domains. To assign this member a domain, click \"Manage\"") + self.assertContains( + response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + ) # Assert buttons and links within the page are correct - self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present - self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present - self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present + self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present + self.assertContains(response, "sprite.svg#edit") # test that Edit link is present + self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -987,7 +989,7 @@ def test_cannot_view_invitedmember_page_when_user_has_no_permission(self): response = self.client.get(reverse("invitedmember", kwargs={"pk": 1}), follow=True) # Make sure the page is denied self.assertEqual(response.status_code, 403) - + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @@ -1027,10 +1029,10 @@ def test_can_view_invitedmember_page_when_user_has_view_members(self): self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct - self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present - self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present - self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present - self.assertContains(response, "sprite.svg#visibility") # test that View link is present + self.assertNotContains(response, "usa-button--more-actions") # test that 3 dot is not present + self.assertNotContains(response, "sprite.svg#edit") # test that Edit link is not present + self.assertNotContains(response, "sprite.svg#settings") # test that Manage link is not present + self.assertContains(response, "sprite.svg#visibility") # test that View link is present @less_console_noise_decorator @override_flag("organization_feature", active=True) @@ -1068,13 +1070,15 @@ def test_can_view_invitedmember_page_when_user_has_edit_members(self): self.assertContains(response, "Admin access") self.assertContains(response, "View all requests plus create requests") self.assertContains(response, "View all members plus manage members") - self.assertContains(response, "This member does not manage any domains. To assign this member a domain, click \"Manage\"") + self.assertContains( + response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + ) # Assert buttons and links within the page are correct - self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present - self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present - self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present + self.assertContains(response, "usa-button--more-actions") # test that 3 dot is present + self.assertContains(response, "sprite.svg#edit") # test that Edit link is present + self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 8b81a26a9c..948baa07a1 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -2,14 +2,11 @@ from django.http import JsonResponse from django.core.paginator import Paginator from django.contrib.auth.decorators import login_required -from django.db.models import Q from django.urls import reverse from registrar.models.portfolio_invitation import PortfolioInvitation -from registrar.models.user import User from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices -from operator import itemgetter @login_required @@ -18,21 +15,25 @@ def get_portfolio_members_json(request): get all members that are associated with the given portfolio""" portfolio = request.GET.get("portfolio") - permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio).select_related("user").values_list("pk", "user__first_name", "user__last_name", "user__email", "user__last_login", "roles") + permissions = ( + UserPortfolioPermission.objects.filter(portfolio=portfolio) + .select_related("user") + .values_list("pk", "user__first_name", "user__last_name", "user__email", "user__last_login", "roles") + ) invitations = PortfolioInvitation.objects.filter(portfolio=portfolio).values_list( - 'pk', 'email', 'roles', 'additional_permissions', 'status' + "pk", "email", "roles", "additional_permissions", "status" ) # Convert the permissions queryset into a list of dictionaries permission_list = [ { - 'id': perm[0], - 'first_name': perm[1], - 'last_name': perm[2], - 'email': perm[3], - 'last_active': perm[4], - 'roles': perm[5], - 'source': 'permission' # Mark the source as permissions + "id": perm[0], + "first_name": perm[1], + "last_name": perm[2], + "email": perm[3], + "last_active": perm[4], + "roles": perm[5], + "source": "permission", # Mark the source as permissions } for perm in permissions ] @@ -40,15 +41,15 @@ def get_portfolio_members_json(request): # Convert the invitations queryset into a list of dictionaries invitation_list = [ { - 'id': invite[0], - 'first_name': None, # No first name in invitations - 'last_name': None, # No last name in invitations - 'email': invite[1], - 'roles': invite[2], - 'additional_permissions': invite[3], - 'status': invite[4], - 'last_active': 'Invited', - 'source': 'invitation' # Mark the source as invitations + "id": invite[0], + "first_name": None, # No first name in invitations + "last_name": None, # No last name in invitations + "email": invite[1], + "roles": invite[2], + "additional_permissions": invite[3], + "status": invite[4], + "last_active": "Invited", + "source": "invitation", # Mark the source as invitations } for invite in invitations ] @@ -64,12 +65,8 @@ def get_portfolio_members_json(request): paginator = Paginator(combined_list, 10) page_number = request.GET.get("page", 1) page_obj = paginator.get_page(page_number) - - - members = [ - serialize_members(request, portfolio, item, request.user) - for item in page_obj.object_list - ] + + members = [serialize_members(request, portfolio, item, request.user) for item in page_obj.object_list] return JsonResponse( { @@ -90,12 +87,13 @@ def apply_search(data_list, request): if search_term: # Filter the list based on the search term (case-insensitive) data_list = [ - item for item in data_list - if search_term in (item.get('first_name', '') or '').lower() - or search_term in (item.get('last_name', '') or '').lower() - or search_term in (item.get('email', '') or '').lower() + item + for item in data_list + if search_term in (item.get("first_name", "") or "").lower() + or search_term in (item.get("last_name", "") or "").lower() + or search_term in (item.get("email", "") or "").lower() ] - + return data_list @@ -115,11 +113,11 @@ def sort_key(item): # Second element: the actual value to sort by if value is None: return (2, value) # Position None last - if value == 'Invited': + if value == "Invited": return (1, value) # Position 'Invited' before None but after valid datetimes if isinstance(value, datetime): return (0, value) # Position valid datetime values first - + # Default case: return the value as is for comparison return value @@ -144,22 +142,22 @@ def serialize_members(request, portfolio, item, user): # ------- USER STATUSES is_admin = False - if item['roles']: - is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item['roles'] + if item["roles"]: + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item["roles"] - action_url = '#' - if item['source'] == 'permission': - action_url = reverse("member", kwargs={"pk": item['id']}) - elif item['source'] == 'invitation': - action_url = reverse("invitedmember", kwargs={"pk": item['id']}) + action_url = "#" + if item["source"] == "permission": + action_url = reverse("member", kwargs={"pk": item["id"]}) + elif item["source"] == "invitation": + action_url = reverse("invitedmember", kwargs={"pk": item["id"]}) # ------- SERIALIZE member_json = { - "id": item['id'], - "name": (item['first_name'] or '') + ' ' + (item['last_name'] or ''), - "email": item['email'], + "id": item["id"], + "name": (item["first_name"] or "") + " " + (item["last_name"] or ""), + "email": item["email"], "is_admin": is_admin, - "last_active": item['last_active'], + "last_active": item["last_active"], "action_url": action_url, "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index cd79236685..cc1a09b25d 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -3,7 +3,12 @@ from django.shortcuts import render from django.urls import reverse from django.contrib import messages -from registrar.forms.portfolio import PortfolioInvitedMemberForm, PortfolioMemberForm, PortfolioOrgAddressForm, PortfolioSeniorOfficialForm +from registrar.forms.portfolio import ( + PortfolioInvitedMemberForm, + PortfolioMemberForm, + PortfolioOrgAddressForm, + PortfolioSeniorOfficialForm, +) from registrar.models import Portfolio, User from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission @@ -65,22 +70,34 @@ def get(self, request, pk): member = portfolio_permission.user # We have to explicitely name these with member_ otherwise we'll have conflicts with context preprocessors - member_has_view_all_requests_portfolio_permission = member.has_view_all_requests_portfolio_permission(portfolio_permission.portfolio) - member_has_edit_request_portfolio_permission = member.has_edit_request_portfolio_permission(portfolio_permission.portfolio) - member_has_view_members_portfolio_permission = member.has_view_members_portfolio_permission(portfolio_permission.portfolio) - member_has_edit_members_portfolio_permission = member.has_edit_members_portfolio_permission(portfolio_permission.portfolio) - - return render(request, self.template_name, { - 'edit_url': reverse('member-permissions', args=[pk]), - 'portfolio_permission': portfolio_permission, - 'member': member, - 'member_has_view_all_requests_portfolio_permission': member_has_view_all_requests_portfolio_permission, - 'member_has_edit_request_portfolio_permission': member_has_edit_request_portfolio_permission, - 'member_has_view_members_portfolio_permission': member_has_view_members_portfolio_permission, - 'member_has_edit_members_portfolio_permission': member_has_edit_members_portfolio_permission - }) - - + member_has_view_all_requests_portfolio_permission = member.has_view_all_requests_portfolio_permission( + portfolio_permission.portfolio + ) + member_has_edit_request_portfolio_permission = member.has_edit_request_portfolio_permission( + portfolio_permission.portfolio + ) + member_has_view_members_portfolio_permission = member.has_view_members_portfolio_permission( + portfolio_permission.portfolio + ) + member_has_edit_members_portfolio_permission = member.has_edit_members_portfolio_permission( + portfolio_permission.portfolio + ) + + return render( + request, + self.template_name, + { + "edit_url": reverse("member-permissions", args=[pk]), + "portfolio_permission": portfolio_permission, + "member": member, + "member_has_view_all_requests_portfolio_permission": member_has_view_all_requests_portfolio_permission, + "member_has_edit_request_portfolio_permission": member_has_edit_request_portfolio_permission, + "member_has_view_members_portfolio_permission": member_has_view_members_portfolio_permission, + "member_has_edit_members_portfolio_permission": member_has_edit_members_portfolio_permission, + }, + ) + + class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): template_name = "portfolio_member_permissions.html" @@ -89,29 +106,37 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): def get(self, request, pk): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) user = portfolio_permission.user - + form = self.form_class(instance=portfolio_permission) - - return render(request, self.template_name, { - 'form': form, - 'member': user, - }) + + return render( + request, + self.template_name, + { + "form": form, + "member": user, + }, + ) def post(self, request, pk): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=pk) user = portfolio_permission.user - + form = self.form_class(request.POST, instance=portfolio_permission) - + if form.is_valid(): form.save() - return redirect('member', pk=pk) - - return render(request, self.template_name, { - 'form': form, - 'member': user, # Pass the user object again to the template - }) - + return redirect("member", pk=pk) + + return render( + request, + self.template_name, + { + "form": form, + "member": user, # Pass the user object again to the template + }, + ) + class PortfolioInvitedMemberView(PortfolioInvitedMemberPermissionView, View): @@ -123,19 +148,31 @@ def get(self, request, pk): # form = self.form_class(instance=portfolio_invitation) # We have to explicitely name these with member_ otherwise we'll have conflicts with context preprocessors - member_has_view_all_requests_portfolio_permission = UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS in portfolio_invitation.get_portfolio_permissions() - member_has_edit_request_portfolio_permission = UserPortfolioPermissionChoices.EDIT_REQUESTS in portfolio_invitation.get_portfolio_permissions() - member_has_view_members_portfolio_permission = UserPortfolioPermissionChoices.VIEW_MEMBERS in portfolio_invitation.get_portfolio_permissions() - member_has_edit_members_portfolio_permission = UserPortfolioPermissionChoices.EDIT_MEMBERS in portfolio_invitation.get_portfolio_permissions() - - return render(request, self.template_name, { - 'edit_url': reverse('invitedmember-permissions', args=[pk]), - 'portfolio_invitation': portfolio_invitation, - 'member_has_view_all_requests_portfolio_permission': member_has_view_all_requests_portfolio_permission, - 'member_has_edit_request_portfolio_permission': member_has_edit_request_portfolio_permission, - 'member_has_view_members_portfolio_permission': member_has_view_members_portfolio_permission, - 'member_has_edit_members_portfolio_permission': member_has_edit_members_portfolio_permission - }) + member_has_view_all_requests_portfolio_permission = ( + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS in portfolio_invitation.get_portfolio_permissions() + ) + member_has_edit_request_portfolio_permission = ( + UserPortfolioPermissionChoices.EDIT_REQUESTS in portfolio_invitation.get_portfolio_permissions() + ) + member_has_view_members_portfolio_permission = ( + UserPortfolioPermissionChoices.VIEW_MEMBERS in portfolio_invitation.get_portfolio_permissions() + ) + member_has_edit_members_portfolio_permission = ( + UserPortfolioPermissionChoices.EDIT_MEMBERS in portfolio_invitation.get_portfolio_permissions() + ) + + return render( + request, + self.template_name, + { + "edit_url": reverse("invitedmember-permissions", args=[pk]), + "portfolio_invitation": portfolio_invitation, + "member_has_view_all_requests_portfolio_permission": member_has_view_all_requests_portfolio_permission, + "member_has_edit_request_portfolio_permission": member_has_edit_request_portfolio_permission, + "member_has_view_members_portfolio_permission": member_has_view_members_portfolio_permission, + "member_has_edit_members_portfolio_permission": member_has_edit_members_portfolio_permission, + }, + ) class PortfolioInvitedMemberEditView(PortfolioInvitedMemberEditPermissionView, View): @@ -147,23 +184,30 @@ def get(self, request, pk): portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) form = self.form_class(instance=portfolio_invitation) - return render(request, self.template_name, { - 'form': form, - 'invitation': portfolio_invitation, - }) + return render( + request, + self.template_name, + { + "form": form, + "invitation": portfolio_invitation, + }, + ) def post(self, request, pk): portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) form = self.form_class(request.POST, instance=portfolio_invitation) if form.is_valid(): form.save() - return redirect('invitedmember', pk=pk) - - return render(request, self.template_name, { - 'form': form, - 'invitation': portfolio_invitation, # Pass the user object again to the template - }) - + return redirect("invitedmember", pk=pk) + + return render( + request, + self.template_name, + { + "form": form, + "invitation": portfolio_invitation, # Pass the user object again to the template + }, + ) class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 94fc0b1c56..70b7d64a7b 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -515,7 +515,7 @@ def has_permission(self): return False return super().has_permission() - + class PortfolioMemberEditPermission(PortfolioBasePermission): """Permission mixin that allows access to portfolio member pages if user @@ -551,7 +551,7 @@ def has_permission(self): return False return super().has_permission() - + class PortfolioInvitedMemberEditPermission(PortfolioBasePermission): """Permission mixin that allows access to portfolio invited member pages if user diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 6fb7fee502..c1d25d691a 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -288,7 +288,9 @@ class PortfolioInvitedMemberPermissionView(PortfolioInvitedMemberPermission, Por """ -class PortfolioInvitedMemberEditPermissionView(PortfolioInvitedMemberEditPermission, PortfolioBasePermissionView, abc.ABC): +class PortfolioInvitedMemberEditPermissionView( + PortfolioInvitedMemberEditPermission, PortfolioBasePermissionView, abc.ABC +): """Abstract base view for portfolio member edit views that enforces permissions. This abstract view cannot be instantiated. Actual views must specify From acd09d3122dc8d8fe5608d379b8ba8b21425e565 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:05:45 -0400 Subject: [PATCH 050/103] fixed migrations --- ...ns_portfolioinvitation_additional_permissions_and_more.py} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename src/registrar/migrations/{0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py => 0133_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py} (78%) diff --git a/src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py b/src/registrar/migrations/0133_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py similarity index 78% rename from src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py rename to src/registrar/migrations/0133_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py index 338a204933..14b7ac22a3 100644 --- a/src/registrar/migrations/0130_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py +++ b/src/registrar/migrations/0133_rename_portfolio_additional_permissions_portfolioinvitation_additional_permissions_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.10 on 2024-10-07 17:12 +# Generated by Django 4.2.10 on 2024-10-08 19:05 from django.db import migrations @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("registrar", "0129_alter_portfolioinvitation_portfolio_roles_and_more"), + ("registrar", "0132_alter_domaininformation_portfolio_and_more"), ] operations = [ From 0ee78e9102c32947b29c5633148da4b7bfa3d37a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:09:06 -0400 Subject: [PATCH 051/103] comment --- src/registrar/templates/portfolio_member.html | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 46885174d7..5f18442d61 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -124,6 +124,7 @@

    More options

    {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} + {% comment %}view_button is passed below as true in all cases. This is because manage_button logic will trump view_button logic; ie. if manage_button is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} {% elif portfolio_invitation %} From 467b7a90f5f398c693ded053427b92d4e631a73e Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:14:15 -0700 Subject: [PATCH 052/103] Readd try block --- src/registrar/views/domain.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 5d7a840c78..1bc537891b 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -917,17 +917,16 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") + try: + UserDomainRole.objects.create( + user=requested_user, + domain=self.object, + role=UserDomainRole.Roles.MANAGER, + ) + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") else: - try: - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - else: - messages.success(self.request, f"Added user {requested_email}.") + messages.success(self.request, f"Added user {requested_email}.") return redirect(self.get_success_url()) From 17b5f36fbc3e85492b82d995c2c8e12806eff622 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:14:54 -0700 Subject: [PATCH 053/103] Fix indent --- src/registrar/views/domain.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1bc537891b..a30db761ce 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -917,16 +917,16 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - try: - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - else: - messages.success(self.request, f"Added user {requested_email}.") + try: + UserDomainRole.objects.create( + user=requested_user, + domain=self.object, + role=UserDomainRole.Roles.MANAGER, + ) + except IntegrityError: + messages.warning(self.request, f"{requested_email} is already a manager for this domain") + else: + messages.success(self.request, f"Added user {requested_email}.") return redirect(self.get_success_url()) From e396534b3805c65b2a96d687e61cd9ef91cf6ebd Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:15:15 -0700 Subject: [PATCH 054/103] Fix indent --- src/registrar/views/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index a30db761ce..86a22be0f5 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -917,6 +917,7 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") + try: UserDomainRole.objects.create( user=requested_user, From 56d94227e26eeab65ccf829fa7d0a8da351594c2 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:26:09 -0400 Subject: [PATCH 055/103] fixed broken test --- src/registrar/tests/test_views_domains_json.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index 07799104b2..c4e5832c0c 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -37,6 +37,7 @@ def tearDown(self): UserDomainRole.objects.all().delete() UserPortfolioPermission.objects.all().delete() DomainInformation.objects.all().delete() + Domain.objects.all().delete() Portfolio.objects.all().delete() super().tearDown() From 18656a89432951458aa70ed903094ef3bd76aa9f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:30:18 -0400 Subject: [PATCH 056/103] owasp fix --- src/zap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zap.conf b/src/zap.conf index dd9ae15656..710efbc6fa 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -73,6 +73,7 @@ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ +10038 OUTOFSCOPE http://app:8080/permissions # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 6c1c0dd7351421a8c13d9b961d0ae453e1a5e4da Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:35:16 -0400 Subject: [PATCH 057/103] zap fix1 --- src/zap.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index 710efbc6fa..8ae245c5fe 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -73,7 +73,7 @@ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ -10038 OUTOFSCOPE http://app:8080/permissions +10038 OUTOFSCOPE http://app:8080/permissionstemp # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From b1ebfec2fb5e8cddbe62420933c794f2a12c7c96 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:35:37 -0400 Subject: [PATCH 058/103] zap fix2 --- src/zap.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index 8ae245c5fe..710efbc6fa 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -73,7 +73,7 @@ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ -10038 OUTOFSCOPE http://app:8080/permissionstemp +10038 OUTOFSCOPE http://app:8080/permissions # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 40c061af98a6bc3e52270f8d15429bec23ee1cb9 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 15:37:54 -0400 Subject: [PATCH 059/103] fixed tabs in zap --- src/zap.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index 710efbc6fa..9a3897c39a 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -71,9 +71,9 @@ 10038 OUTOFSCOPE http://app:8080/domain_requests/ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ +10038 OUTOFSCOPE http://app:8080/permissions 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ -10038 OUTOFSCOPE http://app:8080/permissions # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 3efd90776457385d7c1f19b113501b44db3ad229 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 16:02:55 -0400 Subject: [PATCH 060/103] incremental fix for spacing in zap.conf --- src/zap.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index 9a3897c39a..dd9ae15656 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -71,7 +71,6 @@ 10038 OUTOFSCOPE http://app:8080/domain_requests/ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ -10038 OUTOFSCOPE http://app:8080/permissions 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. From 2f36be268ea2f24cfe33535f5633e2695ffc14b6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 8 Oct 2024 16:03:54 -0400 Subject: [PATCH 061/103] another incremental fix to zap.conf --- src/zap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zap.conf b/src/zap.conf index dd9ae15656..1f0548f2d7 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -71,6 +71,7 @@ 10038 OUTOFSCOPE http://app:8080/domain_requests/ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ +10038 OUTOFSCOPE http://app:8080/permissions 10038 OUTOFSCOPE http://app:8080/suborganization/ 10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. From 9425d4c35f462eabe7824c7cfc02ca1f156ac28f Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:26:16 -0700 Subject: [PATCH 062/103] Isolate user domain role create --- src/registrar/views/domain.py | 38 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 86a22be0f5..a2d287b696 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -823,10 +823,6 @@ def _send_domain_invitation_email(self, email: str, requestor: User, requested_u email, requestor, requested_user ): add_success = False - messages.error( - self.request, - "That email is already a member of another .gov organization.", - ) raise OutsideOrgMemberError # Check to see if an invite has already been sent @@ -880,6 +876,18 @@ def _make_invitation(self, email_address: str, requestor: User): DomainInvitation.objects.get_or_create(email=email_address, domain=self.object) return redirect(self.get_success_url()) + def _create_user_domain_role(self, requested_user, requested_email, domain, role): + """Assign a user to a domain as a specified role""" + try: + 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") + def form_valid(self, form): """Add the specified user on this domain. Throws EmailSendingError.""" @@ -890,7 +898,8 @@ def form_valid(self, form): requested_user = User.objects.get(email=requested_email) except User.DoesNotExist: # no matching user, go make an invitation - return self._make_invitation(requested_email, requestor) + requested_user = self._make_invitation(requested_email, requestor) + self._create_user_domain_role(requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER) else: # if user already exists then just send an email try: @@ -910,6 +919,10 @@ def form_valid(self, form): self.object, exc_info=True, ) + messages.error( + self.request, + "That email is already a member of another .gov organization.", + ) except Exception: logger.warn( "Could not send email invitation (Other Exception)", @@ -917,17 +930,10 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - - try: - UserDomainRole.objects.create( - user=requested_user, - domain=self.object, - role=UserDomainRole.Roles.MANAGER, - ) - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") - else: - messages.success(self.request, f"Added user {requested_email}.") + else: + self._create_user_domain_role( + requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER + ) return redirect(self.get_success_url()) From 2f2c4e1951701c59ce4e6b9741b87b907b638b31 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:27:36 -0700 Subject: [PATCH 063/103] Simplify try catch --- src/registrar/views/domain.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index a2d287b696..c857e7dd5f 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -906,6 +906,9 @@ def form_valid(self, form): self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) + self._create_user_domain_role( + requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER + ) except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -930,10 +933,6 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - else: - self._create_user_domain_role( - requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER - ) return redirect(self.get_success_url()) From 6a01e5646d969b315761a150b9f931505c31befc Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:39:38 -0700 Subject: [PATCH 064/103] Debug email bug --- src/registrar/views/domain.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index c857e7dd5f..433ae12303 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -876,39 +876,26 @@ def _make_invitation(self, email_address: str, requestor: User): DomainInvitation.objects.get_or_create(email=email_address, domain=self.object) return redirect(self.get_success_url()) - def _create_user_domain_role(self, requested_user, requested_email, domain, role): - """Assign a user to a domain as a specified role""" - try: - 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") - def form_valid(self, form): """Add the specified user on this domain. Throws EmailSendingError.""" requested_email = form.cleaned_data["email"] requestor = self.request.user + email_success = 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 - requested_user = self._make_invitation(requested_email, requestor) - self._create_user_domain_role(requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER) + email_success = True + return self._make_invitation(requested_email, requestor) else: # if user already exists then just send an email try: self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) - self._create_user_domain_role( - requested_user, requested_email, self.object, UserDomainRole.Roles.MANAGER - ) + email_success = True except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -933,6 +920,17 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") + if email_success: + try: + 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") + return redirect(self.get_success_url()) From 889c0a25db9bd2867b5aca571b2d6c1b11267f20 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 8 Oct 2024 16:10:27 -0700 Subject: [PATCH 065/103] Secure portfolio check on requestor org --- src/registrar/views/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 433ae12303..332af5978d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -785,7 +785,7 @@ def _domain_abs_url(self): 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.get(user=requestor).portfolio + 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() From aec1a4f0d439e9892160fd4ee6620a4cb1c5c1d0 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Wed, 9 Oct 2024 15:15:45 -0700 Subject: [PATCH 066/103] Reverse email send check --- src/registrar/views/domain.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 332af5978d..95fa784177 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -881,13 +881,12 @@ def form_valid(self, form): Throws EmailSendingError.""" requested_email = form.cleaned_data["email"] requestor = self.request.user - email_success = False + email_success = True # 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 return self._make_invitation(requested_email, requestor) else: # if user already exists then just send an email @@ -895,7 +894,6 @@ def form_valid(self, form): self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) - email_success = True except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -904,6 +902,7 @@ def form_valid(self, form): ) messages.warning(self.request, "Could not send email invitation.") except OutsideOrgMemberError: + email_send = False logger.warn( "Could not send email. Can not invite member of a .gov organization to a different organization.", self.object, From 76fc71382da59740384a84b11fd2f464ae1f9df4 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Wed, 9 Oct 2024 15:56:20 -0700 Subject: [PATCH 067/103] Re-revert email_success check --- src/registrar/views/domain.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 95fa784177..332af5978d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -881,12 +881,13 @@ def form_valid(self, form): Throws EmailSendingError.""" requested_email = form.cleaned_data["email"] requestor = self.request.user - email_success = True + email_success = 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 return self._make_invitation(requested_email, requestor) else: # if user already exists then just send an email @@ -894,6 +895,7 @@ def form_valid(self, form): self._send_domain_invitation_email( requested_email, requestor, requested_user=requested_user, add_success=False ) + email_success = True except EmailSendingError: logger.warn( "Could not send email invitation (EmailSendingError)", @@ -902,7 +904,6 @@ def form_valid(self, form): ) messages.warning(self.request, "Could not send email invitation.") except OutsideOrgMemberError: - email_send = False logger.warn( "Could not send email. Can not invite member of a .gov organization to a different organization.", self.object, From dd29cbf8cadc168ee9e38641725015dd36eb68d7 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Wed, 9 Oct 2024 16:21:37 -0700 Subject: [PATCH 068/103] Add domain manager after email sending error --- src/registrar/views/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 332af5978d..3865bfc366 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -903,6 +903,7 @@ def form_valid(self, form): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") + email_success = True except OutsideOrgMemberError: logger.warn( "Could not send email. Can not invite member of a .gov organization to a different organization.", From c2bd4cfe974491ab00eb91943e6cca9d06f87d61 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 10 Oct 2024 09:59:58 -0700 Subject: [PATCH 069/103] Update wording for suborganization --- src/registrar/templates/domain_suborganization.html | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index e6152ecd8d..67726e9d52 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -11,8 +11,13 @@

    Suborganization

    The name of your suborganization will be publicly listed as the domain registrant. - This list of suborganizations has been populated the .gov program. - If you believe there is an error please contact help@get.gov. +

    +

    + When this field is blank, the domain registrant will be listed as the overarching organization: {{ portfolio }}. +

    +

    + If you don’t see your suborganization in the menu or need to edit one of the options, + please contact help@get.gov.

    {% if has_any_domains_portfolio_permission and has_edit_suborganization_portfolio_permission %} From 2d88c340a78494d8cb355b245ee71d6d8ae94fb0 Mon Sep 17 00:00:00 2001 From: CuriousX Date: Thu, 10 Oct 2024 21:34:52 -0600 Subject: [PATCH 070/103] Update src/registrar/assets/js/uswds-edited.js Co-authored-by: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> --- src/registrar/assets/js/uswds-edited.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 11a71b0dfd..a76bca276d 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -25,7 +25,7 @@ /** * Edits made for dotgov project: * - tooltip exposed to window to be accessible in other js files - * - tooltip postiioning logic updated to allow position:fixed + * - tooltip positioning logic updated to allow position:fixed * - tooltip dynamic content updated to include nested element (for better sizing control) * - modal exposed to window to be accessible in other js files * - fixed bug in createHeaderButton which added newlines to header button tooltips From d791960fd021ab4fa10f1e490cd98e8649e0937b Mon Sep 17 00:00:00 2001 From: CuriousX Date: Thu, 10 Oct 2024 21:35:04 -0600 Subject: [PATCH 071/103] Update src/registrar/assets/js/uswds-edited.js Co-authored-by: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> --- src/registrar/assets/js/uswds-edited.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index a76bca276d..aee5f05523 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5941,7 +5941,7 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { }; /* - DOTGOV: Tooltip postiioning logic updated to allow position:fixed + DOTGOV: Tooltip positioning logic updated to allow position:fixed */ const tooltipStyle = window.getComputedStyle(tooltipBody); const tooltipIsFixedPositioned = tooltipStyle.position === 'fixed'; From c671c44cbf44484ab1c6481dc5ef4b992f31cb49 Mon Sep 17 00:00:00 2001 From: CuriousX Date: Thu, 10 Oct 2024 21:35:11 -0600 Subject: [PATCH 072/103] Update src/registrar/assets/js/uswds-edited.js Co-authored-by: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> --- src/registrar/assets/js/uswds-edited.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index aee5f05523..c699f14e69 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -6158,7 +6158,7 @@ const setUpAttributes = tooltipTrigger => { tooltipBody.setAttribute("aria-hidden", "true"); // place the text in the tooltip - // DOTGOV: nest the text element to allow us creater control over width and wrapping behavior + // DOTGOV: nest the text element to allow us greater control over width and wrapping behavior tooltipBody.innerHTML = `
    ${tooltipContent} From b34d4cc140f48e01a3b59afcd0860024bec5dedb Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 10 Oct 2024 22:10:11 -0600 Subject: [PATCH 073/103] implemented feedback --- src/registrar/assets/js/uswds-edited.js | 41 +++++++++++++++---- .../assets/sass/_theme/_tooltips.scss | 2 +- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index c699f14e69..42ca52b308 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5940,21 +5940,22 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { return offset; }; - /* - DOTGOV: Tooltip positioning logic updated to allow position:fixed - */ + // ---- DOTGOV EDIT (Added section) + // DOTGOV: Tooltip positioning logic updated to allow position:fixed const tooltipStyle = window.getComputedStyle(tooltipBody); const tooltipIsFixedPositioned = tooltipStyle.position === 'fixed'; const triggerRect = tooltipTrigger.getBoundingClientRect(); //detect if tooltip is set to "fixed" position const targetLeft = tooltipIsFixedPositioned ? triggerRect.left + triggerRect.width/2 + 'px': `50%` const targetTop = tooltipIsFixedPositioned ? triggerRect.top + triggerRect.height/2 + 'px': `50%` if (tooltipIsFixedPositioned) { - // DOTGOV: Add listener to handle scrolling if tooltip position = 'fixed' - // (so that the tooltip doesn't appear to stick to the screen) + /* DOTGOV: Add listener to handle scrolling if tooltip position = 'fixed' + (so that the tooltip doesn't appear to stick to the screen) */ window.addEventListener('scroll', function() { findBestPosition(tooltipBody) }); } + // ---- END DOTGOV EDIT + /** * Positions tooltip at the top * @param {HTMLElement} e - this is the tooltip body @@ -5967,9 +5968,15 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("top"); + // ---- DOTGOV EDIT + // e.style.left = `50%`; // center the element + // e.style.top = `-${TRIANGLE_SIZE}px`; // consider the pseudo element + // DOTGOV: updated logic for position:fixed e.style.left = targetLeft; // center the element e.style.top = tooltipIsFixedPositioned ?`${triggerRect.top-TRIANGLE_SIZE}px`:`-${TRIANGLE_SIZE}px`; // consider the pseudo element + // ---- END DOTGOV EDIT + // apply our margins based on the offset e.style.margin = `-${topMargin}px 0 0 -${leftMargin / 2}px`; }; @@ -5983,10 +5990,15 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", e.offsetWidth, tooltipTrigger); setPositionClass("bottom"); + // ---- DOTGOV EDIT + // e.style.left = `50%`; + // DOTGOV: updated logic for position:fixed if (tooltipIsFixedPositioned){ e.style.top = triggerRect.bottom+'px'; } + // ---- END DOTGOV EDIT + e.style.left = targetLeft; e.style.margin = `${TRIANGLE_SIZE}px 0 0 -${leftMargin / 2}px`; }; @@ -6000,10 +6012,15 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const topMargin = calculateMarginOffset("top", e.offsetHeight, tooltipTrigger); setPositionClass("right"); + // ---- DOTGOV EDIT + // e.style.top = `50%`; + // e.style.left = `${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; + // DOTGOV: updated logic for position:fixed e.style.top = targetTop; e.style.left = tooltipIsFixedPositioned ? `${triggerRect.right + TRIANGLE_SIZE}px`:`${tooltipTrigger.offsetLeft + tooltipTrigger.offsetWidth + TRIANGLE_SIZE}px`; -; + // ---- END DOTGOV EDIT + e.style.margin = `-${topMargin / 2}px 0 0 0`; }; @@ -6019,9 +6036,15 @@ const showToolTip = (tooltipBody, tooltipTrigger, position) => { const leftMargin = calculateMarginOffset("left", tooltipTrigger.offsetLeft > e.offsetWidth ? tooltipTrigger.offsetLeft - e.offsetWidth : e.offsetWidth, tooltipTrigger); setPositionClass("left"); + // ---- DOTGOV EDIT + // e.style.top = `50%`; + // e.style.left = `-${TRIANGLE_SIZE}px`; + // DOTGOV: updated logic for position:fixed e.style.top = targetTop; e.style.left = tooltipIsFixedPositioned ? `${triggerRect.left-TRIANGLE_SIZE}px` : `-${TRIANGLE_SIZE}px`; + // ---- END DOTGOV EDIT + e.style.margin = `-${topMargin / 2}px 0 0 ${tooltipTrigger.offsetLeft > e.offsetWidth ? leftMargin : -leftMargin}px`; // adjust the margin }; @@ -6158,12 +6181,16 @@ const setUpAttributes = tooltipTrigger => { tooltipBody.setAttribute("aria-hidden", "true"); // place the text in the tooltip + + // -- DOTGOV EDIT + // tooltipBody.textContent = tooltipContent; + // DOTGOV: nest the text element to allow us greater control over width and wrapping behavior tooltipBody.innerHTML = `
    ${tooltipContent}
    ` - // tooltipBody.textContent = tooltipContent; + // -- END DOTGOV EDIT return { tooltipBody, diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss index 862d43d3cd..adb8f43a51 100644 --- a/src/registrar/assets/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -66,7 +66,7 @@ text-align: center; font-size: inherit; //inherit tooltip fontsize of .93rem max-width: fit-content; - @include at-media('tablet') { + @include at-media('desktop') { width: 70vw; } } From ce5a11548e4f2f243f08faebdb6ea1ded7dcbb80 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 10:45:45 -0400 Subject: [PATCH 074/103] updated javascript for code readability --- src/registrar/assets/js/get-gov.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index d44e5f2b7d..20c988cdc5 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1932,9 +1932,21 @@ class MembersTable extends LoadTableBase { const member_name = member.name; const member_email = member.email; const options = { year: 'numeric', month: 'short', day: 'numeric' }; - const last_active = member.last_active ? member.last_active != 'Invited' ? new Date(member.last_active) : 'Invited' : null; - const last_active_formatted = last_active ? last_active != 'Invited' ? last_active.toLocaleDateString('en-Us', options) : 'Invited' : ''; - const last_active_sort_value = last_active ? last_active != 'Invited' ? last_active.getTime() : 'Invited' : ''; + // set last_active values + // default values + let last_active = null; + let last_active_formatted = ''; + let last_active_sort_value = ''; + // member.last_active could be null, Invited, or a date; below sets values for all scenarios + if (member.last_active && member.last_active != 'Invited') { + last_active = new Date(member.last_active); + last_active_formatted = last_active.toLocaleDateString('en-Us', options); + last_active_sort_value = last_active.getTime(); + } else { + last_active = 'Invited'; + last_active_formatted = 'Invited'; + last_active_sort_value = 'Invited'; + } const action_url = member.action_url; const action_label = member.action_label; const svg_icon = member.svg_icon; From a466b991ff5fd8509916d866d5251b7abf9accc4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 10:50:24 -0400 Subject: [PATCH 075/103] cleanup of imports --- src/registrar/forms/portfolio.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index b8984023aa..4151ea9bd7 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,12 +4,12 @@ from django import forms from django.core.validators import RegexValidator -from registrar.models.portfolio_invitation import PortfolioInvitation -from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models import ( + PortfolioInvitation, UserPortfolioPermission, + UserPortfolioPermission, DomainInformation, Portfolio, SeniorOfficial +) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from ..models import DomainInformation, Portfolio, SeniorOfficial - logger = logging.getLogger(__name__) From 20eb1e593babf95e718b6a31bba186f51cbbaa1b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 10:58:13 -0400 Subject: [PATCH 076/103] renaming of mgmt template to management --- .../{member_domain_mgmt.html => member_domain_management.html} | 0 src/registrar/templates/includes/summary_item.html | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename src/registrar/templates/includes/{member_domain_mgmt.html => member_domain_management.html} (100%) diff --git a/src/registrar/templates/includes/member_domain_mgmt.html b/src/registrar/templates/includes/member_domain_management.html similarity index 100% rename from src/registrar/templates/includes/member_domain_mgmt.html rename to src/registrar/templates/includes/member_domain_management.html diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index aae7b8cf97..0600d7ea72 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -27,7 +27,7 @@

    {{ sub_header_text }}

    {% if permissions %} {% include "includes/member_permissions.html" with permissions=value %} {% elif domain_mgmt %} - {% include "includes/member_domain_mgmt.html" with domain_count=value %} + {% include "includes/member_domain_management.html" with domain_count=value %} {% elif address %} {% include "includes/organization_address.html" with organization=value %} {% elif contact %} From a31c5ebe318574c6fdf251f72b98c5225e95ab7c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 11:05:37 -0400 Subject: [PATCH 077/103] fixed improper handling of booleans in template --- src/registrar/templates/portfolio_member.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 5f18442d61..0275f84e9b 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -119,18 +119,18 @@

    More options

    {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_permission member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_permission member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions='true' value=portfolio_invitation member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} {% comment %}view_button is passed below as true in all cases. This is because manage_button logic will trump view_button logic; ie. if manage_button is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_permission.get_managed_domains_count edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link='#' editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=portfolio_invitation.get_managed_domains_count edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link='#' editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} {% else %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt='true' value=0 edit_link='#' editable='true' manage_button=has_edit_members_portfolio_permission view_button='true' %} + {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=0 edit_link='#' editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} {% endif %}
    From 5c305c8557dbaf27a85c73f3dbe816cc6a9a7413 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 11:15:43 -0400 Subject: [PATCH 078/103] safer handling of None values in json view --- src/registrar/views/portfolio_members_json.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 948baa07a1..b4e4d230b6 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -89,9 +89,9 @@ def apply_search(data_list, request): data_list = [ item for item in data_list - if search_term in (item.get("first_name", "") or "").lower() - or search_term in (item.get("last_name", "") or "").lower() - or search_term in (item.get("email", "") or "").lower() + if search_term in item.get("first_name", "").lower() + or search_term in item.get("last_name", "").lower() + or search_term in item.get("email", "").lower() ] return data_list @@ -153,11 +153,11 @@ def serialize_members(request, portfolio, item, user): # ------- SERIALIZE member_json = { - "id": item["id"], - "name": (item["first_name"] or "") + " " + (item["last_name"] or ""), - "email": item["email"], + "id": item.get("id",""), + "name": item.get("first_name", "") + " " + item.get("last_name", ""), + "email": item.get("email",""), "is_admin": is_admin, - "last_active": item["last_active"], + "last_active": item.get("last_active", None), "action_url": action_url, "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), From 78a08522237b546236c455be6bb8ca4042640067 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 11 Oct 2024 11:23:42 -0400 Subject: [PATCH 079/103] add member button hidden when user does not have permission --- src/registrar/templates/portfolio_members.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/templates/portfolio_members.html b/src/registrar/templates/portfolio_members.html index 82e06c8089..ffdb63099d 100644 --- a/src/registrar/templates/portfolio_members.html +++ b/src/registrar/templates/portfolio_members.html @@ -18,6 +18,7 @@

    Members

    + {% if has_edit_members_portfolio_permission %}

    Members

    • + {% endif %} {% include "includes/members_table.html" with portfolio=portfolio %} From b683bb1d45a80efecc21422e3dc4cc8d469f9cc1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 11 Oct 2024 16:09:58 -0400 Subject: [PATCH 080/103] fix first and last name join and lint --- src/registrar/forms/portfolio.py | 8 ++++++-- src/registrar/views/portfolio_members_json.py | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 4151ea9bd7..61c2215fa8 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -5,8 +5,12 @@ from django.core.validators import RegexValidator from registrar.models import ( - PortfolioInvitation, UserPortfolioPermission, - UserPortfolioPermission, DomainInformation, Portfolio, SeniorOfficial + PortfolioInvitation, + UserPortfolioPermission, + UserPortfolioPermission, + DomainInformation, + Portfolio, + SeniorOfficial, ) from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index b4e4d230b6..15cda5a9d0 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -153,9 +153,9 @@ def serialize_members(request, portfolio, item, user): # ------- SERIALIZE member_json = { - "id": item.get("id",""), - "name": item.get("first_name", "") + " " + item.get("last_name", ""), - "email": item.get("email",""), + "id": item.get("id", ""), + "name": " ".join(filter(None, [item.get("first_name", ""), item.get("last_name", "")])), + "email": item.get("email", ""), "is_admin": is_admin, "last_active": item.get("last_active", None), "action_url": action_url, From 1aaafb205a07fc95a82f455f7b480873e0d1ea9f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 11 Oct 2024 16:15:38 -0400 Subject: [PATCH 081/103] fix search --- src/registrar/views/portfolio_members_json.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 15cda5a9d0..7c9e9c4f7d 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -89,9 +89,9 @@ def apply_search(data_list, request): data_list = [ item for item in data_list - if search_term in item.get("first_name", "").lower() - or search_term in item.get("last_name", "").lower() - or search_term in item.get("email", "").lower() + if item.get("first_name", "") and search_term in item.get("first_name", "").lower() + or item.get("last_name", "") and search_term in item.get("last_name", "").lower() + or item.get("email", "") and search_term in item.get("email", "").lower() ] return data_list From b103a307e657620c12102d7b1a64f9e8f4f81958 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 11 Oct 2024 16:17:24 -0400 Subject: [PATCH 082/103] lint --- src/registrar/forms/portfolio.py | 1 - src/registrar/views/portfolio_members_json.py | 9 ++++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 61c2215fa8..d24943aac1 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -7,7 +7,6 @@ from registrar.models import ( PortfolioInvitation, UserPortfolioPermission, - UserPortfolioPermission, DomainInformation, Portfolio, SeniorOfficial, diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 7c9e9c4f7d..4978d422b9 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -89,9 +89,12 @@ def apply_search(data_list, request): data_list = [ item for item in data_list - if item.get("first_name", "") and search_term in item.get("first_name", "").lower() - or item.get("last_name", "") and search_term in item.get("last_name", "").lower() - or item.get("email", "") and search_term in item.get("email", "").lower() + if item.get("first_name", "") + and search_term in item.get("first_name", "").lower() + or item.get("last_name", "") + and search_term in item.get("last_name", "").lower() + or item.get("email", "") + and search_term in item.get("email", "").lower() ] return data_list From af83636e02ec0f6055dfb67b5c67dfd7f696f413 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 11 Oct 2024 17:47:03 -0400 Subject: [PATCH 083/103] refactor portfolio_members_json and move back the computation to the ORM --- src/registrar/assets/js/get-gov.js | 35 ++-- src/registrar/views/portfolio_members_json.py | 196 ++++++++---------- 2 files changed, 104 insertions(+), 127 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index 20c988cdc5..5c1967d150 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1880,11 +1880,10 @@ class MembersTable extends LoadTableBase { * @param {*} sortBy - the sort column option * @param {*} order - the sort order {asc, desc} * @param {*} scroll - control for the scrollToElement functionality - * @param {*} status - control for the status filter * @param {*} searchTerm - the search term * @param {*} portfolio - the portfolio id */ - loadTable(page, sortBy = this.currentSortBy, order = this.currentOrder, scroll = this.scrollToTable, status = this.currentStatus, searchTerm =this.currentSearchTerm, portfolio = this.portfolioValue) { + loadTable(page, sortBy = this.currentSortBy, order = this.currentOrder, scroll = this.scrollToTable, searchTerm =this.currentSearchTerm, portfolio = this.portfolioValue) { // --------- SEARCH let searchParams = new URLSearchParams( @@ -1892,7 +1891,6 @@ class MembersTable extends LoadTableBase { "page": page, "sort_by": sortBy, "order": order, - "status": status, "search_term": searchTerm } ); @@ -1932,21 +1930,34 @@ class MembersTable extends LoadTableBase { const member_name = member.name; const member_email = member.email; const options = { year: 'numeric', month: 'short', day: 'numeric' }; - // set last_active values - // default values - let last_active = null; + + // Handle last_active values + let last_active = member.last_active; let last_active_formatted = ''; let last_active_sort_value = ''; - // member.last_active could be null, Invited, or a date; below sets values for all scenarios - if (member.last_active && member.last_active != 'Invited') { - last_active = new Date(member.last_active); - last_active_formatted = last_active.toLocaleDateString('en-Us', options); - last_active_sort_value = last_active.getTime(); + + // Handle 'Invited' or null/empty values differently from valid dates + if (last_active && last_active !== 'Invited') { + try { + // Try to parse the last_active as a valid date + last_active = new Date(last_active); + if (!isNaN(last_active)) { + last_active_formatted = last_active.toLocaleDateString('en-US', options); + last_active_sort_value = last_active.getTime(); // For sorting purposes + } else { + last_active_formatted='Invalid date' + } + } catch (e) { + console.error(`Error parsing date: ${last_active}. Error: ${e}`); + last_active_formatted='Invalid date' + } } else { + // Handle 'Invited' or null last_active = 'Invited'; last_active_formatted = 'Invited'; - last_active_sort_value = 'Invited'; + last_active_sort_value = 'Invited'; // Keep 'Invited' as a sortable string } + const action_url = member.action_url; const action_label = member.action_label; const svg_icon = member.svg_icon; diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 4978d422b9..627d824168 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -1,8 +1,9 @@ -from datetime import datetime from django.http import JsonResponse from django.core.paginator import Paginator from django.contrib.auth.decorators import login_required +from django.db.models import Value, F, CharField, TextField, Q from django.urls import reverse +from django.db.models.functions import Cast from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission @@ -11,58 +12,85 @@ @login_required def get_portfolio_members_json(request): - """Given the current request, - get all members that are associated with the given portfolio""" + """Fetch members (permissions and invitations) for the given portfolio.""" portfolio = request.GET.get("portfolio") + search_term = request.GET.get("search_term", "").lower() + + # Permissions queryset + permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio) + + if search_term: + permissions = permissions.filter( + Q(user__first_name__icontains=search_term) + | Q(user__last_name__icontains=search_term) + | Q(user__email__icontains=search_term) + ) permissions = ( - UserPortfolioPermission.objects.filter(portfolio=portfolio) - .select_related("user") - .values_list("pk", "user__first_name", "user__last_name", "user__email", "user__last_login", "roles") - ) - invitations = PortfolioInvitation.objects.filter(portfolio=portfolio).values_list( - "pk", "email", "roles", "additional_permissions", "status" + permissions.select_related("user") + .annotate( + first_name=F("user__first_name"), + last_name=F("user__last_name"), + email_display=F("user__email"), + last_active=Cast(F("user__last_login"), output_field=TextField()), # Cast last_login to text + roles_display=F("roles"), + additional_permissions_display=F("additional_permissions"), + source=Value("permission", output_field=CharField()), + ) + .values( + "id", + "first_name", + "last_name", + "email_display", + "last_active", + "roles_display", + "additional_permissions_display", + "source", + ) ) - # Convert the permissions queryset into a list of dictionaries - permission_list = [ - { - "id": perm[0], - "first_name": perm[1], - "last_name": perm[2], - "email": perm[3], - "last_active": perm[4], - "roles": perm[5], - "source": "permission", # Mark the source as permissions - } - for perm in permissions - ] + # Invitations queryset + invitations = PortfolioInvitation.objects.filter(portfolio=portfolio) - # Convert the invitations queryset into a list of dictionaries - invitation_list = [ - { - "id": invite[0], - "first_name": None, # No first name in invitations - "last_name": None, # No last name in invitations - "email": invite[1], - "roles": invite[2], - "additional_permissions": invite[3], - "status": invite[4], - "last_active": "Invited", - "source": "invitation", # Mark the source as invitations - } - for invite in invitations - ] + if search_term: + invitations = invitations.filter(Q(email__icontains=search_term)) + + invitations = invitations.annotate( + first_name=Value(None, output_field=CharField()), + last_name=Value(None, output_field=CharField()), + email_display=F("email"), + last_active=Value("Invited", output_field=TextField()), # Use "Invited" as a text value + roles_display=F("roles"), + additional_permissions_display=F("additional_permissions"), + source=Value("invitation", output_field=CharField()), + ).values( + "id", + "first_name", + "last_name", + "email_display", + "last_active", + "roles_display", + "additional_permissions_display", + "source", + ) - # Combine both lists into one unified list - combined_list = permission_list + invitation_list + # Union the two querysets after applying search filters + combined_queryset = permissions.union(invitations) - unfiltered_total = len(combined_list) + # Apply sorting + sort_by = request.GET.get("sort_by", "id") # Default to 'id' + order = request.GET.get("order", "asc") # Default to 'asc' + + # Adjust sort_by to match the annotated fields in the unioned queryset + if sort_by == "member": + sort_by = "email_display" # Use email_display instead of email - combined_list = apply_search(combined_list, request) - combined_list = apply_sorting(combined_list, request) + if order == "desc": + combined_queryset = combined_queryset.order_by(F(sort_by).desc()) + else: + combined_queryset = combined_queryset.order_by(sort_by) - paginator = Paginator(combined_list, 10) + paginator = Paginator(combined_queryset, 10) page_number = request.GET.get("page", 1) page_obj = paginator.get_page(page_number) @@ -76,91 +104,29 @@ def get_portfolio_members_json(request): "has_previous": page_obj.has_previous(), "has_next": page_obj.has_next(), "total": paginator.count, - "unfiltered_total": unfiltered_total, + "unfiltered_total": combined_queryset.count(), } ) -def apply_search(data_list, request): - search_term = request.GET.get("search_term", "").lower() - - if search_term: - # Filter the list based on the search term (case-insensitive) - data_list = [ - item - for item in data_list - if item.get("first_name", "") - and search_term in item.get("first_name", "").lower() - or item.get("last_name", "") - and search_term in item.get("last_name", "").lower() - or item.get("email", "") - and search_term in item.get("email", "").lower() - ] - - return data_list - - -def apply_sorting(data_list, request): - sort_by = request.GET.get("sort_by", "id") # Default to 'id' - order = request.GET.get("order", "asc") # Default to 'asc' - - if sort_by == "member": - sort_by = "email" - - # Custom key function that handles None, 'Invited', and datetime values for last_active - def sort_key(item): - value = item.get(sort_by) - if sort_by == "last_active": - # Return a tuple to ensure consistent data types for comparison - # First element: ordering value (0 for valid datetime, 1 for 'Invited', 2 for None) - # Second element: the actual value to sort by - if value is None: - return (2, value) # Position None last - if value == "Invited": - return (1, value) # Position 'Invited' before None but after valid datetimes - if isinstance(value, datetime): - return (0, value) # Position valid datetime values first - - # Default case: return the value as is for comparison - return value - - # Sort the list using the custom key function - data_list = sorted(data_list, key=sort_key, reverse=(order == "desc")) - - return data_list - - def serialize_members(request, portfolio, item, user): - # ------- VIEW ONLY - # If not view_only (the user has permissions to edit/manage users), show the gear icon with "Manage" link. - # If view_only (the user only has view user permissions), show the "View" link (no gear icon). - # We check on user_group_permision to account for the upcoming "Manage portfolio" button on admin. - user_can_edit_other_users = False - for user_group_permission in ["registrar.full_access_permission", "registrar.change_user"]: - if user.has_perm(user_group_permission): - user_can_edit_other_users = True - break + # Check if the user can edit other users + user_can_edit_other_users = any( + user.has_perm(perm) for perm in ["registrar.full_access_permission", "registrar.change_user"] + ) view_only = not user.has_edit_members_portfolio_permission(portfolio) or not user_can_edit_other_users - # ------- USER STATUSES - is_admin = False - if item["roles"]: - is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item["roles"] - - action_url = "#" - if item["source"] == "permission": - action_url = reverse("member", kwargs={"pk": item["id"]}) - elif item["source"] == "invitation": - action_url = reverse("invitedmember", kwargs={"pk": item["id"]}) + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item.get("roles_display", []) + action_url = reverse("member" if item["source"] == "permission" else "invitedmember", kwargs={"pk": item["id"]}) - # ------- SERIALIZE + # Serialize member data member_json = { "id": item.get("id", ""), "name": " ".join(filter(None, [item.get("first_name", ""), item.get("last_name", "")])), - "email": item.get("email", ""), + "email": item.get("email_display", ""), "is_admin": is_admin, - "last_active": item.get("last_active", None), + "last_active": item.get("last_active", ""), "action_url": action_url, "action_label": ("View" if view_only else "Manage"), "svg_icon": ("visibility" if view_only else "settings"), From 0bd48030e5441c572a3600220840e5391f4f4263 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 11 Oct 2024 18:08:15 -0400 Subject: [PATCH 084/103] fix active class on member link --- src/registrar/templatetags/custom_filters.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index a3f35ae8e2..592839a465 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -248,6 +248,10 @@ def is_members_subpage(path): # Since our pages aren't unified under a common path, we need this approach for now. url_names = [ "members", + "member", + "member-permissions", + "invitedmember", + "invitedmember-permissions" ] return get_url_name(path) in url_names From 1cd6fdda15046172b49d2170dfea10d80d185a59 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 12 Oct 2024 09:45:19 -0400 Subject: [PATCH 085/103] refactored members_json, fixed unfiltered_total, and member sorting --- src/registrar/assets/js/get-gov.js | 3 +- src/registrar/views/portfolio_members_json.py | 120 +++++++++++------- 2 files changed, 79 insertions(+), 44 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index 5c1967d150..baacaee483 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1929,6 +1929,7 @@ class MembersTable extends LoadTableBase { data.members.forEach(member => { const member_name = member.name; const member_email = member.email; + const member_sort_value = member.member_sort_value; const options = { year: 'numeric', month: 'short', day: 'numeric' }; // Handle last_active values @@ -1970,7 +1971,7 @@ class MembersTable extends LoadTableBase { row.innerHTML = ` - ${member_email ? member_email : member_name} ${admin_tagHTML} + ${member_sort_value} ${admin_tagHTML} ${last_active_formatted} diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 627d824168..684f0d3378 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -1,7 +1,8 @@ from django.http import JsonResponse from django.core.paginator import Paginator from django.contrib.auth.decorators import login_required -from django.db.models import Value, F, CharField, TextField, Q +from django.db.models import Value, F, CharField, TextField, Q, Case, When +from django.db.models.functions import Concat, Coalesce from django.urls import reverse from django.db.models.functions import Cast @@ -13,19 +14,44 @@ @login_required def get_portfolio_members_json(request): """Fetch members (permissions and invitations) for the given portfolio.""" + portfolio = request.GET.get("portfolio") - search_term = request.GET.get("search_term", "").lower() - # Permissions queryset - permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio) + # Two initial querysets which will be combined + permissions = initial_permissions_search(portfolio) + invitations = initial_invitations_search(portfolio) - if search_term: - permissions = permissions.filter( - Q(user__first_name__icontains=search_term) - | Q(user__last_name__icontains=search_term) - | Q(user__email__icontains=search_term) - ) + # Get total across both querysets before applying filters + unfiltered_total = permissions.count() + invitations.count() + + permissions = apply_search_term(permissions, request) + invitations = apply_search_term(permissions, request) + + # Union the two querysets + objects = permissions.union(invitations) + objects = apply_sorting(objects, request) + + paginator = Paginator(objects, 10) + page_number = request.GET.get("page", 1) + page_obj = paginator.get_page(page_number) + + members = [serialize_members(request, portfolio, item, request.user) for item in page_obj.object_list] + + return JsonResponse( + { + "members": members, + "page": page_obj.number, + "num_pages": paginator.num_pages, + "has_previous": page_obj.has_previous(), + "has_next": page_obj.has_next(), + "total": paginator.count, + "unfiltered_total": unfiltered_total, + } + ) +def initial_permissions_search(portfolio): + """Perform initial search for permissions before applying any filters.""" + permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio) permissions = ( permissions.select_related("user") .annotate( @@ -35,6 +61,21 @@ def get_portfolio_members_json(request): last_active=Cast(F("user__last_login"), output_field=TextField()), # Cast last_login to text roles_display=F("roles"), additional_permissions_display=F("additional_permissions"), + member_sort_value=Case( + # If email is present and not blank, use email + When(Q(user__email__isnull=False) & ~Q(user__email=""), then=F("user__email")), + # If first name or last name is present, use concatenation of first_name + " " + last_name + When(Q(user__first_name__isnull=False) | Q(user__last_name__isnull=False), + then=Concat( + Coalesce(F("user__first_name"), Value("")), + Value(" "), + Coalesce(F("user__last_name"), Value("")) + ) + ), + # If neither, use an empty string + default=Value(""), + output_field=CharField() + ), source=Value("permission", output_field=CharField()), ) .values( @@ -45,23 +86,23 @@ def get_portfolio_members_json(request): "last_active", "roles_display", "additional_permissions_display", + "member_sort_value", "source", ) ) + return permissions - # Invitations queryset +def initial_invitations_search(portfolio): + """Perform initial invitations search before applying any filters.""" invitations = PortfolioInvitation.objects.filter(portfolio=portfolio) - - if search_term: - invitations = invitations.filter(Q(email__icontains=search_term)) - invitations = invitations.annotate( first_name=Value(None, output_field=CharField()), last_name=Value(None, output_field=CharField()), email_display=F("email"), - last_active=Value("Invited", output_field=TextField()), # Use "Invited" as a text value + last_active=Value("Invited", output_field=TextField()), roles_display=F("roles"), additional_permissions_display=F("additional_permissions"), + member_sort_value=F("email"), source=Value("invitation", output_field=CharField()), ).values( "id", @@ -71,43 +112,35 @@ def get_portfolio_members_json(request): "last_active", "roles_display", "additional_permissions_display", + "member_sort_value", "source", ) + return invitations + - # Union the two querysets after applying search filters - combined_queryset = permissions.union(invitations) +def apply_search_term(queryset, request): + """Apply search term to the queryset.""" + search_term = request.GET.get("search_term", "").lower() + if search_term: + queryset = queryset.filter( + Q(first_name__icontains=search_term) + | Q(last_name__icontains=search_term) + | Q(email_display__icontains=search_term) + ) + return queryset - # Apply sorting +def apply_sorting(queryset, request): + """Apply sorting to the queryset.""" sort_by = request.GET.get("sort_by", "id") # Default to 'id' order = request.GET.get("order", "asc") # Default to 'asc' - # Adjust sort_by to match the annotated fields in the unioned queryset if sort_by == "member": - sort_by = "email_display" # Use email_display instead of email - + sort_by = "member_sort_value" if order == "desc": - combined_queryset = combined_queryset.order_by(F(sort_by).desc()) + queryset = queryset.order_by(F(sort_by).desc()) else: - combined_queryset = combined_queryset.order_by(sort_by) - - paginator = Paginator(combined_queryset, 10) - page_number = request.GET.get("page", 1) - page_obj = paginator.get_page(page_number) - - members = [serialize_members(request, portfolio, item, request.user) for item in page_obj.object_list] - - return JsonResponse( - { - "members": members, - "page": page_obj.number, - "num_pages": paginator.num_pages, - "has_previous": page_obj.has_previous(), - "has_next": page_obj.has_next(), - "total": paginator.count, - "unfiltered_total": combined_queryset.count(), - } - ) - + queryset = queryset.order_by(sort_by) + return queryset def serialize_members(request, portfolio, item, user): # Check if the user can edit other users @@ -125,6 +158,7 @@ def serialize_members(request, portfolio, item, user): "id": item.get("id", ""), "name": " ".join(filter(None, [item.get("first_name", ""), item.get("last_name", "")])), "email": item.get("email_display", ""), + "member_sort_value": item.get("member_sort_value", ""), "is_admin": is_admin, "last_active": item.get("last_active", ""), "action_url": action_url, From d469041a8eaeb80f61b4bb7a22a9340ccad109cb Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 12 Oct 2024 09:47:18 -0400 Subject: [PATCH 086/103] formatted code for readability --- src/registrar/templatetags/custom_filters.py | 8 +---- src/registrar/views/portfolio_members_json.py | 31 +++++++++++-------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index 592839a465..b29dccb082 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -246,13 +246,7 @@ def is_members_subpage(path): """Checks if the given page is a subpage of members. Takes a path name, like '/organization/'.""" # Since our pages aren't unified under a common path, we need this approach for now. - url_names = [ - "members", - "member", - "member-permissions", - "invitedmember", - "invitedmember-permissions" - ] + url_names = ["members", "member", "member-permissions", "invitedmember", "invitedmember-permissions"] return get_url_name(path) in url_names diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 684f0d3378..5194cc4932 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -14,7 +14,7 @@ @login_required def get_portfolio_members_json(request): """Fetch members (permissions and invitations) for the given portfolio.""" - + portfolio = request.GET.get("portfolio") # Two initial querysets which will be combined @@ -23,7 +23,7 @@ def get_portfolio_members_json(request): # Get total across both querysets before applying filters unfiltered_total = permissions.count() + invitations.count() - + permissions = apply_search_term(permissions, request) invitations = apply_search_term(permissions, request) @@ -49,6 +49,7 @@ def get_portfolio_members_json(request): } ) + def initial_permissions_search(portfolio): """Perform initial search for permissions before applying any filters.""" permissions = UserPortfolioPermission.objects.filter(portfolio=portfolio) @@ -65,17 +66,18 @@ def initial_permissions_search(portfolio): # If email is present and not blank, use email When(Q(user__email__isnull=False) & ~Q(user__email=""), then=F("user__email")), # If first name or last name is present, use concatenation of first_name + " " + last_name - When(Q(user__first_name__isnull=False) | Q(user__last_name__isnull=False), - then=Concat( - Coalesce(F("user__first_name"), Value("")), - Value(" "), - Coalesce(F("user__last_name"), Value("")) - ) + When( + Q(user__first_name__isnull=False) | Q(user__last_name__isnull=False), + then=Concat( + Coalesce(F("user__first_name"), Value("")), + Value(" "), + Coalesce(F("user__last_name"), Value("")), + ), + ), + # If neither, use an empty string + default=Value(""), + output_field=CharField(), ), - # If neither, use an empty string - default=Value(""), - output_field=CharField() - ), source=Value("permission", output_field=CharField()), ) .values( @@ -92,6 +94,7 @@ def initial_permissions_search(portfolio): ) return permissions + def initial_invitations_search(portfolio): """Perform initial invitations search before applying any filters.""" invitations = PortfolioInvitation.objects.filter(portfolio=portfolio) @@ -116,7 +119,7 @@ def initial_invitations_search(portfolio): "source", ) return invitations - + def apply_search_term(queryset, request): """Apply search term to the queryset.""" @@ -129,6 +132,7 @@ def apply_search_term(queryset, request): ) return queryset + def apply_sorting(queryset, request): """Apply sorting to the queryset.""" sort_by = request.GET.get("sort_by", "id") # Default to 'id' @@ -142,6 +146,7 @@ def apply_sorting(queryset, request): queryset = queryset.order_by(sort_by) return queryset + def serialize_members(request, portfolio, item, user): # Check if the user can edit other users user_can_edit_other_users = any( From 7bc0dbf9c2e527131a6374a697f58dc0e06dd6da Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 12 Oct 2024 10:19:19 -0400 Subject: [PATCH 087/103] fixed a bug --- src/registrar/views/portfolio_members_json.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 5194cc4932..374982795e 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -25,7 +25,7 @@ def get_portfolio_members_json(request): unfiltered_total = permissions.count() + invitations.count() permissions = apply_search_term(permissions, request) - invitations = apply_search_term(permissions, request) + invitations = apply_search_term(invitations, request) # Union the two querysets objects = permissions.union(invitations) From 71b8bf38a18684f61a283a9542028bdf0147f7de Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 12 Oct 2024 10:40:44 -0400 Subject: [PATCH 088/103] properly handle None condition for roles --- src/registrar/views/portfolio_members_json.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 374982795e..2f43a26eed 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -155,7 +155,7 @@ def serialize_members(request, portfolio, item, user): view_only = not user.has_edit_members_portfolio_permission(portfolio) or not user_can_edit_other_users - is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in item.get("roles_display", []) + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in (item.get("roles_display", [])) action_url = reverse("member" if item["source"] == "permission" else "invitedmember", kwargs={"pk": item["id"]}) # Serialize member data From 857138c4f9bf9625a7d2243c73e286d2f7dec74a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Sat, 12 Oct 2024 10:53:39 -0400 Subject: [PATCH 089/103] properly handle None condition for roles --- src/registrar/views/portfolio_members_json.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 2f43a26eed..3c20e1c5fd 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -155,7 +155,7 @@ def serialize_members(request, portfolio, item, user): view_only = not user.has_edit_members_portfolio_permission(portfolio) or not user_can_edit_other_users - is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in (item.get("roles_display", [])) + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in (item.get("roles_display") or []) action_url = reverse("member" if item["source"] == "permission" else "invitedmember", kwargs={"pk": item["id"]}) # Serialize member data From 445af256b659fbc613c10c74570e99ffd8afdf2e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 15 Oct 2024 08:09:24 -0400 Subject: [PATCH 090/103] variable name cleanup --- src/registrar/assets/js/get-gov.js | 5 ++--- src/registrar/views/portfolio_members_json.py | 20 +++++++++---------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index baacaee483..337baf11c5 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1928,8 +1928,7 @@ class MembersTable extends LoadTableBase { data.members.forEach(member => { const member_name = member.name; - const member_email = member.email; - const member_sort_value = member.member_sort_value; + const member_display = member.member_display; const options = { year: 'numeric', month: 'short', day: 'numeric' }; // Handle last_active values @@ -1971,7 +1970,7 @@ class MembersTable extends LoadTableBase { row.innerHTML = ` - ${member_sort_value} ${admin_tagHTML} + ${member_display} ${admin_tagHTML} ${last_active_formatted} diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 3c20e1c5fd..d2f2276cf1 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -60,9 +60,8 @@ def initial_permissions_search(portfolio): last_name=F("user__last_name"), email_display=F("user__email"), last_active=Cast(F("user__last_login"), output_field=TextField()), # Cast last_login to text - roles_display=F("roles"), additional_permissions_display=F("additional_permissions"), - member_sort_value=Case( + member_display=Case( # If email is present and not blank, use email When(Q(user__email__isnull=False) & ~Q(user__email=""), then=F("user__email")), # If first name or last name is present, use concatenation of first_name + " " + last_name @@ -86,9 +85,9 @@ def initial_permissions_search(portfolio): "last_name", "email_display", "last_active", - "roles_display", + "roles", "additional_permissions_display", - "member_sort_value", + "member_display", "source", ) ) @@ -103,9 +102,8 @@ def initial_invitations_search(portfolio): last_name=Value(None, output_field=CharField()), email_display=F("email"), last_active=Value("Invited", output_field=TextField()), - roles_display=F("roles"), additional_permissions_display=F("additional_permissions"), - member_sort_value=F("email"), + member_display=F("email"), source=Value("invitation", output_field=CharField()), ).values( "id", @@ -113,9 +111,9 @@ def initial_invitations_search(portfolio): "last_name", "email_display", "last_active", - "roles_display", + "roles", "additional_permissions_display", - "member_sort_value", + "member_display", "source", ) return invitations @@ -139,7 +137,7 @@ def apply_sorting(queryset, request): order = request.GET.get("order", "asc") # Default to 'asc' # Adjust sort_by to match the annotated fields in the unioned queryset if sort_by == "member": - sort_by = "member_sort_value" + sort_by = "member_display" if order == "desc": queryset = queryset.order_by(F(sort_by).desc()) else: @@ -155,7 +153,7 @@ def serialize_members(request, portfolio, item, user): view_only = not user.has_edit_members_portfolio_permission(portfolio) or not user_can_edit_other_users - is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in (item.get("roles_display") or []) + is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in (item.get("roles") or []) action_url = reverse("member" if item["source"] == "permission" else "invitedmember", kwargs={"pk": item["id"]}) # Serialize member data @@ -163,7 +161,7 @@ def serialize_members(request, portfolio, item, user): "id": item.get("id", ""), "name": " ".join(filter(None, [item.get("first_name", ""), item.get("last_name", "")])), "email": item.get("email_display", ""), - "member_sort_value": item.get("member_sort_value", ""), + "member_display": item.get("member_display", ""), "is_admin": is_admin, "last_active": item.get("last_active", ""), "action_url": action_url, From 5f55c6dc7d51e9e7640b926d072060ed8ad8f91c Mon Sep 17 00:00:00 2001 From: CocoByte Date: Tue, 15 Oct 2024 12:55:13 -0600 Subject: [PATCH 091/103] re-push feedback implementation --- src/registrar/assets/js/uswds-edited.js | 4 ++-- src/registrar/assets/sass/_theme/_tooltips.scss | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 42ca52b308..52dc441fc2 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -6187,9 +6187,9 @@ const setUpAttributes = tooltipTrigger => { // DOTGOV: nest the text element to allow us greater control over width and wrapping behavior tooltipBody.innerHTML = ` -
    + ${tooltipContent} -
    ` + ` // -- END DOTGOV EDIT return { diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss index adb8f43a51..58beb8ae67 100644 --- a/src/registrar/assets/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -69,5 +69,6 @@ @include at-media('desktop') { width: 70vw; } + display: block; } } \ No newline at end of file From 4393b5a1d7eb6ac4e57931b2536b2ac87ac130b8 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:15:09 -0700 Subject: [PATCH 092/103] Add domain manager page content updates --- src/registrar/templates/domain_add_user.html | 13 ++++++++++--- src/registrar/templates/domain_users.html | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index e95bacd76f..81b6678afe 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -18,10 +18,17 @@ {% endblock breadcrumb %}

    Add a domain manager

    - -

    You can add another user to help manage your domain. If they aren't an organization member they will - need to sign in to the .gov registrar with their Login.gov account. +{% if has_organization_feature %} +

    + You can add another user to help manage your domain. Users can only be a member of one .gov organization, + and they'll need to sign in with their Login.gov account.

    +{% else %} +

    + You can add another user to help manage your domain. If they aren't an organization member they will + need to sign in to the .gov registrar with their Login.gov account. +

    +{% endif %} {% csrf_token %} diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 412f4ee73d..7125f9cb2a 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -8,8 +8,7 @@

    Domain managers

    Domain managers can update all information related to a domain within the - .gov registrar, including contact details, senior official, security - email, and DNS name servers. + .gov registrar, including including security email and DNS name servers.

      @@ -17,6 +16,7 @@

      Domain managers

    • After adding a domain manager, an email invitation will be sent to that user with instructions on how to set up an account.
    • All domain managers must keep their contact information updated and be responsive if contacted by the .gov team.
      • +
    • All domain managers will be notified when updates are made to this domain.
    • Domains must have at least one domain manager. You can’t remove yourself as a domain manager if you’re the only one assigned to this domain. Add another domain manager before you remove yourself from this domain.
    From 961a289e663769155efebfa98cc85020400f2dfb Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:22:16 -0700 Subject: [PATCH 093/103] Update domain manager page content --- src/registrar/templates/domain_add_user.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/templates/domain_add_user.html b/src/registrar/templates/domain_add_user.html index 81b6678afe..fa3f8e8214 100644 --- a/src/registrar/templates/domain_add_user.html +++ b/src/registrar/templates/domain_add_user.html @@ -18,15 +18,15 @@ {% endblock breadcrumb %}

    Add a domain manager

    -{% if has_organization_feature %} +{% if has_organization_feature_flag %}

    You can add another user to help manage your domain. Users can only be a member of one .gov organization, and they'll need to sign in with their Login.gov account.

    {% else %}

    - You can add another user to help manage your domain. If they aren't an organization member they will - need to sign in to the .gov registrar with their Login.gov account. + You can add another user to help manage your domain. They will need to sign in to the .gov registrar with + their Login.gov account.

    {% endif %} From 2dd8d53ec9e7c7099701e33bdde5eb5ffb96b30c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:50:43 -0700 Subject: [PATCH 094/103] Updated with review comments --- .github/pull_request_template.md | 9 +++++---- docs/dev-practices/code_review.md | 8 +++----- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 934c95ab84..e457d7a63c 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -44,14 +44,14 @@ Resolves #00 - [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) - + - [ ] If any updated dependencies on Pipfile, also update dependencies in requirements.txt. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values #### Validated user-facing changes (if applicable) -- [ ] Tag @dotgov-designers for design review. If code is not user-facing, delete design reviewer checklist +- [ ] Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist - [ ] Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) @@ -61,13 +61,13 @@ Resolves #00 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets above code standards and user-facing checklist. Address any checks that are not satisfied +- [ ] Verified code meets all checks above. Address any checks that are not satisfied - [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged - [ ] Checked that all code is adequately covered by tests - [ ] Verify migrations are valid and do not conflict with existing migrations #### Validated user-facing changes as a developer -**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist +**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A. - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability @@ -81,6 +81,7 @@ Resolves #00 - [ ] Checked that the design translated visually - [ ] Checked behavior. Comment any found issues or broken flows. +- [ ] Checked keyboard navigability - [ ] Checked different states (empty, one, some, error) - [ ] Checked for landmarks, page heading structure, and links diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 4a27d71d64..5a8849754e 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -12,12 +12,12 @@ After creating a pull request, pull request submitters should: Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. All other changes require a single approving review. -The submitter is responsible for merging their PR unless the approver is given explcit permission. Similarly, do not commit to another person's branch unless given explicit permission. +The submitter is responsible for merging their PR unless the approver is given explicit permission. Similarly, do not commit to another person's branch unless given explicit permission. Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. ## Pull Requests for User-facing changes -When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. +When making or reviewing user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. Add new pages to the .pa11yci file so they are included in our automated accessibility testing. @@ -29,6 +29,4 @@ Add new pages to the .pa11yci file so they are included in our automated accessi ## Coding standards ### Plain language -All functions and methods should use plain language. - -TODO: Plain language description and examples in code standards ticket. +All functions and methods should use plain language. \ No newline at end of file From 928fe01a6c8995e9d78745d801c371c40c33e393 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 16 Oct 2024 12:12:16 -0400 Subject: [PATCH 095/103] fixes related to PR review comments --- src/registrar/assets/js/get-gov.js | 10 ++++++---- src/registrar/templates/portfolio_member.html | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index 337baf11c5..8281aa50ae 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1926,6 +1926,8 @@ class MembersTable extends LoadTableBase { const memberList = document.querySelector('.members__table tbody'); memberList.innerHTML = ''; + const invited = 'Invited'; + data.members.forEach(member => { const member_name = member.name; const member_display = member.member_display; @@ -1937,7 +1939,7 @@ class MembersTable extends LoadTableBase { let last_active_sort_value = ''; // Handle 'Invited' or null/empty values differently from valid dates - if (last_active && last_active !== 'Invited') { + if (last_active && last_active !== invited) { try { // Try to parse the last_active as a valid date last_active = new Date(last_active); @@ -1953,9 +1955,9 @@ class MembersTable extends LoadTableBase { } } else { // Handle 'Invited' or null - last_active = 'Invited'; - last_active_formatted = 'Invited'; - last_active_sort_value = 'Invited'; // Keep 'Invited' as a sortable string + last_active = invited; + last_active_formatted = invited; + last_active_sort_value = invited; // Keep 'Invited' as a sortable string } const action_url = member.action_url; diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 0275f84e9b..f2ee8f4c58 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -119,9 +119,9 @@

    More options

    {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_permission member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation member_has_view_all_requests_portfolio_permission=member_has_view_all_requests_portfolio_permission member_has_edit_request_portfolio_permission=member_has_edit_request_portfolio_permission member_has_view_members_portfolio_permission=member_has_view_members_portfolio_permission member_has_edit_members_portfolio_permission=member_has_edit_members_portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} + {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} {% comment %}view_button is passed below as true in all cases. This is because manage_button logic will trump view_button logic; ie. if manage_button is true, view_button will never be looked at{% endcomment %} From 3cb341da595f15f471a2a2f5ecc486ae4b4de2f7 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:46:39 -0700 Subject: [PATCH 096/103] Add content updates --- src/registrar/templates/domain_users.html | 2 +- src/registrar/views/domain.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 7125f9cb2a..c41902c6f4 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -17,7 +17,7 @@

    Domain managers

    instructions on how to set up an account.
  • All domain managers must keep their contact information updated and be responsive if contacted by the .gov team.
  • All domain managers will be notified when updates are made to this domain.
    • -
  • Domains must have at least one domain manager. You can’t remove yourself as a domain manager if you’re the only one assigned to this domain. Add another domain manager before you remove yourself from this domain.
    • +
  • Domains must have at least one domain manager. You can’t remove yourself as a domain manager if you’re the only one assigned to this domain.
    • {% if domain.permissions %} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 3865bfc366..de156598a1 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -912,7 +912,7 @@ def form_valid(self, form): ) messages.error( self.request, - "That email is already a member of another .gov organization.", + f"{requested_email} is already a member of another .gov organization.", ) except Exception: logger.warn( From 7885f389212a6635f78d9b74b59ec2a3ce00aa2f Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:49:06 -0700 Subject: [PATCH 097/103] Remove code standards comment --- docs/dev-practices/code_review.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 5a8849754e..7b054cad5e 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -25,7 +25,6 @@ Add new pages to the .pa11yci file so they are included in our automated accessi - Keep pull requests as small as possible. This makes them easier to review and track changes. - Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. -[comment]: The Coding standards section will be moved to a new code standards file in #2898. For now we're simply moving PR template content into the code review document for consolidation ## Coding standards ### Plain language From 6e943b7ecade76a39b9a53dafef50b13855804d4 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 17 Oct 2024 14:17:45 -0700 Subject: [PATCH 098/103] Readd keyboard navigability check on design review --- .github/pull_request_template.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e457d7a63c..b646f78176 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -81,12 +81,10 @@ Resolves #00 - [ ] Checked that the design translated visually - [ ] Checked behavior. Comment any found issues or broken flows. -- [ ] Checked keyboard navigability -- [ ] Checked different states (empty, one, some, error) -- [ ] Checked for landmarks, page heading structure, and links #### Validated user-facing changes as a designer +- [ ] Checked keyboard navigability - [ ] Checked different states (empty, one, some, error) - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - [ ] Tested with multiple browsers (check off which ones were used) From 55841e3ad5de3f9c2dbe5ff073bb6fef9ec01f4d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 17 Oct 2024 14:21:31 -0700 Subject: [PATCH 099/103] Remove references section --- .github/pull_request_template.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b646f78176..40311bd5ff 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -94,9 +94,6 @@ Resolves #00 - [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user -### References -- [Code review best practices](../docs/dev-practices/code_review.md) - ## Screenshots