From 94d5682b242d92cb0678a4865b5dfad42610e8be Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Tue, 20 Jun 2023 19:10:58 -0400 Subject: [PATCH 01/16] Implement all statuses in application admin class, implement approved email, consolidate email methods in application model class, unit test emails sends as side-effects of admin save --- src/registrar/admin.py | 40 +++++-- src/registrar/models/domain_application.py | 102 +++++++++-------- .../emails/status_change_approved.txt | 40 +++++++ .../emails/status_change_approved_subject.txt | 1 + src/registrar/tests/test_admin.py | 105 +++++++++++++++++- 5 files changed, 228 insertions(+), 60 deletions(-) create mode 100644 src/registrar/templates/emails/status_change_approved.txt create mode 100644 src/registrar/templates/emails/status_change_approved_subject.txt diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7eb1668d8..3e524ab80 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -90,16 +90,36 @@ class DomainApplicationAdmin(AuditedAdmin): # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if ( - obj.status != original_obj.status - and obj.status == models.DomainApplication.INVESTIGATING - ): - # This is a transition annotated method in model which will throw an - # error if the condition is violated. To make this work, we need to - # call it on the original object which has the right status value, - # but pass the current object which contains the up-to-date data - # for the email. - original_obj.in_review(obj) + if obj.status != original_obj.status: + if obj.status == models.DomainApplication.STARTED: + # No conditions + pass + if obj.status == models.DomainApplication.SUBMITTED: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we call it on the + # original object which has the right status value, and pass + # the updated object which contains the up-to-date data + # for the side effects (like an email send). + original_obj.submit(updated_domain_application=obj) + if obj.status == models.DomainApplication.INVESTIGATING: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we call it on the + # original object which has the right status value, and pass + # the updated object which contains the up-to-date data + # for the side effects (like an email send). + original_obj.in_review(updated_domain_application=obj) + if obj.status == models.DomainApplication.APPROVED: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we call it on the + # original object which has the right status value, and pass + # the updated object which contains the up-to-date data + # for the side effects (like an email send). + original_obj.approve(updated_domain_application=obj) + if obj.status == models.DomainApplication.WITHDRAWN: + # This is a transition annotated method in model which will throw an + # error if the condition is violated. To make this work, we need to + # call it on the original object which has the right status value. + original_obj.withdraw() super().save_model(request, obj, form, change) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 57be80b36..6cb1482e7 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -471,59 +471,34 @@ class DomainApplication(TimeStampedModel): except Exception: return "" - def _send_confirmation_email(self): - """Send a confirmation email that this application was submitted. + def _send_status_update_email( + self, new_status, email_template, email_template_subject + ): + """Send a atatus update email to the submitter. The email goes to the email address that the submitter gave as their contact information. If there is not submitter information, then do nothing. """ + if self.submitter is None or self.submitter.email is None: logger.warning( - "Cannot send confirmation email, no submitter email address." + f"Cannot send {new_status} email, no submitter email address." ) return try: send_templated_email( - "emails/submission_confirmation.txt", - "emails/submission_confirmation_subject.txt", + email_template, + email_template_subject, self.submitter.email, context={"application": self}, ) - logger.info( - f"Submission confirmation email sent to: {self.submitter.email}" - ) + logger.info(f"The {new_status} email sent to: {self.submitter.email}") except EmailSendingError: logger.warning("Failed to send confirmation email", exc_info=True) - def _send_in_review_email(self): - """Send an email that this application is now in review. - - The email goes to the email address that the submitter gave as their - contact information. If there is not submitter information, then do - nothing. - """ - if self.submitter is None or self.submitter.email is None: - logger.warning( - "Cannot send status change (in review) email," - "no submitter email address." - ) - return - try: - send_templated_email( - "emails/status_change_in_review.txt", - "emails/status_change_in_review_subject.txt", - self.submitter.email, - context={"application": self}, - ) - logging.info(f"In review email sent to: {self.submitter.email}") - except EmailSendingError: - logger.warning( - "Failed to send status change (in review) email", exc_info=True - ) - @transition(field="status", source=[STARTED, WITHDRAWN], target=SUBMITTED) - def submit(self): + def submit(self, updated_domain_application=None): """Submit an application that is started.""" # check our conditions here inside the `submit` method so that we @@ -542,10 +517,40 @@ class DomainApplication(TimeStampedModel): # When an application is submitted, we need to send a confirmation email # This is a side-effect of the state transition - self._send_confirmation_email() + if updated_domain_application is not None: + # A DomainApplication is being passed to this method (ie from admin) + updated_domain_application._send_status_update_email( + "submission confirmation", + "emails/submission_confirmation.txt", + "emails/submission_confirmation_subject.txt", + ) + else: + # views/application.py + self._send_status_update_email( + "submission confirmation", + "emails/submission_confirmation.txt", + "emails/submission_confirmation_subject.txt", + ) + + @transition(field="status", source=SUBMITTED, target=INVESTIGATING) + def in_review(self, updated_domain_application): + """Investigate an application that has been submitted. + + This method is called in admin.py on the original application + which has the correct status value, but is passed the changed + application which has the up-to-date data that we'll use + in the email.""" + + # When an application is moved to in review, we need to send a + # confirmation email. This is a side-effect of the state transition + updated_domain_application._send_status_update_email( + "application in review", + "emails/status_change_in_review.txt", + "emails/status_change_in_review_subject.txt", + ) @transition(field="status", source=[SUBMITTED, INVESTIGATING], target=APPROVED) - def approve(self): + def approve(self, updated_domain_application=None): """Approve an application that has been submitted. This has substantial side-effects because it creates another database @@ -570,18 +575,21 @@ class DomainApplication(TimeStampedModel): user=self.creator, domain=created_domain, role=UserDomainRole.Roles.ADMIN ) - @transition(field="status", source=SUBMITTED, target=INVESTIGATING) - def in_review(self, updated_domain_application): - """Investigate an application that has been submitted. - - This method is called in admin.py on the original application - which has the correct status value, but is passed the changed - application which has the up-to-date data that we'll use - in the email.""" - # When an application is moved to in review, we need to send a # confirmation email. This is a side-effect of the state transition - updated_domain_application._send_in_review_email() + if updated_domain_application is not None: + # A DomainApplication is being passed to this method (ie from admin) + updated_domain_application._send_status_update_email( + "application approved", + "emails/status_change_approved.txt", + "emails/status_change_approved_subject.txt", + ) + else: + self._send_status_update_email( + "application approved", + "emails/status_change_approved.txt", + "emails/status_change_approved.txt", + ) @transition(field="status", source=[SUBMITTED, INVESTIGATING], target=WITHDRAWN) def withdraw(self): diff --git a/src/registrar/templates/emails/status_change_approved.txt b/src/registrar/templates/emails/status_change_approved.txt new file mode 100644 index 000000000..80bf78842 --- /dev/null +++ b/src/registrar/templates/emails/status_change_approved.txt @@ -0,0 +1,40 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi {{ application.submitter.first_name }}. + +Congratulations! Your .gov domain request has been approved. + +DOMAIN REQUESTED: {{ application.requested_domain.name }} +REQUEST RECEIVED ON: {{ application.updated_at|date }} +REQUEST #: {{ application.id }} +STATUS: In review + +Now that your .gov domain has been approved, there are a few more things to do before your domain can be used. + + +YOU MUST ADD DOMAIN NAME SERVER INFORMATION + +Before your .gov domain can be used, you have to connect it to your Domain Name System (DNS) hosting service. At this time, we don’t provide DNS hosting services. +Go to the domain management page to add your domain name server information . + +Get help with adding your domain name server information . + + +ADD DOMAIN MANAGERS, SECURITY EMAIL + +We strongly recommend that you add other points of contact who will help manage your domain. We also recommend that you provide a security email. This email will allow the public to report security issues on your domain. Security emails are made public. + +Go to the domain management page to add domain contacts and a security email . + +Get help with managing your .gov domain . + + +THANK YOU + +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Visit +{% endautoescape %} diff --git a/src/registrar/templates/emails/status_change_approved_subject.txt b/src/registrar/templates/emails/status_change_approved_subject.txt new file mode 100644 index 000000000..32756d463 --- /dev/null +++ b/src/registrar/templates/emails/status_change_approved_subject.txt @@ -0,0 +1 @@ +Your .gov domain request is approved \ No newline at end of file diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 2f2e1190b..b34c3a08a 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1,7 +1,7 @@ from django.test import TestCase, RequestFactory from django.contrib.admin.sites import AdminSite from registrar.admin import DomainApplicationAdmin -from registrar.models import DomainApplication, User +from registrar.models import DomainApplication, DomainInformation, User from .common import completed_application from django.conf import settings @@ -15,7 +15,56 @@ class TestDomainApplicationAdmin(TestCase): self.factory = RequestFactory() @boto3_mocking.patching - def test_save_model_sends_email_on_property_change(self): + def test_save_model_sends_submitted_email(self): + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + mock_client = MagicMock() + mock_client_instance = mock_client.return_value + + with boto3_mocking.clients.handler_for("sesv2", mock_client): + # Create a sample application + application = completed_application() + + # Create a mock request + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + + # Create an instance of the model admin + model_admin = DomainApplicationAdmin(DomainApplication, self.site) + + # Modify the application's property + application.status = DomainApplication.SUBMITTED + + # Use the model admin's save_model method + model_admin.save_model(request, application, form=None, change=True) + + # Access the arguments passed to send_email + call_args = mock_client_instance.send_email.call_args + args, kwargs = call_args + + # Retrieve the email details from the arguments + from_email = kwargs.get("FromEmailAddress") + to_email = kwargs["Destination"]["ToAddresses"][0] + email_content = kwargs["Content"] + email_body = email_content["Simple"]["Body"]["Text"]["Data"] + + # Assert or perform other checks on the email details + expected_string = "We received your .gov domain request." + self.assertEqual(from_email, settings.DEFAULT_FROM_EMAIL) + self.assertEqual(to_email, EMAIL) + self.assertIn(expected_string, email_body) + + # Perform assertions on the mock call itself + mock_client_instance.send_email.assert_called_once() + + # Cleanup + application.delete() + + @boto3_mocking.patching + def test_save_model_sends_in_review_email(self): # make sure there is no user with this email EMAIL = "mayor@igorville.gov" User.objects.filter(email=EMAIL).delete() @@ -52,7 +101,7 @@ class TestDomainApplicationAdmin(TestCase): email_body = email_content["Simple"]["Body"]["Text"]["Data"] # Assert or perform other checks on the email details - expected_string = "Your .gov domain request is being reviewed" + expected_string = "Your .gov domain request is being reviewed." self.assertEqual(from_email, settings.DEFAULT_FROM_EMAIL) self.assertEqual(to_email, EMAIL) self.assertIn(expected_string, email_body) @@ -62,3 +111,53 @@ class TestDomainApplicationAdmin(TestCase): # Cleanup application.delete() + + @boto3_mocking.patching + def test_save_model_sends_approved_email(self): + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + + mock_client = MagicMock() + mock_client_instance = mock_client.return_value + + with boto3_mocking.clients.handler_for("sesv2", mock_client): + # Create a sample application + application = completed_application(status=DomainApplication.INVESTIGATING) + + # Create a mock request + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + + # Create an instance of the model admin + model_admin = DomainApplicationAdmin(DomainApplication, self.site) + + # Modify the application's property + application.status = DomainApplication.APPROVED + + # Use the model admin's save_model method + model_admin.save_model(request, application, form=None, change=True) + + # Access the arguments passed to send_email + call_args = mock_client_instance.send_email.call_args + args, kwargs = call_args + + # Retrieve the email details from the arguments + from_email = kwargs.get("FromEmailAddress") + to_email = kwargs["Destination"]["ToAddresses"][0] + email_content = kwargs["Content"] + email_body = email_content["Simple"]["Body"]["Text"]["Data"] + + # Assert or perform other checks on the email details + expected_string = "Congratulations! Your .gov domain request has been approved." + self.assertEqual(from_email, settings.DEFAULT_FROM_EMAIL) + self.assertEqual(to_email, EMAIL) + self.assertIn(expected_string, email_body) + + # Perform assertions on the mock call itself + mock_client_instance.send_email.assert_called_once() + + # Cleanup + DomainInformation.objects.get(id=application.pk).delete() + application.delete() From 22e73c6fc2adb2386aa8bc9aaf94389b3c1a3ffd Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Wed, 21 Jun 2023 18:15:13 -0400 Subject: [PATCH 02/16] test unallowed transitions in model --- src/registrar/tests/test_admin.py | 3 +- src/registrar/tests/test_models.py | 120 ++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index b34c3a08a..4dc2070b5 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -159,5 +159,6 @@ class TestDomainApplicationAdmin(TestCase): mock_client_instance.send_email.assert_called_once() # Cleanup - DomainInformation.objects.get(id=application.pk).delete() + if DomainInformation.objects.get(id=application.pk) is not None: + DomainInformation.objects.get(id=application.pk).delete() application.delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 65d2c8d11..ba02fa971 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -14,7 +14,8 @@ from registrar.models import ( ) import boto3_mocking # type: ignore -from .common import MockSESClient, less_console_noise +from .common import MockSESClient, less_console_noise, completed_application +from django_fsm import TransitionNotAllowed boto3_mocking.clients.register_handler("sesv2", MockSESClient) @@ -134,6 +135,123 @@ class TestDomainApplication(TestCase): 0, ) + def test_transition_not_allowed_submitted_submitted(self): + """Create an application with status submitted and call submit + against transition rules""" + + application = completed_application(status=DomainApplication.SUBMITTED) + + with self.assertRaises(TransitionNotAllowed): + application.submit() + + def test_transition_not_allowed_investigating_submitted(self): + """Create an application with status investigating and call submit + against transition rules""" + + application = completed_application(status=DomainApplication.INVESTIGATING) + + with self.assertRaises(TransitionNotAllowed): + application.submit() + + def test_transition_not_allowed_approved_submitted(self): + """Create an application with status approved and call submit + against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + + with self.assertRaises(TransitionNotAllowed): + application.submit() + + def test_transition_not_allowed_started_investigating(self): + """Create an application with status started and call in_review + against transition rules""" + + application = completed_application(status=DomainApplication.STARTED) + + with self.assertRaises(TransitionNotAllowed): + application.in_review() + + def test_transition_not_allowed_investigating_investigating(self): + """Create an application with status investigating and call in_review + against transition rules""" + + application = completed_application(status=DomainApplication.INVESTIGATING) + + with self.assertRaises(TransitionNotAllowed): + application.in_review() + + def test_transition_not_allowed_approved_investigating(self): + """Create an application with status approved and call in_review + against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + + with self.assertRaises(TransitionNotAllowed): + application.in_review() + + def test_transition_not_allowed_withdrawn_investigating(self): + """Create an application with status withdrawn and call in_review + against transition rules""" + + application = completed_application(status=DomainApplication.WITHDRAWN) + + with self.assertRaises(TransitionNotAllowed): + application.in_review() + + def test_transition_not_allowed_started_approved(self): + """Create an application with status started and call approve + against transition rules""" + + application = completed_application(status=DomainApplication.STARTED) + + with self.assertRaises(TransitionNotAllowed): + application.approve() + + def test_transition_not_allowed_approved_approved(self): + """Create an application with status approved and call approve + against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + + with self.assertRaises(TransitionNotAllowed): + application.approve() + + def test_transition_not_allowed_withdrawn_approved(self): + """Create an application with status withdrawn and call approve + against transition rules""" + + application = completed_application(status=DomainApplication.WITHDRAWN) + + with self.assertRaises(TransitionNotAllowed): + application.approve() + + def test_transition_not_allowed_started_withdrawn(self): + """Create an application with status started and call withdraw + against transition rules""" + + application = completed_application(status=DomainApplication.STARTED) + + with self.assertRaises(TransitionNotAllowed): + application.withdraw() + + def test_transition_not_allowed_approved_withdrawn(self): + """Create an application with status approved and call withdraw + against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + + with self.assertRaises(TransitionNotAllowed): + application.withdraw() + + def test_transition_not_allowed_withdrawn_withdrawn(self): + """Create an application with status withdrawn and call withdraw + against transition rules""" + + application = completed_application(status=DomainApplication.WITHDRAWN) + + with self.assertRaises(TransitionNotAllowed): + application.withdraw() + class TestPermissions(TestCase): From 0fcf7de5464ca45afcce2bf9f40d8caed3a272a7 Mon Sep 17 00:00:00 2001 From: brandonlenz Date: Thu, 22 Jun 2023 10:21:47 -0400 Subject: [PATCH 03/16] First Draft ADR --- .../decisions/0021-django-admin.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 docs/architecture/decisions/0021-django-admin.md diff --git a/docs/architecture/decisions/0021-django-admin.md b/docs/architecture/decisions/0021-django-admin.md new file mode 100644 index 000000000..20dc747cb --- /dev/null +++ b/docs/architecture/decisions/0021-django-admin.md @@ -0,0 +1,40 @@ +# 21. Use Django Admin for Application Management + +Date: 2023-06-22 + +## Status + +Accepted + +## Context + +CISA needs a way to perform administrative actions to manage the new get.gov application as well as the .gov domain +application requests submitted. Analysts need to be able to view, review, and approve domain applications. Other +dashboard views, reports, searches (with filters and sorting) are also highly desired. + +## Decision + +Use Django's [Admin](https://docs.djangoproject.com/en/4.2/ref/contrib/admin/) site for administrative actions. Django +Admin gives administrators all the powers we anticipate needing (and more), with relatively little overhead on the +development team. + +## Consequences + +Django admin provides the team with a _huge_ head start on the creation of an administrator portal. + +While Django Admin is highly customizable, development will be constrained by what is possible within Django Admin. + +We anticipate that this will, overall, speed up the time to MVP compared to building a completely custom solution. + +Django Admin offers omnipotence for administrators out of the box, with direct access to database objects. This includes +the ability to put the application and its data in an erroneous state, based on otherwise normal business rules/logic. + +In contrast to building an admin interface from scratch where development activities would predominantly +involve _building up_, leveraging Django Admin will require carefully _pairing back_ the functionalities available to +users such as analysts. + +While we anticipate that Django Admin will meet (or even exceed) the user needs that we are aware of today, it is still +an open question whether Django Admin will be the long-term administrator tool of choice. A pivot away from Django Admin +in the future would of course mean starting from scratch at a later date, and potentially juggling two separate admin +portals for a period of time while a custom solution is incrementally developed. This would result in an overall +_increase_ to the total amount of development time invested in the administrator portal. From bba9f1b3762cb0b38b26b75043dc26fc55856a87 Mon Sep 17 00:00:00 2001 From: brandonlenz Date: Thu, 22 Jun 2023 10:26:27 -0400 Subject: [PATCH 04/16] Small language tweaks --- docs/architecture/decisions/0021-django-admin.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/architecture/decisions/0021-django-admin.md b/docs/architecture/decisions/0021-django-admin.md index 20dc747cb..6e9580932 100644 --- a/docs/architecture/decisions/0021-django-admin.md +++ b/docs/architecture/decisions/0021-django-admin.md @@ -22,7 +22,8 @@ development team. Django admin provides the team with a _huge_ head start on the creation of an administrator portal. -While Django Admin is highly customizable, development will be constrained by what is possible within Django Admin. +While Django Admin is highly customizable, design and development will be constrained by what is possible within Django +Admin. We anticipate that this will, overall, speed up the time to MVP compared to building a completely custom solution. @@ -37,4 +38,4 @@ While we anticipate that Django Admin will meet (or even exceed) the user needs an open question whether Django Admin will be the long-term administrator tool of choice. A pivot away from Django Admin in the future would of course mean starting from scratch at a later date, and potentially juggling two separate admin portals for a period of time while a custom solution is incrementally developed. This would result in an overall -_increase_ to the total amount of development time invested in the administrator portal. +_increase_ to the total amount of time invested in building an administrator portal. From 0e9d1d5f0d9a05a6b9d12cca06d788ea880af0a2 Mon Sep 17 00:00:00 2001 From: Mohammad Minaie Date: Thu, 22 Jun 2023 12:00:33 -0400 Subject: [PATCH 05/16] Updates to the base issue type --- .github/ISSUE_TEMPLATE/issue-default.yml | 49 +++++++++++------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/issue-default.yml b/.github/ISSUE_TEMPLATE/issue-default.yml index 665ba530d..1212f26db 100644 --- a/.github/ISSUE_TEMPLATE/issue-default.yml +++ b/.github/ISSUE_TEMPLATE/issue-default.yml @@ -1,37 +1,34 @@ name: Issue -description: Provide a title for the issue you are describing -title: "Please provide a clear title" -labels: ["review"] -# assignees: -# - kitsushadow +description: Describe the item + body: - type: markdown + id: help attributes: value: | - Describe the ticket your are capturing in further detail. + > **Note** + > GitHub Issues use [GitHub Flavored Markdown](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax) for formatting. - type: textarea - id: why + id: issue attributes: - label: Ticket Description - description: Please provide details to accurately reflect why this ticket is being captured and also what is necessary to resolve. - placeholder: Provide details describing your lead up to needing this issue as well as any resolution or requirements for resolving or working on this more. - value: "While (working on or discussing) (issue #000 or domain request validation) I discovered there was (a missing workflow, an improvement, a missing feature). To resolve this (more research, a new feature, a new field, an interview) is required. Provide any links, screenshots, or mockups which would further detail the description." - validations: - required: true - - type: dropdown - id: type - attributes: - label: Issue Type - description: Does this work require - options: - - discovery (Default) - - development - - design review + label: Issue + description: | + Please describe the issue you are adding or content you are suggesting. + Share any next steps that should be taken our outcomes that would be beneficial. validations: required: true - type: textarea - id: Dependencies + id: additional-context attributes: - label: Link dependent issues - description: If this ticket is dependent on another issue or blocks a current issue, please link. - render: shell \ No newline at end of file + label: Additional Context (optional) + description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" + - type: textarea + id: issue-links + attributes: + label: Issue Links (optional) + description: | + Does this issue relate to any other existing work? + + Example: + - 🚧 Blocked by: #123 + - πŸ”„ Relates to: #234 \ No newline at end of file From c567296eae318f6721c3f1f80579f8f4cd221c0e Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 22 Jun 2023 13:22:06 -0700 Subject: [PATCH 06/16] added ryan to fixtures --- src/registrar/fixtures.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index 3aefbe68b..cc505ec16 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -49,6 +49,11 @@ class UserFixture: "first_name": "Cameron", "last_name": "Dixon", }, + { + "username": "0353607a-cbba-47d2-98d7-e83dcd5b90ea", + "first_name": "Ryan", + "last_name": "Brooks", + }, ] @classmethod From 33461f054f80df1f223d970e687cd3042bf43a1d Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 22 Jun 2023 15:25:16 -0700 Subject: [PATCH 07/16] fixed linting issue --- src/registrar/fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index cc505ec16..41295df38 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -49,7 +49,7 @@ class UserFixture: "first_name": "Cameron", "last_name": "Dixon", }, - { + { "username": "0353607a-cbba-47d2-98d7-e83dcd5b90ea", "first_name": "Ryan", "last_name": "Brooks", From 05e990a8fd12cf4bf3c6fa7fefa21c33e5287ad7 Mon Sep 17 00:00:00 2001 From: Mohammad Minaie Date: Fri, 23 Jun 2023 09:17:46 -0400 Subject: [PATCH 08/16] updates to remove please --- .github/ISSUE_TEMPLATE/issue-default.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/issue-default.yml b/.github/ISSUE_TEMPLATE/issue-default.yml index 1212f26db..81ae25860 100644 --- a/.github/ISSUE_TEMPLATE/issue-default.yml +++ b/.github/ISSUE_TEMPLATE/issue-default.yml @@ -13,7 +13,7 @@ body: attributes: label: Issue description: | - Please describe the issue you are adding or content you are suggesting. + Describe the issue you are adding or content you are suggesting. Share any next steps that should be taken our outcomes that would be beneficial. validations: required: true @@ -21,7 +21,7 @@ body: id: additional-context attributes: label: Additional Context (optional) - description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" + description: "Include additional references (screenshots, design links, documentation, etc.) that are relevant" - type: textarea id: issue-links attributes: From 729cc6745515458cde706c2e3181d7b815381d73 Mon Sep 17 00:00:00 2001 From: Mohammad Minaie Date: Fri, 23 Jun 2023 09:21:12 -0400 Subject: [PATCH 09/16] updates to layout and language after seeing the issue merged. --- .github/ISSUE_TEMPLATE/issue-default.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/issue-default.yml b/.github/ISSUE_TEMPLATE/issue-default.yml index 81ae25860..f8f422d58 100644 --- a/.github/ISSUE_TEMPLATE/issue-default.yml +++ b/.github/ISSUE_TEMPLATE/issue-default.yml @@ -1,4 +1,3 @@ -name: Issue description: Describe the item body: @@ -11,7 +10,7 @@ body: - type: textarea id: issue attributes: - label: Issue + label: Issue Description description: | Describe the issue you are adding or content you are suggesting. Share any next steps that should be taken our outcomes that would be beneficial. From e086f03ad8515253b94a056dbe7343d100d13959 Mon Sep 17 00:00:00 2001 From: Mohammad Minaie Date: Fri, 23 Jun 2023 14:38:44 -0400 Subject: [PATCH 10/16] removing name, removed the name fully. added back to debug further --- .github/ISSUE_TEMPLATE/issue-default.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/issue-default.yml b/.github/ISSUE_TEMPLATE/issue-default.yml index f8f422d58..ea12b5658 100644 --- a/.github/ISSUE_TEMPLATE/issue-default.yml +++ b/.github/ISSUE_TEMPLATE/issue-default.yml @@ -1,4 +1,5 @@ -description: Describe the item +name: Issue +description: Capture uncategorized work or content body: - type: markdown From c2e24f4fb9954ab73e24e9acb24f6235e0b237f0 Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 23 Jun 2023 15:28:51 -0400 Subject: [PATCH 11/16] tweak documentation and change ifs to elifs --- src/registrar/admin.py | 24 +++++-------------- src/registrar/models/domain_application.py | 28 +++++++++++++++------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3e524ab80..93dc8ac38 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -94,31 +94,19 @@ class DomainApplicationAdmin(AuditedAdmin): if obj.status == models.DomainApplication.STARTED: # No conditions pass - if obj.status == models.DomainApplication.SUBMITTED: + elif obj.status == models.DomainApplication.SUBMITTED: # This is an fsm in model which will throw an error if the # transition condition is violated, so we call it on the # original object which has the right status value, and pass # the updated object which contains the up-to-date data - # for the side effects (like an email send). + # for the side effects (like an email send). Same + # comment applies to original_obj method calls below. original_obj.submit(updated_domain_application=obj) - if obj.status == models.DomainApplication.INVESTIGATING: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we call it on the - # original object which has the right status value, and pass - # the updated object which contains the up-to-date data - # for the side effects (like an email send). + elif obj.status == models.DomainApplication.INVESTIGATING: original_obj.in_review(updated_domain_application=obj) - if obj.status == models.DomainApplication.APPROVED: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we call it on the - # original object which has the right status value, and pass - # the updated object which contains the up-to-date data - # for the side effects (like an email send). + elif obj.status == models.DomainApplication.APPROVED: original_obj.approve(updated_domain_application=obj) - if obj.status == models.DomainApplication.WITHDRAWN: - # This is a transition annotated method in model which will throw an - # error if the condition is violated. To make this work, we need to - # call it on the original object which has the right status value. + elif obj.status == models.DomainApplication.WITHDRAWN: original_obj.withdraw() super().save_model(request, obj, form, change) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 6cb1482e7..71745b17c 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -499,7 +499,14 @@ class DomainApplication(TimeStampedModel): @transition(field="status", source=[STARTED, WITHDRAWN], target=SUBMITTED) def submit(self, updated_domain_application=None): - """Submit an application that is started.""" + """Submit an application that is started. + + As a side effect, an email notification is sent. + + This method is called in admin.py on the original application + which has the correct status value, but is passed the changed + application which has the up-to-date data that we'll use + in the email.""" # check our conditions here inside the `submit` method so that we # can raise more informative exceptions @@ -515,8 +522,6 @@ class DomainApplication(TimeStampedModel): if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - # When an application is submitted, we need to send a confirmation email - # This is a side-effect of the state transition if updated_domain_application is not None: # A DomainApplication is being passed to this method (ie from admin) updated_domain_application._send_status_update_email( @@ -525,7 +530,8 @@ class DomainApplication(TimeStampedModel): "emails/submission_confirmation_subject.txt", ) else: - # views/application.py + # Or this method is called with the right application + # for context, ie from views/application.py self._send_status_update_email( "submission confirmation", "emails/submission_confirmation.txt", @@ -536,13 +542,13 @@ class DomainApplication(TimeStampedModel): def in_review(self, updated_domain_application): """Investigate an application that has been submitted. + As a side effect, an email notification is sent. + This method is called in admin.py on the original application which has the correct status value, but is passed the changed application which has the up-to-date data that we'll use in the email.""" - # When an application is moved to in review, we need to send a - # confirmation email. This is a side-effect of the state transition updated_domain_application._send_status_update_email( "application in review", "emails/status_change_in_review.txt", @@ -555,7 +561,13 @@ class DomainApplication(TimeStampedModel): This has substantial side-effects because it creates another database object for the approved Domain and makes the user who created the - application into an admin on that domain. + application into an admin on that domain. It also triggers an email + notification. + + This method is called in admin.py on the original application + which has the correct status value, but is passed the changed + application which has the up-to-date data that we'll use + in the email. """ # create the domain @@ -575,8 +587,6 @@ class DomainApplication(TimeStampedModel): user=self.creator, domain=created_domain, role=UserDomainRole.Roles.ADMIN ) - # When an application is moved to in review, we need to send a - # confirmation email. This is a side-effect of the state transition if updated_domain_application is not None: # A DomainApplication is being passed to this method (ie from admin) updated_domain_application._send_status_update_email( From 7445eff91aca48bd17bfc4e3f2585af254e6278c Mon Sep 17 00:00:00 2001 From: brandonlenz Date: Fri, 23 Jun 2023 15:33:22 -0400 Subject: [PATCH 12/16] Remove '(optional)' from question text, since it looks weird in rendered issue --- .github/ISSUE_TEMPLATE/bug.yml | 6 +++--- .github/ISSUE_TEMPLATE/story.yml | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index 53533a951..7cd332156 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -43,7 +43,7 @@ body: - type: textarea id: environment attributes: - label: Environment (optional) + label: Environment description: | Where is this issue occurring? If related to development environment, list the relevant tool versions. @@ -54,12 +54,12 @@ body: - type: textarea id: additional-context attributes: - label: Additional Context (optional) + label: Additional Context description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" - type: textarea id: issue-links attributes: - label: Issue Links (optional) + label: Issue Links description: | What other issues does this story relate to and how? diff --git a/.github/ISSUE_TEMPLATE/story.yml b/.github/ISSUE_TEMPLATE/story.yml index 6173dd9e1..41516cc29 100644 --- a/.github/ISSUE_TEMPLATE/story.yml +++ b/.github/ISSUE_TEMPLATE/story.yml @@ -47,12 +47,12 @@ body: - type: textarea id: additional-context attributes: - label: Additional Context (optional) + label: Additional Context description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" - type: textarea id: issue-links attributes: - label: Issue Links (optional) + label: Issue Links description: | What other issues does this story relate to and how? From 1cb380c03063b9f4db27036366a8c2399dc5590f Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 23 Jun 2023 15:37:03 -0400 Subject: [PATCH 13/16] cleanup --- src/registrar/admin.py | 2 +- src/registrar/models/domain_application.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 93dc8ac38..fee5033af 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -99,7 +99,7 @@ class DomainApplicationAdmin(AuditedAdmin): # transition condition is violated, so we call it on the # original object which has the right status value, and pass # the updated object which contains the up-to-date data - # for the side effects (like an email send). Same + # for the side effects (like an email send). Same # comment applies to original_obj method calls below. original_obj.submit(updated_domain_application=obj) elif obj.status == models.DomainApplication.INVESTIGATING: diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 71745b17c..10c9c7d67 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -500,9 +500,9 @@ class DomainApplication(TimeStampedModel): @transition(field="status", source=[STARTED, WITHDRAWN], target=SUBMITTED) def submit(self, updated_domain_application=None): """Submit an application that is started. - + As a side effect, an email notification is sent. - + This method is called in admin.py on the original application which has the correct status value, but is passed the changed application which has the up-to-date data that we'll use @@ -543,7 +543,7 @@ class DomainApplication(TimeStampedModel): """Investigate an application that has been submitted. As a side effect, an email notification is sent. - + This method is called in admin.py on the original application which has the correct status value, but is passed the changed application which has the up-to-date data that we'll use @@ -563,7 +563,7 @@ class DomainApplication(TimeStampedModel): object for the approved Domain and makes the user who created the application into an admin on that domain. It also triggers an email notification. - + This method is called in admin.py on the original application which has the correct status value, but is passed the changed application which has the up-to-date data that we'll use From 33e39bbe7027e202d76df30dc36b4790fa23ae58 Mon Sep 17 00:00:00 2001 From: Mohammad Minaie Date: Fri, 23 Jun 2023 16:03:31 -0400 Subject: [PATCH 14/16] update from main, add optional removal from brandon, and fix title --- .github/ISSUE_TEMPLATE/issue-default.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/issue-default.yml b/.github/ISSUE_TEMPLATE/issue-default.yml index ea12b5658..35c816e35 100644 --- a/.github/ISSUE_TEMPLATE/issue-default.yml +++ b/.github/ISSUE_TEMPLATE/issue-default.yml @@ -25,9 +25,9 @@ body: - type: textarea id: issue-links attributes: - label: Issue Links (optional) + label: Issue Links description: | - Does this issue relate to any other existing work? + What other issues does this story relate to and how? Example: - 🚧 Blocked by: #123 From 05c755be96d31e08aaafc667a0c257de3e69664c Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Fri, 23 Jun 2023 16:37:07 -0400 Subject: [PATCH 15/16] add one last else and a logger warning for unknown status transition in django admin --- src/registrar/admin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index fee5033af..2c65d2125 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -108,6 +108,8 @@ class DomainApplicationAdmin(AuditedAdmin): original_obj.approve(updated_domain_application=obj) elif obj.status == models.DomainApplication.WITHDRAWN: original_obj.withdraw() + else: + logger.warning("Unknown status selected in django admin") super().save_model(request, obj, form, change) From 849bdfac5da0e6c03ce78cd0979d138e26f40bc9 Mon Sep 17 00:00:00 2001 From: rachidatecs Date: Wed, 28 Jun 2023 17:11:48 -0400 Subject: [PATCH 16/16] Fix typo in subject line argument passed to the email sending method for approved domains --- src/registrar/models/domain_application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 10c9c7d67..a50cb34b8 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -598,7 +598,7 @@ class DomainApplication(TimeStampedModel): self._send_status_update_email( "application approved", "emails/status_change_approved.txt", - "emails/status_change_approved.txt", + "emails/status_change_approved_subject.txt", ) @transition(field="status", source=[SUBMITTED, INVESTIGATING], target=WITHDRAWN)