Merge pull request #1926 from GSA/job_details

fix job details

app/job/rest.py

@@ -5,6 +5,9 @@ from flask import Blueprint, current_app, jsonify, request
 
 from app import db
 from app.aws.s3 import (
+    extract_personalisation,
+    extract_phones,
+    get_job_from_s3,
     get_job_metadata_from_s3,
     get_personalisation_from_s3,
     get_phone_number_from_s3,
@@ -36,7 +39,7 @@ from app.schemas import (
     notification_with_template_schema,
     notifications_filter_schema,
 )
-from app.utils import check_suspicious_id, midnight_n_days_ago, pagination_links
+from app.utils import check_suspicious_id, hilite, midnight_n_days_ago, pagination_links
 
 job_blueprint = Blueprint("job", __name__, url_prefix="/service/<uuid:service_id>/job")
 
@@ -46,6 +49,7 @@ register_errors(job_blueprint)
 
 @job_blueprint.route("/<job_id>", methods=["GET"])
 def get_job_by_service_and_job_id(service_id, job_id):
+    current_app.logger.info(hilite("ENTER get_job_by_service_and_job_id"))
     check_suspicious_id(service_id, job_id)
     job = dao_get_job_by_service_id_and_job_id(service_id, job_id)
     statistics = dao_get_notification_outcomes_for_job(service_id, job_id)
@@ -71,8 +75,12 @@ def cancel_job(service_id, job_id):
 
 @job_blueprint.route("/<job_id>/notifications", methods=["GET"])
 def get_all_notifications_for_service_job(service_id, job_id):
+
     check_suspicious_id(service_id, job_id)
 
+    job = get_job_from_s3(service_id, job_id)
+    phones = extract_phones(job, service_id, job_id)
+    personalisation = extract_personalisation(job)
     data = notifications_filter_schema.load(request.args)
     page = data["page"] if "page" in data else 1
     page_size = (
@@ -90,21 +98,13 @@ def get_all_notifications_for_service_job(service_id, job_id):
 
     for notification in paginated_notifications.items:
         if notification.job_id is not None:
-            recipient = get_phone_number_from_s3(
+            recipient = phones[notification.job_row_number]
-                notification.service_id,
-                notification.job_id,
-                notification.job_row_number,
-            )
             notification.to = recipient
             notification.normalised_to = recipient
 
     for notification in paginated_notifications.items:
         if notification.job_id is not None:
-            notification.personalisation = get_personalisation_from_s3(
+            notification.personalisation = personalisation[notification.job_row_number]
-                notification.service_id,
-                notification.job_id,
-                notification.job_row_number,
-            )
 
     notifications = None
     if data.get("format_for_csv"):
@@ -116,6 +116,7 @@ def get_all_notifications_for_service_job(service_id, job_id):
         notifications = notification_with_template_schema.dump(
             paginated_notifications.items, many=True
         )
+        current_app.logger.info(hilite("Got the dumped notifications and returning"))
 
     return (
         jsonify(
@@ -134,6 +135,8 @@ def get_all_notifications_for_service_job(service_id, job_id):
 
 @job_blueprint.route("/<job_id>/recent_notifications", methods=["GET"])
 def get_recent_notifications_for_service_job(service_id, job_id):
+
+    current_app.logger.info(hilite("ENTER get_recent_notifications_for_service_job"))
     check_suspicious_id(service_id, job_id)
 
     data = notifications_filter_schema.load(request.args)
@@ -220,7 +223,9 @@ def get_jobs_by_service(service_id):
     else:
         limit_days = None
 
-    use_processing_time = request.args.get("use_processing_time", "false").lower() == "true"
+    use_processing_time = (
+        request.args.get("use_processing_time", "false").lower() == "true"
+    )
 
     valid_statuses = set(JobStatus)
     statuses_arg = request.args.get("statuses", "")
@@ -332,6 +337,8 @@ def get_paginated_jobs(
     statuses,
     page,
 ):
+
+    current_app.logger.info(hilite("ENTER get_paginated_jobs"))
     pagination = dao_get_jobs_by_service_id(
         service_id,
         limit_days=limit_days,

tests/app/job/test_rest.py

@@ -459,11 +459,18 @@ def _setup_jobs(template, number_of_jobs=5):
 def test_get_all_notifications_for_job_in_order_of_job_number(
     admin_request, sample_template, mocker
 ):
-    mock_s3 = mocker.patch("app.job.rest.get_phone_number_from_s3")
-    mock_s3.return_value = "15555555555"
 
-    mock_s3_personalisation = mocker.patch("app.job.rest.get_personalisation_from_s3")
+    mock_job = mocker.patch("app.job.rest.get_job_from_s3")
-    mock_s3_personalisation.return_value = {}
+    mock_job.return_value = None
+    mock_s3 = mocker.patch("app.job.rest.extract_phones")
+    mock_s3.return_value = {
+        0: "15555555555",
+        1: "15555555555",
+        2: "15555555555",
+        3: "15555555555",
+    }
+    mock_s3_personalisation = mocker.patch("app.job.rest.extract_personalisation")
+    mock_s3_personalisation.return_value = {0: "", 1: "", 2: "", 3: ""}
 
     main_job = create_job(sample_template)
     another_job = create_job(sample_template)
@@ -531,12 +538,17 @@ def test_get_recent_notifications_for_job_in_reverse_order_of_job_number(
 
 
 @pytest.mark.parametrize(
-    "expected_notification_count, status_args",
+    "expected_notification_count, status_args, expected_phones, expected_personalisation",
     [
-        (1, [NotificationStatus.CREATED]),
+        (1, [NotificationStatus.CREATED], {0: "15555555555"}, {0: ""}),
-        (0, [NotificationStatus.SENDING]),
+        (0, [NotificationStatus.SENDING], {}, {}),
-        (1, [NotificationStatus.CREATED, NotificationStatus.SENDING]),
+        (
-        (0, [NotificationStatus.SENDING, NotificationStatus.DELIVERED]),
+            1,
+            [NotificationStatus.CREATED, NotificationStatus.SENDING],
+            {0: "15555555555"},
+            {0: ""},
+        ),
+        (0, [NotificationStatus.SENDING, NotificationStatus.DELIVERED], {}, {}),
     ],
 )
 def test_get_all_notifications_for_job_filtered_by_status(
@@ -544,15 +556,24 @@ def test_get_all_notifications_for_job_filtered_by_status(
     sample_job,
     expected_notification_count,
     status_args,
+    expected_phones,
+    expected_personalisation,
     mocker,
 ):
-    mock_s3 = mocker.patch("app.job.rest.get_phone_number_from_s3")
-    mock_s3.return_value = "15555555555"
 
-    mock_s3_personalisation = mocker.patch("app.job.rest.get_personalisation_from_s3")
+    mock_job = mocker.patch("app.job.rest.get_job_from_s3")
-    mock_s3_personalisation.return_value = {}
+    mock_job.return_value = None
+    mock_s3 = mocker.patch("app.job.rest.extract_phones")
+    mock_s3.return_value = expected_phones
+    mock_s3_personalisation = mocker.patch("app.job.rest.extract_personalisation")
+    mock_s3_personalisation.return_value = expected_personalisation
 
-    create_notification(job=sample_job, to_field="1", status=NotificationStatus.CREATED)
+    create_notification(
+        job=sample_job,
+        job_row_number=0,
+        to_field="1",
+        status=NotificationStatus.CREATED,
+    )
 
     resp = admin_request.get(
         "job.get_all_notifications_for_service_job",
@@ -566,11 +587,14 @@ def test_get_all_notifications_for_job_filtered_by_status(
 def test_get_all_notifications_for_job_returns_correct_format(
     admin_request, sample_notification_with_job, mocker
 ):
-    mock_s3 = mocker.patch("app.job.rest.get_phone_number_from_s3")
-    mock_s3.return_value = "15555555555"
 
-    mock_s3_personalisation = mocker.patch("app.job.rest.get_personalisation_from_s3")
+    mock_job = mocker.patch("app.job.rest.get_job_from_s3")
-    mock_s3_personalisation.return_value = {}
+    mock_job.return_value = None
+    mock_s3 = mocker.patch("app.job.rest.extract_phones")
+    mock_s3.return_value = {0: "15555555555"}
+    mock_s3_personalisation = mocker.patch("app.job.rest.extract_personalisation")
+    mock_s3_personalisation.return_value = {0: ""}
+    sample_notification_with_job.job_row_number = 0
 
     service_id = sample_notification_with_job.service_id
     job_id = sample_notification_with_job.job_id
@@ -943,11 +967,13 @@ def create_10_jobs(template):
 def test_get_all_notifications_for_job_returns_csv_format(
     admin_request, sample_notification_with_job, mocker
 ):
-    mock_s3 = mocker.patch("app.job.rest.get_phone_number_from_s3")
+    mock_job = mocker.patch("app.job.rest.get_job_from_s3")
-    mock_s3.return_value = "15555555555"
+    mock_job.return_value = None
-
+    mock_s3 = mocker.patch("app.job.rest.extract_phones")
-    mock_s3_personalisation = mocker.patch("app.job.rest.get_personalisation_from_s3")
+    mock_s3.return_value = {0: "15555555555"}
-    mock_s3_personalisation.return_value = {}
+    mock_s3_personalisation = mocker.patch("app.job.rest.extract_personalisation")
+    mock_s3_personalisation.return_value = {0: ""}
+    sample_notification_with_job.job_row_number = 0
 
     resp = admin_request.get(
         "job.get_all_notifications_for_service_job",
@@ -955,7 +981,6 @@ def test_get_all_notifications_for_job_returns_csv_format(
         job_id=sample_notification_with_job.job_id,
         format_for_csv=True,
     )
-
     assert len(resp["notifications"]) == 1
     assert set(resp["notifications"][0].keys()) == {
         "created_at",