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 01/46] 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 db0572bb3..dae3b60cd 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 02/46] 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 b2f9fef24..320404fa9 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 03/46] 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 320404fa9..e95bacd76 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 dae3b60cd..c3bbe037a 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 04/46] 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 8cb83c0ee..6a75091a6 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 c3bbe037a..92da57e06 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 05/46] 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 6a75091a6..f12aba221 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 92da57e06..77c02d990 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 06/46] 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 77c02d990..02019c601 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 07/46] 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 02019c601..c2ca65bab 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 08/46] 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 c2ca65bab..5d7a840c7 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 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 09/46] 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 dec0b9fac..4f2349204 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 56d4db394..38ed83232 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 11/46] 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 e493e0a92..e2340bebe 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 12/46] 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 e2340bebe..351ce579b 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 13/46] 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 351ce579b..4d3b76746 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 14/46] 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 4d3b76746..a3646c40a 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 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 15/46] 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 38ed83232..aa3d13404 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 16/46] 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 a3646c40a..ecf117f15 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 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 17/46] 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 ecf117f15..c4e63bccd 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 aa3d13404..f30eec64e 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 18/46] 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 c4e63bccd..20571b305 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 f30eec64e..09e6e0c1c 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 19/46] 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 ab15c660f..5e1c01be9 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 09e6e0c1c..1cea4aa04 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 ec74cfb24d316a152e18343ac309942283407d5d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 7 Oct 2024 11:30:31 -0700 Subject: [PATCH 20/46] Incorporate feedback --- .github/pull_request_template.md | 13 +++++++++---- docs/dev-practices/code_review.md | 1 - 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 20571b305..09f966148 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,6 +1,6 @@ ## Ticket -Resolves #001 +Resolves #00 ## Changes @@ -44,12 +44,14 @@ Resolves #001 #### 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) -- [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing +- [ ] Tag @dotgov-designers 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) @@ -58,10 +60,10 @@ Resolves #001 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets above code standards and user-facing checks. Addresses any checks that are not satisfied +- [ ] Verified code meets above code standards and user-facing checklist. 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 -- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` +- [ ] 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 @@ -92,6 +94,9 @@ Resolves #001 - [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user +### References +- [Code review best practices](../docs/dev-practices/code_review.md) + ## Screenshots Resolves #00 ## Changes 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 23/46] 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 3c64eb6b5..6b6e137cd 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 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 24/46] 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 5d7a840c7..1bc537891 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 25/46] 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 1bc537891..a30db761c 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 26/46] 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 a30db761c..86a22be0f 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 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 27/46] 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 86a22be0f..a2d287b69 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 28/46] 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 a2d287b69..c857e7dd5 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 29/46] 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 c857e7dd5..433ae1230 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 30/46] 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 433ae1230..332af5978 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 31/46] 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 332af5978..95fa78417 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 32/46] 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 95fa78417..332af5978 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 33/46] 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 332af5978..3865bfc36 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 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 34/46] 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 e95bacd76..81b6678af 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 412f4ee73..7125f9cb2 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.

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 35/46] 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 81b6678af..fa3f8e821 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 36/46] 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 934c95ab8..e457d7a63 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 4a27d71d6..5a8849754 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 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 37/46] 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 7125f9cb2..c41902c6f 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 3865bfc36..de156598a 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 38/46] 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 5a8849754..7b054cad5 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 39/46] 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 e457d7a63..b646f7817 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 40/46] 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 b646f7817..40311bd5f 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