diff --git a/app/main/views/send.py b/app/main/views/send.py index 0fa35eaab..c85280185 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -609,6 +609,9 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ )), required_recipient_columns=OrderedSet(recipients.recipient_column_headers) - optional_address_columns, preview_row=preview_row, + sent_previously=job_api_client.has_sent_previously( + service_id, template.id, db_template['version'], request.args.get('original_file_name', '') + ) ) @@ -625,7 +628,8 @@ def check_messages(service_id, template_id, upload_id, row_index=2): not data['count_of_recipients'] or not data['recipients'].has_recipient_columns or data['recipients'].duplicate_recipient_column_headers or - data['recipients'].missing_column_headers + data['recipients'].missing_column_headers or + data['sent_previously'] ): return render_template('views/check/column-errors.html', **data) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index c2a882e85..e835ac601 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -60,6 +60,17 @@ class JobApiClient(NotifyAdminAPIClient): return jobs + def has_sent_previously(self, service_id, template_id, template_version, original_file_name): + return ( + template_id, template_version, original_file_name + ) in ( + ( + job['template'], job['template_version'], job['original_file_name'], + ) + for job in self.get_jobs(service_id, limit_days=0)['data'] + if job['job_status'] != 'cancelled' + ) + def get_page_of_jobs(self, service_id, page): return self.get_jobs( service_id, diff --git a/app/templates/partials/check/sent-previously.html b/app/templates/partials/check/sent-previously.html new file mode 100644 index 000000000..8cd8d50a3 --- /dev/null +++ b/app/templates/partials/check/sent-previously.html @@ -0,0 +1,6 @@ +
+ If you want to send the same messages again, rename the file and re-upload it +
diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 94c81a559..72f80cca7 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -98,6 +98,10 @@ ) }}. + {% elif sent_previously %} + + {% include "partials/check/sent-previously.html" %} + {% elif not recipients.allowed_to_send_to %} {% with diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 2109abdb1..fa5d1d55a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -387,6 +387,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, ): @@ -512,6 +513,7 @@ def test_upload_csvfile_with_missing_columns_shows_error( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, service_one, fake_uuid, file_contents, @@ -593,6 +595,7 @@ def test_upload_valid_csv_shows_preview_and_table( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, extra_args, @@ -703,6 +706,7 @@ def test_upload_valid_csv_only_sets_meta_if_filename_known( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, ): @@ -740,6 +744,7 @@ def test_file_name_truncated_to_fit_in_s3_metadata( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, ): @@ -781,6 +786,7 @@ def test_check_messages_replaces_invalid_characters_in_file_name( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, ): @@ -822,6 +828,7 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, ): @@ -867,6 +874,7 @@ def test_404_for_previewing_a_row_out_of_range( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, row_index, @@ -1887,6 +1895,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_service_statistics, mock_get_live_service, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, service_one, fake_uuid, @@ -1943,6 +1952,7 @@ def test_upload_csvfile_with_international_validates( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, service_mock, should_allow_international, @@ -1975,6 +1985,7 @@ def test_test_message_can_only_be_sent_now( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid ): @@ -1998,6 +2009,7 @@ def test_letter_can_only_be_sent_now( mock_get_service_statistics, mock_s3_set_metadata, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, ): mocker.patch('app.main.views.send.s3download', return_value="addressline1, addressline2, postcode\na,b,c") @@ -2138,6 +2150,7 @@ def test_should_show_preview_letter_message( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, service_one, fake_uuid, mocker, @@ -2356,6 +2369,7 @@ def test_check_messages_back_link( mock_has_permissions, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_download, mock_s3_set_metadata, fake_uuid, @@ -2449,6 +2463,7 @@ def test_check_messages_shows_too_many_messages_errors( mock_get_service, mock_get_service_template, mock_get_job_doesnt_exist, + mock_get_jobs, mock_has_permissions, fake_uuid, num_requested, @@ -2498,6 +2513,7 @@ def test_check_messages_shows_trial_mode_error( mock_has_permissions, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, mocker ): @@ -2546,6 +2562,7 @@ def test_check_messages_shows_trial_mode_error_for_letters( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, mocker, @@ -2605,6 +2622,7 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, fake_uuid, ): @@ -2644,6 +2662,50 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( assert not page.select('.table-field-index a') +@pytest.mark.parametrize('uploaded_file_name', ( + pytest.param('applicants.ods'), # normal job + pytest.param('thisisatest.csv', marks=pytest.mark.xfail), # different template version + pytest.param('send_me_later.csv'), # should look at scheduled job + pytest.param('full_of_regret.csv', marks=pytest.mark.xfail), # job is cancelled +)) +def test_warns_if_file_sent_already( + client_request, + mock_get_users_by_service, + mock_get_live_service, + mock_get_service_template, + mock_has_permissions, + mock_get_service_statistics, + mock_get_job_doesnt_exist, + mock_get_jobs, + fake_uuid, + mocker, + uploaded_file_name, +): + mocker.patch('app.main.views.send.s3download', return_value=( + 'phone number,\n07900900321' + )) + + page = client_request.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_id="5d729fbd-239c-44ab-b498-75a985f3198f", + upload_id=fake_uuid, + original_file_name=uploaded_file_name, + _test_page_title=False, + ) + + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == ( + 'You already sent these messages ' + 'If you want to send the same messages again, rename the file ' + 'and re-upload it ' + 'Skip to file contents' + ) + + mock_get_jobs.assert_called_once_with(SERVICE_ONE_ID, limit_days=0) + + def test_check_messages_column_error_doesnt_show_optional_columns( mocker, client_request, @@ -2653,6 +2715,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, ): mocker.patch('app.main.views.send.s3download', return_value='\n'.join( @@ -2696,6 +2759,7 @@ def test_check_messages_adds_sender_id_in_session_to_metadata( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, fake_uuid, ): @@ -2744,6 +2808,7 @@ def test_letters_from_csv_files_dont_have_download_link( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_set_metadata, extra_args, ): @@ -2890,6 +2955,7 @@ def test_check_messages_shows_over_max_row_error( mock_has_permissions, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, mock_s3_download, fake_uuid, mocker @@ -3285,6 +3351,7 @@ def test_reply_to_is_previewed_if_chosen( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, get_default_reply_to_email_address, fake_uuid, endpoint, @@ -3335,6 +3402,7 @@ def test_sms_sender_is_previewed( mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, + mock_get_jobs, get_default_sms_sender, fake_uuid, endpoint, diff --git a/tests/conftest.py b/tests/conftest.py index 14de9ce8b..26a519c4a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1854,7 +1854,7 @@ def mock_has_no_jobs(mocker): def mock_get_jobs(mocker, api_user_active): def _get_jobs(service_id, limit_days=None, statuses=None, page=1): if statuses is None: - statuses = ['', 'scheduled', 'pending', 'cancelled'] + statuses = ['', 'scheduled', 'pending', 'cancelled', 'finished'] jobs = [ job_json( @@ -1862,16 +1862,17 @@ def mock_get_jobs(mocker, api_user_active): api_user_active, original_file_name=filename, scheduled_for=scheduled_for, - job_status=job_status + job_status=job_status, + template_version=template_version, ) - for filename, scheduled_for, job_status in ( - ('export 1/1/2016.xls', '', 'finished'), - ('all email addresses.xlsx', '', 'pending'), - ('applicants.ods', '', 'finished'), - ('thisisatest.csv', '', 'finished'), - ('send_me_later.csv', '2016-01-01 11:09:00.061258', 'scheduled'), - ('even_later.csv', '2016-01-01 23:09:00.061258', 'scheduled'), - ('full_of_regret.csv', '2016-01-01 23:09:00.061258', 'cancelled') + for filename, scheduled_for, job_status, template_version in ( + ('export 1/1/2016.xls', '', 'finished', 1), + ('all email addresses.xlsx', '', 'pending', 1), + ('applicants.ods', '', 'finished', 1), + ('thisisatest.csv', '', 'finished', 2), + ('send_me_later.csv', '2016-01-01 11:09:00.061258', 'scheduled', 1), + ('even_later.csv', '2016-01-01 23:09:00.061258', 'scheduled', 1), + ('full_of_regret.csv', '2016-01-01 23:09:00.061258', 'cancelled', 1) ) ] return {