From 0118787a56f2e373ea15e6f91a5336b5b6a8cffe Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 24 Jun 2019 17:57:04 +0100 Subject: [PATCH] Can't cancel letter job if job not processed yet Also add more tests for showing or not the cancel those letters link Also check if all notifications already in database Upgrade delete button text logic to handle more cases Also corrections following review --- app/main/views/jobs.py | 20 ++++-- app/templates/flash_messages.html | 9 ++- app/templates/views/jobs/job.html | 6 +- tests/app/main/views/test_jobs.py | 102 ++++++++++++++++++++++++++++-- 4 files changed, 118 insertions(+), 19 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index fb63f9319..acb1a4346 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -173,17 +173,25 @@ def cancel_job(service_id, job_id): @main.route("/services//jobs//cancel", methods=['GET', 'POST']) @user_has_permissions() def cancel_letter_job(service_id, job_id): - if request.method == 'POST': + job = job_api_client.get_job(service_id, job_id)['data'] + notifications = notification_api_client.get_notifications_for_service( + job['service'], job['id'] + )['notifications'] + if job['job_status'] != 'finished' or len(notifications) < job['notification_count']: + flash("We are still processing these letters, please try again in a minute.", 'try again') + return view_job(service_id, job_id) try: number_of_letters = job_api_client.cancel_letter_job(current_service.id, job_id) except HTTPError as e: flash(e.message, 'dangerous') return redirect(url_for('main.view_job', service_id=service_id, job_id=job_id)) - flash(" {} letters have been cancelled succesfully".format(number_of_letters), 'default_with_tick') + flash("Cancelled {:,.0f} letters from {}".format( + number_of_letters, job['original_file_name'] + ), 'default_with_tick') return redirect(url_for('main.service_dashboard', service_id=service_id)) - flash("Are you sure you want to cancel sending those letters?", 'cancel') + flash("Are you sure you want to cancel sending these letters?", 'cancel') return view_job(service_id, job_id) @@ -435,12 +443,10 @@ def get_job_partials(job, template): n for n in notifications["notifications"] if n["status"] not in CANCELLABLE_JOB_LETTER_STATUSES ] job_created = job["created_at"][:-6] - if job["job_status"] != "finished": - can_letter_job_be_cancelled = "This job is still being processed. Wait a couple of minutes and try again." - elif not letter_can_be_cancelled( + if not letter_can_be_cancelled( "created", datetime.strptime(job_created, '%Y-%m-%dT%H:%M:%S.%f') ) or len(not_cancellable) != 0: - can_letter_job_be_cancelled = "Cancel sending those letters" + can_letter_job_be_cancelled = False else: can_letter_job_be_cancelled = True return { diff --git a/app/templates/flash_messages.html b/app/templates/flash_messages.html index a95f2fb19..08abc1b3b 100644 --- a/app/templates/flash_messages.html +++ b/app/templates/flash_messages.html @@ -2,11 +2,18 @@ {% with messages = get_flashed_messages(with_categories=true) %} {% if messages %} {% for category, message in messages %} + {% if category in ['cancel', 'delete', 'suspend', 'resume', 'remove', 'revoke this API key'] %} + {% set delete_button_text = "Yes, {}".format(category) %} + {% elif category == 'try again' %} + {% set delete_button_text = category|capitalize %} + {% else %} + {% set delete_button_text = None %} + {% endif %}
{{ banner( message if message is string else message[0], 'default' if ((category == 'default') or (category == 'default_with_tick')) else 'dangerous', - delete_button="Yes, {}".format(category) if category in ['cancel', 'delete', 'suspend', 'resume', 'remove', 'revoke this API key'] else None, + delete_button=delete_button_text, with_tick=True if category == 'default_with_tick' else False, context=message[1] if message is not string )}} diff --git a/app/templates/views/jobs/job.html b/app/templates/views/jobs/job.html index 2325f71c9..afa13138f 100644 --- a/app/templates/views/jobs/job.html +++ b/app/templates/views/jobs/job.html @@ -25,11 +25,7 @@
- {% if can_cancel_letter_job == True %} - Cancel sending those letters - {% else %} -

Can't cancel: {{ can_cancel_letter_job }}

- {% endif %} + Cancel sending these letters {% else %}
 
diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 98b0190f9..f9083c0c1 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -261,6 +261,7 @@ def test_should_show_page_for_one_job_with_flexible_data_retention( ) assert page.find('span', {'id': 'time-left'}).text == 'Data available for 10 days' + assert "Cancel sending these letters" not in page def test_get_jobs_should_tell_user_if_more_than_one_page( @@ -490,9 +491,24 @@ def test_should_not_show_cancelled_job( def test_should_cancel_letter_job( client_request, mocker, + active_user_with_permissions ): job_id = uuid.uuid4() - mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_letter_job') + job = job_json( + SERVICE_ONE_ID, + active_user_with_permissions, + job_id=job_id, + created_at="2019-06-20T15:30:00.000001+00:00", + job_status="finished" + ) + mocker.patch('app.job_api_client.get_job', side_effect=[{"data": job}]) + notifications_json = notification_json(SERVICE_ONE_ID, job=job, status="created", template_type="letter") + mocker.patch('app.job_api_client.get_job', side_effect=[{"data": job}]) + mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + side_effect=[notifications_json] + ) + mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_letter_job', return_value=5) client_request.post( 'main.cancel_letter_job', service_id=SERVICE_ONE_ID, @@ -539,16 +555,90 @@ def test_should_not_show_cancel_link_for_letter_job_if_too_late( job_id=str(job_id) ) - assert "Cancel sending those letters" not in page + assert "Cancel sending these letters" not in page assert page.find('p', {'id': 'printing-info'}).text.strip() == "Printed {} at 5:30pm".format(expected_fragment) -def test_dont_cancel_letter_job_when_to_early_to_cancel(): - pass +@freeze_time("2019-06-20 15:32:00.000001") +@pytest.mark.parametrize(" job_status", [ + "finished", "in progress" +]) +def test_should_show_cancel_link_for_letter_job( + client_request, + mocker, + mock_get_service_letter_template, + mock_get_service_data_retention, + active_user_with_permissions, + job_status, +): + job_id = uuid.uuid4() + job = job_json( + SERVICE_ONE_ID, + active_user_with_permissions, + job_id=job_id, + created_at="2019-06-20T15:30:00.000001+00:00", + job_status=job_status + ) + notifications_json = notification_json(SERVICE_ONE_ID, job=job, status="created", template_type="letter") + mocker.patch('app.job_api_client.get_job', side_effect=[{"data": job}]) + mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + side_effect=[notifications_json] + ) + + page = client_request.get( + 'main.view_job', + service_id=SERVICE_ONE_ID, + job_id=str(job_id) + ) + + assert page.find('a', text='Cancel sending these letters').attrs["href"] == url_for( + "main.cancel_letter_job", service_id=SERVICE_ONE_ID, job_id=job_id + ) + assert page.find('p', {'id': 'printing-info'}).text.strip() == "Printing starts today at 5:30pm" -def test_page_when_user_clicks_cancel_link_letter_job_and_to_early_to_cancel(): - pass +@freeze_time("2019-06-20 15:31:00.000001") +@pytest.mark.parametrize('job_status,number_of_processed_notifications', [['in progress', 2], ['finished', 1]]) +def test_dont_cancel_letter_job_when_to_early_to_cancel( + client_request, + mocker, + mock_get_service_letter_template, + mock_get_service_data_retention, + active_user_with_permissions, + job_status, + number_of_processed_notifications, +): + job_id = uuid.uuid4() + job = job_json( + SERVICE_ONE_ID, + active_user_with_permissions, + job_id=job_id, + created_at="2019-06-20T15:30:00.000001+00:00", + job_status=job_status, + notification_count=2 + ) + mocker.patch('app.job_api_client.get_job', side_effect=[{"data": job}, {"data": job}]) + + notifications_json = notification_json( + SERVICE_ONE_ID, job=job, status="created", template_type="letter", rows=number_of_processed_notifications + ) + mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + side_effect=[notifications_json, notifications_json] + ) + + mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_letter_job') + page = client_request.post( + 'main.cancel_letter_job', + service_id=SERVICE_ONE_ID, + job_id=str(job_id), + _expected_status=200, + ) + mock_cancel.assert_not_called() + flash_message = normalize_spaces(page.find('div', class_='banner-dangerous').text) + + assert 'We are still processing these letters, please try again in a minute.' in flash_message @freeze_time("2016-01-01 00:00:00.000001")