Add job_id to the create_service_report endpoint (#2512)

This change:
- adds the job_id to the create_service_report endpoint
- adds a filter for job_id to the sql query that creates the csv file in build_notifications_query
- adds the template_type to the data that gets returned from the get_job_by_service_and_job_id endpoint (this is a non-breaking change, just adding some data)
- adds some tests for the create_service_report endpoint, including for different notification_statuses

Author: smcmurtry

app/celery/tasks.py

@@ -905,6 +905,7 @@ def generate_report(report_id: str, notification_statuses=[]):
             notification_type=report.report_type,
             language=report.language,
             notification_statuses=notification_statuses,
+            job_id=report.job_id,
             days_limit=LIMIT_DAYS,
             s3_bucket=current_app.config["REPORTS_BUCKET_NAME"],
             s3_key=s3_key,

app/report/rest.py

@@ -42,6 +42,7 @@ def create_service_report(service_id):
             "requesting_user_id": data.get("requesting_user_id"),
             "language": data.get("language"),
             "notification_statuses": data.get("notification_statuses"),
+            "job_id": data.get("job_id"),
         }
 
         # Validate against the schema
@@ -55,6 +56,7 @@ def create_service_report(service_id):
             status=report_data["status"],
             requesting_user_id=report_data["requesting_user_id"],
             language=report_data["language"],
+            job_id=report_data["job_id"],
         )
 
         # Save the report to the database

app/report/utils.py

@@ -54,7 +54,7 @@ def translate(self, x):
         return x
 
 
-def build_notifications_query(service_id, notification_type, language, notification_statuses=[], days_limit=7):
+def build_notifications_query(service_id, notification_type, language, notification_statuses=[], job_id=None, days_limit=7):
     """
     Builds and returns an SQLAlchemy query for notifications with the specified parameters.
 
@@ -64,6 +64,7 @@ def build_notifications_query(service_id, notification_type, language, notificat
         language: "en" or "fr"
         days_limit: Number of days to look back in history
         notification_statuses: List of notification statuses to filter by
+        job_id: Optional filter by specific job ID
     Returns:
         SQLAlchemy query object for notifications
     """
