Merge pull request #582 from alphagov/better-errors-in-trial-mode

Make ‘Too many recipients’ error lower priority

app/templates/views/check.html

@@ -13,31 +13,20 @@
 
 {% block maincolumn_content %}
 
-  {% if count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) %}
+  {% if not recipients.allowed_to_send_to and not recipients.missing_column_headers %}
     <div class="bottom-gutter">
       {% call banner_wrapper(type='dangerous') %}
         <h1 class='banner-title'>
-          Too many recipients
-        </h1>
-        {% if statistics.emails_requested or statistics.sms_requested %}
-          <p>
-            You can only send {{ current_service.message_limit }}
-            messages per day
-            {%- if current_service.restricted %}
-              in <a href="{{ url_for('.trial_mode')}}">trial mode</a>
-            {%- endif -%}
-            .
-          </p>
-        {% endif %}
-        <p>
-          You can still send
-          {{ current_service.message_limit - statistics.emails_requested - statistics.sms_requested }}
-          messages today, but
-          ‘{{ original_file_name }}’ contains
-          {{ count_of_recipients }} {{ recipients.recipient_column_header }}
+          You can’t send to
+          {{ 'this' if count_of_recipients == 1 else 'these' }}
+          {{ recipients.recipient_column_header }}
           {%- if count_of_recipients != 1 -%}
             {{ 'es' if 'email address' == recipients.recipient_column_header else 's' }}
-          {%- endif -%}.
+          {%- endif %}
+        </h1>
+        <p>
+          In <a href="{{ url_for('.trial_mode') }}">trial mode</a> you can only
+          send to yourself and members of your team
         </p>
       {% endcall %}
     </div>
@@ -66,6 +55,34 @@
         {% endif %}
       {% endcall %}
     </div>
+  {% elif count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) %}
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous') %}
+        <h1 class='banner-title'>
+          Too many recipients
+        </h1>
+        {% if statistics.emails_requested or statistics.sms_requested %}
+          <p>
+            You can only send {{ current_service.message_limit }}
+            messages per day
+            {%- if current_service.restricted %}
+              in <a href="{{ url_for('.trial_mode')}}">trial mode</a>
+            {%- endif -%}
+            .
+          </p>
+        {% endif %}
+        <p>
+          You can still send
+          {{ current_service.message_limit - statistics.emails_requested - statistics.sms_requested }}
+          messages today, but
+          ‘{{ original_file_name }}’ contains
+          {{ count_of_recipients }} {{ recipients.recipient_column_header }}
+          {%- if count_of_recipients != 1 -%}
+            {{ 'es' if 'email address' == recipients.recipient_column_header else 's' }}
+          {%- endif -%}.
+        </p>
+      {% endcall %}
+    </div>
   {% else %}
     <h1 class="heading-large">
       Check and confirm
@@ -95,7 +112,7 @@
 
   {% if (
     errors or
-    count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) 
+    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>

requirements.txt

@@ -18,4 +18,4 @@ pytz==2016.4
 
 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@5.2.5#egg=notifications-utils==5.2.5
+git+https://github.com/alphagov/notifications-utils.git@5.3.0#egg=notifications-utils==5.3.0

tests/app/main/views/test_send.py

@@ -72,7 +72,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     mocker,
     mock_login,
     mock_get_service,
-    mock_get_service_template,
+    mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mock_has_permissions,
     mock_get_users_by_service,
@@ -80,21 +80,27 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     fake_uuid
 ):
 
-    contents = u'phone number,name\n+44 123,test1\n+44 456,test2'
-    mocker.patch('app.main.views.send.s3download', return_value=contents)
+    mocker.patch(
+        'app.main.views.send.s3download',
+        return_value="""
+            phone number,name
+            +447700900986
+            +447700900986
+        """
+    )
 
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(api_user_active)
             initial_upload = client.post(
                 url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid),
-                data={'file': (BytesIO(contents.encode('utf-8')), 'invalid.csv')},
+                data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
                 content_type='multipart/form-data',
                 follow_redirects=True
             )
             reupload = client.post(
                 url_for('main.check_messages', service_id=fake_uuid, template_type='sms', upload_id='abc123'),
-                data={'file': (BytesIO(contents.encode('utf-8')), 'invalid.csv')},
+                data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
                 content_type='multipart/form-data',
                 follow_redirects=True
             )
@@ -102,9 +108,8 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
             assert response.status_code == 200
             content = response.get_data(as_text=True)
             assert 'There was a problem with invalid.csv' in content
-            assert '+44 123' in content
-            assert '+44 456' in content
-            assert 'Not a UK mobile number' in content
+            assert '+447700900986' in content
+            assert 'Missing' in content
             assert 'Re-upload your file' in content
 
 
@@ -354,7 +359,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
     active_user_with_permissions,
     job_data,
     mock_create_job,
-    mock_get_service_template,
+    mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mocker,
     mock_has_permissions,
@@ -368,8 +373,8 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
         'app.main.views.send.s3download',
         return_value="""
             phone number,name,,,
-            123,test1,,,
-            123,test2,,,
+            +447700900986,,,,
+            +447700900986,,,,
         """
     )
     with app_.test_request_context():