From 8cf8d24e3717973fd29456a2578efd9fcad7f52e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 24 Sep 2019 16:52:18 +0100 Subject: [PATCH 1/3] Return count of notifications in the database for a job When we cancel a job, we need to check if all notifications are already in the database. So far, we were querying for all notification objects in the database and counting them in admin app, which runs into pagination problems for large jobs, and could time out for very large jobs. --- app/dao/notifications_dao.py | 5 +++++ app/job/rest.py | 10 ++++++++- .../notification_dao/test_notification_dao.py | 22 +++++++++++++++++++ tests/app/job/test_rest.py | 8 +++++++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8976bbebc..7c4e1e91a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -178,6 +178,11 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page ) +@statsd(namespace="dao") +def dao_get_notification_count_for_job_id(job_id): + return Notification.query.filter_by(job_id=job_id).count() + + @statsd(namespace="dao") def get_notification_with_personalisation(service_id, notification_id, key_type): filter_dict = {'service_id': service_id, 'id': notification_id} diff --git a/app/job/rest.py b/app/job/rest.py index 145f3c004..e2abe2bcb 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -20,7 +20,7 @@ from app.dao.jobs_dao import ( from app.dao.fact_notification_status_dao import fetch_notification_statuses_for_job from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id -from app.dao.notifications_dao import get_notifications_for_job +from app.dao.notifications_dao import dao_get_notification_count_for_job_id, get_notifications_for_job from app.schemas import ( job_schema, unarchived_template_schema, @@ -108,6 +108,14 @@ def get_all_notifications_for_service_job(service_id, job_id): ), 200 +@job_blueprint.route('//notification_count', methods=['GET']) +def get_notification_count_for_job_id(service_id, job_id): + count = dao_get_notification_count_for_job_id(job_id) + return jsonify( + count=count + ), 200 + + @job_blueprint.route('', methods=['GET']) def get_jobs_by_service(service_id): if request.args.get('limit_days'): diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8bbbb98bd..2cfa369d7 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -15,6 +15,7 @@ from app.dao.notifications_dao import ( dao_get_last_notification_added_for_job_id, dao_get_last_template_usage, dao_get_notifications_by_to_field, + dao_get_notification_count_for_job_id, dao_get_scheduled_notifications, dao_timeout_notifications, dao_update_notification, @@ -554,6 +555,27 @@ def test_get_all_notifications_for_job_by_status(sample_job): assert len(notifications(filter_dict={'status': NOTIFICATION_STATUS_TYPES[:3]}).items) == 3 +def test_dao_get_notification_count_for_job_id(notify_db_session, notify_db): + service = create_service() + template = create_template(service) + job = create_job(template, notification_count=3) + for i in range(3): + create_notification(job=job) + + create_notification(template) + + assert dao_get_notification_count_for_job_id(job.id) == 3 + + +def test_dao_get_notification_count_for_job_id_only_finds_notification_already_in_db(notify_db_session, notify_db): + service = create_service() + template = create_template(service) + job = create_job(template, notification_count=3) + create_notification(template) + + assert dao_get_notification_count_for_job_id(job.id) == 0 + + def test_update_notification_sets_status(sample_notification): assert sample_notification.status == 'created' sample_notification.status = 'failed' diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index d81210491..312e46e92 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -563,6 +563,14 @@ def test_get_all_notifications_for_job_returns_correct_format( assert resp['notifications'][0]['status'] == sample_notification_with_job.status +def test_get_notification_count_for_job_id(admin_request, mocker): + mock_dao = mocker.patch('app.job.rest.dao_get_notification_count_for_job_id', return_value=3) + job_id = uuid.uuid4() + response = admin_request.get('job.get_notification_count_for_job_id', service_id=uuid.uuid4(), job_id=job_id) + mock_dao.assert_called_once_with(str(job_id)) + assert response["count"] == 3 + + def test_get_job_by_id(admin_request, sample_job): job_id = str(sample_job.id) service_id = sample_job.service.id From c48aa77dd50e1e30fd29834bd2499b333f34a3ce Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 25 Sep 2019 15:48:28 +0100 Subject: [PATCH 2/3] Use service_id in the query to make it safer, also use named parameters --- app/dao/notifications_dao.py | 4 ++-- app/job/rest.py | 2 +- .../dao/notification_dao/test_notification_dao.py | 15 +++++++++++++-- tests/app/job/test_rest.py | 5 +++-- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 7c4e1e91a..273bd5eb1 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -179,8 +179,8 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page @statsd(namespace="dao") -def dao_get_notification_count_for_job_id(job_id): - return Notification.query.filter_by(job_id=job_id).count() +def dao_get_notification_count_for_job_id(*, service_id, job_id): + return Notification.query.filter_by(service_id=service_id, job_id=job_id).count() @statsd(namespace="dao") diff --git a/app/job/rest.py b/app/job/rest.py index e2abe2bcb..7de187698 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -110,7 +110,7 @@ def get_all_notifications_for_service_job(service_id, job_id): @job_blueprint.route('//notification_count', methods=['GET']) def get_notification_count_for_job_id(service_id, job_id): - count = dao_get_notification_count_for_job_id(job_id) + count = dao_get_notification_count_for_job_id(service_id=service_id, job_id=job_id) return jsonify( count=count ), 200 diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2cfa369d7..97cdf38af 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -564,7 +564,7 @@ def test_dao_get_notification_count_for_job_id(notify_db_session, notify_db): create_notification(template) - assert dao_get_notification_count_for_job_id(job.id) == 3 + assert dao_get_notification_count_for_job_id(service_id=service.id, job_id=job.id) == 3 def test_dao_get_notification_count_for_job_id_only_finds_notification_already_in_db(notify_db_session, notify_db): @@ -573,7 +573,18 @@ def test_dao_get_notification_count_for_job_id_only_finds_notification_already_i job = create_job(template, notification_count=3) create_notification(template) - assert dao_get_notification_count_for_job_id(job.id) == 0 + assert dao_get_notification_count_for_job_id(service_id=service.id, job_id=job.id) == 0 + + +def test_dao_get_notification_count_for_job_id_doesnt_work_with_non_existing_service_id(notify_db_session, notify_db): + service = create_service() + template = create_template(service) + job = create_job(template, notification_count=3) + for i in range(3): + create_notification(job=job) + fake_service_id = str(uuid.uuid4()) + + assert dao_get_notification_count_for_job_id(service_id=fake_service_id, job_id=job.id) == 0 def test_update_notification_sets_status(sample_notification): diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 312e46e92..6a96c9adc 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -566,8 +566,9 @@ def test_get_all_notifications_for_job_returns_correct_format( def test_get_notification_count_for_job_id(admin_request, mocker): mock_dao = mocker.patch('app.job.rest.dao_get_notification_count_for_job_id', return_value=3) job_id = uuid.uuid4() - response = admin_request.get('job.get_notification_count_for_job_id', service_id=uuid.uuid4(), job_id=job_id) - mock_dao.assert_called_once_with(str(job_id)) + service_id = uuid.uuid4() + response = admin_request.get('job.get_notification_count_for_job_id', service_id=service_id, job_id=job_id) + mock_dao.assert_called_once_with(service_id=service_id, job_id=str(job_id)) assert response["count"] == 3 From 7fc7d99dacbf148a0442043a25db00f2f92c4594 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 3 Oct 2019 14:58:49 +0100 Subject: [PATCH 3/3] Update the new endpoint to return a 404 if the job or service id are not found. All our endpoint should perform a check that the params are valid - this is an easy whay to check that and is standard for our endpoints. I reverted the query to just filter by job id. --- app/dao/notifications_dao.py | 4 ++-- app/job/rest.py | 3 ++- .../notification_dao/test_notification_dao.py | 19 ++++----------- tests/app/job/test_rest.py | 23 +++++++++++++++---- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 273bd5eb1..a756a1ca2 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -179,8 +179,8 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page @statsd(namespace="dao") -def dao_get_notification_count_for_job_id(*, service_id, job_id): - return Notification.query.filter_by(service_id=service_id, job_id=job_id).count() +def dao_get_notification_count_for_job_id(*, job_id): + return Notification.query.filter_by(job_id=job_id).count() @statsd(namespace="dao") diff --git a/app/job/rest.py b/app/job/rest.py index 7de187698..e5ce149fe 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -110,7 +110,8 @@ def get_all_notifications_for_service_job(service_id, job_id): @job_blueprint.route('//notification_count', methods=['GET']) def get_notification_count_for_job_id(service_id, job_id): - count = dao_get_notification_count_for_job_id(service_id=service_id, job_id=job_id) + dao_get_job_by_service_id_and_job_id(service_id, job_id) + count = dao_get_notification_count_for_job_id(job_id=job_id) return jsonify( count=count ), 200 diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 97cdf38af..eaf35fcc5 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -555,7 +555,7 @@ def test_get_all_notifications_for_job_by_status(sample_job): assert len(notifications(filter_dict={'status': NOTIFICATION_STATUS_TYPES[:3]}).items) == 3 -def test_dao_get_notification_count_for_job_id(notify_db_session, notify_db): +def test_dao_get_notification_count_for_job_id(notify_db_session): service = create_service() template = create_template(service) job = create_job(template, notification_count=3) @@ -564,27 +564,16 @@ def test_dao_get_notification_count_for_job_id(notify_db_session, notify_db): create_notification(template) - assert dao_get_notification_count_for_job_id(service_id=service.id, job_id=job.id) == 3 + assert dao_get_notification_count_for_job_id(job_id=job.id) == 3 -def test_dao_get_notification_count_for_job_id_only_finds_notification_already_in_db(notify_db_session, notify_db): +def test_dao_get_notification_count_for_job_id_returns_zero_for_no_notifications_for_job(notify_db_session): service = create_service() template = create_template(service) job = create_job(template, notification_count=3) create_notification(template) - assert dao_get_notification_count_for_job_id(service_id=service.id, job_id=job.id) == 0 - - -def test_dao_get_notification_count_for_job_id_doesnt_work_with_non_existing_service_id(notify_db_session, notify_db): - service = create_service() - template = create_template(service) - job = create_job(template, notification_count=3) - for i in range(3): - create_notification(job=job) - fake_service_id = str(uuid.uuid4()) - - assert dao_get_notification_count_for_job_id(service_id=fake_service_id, job_id=job.id) == 0 + assert dao_get_notification_count_for_job_id(job_id=job.id) == 0 def test_update_notification_sets_status(sample_notification): diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 6a96c9adc..998c576d5 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -563,15 +563,28 @@ def test_get_all_notifications_for_job_returns_correct_format( assert resp['notifications'][0]['status'] == sample_notification_with_job.status -def test_get_notification_count_for_job_id(admin_request, mocker): +def test_get_notification_count_for_job_id(admin_request, mocker, sample_job): mock_dao = mocker.patch('app.job.rest.dao_get_notification_count_for_job_id', return_value=3) - job_id = uuid.uuid4() - service_id = uuid.uuid4() - response = admin_request.get('job.get_notification_count_for_job_id', service_id=service_id, job_id=job_id) - mock_dao.assert_called_once_with(service_id=service_id, job_id=str(job_id)) + response = admin_request.get('job.get_notification_count_for_job_id', + service_id=sample_job.service_id, job_id=sample_job.id) + mock_dao.assert_called_once_with(job_id=str(sample_job.id)) assert response["count"] == 3 +def test_get_notification_count_for_job_id_for_wrong_service_id(admin_request, sample_job): + service_id = uuid.uuid4() + response = admin_request.get('job.get_notification_count_for_job_id', service_id=service_id, + job_id=sample_job.id, _expected_status=404) + assert response['message'] == 'No result found' + + +def test_get_notification_count_for_job_id_for_wrong_job_id(admin_request, sample_service): + job_id = uuid.uuid4() + response = admin_request.get('job.get_notification_count_for_job_id', service_id=sample_service.id, + job_id=job_id, _expected_status=404) + assert response['message'] == 'No result found' + + def test_get_job_by_id(admin_request, sample_job): job_id = str(sample_job.id) service_id = sample_job.service.id