@@ -84,6 +85,9 @@ def build_notifications_query(service_id, notification_type, language, notificat
         statuses = Notification.substitute_status(notification_statuses)
         query_filters.append(n.status.in_(statuses))
 
+    if job_id:
+        query_filters.append(n.job_id == job_id)
+
     inner_query = (
         db.session.query(
             n.to.label("to"),
@@ -199,7 +203,7 @@ def stream_query_to_s3(copy_command, s3_bucket, s3_key):
 
 
 def generate_csv_from_notifications(
-    service_id, notification_type, language, notification_statuses=[], days_limit=7, s3_bucket=None, s3_key=None
+    service_id, notification_type, language, notification_statuses=[], job_id=None, days_limit=7, s3_bucket=None, s3_key=None
 ):
     """
     Generate CSV using SQLAlchemy for improved compatibility and type safety, and stream it directly to S3.
@@ -217,6 +221,7 @@ def generate_csv_from_notifications(
         notification_type=notification_type,
         language=language,
         notification_statuses=notification_statuses,
+        job_id=job_id,
         days_limit=days_limit,
     )
     copy_command = compile_query_for_copy(query)

app/schemas.py

@@ -499,6 +499,11 @@ class JobSchema(BaseSchema):
     )
     sender_id = fields.UUID(required=False, allow_none=True)
 
+    template_type = fields.Method("get_template_type", dump_only=True)
+
+    def get_template_type(self, job):
+        return job.template.template_type if job.template else None
+
     @validates("scheduled_for")
     def validate_scheduled_for(self, value):
         _validate_datetime_not_in_past(value)
@@ -843,6 +848,7 @@ class Meta:
     url = fields.String()
     language = fields.String()
     notification_statuses = fields.List(fields.String(), required=False, allow_none=True)
+    job_id = fields.UUID(required=False, allow_none=True)
 
     requesting_user = fields.Nested(
         UserSchema,

tests/app/report/test_rest.py

@@ -5,22 +5,128 @@
 
 def test_create_report_succeeds_with_valid_data(mocker, admin_request, sample_service, sample_user):
     """Test that creating a report with valid data succeeds"""
+    # Mock the generate_report Celery task to avoid actual task execution
     generate_report_mock = mocker.patch("app.report.rest.generate_report.apply_async")
+
+    # Use actual database operations instead of mocking create_report
     data = {"report_type": ReportType.EMAIL.value, "requesting_user_id": str(sample_user.id), "language": "en"}
 
+    # Call the endpoint which will create a real report in the database
+    response = admin_request.post("report.create_service_report", service_id=sample_service.id, _data=data, _expected_status=201)
+
+    # Verify response contains expected data
+    assert response["data"]["report_type"] == ReportType.EMAIL.value
+    assert response["data"]["service_id"] == str(sample_service.id)
+    assert response["data"]["language"] == "en"
+    assert response["data"]["status"] == ReportStatus.REQUESTED.value
+
+    # Extract the report ID from the response
+    report_id = response["data"]["id"]
+
+    # Verify generate_report was called with correct parameters
+    generate_report_mock.assert_called_once()
+    assert str(generate_report_mock.call_args[0][0][0]) == report_id
+    assert generate_report_mock.call_args[0][0][1] == []  # Empty notification_statuses
+    assert generate_report_mock.call_args[1]["queue"] == "generate-reports"
+
+
+def test_create_report_succeeds_with_notification_statuses(mocker, admin_request, sample_service, sample_user):
+    """Test that creating a report with notification_statuses passes the data to generate_report task"""
+    # Mock the generate_report Celery task to avoid actual task execution
+    generate_report_mock = mocker.patch("app.report.rest.generate_report.apply_async")
+
+    notification_statuses = ["delivered", "sending"]
+    data = {
+        "report_type": ReportType.EMAIL.value,
+        "requesting_user_id": str(sample_user.id),
+        "notification_statuses": notification_statuses,
+        "language": "en",
+    }
+
+    # Call the endpoint which will create a real report in the database
+    response = admin_request.post("report.create_service_report", service_id=sample_service.id, _data=data, _expected_status=201)
+
+    # Verify response contains expected data
+    assert response["data"]["report_type"] == ReportType.EMAIL.value
+    assert str(response["data"]["service_id"]) == str(sample_service.id)
+    assert response["data"]["language"] == "en"
+    assert response["data"]["status"] == ReportStatus.REQUESTED.value
+
+    # Extract the report ID from the response
+    report_id = response["data"]["id"]
+
+    # Verify generate_report was called with notification_statuses
+    generate_report_mock.assert_called_once()
+    assert str(generate_report_mock.call_args[0][0][0]) == report_id
+    assert generate_report_mock.call_args[0][0][1] == notification_statuses
+    assert generate_report_mock.call_args[1]["queue"] == "generate-reports"
+
+
+def test_create_report_succeeds_with_job_id(mocker, admin_request, sample_service, sample_user, sample_job):
+    """Test that creating a report with job_id passes the data to generate_report task"""
+    # Mock the generate_report Celery task to avoid actual task execution
+    generate_report_mock = mocker.patch("app.report.rest.generate_report.apply_async")
+
+    data = {
+        "report_type": ReportType.SMS.value,
+        "requesting_user_id": str(sample_user.id),
+        "job_id": str(sample_job.id),
+        "language": "en",
+    }
+
+    # Call the endpoint which will create a real report in the database
+    response = admin_request.post("report.create_service_report", service_id=sample_service.id, _data=data, _expected_status=201)
+
+    # Verify response contains expected data
+    assert response["data"]["report_type"] == ReportType.SMS.value
+    assert str(response["data"]["service_id"]) == str(sample_service.id)
+    assert response["data"]["job_id"] == str(sample_job.id)
+    assert response["data"]["language"] == "en"
+    assert response["data"]["status"] == ReportStatus.REQUESTED.value
+
+    # Extract the report ID from the response
+    report_id = response["data"]["id"]
+
+    # Verify generate_report was called with empty notification_statuses
+    generate_report_mock.assert_called_once()
+    assert str(generate_report_mock.call_args[0][0][0]) == report_id
+    assert generate_report_mock.call_args[0][0][1] == []  # Empty notification_statuses
+    assert generate_report_mock.call_args[1]["queue"] == "generate-reports"
+
+
+def test_create_report_succeeds_with_both_notification_statuses_and_job_id(
+    mocker, admin_request, sample_service, sample_user, sample_job
+):
+    """Test that creating a report with both notification_statuses and job_id passes both parameters correctly"""
+    # Mock the generate_report Celery task to avoid actual task execution
+    generate_report_mock = mocker.patch("app.report.rest.generate_report.apply_async")
+
+    notification_statuses = ["delivered", "failed"]
+    data = {
+        "report_type": ReportType.EMAIL.value,
+        "requesting_user_id": str(sample_user.id),
+        "job_id": str(sample_job.id),
+        "notification_statuses": notification_statuses,
+        "language": "en",
+    }
+
+    # Call the endpoint which will create a real report in the database
     response = admin_request.post("report.create_service_report", service_id=sample_service.id, _data=data, _expected_status=201)
 
+    # Verify response contains expected data
     assert response["data"]["report_type"] == ReportType.EMAIL.value
     assert response["data"]["service_id"] == str(sample_service.id)
+    assert response["data"]["job_id"] == str(sample_job.id)
     assert response["data"]["language"] == "en"
     assert response["data"]["status"] == ReportStatus.REQUESTED.value
-    assert "id" in response["data"]
-    assert "requested_at" in response["data"]
 
-    # assert generate_report_mock called once with the correct report ID
+    # Extract the report ID from the response
+    report_id = response["data"]["id"]
+
+    # Verify generate_report was called with notification_statuses
     generate_report_mock.assert_called_once()
-    assert str(generate_report_mock.call_args[0][0][0]) == response["data"]["id"]
-    # check the queue name
+    assert str(generate_report_mock.call_args[0][0][0]) == report_id
+    assert generate_report_mock.call_args[0][0][1] == notification_statuses
     assert generate_report_mock.call_args[1]["queue"] == "generate-reports"
 
 

tests/app/report/test_utils.py

@@ -35,6 +35,7 @@ def test_calls_helper_functions_with_correct_parameters(self):
         s3_key = "test-key.csv"
         language = "en"
         notification_statuses = ["delivered", "failed"]
+        job_id = "job-id-1"
         # When
         with patch("app.report.utils.build_notifications_query") as mock_build_query:
             with patch("app.report.utils.compile_query_for_copy") as mock_compile_query:
@@ -44,7 +45,7 @@ def test_calls_helper_functions_with_correct_parameters(self):
 
                     # Call the function
                     generate_csv_from_notifications(
-                        service_id, notification_type, language, notification_statuses, days_limit, s3_bucket, s3_key
+                        service_id, notification_type, language, notification_statuses, job_id, days_limit, s3_bucket, s3_key
                     )
 
                     # Then
@@ -53,6 +54,7 @@ def test_calls_helper_functions_with_correct_parameters(self):
                         notification_type=notification_type,
                         language=language,
                         notification_statuses=notification_statuses,
+                        job_id=job_id,
                         days_limit=days_limit,
                     )
                     mock_compile_query.assert_called_once_with("mock query")
@@ -111,6 +113,26 @@ def test_build_notifications_query_with_empty_status_filter(self):
         sql_str = str(query.statement.compile(compile_kwargs={"literal_binds": True}))
         assert "notification.status IN" not in sql_str  # No status filter should be applied
 
+    def test_build_notifications_query_with_job_id_filter(self):
+        # Given
+        service_id = "service-id-1"
+        notification_type = "email"
+        language = "en"
+        job_id = "job-id-1"
+
+        # When
+        query = build_notifications_query(
+            service_id=service_id,
+            notification_type=notification_type,
+            language=language,
+            job_id=job_id,
+        )
+
+        # Then
+        # Convert query to string to check the SQL
+        sql_str = str(query.statement.compile(compile_kwargs={"literal_binds": True}))
+        assert f".job_id = '{job_id}'" in sql_str
+
 
 class TestEmails:
     def test_send_email_notification2(self, mocker, sample_template, sample_report, sample_service, sample_notification):
@@ -170,6 +192,7 @@ def fake_stream_query_to_s3(copy_command, s3_bucket, s3_key):
                 "email",
                 "en",
                 days_limit=7,
+                job_id=None,
                 s3_bucket="test-bucket",
                 s3_key="test-key.csv",
             )
@@ -226,6 +249,7 @@ def fake_stream_query_to_s3(copy_command, s3_bucket, s3_key):
                 "email",
                 "en",
                 notification_statuses=["delivered", "failed"],
+                job_id=None,
                 days_limit=7,
                 s3_bucket="test-bucket",
                 s3_key="test-key.csv",

tests/app/v2/notifications/test_post_notifications.py

@@ -38,6 +38,7 @@
 from app.v2.notifications.post_notifications import _seed_bounce_data
 from tests import create_authorization_header
 from tests.app.conftest import (
+    create_sample_job,
     create_sample_notification,
     create_sample_template,
     document_download_response,
@@ -945,7 +946,7 @@ def test_team_keys_only_send_to_team_members(
         ],
     )
     def test_team_keys_only_send_to_team_members_bulk_endpoint(
-        self, notify_db_session, client, mocker, notification_type, to_key, to_a, to_b, response_code
+        self, notify_db, notify_db_session, client, mocker, notification_type, to_key, to_a, to_b, response_code
     ):
         service = create_service(
             restricted=True,
@@ -956,8 +957,8 @@ def test_team_keys_only_send_to_team_members_bulk_endpoint(
         service.users = [user_1, user_2]
         template = create_template(service=service, template_type=notification_type)
         create_api_key(service=service, key_type="team")
-        job_id = str(uuid.uuid4())
-        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=job_id)
+        job = create_sample_job(notify_db=notify_db, notify_db_session=notify_db_session)
+        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=job)
 
         data = {
             "name": "job_name",
@@ -1607,7 +1608,6 @@ def test_API_BULK_post_sms_with_test_key_does_not_count_towards_limits(
     ):
         # test setup
         mocker.patch("app.sms_normal_publish.publish")
-        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=str(uuid.uuid4()))
         increment_todays_requested_sms_count = mocker.patch("app.notifications.validators.increment_todays_requested_sms_count")
 
         def __send_sms():
@@ -1640,7 +1640,8 @@ def __send_sms():
             "template_id": str(template.id),
             "rows": [["phone number"], ["+16132532222"], ["+16132532223"], ["+16132532224"]],
         }
-
+        job = create_sample_job(notify_db=notify_db, notify_db_session=notify_db_session, service=service, template=template)
+        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=job)
         response = __send_sms()
 
         assert response.status_code == 201
@@ -1653,7 +1654,6 @@ def __send_sms():
     def test_API_BULK_post_sms_with_mixed_numbers(self, notify_api, client, notify_db, notify_db_session, mocker, key_type):
         # test setup
         mocker.patch("app.sms_normal_publish.publish")
-        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=str(uuid.uuid4()))
         increment_todays_requested_sms_count = mocker.patch("app.notifications.validators.increment_todays_requested_sms_count")
 
         def __send_sms():
@@ -1681,6 +1681,9 @@ def __send_sms():
         # Create a service, Set limit to 10 fragments
         service = create_service(sms_daily_limit=10, message_limit=100)
         template = create_sample_template(notify_db, notify_db_session, content="Hello", service=service, template_type="sms")
+        job = create_sample_job(notify_db=notify_db, notify_db_session=notify_db_session, service=service, template=template)
+        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=job)
+
         data = {
             "name": "Bulk send name",
             "template_id": str(template.id),
@@ -1834,7 +1837,6 @@ def __send_sms():
     def test_API_BULK_sends_warning_emails_and_blocks_sends(self, notify_api, client, notify_db, notify_db_session, mocker):
         # test setup
         mocker.patch("app.sms_normal_publish.publish")
-        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=str(uuid.uuid4()))
         send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email")
         send_limit_reached_email = mocker.patch("app.notifications.validators.send_sms_limit_reached_email")
 
@@ -1861,6 +1863,8 @@ def __send_sms():
 
         # Create 7 notifications in the db
         template = create_sample_template(notify_db, notify_db_session, content="Hello", service=service, template_type="sms")
+        job = create_sample_job(notify_db=notify_db, notify_db_session=notify_db_session, service=service, template=template)
+        mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=job)
         for x in range(7):
             create_sample_notification(notify_db, notify_db_session, service=service)
 
@@ -2539,6 +2543,7 @@ def test_post_bulk_creates_job_and_dispatches_celery_task(
                 "service": str(sample_email_template.service_id),
                 "service_name": {"name": sample_email_template.service.name},
                 "template": str(sample_email_template.id),
+                "template_type": str(sample_email_template.template_type),
                 "template_version": sample_email_template.version,
                 "updated_at": None,
                 "sender_id": str(reply_to_email.id) if use_sender_id else None,