Merge pull request #809 from alphagov/remove-stats-from-send

don't hit statistics endpoints from send

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)
+    statistics = service_api_client.get_detailed_service_for_today(service_id)['data']['statistics']
-    if not statistics:
+    remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values()))
-        statistics = {}
 
     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()
     )

app/notify_client/service_api_client.py

@@ -42,16 +42,27 @@ 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, detailed=False, today_only=False)
 
     def get_detailed_service(self, service_id):
-        return self._get_service(service_id, True)
+        return self._get_service(service_id, detailed=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, detailed=True, 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 {}
+        params = {}
+        if detailed:
+            params['detailed'] = detailed
+        if today_only:
+            params['today_only'] = today_only
+
         return self.get(
             '/service/{0}'.format(service_id),
             params=params)

app/notify_client/statistics_api_client.py

@@ -1,5 +1,4 @@
 from notifications_python_client.base import BaseAPIClient
-from notifications_python_client.errors import HTTPError
 
 
 class StatisticsApiClient(BaseAPIClient):

app/templates/partials/check/too-many-messages.html

@@ -11,10 +11,8 @@
       .
     </p>
     <p>
-      {% if statistics.emails_requested or statistics.sms_requested %}
+      {% if current_service.message_limit != remaining_messages %}
-        You can still send
+        You can still send {{ remaining_messages }} messages today, but
-        {{ current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0) }}
-        messages today, but
       {% endif %}
       ‘{{ original_file_name }}’ contains
       {{ count_of_recipients }} {{ recipients.recipient_column_header }}

app/templates/views/check.html

@@ -103,8 +103,8 @@
       {% endcall %}
     </div>
 
-  {% 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" %}
+    {% include "partials/check/too-many-messages.html" %}
   {% else %}
 
     <h1 class="heading-large">
@@ -134,10 +134,7 @@
     </div>
   {% endif %}
 
-  {% if (
+  {% if errors %}
-    errors or
-    count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0))
-  ) %}
     {% if request.args.from_test %}
       <a href="{{ back_link }}" class="page-footer-back-link">Back</a>
     {% else %}

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_detailed_service_for_today.assert_called_once_with(fake_uuid)
-            mock_get_service_statistics_for_day.assert_called_once_with(fake_uuid, today)
 
 
 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,expected_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
+    expected_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={
+    mocker.patch('app.service_api_client.get_detailed_service_for_today', return_value={
-        'day': datetime.today().date().strftime('%Y-%m-%d'),
+        'data': {
-        'sms_requested': num_already_sent, 'sms_delivered': num_already_sent, 'sms_failed': 0,
+            'statistics': {
-        'emails_requested': 0, 'emails_delivered': 0, 'emails_failed': 0, 'service': fake_uuid, 'id': fake_uuid
+                '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:
@@ -769,4 +726,4 @@ def test_check_messages_shows_too_many_messages_errors(
     # remove excess whitespace from element
     details = page.find('div', class_='banner-dangerous').findAll('p')[1]
     details = ' '.join([line.strip() for line in details.text.split('\n') if line.strip() != ''])
-    assert details == msg
+    assert details == expected_msg

tests/app/notify_client/test_service_api_client.py

@@ -1,3 +1,5 @@
+import pytest
+
 from app.notify_client.service_api_client import ServiceAPIClient
 from tests.conftest import fake_uuid
 
@@ -21,17 +23,17 @@ def test_client_posts_archived_true_when_deleting_template(mocker):
     mock_post.assert_called_once_with(expected_url, data=expected_data)
 
 
-def test_client_gets_service_with_no_params(mocker):
+@pytest.mark.parametrize(
+    'function,params', [
+        (ServiceAPIClient.get_service, {}),
+        (ServiceAPIClient.get_detailed_service, {'detailed': True}),
+        (ServiceAPIClient.get_detailed_service_for_today, {'detailed': True, 'today_only': True})
+    ],
+    ids=lambda x: x.__name__
+)
+def test_client_gets_service(mocker, function, params):
     client = ServiceAPIClient()
-    mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.get')
+    mock_get = mocker.patch.object(client, 'get')
 
-    client.get_service('foo')
+    function(client, 'foo')
-    mock_post.assert_called_once_with('/service/foo', params={})
+    mock_get.assert_called_once_with('/service/foo', params=params)
-
-
-def test_client_gets_service_with_detailed_params(mocker):
-    client = ServiceAPIClient()
-    mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.get')
-
-    client.get_detailed_service('foo')
-    mock_post.assert_called_once_with('/service/foo', params={'detailed': True})

tests/app/notify_client/test_statistics_client.py

@@ -1,20 +0,0 @@
-import uuid
-from datetime import datetime
-
-from app.notify_client.statistics_api_client import StatisticsApiClient
-
-
-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)

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):