Merge pull request #832 from alphagov/make-numbers-less-confusing

Make numbers on a job less confusing
2026-08-14 10:58:58 -04:00 · 2016-08-05 12:14:29 +01:00
parent 040af920e9 652534533b
commit e482404eea
8 changed files with 130 additions and 64 deletions
--- a/app/init.py
+++ b/app/init.py
@@ -243,7 +243,8 @@ def format_notification_status(status, template_type):
            'temporary-failure': 'Inbox not accepting messages right now',
            'permanent-failure': 'Email address doesn’t exist',
            'delivered': 'Delivered',
-            'sending': 'Sending'
+            'sending': 'Sending',
+            'created': 'Sending'
        },
        'sms': {
            'failed': 'Failed',
@@ -251,7 +252,8 @@ def format_notification_status(status, template_type):
            'temporary-failure': 'Phone not accepting messages right now',
            'permanent-failure': 'Phone number doesn’t exist',
            'delivered': 'Delivered',
-            'sending': 'Sending'
+            'sending': 'Sending',
+            'created': 'Sending'
        }
    }.get(template_type).get(status, status)

@@ -263,7 +265,8 @@ def format_notification_status_as_field_status(status):
        'temporary-failure': 'error',
        'permanent-failure': 'error',
        'delivered': None,
-        'sending': 'default'
+        'sending': 'default',
+        'created': 'default'
    }.get(status, 'error')


--- a/app/assets/stylesheets/_grids.scss
+++ b/app/assets/stylesheets/_grids.scss
@@ -31,6 +31,10 @@
  margin-bottom: $gutter-two-thirds;
 }

