From b4291684b769eee8d1c2d3313ae7d024ba00f0dc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 8 Oct 2016 11:44:55 +0100 Subject: [PATCH 1/4] Sort jobs by processed time first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Say you have a dashboard with some jobs you sent. Normally looks like: job | sent --- | --- file.csv | **5pm** file.csv | 3pm file.csv | 1pm file.csv | 11am However if your 5pm job was scheduled at lunchtime, then it will look like this: job | sent --- | --- file.csv | 3pm file.csv | 1pm file.csv | **5pm** file.csv | 11am This is because the jobs are sorted by when they were created, not when they were sent. It looks wrong. **For jobs that have already been sent** This commit changes the sort order to be based on `processed_at` instead. **For upcoming jobs** If a job doesn’t have a `processed_at` time then it’s scheduled, but hasn’t started yet. Only in this case should we still be sorting by `created_at`. --- app/dao/jobs_dao.py | 2 +- tests/app/conftest.py | 6 ++++-- tests/app/dao/test_jobs_dao.py | 32 ++++++++++++++++++-------------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index e78b27645..f562a091f 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -37,7 +37,7 @@ def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50 ) return Job.query \ .filter(*query_filter) \ - .order_by(desc(Job.created_at)) \ + .order_by(Job.processing_started.desc(), Job.created_at.desc()) \ .paginate(page=page, per_page=page_size) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c3b8e1093..1dd226b7c 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -264,7 +264,8 @@ def sample_job(notify_db, notification_count=1, created_at=None, job_status='pending', - scheduled_for=None): + scheduled_for=None, + processing_started=None): if service is None: service = sample_service(notify_db, notify_db_session) if template is None: @@ -281,7 +282,8 @@ def sample_job(notify_db, 'created_at': created_at or datetime.utcnow(), 'created_by': service.created_by, 'job_status': job_status, - 'scheduled_for': scheduled_for + 'scheduled_for': scheduled_for, + 'processing_started': processing_started } job = Job(**data) dao_create_job(job) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index e13a00795..f823acb3c 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta +from functools import partial import uuid from freezegun import freeze_time @@ -192,23 +193,26 @@ def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_ses assert job_eight_days_old not in jobs_limit_days -def test_get_jobs_for_service_in_created_at_order(notify_db, notify_db_session, sample_template): - job_1 = create_job( - notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) - job_2 = create_job( - notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) - job_3 = create_job( - notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) - job_4 = create_job( - notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) +def test_get_jobs_for_service_in_processed_at_then_created_at_order(notify_db, notify_db_session, sample_template): + _create_job = partial(create_job, notify_db, notify_db_session, sample_template.service, sample_template) + scheduled_jobs, processed_jobs = [], [] + + for index in range(0, 4): + scheduled_jobs.append(_create_job( + created_at=datetime.utcnow() - timedelta(seconds=index) + )) + processed_jobs.append(_create_job( + created_at=datetime.utcnow() - timedelta(minutes=(4 - index)), + processing_started=datetime.utcnow() - timedelta(hours=index) + )) jobs = dao_get_jobs_by_service_id(sample_template.service.id).items - assert len(jobs) == 4 - assert jobs[0].id == job_4.id - assert jobs[1].id == job_3.id - assert jobs[2].id == job_2.id - assert jobs[3].id == job_1.id + assert len(jobs) == 8 + + for index in range(0, 4): + assert jobs[index].id == scheduled_jobs[index].id + assert jobs[index + 4].id == processed_jobs[index].id def test_update_job(sample_job): From df0504dddaf65568b92065c6c2e605279d65f387 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 10 Oct 2016 12:45:48 +0100 Subject: [PATCH 2/4] Refactor job order test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - uses 4 rather than 8 entries to test the sort (2 notifications × 2 columns on which we’re sorting) - makes sure we test for when a scheduled job was created before a job that’s been processed already - removes any relative datetimes so the tests are independant of database speed --- tests/app/dao/test_jobs_dao.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index f823acb3c..e9b4a2376 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -194,25 +194,31 @@ def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_ses def test_get_jobs_for_service_in_processed_at_then_created_at_order(notify_db, notify_db_session, sample_template): - _create_job = partial(create_job, notify_db, notify_db_session, sample_template.service, sample_template) - scheduled_jobs, processed_jobs = [], [] - for index in range(0, 4): - scheduled_jobs.append(_create_job( - created_at=datetime.utcnow() - timedelta(seconds=index) - )) - processed_jobs.append(_create_job( - created_at=datetime.utcnow() - timedelta(minutes=(4 - index)), - processing_started=datetime.utcnow() - timedelta(hours=index) - )) + _create_job = partial(create_job, notify_db, notify_db_session, sample_template.service, sample_template) + + def _time_from_hour(hour): + return datetime(2001, 1, 1, hour) if hour is not None else None + + created_jobs = [ + _create_job( + created_at=_time_from_hour(created_at_hour), + processing_started=_time_from_hour(processing_started_hour), + ) + for created_at_hour, processing_started_hour in [ + (2, None), + (1, None), + (1, 4), + (4, 3), + ] + ] jobs = dao_get_jobs_by_service_id(sample_template.service.id).items - assert len(jobs) == 8 + assert len(jobs) == len(created_jobs) - for index in range(0, 4): - assert jobs[index].id == scheduled_jobs[index].id - assert jobs[index + 4].id == processed_jobs[index].id + for index in range(0, len(created_jobs)): + assert jobs[index].id == created_jobs[index].id def test_update_job(sample_job): From a2ea0db3939a80a4f1c52330d2d365b0e740c188 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 10 Oct 2016 13:10:53 +0100 Subject: [PATCH 3/4] Make test easier to read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Explicity labelling what the numbers mean makes it easier to understand what’s going on rather than having to refer back to the function call. --- tests/app/dao/test_jobs_dao.py | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index e9b4a2376..af40d995d 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -196,21 +196,13 @@ def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_ses def test_get_jobs_for_service_in_processed_at_then_created_at_order(notify_db, notify_db_session, sample_template): _create_job = partial(create_job, notify_db, notify_db_session, sample_template.service, sample_template) - - def _time_from_hour(hour): - return datetime(2001, 1, 1, hour) if hour is not None else None + from_hour = partial(datetime, 2001, 1, 1) created_jobs = [ - _create_job( - created_at=_time_from_hour(created_at_hour), - processing_started=_time_from_hour(processing_started_hour), - ) - for created_at_hour, processing_started_hour in [ - (2, None), - (1, None), - (1, 4), - (4, 3), - ] + _create_job(created_at=from_hour(2), processing_started=None), + _create_job(created_at=from_hour(1), processing_started=None), + _create_job(created_at=from_hour(1), processing_started=from_hour(4)), + _create_job(created_at=from_hour(4), processing_started=from_hour(3)), ] jobs = dao_get_jobs_by_service_id(sample_template.service.id).items From ac1f5fcc91e93d353e5145deeaff66265472134b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 10 Oct 2016 13:12:50 +0100 Subject: [PATCH 4/4] Fix impossible test notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No way a notification can be processed before it’s created. --- tests/app/dao/test_jobs_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index af40d995d..59b32237e 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -202,7 +202,7 @@ def test_get_jobs_for_service_in_processed_at_then_created_at_order(notify_db, n _create_job(created_at=from_hour(2), processing_started=None), _create_job(created_at=from_hour(1), processing_started=None), _create_job(created_at=from_hour(1), processing_started=from_hour(4)), - _create_job(created_at=from_hour(4), processing_started=from_hour(3)), + _create_job(created_at=from_hour(2), processing_started=from_hour(3)), ] jobs = dao_get_jobs_by_service_id(sample_template.service.id).items