Merge remote-tracking branch 'origin/main' into 1169-display-confirmation-summary-on-the-preview-page

2026-08-12 01:48:54 -04:00 · 2024-02-16 11:50:01 -08:00
parent eccf8cb3e7 7471cefd2b
commit 44ec565d57
10 changed files with 170 additions and 110 deletions
--- a/.github/pull_request_template.md
+++ b/.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.
--- a/app/init.py
+++ b/app/init.py
@@ -381,6 +381,10 @@ def load_service_before_request():
                request_ctx.service = Service(
                    service_api_client.get_service(service_id)["data"]
                )
+                stats = service_api_client.get_service_statistics(
+                    service_id, limit_days=7
+                )
+                request_ctx.service.stats = stats
            except HTTPError as exc:
                # if service id isn't real, then 404 rather than 500ing later because we expect service to be set
                if exc.status_code == 404:
--- a/app/assets/javascripts/collapsibleCheckboxes.js
+++ b/app/assets/javascripts/collapsibleCheckboxes.js
@@ -65,7 +65,7 @@

    return $(`<div class="selection-footer${stickyClass}">
              <button
-                class="govuk-button govuk-button--secondary selection-footer__button"
+                class="govuk-button govuk-button--secondary selection-footer__button usa-button usa-button--outline"
                aria-expanded="${expanded ? 'true' : 'false'}"
                aria-controls="${this.fieldsetId}">
              ${buttonContent}
--- a/app/assets/sass/uswds/_uswds-theme-custom-styles.scss
+++ b/app/assets/sass/uswds/_uswds-theme-custom-styles.scss
@@ -140,15 +140,17 @@ td.table-empty-message {
 }

 .user-list {
-   display: inline-flex;
+   display: flex;
   flex-wrap: wrap;
-   gap: units(4);
+   justify-content: space-between;
+   padding: 1rem;
   .user-list-item {
      @include at-media(desktop) {
-         flex: 1 1 calc(50% - units(2));
-         flex-grow: 0;
+         width: calc(50% - units(1));
+         margin-bottom: 1rem;
+         box-sizing: border-box;
      }
-      flex: 1 1 calc(100% - units(2));
+      // flex: 1 1 calc(100% - units(2));
      border: 1px solid color('gray-cool-10');
      padding: units(2);
      .tick-cross-list-permissions {
@@ -354,6 +356,7 @@ td.table-empty-message {
   }
   .file-list-hint {
      margin: 0;
+      word-break: break-word;
   }
   .table-field, .table-field-right-aligned {
      width: 50%;
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -70,7 +70,7 @@ from app.utils.user_permissions import all_ui_permissions, permission_options
 def get_time_value_and_label(future_time):
    preferred_tz = pytz.timezone(get_user_preferred_timezone())
    return (
-        future_time.astimezone(preferred_tz).replace(tzinfo=None).isoformat(),
+        future_time.astimezone(preferred_tz).isoformat(),
        "{} at {} {}".format(
            get_human_day(future_time.astimezone(preferred_tz)),
            get_human_time(future_time.astimezone(preferred_tz)),
@@ -1253,7 +1253,6 @@ class ChangePreferredTimezoneForm(StripWhitespaceForm):

    new_preferred_timezone = GovukRadiosField(
        "What timezone would you like to use?",
-        default="US/Eastern",
    )


--- a/app/utils/csv.py
+++ b/app/utils/csv.py
@@ -78,32 +78,29 @@ def generate_notifications_csv(**kwargs):
            template=get_sample_template(kwargs["template_type"]),
        )
        original_column_headers = original_upload.column_headers
-        fieldnames = (
-            ["Row number"]
-            + original_column_headers
-            + [
-                "Template",
-                "Type",
-                "Sent by",
-                "Job",
-                "Carrier",
-                "Carrier Response",
-                "Status",
-                "Time",
-            ]
-        )
-    else:
        fieldnames = [
-            "Recipient",
            "Template",
-            "Type",
            "Sent by",
-            "Job",
-            "Carrier",
+            "Batch File",
            "Carrier Response",
            "Status",
            "Time",
        ]
+        for header in original_column_headers:
+            fieldnames.append(header)
+
+    else:
+        # TODO This is deprecated because everything should be a job now, is it ever invoked?
+        fieldnames = [
+            "Recipient",
+            "Template",
+            "Sent by",
+            "Batch File",
+            "Carrier Response",
+            "Status",
+            "Time",
+        ]
+        current_app.logger.warning("Invoking deprecated report format")

    yield ",".join(fieldnames) + "\n"

@@ -118,33 +115,26 @@ def generate_notifications_csv(**kwargs):

            current_app.logger.info(f"\n\n{notification}")
            if kwargs.get("job_id"):
-                values = (
-                    [
-                        notification["row_number"],
-                    ]
-                    + [
+                values = [
+                    notification["template_name"],
+                    notification["created_by_name"],
+                    notification["job_name"],
+                    notification["provider_response"],
+                    notification["status"],
+                    preferred_tz_created_at,
+                ]
+                for header in original_column_headers:
+                    values.append(
                        original_upload[notification["row_number"] - 1].get(header).data
-                        for header in original_column_headers
-                    ]
-                    + [
-                        notification["template_name"],
-                        notification["template_type"],
-                        notification["created_by_name"],
-                        notification["job_name"],
-                        notification["carrier"],
-                        notification["provider_response"],
-                        notification["status"],
-                        preferred_tz_created_at,
-                    ]
-                )
+                    )
+
            else:
+                # TODO This is deprecated, should not be invoked.  See above
                values = [
                    notification["recipient"],
                    notification["template_name"],
-                    notification["template_type"],
                    notification["created_by_name"] or "",
                    notification["job_name"] or "",
-                    notification["carrier"],
                    notification["provider_response"],
                    notification["status"],
                    preferred_tz_created_at,
--- a/tests/app/main/forms/test_choose_time_form.py
+++ b/tests/app/main/forms/test_choose_time_form.py
@@ -10,21 +10,21 @@ def test_form_contains_next_24h(notify_admin):

    # Friday
    assert choices[0] == ("", "Now")
-    assert choices[1] == ("2016-01-01T07:00:00", "Today at 7am US/Eastern")
-    assert choices[13] == ("2016-01-01T19:00:00", "Today at 7pm US/Eastern")
+    assert choices[1] == ("2016-01-01T07:00:00-05:00", "Today at 7am US/Eastern")
+    assert choices[13] == ("2016-01-01T19:00:00-05:00", "Today at 7pm US/Eastern")

    # Saturday
-    assert choices[14] == ("2016-01-01T20:00:00", "Today at 8pm US/Eastern")
-    assert choices[37] == ("2016-01-02T19:00:00", "Tomorrow at 7pm US/Eastern")
+    assert choices[14] == ("2016-01-01T20:00:00-05:00", "Today at 8pm US/Eastern")
+    assert choices[37] == ("2016-01-02T19:00:00-05:00", "Tomorrow at 7pm US/Eastern")

    # Sunday
-    assert choices[38] == ("2016-01-02T20:00:00", "Tomorrow at 8pm US/Eastern")
+    assert choices[38] == ("2016-01-02T20:00:00-05:00", "Tomorrow at 8pm US/Eastern")

    # Monday
-    assert choices[62] == ("2016-01-03T20:00:00", "Sunday at 8pm US/Eastern")
-    assert choices[80] == ("2016-01-04T14:00:00", "Monday at 2pm US/Eastern")
-    assert choices[84] == ("2016-01-04T18:00:00", "Monday at 6pm US/Eastern")
-    assert choices[85] == ("2016-01-04T19:00:00", "Monday at 7pm US/Eastern")
+    assert choices[62] == ("2016-01-03T20:00:00-05:00", "Sunday at 8pm US/Eastern")
+    assert choices[80] == ("2016-01-04T14:00:00-05:00", "Monday at 2pm US/Eastern")
+    assert choices[84] == ("2016-01-04T18:00:00-05:00", "Monday at 6pm US/Eastern")
+    assert choices[85] == ("2016-01-04T19:00:00-05:00", "Monday at 7pm US/Eastern")

    with pytest.raises(IndexError):
        assert choices[
--- a/tests/app/main/views/test_api_integration.py
+++ b/tests/app/main/views/test_api_integration.py
@@ -325,6 +325,7 @@ def test_route_permissions(
        def _get(mocker):
            return {"count": 0}

+        mocker.patch("app.service_api_client.get_service_statistics")
        mocker.patch(
            "app.service_api_client.get_global_notification_count", side_effect=_get
        )
@@ -358,6 +359,8 @@ def test_route_invalid_permissions(
        def _get(mocker):
            return {"count": 0}

+        mocker.patch("app.service_api_client.get_service_statistics")
+
        mocker.patch(
            "app.service_api_client.get_global_notification_count", side_effect=_get
        )
--- a/tests/app/utils/test_csv.py
+++ b/tests/app/utils/test_csv.py
@@ -18,7 +18,6 @@ def _get_notifications_csv(
    template_name="foo",
    template_type="sms",
    job_name="bar.csv",
-    carrier="ATT Mobility",
    provider_response="Did not like it",
    status="Delivered",
    created_at="1943-04-19 12:00:00",
@@ -46,20 +45,19 @@ def _get_notifications_csv(
            "notifications": [
                {
                    "row_number": row_number + i,
-                    "to": recipient,
-                    "recipient": recipient,
-                    "client_reference": "ref 1234",
                    "template_name": template_name,
                    "template_type": template_type,
                    "template": {"name": template_name, "template_type": template_type},
                    "job_name": job_name,
-                    "carrier": carrier,
                    "provider_response": provider_response,
                    "status": status,
                    "created_at": created_at,
                    "updated_at": None,
                    "created_by_name": created_by_name,
                    "created_by_email_address": created_by_email_address,
+                    "to": recipient,
+                    "recipient": recipient,
+                    "client_reference": "ref 1234",
                }
                for i in range(rows)
            ],
@@ -90,15 +88,15 @@ def get_notifications_csv_mock(
        (
            None,
            [
-                "Recipient,Template,Type,Sent by,Job,Carrier,Carrier Response,Status,Time\n",
-                "foo@bar.com,foo,sms,,,ATT Mobility,Did not like it,Delivered,1943-04-19 08:00:00 AM US/Eastern\r\n",
+                "Recipient,Template,Sent by,Batch File,Carrier Response,Status,Time\n",
+                "foo@bar.com,foo,,,Did not like it,Delivered,1943-04-19 08:00:00 AM US/Eastern\r\n",
            ],
        ),
        (
            "Anne Example",
            [
-                "Recipient,Template,Type,Sent by,Job,Carrier,Carrier Response,Status,Time\n",
-                "foo@bar.com,foo,sms,Anne Example,,ATT Mobility,Did not like it,Delivered,1943-04-19 08:00:00 AM US/Eastern\r\n",  # noqa
+                "Recipient,Template,Sent by,Batch File,Carrier Response,Status,Time\n",
+                "foo@bar.com,foo,Anne Example,,Did not like it,Delivered,1943-04-19 08:00:00 AM US/Eastern\r\n",  # noqa
            ],
        ),
    ],
@@ -130,28 +128,22 @@ def test_generate_notifications_csv_without_job(
            2028675309
        """,
            [
-                "Row number",
-                "phone_number",
                "Template",
-                "Type",
                "Sent by",
-                "Job",
-                "Carrier",
+                "Batch File",
                "Carrier Response",
                "Status",
                "Time",
+                "phone_number",
            ],
            [
-                "1",
-                "2028675309",
                "foo",
-                "sms",
                "Fake Person",
                "bar.csv",
-                "ATT Mobility",
                "Did not like it",
                "Delivered",
                "1943-04-19 08:00:00 AM US/Eastern",
+                "2028675309",
            ],
        ),
        (
@@ -160,34 +152,28 @@ def test_generate_notifications_csv_without_job(
            2028675309,  🐜,🐝,🦀
        """,
            [
-                "Row number",
+                "Template",
+                "Sent by",
+                "Batch File",
+                "Carrier Response",
+                "Status",
+                "Time",
                "phone_number",
                "a",
                "b",
                "c",
-                "Template",
-                "Type",
-                "Sent by",
-                "Job",
-                "Carrier",
-                "Carrier Response",
-                "Status",
-                "Time",
            ],
            [
-                "1",
+                "foo",
+                "Fake Person",
+                "bar.csv",
+                "Did not like it",
+                "Delivered",
+                "1943-04-19 08:00:00 AM US/Eastern",
                "2028675309",
                "🐜",
                "🐝",
                "🦀",
-                "foo",
-                "sms",
-                "Fake Person",
-                "bar.csv",
-                "ATT Mobility",
-                "Did not like it",
-                "Delivered",
-                "1943-04-19 08:00:00 AM US/Eastern",
            ],
        ),
        (
@@ -196,34 +182,28 @@ def test_generate_notifications_csv_without_job(
            "2028675309","🐜,🐜","🐝,🐝","🦀"
        """,
            [
-                "Row number",
+                "Template",
+                "Sent by",
+                "Batch File",
+                "Carrier Response",
+                "Status",
+                "Time",
                "phone_number",
                "a",
                "b",
                "c",
-                "Template",
-                "Type",
-                "Sent by",
-                "Job",
-                "Carrier",
-                "Carrier Response",
-                "Status",
-                "Time",
            ],
            [
-                "1",
+                "foo",
+                "Fake Person",
+                "bar.csv",
+                "Did not like it",
+                "Delivered",
+                "1943-04-19 08:00:00 AM US/Eastern",
                "2028675309",
                "🐜,🐜",
                "🐝,🐝",
                "🦀",
-                "foo",
-                "sms",
-                "Fake Person",
-                "bar.csv",
-                "ATT Mobility",
-                "Did not like it",
-                "Delivered",
-                "1943-04-19 08:00:00 AM US/Eastern",
            ],
        ),
    ],
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2319,6 +2319,8 @@ def client_request(logged_in_client, mocker, service_one):  # noqa (C901 too com
    def _get(mocker):
        return {"count": 0}

+    mocker.patch("app.service_api_client.get_service_statistics")
+
    mocker.patch(
        "app.service_api_client.get_global_notification_count", side_effect=_get
    )