Return specific error messages if job cannot be cancelled

Also move checking if job can be cancelled to the endpoint

app/dao/jobs_dao.py

@@ -30,6 +30,11 @@ from app.models import (
 )
 
 
+CANCELLABLE_LETTER_STATUSES = [
+    'created', 'cancelled', 'virus-scan-failed', 'validation-failed', 'technical-failure', 'pending-virus-check'
+]
+
+
 @statsd(namespace="dao")
 def dao_get_notification_outcomes_for_job(service_id, job_id):
     return db.session.query(
@@ -157,7 +162,6 @@ def dao_get_jobs_older_than_data_retention(notification_types):
 
 @transactional
 def dao_cancel_letter_job(job):
-    if can_cancel_letter_job(job):
     number_of_notifications_cancelled = Notification.query.filter(
         Notification.job_id == job.id
     ).update({'status': NOTIFICATION_CANCELLED,
@@ -166,21 +170,22 @@ def dao_cancel_letter_job(job):
     job.job_status = JOB_STATUS_CANCELLED
     dao_update_job(job)
     return number_of_notifications_cancelled
-    else:
-        return False
 
 
-def can_cancel_letter_job(job):
+def can_letter_job_be_cancelled(job):
     template = dao_get_template_by_id(job.template_id)
     if template.template_type != LETTER_TYPE:
-        return False
+        return "Only letter jobs can be cancelled through this endpoint. This is not a letter job."
     if job.job_status != JOB_STATUS_FINISHED:
-        return False
+        return "This job is still being processed. Wait a couple of minutes and try again."
-    # Notifications are not in pending-virus-check
+
     count_notifications = Notification.query.filter(
         Notification.job_id == job.id,
-        Notification.status.in_(['created', 'pending-virus-check', 'cancelled'])
+        Notification.status.in_(CANCELLABLE_LETTER_STATUSES)
     ).count()
     if count_notifications != job.notification_count:
-        return False
+        return "This job is still being processed. Wait a couple of minutes and try again."
-    return letter_can_be_cancelled(NOTIFICATION_CREATED, job.created_at)
+    if letter_can_be_cancelled(NOTIFICATION_CREATED, job.created_at):
+        return True
+    else:
+        return "Sorry, it's too late, letters have already been sent."

app/job/rest.py

@@ -15,7 +15,7 @@ from app.dao.jobs_dao import (
     dao_get_future_scheduled_job_by_id_and_service_id,
     dao_get_notification_outcomes_for_job,
     dao_cancel_letter_job,
-    can_cancel_letter_job
+    can_letter_job_be_cancelled
 )
 from app.dao.fact_notification_status_dao import fetch_notification_statuses_for_job
 from app.dao.services_dao import dao_fetch_service_by_id
@@ -63,17 +63,15 @@ def cancel_job(service_id, job_id):
     return get_job_by_service_and_job_id(service_id, job_id)
 
 
-@job_blueprint.route('/<job_id>/cancel-letter-job')
+@job_blueprint.route('/<job_id>/cancel-letter-job', methods=['POST'])
 def cancel_letter_job(service_id, job_id):
     job = dao_get_job_by_service_id_and_job_id(service_id, job_id)
-    if can_cancel_letter_job(job):
+    can_we_cancel = can_letter_job_be_cancelled(job)
-        dao_cancel_letter_job(job)
+    if can_we_cancel is True:
-        return jsonify(), 201
+        data = dao_cancel_letter_job(job)
+        return jsonify(data), 200
     else:
-        # reasons the letter can't be cancelled:
+        return jsonify(message=can_we_cancel), 400
-        # letters are not yet saved to db  --> can still cancel but later
-        # it's too late
-        return jsonify("CHANGE ME: some error to say the job can't be cancelled"), 400
 
 
 @job_blueprint.route('/<job_id>/notifications', methods=['GET'])

tests/app/dao/test_jobs_dao.py

@@ -13,8 +13,7 @@ from app.dao.jobs_dao import (
     dao_set_scheduled_jobs_to_pending,
     dao_get_future_scheduled_job_by_id_and_service_id,
     dao_get_notification_outcomes_for_job,
-    dao_get_jobs_older_than_data_retention,
+    dao_get_jobs_older_than_data_retention
-    dao_cancel_letter_job
 )
 from app.models import (
     Job,
@@ -309,57 +308,3 @@ def assert_job_stat(job, result, sent, delivered, failed):
     assert result.sent == sent
     assert result.delivered == delivered
     assert result.failed == failed
-
-
-@freeze_time('2019-06-13 13:00')
-def test_dao_cancel_letter_job_updates_notifications_and_job_to_cancelled(sample_letter_template):
-    job = create_job(template=sample_letter_template, notification_count=1, job_status='finished')
-    notification = create_notification(template=job.template, job=job, status='created')
-    assert dao_cancel_letter_job(job) == 1
-    assert notification.status == 'cancelled'
-    assert job.job_status == 'cancelled'
-
-
-@freeze_time('2019-06-13 13:00')
-def test_dao_cancel_letter_job_does_not_allow_cancel_if_notification_in_sending(sample_letter_template):
-    job = create_job(template=sample_letter_template, notification_count=2, job_status='finished')
-    letter_1 = create_notification(template=job.template, job=job, status='sending')
-    letter_2 = create_notification(template=job.template, job=job, status='created')
-    assert not dao_cancel_letter_job(job)
-    assert letter_1.status == 'sending'
-    assert letter_2.status == 'created'
-    assert job.job_status == 'finished'
-
-
-def test_dao_cancel_letter_job_does_not_allow_cancel_if_letters_already_sent_to_dvla(sample_letter_template):
-    with freeze_time('2019-06-13 13:00'):
-        job = create_job(template=sample_letter_template, notification_count=1, job_status='finished')
-        letter = create_notification(template=job.template, job=job, status='created')
-
-    with freeze_time('2019-06-13 17:32'):
-        assert not dao_cancel_letter_job(job)
-    assert letter.status == 'created'
-    assert job.job_status == 'finished'
-
-
-@freeze_time('2019-06-13 13:00')
-def test_dao_cancel_letter_job_does_not_allow_cancel_if_not_a_letter_job(sample_template):
-    job = create_job(template=sample_template, notification_count=1, job_status='finished')
-    notification = create_notification(template=job.template, job=job, status='created')
-    assert not dao_cancel_letter_job(job)
-    assert notification.status == 'created'
-    assert job.job_status == 'finished'
-
-
-@freeze_time('2019-06-13 13:00')
-def test_dao_cancel_letter_job_does_not_allow_cancel_if_job_not_finished(sample_letter_template):
-    job = create_job(template=sample_letter_template, notification_count=1, job_status="finished")
-    create_notification(template=job.template, job=job, status='created')
-    assert not dao_cancel_letter_job(job)
-
-
-@freeze_time('2019-06-13 13:00')
-def test_dao_cancel_letter_job_does_not_allow_cancel_if_notifications_not_in_db_yet(sample_letter_template):
-    job = create_job(template=sample_letter_template, notification_count=2, job_status='finished')
-    create_notification(template=job.template, job=job, status='created')
-    assert not dao_cancel_letter_job(job)

tests/app/job/test_rest.py

@@ -71,6 +71,103 @@ def test_cant_cancel_normal_job(client, sample_job, mocker):
     assert mock_update.call_count == 0
 
 
+@freeze_time('2019-06-13 13:00')
+def test_dao_cancel_letter_job_updates_notifications_and_job_to_cancelled(sample_letter_template, admin_request):
+    job = create_job(template=sample_letter_template, notification_count=1, job_status='finished')
+    notification = create_notification(template=job.template, job=job, status='created')
+    response = admin_request.post(
+        'job.cancel_letter_job',
+        service_id=job.service_id,
+        job_id=job.id,
+    )
+    assert response == 1
+    assert notification.status == 'cancelled'
+    assert job.job_status == 'cancelled'
+
+
+@freeze_time('2019-06-13 13:00')
+def test_dao_cancel_letter_job_does_not_allow_cancel_if_notification_in_sending(sample_letter_template, admin_request):
+    job = create_job(template=sample_letter_template, notification_count=2, job_status='finished')
+    letter_1 = create_notification(template=job.template, job=job, status='sending')
+    letter_2 = create_notification(template=job.template, job=job, status='created')
+    response = admin_request.post(
+        'job.cancel_letter_job',
+        service_id=job.service_id,
+        job_id=job.id,
+        _expected_status=400
+    )
+    assert response == "This job is still being processed. Wait a couple of minutes and try again."
+    assert letter_1.status == 'sending'
+    assert letter_2.status == 'created'
+    assert job.job_status == 'finished'
+
+
+def test_dao_cancel_letter_job_does_not_allow_cancel_if_letters_already_sent_to_dvla(
+    sample_letter_template, admin_request
+):
+    with freeze_time('2019-06-13 13:00'):
+        job = create_job(template=sample_letter_template, notification_count=1, job_status='finished')
+        letter = create_notification(template=job.template, job=job, status='created')
+
+    with freeze_time('2019-06-13 17:32'):
+        response = admin_request.post(
+            'job.cancel_letter_job',
+            service_id=job.service_id,
+            job_id=job.id,
+            _expected_status=400
+        )
+    assert response == "Sorry, it's too late, letters have already been sent."
+    assert letter.status == 'created'
+    assert job.job_status == 'finished'
+
+
+@freeze_time('2019-06-13 13:00')
+def test_dao_cancel_letter_job_does_not_allow_cancel_if_not_a_letter_job(sample_template, admin_request):
+    job = create_job(template=sample_template, notification_count=1, job_status='finished')
+    notification = create_notification(template=job.template, job=job, status='created')
+    response = admin_request.post(
+        'job.cancel_letter_job',
+        service_id=job.service_id,
+        job_id=job.id,
+        _expected_status=400
+    )
+    assert response == "Only letter jobs can be cancelled through this endpoint. This is not a letter job."
+    assert notification.status == 'created'
+    assert job.job_status == 'finished'
+
+
+@freeze_time('2019-06-13 13:00')
+def test_dao_cancel_letter_job_does_not_allow_cancel_if_job_not_finished(sample_letter_template, admin_request):
+    job = create_job(template=sample_letter_template, notification_count=1, job_status="in progress")
+    letter = create_notification(template=job.template, job=job, status='created')
+    response = admin_request.post(
+        'job.cancel_letter_job',
+        service_id=job.service_id,
+        job_id=job.id,
+        _expected_status=400
+    )
+    assert response == "This job is still being processed. Wait a couple of minutes and try again."
+    assert letter.status == 'created'
+    assert job.job_status == 'in progress'
+
+
+@freeze_time('2019-06-13 13:00')
+def test_dao_cancel_letter_job_does_not_allow_cancel_if_notifications_not_in_db_yet(
+    sample_letter_template, admin_request
+):
+    job = create_job(template=sample_letter_template, notification_count=2, job_status='finished')
+    letter = create_notification(template=job.template, job=job, status='created')
+    response = admin_request.post(
+        'job.cancel_letter_job',
+        service_id=job.service_id,
+        job_id=job.id,
+        _expected_status=400
+    )
+    assert response == "This job is still being processed. Wait a couple of minutes and try again."
+    assert letter.status == 'created'
+    assert job.job_status == 'finished'
+
+
 def test_create_unscheduled_job(client, sample_template, mocker, fake_uuid):
     mocker.patch('app.celery.tasks.process_job.apply_async')
     mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value={