From 4a7267be8bf59ba65f5472618eaecc032b379080 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Sep 2016 14:31:01 +0100 Subject: [PATCH 1/6] Add an endpoint to cancel a job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you schedule a job you might change your mind or circumstances might change. So you need to be able to cancel it. This commit adds a `POST` endpoint for individual jobs which sets their status to `cancelled`. This also means adding a new status of `cancelled`, so there’s a migration… --- app/dao/jobs_dao.py | 7 ++++++ app/job/rest.py | 13 +++++++++- app/models.py | 1 + .../versions/0051_cancelled_job_status.py | 22 ++++++++++++++++ tests/app/conftest.py | 18 ++++++++++++- tests/app/dao/test_jobs_dao.py | 8 ++++++ tests/app/job/test_rest.py | 25 +++++++++++++++++++ 7 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0051_cancelled_job_status.py diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 3ad1f84fd..94bcc6210 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -44,6 +44,13 @@ def dao_get_scheduled_jobs(): .all() +def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): + return Job.query \ + .filter_by(service_id=service_id, id=job_id) \ + .filter(Job.job_status == 'scheduled', Job.scheduled_for > datetime.utcnow()) \ + .one() + + def dao_create_job(job): db.session.add(job) db.session.commit() diff --git a/app/job/rest.py b/app/job/rest.py index 8d490c333..9f9ef0047 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -7,8 +7,10 @@ from flask import ( from app.dao.jobs_dao import ( dao_create_job, + dao_update_job, dao_get_job_by_service_id_and_job_id, dao_get_jobs_by_service_id, + dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job ) @@ -28,7 +30,7 @@ from app.schemas import ( from app.celery.tasks import process_job -from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING +from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANCELLED from app.utils import pagination_links @@ -53,6 +55,15 @@ def get_job_by_service_and_job_id(service_id, job_id): return jsonify(data=data) +@job.route('//cancel', methods=['POST']) +def cancel_job(service_id, job_id): + job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) + job.job_status = JOB_STATUS_CANCELLED + dao_update_job(job) + + return get_job_by_service_and_job_id(service_id, job_id) + + @job.route('//notifications', methods=['GET']) def get_all_notifications_for_service_job(service_id, job_id): data = notifications_filter_schema.load(request.args).data diff --git a/app/models.py b/app/models.py index e3bb53f2c..6345071aa 100644 --- a/app/models.py +++ b/app/models.py @@ -308,6 +308,7 @@ JOB_STATUS_IN_PROGRESS = 'in progress' JOB_STATUS_FINISHED = 'finished' JOB_STATUS_SENDING_LIMITS_EXCEEDED = 'sending limits exceeded' JOB_STATUS_SCHEDULED = 'scheduled' +JOB_STATUS_CANCELLED = 'cancelled' class JobStatus(db.Model): diff --git a/migrations/versions/0051_cancelled_job_status.py b/migrations/versions/0051_cancelled_job_status.py new file mode 100644 index 000000000..902da29ac --- /dev/null +++ b/migrations/versions/0051_cancelled_job_status.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0051_cancelled_job_status +Revises: 0050_index_for_stats +Create Date: 2016-09-01 14:34:06.839381 + +""" + +# revision identifiers, used by Alembic. +revision = '0051_cancelled_job_status' +down_revision = '0050_index_for_stats' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.execute("INSERT INTO job_status VALUES ('cancelled')") + +def downgrade(): + op.execute("UPDATE jobs SET job_status = 'finished' WHERE job_status = 'cancelled'") + op.execute("DELETE FROM job_status WHERE name = 'cancelled';") diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 76aba362a..1296d8bcd 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,7 +1,7 @@ import requests_mock import pytest import uuid -from datetime import (datetime, date) +from datetime import (datetime, date, timedelta) from flask import current_app @@ -288,6 +288,22 @@ def sample_job_with_placeholdered_template( ) +@pytest.fixture(scope='function') +def sample_scheduled_job( + notify_db, + notify_db_session, + service=None +): + return sample_job( + notify_db, + notify_db_session, + service=service, + template=sample_template_with_placeholders(notify_db, notify_db_session), + scheduled_for=(datetime.utcnow() + timedelta(minutes=60)).isoformat(), + job_status='scheduled' + ) + + @pytest.fixture(scope='function') def sample_email_job(notify_db, notify_db_session, diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 3674ef24c..0c23bad9d 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -7,6 +7,7 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_jobs_by_service_id, dao_get_scheduled_jobs, + dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job ) @@ -251,3 +252,10 @@ def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(notify_db, sample_job(notify_db, notify_db_session, scheduled_for=one_minute_in_the_future, job_status='scheduled') jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 + + +def test_get_future_scheduled_job_gets_a_job_yet_to_send(notify_db, notify_db_session): + one_hour_from_now = datetime.utcnow() + timedelta(minutes=60) + job = sample_job(notify_db, notify_db_session, scheduled_for=one_hour_from_now, job_status='scheduled') + result = dao_get_future_scheduled_job_by_id_and_service_id(job.id, job.service_id) + assert result.id == job.id diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 488a75419..3fb3cd9a9 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -109,6 +109,31 @@ def test_get_job_by_id(notify_api, sample_job): assert resp_json['data']['created_by']['name'] == 'Test User' +def test_cancel_job(notify_api, sample_scheduled_job): + job_id = str(sample_scheduled_job.id) + service_id = sample_scheduled_job.service.id + with notify_api.test_request_context(), notify_api.test_client() as client: + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.post(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + assert resp_json['data']['job_status'] == 'cancelled' + + +def test_cant_cancel_normal_job(notify_api, sample_job, mocker): + job_id = str(sample_job.id) + service_id = sample_job.service.id + with notify_api.test_request_context(), notify_api.test_client() as client: + mock_update = mocker.patch('app.dao.jobs_dao.dao_update_job') + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.post(path, headers=[auth_header]) + assert response.status_code == 404 + assert mock_update.call_count == 0 + + def test_create_unscheduled_job(notify_api, sample_template, mocker, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: From 11a4b1845181a97eea943d6651789bb5258a5bc7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 12:20:28 +0100 Subject: [PATCH 2/6] Combine query filters when getting scheduled job > filter_by and filter are just aliases for each other so can be > combined together - filter is probably the better one (and then use > == instead of keyword args) --- app/dao/jobs_dao.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 94bcc6210..74d9a4f90 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -46,8 +46,12 @@ def dao_get_scheduled_jobs(): def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): return Job.query \ - .filter_by(service_id=service_id, id=job_id) \ - .filter(Job.job_status == 'scheduled', Job.scheduled_for > datetime.utcnow()) \ + .filter( + Job.service_id == service_id, + Job.id == job_id, + Job.job_status == 'scheduled', + Job.scheduled_for > datetime.utcnow() + ) \ .one() From 5d4e942fa15c3f4231948813e1f5f010bfbb7dd6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 12:24:14 +0100 Subject: [PATCH 3/6] Use fixture for testing scheduled jobs --- tests/app/dao/test_jobs_dao.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 0c23bad9d..69efcd08d 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -247,15 +247,15 @@ def test_get_scheduled_jobs_gets_ignores_jobs_not_scheduled(notify_db, notify_db assert jobs[0].id == job_scheduled.id -def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(notify_db, notify_db_session): - one_minute_in_the_future = datetime.utcnow() + timedelta(minutes=1) - sample_job(notify_db, notify_db_session, scheduled_for=one_minute_in_the_future, job_status='scheduled') +def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future( + notify_db, notify_db_session, sample_scheduled_job +): jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 -def test_get_future_scheduled_job_gets_a_job_yet_to_send(notify_db, notify_db_session): - one_hour_from_now = datetime.utcnow() + timedelta(minutes=60) - job = sample_job(notify_db, notify_db_session, scheduled_for=one_hour_from_now, job_status='scheduled') - result = dao_get_future_scheduled_job_by_id_and_service_id(job.id, job.service_id) - assert result.id == job.id +def test_get_future_scheduled_job_gets_a_job_yet_to_send( + notify_db, notify_db_session, sample_scheduled_job +): + result = dao_get_future_scheduled_job_by_id_and_service_id(sample_scheduled_job.id, sample_scheduled_job.service_id) + assert result.id == sample_scheduled_job.id From 2c17825c128d25d0031cb15f66840dea9320936b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:13:32 +0100 Subject: [PATCH 4/6] =?UTF-8?q?Don=E2=80=99t=20use=20magic=20string=20for?= =?UTF-8?q?=20job=20status?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/dao/jobs_dao.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 74d9a4f90..0d4634d56 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -2,7 +2,7 @@ from datetime import date, timedelta, datetime from sqlalchemy import desc, asc, cast, Date as sql_date from app import db from app.dao import days_ago -from app.models import Job, NotificationHistory +from app.models import Job, NotificationHistory, JOB_STATUS_SCHEDULED from app.statsd_decorators import statsd from sqlalchemy import func, asc @@ -39,7 +39,7 @@ def dao_get_job_by_id(job_id): def dao_get_scheduled_jobs(): return Job.query \ - .filter(Job.job_status == 'scheduled', Job.scheduled_for < datetime.utcnow()) \ + .filter(Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for < datetime.utcnow()) \ .order_by(asc(Job.scheduled_for)) \ .all() @@ -49,7 +49,7 @@ def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): .filter( Job.service_id == service_id, Job.id == job_id, - Job.job_status == 'scheduled', + Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for > datetime.utcnow() ) \ .one() From 2aa8baec630985669aa8e75bbf0fb77fb2b017d2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:14:03 +0100 Subject: [PATCH 5/6] Make indentation consistent --- app/dao/jobs_dao.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 0d4634d56..790b9cb18 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -39,7 +39,10 @@ def dao_get_job_by_id(job_id): def dao_get_scheduled_jobs(): return Job.query \ - .filter(Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for < datetime.utcnow()) \ + .filter( + Job.job_status == JOB_STATUS_SCHEDULED, + Job.scheduled_for < datetime.utcnow() + ) \ .order_by(asc(Job.scheduled_for)) \ .all() From e28ce50d179e1c85470d9730a2d24b19365b2503 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:18:55 +0100 Subject: [PATCH 6/6] =?UTF-8?q?Don=E2=80=99t=20pass=20notify=5Fdb=20or=20n?= =?UTF-8?q?otify=5Fdb=5Fsession=20to=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They get set up by the `sample_scheduled_job` fixture. --- tests/app/dao/test_jobs_dao.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 69efcd08d..0c3153e95 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -247,15 +247,11 @@ def test_get_scheduled_jobs_gets_ignores_jobs_not_scheduled(notify_db, notify_db assert jobs[0].id == job_scheduled.id -def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future( - notify_db, notify_db_session, sample_scheduled_job -): +def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(sample_scheduled_job): jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 -def test_get_future_scheduled_job_gets_a_job_yet_to_send( - notify_db, notify_db_session, sample_scheduled_job -): +def test_get_future_scheduled_job_gets_a_job_yet_to_send(sample_scheduled_job): result = dao_get_future_scheduled_job_by_id_and_service_id(sample_scheduled_job.id, sample_scheduled_job.service_id) assert result.id == sample_scheduled_job.id