Merge pull request #807 from GSA/main

Production Deploy 2/20/2024

.github/pull_request_template.md

@@ -0,0 +1,79 @@
+<!--
+Please follow the instructions found in this pull request template so that we
+have all of the relevant details needed for our work.
+
+At the minimum, please be sure to fill in all sections found below and also do
+the following:
+
+- Provide an appropriate and descriptive title for the pull request
+- Link the pull request to its corresponding issue (must be done after creating
+  the pull request itself)
+- Assign yourself as the author
+- Attach the appropriate labels to it
+- Set it to be on the Notify.gov project board
+- Select one or more reviewers from the team or mark the pull request as a draft
+  depending on its current state
+   - If the pull request is a draft, please be sure to add reviewers once it is
+     ready for review and mark it ready for review
+
+For each section, please delete the instructions/sample text (that includes this
+text, though it is wrapped in an HTML comment just in case) and put in your own
+information.  Thank you!
+-->
+
+*A note to PR reviewers: it may be helpful to review our
+[code review documentation](https://github.com/GSA/notifications-api/blob/main/docs/all.md#code-reviews)
+to know what to keep in mind while reviewing pull requests.*
+
+## Description
+
+Please enter a clear description about your proposed changes and what the
+expected outcome(s) is/are from there.  If there are complex implementation
+details within the changes, this is a great place to explain those details using
+plain language.
+
+This should include:
+
+- Links to issues that this PR addresses
+- Screenshots or screen captures of any visible changes, especially for UI work
+- Dependency changes
+
+If there are any caveats, known issues, follow-up items, etc., make a quick note
+of them here as well, though more details are probably warranted in the issue
+itself in this case.
+
+## TODO (optional)
+
+If you're opening a draft PR, it might be helpful to list any outstanding work,
+especially if you're asking folks to take a look before it's ready for full
+review.  In this case, create a small checklist with the outstanding items:
+
+- [ ] TODO item 1
+- [ ] TODO item 2
+- [ ] TODO item ...
+
+## Security Considerations
+
+Please think about the security compliance aspect of your changes and what the
+potential impacts might be.
+
+**NOTE:  Please be mindful of sharing sensitive information here!  If you're not
+sure of what to write, please ask the team first before writing anything here.**
+
+Relevant details could include (and are not limited to) the following:
+
+- Handling secrets/credential management (or specifically calling out that there
+  is nothing to handle)
+- Any adjustments to the flow of data in and out the system, or even within it
+- Connecting or disconnecting any external services to the application
+- Handling of any sensitive information, such as PII
+- Handling of information within log statements or other application monitoring
+  services/hooks
+- The inclusion of a new external dependency or the removal of an existing one
+- ... (anything else relevant from a security compliance perspective)
+
+There are some cases where there are no security considerations to be had, e.g.,
+updating our documentation with publicly available information.  In those cases
+it is fine to simply put something like this:
+
+- None; this is a documentation update with publicly available information.

app/clients/cloudwatch/aws_cloudwatch.py

@@ -90,23 +90,39 @@ class AwsCloudwatchClient(Client):
         account_number = ses_domain_arn.split(":")
         return account_number
 
+    def warn_if_dev_is_opted_out(self, provider_response, notification_id):
+        if (
+            "is opted out" in provider_response.lower()
+            or "has blocked sms" in provider_response.lower()
+        ):
+            if os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]:
+                ansi_red = "\033[31m"
+                ansi_reset = "\033[0m"
+                logline = (
+                    ansi_red
+                    + f"The phone number for notification_id {notification_id} is OPTED OUT. You need to opt back in"
+                    + ansi_reset
+                )
+                current_app.logger.warning(logline)
+                return logline
+        return None
+
     def check_sms(self, message_id, notification_id, created_at):
-        current_app.logger.info(f"CREATED AT = {created_at}")
         region = cloud_config.sns_region
         # TODO this clumsy approach to getting the account number will be fixed as part of notify-api #258
         account_number = self._extract_account_number(cloud_config.ses_domain_arn)
 
         time_now = datetime.utcnow()
         log_group_name = f"sns/{region}/{account_number[4]}/DirectPublishToPhoneNumber"
-        current_app.logger.info(
-            f"Log group name: {log_group_name} message id: {message_id}"
-        )
         filter_pattern = '{$.notification.messageId="XXXXX"}'
         filter_pattern = filter_pattern.replace("XXXXX", message_id)
         all_log_events = self._get_log(filter_pattern, log_group_name, created_at)
         if all_log_events and len(all_log_events) > 0:
             event = all_log_events[0]
             message = json.loads(event["message"])
