diff --git a/app/main/views/send.py b/app/main/views/send.py index 07aa0eb52..edf8244f6 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -212,11 +212,9 @@ def check_messages(service_id, template_type, upload_id): return redirect(url_for('main.choose_template', service_id=service_id, template_type=template_type)) users = user_api_client.get_users_for_service(service_id=service_id) - today = datetime.utcnow().date().strftime('%Y-%m-%d') - statistics = statistics_api_client.get_statistics_for_service_for_day(service_id, today) - if not statistics: - statistics = {} + statistics = service_api_client.get_detailed_service_for_today(service_id)['data']['statistics'] + remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values())) contents = s3download(service_id, upload_id) if not contents: @@ -240,7 +238,8 @@ def check_messages(service_id, template_type, upload_id): max_errors_shown=50, whitelist=itertools.chain.from_iterable( [user.mobile_number, user.email_address] for user in users - ) if current_service['restricted'] else None + ) if current_service['restricted'] else None, + remaining_messages=remaining_messages ) if request.args.get('from_test'): @@ -278,7 +277,7 @@ def check_messages(service_id, template_type, upload_id): original_file_name=session['upload_data'].get('original_file_name'), upload_id=upload_id, form=CsvUploadForm(), - statistics=statistics, + remaining_messages=remaining_messages, back_link=back_link, help=get_help_argument() ) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index f2896a49e..4e9e84c2b 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -42,14 +42,20 @@ class ServiceAPIClient(NotificationsAPIClient): return self.delete(endpoint, data) def get_service(self, service_id): - return self._get_service(service_id, False) + return self._get_service(service_id, False, today_only=False) def get_detailed_service(self, service_id): - return self._get_service(service_id, True) + return self._get_service(service_id, True, today_only=False) - def _get_service(self, service_id, detailed): + def get_detailed_service_for_today(self, service_id): + return self._get_service(service_id, False, today_only=True) + + def _get_service(self, service_id, detailed, today_only): """ Retrieve a service. + + :param detailed - return additional details, including notification statistics + :param today_only - return statistics only for today. No effect if detailed not passed in """ params = {'detailed': True} if detailed else {} return self.get( diff --git a/app/templates/partials/check/too-many-messages.html b/app/templates/partials/check/too-many-messages.html index 457c7321e..b063640ee 100644 --- a/app/templates/partials/check/too-many-messages.html +++ b/app/templates/partials/check/too-many-messages.html @@ -11,10 +11,8 @@ .

- {% if statistics.emails_requested or statistics.sms_requested %} - You can still send - {{ current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0) }} - messages today, but + {% if current_service.message_limit != remaining_messages %} + You can still send {{ remaining_messages }} messages today, but {% endif %} ‘{{ original_file_name }}’ contains {{ count_of_recipients }} {{ recipients.recipient_column_header }} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 4aadfcaf5..cb073ef72 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -103,7 +103,7 @@ {% endcall %} - {% elif count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) %} + {% elif recipients.more_rows_than_can_send %} {% include "partials/check/too-many-messages.html" %} {% else %} @@ -134,10 +134,7 @@ {% endif %} - {% if ( - errors or - count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) - ) %} + {% if errors %} {% if request.args.from_test %} Back {% else %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 026dc55cf..65d292e62 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -77,7 +77,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, fake_uuid ): @@ -123,7 +123,7 @@ def test_upload_csv_invalid_extension(app_, mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, fake_uuid): with app_.test_request_context(): @@ -150,7 +150,7 @@ def test_send_test_sms_message( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, fake_uuid ): @@ -178,7 +178,7 @@ def test_send_test_email_message( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, fake_uuid ): @@ -206,6 +206,7 @@ def test_send_test_sms_message_with_placeholders( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, + mock_get_detailed_service_for_today, fake_uuid ): @@ -214,7 +215,6 @@ def test_send_test_sms_message_with_placeholders( 'file_name': 'Test message' } mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') - mocker.patch('app.statistics_api_client.get_statistics_for_service_for_day', return_value=None) with app_.test_request_context(): with app_.test_client() as client: @@ -287,7 +287,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, fake_uuid ): @@ -319,8 +319,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( assert '07700 900750' not in content assert 'Only showing the first 50 rows' in content - today = datetime.today().date().strftime('%Y-%m-%d') - mock_get_service_statistics_for_day.assert_called_once_with(fake_uuid, today) + mock_get_detailed_service_for_today.assert_called_once_with(fake_uuid) def test_create_job_should_call_api( @@ -366,7 +365,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( mock_s3_upload, mocker, mock_has_permissions, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, mock_get_users_by_service, fake_uuid ): @@ -625,7 +624,7 @@ def test_check_messages_back_link( mock_get_service, mock_get_service_template_with_placeholders, mock_has_permissions, - mock_get_service_statistics_for_day, + mock_get_detailed_service_for_today, mock_s3_download, fake_uuid, extra_args, @@ -653,51 +652,6 @@ def test_check_messages_back_link( ) == expected_url(service_id=fake_uuid, template_id=fake_uuid) -def test_send_and_check_page_renders_if_no_statistics( - app_, - mocker, - api_user_active, - mock_login, - mock_get_service, - mock_get_service_template, - mock_s3_upload, - mock_has_permissions, - mock_get_users_by_service, - fake_uuid -): - - mock_get_stats = mocker.patch('app.statistics_api_client.get_statistics_for_service_for_day', return_value=None) - - mocker.patch( - 'app.main.views.send.s3download', - return_value=""" - phone number - +447700900986 - """ - ) - - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - session['upload_data'] = {'original_file_name': 'some.csv', - 'template_id': fake_uuid, - 'notification_count': 0, - 'valid': True} - resp = client.post( - url_for('main.check_messages', service_id=fake_uuid, template_type='sms', upload_id='abc123'), - data={'file': (BytesIO(''.encode('utf-8')), 'some.csv')}, - content_type='multipart/form-data', - follow_redirects=True - ) - - today = datetime.today().date().strftime('%Y-%m-%d') - assert resp.status_code == 200 - page = BeautifulSoup(resp.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Preview' - mock_get_stats.assert_called_once_with(fake_uuid, today) - - def test_go_to_dashboard_after_tour( app_, mocker, @@ -721,7 +675,7 @@ def test_go_to_dashboard_after_tour( mock_delete_service_template.assert_called_once_with(fake_uuid, fake_uuid) -@pytest.mark.parametrize('num_already_sent,msg', [ +@pytest.mark.parametrize('num_requested,msg', [ (0, '‘valid.csv’ contains 100 phone numbers.'), (1, 'You can still send 49 messages today, but ‘valid.csv’ contains 100 phone numbers.') ], ids=['none_sent', 'some_sent']) @@ -735,17 +689,20 @@ def test_check_messages_shows_too_many_messages_errors( mock_get_service_template, mock_has_permissions, fake_uuid, - num_already_sent, + num_requested, msg ): # csv with 100 phone numbers mocker.patch('app.main.views.send.s3download', return_value=',\n'.join( ['phone number'] + ([mock_get_users_by_service(None)[0]._mobile_number]*100) )) - mocker.patch('app.statistics_api_client.get_statistics_for_service_for_day', return_value={ - 'day': datetime.today().date().strftime('%Y-%m-%d'), - 'sms_requested': num_already_sent, 'sms_delivered': num_already_sent, 'sms_failed': 0, - 'emails_requested': 0, 'emails_delivered': 0, 'emails_failed': 0, 'service': fake_uuid, 'id': fake_uuid + mocker.patch('app.service_api_client.get_detailed_service_for_today', return_value={ + 'data': { + 'statistics': { + 'sms': {'requested': num_requested, 'delivered': 0, 'failed': 0}, + 'email': {'requested': 0, 'delivered': 0, 'failed': 0} + } + } }) with app_.test_request_context(), app_.test_client() as client: diff --git a/tests/conftest.py b/tests/conftest.py index 15b4dd777..619089fb1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -80,6 +80,22 @@ def mock_get_detailed_service(mocker, api_user_active): return mocker.patch('app.service_api_client.get_detailed_service', side_effect=_get) +@pytest.fixture(scope='function') +def mock_get_detailed_service_for_today(mocker, api_user_active): + def _get(service_id): + return { + 'data': { + 'id': service_id, + 'statistics': { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + } + } + } + + return mocker.patch('app.service_api_client.get_detailed_service_for_today', side_effect=_get) + + @pytest.fixture(scope='function') def mock_get_live_service(mocker, api_user_active): def _get(service_id):