From f030d1cb8a0e8e859ff12d633be268602ee3c118 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 17 Jun 2016 11:54:01 +0100 Subject: [PATCH] Move check_messages in admin over to using get notification stats for day. --- app/main/views/send.py | 9 ++- app/notify_client/statistics_api_client.py | 11 +++ .../notify_client/test_statistics_client.py | 17 +++++ tests/app/main/views/test_send.py | 72 ++++++++++++++++--- tests/conftest.py | 20 ++++++ 5 files changed, 116 insertions(+), 13 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 77c9f550a..929d4320d 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -1,5 +1,6 @@ import json import itertools +from datetime import datetime from string import ascii_uppercase from contextlib import suppress @@ -205,12 +206,16 @@ def send_from_api(service_id, template_id): @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def check_messages(service_id, template_type, upload_id): + if not session.get('upload_data'): 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) - statistics = statistics_api_client.get_statistics_for_service(service_id, limit_days=1)['data'] - statistics = statistics[0] if statistics else {} + today = datetime.today().date().strftime('%Y-%m-%d') + + statistics = statistics_api_client.get_statistics_for_service_for_day(service_id, today) + if not statistics: + statistics = {} contents = s3download(service_id, upload_id) if not contents: diff --git a/app/notify_client/statistics_api_client.py b/app/notify_client/statistics_api_client.py index 33e5a5155..e9250b1f9 100644 --- a/app/notify_client/statistics_api_client.py +++ b/app/notify_client/statistics_api_client.py @@ -1,4 +1,5 @@ from notifications_python_client.base import BaseAPIClient +from notifications_python_client.errors import HTTPError class StatisticsApiClient(BaseAPIClient): @@ -21,6 +22,16 @@ class StatisticsApiClient(BaseAPIClient): params=params ) + def get_statistics_for_service_for_day(self, service_id, day): + url = '/service/{}/notifications-statistics/day/{}'.format(service_id, day) + try: + return self.get(url=url)['data'] + except HTTPError as e: + if e.status_code == 404: + return None + else: + raise e + def get_7_day_aggregate_for_service(self, service_id, date_from=None, week_count=None): params = {} if date_from is not None: diff --git a/tests/app/main/notify_client/test_statistics_client.py b/tests/app/main/notify_client/test_statistics_client.py index 70a83f795..200958d2b 100644 --- a/tests/app/main/notify_client/test_statistics_client.py +++ b/tests/app/main/notify_client/test_statistics_client.py @@ -1,4 +1,5 @@ import uuid +from datetime import datetime from app.notify_client.statistics_api_client import StatisticsApiClient @@ -29,3 +30,19 @@ def test_notifications_statistics_client_calls_correct_api_endpoint_with_params( client.get_statistics_for_service(some_service_id, limit_days=99) mock_get.assert_called_once_with(url=expected_url, params={'limit_days': 99}) + + +def test_notifications_statistics_client_for_stats_by_day_calls_correct_api_endpoint(mocker, api_user_active): + + some_service_id = uuid.uuid4() + today = datetime.today().date().strftime('%Y-%m-%d') + + expected_url = '/service/{}/notifications-statistics/day/{}'.format(some_service_id, today) + + client = StatisticsApiClient() + + mock_get = mocker.patch('app.notify_client.statistics_api_client.StatisticsApiClient.get') + + client.get_statistics_for_service_for_day(some_service_id, today) + + mock_get.assert_called_once_with(url=expected_url) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index fdee927f4..58fbe8efd 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -6,8 +6,8 @@ from os import path from glob import glob from bs4 import BeautifulSoup from flask import url_for -from unittest.mock import ANY -from tests import (validate_route_permission, job_json) +from tests import validate_route_permission +from datetime import datetime template_types = ['email', 'sms'] @@ -76,7 +76,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, + mock_get_service_statistics_for_day, fake_uuid ): @@ -115,14 +115,16 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( def test_upload_csv_invalid_extension(app_, api_user_active, + mocker, mock_login, mock_get_service, mock_get_service_template, mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics, + mock_get_service_statistics_for_day, fake_uuid): + with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -147,7 +149,7 @@ def test_send_test_sms_message( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics, + mock_get_service_statistics_for_day, fake_uuid ): @@ -175,7 +177,7 @@ def test_send_test_email_message( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics, + mock_get_service_statistics_for_day, fake_uuid ): @@ -203,7 +205,6 @@ def test_send_test_sms_message_with_placeholders( mock_s3_upload, mock_has_permissions, mock_get_users_by_service, - mock_get_service_statistics, fake_uuid ): @@ -212,6 +213,7 @@ 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: @@ -284,7 +286,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, + mock_get_service_statistics_for_day, fake_uuid ): @@ -316,6 +318,9 @@ 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) + def test_create_job_should_call_api( app_, @@ -360,7 +365,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( mock_s3_upload, mocker, mock_has_permissions, - mock_get_service_statistics, + mock_get_service_statistics_for_day, mock_get_users_by_service, fake_uuid ): @@ -601,7 +606,7 @@ def test_check_messages_back_link_with_help(app_, mock_get_service, mock_get_service_template_with_placeholders, mock_has_permissions, - mock_get_service_statistics, + mock_get_service_statistics_for_day, mock_s3_download, fake_uuid): with app_.test_request_context(): @@ -633,7 +638,7 @@ def test_check_messages_back_link_without_help(app_, mock_get_service, mock_get_service_template_with_placeholders, mock_has_permissions, - mock_get_service_statistics, + mock_get_service_statistics_for_day, mock_s3_download, fake_uuid): with app_.test_request_context(): @@ -654,3 +659,48 @@ def test_check_messages_back_link_without_help(app_, assert response.status_code == 200 content = response.get_data(as_text=True) assert url_for('.send_test', service_id=fake_uuid, template_id=fake_uuid) in content + + +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() == 'Check and confirm' + mock_get_stats.assert_called_once_with(fake_uuid, today) diff --git a/tests/conftest.py b/tests/conftest.py index ddee3ae45..8036a8e74 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -193,6 +193,26 @@ def mock_get_service_statistics(mocker): 'app.statistics_api_client.get_statistics_for_service', side_effect=_create) +@pytest.fixture(scope='function') +def mock_get_service_statistics_for_day(mocker): + + stats = {'day': datetime.today().date().strftime('%Y-%m-%d'), + 'emails_delivered': 0, + 'sms_requested': 0, + 'sms_delivered': 0, + 'sms_failed': 0, + 'emails_requested': 0, + 'emails_failed': 0, + 'service': fake_uuid, + 'id': fake_uuid} + + def _stats(service_id, day): + return {'data': stats} + + return mocker.patch( + 'app.statistics_api_client.get_statistics_for_service_for_day', side_effect=_stats) + + @pytest.fixture(scope='function') def mock_get_aggregate_service_statistics(mocker): def _create(service_id, limit_days=None):