+.bottom-gutter-1-2 {
+  margin-bottom: $gutter-half;
+}
+
 .align-with-heading {
  display: block;
  text-align: center;
--- a/app/main/views/jobs.py
+++ b/app/main/views/jobs.py
@@ -1,8 +1,10 @@
 # -*- coding: utf-8 -*-
+import ago
 import time
 import dateutil
+from orderedset import OrderedSet
 from datetime import datetime, timedelta, timezone
-import ago
+from itertools import chain

 from flask import (
    render_template,
@@ -47,15 +49,14 @@ def _parse_filter_args(filter_dict):


 def _set_status_filters(filter_args):
+    status_filters = filter_args.get('status', [])
    all_failure_statuses = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
-    all_statuses = ['sending', 'delivered'] + all_failure_statuses
-    if filter_args.get('status'):
-        if 'processed' in filter_args.get('status') or not filter_args.get('status'):
-            filter_args['status'] = all_statuses
-        elif 'failed' in filter_args.get('status'):
-            filter_args['status'].extend(all_failure_statuses[1:])
-    else:
-        filter_args['status'] = all_statuses
+    all_sending_statuses = ['created', 'sending']
+    return list(OrderedSet(chain(
+        (status_filters or all_sending_statuses + ['delivered'] + all_failure_statuses),
+        all_sending_statuses if 'sending' in status_filters else [],
+        all_failure_statuses if 'failed' in status_filters else []
+    )))


@main.route("/services/<service_id>/jobs")
@@ -75,7 +76,7 @@ def view_job(service_id, job_id):

    job = job_api_client.get_job(service_id, job_id)['data']
    filter_args = _parse_filter_args(request.args)
-    _set_status_filters(filter_args)
+    filter_args['status'] = _set_status_filters(filter_args)

    return render_template(
        'views/jobs/job.html',
@@ -117,7 +118,7 @@ def view_job_csv(service_id, job_id):
        version=job['template_version']
    )['data']
    filter_args = _parse_filter_args(request.args)
-    _set_status_filters(filter_args)
+    filter_args['status'] = _set_status_filters(filter_args)

    return (
        generate_notifications_csv(
@@ -161,7 +162,7 @@ def view_notifications(service_id, message_type):
        abort(404)

    filter_args = _parse_filter_args(request.args)
-    _set_status_filters(filter_args)
+    filter_args['status'] = _set_status_filters(filter_args)

    notifications = notification_api_client.get_notifications_for_service(
        service_id=service_id,
@@ -210,7 +211,7 @@ def view_notifications(service_id, message_type):
        page=page,
        prev_page=prev_page,
        next_page=next_page,
-        request_args=request.args,
+        status=request.args.get('status'),
        message_type=message_type,
        download_link=url_for(
            '.view_notifications_csv',
@@ -250,7 +251,7 @@ def get_status_filters(service, message_type, statistics):

    filters = [
        # key, label, option
-        ('requested', 'processed', 'sending,delivered,failed'),
+        ('requested', 'total', 'sending,delivered,failed'),
        ('sending', 'sending', 'sending'),
        ('delivered', 'delivered', 'delivered'),
        ('failed', 'failed', 'failed'),
@@ -287,24 +288,22 @@ def _get_job_counts(job, help_argument):
            count
        ) for label, query_param, count in [
            [
-              'Processed', '',
-              job.get('notifications_sent', 0)
+              'total', '',
+              job.get('notification_count', 0)
            ],
            [
-              'Sending', 'sending',
-              (
-                  job.get('notifications_sent', 0) -
-                  job.get('notifications_delivered', 0) -
-                  job.get('notifications_failed', 0)
-              )
+              'sending', 'sending',
+              job.get('notification_count', 0) -
+              job.get('notifications_delivered', 0) -
+              job.get('notifications_failed', 0)
            ],
            [
-              'Delivered', 'delivered',
+              'delivered', 'delivered',
              job.get('notifications_delivered', 0)
            ],
            [
-              'Failed', 'failed',
-              job.get('notifications_failed')
+              'failed', 'failed',
+              job.get('notifications_failed', 0)
            ]
        ]
    ]
@@ -312,24 +311,27 @@ def _get_job_counts(job, help_argument):

 def get_job_partials(job):
    filter_args = _parse_filter_args(request.args)
-    _set_status_filters(filter_args)
+    filter_args['status'] = _set_status_filters(filter_args)
+    notifications = notification_api_client.get_notifications_for_service(
+        job['service'], job['id'], status=filter_args['status']
+    )
    return {
        'counts': render_template(
            'partials/jobs/count.html',
            job=job,
            counts=_get_job_counts(job, request.args.get('help', 0)),
-            status=request.args.get('status', '')
+            status=filter_args['status']
        ),
        'notifications': render_template(
            'partials/jobs/notifications.html',
-            notifications=notification_api_client.get_notifications_for_service(
-                job['service'], job['id'], status=filter_args.get('status')
-            )['notifications'],
+            notifications=notifications['notifications'],
+            more_than_one_page=bool(notifications.get('links', {}).get('next')),
+            percentage_complete=(job['notifications_sent'] / job['notification_count'] * 100),
            download_link=url_for(
                '.view_job_csv',
                service_id=current_service['id'],
                job_id=job['id'],
-                status=request.args.get('status', '')
+                status=request.args.get('status')
            ),
            help=get_help_argument(),
            time_left=get_time_left(job['created_at'])
--- a/app/templates/partials/jobs/notifications.html
+++ b/app/templates/partials/jobs/notifications.html
@@ -4,12 +4,18 @@
  <div class="dashboard-table">
 {% endif %}

-  {% if notifications and not help %}
-    <p class="bottom-gutter">
-      <a href="{{ download_link }}" download="download" class="heading-small">Download this report</a>
-      &emsp;
-      <span id="time-left">{{ time_left }}</span>
-    </p>
+  {% if not help %}
+    {% if percentage_complete < 100 %}
+      <p class="bottom-gutter-1-2 hint">
+        Report is {{ "{:.0f}%".format(percentage_complete) }} complete…
+      </p>
+    {% elif notifications %}
+      <p class="bottom-gutter-1-2">
+        <a href="{{ download_link }}" download="download" class="heading-small">Download this report</a>
+        &emsp;
+        <span id="time-left">{{ time_left }}</span>
+      </p>
+    {% endif %}
  {% endif %}

  {% call(item, row_number) list_table(
@@ -44,6 +50,12 @@
    {% endcall %}
  {% endcall %}

+  {% if more_than_one_page %}
+    <p class="table-show-more-link">
+      Only showing the first 50 rows
+    </p>
+  {% endif %}
+
 {% if notifications %}
  </div>
 {% endif %}
--- a/app/templates/views/dashboard/_jobs.html
+++ b/app/templates/views/dashboard/_jobs.html
@@ -23,7 +23,7 @@
    {% endcall %}
    {% call field() %}
      {{ big_number(
-        item.get('notifications_sent', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0),
+        item.get('notification_count', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0),
        smallest=True
      ) }}
    {% endcall %}
--- a/app/templates/views/notifications.html
+++ b/app/templates/views/notifications.html
@@ -14,9 +14,9 @@
  <h1 class="heading-large">

    <span class="visually-hidden">
-      {%- if request_args.get('status') != 'delivered,failed' -%}
+      {%- if status != 'delivered,failed' -%}
        {%- for label, option, _, _ in status_filters -%}
-          {%- if request_args.get('status', 'delivered,failed') == option -%}{{label}} {% endif -%}
+          {%- if status == option -%}{{label}} {% endif -%}
        {%- endfor -%}
      {%- endif -%}
    </span>
@@ -29,15 +29,15 @@
    {{ pill(
      'Status',
      status_filters,
-      request_args.get('status', '')
+      status
    ) }}
  </div>

  {% if notifications %}
-    <p class="bottom-gutter">
-      <a href="{{ download_link }}" download="download" class="heading-small">Download as a CSV file</a>
+    <p class="bottom-gutter-1-2">
+      <a href="{{ download_link }}" download="download" class="heading-small">Download this report</a>
      &emsp;
-      Delivery information is available for 7 days
+      Data available for 7 days
    </p>
  {% endif %}

--- a/tests/app/main/views/test_jobs.py
+++ b/tests/app/main/views/test_jobs.py
@@ -32,15 +32,11 @@ def test_should_return_list_of_all_jobs(app_,
    "status_argument, expected_api_call", [
        (
            '',
-            ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
-        ),
-        (
-            'processed',
-            ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
+            ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
        ),
        (
            'sending',
-            ['sending']
+            ['sending', 'created']
        ),
        (
            'delivered',
@@ -99,6 +95,7 @@ def test_should_show_page_for_one_job(
        )
        assert csv_link.text == 'Download this report'
        assert page.find('span', {'id': 'time-left'}).text == 'Data available for 7 days'
+        assert page.find('p', {'class': 'table-show-more-link'}).text.strip() == 'Only showing the first 50 rows'
        mock_get_notifications.assert_called_with(
            service_one['id'],
            fake_uuid,
@@ -106,6 +103,29 @@ def test_should_show_page_for_one_job(
        )


+def test_should_show_job_in_progress(
+    app_,
+    service_one,
+    active_user_with_permissions,
+    mock_get_service_template,
+    mock_get_job_in_progress,
+    mocker,
+    mock_get_notifications,
+    fake_uuid
+):
+    with app_.test_request_context(), app_.test_client() as client:
+        client.login(active_user_with_permissions, mocker, service_one)
+        response = client.get(url_for(
+            'main.view_job',
+            service_id=service_one['id'],
+            job_id=fake_uuid
+        ))
+
+        assert response.status_code == 200
+        page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+        assert page.find('p', {'class': 'hint'}).text.strip() == 'Report is 50% complete…'
+
+
 def test_should_show_not_show_csv_download_in_tour(
    app_,
    service_one,
@@ -179,12 +199,12 @@ def test_should_show_updates_for_one_job_as_json(
@pytest.mark.parametrize(
    "status_argument, expected_api_call", [
        (
-            'processed',
-            ['sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
+            '',
+            ['created', 'sending', 'delivered', 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure']
        ),
        (
            'sending',
-            ['sending']
+            ['sending', 'created']
        ),
        (
            'delivered',
@@ -275,10 +295,21 @@ def test_should_show_notifications_for_a_service_with_next_previous(
            ))
        assert response.status_code == 200
        content = response.get_data(as_text=True)
-        assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=3) in content
-        assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=1) in content
-        assert 'Previous page' in content
-        assert 'Next page' in content
+        page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+        next_page_link = page.find('a', {'rel': 'next'})
+        prev_page_link = page.find('a', {'rel': 'previous'})
+        assert (
+            url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=3) in
+            next_page_link['href']
+        )
+        assert 'Next page' in next_page_link.text.strip()
+        assert 'page 3' in next_page_link.text.strip()
+        assert (
+            url_for('main.view_notifications', service_id=service_one['id'], message_type='sms', page=1) in
+            prev_page_link['href']
+        )
+        assert 'Previous page' in prev_page_link.text.strip()
+        assert 'page 1' in prev_page_link.text.strip()


@freeze_time("2016-01-01 11:09:00.061258")
@@ -335,7 +366,7 @@ def test_get_status_filters_calculates_stats(app_):
        ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS)

    assert {label: count for label, _option, _link, count in ret} == {
-        'processed': 6,
+        'total': 6,
        'sending': 3,
        'failed': 2,
        'delivered': 1
@@ -347,7 +378,7 @@ def test_get_status_filters_in_right_order(app_):
        ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS)

    assert [label for label, _option, _link, _count in ret] == [
-        'processed', 'sending', 'delivered', 'failed'
+        'total', 'sending', 'delivered', 'failed'
    ]


--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -858,6 +858,18 @@ def mock_get_job(mocker, api_user_active):
    return mocker.patch('app.job_api_client.get_job', side_effect=_get_job)


+@pytest.fixture(scope='function')
+def mock_get_job_in_progress(mocker, api_user_active):
+    def _get_job(service_id, job_id):
+        return {"data": job_json(
+            service_id, api_user_active, job_id=job_id,
+            notification_count=10,
+            notifications_sent=5
+        )}
+
+    return mocker.patch('app.job_api_client.get_job', side_effect=_get_job)
+
+
@pytest.fixture(scope='function')
 def mock_get_jobs(mocker, api_user_active):
    def _get_jobs(service_id, limit_days=None):
@@ -898,14 +910,16 @@ def mock_get_notifications(mocker, api_user_active):
                template={'template_type': set_template_type, 'name': 'name', 'id': 'id', 'version': 1},
                rows=rows,
                status=set_status,
-                job=job
+                job=job,
+                with_links=True
            )
        else:
            return notification_json(
                service_id,
                rows=rows,
                status=set_status,
-                job=job
+                job=job,
+                with_links=True
            )

    return mocker.patch(