From 9c090ae5fe640767da48cb297eadf56e640ef0bd Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 10 Sep 2024 15:37:04 -0500 Subject: [PATCH 001/198] send notification on nameserver changes --- src/registrar/fixtures_users.py | 2 ++ src/registrar/views/domain.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 1b8eda9ab..a1ad0ecf7 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -39,6 +39,7 @@ class UserFixture: "username": "be17c826-e200-4999-9389-2ded48c43691", "first_name": "Matthew", "last_name": "Spence", + "email": "mspence1845@gmail.com" }, { "username": "5f283494-31bd-49b5-b024-a7e7cae00848", @@ -155,6 +156,7 @@ class UserFixture: "username": "d6bf296b-fac5-47ff-9c12-f88ccc5c1b99", "first_name": "Matthew-Analyst", "last_name": "Spence-Analyst", + "email": "mspence1845+1@gmail.com" }, { "username": "319c490d-453b-43d9-bc4d-7d6cd8ff6844", diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 003f8dd0d..1abdb10e5 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -441,6 +441,9 @@ class DomainNameserversView(DomainFormBaseView): # no server information in this field, skip it pass + old_nameservers = self.object.nameservers + should_notify = old_nameservers and old_nameservers != nameservers + try: self.object.nameservers = nameservers except NameserverError as Err: @@ -467,6 +470,30 @@ class DomainNameserversView(DomainFormBaseView): "48 hours to propagate across the internet.", ) + # if the nameservers where changed, send notification to domain managers. + if should_notify: + managers = UserDomainRole.objects.filter(domain=self.object.name, role=UserDomainRole.Roles.MANAGER) + emails = list(managers.values_list("user", flat=True).values_list("email", flat=True)) + to_addresses=', '.join(emails) + + try: + send_templated_email( + "templateName", + "Subject Template Name", + to_address=to_addresses, + context={ + "nameservers": nameservers, + "domain": self.object, + }, + ) + except EmailSendingError as exc: + logger.warn( + "Could not sent notification email to %s for domain %s", + to_addresses, + self.object, + exc_info=True, + ) + # superclass has the redirect return super().form_valid(formset) From 3475a76899b07cee707b355ada8765d3c8289cd3 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 12 Sep 2024 14:39:29 -0500 Subject: [PATCH 002/198] test on nameservers --- src/registrar/utility/email.py | 33 ++++++++++++++++++++++++++++++++- src/registrar/views/domain.py | 24 ++++-------------------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 1fe1be596..39bf9df5b 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -10,6 +10,7 @@ from django.template.loader import get_template from email.mime.application import MIMEApplication from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText +from registrar.models.user_domain_role import UserDomainRole from waffle import flag_is_active @@ -31,7 +32,7 @@ def send_templated_email( attachment_file=None, wrap_email=False, ): - """Send an email built from a template to one email address. + """Send an email built from a template. template_name and subject_template_name are relative to the same template context as Django's HTML templates. context gives additional information @@ -164,3 +165,33 @@ def send_email_with_attachment(sender, recipient, subject, body, attachment_file response = ses_client.send_raw_email(Source=sender, Destinations=[recipient], RawMessage={"Data": msg.as_string()}) return response + +def email_domain_managers(domain, template: str, subject_template: str, context: any = {}): + """Send a single email built from a template to all managers for a given domain. + + template_name and subject_template_name are relative to the same template + context as Django's HTML templates. context gives additional information + that the template may use. + + context is a dictionary containing any information needed to fill in values + in the provided template, exactly the same as with send_templated_email. + + Will log a warning if the email fails to send for any reason, but will not raise an error. + """ + managers = UserDomainRole.objects.filter(domain=domain, role=UserDomainRole.Roles.MANAGER) + emails = list(managers.values_list("user", flat=True).values_list("email", flat=True)) + + try: + send_templated_email( + template, + subject_template, + to_address=emails, + context=context, + ) + except EmailSendingError as exc: + logger.warn( + "Could not sent notification email to %s for domain %s", + emails, + domain, + exc_info=True, + ) \ No newline at end of file diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1abdb10e5..1ac19ec44 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -57,7 +57,7 @@ from epplibwrapper import ( RegistryError, ) -from ..utility.email import send_templated_email, EmailSendingError +from ..utility.email import send_templated_email, EmailSendingError, email_domain_managers from .utility import DomainPermissionView, DomainInvitationPermissionDeleteView from waffle.decorators import waffle_flag @@ -472,27 +472,11 @@ class DomainNameserversView(DomainFormBaseView): # if the nameservers where changed, send notification to domain managers. if should_notify: - managers = UserDomainRole.objects.filter(domain=self.object.name, role=UserDomainRole.Roles.MANAGER) - emails = list(managers.values_list("user", flat=True).values_list("email", flat=True)) - to_addresses=', '.join(emails) - - try: - send_templated_email( - "templateName", - "Subject Template Name", - to_address=to_addresses, - context={ + context={ "nameservers": nameservers, "domain": self.object, - }, - ) - except EmailSendingError as exc: - logger.warn( - "Could not sent notification email to %s for domain %s", - to_addresses, - self.object, - exc_info=True, - ) + } + email_domain_managers(self.object.name, "template", "subject", context) # superclass has the redirect return super().form_valid(formset) From 6c605566a23c5da32fd2168815f6ad7e71d4a9df Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 12 Sep 2024 14:55:44 -0500 Subject: [PATCH 003/198] add temp email template and subject --- src/registrar/templates/emails/domain_change_notification.txt | 1 + .../templates/emails/domain_change_notification_subject.txt | 0 src/registrar/views/domain.py | 3 +-- 3 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 src/registrar/templates/emails/domain_change_notification.txt create mode 100644 src/registrar/templates/emails/domain_change_notification_subject.txt diff --git a/src/registrar/templates/emails/domain_change_notification.txt b/src/registrar/templates/emails/domain_change_notification.txt new file mode 100644 index 000000000..b3c750257 --- /dev/null +++ b/src/registrar/templates/emails/domain_change_notification.txt @@ -0,0 +1 @@ +There has been a change to {{ domain }} \ No newline at end of file diff --git a/src/registrar/templates/emails/domain_change_notification_subject.txt b/src/registrar/templates/emails/domain_change_notification_subject.txt new file mode 100644 index 000000000..e69de29bb diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1ac19ec44..aba504c41 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -473,10 +473,9 @@ class DomainNameserversView(DomainFormBaseView): # if the nameservers where changed, send notification to domain managers. if should_notify: context={ - "nameservers": nameservers, "domain": self.object, } - email_domain_managers(self.object.name, "template", "subject", context) + email_domain_managers(self.object.name, "emails/domain_change_notification.txt", "emails.domain_change_notification_subject.txt", context) # superclass has the redirect return super().form_valid(formset) From 1b7408aebc7c8f1fdcd734c794a5e479c429f415 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 12 Sep 2024 15:12:38 -0500 Subject: [PATCH 004/198] debug logs --- .../templates/emails/domain_change_notification_subject.txt | 1 + src/registrar/utility/email.py | 2 +- src/registrar/views/domain.py | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/templates/emails/domain_change_notification_subject.txt b/src/registrar/templates/emails/domain_change_notification_subject.txt index e69de29bb..d3f6fbedb 100644 --- a/src/registrar/templates/emails/domain_change_notification_subject.txt +++ b/src/registrar/templates/emails/domain_change_notification_subject.txt @@ -0,0 +1 @@ +Change Notification \ No newline at end of file diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 39bf9df5b..4a53661cf 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -180,7 +180,7 @@ def email_domain_managers(domain, template: str, subject_template: str, context: """ managers = UserDomainRole.objects.filter(domain=domain, role=UserDomainRole.Roles.MANAGER) emails = list(managers.values_list("user", flat=True).values_list("email", flat=True)) - + logger.debug("attempting to send templated email to domain managers") try: send_templated_email( template, diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index aba504c41..603fbbab5 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -472,6 +472,7 @@ class DomainNameserversView(DomainFormBaseView): # if the nameservers where changed, send notification to domain managers. if should_notify: + logger.debug("Sending email to domain managers") context={ "domain": self.object, } From 9609db5a53cbb8ca37d18815cedd98547f7de764 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 12 Sep 2024 15:32:49 -0500 Subject: [PATCH 005/198] MOAR LOGS --- src/registrar/views/domain.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 603fbbab5..97fa22d88 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -417,6 +417,8 @@ class DomainNameserversView(DomainFormBaseView): def form_valid(self, formset): """The formset is valid, perform something with it.""" + + logger.debug("------ Form is valid -------") self.request.session["nameservers_form_domain"] = self.object @@ -442,8 +444,9 @@ class DomainNameserversView(DomainFormBaseView): pass old_nameservers = self.object.nameservers + logger.debug("nameservers", nameservers) should_notify = old_nameservers and old_nameservers != nameservers - + logger.debug("should_notify", should_notify) try: self.object.nameservers = nameservers except NameserverError as Err: From 0ffff70cb4c52c45df20f5223a406d78bb46b8b8 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 12 Sep 2024 15:53:14 -0500 Subject: [PATCH 006/198] MOAR DEBUG LOGS --- src/registrar/views/domain.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 97fa22d88..6fbb0fe08 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -403,6 +403,9 @@ class DomainNameserversView(DomainFormBaseView): This post method harmonizes using DomainBaseView and FormMixin """ + + logger.debug("Posted to Namservers View") + self._get_domain(request) formset = self.get_form() From d6996bd189ae50ebfe6480ca79a5c5757b3d92d9 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Fri, 13 Sep 2024 10:07:53 -0500 Subject: [PATCH 007/198] more debug messages --- 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 6fbb0fe08..1a87f185d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -421,7 +421,7 @@ class DomainNameserversView(DomainFormBaseView): def form_valid(self, formset): """The formset is valid, perform something with it.""" - logger.debug("------ Form is valid -------") + logger.debug("------ Nameserver Form is valid -------") self.request.session["nameservers_form_domain"] = self.object From 579a8ac8aa7e129fa5eec90de18030f166a87224 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Fri, 13 Sep 2024 10:41:32 -0500 Subject: [PATCH 008/198] change to info level --- src/registrar/views/domain.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1a87f185d..cc6816121 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -404,7 +404,7 @@ class DomainNameserversView(DomainFormBaseView): This post method harmonizes using DomainBaseView and FormMixin """ - logger.debug("Posted to Namservers View") + logger.info("Posted to Namservers View") self._get_domain(request) formset = self.get_form() @@ -421,7 +421,7 @@ class DomainNameserversView(DomainFormBaseView): def form_valid(self, formset): """The formset is valid, perform something with it.""" - logger.debug("------ Nameserver Form is valid -------") + logger.info("------ Nameserver Form is valid -------") self.request.session["nameservers_form_domain"] = self.object @@ -447,9 +447,9 @@ class DomainNameserversView(DomainFormBaseView): pass old_nameservers = self.object.nameservers - logger.debug("nameservers", nameservers) + logger.info("nameservers", nameservers) should_notify = old_nameservers and old_nameservers != nameservers - logger.debug("should_notify", should_notify) + logger.info("should_notify", should_notify) try: self.object.nameservers = nameservers except NameserverError as Err: @@ -478,7 +478,7 @@ class DomainNameserversView(DomainFormBaseView): # if the nameservers where changed, send notification to domain managers. if should_notify: - logger.debug("Sending email to domain managers") + logger.info("Sending email to domain managers") context={ "domain": self.object, } From 9bbcb98071f5c6a9878d989c30f84f5bfe7d0f1e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 17 Sep 2024 14:22:58 -0500 Subject: [PATCH 009/198] tested up to email sending --- src/registrar/utility/email.py | 79 ++++++++++++++++++---------------- src/registrar/views/domain.py | 69 ++++++++++++++++++++++++++--- 2 files changed, 106 insertions(+), 42 deletions(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 4a53661cf..c1082f50d 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -10,7 +10,6 @@ from django.template.loader import get_template from email.mime.application import MIMEApplication from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText -from registrar.models.user_domain_role import UserDomainRole from waffle import flag_is_active @@ -26,14 +25,20 @@ class EmailSendingError(RuntimeError): def send_templated_email( template_name: str, subject_template_name: str, - to_address: str, - bcc_address="", + to_address: str="", + bcc_address: str="", context={}, attachment_file=None, wrap_email=False, + cc_addresses: list[str]=[], ): """Send an email built from a template. + to can be either a string representing a single address or a + list of strings for multi-recipient emails. + + bcc currently only supports a single address. + template_name and subject_template_name are relative to the same template context as Django's HTML templates. context gives additional information that the template may use. @@ -46,6 +51,8 @@ def send_templated_email( # Raises an error if we cannot send an email (due to restrictions). # Does nothing otherwise. _can_send_email(to_address, bcc_address) + sendable_cc_addresses = get_sendable_addresses(cc_addresses) + template = get_template(template_name) email_body = template.render(context=context) @@ -70,9 +77,18 @@ def send_templated_email( logger.debug("E-mail unable to send! Could not access the SES client.") raise EmailSendingError("Could not access the SES client.") from exc - destination = {"ToAddresses": [to_address]} + destination = {} + if to_address: + destination["ToAddresses"] = [to_address] if bcc_address: destination["BccAddresses"] = [bcc_address] + if cc_addresses: + destination["CcAddresses"] = sendable_cc_addresses + + # make sure we don't try and send an email to nowhere + if not destination: + message = "E-mail unable to send, no valid recipients provided." + raise EmailSendingError(message) try: if not attachment_file: @@ -105,7 +121,6 @@ def send_templated_email( except Exception as exc: raise EmailSendingError("Could not send SES email.") from exc - def _can_send_email(to_address, bcc_address): """Raises an EmailSendingError if we cannot send an email. Does nothing otherwise.""" @@ -123,6 +138,28 @@ def _can_send_email(to_address, bcc_address): if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): raise EmailSendingError(message.format(bcc_address)) +def get_sendable_addresses(addresses: list[str]) -> list[str]: + """Checks whether a list of addresses can be sent to. + + Returns: a lists of all provided addresses that are ok to send to + + Paramaters: + + addresses: a list of strings representing all addresses to be checked. + + raises: + EmailSendingError if email sending is disabled + """ + + if flag_is_active(None, "disable_email_sending"): # type: ignore + message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." + raise EmailSendingError(message) + else: + AllowedEmail = apps.get_model("registrar", "AllowedEmail") + allowed_emails = [address for address in addresses if (address and AllowedEmail.is_allowed_email(address))] + + return allowed_emails + def wrap_text_and_preserve_paragraphs(text, width): """ @@ -164,34 +201,4 @@ def send_email_with_attachment(sender, recipient, subject, body, attachment_file msg.attach(attachment_part) response = ses_client.send_raw_email(Source=sender, Destinations=[recipient], RawMessage={"Data": msg.as_string()}) - return response - -def email_domain_managers(domain, template: str, subject_template: str, context: any = {}): - """Send a single email built from a template to all managers for a given domain. - - template_name and subject_template_name are relative to the same template - context as Django's HTML templates. context gives additional information - that the template may use. - - context is a dictionary containing any information needed to fill in values - in the provided template, exactly the same as with send_templated_email. - - Will log a warning if the email fails to send for any reason, but will not raise an error. - """ - managers = UserDomainRole.objects.filter(domain=domain, role=UserDomainRole.Roles.MANAGER) - emails = list(managers.values_list("user", flat=True).values_list("email", flat=True)) - logger.debug("attempting to send templated email to domain managers") - try: - send_templated_email( - template, - subject_template, - to_address=emails, - context=context, - ) - except EmailSendingError as exc: - logger.warn( - "Could not sent notification email to %s for domain %s", - emails, - domain, - exc_info=True, - ) \ No newline at end of file + return response \ No newline at end of file diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index cc6816121..6b3b6095f 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -57,7 +57,7 @@ from epplibwrapper import ( RegistryError, ) -from ..utility.email import send_templated_email, EmailSendingError, email_domain_managers +from ..utility.email import send_templated_email, EmailSendingError from .utility import DomainPermissionView, DomainInvitationPermissionDeleteView from waffle.decorators import waffle_flag @@ -149,6 +149,43 @@ class DomainFormBaseView(DomainBaseView, FormMixin): logger.error("Could get domain_info. No domain info exists, or duplicates exist.") return current_domain_info + + def email_domain_managers(self, domain_name, template: str, subject_template: str, context: any = {}): + """Send a single email built from a template to all managers for a given domain. + + template_name and subject_template_name are relative to the same template + context as Django's HTML templates. context gives additional information + that the template may use. + + context is a dictionary containing any information needed to fill in values + in the provided template, exactly the same as with send_templated_email. + + Will log a warning if the email fails to send for any reason, but will not raise an error. + """ + try: + domain = Domain.objects.get(name=domain_name) + except Domain.DoesNotExist: + logger.warn( + "Could not send notification email for domain %s, unable to find matching domain object", + domain_name + ) + manager_pks = UserDomainRole.objects.filter(domain=domain.pk, role=UserDomainRole.Roles.MANAGER).values_list("user", flat=True) + emails = list(User.objects.filter(pk__in=manager_pks).values_list("email", flat=True)) + logger.debug("attempting to send templated email to domain managers") + try: + send_templated_email( + template, + subject_template, + context=context, + cc_addresses=emails + ) + except EmailSendingError as exc: + logger.warn( + "Could not sent notification email to %s for domain %s", + emails, + domain_name, + exc_info=True, + ) class DomainView(DomainBaseView): @@ -225,6 +262,13 @@ class DomainOrgNameAddressView(DomainFormBaseView): def form_valid(self, form): """The form is valid, save the organization name and mailing address.""" + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + form.save() messages.success(self.request, "The organization information for this domain has been updated.") @@ -325,6 +369,14 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Set the domain information in the form so that it can be accessible # to associate a new Contact, if a new Contact is needed # in the save() method + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + + form.set_domain_info(self.object.domain_info) form.save() @@ -447,10 +499,15 @@ class DomainNameserversView(DomainFormBaseView): pass old_nameservers = self.object.nameservers - logger.info("nameservers", nameservers) - should_notify = old_nameservers and old_nameservers != nameservers - logger.info("should_notify", should_notify) + logger.info("nameservers %s", nameservers) + logger.info("old nameservers: %s", old_nameservers) + + logger.info("State: %s", self.object.state) + + # if there are existing + logger.info("has changed? %s", formset.has_changed()) try: + # logger.info("skipping actual assignment of nameservers") self.object.nameservers = nameservers except NameserverError as Err: # NamserverErrors *should* be caught in form; if reached here, @@ -477,12 +534,12 @@ class DomainNameserversView(DomainFormBaseView): ) # if the nameservers where changed, send notification to domain managers. - if should_notify: + if formset.has_changed(): logger.info("Sending email to domain managers") context={ "domain": self.object, } - email_domain_managers(self.object.name, "emails/domain_change_notification.txt", "emails.domain_change_notification_subject.txt", context) + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) # superclass has the redirect return super().form_valid(formset) From b8d697ebf04879030b3c08f41398bfe441d05d09 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 18 Sep 2024 14:33:44 -0500 Subject: [PATCH 010/198] another stab at email sending --- ops/manifests/manifest-ms.yaml | 2 +- src/registrar/tests/test_emails.py | 16 ++++++++ src/registrar/tests/test_views_domain.py | 48 ++++++++++++++++++++++++ src/registrar/utility/email.py | 13 +++++-- 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/ops/manifests/manifest-ms.yaml b/ops/manifests/manifest-ms.yaml index 153ee5f08..ac46f5d92 100644 --- a/ops/manifests/manifest-ms.yaml +++ b/ops/manifests/manifest-ms.yaml @@ -20,7 +20,7 @@ applications: # Tell Django where it is being hosted DJANGO_BASE_URL: https://getgov-ms.app.cloud.gov # Tell Django how much stuff to log - DJANGO_LOG_LEVEL: INFO + DJANGO_LOG_LEVEL: DEBUG # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index e699d9b75..e798a0e8f 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -60,6 +60,22 @@ class TestEmails(TestCase): # Assert that an email wasn't sent self.assertFalse(self.mock_client.send_email.called) + + @boto3_mocking.patching + def test_email_with_cc(self): + """Test sending email with cc works""" + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + send_templated_email( + "test content", + "test subject", + "doesnotexist@igorville.com", + context={"domain_request": self}, + bcc_address=None, + cc=["test_email1@example.com", "test_email2@example.com"] + ) + + # check that an email was sent + self.assertTrue(self.mock_client.send_email.called) @boto3_mocking.patching @less_console_noise_decorator diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index b096527f9..a4a9ecf96 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -2,6 +2,7 @@ from unittest import skip from unittest.mock import MagicMock, ANY, patch from django.conf import settings +from django.test import override_settings from django.urls import reverse from django.contrib.auth import get_user_model from waffle.testutils import override_flag @@ -10,6 +11,7 @@ from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore +from django.middleware.csrf import get_token from registrar.utility.errors import ( NameserverError, @@ -1973,3 +1975,49 @@ class TestDomainDNSSEC(TestDomainOverview): self.assertContains( result, str(DsDataError(code=DsDataErrorCodes.INVALID_DIGEST_SHA256)), count=2, status_code=200 ) + + +# class TestDomainChangeNotifications(TestDomainOverview): +# """Test email notifications on updates to domain information""" + +# @classmethod +# def setUpClass(cls): +# super().setUpClass() +# allowed_emails = [ +# AllowedEmail(email="info@example.com"), +# ] +# AllowedEmail.objects.bulk_create(allowed_emails) + +# @classmethod +# def tearDownClass(cls): +# super().tearDownClass() +# AllowedEmail.objects.all().delete() + +# def test_notification_email_sent_on_org_name_change(self): +# """Test that an email is sent when the organization name is changed.""" +# with patch('registrar.utility.email.boto3.client') as mock_boto3_client: +# mock_ses_client = mock_boto3_client.return_value + +# self.domain_information.organization_name = "Town of Igorville" +# self.domain_information.save() + +# org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) +# session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + +# org_name_page.form["organization_name"] = "Not igorville" + +# self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) +# success_result_page = org_name_page.form.submit() + +# # Check that the page loads successfully +# self.assertEqual(success_result_page.status_code, 200) +# self.assertContains(success_result_page, "Not igorville") + +# # Check that an email was sent +# mock_ses_client.send_email.assert_called_once() + +# # Check email content +# call_kwargs = mock_ses_client.send_email.call_args[1] +# self.assertEqual(call_kwargs['FromEmailAddress'], settings.DEFAULT_FROM_EMAIL) +# self.assertIn('Domain information updated', call_kwargs['Content']['Simple']['Subject']['Data']) +# self.assertIn('City of Igorville', call_kwargs['Content']['Simple']['Body']['Text']['Data']) \ No newline at end of file diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index c1082f50d..63d347cae 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -51,7 +51,10 @@ def send_templated_email( # Raises an error if we cannot send an email (due to restrictions). # Does nothing otherwise. _can_send_email(to_address, bcc_address) - sendable_cc_addresses = get_sendable_addresses(cc_addresses) + sendable_cc_addresses, blocked_cc_addresses = get_sendable_addresses(cc_addresses) + + if len(sendable_cc_addresses) < len(cc_addresses): + logger.warning("Some CC'ed addresses were removed: %s.", blocked_cc_addresses) template = get_template(template_name) @@ -107,6 +110,7 @@ def send_templated_email( }, }, ) + logger.info("Email sent to %s, bcc %s, cc %s", to_address, bcc_address, cc_addresses) else: ses_client = boto3.client( "ses", @@ -138,10 +142,10 @@ def _can_send_email(to_address, bcc_address): if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): raise EmailSendingError(message.format(bcc_address)) -def get_sendable_addresses(addresses: list[str]) -> list[str]: +def get_sendable_addresses(addresses: list[str]) -> tuple[list[str], list[str]]: """Checks whether a list of addresses can be sent to. - Returns: a lists of all provided addresses that are ok to send to + Returns: a lists of all provided addresses that are ok to send to and a list of addresses that were blocked. Paramaters: @@ -157,8 +161,9 @@ def get_sendable_addresses(addresses: list[str]) -> list[str]: else: AllowedEmail = apps.get_model("registrar", "AllowedEmail") allowed_emails = [address for address in addresses if (address and AllowedEmail.is_allowed_email(address))] + blocked_emails = [address for address in addresses if (address and not AllowedEmail.is_allowed_email(address))] - return allowed_emails + return allowed_emails, blocked_emails def wrap_text_and_preserve_paragraphs(text, width): From f254e0441fdea44207e16441dd48e143436c260e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 19 Sep 2024 15:10:00 -0500 Subject: [PATCH 011/198] add email sending to all required forms --- src/registrar/views/domain.py | 37 +++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 6b3b6095f..1d693d6e6 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -526,6 +526,14 @@ class DomainNameserversView(DomainFormBaseView): messages.error(self.request, NameserverError(code=nsErrorCodes.BAD_DATA)) logger.error(f"Registry error: {Err}") else: + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + + messages.success( self.request, "The name servers for this domain have been updated. " @@ -533,14 +541,6 @@ class DomainNameserversView(DomainFormBaseView): "48 hours to propagate across the internet.", ) - # if the nameservers where changed, send notification to domain managers. - if formset.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - # superclass has the redirect return super().form_valid(formset) @@ -586,6 +586,13 @@ class DomainDNSSECView(DomainFormBaseView): errmsg = "Error removing existing DNSSEC record(s)." logger.error(errmsg + ": " + err) messages.error(self.request, errmsg) + else: + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) return self.form_valid(form) @@ -710,6 +717,13 @@ class DomainDsDataView(DomainFormBaseView): logger.error(f"Registry error: {err}") return self.form_invalid(formset) else: + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + messages.success(self.request, "The DS data records for this domain have been updated.") # superclass has the redirect return super().form_valid(formset) @@ -808,6 +822,13 @@ class DomainSecurityEmailView(DomainFormBaseView): messages.error(self.request, SecurityEmailError(code=SecurityEmailErrorCodes.BAD_DATA)) logger.error(f"Generic registry error: {Err}") else: + if form.has_changed(): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + messages.success(self.request, "The security email for this domain has been updated.") # superclass has the redirect From 4ce28fd18baa4c9afe4bfb54d568541b4916240a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 19 Sep 2024 15:34:50 -0500 Subject: [PATCH 012/198] test simpler way to organize which emails to send --- src/registrar/views/domain.py | 93 ++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 39 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1d693d6e6..8973ec8ec 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -110,6 +110,22 @@ class DomainFormBaseView(DomainBaseView, FormMixin): implementations of post, form_valid and form_invalid. """ + # send notification email for changes to any of these forms + notify_on_change = ( + DomainSecurityEmailForm, + DomainDnssecForm, + DomainDsdataFormset, + ) + + # forms of these types should not send notifications if they're part of a portfolio/Organization + notify_unless_portfolio = ( + DomainOrgNameAddressForm, + SeniorOfficialContactForm + ) + + def should_notify(self, form) -> bool: + return isinstance(form, self.notify_on_change) or isinstance(form, self.notify_unless_portfolio) + def post(self, request, *args, **kwargs): """Form submission posts to this view. @@ -126,6 +142,13 @@ class DomainFormBaseView(DomainBaseView, FormMixin): # updates session cache with domain self._update_session_with_domain() + if self.should_notify(form): + logger.info("Sending email to domain managers") + context={ + "domain": self.object, + } + self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + # superclass has the redirect return super().form_valid(form) @@ -262,13 +285,6 @@ class DomainOrgNameAddressView(DomainFormBaseView): def form_valid(self, form): """The form is valid, save the organization name and mailing address.""" - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - form.save() messages.success(self.request, "The organization information for this domain has been updated.") @@ -369,12 +385,12 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Set the domain information in the form so that it can be accessible # to associate a new Contact, if a new Contact is needed # in the save() method - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + # if form.has_changed(): + # logger.info("Sending email to domain managers") + # context={ + # "domain": self.object, + # } + # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) form.set_domain_info(self.object.domain_info) @@ -526,13 +542,12 @@ class DomainNameserversView(DomainFormBaseView): messages.error(self.request, NameserverError(code=nsErrorCodes.BAD_DATA)) logger.error(f"Registry error: {Err}") else: - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - + # if form.has_changed(): + # logger.info("Sending email to domain managers") + # context={ + # "domain": self.object, + # } + # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) messages.success( self.request, @@ -586,13 +601,13 @@ class DomainDNSSECView(DomainFormBaseView): errmsg = "Error removing existing DNSSEC record(s)." logger.error(errmsg + ": " + err) messages.error(self.request, errmsg) - else: - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + # else: + # if form.has_changed(): + # logger.info("Sending email to domain managers") + # context={ + # "domain": self.object, + # } + # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) return self.form_valid(form) @@ -717,12 +732,12 @@ class DomainDsDataView(DomainFormBaseView): logger.error(f"Registry error: {err}") return self.form_invalid(formset) else: - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + # if formset.has_changed(): + # logger.info("Sending email to domain managers") + # context={ + # "domain": self.object, + # } + # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) messages.success(self.request, "The DS data records for this domain have been updated.") # superclass has the redirect @@ -822,12 +837,12 @@ class DomainSecurityEmailView(DomainFormBaseView): messages.error(self.request, SecurityEmailError(code=SecurityEmailErrorCodes.BAD_DATA)) logger.error(f"Generic registry error: {Err}") else: - if form.has_changed(): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + # if form.has_changed(): + # logger.info("Sending email to domain managers") + # context={ + # "domain": self.object, + # } + # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) messages.success(self.request, "The security email for this domain has been updated.") From 2f270cded85e3fdef4089e91fc2c1dc8b7536868 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 19 Sep 2024 16:05:15 -0500 Subject: [PATCH 013/198] cleanup and add email content --- ops/manifests/manifest-ms.yaml | 2 +- src/registrar/fixtures_users.py | 2 -- .../emails/domain_change_notification.txt | 1 - .../domain_change_notification_subject.txt | 1 - .../emails/update_to_approved_domain.txt | 23 +++++++++++++++++++ .../update_to_approved_domain_subject.txt | 1 + src/registrar/views/domain.py | 4 ++++ 7 files changed, 29 insertions(+), 5 deletions(-) delete mode 100644 src/registrar/templates/emails/domain_change_notification.txt delete mode 100644 src/registrar/templates/emails/domain_change_notification_subject.txt create mode 100644 src/registrar/templates/emails/update_to_approved_domain.txt create mode 100644 src/registrar/templates/emails/update_to_approved_domain_subject.txt diff --git a/ops/manifests/manifest-ms.yaml b/ops/manifests/manifest-ms.yaml index ac46f5d92..153ee5f08 100644 --- a/ops/manifests/manifest-ms.yaml +++ b/ops/manifests/manifest-ms.yaml @@ -20,7 +20,7 @@ applications: # Tell Django where it is being hosted DJANGO_BASE_URL: https://getgov-ms.app.cloud.gov # Tell Django how much stuff to log - DJANGO_LOG_LEVEL: DEBUG + DJANGO_LOG_LEVEL: INFO # default public site location GETGOV_PUBLIC_SITE_URL: https://get.gov # Flag to disable/enable features in prod environments diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index a1ad0ecf7..1b8eda9ab 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -39,7 +39,6 @@ class UserFixture: "username": "be17c826-e200-4999-9389-2ded48c43691", "first_name": "Matthew", "last_name": "Spence", - "email": "mspence1845@gmail.com" }, { "username": "5f283494-31bd-49b5-b024-a7e7cae00848", @@ -156,7 +155,6 @@ class UserFixture: "username": "d6bf296b-fac5-47ff-9c12-f88ccc5c1b99", "first_name": "Matthew-Analyst", "last_name": "Spence-Analyst", - "email": "mspence1845+1@gmail.com" }, { "username": "319c490d-453b-43d9-bc4d-7d6cd8ff6844", diff --git a/src/registrar/templates/emails/domain_change_notification.txt b/src/registrar/templates/emails/domain_change_notification.txt deleted file mode 100644 index b3c750257..000000000 --- a/src/registrar/templates/emails/domain_change_notification.txt +++ /dev/null @@ -1 +0,0 @@ -There has been a change to {{ domain }} \ No newline at end of file diff --git a/src/registrar/templates/emails/domain_change_notification_subject.txt b/src/registrar/templates/emails/domain_change_notification_subject.txt deleted file mode 100644 index d3f6fbedb..000000000 --- a/src/registrar/templates/emails/domain_change_notification_subject.txt +++ /dev/null @@ -1 +0,0 @@ -Change Notification \ No newline at end of file diff --git a/src/registrar/templates/emails/update_to_approved_domain.txt b/src/registrar/templates/emails/update_to_approved_domain.txt new file mode 100644 index 000000000..93ab4819f --- /dev/null +++ b/src/registrar/templates/emails/update_to_approved_domain.txt @@ -0,0 +1,23 @@ +Hi, +An update was made to a domain you manage. +DOMAIN: {{domain.gov}} +UPDATED BY: {{user}} +UPDATED ON: {{date}} +INFORMATION UPDATED: {{changes}} +You can view this update in the .gov registrar . + + +Get help with managing your .gov domain . + +---------------------------------------------------------------- +WHY DID YOU RECEIVE THIS EMAIL? +You’re listed as a domain manager for $domain.gov, so you’ll receive a notification whenever changes are made to that domain. +If you have questions or concerns, reach out to the person who made the change or reply to this email. + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- +The .gov team +Contact us +Learn about .gov \ No newline at end of file diff --git a/src/registrar/templates/emails/update_to_approved_domain_subject.txt b/src/registrar/templates/emails/update_to_approved_domain_subject.txt new file mode 100644 index 000000000..cf4c9a14c --- /dev/null +++ b/src/registrar/templates/emails/update_to_approved_domain_subject.txt @@ -0,0 +1 @@ +An update was made to {{domain}} \ No newline at end of file diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 8973ec8ec..895d58090 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -5,6 +5,7 @@ authorized users can see information on a domain, every view here should inherit from `DomainPermissionView` (or DomainInvitationPermissionDeleteView). """ +from datetime import date import logging from django.contrib import messages @@ -146,6 +147,9 @@ class DomainFormBaseView(DomainBaseView, FormMixin): logger.info("Sending email to domain managers") context={ "domain": self.object, + "user": self.request.user, + "date": date.today(), + "changes": form.changed_data } self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) From e0025dfa4a952ca51747606a1656c49c758bafe7 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 19 Sep 2024 16:18:11 -0500 Subject: [PATCH 014/198] more cleanup --- src/registrar/utility/email.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 63d347cae..35d8a2029 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -34,16 +34,18 @@ def send_templated_email( ): """Send an email built from a template. - to can be either a string representing a single address or a - list of strings for multi-recipient emails. + to_address and bcc_address currently only supports a single address. - bcc currently only supports a single address. + cc_address is a list and can contain many addresses. Emails not in the + whitelist (if applicable) will be filtered out before sending. template_name and subject_template_name are relative to the same template context as Django's HTML templates. context gives additional information that the template may use. - Raises EmailSendingError if SES client could not be accessed + Raises EmailSendingError if: + SES client could not be accessed + No valid recipient addresses are provided """ if not settings.IS_PRODUCTION: # type: ignore @@ -160,8 +162,13 @@ def get_sendable_addresses(addresses: list[str]) -> tuple[list[str], list[str]]: raise EmailSendingError(message) else: AllowedEmail = apps.get_model("registrar", "AllowedEmail") - allowed_emails = [address for address in addresses if (address and AllowedEmail.is_allowed_email(address))] - blocked_emails = [address for address in addresses if (address and not AllowedEmail.is_allowed_email(address))] + allowed_emails = [] + blocked_emails = [] + for address in addresses: + if AllowedEmail.is_allowed_email(address): + allowed_emails.append(address) + else: + blocked_emails.append(address) return allowed_emails, blocked_emails @@ -206,4 +213,4 @@ def send_email_with_attachment(sender, recipient, subject, body, attachment_file msg.attach(attachment_part) response = ses_client.send_raw_email(Source=sender, Destinations=[recipient], RawMessage={"Data": msg.as_string()}) - return response \ No newline at end of file + return response From 68b8c7de41b7b6c2bed83aa95006afe252162f21 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 19 Sep 2024 16:41:27 -0500 Subject: [PATCH 015/198] fix email template/subject name --- 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 895d58090..bd5d60756 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -151,7 +151,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): "date": date.today(), "changes": form.changed_data } - self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) # superclass has the redirect return super().form_valid(form) From 58d42156ba1d0ba14951985826e7abf7c9b2e26c Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 19 Sep 2024 16:57:56 -0500 Subject: [PATCH 016/198] add super call for form valid on security email form --- src/registrar/views/domain.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index bd5d60756..fc7be8fa7 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -143,6 +143,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): # updates session cache with domain self._update_session_with_domain() + logger.info("Valid form has changed? %s", form.has_changed()) if self.should_notify(form): logger.info("Sending email to domain managers") context={ @@ -849,6 +850,9 @@ class DomainSecurityEmailView(DomainFormBaseView): # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) messages.success(self.request, "The security email for this domain has been updated.") + + # superclass has the redirect + return super().form_valid(form) # superclass has the redirect return redirect(self.get_success_url()) From 63eea77e802a12f9502d17bd4b22b048a370b3b4 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 20 Sep 2024 13:11:37 -0500 Subject: [PATCH 017/198] another refactor --- src/registrar/views/domain.py | 145 ++++++++++++++++------------------ 1 file changed, 69 insertions(+), 76 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 1ab8af816..5d91aadb3 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -109,22 +109,6 @@ class DomainFormBaseView(DomainBaseView, FormMixin): implementations of post, form_valid and form_invalid. """ - # send notification email for changes to any of these forms - notify_on_change = ( - DomainSecurityEmailForm, - DomainDnssecForm, - DomainDsdataFormset, - ) - - # forms of these types should not send notifications if they're part of a portfolio/Organization - notify_unless_portfolio = ( - DomainOrgNameAddressForm, - SeniorOfficialContactForm - ) - - def should_notify(self, form) -> bool: - return isinstance(form, self.notify_on_change) or isinstance(form, self.notify_unless_portfolio) - def post(self, request, *args, **kwargs): """Form submission posts to this view. @@ -141,17 +125,6 @@ class DomainFormBaseView(DomainBaseView, FormMixin): # updates session cache with domain self._update_session_with_domain() - logger.info("Valid form has changed? %s", form.has_changed()) - if self.should_notify(form): - logger.info("Sending email to domain managers") - context={ - "domain": self.object, - "user": self.request.user, - "date": date.today(), - "changes": form.changed_data - } - self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) - # superclass has the redirect return super().form_valid(form) @@ -176,6 +149,63 @@ class DomainFormBaseView(DomainBaseView, FormMixin): return current_domain_info + def send_update_notification(self, form, is_formset: bool=False): + """Send a notification to all domain managers that an update has occured + for a single domain. Uses update_to_approved_domain.txt template. + + Checks for changes, and does nothing if the form has not changed. + + Formsets have to be handled in a special way, so use is_formset to indicate + whether the value passed into form is actually a formset. + """ + + # send notification email for changes to any of these forms + notify_on_change = ( + DomainSecurityEmailForm, + DomainDnssecForm, + DomainDsdataFormset, + ) + + # forms of these types should not send notifications if they're part of a portfolio/Organization + notify_unless_in_portfolio = ( + DomainOrgNameAddressForm, + SeniorOfficialContactForm + ) + + if isinstance(form, notify_on_change): + # always notify for these forms + should_notify=True + elif isinstance(form, notify_unless_in_portfolio): + # for these forms, only notify if the domain is not in a portfolio + info: DomainInformation = self.get_domain_info_from_domain() + if not info or info.portfolio: + should_notify = False + else: + should_notify=True + else: + # don't notify for any other types of forms + should_notify=False + + if should_notify and form.has_changed: + logger.info("Sending email to domain managers") + + changes = self._get_changes_from_formset(form) if is_formset else form.changed_data + + context={ + "domain": self.object.name, + "user": self.request.user, + "date": date.today(), + "changes": str(changes).strip("'") # django templates auto-escape quotes + } + self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) + + def _get_changes_from_formset(self, formset): + changes = set() + for form in formset: + changes.update(form.get_changes) + + return list(changes) + def email_domain_managers(self, domain_name, template: str, subject_template: str, context: any = {}): """Send a single email built from a template to all managers for a given domain. @@ -191,7 +221,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): try: domain = Domain.objects.get(name=domain_name) except Domain.DoesNotExist: - logger.warn( + logger.warning( "Could not send notification email for domain %s, unable to find matching domain object", domain_name ) @@ -206,7 +236,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): cc_addresses=emails ) except EmailSendingError as exc: - logger.warn( + logger.warning( "Could not sent notification email to %s for domain %s", emails, domain_name, @@ -290,6 +320,8 @@ class DomainOrgNameAddressView(DomainFormBaseView): """The form is valid, save the organization name and mailing address.""" form.save() + self.send_update_notification(form) + messages.success(self.request, "The organization information for this domain has been updated.") # superclass has the redirect @@ -387,18 +419,12 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Set the domain information in the form so that it can be accessible # to associate a new Contact, if a new Contact is needed - # in the save() method - # if form.has_changed(): - # logger.info("Sending email to domain managers") - # context={ - # "domain": self.object, - # } - # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - - + # in the save() methodS form.set_domain_info(self.object.domain_info) form.save() + self.send_update_notification(form) + messages.success(self.request, "The senior official for this domain has been updated.") # superclass has the redirect @@ -516,17 +542,7 @@ class DomainNameserversView(DomainFormBaseView): except KeyError: # no server information in this field, skip it pass - - old_nameservers = self.object.nameservers - logger.info("nameservers %s", nameservers) - logger.info("old nameservers: %s", old_nameservers) - - logger.info("State: %s", self.object.state) - - # if there are existing - logger.info("has changed? %s", formset.has_changed()) try: - # logger.info("skipping actual assignment of nameservers") self.object.nameservers = nameservers except NameserverError as Err: # NamserverErrors *should* be caught in form; if reached here, @@ -545,13 +561,7 @@ class DomainNameserversView(DomainFormBaseView): messages.error(self.request, NameserverError(code=nsErrorCodes.BAD_DATA)) logger.error(f"Registry error: {Err}") else: - # if form.has_changed(): - # logger.info("Sending email to domain managers") - # context={ - # "domain": self.object, - # } - # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - + self.send_update_notification(formset, is_formset=True) messages.success( self.request, "The name servers for this domain have been updated. " @@ -604,14 +614,8 @@ class DomainDNSSECView(DomainFormBaseView): errmsg = "Error removing existing DNSSEC record(s)." logger.error(errmsg + ": " + err) messages.error(self.request, errmsg) - # else: - # if form.has_changed(): - # logger.info("Sending email to domain managers") - # context={ - # "domain": self.object, - # } - # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - + else: + self.send_update_notification(form) return self.form_valid(form) @@ -735,12 +739,7 @@ class DomainDsDataView(DomainFormBaseView): logger.error(f"Registry error: {err}") return self.form_invalid(formset) else: - # if formset.has_changed(): - # logger.info("Sending email to domain managers") - # context={ - # "domain": self.object, - # } - # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) + self.send_update_notification(formset, is_formset=True) messages.success(self.request, "The DS data records for this domain have been updated.") # superclass has the redirect @@ -808,13 +807,7 @@ class DomainSecurityEmailView(DomainFormBaseView): messages.error(self.request, SecurityEmailError(code=SecurityEmailErrorCodes.BAD_DATA)) logger.error(f"Generic registry error: {Err}") else: - # if form.has_changed(): - # logger.info("Sending email to domain managers") - # context={ - # "domain": self.object, - # } - # self.email_domain_managers(self.object, "emails/domain_change_notification.txt", "emails/domain_change_notification_subject.txt", context) - + self.send_update_notification(form) messages.success(self.request, "The security email for this domain has been updated.") # superclass has the redirect From 07c1060f6e98eaa324a42a03530af74011733057 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 20 Sep 2024 13:19:29 -0500 Subject: [PATCH 018/198] fix minor bug in logger formatting --- src/registrar/config/settings.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 03d9e38c6..1b20caf2a 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -475,6 +475,10 @@ class JsonServerFormatter(ServerFormatter): def format(self, record): formatted_record = super().format(record) + + if not hasattr(record, "server_time"): + record.server_time = self.formatTime(record, self.datefmt) + log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) From cb7005611a344c69dc48f196c230406b6d18a109 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 20 Sep 2024 13:28:45 -0500 Subject: [PATCH 019/198] typo fix --- 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 5d91aadb3..89a1338e6 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -202,7 +202,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): def _get_changes_from_formset(self, formset): changes = set() for form in formset: - changes.update(form.get_changes) + changes.update(form.changed_data) return list(changes) From 5dbc356d711d1163c9e611b754b24509afcc0211 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 20 Sep 2024 13:53:43 -0500 Subject: [PATCH 020/198] map forms to labels --- src/registrar/views/domain.py | 53 +++++++++++++---------------------- 1 file changed, 20 insertions(+), 33 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 89a1338e6..47f72d0f8 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -149,39 +149,35 @@ class DomainFormBaseView(DomainBaseView, FormMixin): return current_domain_info - def send_update_notification(self, form, is_formset: bool=False): + def send_update_notification(self, form): """Send a notification to all domain managers that an update has occured for a single domain. Uses update_to_approved_domain.txt template. Checks for changes, and does nothing if the form has not changed. - - Formsets have to be handled in a special way, so use is_formset to indicate - whether the value passed into form is actually a formset. """ # send notification email for changes to any of these forms - notify_on_change = ( - DomainSecurityEmailForm, - DomainDnssecForm, - DomainDsdataFormset, - ) + form_label_dict = { + DomainSecurityEmailForm: "Security Email", + DomainDnssecForm: "DNSSec", + DomainDsdataFormset: "DS Data", + DomainOrgNameAddressForm: "Org Name/Address", + SeniorOfficialContactForm: "Senior Official", + } # forms of these types should not send notifications if they're part of a portfolio/Organization - notify_unless_in_portfolio = ( + check_for_portfolio = { DomainOrgNameAddressForm, - SeniorOfficialContactForm - ) + SeniorOfficialContactForm, + } - if isinstance(form, notify_on_change): - # always notify for these forms + if form.__class__ in form_label_dict: should_notify=True - elif isinstance(form, notify_unless_in_portfolio): - # for these forms, only notify if the domain is not in a portfolio - info: DomainInformation = self.get_domain_info_from_domain() - if not info or info.portfolio: - should_notify = False - else: - should_notify=True + if form.__class__ in check_for_portfolio: + # check for portfolio + info = self.get_domain_info_from_domain() + if not info or info.portfolio: + should_notify = False else: # don't notify for any other types of forms should_notify=False @@ -189,22 +185,13 @@ class DomainFormBaseView(DomainBaseView, FormMixin): if should_notify and form.has_changed: logger.info("Sending email to domain managers") - changes = self._get_changes_from_formset(form) if is_formset else form.changed_data - context={ "domain": self.object.name, "user": self.request.user, "date": date.today(), - "changes": str(changes).strip("'") # django templates auto-escape quotes + "changes": form_label_dict[form.__class__] } self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) - - def _get_changes_from_formset(self, formset): - changes = set() - for form in formset: - changes.update(form.changed_data) - - return list(changes) def email_domain_managers(self, domain_name, template: str, subject_template: str, context: any = {}): """Send a single email built from a template to all managers for a given domain. @@ -561,7 +548,7 @@ class DomainNameserversView(DomainFormBaseView): messages.error(self.request, NameserverError(code=nsErrorCodes.BAD_DATA)) logger.error(f"Registry error: {Err}") else: - self.send_update_notification(formset, is_formset=True) + self.send_update_notification(formset) messages.success( self.request, "The name servers for this domain have been updated. " @@ -739,7 +726,7 @@ class DomainDsDataView(DomainFormBaseView): logger.error(f"Registry error: {err}") return self.form_invalid(formset) else: - self.send_update_notification(formset, is_formset=True) + self.send_update_notification(formset) messages.success(self.request, "The DS data records for this domain have been updated.") # superclass has the redirect From ccbefe209c408c1dba14179c327921c234315989 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Tue, 24 Sep 2024 10:11:55 -0500 Subject: [PATCH 021/198] Minor refactor fo update notification and formatting changes --- .../emails/update_to_approved_domain.txt | 9 ++++++--- src/registrar/views/domain.py | 16 +++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/registrar/templates/emails/update_to_approved_domain.txt b/src/registrar/templates/emails/update_to_approved_domain.txt index 93ab4819f..bc0950808 100644 --- a/src/registrar/templates/emails/update_to_approved_domain.txt +++ b/src/registrar/templates/emails/update_to_approved_domain.txt @@ -1,6 +1,8 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} + Hi, An update was made to a domain you manage. -DOMAIN: {{domain.gov}} +DOMAIN: {{domain}} UPDATED BY: {{user}} UPDATED ON: {{date}} INFORMATION UPDATED: {{changes}} @@ -11,7 +13,7 @@ Get help with managing your .gov domain -Learn about .gov \ No newline at end of file +Learn about .gov +{% endautoescape %} \ No newline at end of file diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 47f72d0f8..04b740f6e 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -149,11 +149,12 @@ class DomainFormBaseView(DomainBaseView, FormMixin): return current_domain_info - def send_update_notification(self, form): + def send_update_notification(self, form, force_send=False): """Send a notification to all domain managers that an update has occured for a single domain. Uses update_to_approved_domain.txt template. - Checks for changes, and does nothing if the form has not changed. + If there are no changes to the form, emails will NOT be sent unless force_send + is set to True. """ # send notification email for changes to any of these forms @@ -172,9 +173,10 @@ class DomainFormBaseView(DomainBaseView, FormMixin): } if form.__class__ in form_label_dict: + # these types of forms can cause notifications should_notify=True if form.__class__ in check_for_portfolio: - # check for portfolio + # some forms shouldn't cause notifications if they are in a portfolio info = self.get_domain_info_from_domain() if not info or info.portfolio: should_notify = False @@ -182,7 +184,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): # don't notify for any other types of forms should_notify=False - if should_notify and form.has_changed: + if (should_notify and form.has_changed()) or force_send: logger.info("Sending email to domain managers") context={ @@ -305,10 +307,10 @@ class DomainOrgNameAddressView(DomainFormBaseView): def form_valid(self, form): """The form is valid, save the organization name and mailing address.""" - form.save() - self.send_update_notification(form) + form.save() + messages.success(self.request, "The organization information for this domain has been updated.") # superclass has the redirect @@ -602,7 +604,7 @@ class DomainDNSSECView(DomainFormBaseView): logger.error(errmsg + ": " + err) messages.error(self.request, errmsg) else: - self.send_update_notification(form) + self.send_update_notification(form, force_send=True) return self.form_valid(form) From 89f19bf80718b1f913182edf1c7ad60379f05307 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 24 Sep 2024 10:23:51 -0500 Subject: [PATCH 022/198] uncomment test --- src/registrar/tests/test_views_domain.py | 69 ++++++++++++------------ 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index f0aea8588..109b80913 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -1968,47 +1968,46 @@ class TestDomainDNSSEC(TestDomainOverview): ) -# class TestDomainChangeNotifications(TestDomainOverview): -# """Test email notifications on updates to domain information""" +class TestDomainChangeNotifications(TestDomainOverview): + """Test email notifications on updates to domain information""" -# @classmethod -# def setUpClass(cls): -# super().setUpClass() -# allowed_emails = [ -# AllowedEmail(email="info@example.com"), -# ] -# AllowedEmail.objects.bulk_create(allowed_emails) + @classmethod + def setUpClass(cls): + super().setUpClass() + allowed_emails = [ + AllowedEmail(email="info@example.com"), + ] + AllowedEmail.objects.bulk_create(allowed_emails) -# @classmethod -# def tearDownClass(cls): -# super().tearDownClass() -# AllowedEmail.objects.all().delete() + @classmethod + def tearDownClass(cls): + super().tearDownClass() + AllowedEmail.objects.all().delete() -# def test_notification_email_sent_on_org_name_change(self): -# """Test that an email is sent when the organization name is changed.""" -# with patch('registrar.utility.email.boto3.client') as mock_boto3_client: -# mock_ses_client = mock_boto3_client.return_value + def test_notification_email_sent_on_org_name_change(self): + """Test that an email is sent when the organization name is changed.""" + with patch('registrar.utility.email.boto3.client') as mock_boto3_client: + mock_ses_client = mock_boto3_client.return_value -# self.domain_information.organization_name = "Town of Igorville" -# self.domain_information.save() + self.domain_information.organization_name = "Town of Igorville" + self.domain_information.save() -# org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) -# session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] -# org_name_page.form["organization_name"] = "Not igorville" + org_name_page.form["organization_name"] = "Not igorville" -# self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) -# success_result_page = org_name_page.form.submit() + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + success_result_page = org_name_page.form.submit() + # Check that the page loads successfully + self.assertEqual(success_result_page.status_code, 200) + self.assertContains(success_result_page, "Not igorville") -# # Check that the page loads successfully -# self.assertEqual(success_result_page.status_code, 200) -# self.assertContains(success_result_page, "Not igorville") - -# # Check that an email was sent -# mock_ses_client.send_email.assert_called_once() + # Check that an email was sent + mock_ses_client.send_email.assert_called_once() -# # Check email content -# call_kwargs = mock_ses_client.send_email.call_args[1] -# self.assertEqual(call_kwargs['FromEmailAddress'], settings.DEFAULT_FROM_EMAIL) -# self.assertIn('Domain information updated', call_kwargs['Content']['Simple']['Subject']['Data']) -# self.assertIn('City of Igorville', call_kwargs['Content']['Simple']['Body']['Text']['Data']) \ No newline at end of file + # Check email content + call_kwargs = mock_ses_client.send_email.call_args[1] + self.assertEqual(call_kwargs['FromEmailAddress'], settings.DEFAULT_FROM_EMAIL) + self.assertIn('DOMAIN: Igorville.gov', call_kwargs['Content']['Simple']['Subject']['Data']) + self.assertIn('INFORMATION UPDATED: Org Name/Address', call_kwargs['Content']['Simple']['Body']['Text']['Data']) \ No newline at end of file From 9de6831591f657d80487aeddb54bd2f4cebbda07 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 24 Sep 2024 10:48:23 -0500 Subject: [PATCH 023/198] tests --- src/registrar/tests/test_views_domain.py | 49 +++++++++++++++--------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 109b80913..43ea9bdd4 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -1978,36 +1978,47 @@ class TestDomainChangeNotifications(TestDomainOverview): AllowedEmail(email="info@example.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) + + def setUp(self): + super().setUp() + self.mock_client_class = MagicMock() + self.mock_client = self.mock_client_class.return_value @classmethod def tearDownClass(cls): super().tearDownClass() AllowedEmail.objects.all().delete() + @boto3_mocking.patching def test_notification_email_sent_on_org_name_change(self): """Test that an email is sent when the organization name is changed.""" - with patch('registrar.utility.email.boto3.client') as mock_boto3_client: - mock_ses_client = mock_boto3_client.return_value - self.domain_information.organization_name = "Town of Igorville" - self.domain_information.save() - - org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) - session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + self.domain_information.organization_name = "Town of Igorville" + self.domain_information.save() + + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] - org_name_page.form["organization_name"] = "Not igorville" + org_name_page.form["organization_name"] = "Not igorville" - self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): success_result_page = org_name_page.form.submit() # Check that the page loads successfully - self.assertEqual(success_result_page.status_code, 200) - self.assertContains(success_result_page, "Not igorville") + self.assertEqual(success_result_page.status_code, 200) + self.assertContains(success_result_page, "Not igorville") - # Check that an email was sent - mock_ses_client.send_email.assert_called_once() - - # Check email content - call_kwargs = mock_ses_client.send_email.call_args[1] - self.assertEqual(call_kwargs['FromEmailAddress'], settings.DEFAULT_FROM_EMAIL) - self.assertIn('DOMAIN: Igorville.gov', call_kwargs['Content']['Simple']['Subject']['Data']) - self.assertIn('INFORMATION UPDATED: Org Name/Address', call_kwargs['Content']['Simple']['Body']['Text']['Data']) \ No newline at end of file + # Check that an email was sent + self.assertTrue(self.mock_client.send_email.called) + # Check email content + # check the call sequence for the email + args, kwargs = self.mock_client.send_email.call_args + self.assertIn("Content", kwargs) + self.assertIn("Simple", kwargs["Content"]) + self.assertIn("Subject", kwargs["Content"]["Simple"]) + self.assertIn("Body", kwargs["Content"]["Simple"]) + + # check for things in the email content (not an exhaustive list) + body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] + + self.assertIn("DOMAIN:", body) From ab4024bab509b54670f413b3945d18c0cd36cfab Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 25 Sep 2024 15:24:21 -0500 Subject: [PATCH 024/198] all tests passing --- src/registrar/templates/domain_users.html | 6 +- src/registrar/tests/test_emails.py | 2 +- src/registrar/tests/test_views_domain.py | 231 +++++++++++++++++++++- src/registrar/views/domain.py | 18 +- 4 files changed, 237 insertions(+), 20 deletions(-) diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 412f4ee73..1b789e590 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -8,8 +8,7 @@

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, security email and DNS name servers.

    @@ -17,7 +16,8 @@
  • 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.
  • -
  • 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.
  • +
  • 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.
{% if domain.permissions %} diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 87e2d551f..21ba06316 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -71,7 +71,7 @@ class TestEmails(TestCase): "doesnotexist@igorville.com", context={"domain_request": self}, bcc_address=None, - cc=["test_email1@example.com", "test_email2@example.com"] + cc_addresses=["test_email1@example.com", "test_email2@example.com"] ) # check that an email was sent diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 43ea9bdd4..d258dc472 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -8,6 +8,7 @@ from django.contrib.auth import get_user_model from waffle.testutils import override_flag from api.tests.common import less_console_noise_decorator from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices +from registrar.utility.email import send_templated_email from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore @@ -67,6 +68,10 @@ class TestWithDomainPermissions(TestWithUser): datetime.combine(date.today() + timedelta(days=1), datetime.min.time()) ), ) + self.domain_dns_needed, _ = Domain.objects.get_or_create( + name="dns-needed.gov", + state=Domain.State.DNS_NEEDED, + ) self.domain_deleted, _ = Domain.objects.get_or_create( name="deleted.gov", state=Domain.State.DELETED, @@ -85,6 +90,12 @@ class TestWithDomainPermissions(TestWithUser): self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) + self.security_contact, _ = PublicContact.objects.get_or_create( + domain=self.domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY, + email="security@igorville.gov", + ) + DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dsdata) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_multdsdata) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dnssec_none) @@ -93,6 +104,8 @@ class TestWithDomainPermissions(TestWithUser): DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_just_nameserver) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_on_hold) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_deleted) + DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dns_needed) + self.role, _ = UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER @@ -101,6 +114,9 @@ class TestWithDomainPermissions(TestWithUser): UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain_dsdata, role=UserDomainRole.Roles.MANAGER ) + UserDomainRole.objects.get_or_create( + user=self.user, domain=self.domain_dns_needed, role=UserDomainRole.Roles.MANAGER + ) UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain_multdsdata, @@ -1976,6 +1992,7 @@ class TestDomainChangeNotifications(TestDomainOverview): super().setUpClass() allowed_emails = [ AllowedEmail(email="info@example.com"), + AllowedEmail(email="doesnotexist@igorville.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) @@ -1990,10 +2007,15 @@ class TestDomainChangeNotifications(TestDomainOverview): AllowedEmail.objects.all().delete() @boto3_mocking.patching - def test_notification_email_sent_on_org_name_change(self): + @less_console_noise_decorator + def test_notification_on_org_name_change(self): """Test that an email is sent when the organization name is changed.""" self.domain_information.organization_name = "Town of Igorville" + self.domain_information.address_line1 = "123 Main St" + self.domain_information.city = "Igorville" + self.domain_information.state_territory = "IL" + self.domain_information.zipcode = "62052" self.domain_information.save() org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) @@ -2003,22 +2025,215 @@ class TestDomainChangeNotifications(TestDomainOverview): self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): - success_result_page = org_name_page.form.submit() - # Check that the page loads successfully - self.assertEqual(success_result_page.status_code, 200) - self.assertContains(success_result_page, "Not igorville") + org_name_page.form.submit() # Check that an email was sent self.assertTrue(self.mock_client.send_email.called) + # Check email content # check the call sequence for the email - args, kwargs = self.mock_client.send_email.call_args + _, kwargs = self.mock_client.send_email.call_args self.assertIn("Content", kwargs) self.assertIn("Simple", kwargs["Content"]) self.assertIn("Subject", kwargs["Content"]["Simple"]) self.assertIn("Body", kwargs["Content"]["Simple"]) - # check for things in the email content (not an exhaustive list) body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] - self.assertIn("DOMAIN:", body) + self.assertIn("DOMAIN: igorville.gov", body) + self.assertIn("UPDATED BY: First Last info@example.com", body) + self.assertIn("INFORMATION UPDATED: Org Name/Address", body) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_no_notification_on_org_name_change_with_portfolio(self): + """Test that an email is not sent on org name change when the domain is in a portfolio""" + + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user) + + self.domain_information.organization_name = "Town of Igorville" + self.domain_information.address_line1 = "123 Main St" + self.domain_information.city = "Igorville" + self.domain_information.state_territory = "IL" + self.domain_information.zipcode = "62052" + self.domain_information.portfolio = portfolio + self.domain_information.save() + + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + org_name_page.form["organization_name"] = "Not igorville" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + org_name_page.form.submit() + + # Check that an email was not sent + self.assertFalse(self.mock_client.send_email.called) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_notification_on_security_email_change(self): + """Test that an email is sent when the security email is changed.""" + + security_email_page = self.app.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + security_email_page.form["security_email"] = "new_security@example.com" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + security_email_page.form.submit() + + self.assertTrue(self.mock_client.send_email.called) + + _, kwargs = self.mock_client.send_email.call_args + body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] + + self.assertIn("DOMAIN: igorville.gov", body) + self.assertIn("UPDATED BY: First Last info@example.com", body) + self.assertIn("INFORMATION UPDATED: Security Email", body) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_notification_on_dnssec_enable(self): + """Test that an email is sent when DNSSEC is enabled.""" + + page = self.client.get(reverse("domain-dns-dnssec", kwargs={"pk": self.domain_multdsdata.id})) + self.assertContains(page, "Disable DNSSEC") + + # Prepare the data for the POST request + post_data = { + "disable_dnssec": "Disable DNSSEC", + } + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + updated_page = self.client.post( + reverse("domain-dns-dnssec", kwargs={"pk": self.domain.id}), + post_data, + follow=True, + ) + + self.assertEqual(updated_page.status_code, 200) + + self.assertContains(updated_page, "Enable DNSSEC") + + self.assertTrue(self.mock_client.send_email.called) + + _, kwargs = self.mock_client.send_email.call_args + body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] + + self.assertIn("DOMAIN: igorville.gov", body) + self.assertIn("UPDATED BY: First Last info@example.com", body) + self.assertIn("INFORMATION UPDATED: DNSSec", body) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_notification_on_ds_data_change(self): + """Test that an email is sent when DS data is changed.""" + + ds_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + # Add DS data + ds_data_page.forms[0]["form-0-key_tag"] = "12345" + ds_data_page.forms[0]["form-0-algorithm"] = "13" + ds_data_page.forms[0]["form-0-digest_type"] = "2" + ds_data_page.forms[0]["form-0-digest"] = "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + ds_data_page.forms[0].submit() + + # check that the email was sent + self.assertTrue(self.mock_client.send_email.called) + + # check some stuff about the email + _, kwargs = self.mock_client.send_email.call_args + body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] + + self.assertIn("DOMAIN: igorville.gov", body) + self.assertIn("UPDATED BY: First Last info@example.com", body) + self.assertIn("INFORMATION UPDATED: DS Data", body) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_notification_on_senior_official_change(self): + """Test that an email is sent when the senior official information is changed.""" + + self.domain_information.senior_official = Contact.objects.create( + first_name="Old", last_name="Official", title="Manager", email="old_official@example.com" + ) + self.domain_information.save() + + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + senior_official_page.form["first_name"] = "New" + senior_official_page.form["last_name"] = "Official" + senior_official_page.form["title"] = "Director" + senior_official_page.form["email"] = "new_official@example.com" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + senior_official_page.form.submit() + + self.assertTrue(self.mock_client.send_email.called) + + _, kwargs = self.mock_client.send_email.call_args + body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] + + self.assertIn("DOMAIN: igorville.gov", body) + self.assertIn("UPDATED BY: First Last info@example.com", body) + self.assertIn("INFORMATION UPDATED: Senior Official", body) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_no_notification_on_senior_official_when_portfolio(self): + """Test that an email is not sent when the senior official information is changed + and the domain is in a portfolio.""" + + self.domain_information.senior_official = Contact.objects.create( + first_name="Old", last_name="Official", title="Manager", email="old_official@example.com" + ) + portfolio, _ =Portfolio.objects.get_or_create( + organization_name="portfolio", + creator=self.user, + ) + self.domain_information.portfolio = portfolio + self.domain_information.save() + + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + senior_official_page.form["first_name"] = "New" + senior_official_page.form["last_name"] = "Official" + senior_official_page.form["title"] = "Director" + senior_official_page.form["email"] = "new_official@example.com" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + senior_official_page.form.submit() + + self.assertFalse(self.mock_client.send_email.called) + + @boto3_mocking.patching + @less_console_noise_decorator + def test_no_notification_when_dns_needed(self): + """Test that an email is not sent when nameservers are changed while the state is DNS_NEEDED.""" + + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain_dns_needed.id})) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + # add nameservers + nameservers_page.form["form-0-server"] = "ns1-new.igorville.gov" + nameservers_page.form["form-0-ip"] = "192.168.1.1" + nameservers_page.form["form-1-server"] = "ns2-new.igorville.gov" + nameservers_page.form["form-1-ip"] = "192.168.1.2" + + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + nameservers_page.form.submit() + + # Check that an email was not sent + self.assertFalse(self.mock_client.send_email.called) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 04b740f6e..22ece989b 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -119,6 +119,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): if form.is_valid(): return self.form_valid(form) else: + logger.debug(f"Form errors: {form.errors}") return self.form_invalid(form) def form_valid(self, form): @@ -164,6 +165,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): DomainDsdataFormset: "DS Data", DomainOrgNameAddressForm: "Org Name/Address", SeniorOfficialContactForm: "Senior Official", + NameserverFormset: "Nameservers", } # forms of these types should not send notifications if they're part of a portfolio/Organization @@ -179,14 +181,13 @@ class DomainFormBaseView(DomainBaseView, FormMixin): # some forms shouldn't cause notifications if they are in a portfolio info = self.get_domain_info_from_domain() if not info or info.portfolio: + logger.info(f"Not notifying because of portfolio") should_notify = False else: # don't notify for any other types of forms should_notify=False - + logger.info(f"Not notifying for {form.__class__}") if (should_notify and form.has_changed()) or force_send: - logger.info("Sending email to domain managers") - context={ "domain": self.object.name, "user": self.request.user, @@ -194,6 +195,8 @@ class DomainFormBaseView(DomainBaseView, FormMixin): "changes": form_label_dict[form.__class__] } self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) + else: + logger.info(f"Not notifying for {form.__class__}, form changes: {form.has_changed()}, force_send: {force_send}") def email_domain_managers(self, domain_name, template: str, subject_template: str, context: any = {}): """Send a single email built from a template to all managers for a given domain. @@ -489,8 +492,7 @@ class DomainNameserversView(DomainFormBaseView): This post method harmonizes using DomainBaseView and FormMixin """ - - logger.info("Posted to Namservers View") + logger.info(f"POST request to DomainNameserversView") self._get_domain(request) formset = self.get_form() @@ -500,6 +502,7 @@ class DomainNameserversView(DomainFormBaseView): return HttpResponseRedirect(url) if formset.is_valid(): + logger.info(f"Formset is valid") return self.form_valid(formset) else: return self.form_invalid(formset) @@ -507,8 +510,6 @@ class DomainNameserversView(DomainFormBaseView): def form_valid(self, formset): """The formset is valid, perform something with it.""" - logger.info("------ Nameserver Form is valid -------") - self.request.session["nameservers_form_domain"] = self.object # Set the nameservers from the formset @@ -550,7 +551,8 @@ class DomainNameserversView(DomainFormBaseView): messages.error(self.request, NameserverError(code=nsErrorCodes.BAD_DATA)) logger.error(f"Registry error: {Err}") else: - self.send_update_notification(formset) + if self.object.state == Domain.State.READY: + self.send_update_notification(formset) messages.success( self.request, "The name servers for this domain have been updated. " From 43b48edf3639c22bd3a29439c1a23636671a8296 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 25 Sep 2024 15:31:27 -0500 Subject: [PATCH 025/198] linter errors --- src/registrar/config/settings.py | 2 +- src/registrar/tests/test_emails.py | 18 +++---- src/registrar/tests/test_views_domain.py | 60 +++++++++++----------- src/registrar/utility/email.py | 17 ++++--- src/registrar/views/domain.py | 63 ++++++++++++------------ 5 files changed, 78 insertions(+), 82 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 1b20caf2a..c5d4fa95d 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -478,7 +478,7 @@ class JsonServerFormatter(ServerFormatter): if not hasattr(record, "server_time"): record.server_time = self.formatTime(record, self.datefmt) - + log_entry = {"server_time": record.server_time, "level": record.levelname, "message": formatted_record} return json.dumps(log_entry) diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 21ba06316..abbbb274f 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -60,20 +60,20 @@ class TestEmails(TestCase): # Assert that an email wasn't sent self.assertFalse(self.mock_client.send_email.called) - + @boto3_mocking.patching def test_email_with_cc(self): """Test sending email with cc works""" with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): send_templated_email( - "test content", - "test subject", - "doesnotexist@igorville.com", - context={"domain_request": self}, - bcc_address=None, - cc_addresses=["test_email1@example.com", "test_email2@example.com"] - ) - + "test content", + "test subject", + "doesnotexist@igorville.com", + context={"domain_request": self}, + bcc_address=None, + cc_addresses=["test_email1@example.com", "test_email2@example.com"], + ) + # check that an email was sent self.assertTrue(self.mock_client.send_email.called) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index d258dc472..6ca96f8c0 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -2,17 +2,14 @@ from unittest import skip from unittest.mock import MagicMock, ANY, patch from django.conf import settings -from django.test import override_settings from django.urls import reverse from django.contrib.auth import get_user_model from waffle.testutils import override_flag from api.tests.common import less_console_noise_decorator from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices -from registrar.utility.email import send_templated_email from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore -from django.middleware.csrf import get_token from registrar.utility.errors import ( NameserverError, @@ -106,7 +103,6 @@ class TestWithDomainPermissions(TestWithUser): DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_deleted) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dns_needed) - self.role, _ = UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER ) @@ -1995,7 +1991,7 @@ class TestDomainChangeNotifications(TestDomainOverview): AllowedEmail(email="doesnotexist@igorville.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) - + def setUp(self): super().setUp() self.mock_client_class = MagicMock() @@ -2010,14 +2006,14 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_notification_on_org_name_change(self): """Test that an email is sent when the organization name is changed.""" - + self.domain_information.organization_name = "Town of Igorville" self.domain_information.address_line1 = "123 Main St" self.domain_information.city = "Igorville" self.domain_information.state_territory = "IL" self.domain_information.zipcode = "62052" self.domain_information.save() - + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2028,8 +2024,8 @@ class TestDomainChangeNotifications(TestDomainOverview): org_name_page.form.submit() # Check that an email was sent - self.assertTrue(self.mock_client.send_email.called) - + self.assertTrue(self.mock_client.send_email.called) + # Check email content # check the call sequence for the email _, kwargs = self.mock_client.send_email.call_args @@ -2048,7 +2044,7 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_no_notification_on_org_name_change_with_portfolio(self): """Test that an email is not sent on org name change when the domain is in a portfolio""" - + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user) self.domain_information.organization_name = "Town of Igorville" @@ -2058,7 +2054,7 @@ class TestDomainChangeNotifications(TestDomainOverview): self.domain_information.zipcode = "62052" self.domain_information.portfolio = portfolio self.domain_information.save() - + org_name_page = self.app.get(reverse("domain-org-name-address", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2069,13 +2065,13 @@ class TestDomainChangeNotifications(TestDomainOverview): org_name_page.form.submit() # Check that an email was not sent - self.assertFalse(self.mock_client.send_email.called) + self.assertFalse(self.mock_client.send_email.called) @boto3_mocking.patching @less_console_noise_decorator def test_notification_on_security_email_change(self): """Test that an email is sent when the security email is changed.""" - + security_email_page = self.app.get(reverse("domain-security-email", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2085,8 +2081,8 @@ class TestDomainChangeNotifications(TestDomainOverview): with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): security_email_page.form.submit() - self.assertTrue(self.mock_client.send_email.called) - + self.assertTrue(self.mock_client.send_email.called) + _, kwargs = self.mock_client.send_email.call_args body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] @@ -2098,7 +2094,7 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_notification_on_dnssec_enable(self): """Test that an email is sent when DNSSEC is enabled.""" - + page = self.client.get(reverse("domain-dns-dnssec", kwargs={"pk": self.domain_multdsdata.id})) self.assertContains(page, "Disable DNSSEC") @@ -2118,8 +2114,8 @@ class TestDomainChangeNotifications(TestDomainOverview): self.assertContains(updated_page, "Enable DNSSEC") - self.assertTrue(self.mock_client.send_email.called) - + self.assertTrue(self.mock_client.send_email.called) + _, kwargs = self.mock_client.send_email.call_args body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] @@ -2131,7 +2127,7 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_notification_on_ds_data_change(self): """Test that an email is sent when DS data is changed.""" - + ds_data_page = self.app.get(reverse("domain-dns-dnssec-dsdata", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2140,14 +2136,14 @@ class TestDomainChangeNotifications(TestDomainOverview): ds_data_page.forms[0]["form-0-algorithm"] = "13" ds_data_page.forms[0]["form-0-digest_type"] = "2" ds_data_page.forms[0]["form-0-digest"] = "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF" - + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): ds_data_page.forms[0].submit() # check that the email was sent - self.assertTrue(self.mock_client.send_email.called) - + self.assertTrue(self.mock_client.send_email.called) + # check some stuff about the email _, kwargs = self.mock_client.send_email.call_args body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] @@ -2160,12 +2156,12 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_notification_on_senior_official_change(self): """Test that an email is sent when the senior official information is changed.""" - + self.domain_information.senior_official = Contact.objects.create( first_name="Old", last_name="Official", title="Manager", email="old_official@example.com" ) self.domain_information.save() - + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2178,8 +2174,8 @@ class TestDomainChangeNotifications(TestDomainOverview): with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): senior_official_page.form.submit() - self.assertTrue(self.mock_client.send_email.called) - + self.assertTrue(self.mock_client.send_email.called) + _, kwargs = self.mock_client.send_email.call_args body = kwargs["Content"]["Simple"]["Body"]["Text"]["Data"] @@ -2192,17 +2188,17 @@ class TestDomainChangeNotifications(TestDomainOverview): def test_no_notification_on_senior_official_when_portfolio(self): """Test that an email is not sent when the senior official information is changed and the domain is in a portfolio.""" - + self.domain_information.senior_official = Contact.objects.create( first_name="Old", last_name="Official", title="Manager", email="old_official@example.com" - ) - portfolio, _ =Portfolio.objects.get_or_create( + ) + portfolio, _ = Portfolio.objects.get_or_create( organization_name="portfolio", creator=self.user, ) self.domain_information.portfolio = portfolio self.domain_information.save() - + senior_official_page = self.app.get(reverse("domain-senior-official", kwargs={"pk": self.domain.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] @@ -2216,12 +2212,12 @@ class TestDomainChangeNotifications(TestDomainOverview): senior_official_page.form.submit() self.assertFalse(self.mock_client.send_email.called) - + @boto3_mocking.patching @less_console_noise_decorator def test_no_notification_when_dns_needed(self): """Test that an email is not sent when nameservers are changed while the state is DNS_NEEDED.""" - + nameservers_page = self.app.get(reverse("domain-dns-nameservers", kwargs={"pk": self.domain_dns_needed.id})) session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 35d8a2029..655c432ac 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -22,15 +22,15 @@ class EmailSendingError(RuntimeError): pass -def send_templated_email( +def send_templated_email( # noqa template_name: str, subject_template_name: str, - to_address: str="", - bcc_address: str="", + to_address: str = "", + bcc_address: str = "", context={}, attachment_file=None, wrap_email=False, - cc_addresses: list[str]=[], + cc_addresses: list[str] = [], ): """Send an email built from a template. @@ -58,7 +58,6 @@ def send_templated_email( if len(sendable_cc_addresses) < len(cc_addresses): logger.warning("Some CC'ed addresses were removed: %s.", blocked_cc_addresses) - template = get_template(template_name) email_body = template.render(context=context) @@ -127,6 +126,7 @@ def send_templated_email( except Exception as exc: raise EmailSendingError("Could not send SES email.") from exc + def _can_send_email(to_address, bcc_address): """Raises an EmailSendingError if we cannot send an email. Does nothing otherwise.""" @@ -144,15 +144,16 @@ def _can_send_email(to_address, bcc_address): if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): raise EmailSendingError(message.format(bcc_address)) + def get_sendable_addresses(addresses: list[str]) -> tuple[list[str], list[str]]: """Checks whether a list of addresses can be sent to. - + Returns: a lists of all provided addresses that are ok to send to and a list of addresses that were blocked. Paramaters: - + addresses: a list of strings representing all addresses to be checked. - + raises: EmailSendingError if email sending is disabled """ diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 22ece989b..a3f9d153c 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -149,18 +149,18 @@ class DomainFormBaseView(DomainBaseView, FormMixin): logger.error("Could get domain_info. No domain info exists, or duplicates exist.") return current_domain_info - + def send_update_notification(self, form, force_send=False): - """Send a notification to all domain managers that an update has occured + """Send a notification to all domain managers that an update has occured for a single domain. Uses update_to_approved_domain.txt template. - + If there are no changes to the form, emails will NOT be sent unless force_send is set to True. """ # send notification email for changes to any of these forms form_label_dict = { - DomainSecurityEmailForm: "Security Email", + DomainSecurityEmailForm: "Security Email", DomainDnssecForm: "DNSSec", DomainDsdataFormset: "DS Data", DomainOrgNameAddressForm: "Org Name/Address", @@ -176,29 +176,35 @@ class DomainFormBaseView(DomainBaseView, FormMixin): if form.__class__ in form_label_dict: # these types of forms can cause notifications - should_notify=True + should_notify = True if form.__class__ in check_for_portfolio: # some forms shouldn't cause notifications if they are in a portfolio info = self.get_domain_info_from_domain() if not info or info.portfolio: - logger.info(f"Not notifying because of portfolio") - should_notify = False + should_notify = False else: # don't notify for any other types of forms - should_notify=False + should_notify = False logger.info(f"Not notifying for {form.__class__}") if (should_notify and form.has_changed()) or force_send: - context={ - "domain": self.object.name, - "user": self.request.user, - "date": date.today(), - "changes": form_label_dict[form.__class__] - } - self.email_domain_managers(self.object, "emails/update_to_approved_domain.txt", "emails/update_to_approved_domain_subject.txt", context) + context = { + "domain": self.object.name, + "user": self.request.user, + "date": date.today(), + "changes": form_label_dict[form.__class__], + } + self.email_domain_managers( + self.object, + "emails/update_to_approved_domain.txt", + "emails/update_to_approved_domain_subject.txt", + context, + ) else: - logger.info(f"Not notifying for {form.__class__}, form changes: {form.has_changed()}, force_send: {force_send}") + logger.info( + f"Not notifying for {form.__class__}, form changes: {form.has_changed()}, force_send: {force_send}" + ) - def email_domain_managers(self, domain_name, template: str, subject_template: str, context: any = {}): + def email_domain_managers(self, domain_name, template: str, subject_template: str, context={}): """Send a single email built from a template to all managers for a given domain. template_name and subject_template_name are relative to the same template @@ -214,20 +220,16 @@ class DomainFormBaseView(DomainBaseView, FormMixin): domain = Domain.objects.get(name=domain_name) except Domain.DoesNotExist: logger.warning( - "Could not send notification email for domain %s, unable to find matching domain object", - domain_name + "Could not send notification email for domain %s, unable to find matching domain object", domain_name ) - manager_pks = UserDomainRole.objects.filter(domain=domain.pk, role=UserDomainRole.Roles.MANAGER).values_list("user", flat=True) + manager_pks = UserDomainRole.objects.filter(domain=domain.pk, role=UserDomainRole.Roles.MANAGER).values_list( + "user", flat=True + ) emails = list(User.objects.filter(pk__in=manager_pks).values_list("email", flat=True)) logger.debug("attempting to send templated email to domain managers") try: - send_templated_email( - template, - subject_template, - context=context, - cc_addresses=emails - ) - except EmailSendingError as exc: + send_templated_email(template, subject_template, context=context, cc_addresses=emails) + except EmailSendingError: logger.warning( "Could not sent notification email to %s for domain %s", emails, @@ -492,8 +494,6 @@ class DomainNameserversView(DomainFormBaseView): This post method harmonizes using DomainBaseView and FormMixin """ - logger.info(f"POST request to DomainNameserversView") - self._get_domain(request) formset = self.get_form() @@ -502,14 +502,13 @@ class DomainNameserversView(DomainFormBaseView): return HttpResponseRedirect(url) if formset.is_valid(): - logger.info(f"Formset is valid") return self.form_valid(formset) else: return self.form_invalid(formset) def form_valid(self, formset): """The formset is valid, perform something with it.""" - + self.request.session["nameservers_form_domain"] = self.object # Set the nameservers from the formset @@ -800,7 +799,7 @@ class DomainSecurityEmailView(DomainFormBaseView): else: self.send_update_notification(form) messages.success(self.request, "The security email for this domain has been updated.") - + # superclass has the redirect return super().form_valid(form) From 7a4ed7ab059e6ae6688fcdc84617b7852557b46e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 25 Sep 2024 15:44:49 -0500 Subject: [PATCH 026/198] minor email refactor --- src/registrar/utility/email.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 655c432ac..fc7d4f956 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -76,7 +76,7 @@ def send_templated_email( # noqa aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, config=settings.BOTO_CONFIG, ) - logger.info(f"An email was sent! Template name: {template_name} to {to_address}") + logger.info(f"Connected to SES client! Template name: {template_name} to {to_address}") except Exception as exc: logger.debug("E-mail unable to send! Could not access the SES client.") raise EmailSendingError("Could not access the SES client.") from exc @@ -153,14 +153,12 @@ def get_sendable_addresses(addresses: list[str]) -> tuple[list[str], list[str]]: Paramaters: addresses: a list of strings representing all addresses to be checked. - - raises: - EmailSendingError if email sending is disabled """ if flag_is_active(None, "disable_email_sending"): # type: ignore message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." - raise EmailSendingError(message) + logger.warning(message) + return ([],[]) else: AllowedEmail = apps.get_model("registrar", "AllowedEmail") allowed_emails = [] From 0c9db26a575fcae2ec5f9b239e5bd69ebe66e33f Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 25 Sep 2024 16:05:12 -0500 Subject: [PATCH 027/198] fix email test --- src/registrar/tests/test_emails.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index abbbb274f..c3a84d22f 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -66,10 +66,10 @@ class TestEmails(TestCase): """Test sending email with cc works""" with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): send_templated_email( - "test content", - "test subject", + "emails/update_to_approved_domain.txt", + "emails/update_to_approved_domain_subject.txt", "doesnotexist@igorville.com", - context={"domain_request": self}, + context={"domain": "test", "user": "test", "date": 1, "changes": "test"}, bcc_address=None, cc_addresses=["test_email1@example.com", "test_email2@example.com"], ) From 2e3ff9e547a11300b8719d8bf5bf442b7620ec87 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 26 Sep 2024 10:58:39 -0500 Subject: [PATCH 028/198] fix tests --- src/registrar/models/domain_request.py | 6 ++-- src/registrar/tests/test_emails.py | 46 +++++++++++++++++------- src/registrar/tests/test_models.py | 8 ++--- src/registrar/tests/test_views_domain.py | 2 ++ src/registrar/utility/email.py | 6 ++-- src/registrar/views/domain.py | 2 +- 6 files changed, 48 insertions(+), 22 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 161d85ae5..fdba309f3 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -781,7 +781,7 @@ class DomainRequest(TimeStampedModel): if custom_email_content: context["custom_email_content"] = custom_email_content - + logger.info(f"Sending email to: {recipient.email}") send_templated_email( email_template, email_template_subject, @@ -823,11 +823,12 @@ class DomainRequest(TimeStampedModel): # requested_domain could be None here if not hasattr(self, "requested_domain") or self.requested_domain is None: raise ValueError("Requested domain is missing.") + logger.info(f"Submitting domain request: {self.requested_domain.name}") DraftDomain = apps.get_model("registrar.DraftDomain") if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - + logger.info(f"Draft Domain") # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() @@ -835,6 +836,7 @@ class DomainRequest(TimeStampedModel): # Update last_submitted_date to today self.last_submitted_date = timezone.now().date() self.save() + logger.info(f"updated submission date") # Limit email notifications to transitions from Started and Withdrawn limited_statuses = [self.DomainRequestStatus.STARTED, self.DomainRequestStatus.WITHDRAWN] diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index c3a84d22f..3b1b45e98 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -71,7 +71,7 @@ class TestEmails(TestCase): "doesnotexist@igorville.com", context={"domain": "test", "user": "test", "date": 1, "changes": "test"}, bcc_address=None, - cc_addresses=["test_email1@example.com", "test_email2@example.com"], + cc_addresses=["testy2@town.com", "mayor@igorville.gov"], ) # check that an email was sent @@ -81,7 +81,7 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation(self): """Submission confirmation email works.""" - domain_request = completed_domain_request() + domain_request = completed_domain_request(user=User.objects.create(username="test", email="testy@town.com")) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() @@ -118,7 +118,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_no_current_website_spacing(self): """Test line spacing without current_website.""" - domain_request = completed_domain_request(has_current_website=False) + domain_request = completed_domain_request( + has_current_website=False, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -131,7 +133,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_current_website_spacing(self): """Test line spacing with current_website.""" - domain_request = completed_domain_request(has_current_website=True) + domain_request = completed_domain_request( + has_current_website=True, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -148,7 +152,11 @@ class TestEmails(TestCase): # Create fake creator _creator = User.objects.create( - username="MrMeoward", first_name="Meoward", last_name="Jones", phone="(888) 888 8888" + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + phone="(888) 888 8888", + email="testy@town.com", ) # Create a fake domain request @@ -165,7 +173,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_no_other_contacts_spacing(self): """Test line spacing without other contacts.""" - domain_request = completed_domain_request(has_other_contacts=False) + domain_request = completed_domain_request( + has_other_contacts=False, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -177,7 +187,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_alternative_govdomain_spacing(self): """Test line spacing with alternative .gov domain.""" - domain_request = completed_domain_request(has_alternative_gov_domain=True) + domain_request = completed_domain_request( + has_alternative_gov_domain=True, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -190,7 +202,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_no_alternative_govdomain_spacing(self): """Test line spacing without alternative .gov domain.""" - domain_request = completed_domain_request(has_alternative_gov_domain=False) + domain_request = completed_domain_request( + has_alternative_gov_domain=False, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -203,7 +217,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_about_your_organization_spacing(self): """Test line spacing with about your organization.""" - domain_request = completed_domain_request(has_about_your_organization=True) + domain_request = completed_domain_request( + has_about_your_organization=True, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -216,7 +232,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_no_about_your_organization_spacing(self): """Test line spacing without about your organization.""" - domain_request = completed_domain_request(has_about_your_organization=False) + domain_request = completed_domain_request( + has_about_your_organization=False, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -229,7 +247,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_anything_else_spacing(self): """Test line spacing with anything else.""" - domain_request = completed_domain_request(has_anything_else=True) + domain_request = completed_domain_request( + has_anything_else=True, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -241,7 +261,9 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_no_anything_else_spacing(self): """Test line spacing without anything else.""" - domain_request = completed_domain_request(has_anything_else=False) + domain_request = completed_domain_request( + has_anything_else=False, user=User.objects.create(username="test", email="testy@town.com") + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index a6e889503..8c9a888c2 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -268,7 +268,7 @@ class TestDomainRequest(TestCase): @less_console_noise_decorator def test_submit_from_withdrawn_sends_email(self): msg = "Create a withdrawn domain request and submit it and see if email was sent." - user, _ = User.objects.get_or_create(username="testy") + user, _ = User.objects.get_or_create(username="testy", email="testy@town.com") domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.WITHDRAWN, user=user) self.check_email_sent(domain_request, msg, "submit", 1, expected_content="Hi", expected_email=user.email) @@ -287,14 +287,14 @@ class TestDomainRequest(TestCase): @less_console_noise_decorator def test_approve_sends_email(self): msg = "Create a domain request and approve it and see if email was sent." - user, _ = User.objects.get_or_create(username="testy") + user, _ = User.objects.get_or_create(username="testy", email="testy@town.com") domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=user) self.check_email_sent(domain_request, msg, "approve", 1, expected_content="approved", expected_email=user.email) @less_console_noise_decorator def test_withdraw_sends_email(self): msg = "Create a domain request and withdraw it and see if email was sent." - user, _ = User.objects.get_or_create(username="testy") + user, _ = User.objects.get_or_create(username="testy", email="testy@town.com") domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=user) self.check_email_sent( domain_request, msg, "withdraw", 1, expected_content="withdrawn", expected_email=user.email @@ -303,7 +303,7 @@ class TestDomainRequest(TestCase): @less_console_noise_decorator def test_reject_sends_email(self): msg = "Create a domain request and reject it and see if email was sent." - user, _ = User.objects.get_or_create(username="testy") + user, _ = User.objects.get_or_create(username="testy", email="testy@town.com") domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.APPROVED, user=user) self.check_email_sent(domain_request, msg, "reject", 1, expected_content="Hi", expected_email=user.email) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 6ca96f8c0..7e9f5b9b8 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -250,6 +250,7 @@ class TestDomainDetail(TestDomainOverview): # At the time of this test's writing, there are 6 UNKNOWN domains inherited # from constructors. Let's reset. with less_console_noise(): + PublicContact.objects.all().delete() Domain.objects.all().delete() UserDomainRole.objects.all().delete() @@ -2002,6 +2003,7 @@ class TestDomainChangeNotifications(TestDomainOverview): super().tearDownClass() AllowedEmail.objects.all().delete() + @boto3_mocking.patching @less_console_noise_decorator def test_notification_on_org_name_change(self): diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index fc7d4f956..412838d10 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -34,7 +34,7 @@ def send_templated_email( # noqa ): """Send an email built from a template. - to_address and bcc_address currently only supports a single address. + to_address and bcc_address currently only support single addresses. cc_address is a list and can contain many addresses. Emails not in the whitelist (if applicable) will be filtered out before sending. @@ -111,7 +111,7 @@ def send_templated_email( # noqa }, }, ) - logger.info("Email sent to %s, bcc %s, cc %s", to_address, bcc_address, cc_addresses) + logger.info("Email sent to %s, bcc %s, cc %s", to_address, bcc_address, sendable_cc_addresses) else: ses_client = boto3.client( "ses", @@ -158,7 +158,7 @@ def get_sendable_addresses(addresses: list[str]) -> tuple[list[str], list[str]]: if flag_is_active(None, "disable_email_sending"): # type: ignore message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." logger.warning(message) - return ([],[]) + return ([], []) else: AllowedEmail = apps.get_model("registrar", "AllowedEmail") allowed_emails = [] diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index a3f9d153c..9f6662291 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -413,7 +413,7 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Set the domain information in the form so that it can be accessible # to associate a new Contact, if a new Contact is needed - # in the save() methodS + # in the save() method form.set_domain_info(self.object.domain_info) form.save() From 7d9eabb7271be811b79874e8421859592bb4d03b Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 26 Sep 2024 11:20:18 -0500 Subject: [PATCH 029/198] linter errors --- src/registrar/models/domain_request.py | 2 -- src/registrar/tests/test_views_domain.py | 1 - 2 files changed, 3 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index fdba309f3..08a431967 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -828,7 +828,6 @@ class DomainRequest(TimeStampedModel): DraftDomain = apps.get_model("registrar.DraftDomain") if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - logger.info(f"Draft Domain") # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() @@ -836,7 +835,6 @@ class DomainRequest(TimeStampedModel): # Update last_submitted_date to today self.last_submitted_date = timezone.now().date() self.save() - logger.info(f"updated submission date") # Limit email notifications to transitions from Started and Withdrawn limited_statuses = [self.DomainRequestStatus.STARTED, self.DomainRequestStatus.WITHDRAWN] diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 7e9f5b9b8..939cdaaf9 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -2003,7 +2003,6 @@ class TestDomainChangeNotifications(TestDomainOverview): super().tearDownClass() AllowedEmail.objects.all().delete() - @boto3_mocking.patching @less_console_noise_decorator def test_notification_on_org_name_change(self): 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 030/198] 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 @@ from registrar.models import ( DomainRequest, DomainInformation, DomainInvitation, + PortfolioInvitation, User, UserDomainRole, + UserPortfolioPermission, PublicContact, ) from registrar.utility.enums import DefaultEmail @@ -38,6 +40,7 @@ from registrar.utility.errors import ( ) 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 @@ class DomainAddUserView(DomainFormBaseView): """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 @@ class DomainAddUserView(DomainFormBaseView): ) 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 031/198] 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 032/198] 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 65d89872f2b49a9ce796a5ec8295342cfce55bac Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 26 Sep 2024 16:43:03 -0500 Subject: [PATCH 033/198] remove extra logging statements --- src/registrar/models/domain_request.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 08a431967..2aecb49f7 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -781,7 +781,6 @@ class DomainRequest(TimeStampedModel): if custom_email_content: context["custom_email_content"] = custom_email_content - logger.info(f"Sending email to: {recipient.email}") send_templated_email( email_template, email_template_subject, @@ -823,7 +822,6 @@ class DomainRequest(TimeStampedModel): # requested_domain could be None here if not hasattr(self, "requested_domain") or self.requested_domain is None: raise ValueError("Requested domain is missing.") - logger.info(f"Submitting domain request: {self.requested_domain.name}") DraftDomain = apps.get_model("registrar.DraftDomain") if not DraftDomain.string_could_be_domain(self.requested_domain.name): 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 034/198] 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 @@ from registrar.utility.errors import ( NameserverErrorCodes as nsErrorCodes, DsDataError, DsDataErrorCodes, + OutsideOrgMemberError, SecurityEmailError, SecurityEmailErrorCodes, ) @@ -781,12 +782,15 @@ class DomainAddUserView(DomainFormBaseView): """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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): "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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 035/198] 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 @@ from registrar.utility.errors import ( 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,10 @@ class DomainAddUserView(DomainFormBaseView): 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", @@ -868,6 +872,29 @@ class DomainAddUserView(DomainFormBaseView): ) 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", + email, + self.object, + exc_info=True, + ) + raise EmailSendingError("Could not send email invitation.") from exc + else: + if add_success: + messages.success(self.request, f"{email} has been invited to this domain.") + def _make_invitation(self, email_address: str, requestor: User): """Make a Domain invitation for this email and redirect with a message.""" try: @@ -896,14 +923,6 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): exc_info=True, ) messages.warning(self.request, "Could not send email invitation.") - except IntegrityError: - messages.warning(self.request, f"{requested_email} is already a manager for this domain") + 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 036/198] 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 @@ class DomainAddUserView(DomainFormBaseView): 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 037/198] 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 @@ class DomainAddUserView(DomainFormBaseView): """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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): 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 @@ class DomainAddUserView(DomainFormBaseView): "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 @@ class DomainAddUserView(DomainFormBaseView): 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 038/198] 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 @@ class DomainAddUserView(DomainFormBaseView): "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 @@ class DomainAddUserView(DomainFormBaseView): 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 9940b1657e983828f937255117537ffb9fb3dacb Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Wed, 2 Oct 2024 11:26:12 -0500 Subject: [PATCH 039/198] Update src/registrar/utility/email.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- src/registrar/utility/email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 412838d10..ecae7ed93 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -91,7 +91,7 @@ def send_templated_email( # noqa # make sure we don't try and send an email to nowhere if not destination: - message = "E-mail unable to send, no valid recipients provided." + message = "Email unable to send, no valid recipients provided." raise EmailSendingError(message) try: From f26db7185b3a1a01822ccaa5bce684b131ab2c43 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 2 Oct 2024 11:54:29 -0500 Subject: [PATCH 040/198] review changes --- src/registrar/tests/test_emails.py | 31 ++++++++++++++++++++++++ src/registrar/tests/test_views_domain.py | 8 ++---- src/registrar/utility/email.py | 13 ++++++++-- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 3b1b45e98..e76a6124f 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -77,6 +77,37 @@ class TestEmails(TestCase): # check that an email was sent self.assertTrue(self.mock_client.send_email.called) + # check the call sequence for the email + args, kwargs = self.mock_client.send_email.call_args + self.assertIn("Destination", kwargs) + self.assertIn("CcAddresses", kwargs["Destination"]) + + self.assertEqual(["testy2@town.com", "mayor@igorville.gov"], kwargs["Destination"]["CcAddresses"]) + + @boto3_mocking.patching + @override_settings(IS_PRODUCTION=True) + def test_email_with_cc_in_prod(self): + """Test sending email with cc works in prod""" + with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): + send_templated_email( + "emails/update_to_approved_domain.txt", + "emails/update_to_approved_domain_subject.txt", + "doesnotexist@igorville.com", + context={"domain": "test", "user": "test", "date": 1, "changes": "test"}, + bcc_address=None, + cc_addresses=["testy2@town.com", "mayor@igorville.gov"], + ) + + # check that an email was sent + self.assertTrue(self.mock_client.send_email.called) + + # check the call sequence for the email + args, kwargs = self.mock_client.send_email.call_args + self.assertIn("Destination", kwargs) + self.assertIn("CcAddresses", kwargs["Destination"]) + + self.assertEqual(["testy2@town.com", "mayor@igorville.gov"], kwargs["Destination"]["CcAddresses"]) + @boto3_mocking.patching @less_console_noise_decorator def test_submission_confirmation(self): diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 939cdaaf9..15e21169e 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -87,12 +87,6 @@ class TestWithDomainPermissions(TestWithUser): self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) - self.security_contact, _ = PublicContact.objects.get_or_create( - domain=self.domain, - contact_type=PublicContact.ContactTypeChoices.SECURITY, - email="security@igorville.gov", - ) - DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dsdata) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_multdsdata) DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain_dnssec_none) @@ -2007,6 +2001,8 @@ class TestDomainChangeNotifications(TestDomainOverview): @less_console_noise_decorator def test_notification_on_org_name_change(self): """Test that an email is sent when the organization name is changed.""" + # We may end up sending emails on org name changes later, but it will be addressed + # in the portfolio itself, rather than the individual domain. self.domain_information.organization_name = "Town of Igorville" self.domain_information.address_line1 = "123 Main St" diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 412838d10..6c5f5f172 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -48,14 +48,19 @@ def send_templated_email( # noqa No valid recipient addresses are provided """ + # by default assume we can send to all addresses (prod has no whitelist) + sendable_cc_addresses = cc_addresses + if not settings.IS_PRODUCTION: # type: ignore # Split into a function: C901 'send_templated_email' is too complex. # Raises an error if we cannot send an email (due to restrictions). # Does nothing otherwise. _can_send_email(to_address, bcc_address) + + # if we're not in prod, we need to check the whitelist for CC'ed addresses sendable_cc_addresses, blocked_cc_addresses = get_sendable_addresses(cc_addresses) - if len(sendable_cc_addresses) < len(cc_addresses): + if blocked_cc_addresses: logger.warning("Some CC'ed addresses were removed: %s.", blocked_cc_addresses) template = get_template(template_name) @@ -111,7 +116,7 @@ def send_templated_email( # noqa }, }, ) - logger.info("Email sent to %s, bcc %s, cc %s", to_address, bcc_address, sendable_cc_addresses) + logger.info("Email sent to [%s], bcc [%s], cc %s", to_address, bcc_address, sendable_cc_addresses) else: ses_client = boto3.client( "ses", @@ -123,6 +128,10 @@ def send_templated_email( # noqa send_email_with_attachment( settings.DEFAULT_FROM_EMAIL, to_address, subject, email_body, attachment_file, ses_client ) + logger.info( + "Email with attachment sent to [%s], bcc [%s], cc %s", to_address, bcc_address, sendable_cc_addresses + ) + except Exception as exc: raise EmailSendingError("Could not send SES email.") from exc From a42ea0a19133ed61af7cc146928d4b0d16a9ead9 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Wed, 2 Oct 2024 15:13:31 -0500 Subject: [PATCH 041/198] Update src/registrar/views/domain.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- src/registrar/views/domain.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 9f6662291..88da7320a 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -226,7 +226,6 @@ class DomainFormBaseView(DomainBaseView, FormMixin): "user", flat=True ) emails = list(User.objects.filter(pk__in=manager_pks).values_list("email", flat=True)) - logger.debug("attempting to send templated email to domain managers") try: send_templated_email(template, subject_template, context=context, cc_addresses=emails) except EmailSendingError: From 50ca20fe576001f924d3c617c100733642687772 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 2 Oct 2024 15:15:59 -0500 Subject: [PATCH 042/198] minor fixes --- src/registrar/views/domain.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 9f6662291..206509e07 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -204,7 +204,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): f"Not notifying for {form.__class__}, form changes: {form.has_changed()}, force_send: {force_send}" ) - def email_domain_managers(self, domain_name, template: str, subject_template: str, context={}): + def email_domain_managers(self, domain: Domain, template: str, subject_template: str, context={}): """Send a single email built from a template to all managers for a given domain. template_name and subject_template_name are relative to the same template @@ -216,12 +216,6 @@ class DomainFormBaseView(DomainBaseView, FormMixin): Will log a warning if the email fails to send for any reason, but will not raise an error. """ - try: - domain = Domain.objects.get(name=domain_name) - except Domain.DoesNotExist: - logger.warning( - "Could not send notification email for domain %s, unable to find matching domain object", domain_name - ) manager_pks = UserDomainRole.objects.filter(domain=domain.pk, role=UserDomainRole.Roles.MANAGER).values_list( "user", flat=True ) @@ -233,7 +227,7 @@ class DomainFormBaseView(DomainBaseView, FormMixin): logger.warning( "Could not sent notification email to %s for domain %s", emails, - domain_name, + domain.name, exc_info=True, ) From bb3ceb4dea34eb55328e63897cc6223ff4a47630 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 2 Oct 2024 19:11:12 -0400 Subject: [PATCH 043/198] updated error messages --- src/registrar/forms/domain.py | 8 ++++---- src/registrar/forms/domain_request_wizard.py | 6 +++--- src/registrar/forms/portfolio.py | 3 ++- src/registrar/forms/user_profile.py | 2 +- src/registrar/templates/domain_org_name_address.html | 2 +- src/registrar/templates/domain_request_org_contact.html | 2 +- src/registrar/templates/portfolio_organization.html | 2 +- src/registrar/tests/test_forms.py | 2 +- src/registrar/utility/errors.py | 2 +- 9 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 84fcbe973..3d79790ca 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -35,7 +35,7 @@ class DomainAddUserForm(forms.Form): email = forms.EmailField( label="Email", max_length=None, - error_messages={"invalid": ("Enter your email address in the required format, like name@example.com.")}, + error_messages={"invalid": ("Enter an email address in the required format, like name@example.com.")}, validators=[ MaxLengthValidator( 320, @@ -285,7 +285,7 @@ class UserForm(forms.ModelForm): "required": "Enter your title or role in your organization (e.g., Chief Information Officer)" } self.fields["email"].error_messages = { - "required": "Enter your email address in the required format, like name@example.com." + "required": "Enter an email address in the required format, like name@example.com." } self.fields["phone"].error_messages["required"] = "Enter your phone number." self.domainInfo = None @@ -342,7 +342,7 @@ class ContactForm(forms.ModelForm): "required": "Enter your title or role in your organization (e.g., Chief Information Officer)" } self.fields["email"].error_messages = { - "required": "Enter your email address in the required format, like name@example.com." + "required": "Enter an email address in the required format, like name@example.com." } self.fields["phone"].error_messages["required"] = "Enter your phone number." self.domainInfo = None @@ -458,7 +458,7 @@ class DomainOrgNameAddressForm(forms.ModelForm): validators=[ RegexValidator( "^[0-9]{5}(?:-[0-9]{4})?$|^$", - message="Enter a zip code in the required format, like 12345 or 12345-6789.", + message="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789.", ) ], ) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index f2fdd32bc..4fb2f9265 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -137,10 +137,10 @@ class OrganizationContactForm(RegistrarForm): validators=[ RegexValidator( "^[0-9]{5}(?:-[0-9]{4})?$|^$", - message="Enter a zip code in the form of 12345 or 12345-6789.", + message="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789.", ) ], - error_messages={"required": ("Enter a zip code in the form of 12345 or 12345-6789.")}, + error_messages={"required": ("Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789.")}, ) urbanization = forms.CharField( required=False, @@ -603,7 +603,7 @@ class CisaRepresentativeForm(BaseDeletableRegistrarForm): max_length=None, required=False, error_messages={ - "invalid": ("Enter your representative’s email address in the required format, like name@example.com."), + "invalid": ("Enter an email address in the required format, like name@example.com."), }, validators=[ MaxLengthValidator( diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 14a45f6ae..85bc51240 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -17,7 +17,7 @@ class PortfolioOrgAddressForm(forms.ModelForm): validators=[ RegexValidator( "^[0-9]{5}(?:-[0-9]{4})?$|^$", - message="Enter a zip code in the required format, like 12345 or 12345-6789.", + message="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789.", ) ], ) @@ -38,6 +38,7 @@ class PortfolioOrgAddressForm(forms.ModelForm): "state_territory": { "required": "Select the state, territory, or military post where your organization is located." }, + "zipcode": {"required": "Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789."}, } widgets = { # We need to set the required attributed for State/territory diff --git a/src/registrar/forms/user_profile.py b/src/registrar/forms/user_profile.py index 2a6ed4a47..8d8e2973d 100644 --- a/src/registrar/forms/user_profile.py +++ b/src/registrar/forms/user_profile.py @@ -58,7 +58,7 @@ class UserProfileForm(forms.ModelForm): "required": "Enter your title or role in your organization (e.g., Chief Information Officer)" } self.fields["email"].error_messages = { - "required": "Enter your email address in the required format, like name@example.com." + "required": "Enter an email address in the required format, like name@example.com." } self.fields["phone"].error_messages["required"] = "Enter your phone number." diff --git a/src/registrar/templates/domain_org_name_address.html b/src/registrar/templates/domain_org_name_address.html index 78baed09e..f7f6b558b 100644 --- a/src/registrar/templates/domain_org_name_address.html +++ b/src/registrar/templates/domain_org_name_address.html @@ -42,7 +42,7 @@ {% input_with_errors form.state_territory %} - {% with add_class="usa-input--small" sublabel_text="Enter a zip code in the required format, like 12345 or 12345-6789." %} + {% with add_class="usa-input--small" sublabel_text="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789." %} {% input_with_errors form.zipcode %} {% endwith %} diff --git a/src/registrar/templates/domain_request_org_contact.html b/src/registrar/templates/domain_request_org_contact.html index d4f3c2071..72c6e4b6d 100644 --- a/src/registrar/templates/domain_request_org_contact.html +++ b/src/registrar/templates/domain_request_org_contact.html @@ -33,7 +33,7 @@ {% input_with_errors forms.0.state_territory %} - {% with add_class="usa-input--small" sublabel_text="Enter a zip code in the required format, like 12345 or 12345-6789." %} + {% with add_class="usa-input--small" sublabel_text="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789." %} {% input_with_errors forms.0.zipcode %} {% endwith %} diff --git a/src/registrar/templates/portfolio_organization.html b/src/registrar/templates/portfolio_organization.html index 2e6a1f488..1c59ae726 100644 --- a/src/registrar/templates/portfolio_organization.html +++ b/src/registrar/templates/portfolio_organization.html @@ -45,7 +45,7 @@ {% input_with_errors form.address_line2 %} {% input_with_errors form.city %} {% input_with_errors form.state_territory %} - {% with add_class="usa-input--small" sublabel_text="Enter a zip code in the required format, like 12345 or 12345-6789." %} + {% with add_class="usa-input--small" sublabel_text="Enter a 5-digit or 9-digit ZIP code, like 12345 or 12345-6789." %} {% input_with_errors form.zipcode %} {% endwith %} " @@ -390,7 +395,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): "is_federal": self.domain_request.is_federal(), "modal_button": modal_button, "modal_heading": "You are about to submit a domain request for " - + str(self.domain_request.requested_domain), + + text_domain_name, "modal_description": "Once you submit this request, you won’t be able to edit it until we review it.\ You’ll only be able to withdraw your request.", "review_form_is_complete": True, From 25b460c7246e5693419050748b4796cfa4179d08 Mon Sep 17 00:00:00 2001 From: asaki222 Date: Thu, 3 Oct 2024 12:44:45 -0400 Subject: [PATCH 049/198] ran lin --- src/registrar/views/domain_request.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index bbd27288b..55e1e2363 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -380,7 +380,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): requested_domain_name = self.domain_request.requested_domain.name text_domain_name = str(requested_domain_name) if requested_domain_name is not None else None - + if text_domain_name and len(text_domain_name) > 30: text_domain_name = text_domain_name[:27] + "..." @@ -394,8 +394,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): "visited": self.storage.get("step_history", []), "is_federal": self.domain_request.is_federal(), "modal_button": modal_button, - "modal_heading": "You are about to submit a domain request for " - + text_domain_name, + "modal_heading": "You are about to submit a domain request for " + text_domain_name, "modal_description": "Once you submit this request, you won’t be able to edit it until we review it.\ You’ll only be able to withdraw your request.", "review_form_is_complete": True, 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 050/198] 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 057/198] 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 058/198] 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 059/198] 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 060/198] 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 7ced4b73e0f38fb3ee6a5ec9d1756bef68df130a Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 3 Oct 2024 15:52:26 -0600 Subject: [PATCH 061/198] 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 49a87c4bd..11a71b0df 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 aa2fc10c0..928cea43e 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -340,7 +340,10 @@ class TestDomainDetail(TestDomainOverview): 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 062/198] 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 063/198] 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 d933da326cb30f6c40cc735cbc13b676c0a586f7 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 3 Oct 2024 18:44:34 -0400 Subject: [PATCH 064/198] 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 | 133 +++++++++++------- src/registrar/views/portfolios.py | 34 ++++- 6 files changed, 165 insertions(+), 73 deletions(-) diff --git a/src/registrar/forms/__init__.py b/src/registrar/forms/__init__.py index 033e955ed..121e2b3f7 100644 --- a/src/registrar/forms/__init__.py +++ b/src/registrar/forms/__init__.py @@ -13,4 +13,5 @@ from .domain import ( ) from .portfolio import ( PortfolioOrgAddressForm, + PortfolioMemberForm, ) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 14a45f6ae..2b669c50c 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,6 +4,9 @@ import logging 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 @@ class PortfolioSeniorOfficialForm(forms.ModelForm): 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 a3b2364a9..43467602e 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 55435d3b1..3f089ebe1 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 34cc08ee7..f3a175980 100644 --- a/src/registrar/views/portfolio_members_json.py +++ b/src/registrar/views/portfolio_members_json.py @@ -8,6 +8,7 @@ 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 @@ -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) + # member_ids = get_member_ids_from_request(request, portfolio) + # members = 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 + 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 + ] - objects = apply_search(objects, request) - # objects = apply_status_filter(objects, request) - objects = apply_sorting(objects, 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 + } + for invite in invitations + ] - paginator = Paginator(objects, 10) + # Combine both lists into one unified list + combined_list = permission_list + invitation_list + + unfiltered_total = len(combined_list) + + combined_list = apply_search(combined_list, request) + combined_list = apply_sorting(combined_list, 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, 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 + # 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(queryset, request): +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 fe15ccd27..c4a19e2cd 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -3,7 +3,7 @@ from django.http import Http404 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 registrar.views.utility.permission_views import ( ) 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 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): class PortfolioMemberView(PortfolioMemberPermissionView, View): template_name = "portfolio_member.html" - model = User + form_class = PortfolioMemberForm - # def get(self, request): - # """Add additional context data to the template.""" - # return render(request, self.template_name, context=self.get_context_data()) + 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 065/198] 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 f3a175980..ed8fb9c3f 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 066/198] 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 754edca1c..da115d471 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -86,6 +86,11 @@ urlpatterns = [ 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 2b669c50c..cdf00c625 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -4,6 +4,7 @@ import logging 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 PortfolioMemberForm(forms.ModelForm): "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 3f089ebe1..284a795eb 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 98402fb95..d33c327f8 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -13,7 +13,9 @@ from registrar.views.utility.permission_views import ( PortfolioDomainsPermissionView, PortfolioBasePermissionView, NoPortfolioDomainsPermissionView, + PortfolioInvitedMemberEditPermissionView, PortfolioInvitedMemberPermissionView, + PortfolioMemberEditPermissionView, PortfolioMemberPermissionView, PortfolioMembersPermissionView, ) @@ -57,6 +59,20 @@ class PortfolioMembersView(PortfolioMembersPermissionView, View): 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 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): 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 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): }) - 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 @@ class PortfolioInvitedMemberView(PortfolioInvitedMemberPermissionView, View): 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 10e8fe3c6..94fc0b1c5 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -515,6 +515,23 @@ class PortfolioMemberPermission(PortfolioBasePermission): 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 @@ class PortfolioInvitedMemberPermission(PortfolioBasePermission): 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 65af99a14..6fb7fee50 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -15,7 +15,9 @@ from .mixins import ( 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 070/198] 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 071/198] 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 072/198] 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 a4a83986388a9f15f3690b7eeee6d0052a21faa9 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Fri, 4 Oct 2024 13:32:47 -0600 Subject: [PATCH 073/198] 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 5cb559a5a..0fb29d2eb 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -46,7 +46,6 @@

    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 074/198] wip member edit ui --- .../includes/member_permissions.html | 44 ++++++ .../templates/includes/summary_item.html | 8 +- src/registrar/templates/portfolio_member.html | 133 +++++++++--------- src/registrar/views/portfolio_members_json.py | 2 - src/registrar/views/portfolios.py | 4 +- 5 files changed, 119 insertions(+), 72 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 000000000..d2e8d5392 --- /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 d4c68395f..fbe392c4d 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 @@ 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 c68f561d2..718970818 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

    +

    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 %} +
    + + 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 %} +
    + + {% 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 %} -

    - {% 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 %} -

    - -

    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 %} - -
    - - - -
    {% endblock %} diff --git a/src/registrar/views/portfolio_members_json.py b/src/registrar/views/portfolio_members_json.py index 56904988e..ec0004654 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 d33c327f8..2b231a0b9 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -65,6 +65,7 @@ class PortfolioMemberView(PortfolioMemberPermissionView, View): 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 @@ class PortfolioMemberEditView(PortfolioMemberEditPermissionView, View): 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 @@ class PortfolioInvitedMemberView(PortfolioInvitedMemberPermissionView, View): # 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 d86a676f4f24f0d4570216407a05cbb76f0b093f Mon Sep 17 00:00:00 2001 From: asaki222 Date: Fri, 4 Oct 2024 16:30:11 -0400 Subject: [PATCH 075/198] trying it via styling --- src/registrar/templates/includes/modal.html | 2 +- src/registrar/views/domain_request.py | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/registrar/templates/includes/modal.html b/src/registrar/templates/includes/modal.html index b0b316a2f..3a6535a4c 100644 --- a/src/registrar/templates/includes/modal.html +++ b/src/registrar/templates/includes/modal.html @@ -2,7 +2,7 @@
    -