From 27a0ba1a6519a06abdf4ce1eee1eff2458e37967 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 10:47:14 +0100 Subject: [PATCH 1/4] Reformat arguments for readability We want to add another argument here, and doing so would make the line length too long with all the arguments on one line. Also uses the * operator to enforce keyword-only arguments. --- app/dao/jobs_dao.py | 9 ++++++++- app/job/rest.py | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index baff3bbb5..eef1486fd 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -58,7 +58,14 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() -def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50, statuses=None): +def dao_get_jobs_by_service_id( + service_id, + *, + limit_days=None, + page=1, + page_size=50, + statuses=None, +): query_filter = [ Job.service_id == service_id, Job.original_file_name != current_app.config['TEST_MESSAGE_FILENAME'], diff --git a/app/job/rest.py b/app/job/rest.py index cde9c2b11..d324f7e7e 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -128,10 +128,12 @@ def get_jobs_by_service(service_id): else: limit_days = None - statuses = [x.strip() for x in request.args.get('statuses', '').split(',')] - - page = int(request.args.get('page', 1)) - return jsonify(**get_paginated_jobs(service_id, limit_days, statuses, page)) + return jsonify(**get_paginated_jobs( + service_id, + limit_days=limit_days, + statuses=[x.strip() for x in request.args.get('statuses', '').split(',')], + page=int(request.args.get('page', 1)), + )) @job_blueprint.route('', methods=['POST']) @@ -185,7 +187,13 @@ def create_job(service_id): return jsonify(data=job_json), 201 -def get_paginated_jobs(service_id, limit_days, statuses, page): +def get_paginated_jobs( + service_id, + *, + limit_days, + statuses, + page, +): pagination = dao_get_jobs_by_service_id( service_id, limit_days=limit_days, From 18ffccf8c995b417b4b3d28e71691c52bc3131fc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 11:03:09 +0100 Subject: [PATCH 2/4] Allow jobs to be filtered by contact list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than showing all jobs that have been ‘copied’ from a contact list I think it makes more sense to group them under the contact list. This way it’s easier to see what messages have been sent to a given group of contacts over time. Part of this work means the API needs to return only jobs that have been created from a given contact list, when asked. --- app/dao/jobs_dao.py | 3 +++ app/job/rest.py | 5 ++++- tests/app/dao/test_jobs_dao.py | 22 +++++++++++++++++++++- tests/app/job/test_rest.py | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index eef1486fd..745e909ae 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -65,6 +65,7 @@ def dao_get_jobs_by_service_id( page=1, page_size=50, statuses=None, + contact_list_id=None, ): query_filter = [ Job.service_id == service_id, @@ -77,6 +78,8 @@ def dao_get_jobs_by_service_id( query_filter.append( Job.job_status.in_(statuses) ) + if contact_list_id is not None: + query_filter.append(Job.contact_list_id == contact_list_id) return Job.query \ .filter(*query_filter) \ .order_by(Job.processing_started.desc(), Job.created_at.desc()) \ diff --git a/app/job/rest.py b/app/job/rest.py index d324f7e7e..2831f9da9 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -133,6 +133,7 @@ def get_jobs_by_service(service_id): limit_days=limit_days, statuses=[x.strip() for x in request.args.get('statuses', '').split(',')], page=int(request.args.get('page', 1)), + contact_list_id=request.args.get('contact_list_id'), )) @@ -193,13 +194,15 @@ def get_paginated_jobs( limit_days, statuses, page, + contact_list_id, ): pagination = dao_get_jobs_by_service_id( service_id, limit_days=limit_days, page=page, page_size=current_app.config['PAGE_SIZE'], - statuses=statuses + statuses=statuses, + contact_list_id=contact_list_id, ) data = job_schema.dump(pagination.items, many=True).data for job_data in data: diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 230e279d6..baa6746d4 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -25,7 +25,7 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, JOB_STATUS_FINISHED ) -from tests.app.db import create_job, create_service, create_template, create_notification +from tests.app.db import create_job, create_service, create_template, create_notification, create_service_contact_list def test_should_have_decorated_notifications_dao_functions(): @@ -168,6 +168,26 @@ def test_get_jobs_for_service_in_processed_at_then_created_at_order(notify_db, n assert jobs[index].id == created_jobs[index].id +def test_get_jobs_for_service_by_contact_list(sample_template): + contact_list = create_service_contact_list() + job_1 = create_job(sample_template) + job_2 = create_job(sample_template, contact_list_id=contact_list.id) + + assert dao_get_jobs_by_service_id( + sample_template.service.id + ).items == [ + job_2, + job_1, + ] + + assert dao_get_jobs_by_service_id( + sample_template.service.id, + contact_list_id=contact_list.id, + ).items == [ + job_2, + ] + + def test_update_job(sample_job): assert sample_job.job_status == 'pending' diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 98605b0d3..d6db5e917 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -740,6 +740,20 @@ def test_get_jobs_with_limit_days(admin_request, sample_template): assert len(resp_json['data']) == 2 +def test_get_jobs_by_contact_list(admin_request, sample_template): + contact_list = create_service_contact_list() + create_job(template=sample_template) + create_job(template=sample_template, contact_list_id=contact_list.id) + + resp_json = admin_request.get( + 'job.get_jobs_by_service', + service_id=sample_template.service_id, + contact_list_id=contact_list.id, + ) + + assert len(resp_json['data']) == 1 + + def test_get_jobs_should_return_statistics(admin_request, sample_template): now = datetime.utcnow() earlier = datetime.utcnow() - timedelta(days=1) From 3ed1700231f51afd4ce6fa94d6a439a0a2b0eff3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 11:53:36 +0100 Subject: [PATCH 3/4] Count how many times a contact list has been used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we’ll be grouping jobs under their parent contact lists it will be useful to have a way of showing how many times a contact list has been used. This will give the right information scent to indicate that clicking into a contact list is where you go to see its jobs. This means that the API needs to return a count of jobs for each contact list. Putting this code feels very non-idiomatic for our API. So suggestions about how to better architect it welcome… --- app/models.py | 17 ++++++- .../service/test_service_contact_list_rest.py | 46 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 4c24fde92..0e0afd521 100644 --- a/app/models.py +++ b/app/models.py @@ -12,7 +12,7 @@ from sqlalchemy.dialects.postgresql import ( JSONB, ) from sqlalchemy.orm.collections import attribute_mapped_collection -from sqlalchemy import UniqueConstraint, CheckConstraint, Index +from sqlalchemy import UniqueConstraint, CheckConstraint, Index, String, and_, func from notifications_utils.columns import Columns from notifications_utils.recipients import ( validate_email_address, @@ -2138,12 +2138,27 @@ class ServiceContactList(db.Model): updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) archived = db.Column(db.Boolean, nullable=False, default=False) + def get_job_count(self): + today = datetime.datetime.utcnow().date() + return Job.query.filter( + Job.contact_list_id == self.id, + func.coalesce( + Job.processing_started, Job.created_at + ) >= today - func.coalesce(ServiceDataRetention.days_of_retention, 7) + ).outerjoin( + ServiceDataRetention, and_( + self.service_id == ServiceDataRetention.service_id, + func.cast(self.template_type, String) == func.cast(ServiceDataRetention.notification_type, String) + ) + ).count() + def serialize(self): created_at_in_bst = convert_utc_to_bst(self.created_at) contact_list = { "id": str(self.id), "original_file_name": self.original_file_name, "row_count": self.row_count, + "job_count": self.get_job_count(), "template_type": self.template_type, "service_id": str(self.service_id), "created_by": self.created_by.name, diff --git a/tests/app/service/test_service_contact_list_rest.py b/tests/app/service/test_service_contact_list_rest.py index 5b351c23b..3ce45755d 100644 --- a/tests/app/service/test_service_contact_list_rest.py +++ b/tests/app/service/test_service_contact_list_rest.py @@ -1,9 +1,13 @@ +import pytest import uuid +from datetime import datetime, timedelta + from app.models import ServiceContactList from tests.app.db import ( create_job, create_service_contact_list, + create_service_data_retention, create_service, create_template, ) @@ -65,6 +69,48 @@ def test_get_contact_list(admin_request, notify_db_session): assert len(response) == 1 assert response[0] == contact_list.serialize() + assert response[0]['job_count'] == 0 + + +@pytest.mark.parametrize('days_of_email_retention, expected_job_count', ( + (None, 8), + (7, 8), + (3, 4), +)) +def test_get_contact_list_counts_jobs( + sample_template, + admin_request, + days_of_email_retention, + expected_job_count, +): + if days_of_email_retention: + create_service_data_retention(sample_template.service, 'email', days_of_email_retention) + + # This should be ignored because it’s another template type + create_service_data_retention(sample_template.service, 'sms', 1) + + contact_list_1 = create_service_contact_list(service=sample_template.service) + contact_list_2 = create_service_contact_list(service=sample_template.service) + + for i in range(10): + create_job( + template=sample_template, + contact_list_id=contact_list_2.id, + created_at=datetime.utcnow() - timedelta(days=i) + ) + + response = admin_request.get( + 'service.get_contact_list', + service_id=contact_list_1.service_id + ) + + assert len(response) == 2 + + assert response[0]['id'] == str(contact_list_2.id) + assert response[0]['job_count'] == expected_job_count + + assert response[1]['id'] == str(contact_list_1.id) + assert response[1]['job_count'] == 0 def test_get_contact_list_returns_for_service(admin_request, notify_db_session): From c8cd3c2b7062ab7916b913e93d47e03881489fc8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 May 2020 12:56:01 +0100 Subject: [PATCH 4/4] Return template name in job response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve sent a bunch of jobs from the same contact list then a handy way to differentiate between them will be date sent, but also template name (in effect the message you sent). This commit extends the job response to include template name, using the same pattern as for template type. --- app/schemas.py | 4 ++++ tests/app/job/test_rest.py | 1 + 2 files changed, 5 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index efa99e7d4..588c92ec0 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -380,9 +380,13 @@ class JobSchema(BaseSchema): service_name = fields.Nested( ServiceSchema, attribute="service", dump_to="service_name", only=["name"], dump_only=True) + template_name = fields.Method('get_template_name', dump_only=True) template_type = fields.Method('get_template_type', dump_only=True) contact_list_id = field_for(models.Job, 'contact_list_id') + def get_template_name(self, job): + return job.template.name + def get_template_type(self, job): return job.template.template_type diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index d6db5e917..0654ed6ff 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -718,6 +718,7 @@ def test_get_jobs(admin_request, sample_template): 'service_name': {'name': sample_template.service.name}, 'statistics': [], 'template': str(sample_template.id), + 'template_name': sample_template.name, 'template_type': 'sms', 'template_version': 1, 'updated_at': None,