+            self.warn_if_dev_is_opted_out(
+                message["delivery"]["providerResponse"], notification_id
+            )
             return (
                 "success",
                 message["delivery"]["providerResponse"],
@@ -116,13 +132,13 @@ class AwsCloudwatchClient(Client):
         log_group_name = (
             f"sns/{region}/{account_number[4]}/DirectPublishToPhoneNumber/Failure"
         )
-        current_app.logger.info(f"Failure log group name: {log_group_name}")
         all_failed_events = self._get_log(filter_pattern, log_group_name, created_at)
         if all_failed_events and len(all_failed_events) > 0:
-            current_app.logger.info("SHOULD RETURN FAILED BECAUSE WE FOUND A FAILURE")
             event = all_failed_events[0]
             message = json.loads(event["message"])
-            current_app.logger.info(f"MESSAGE {message}")
+            self.warn_if_dev_is_opted_out(
+                message["delivery"]["providerResponse"], notification_id
+            )
             return (
                 "failure",
                 message["delivery"]["providerResponse"],

docs/all.md

@@ -36,17 +36,20 @@
   - [Celery scheduled tasks](#celery-scheduled-tasks)
 - [Notify.gov](#notifygov)
   - [System Description](#system-description)
+- [Code Reviews](#code-reviews)
+  - [For the reviewer](#for-the-reviewer)
+  - [For the author](#for-the-author)
 - [Run Book](#run-book)
-  - [ Alerts, Notifications, Monitoring](#-alerts-notifications-monitoring)
+  - [Alerts, Notifications, Monitoring](#-alerts-notifications-monitoring)
-  - [ Restaging Apps](#-restaging-apps)
+  - [Restaging Apps](#-restaging-apps)
-  - [ Smoke-testing the App](#-smoke-testing-the-app)
+  - [Smoke-testing the App](#-smoke-testing-the-app)
-  - [ Simulated bulk send testing](#-simulated-bulk-send-testing)
+  - [Simulated bulk send testing](#-simulated-bulk-send-testing)
-  - [ Configuration Management](#-configuration-management)
+  - [Configuration Management](#-configuration-management)
-  - [ DNS Changes](#-dns-changes)
+  - [DNS Changes](#-dns-changes)
   - [Exporting test results for compliance monitoring](#exporting-test-results-for-compliance-monitoring)
-  - [ Known Gotchas](#-known-gotchas)
+  - [Known Gotchas](#-known-gotchas)
-  - [ User Account Management](#-user-account-management)
+  - [User Account Management](#-user-account-management)
-  - [ SMS Phone Number Management](#-sms-phone-number-management)
+  - [SMS Phone Number Management](#-sms-phone-number-management)
 - [Data Storage Policies \& Procedures](#data-storage-policies--procedures)
   - [Potential PII Locations](#potential-pii-locations)
   - [Data Retention Policy](#data-retention-policy)
@@ -711,10 +714,6 @@ make run-celery-beat
 ```
 
 
-
-
-
-
 Notify.gov
 =========
 
@@ -755,6 +754,102 @@ Notify.gov also provisions and uses two AWS services via a [supplemental service
 For further details of the system and how it connects to supporting services, see the [application boundary diagram](https://github.com/GSA/us-notify-compliance/blob/main/diagrams/rendered/apps/application.boundary.png)
 
 
+Code Reviews
+============
+
+When conducting a code review there are several things to keep in mind to ensure
+a quality and valuable review.  Remember, we're trying to improve Notify.gov as
+best we can; it does us no good if we do not double check that our work meets
+our standards, especially before going out the door!
+
+It also does us no good if we do not treat each other without mutual respect or
+consideration either; if there are mistakes or oversights found in a pull
+request, or even just suggestions for alternative ways of approaching something,
+these become learning opportunities for all parties involved in addition to
+modeling positive behavior and practices for the public and broader open source
+community.
+
+Given this basis of approaching code reviews, here are some general guidelines
+and suggestions for how to approach a code review from the perspectives of both
+the reviewer and the author.
+
+### For the reviewer
+
+When performing a code review, please be curious and critical while also being
+respectful and appreciative of the work submitted.  Code reviews are a chance
+to check that things meet our standards and provide learning opportunities.
+They are not places for belittling or disparaging someone's work or approach to
+a task, and absolutely not the person(s) themselves.
+
+That said, any responses to the code review should also be respectful and
+considerate.  Remember, this is a chance to not only improve our work and the
+state of Notify.gov, it's also a chance to learn something new!
+
+**Note: If a response is condescending, derogatory, disrespectful, etc., please
+do not hesitate to either speak with the author(s) directly about this or reach
+out to a team lead/supervisor for additional help to rectify the issue.  Such
+behavior and lack of professionalism is not acceptable or tolerated.**
+
+When performing a code review, it is helpful to keep the following guidelines in
+mind:
+
+- Be on the lookout for any sensitive information and/or leaked credentials,
+  secrets, PII, etc.
+- Ask and call out things that aren't clear to you; it never hurts to double
+  check your understanding of something!
+- Check that things are named descriptively and appropriately and call out
+  anything that is not.
+- Check that comments are present for complex areas when needed.
+- Make sure the pull request itself is properly prepared - it has a clear
+  description, calls out security concerns, and has the necessary labels, flags,
+  issue link, etc., set on it.
+- Do not be shy about using the suggested changes feature in GitHub pull request
+  comments; this can help save a lot of time!
+- Do not be shy about marking a review with the `Request Changes` status - yes,
+  it looks big and red when it shows up, but this is completely fine and not to
+  be taken as a personal mark against the author(s) of the pull request!
+
+Additionally, if you find yourself making a lot of comments and/or end up having
+several concerns about the overall approach, it will likely be helpful to
+schedule time to speak with the author(s) directly and talk through everything.
+This can save folks a lot of misunderstanding and back-and-forth!
+
+### For the author
+
+When receiving a code review, please remember that someone took the time to look
+over all of your work with a critical eye to make sure our standards are being
+met and that we're producing the best quality work possible.  It's completely
+fine if there are specific changes requested and/or other parts are sent back
+for additional work!
+
+That said, the review should also be respectful, helpful, and a learning
+opportunity where possible.  Remember, this is a chance to not only improve your
+work and the state of Notify.gov, it's also a chance to learn something new!
+
+**Note: If a review is condescending, derogatory, disrespectful, etc., please do
+not hesitate to either speak with the reviewer(s) directly about this or reach
+out to a team lead/supervisor for additional help to rectify the issue.  Such
+behavior and lack of professionalism is not acceptable or tolerated.**
+
+When going over a review, it may be helpful to keep these perspectives in mind:
+
+- Approach the review with an open mind, curiosity, and appreciation.
+- If anything the reviewer(s) mentions is unclear to you, please ask for
+  clarification and engage them in further dialogue!
+- If you disagree with a suggestion or request, please say so and engage in an
+  open and respecful dialogue to come to a mutual understanding of what the
+  appropriate next step(S) should be - accept the change, reject the change,
+  take a different path entirely, etc.
+- If there are no issues with any suggested edits or requested changes, make
+  the necessary adjustments and let the reviewer(s) know when the work is ready
+  for review again.
+
+Additionally, if you find yourself responding to a lot of things and questioning
+the feedback received throughout much of the code review, it will likely be
+helpful to schedule time to speak with the reviewer(s) directly and talk through
+everything.  This can save folks a lot of misunderstanding and back-and-forth!
+
+
 Run Book
 ========
 

tests/app/clients/test_aws_cloudwatch.py

@@ -1,6 +1,7 @@
 # import pytest
 from datetime import datetime
 
+import pytest
 from flask import current_app
 
 from app import aws_cloudwatch_client
@@ -54,6 +55,27 @@ def side_effect(filterPattern, logGroupName, startTime, endTime):
         return {"events": []}
 
 
+@pytest.mark.parametrize(
+    "response, notify_id, expected_message",
+    [
+        (
+            "Phone has blocked SMS",
+            "abc",
+            "\x1b[31mThe phone number for notification_id abc is OPTED OUT. You need to opt back in\x1b[0m",
+        ),
+        (
+            "Some phone is opted out",
+            "xyz",
+            "\x1b[31mThe phone number for notification_id xyz is OPTED OUT. You need to opt back in\x1b[0m",
+        ),
+        ("Phone is A-OK", "123", None),
+    ],
+)
+def test_warn_if_dev_is_opted_out(response, notify_id, expected_message):
+    result = aws_cloudwatch_client.warn_if_dev_is_opted_out(response, notify_id)
+    assert result == expected_message
+
+
 def test_check_sms_success(notify_api, mocker):
     aws_cloudwatch_client.init_app(current_app)
     boto_mock = mocker.patch.object(aws_cloudwatch_client, "_client", create=True)