From d82569f8834a50780d15ab840bdd3c7478b1fb41 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 29 Apr 2016 15:24:48 +0100 Subject: [PATCH 1/4] Fix page titles on activity page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were getting some weirdness like ‘Failed both’. This commit fixes the problem, and adds some tests for the page headings to make sure they don’t break again. --- app/main/views/jobs.py | 6 ++-- app/templates/views/notifications.html | 36 +++++++++----------- tests/app/main/views/test_jobs.py | 47 +++++++++++++++++++++++--- 3 files changed, 62 insertions(+), 27 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index b541ffa32..e78375f07 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -116,6 +116,8 @@ def view_notifications(service_id): filter_args = _parse_filter_args(request.args) + print(filter_args) + notifications = notification_api_client.get_notifications_for_service( service_id=service_id, page=page, @@ -147,8 +149,8 @@ def view_notifications(service_id): service_id=service_id, page=page, page_size=notifications['total'], - template_type=filter_args.getlist('template_type') if 'template_type' in filter_args else None, - status=filter_args.getlist('status') + template_type=filter_args.get('template_type') if 'template_type' in filter_args else ['email', 'sms'], + status=filter_args.get('status') if 'status' in filter_args else ['delivered', 'failed'], limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])['notifications']) return csv_content, 200, { diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 2717be8d0..bcc6a1aeb 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -11,36 +11,32 @@ {% block maincolumn_content %}

- - {%- if (request_args.get('template_type', '') == '') and (request_args.get('status', 'delivered,failed') == 'delivered,failed') -%} + {%- if (request_args.get('template_type', 'email,sms') == 'email,sms') and (request_args.get('status', 'delivered,failed') == 'delivered,failed') -%} Activity {%- else -%} - {% if request_args.get('status') != 'delivered,failed' %} - {% for label, option, _ in status_filters %} - {% if request_args.get('status') == option %} - {{ label }} - {% endif %} - {% endfor %} - {% endif %} + {%- if request_args.get('status') != 'delivered,failed' -%} + {%- for label, option, _ in status_filters -%} + {%- if request_args.get('status', 'delivered,failed') == option -%}{{label}} {% endif -%} + {%- endfor -%} + {%- endif -%} - {% if request_args.get('template_type') == '' %} - emails and text messages - {% else %} + {%- if request_args.get('template_type', 'email,sms') == 'email,sms' %} emails and text messages + {%- else -%} - {% for template_label, template_option, _ in type_filters %} - {% if request_args.get('template_type') == template_option %} - {% if request_args.get('status', 'delivered,failed') == 'delivered,failed' %} + {%- for template_label, template_option, _ in type_filters -%} + {%- if request_args.get('template_type') == template_option -%} + {%- if request_args.get('status', 'delivered,failed') == 'delivered,failed' -%} {{ template_label }} - {% else %} + {%- else -%} {{ template_label | lower }} - {% endif %} - {% endif %} - {% endfor %} + {%- endif -%} + {%- endif -%} + {%- endfor -%} - {% endif %} + {%- endif -%} {%- endif -%} diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 68773880d..09c2f5fdf 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -107,6 +107,8 @@ def test_should_show_notifications_for_a_service(app_, assert notification['status'] in content assert notification['template']['name'] in content assert '.csv' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Activity' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed'], template_type=['email', 'sms']) # noqa @@ -126,7 +128,8 @@ def test_can_view_only_sms_notifications_for_a_service(app_, response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - template_type=['sms'])) + template_type='sms', + status='delivered,failed')) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -136,6 +139,8 @@ def test_can_view_only_sms_notifications_for_a_service(app_, assert notification['status'] in content assert notification['template']['name'] in content assert '.csv' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Text messages' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed'], template_type=['sms']) # noqa @@ -155,8 +160,8 @@ def test_can_view_only_email_notifications_for_a_service(app_, response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - status=['delivered', 'failed'], - template_type=['email'])) + status='delivered,failed', + template_type='email')) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -166,6 +171,8 @@ def test_can_view_only_email_notifications_for_a_service(app_, assert notification['status'] in content assert notification['template']['name'] in content assert '.csv' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Emails' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered', 'failed'], template_type=['email']) # noqa @@ -185,7 +192,7 @@ def test_can_view_successful_notifications_for_a_service(app_, response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - status=['delivered'])) + status='delivered')) assert response.status_code == 200 content = response.get_data(as_text=True) notifications = notification_json(service_one['id']) @@ -194,6 +201,8 @@ def test_can_view_successful_notifications_for_a_service(app_, assert notification['status'] in content assert notification['template']['name'] in content assert '.csv' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Successful emails and text messages' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['delivered'], template_type=['email', 'sms']) # noqa @@ -213,7 +222,7 @@ def test_can_view_failed_notifications_for_a_service(app_, response = client.get(url_for( 'main.view_notifications', service_id=service_one['id'], - status=['failed'])) + status='failed')) assert response.status_code == 200 content = response.get_data(as_text=True) notifications = notification_json(service_one['id']) @@ -222,10 +231,38 @@ def test_can_view_failed_notifications_for_a_service(app_, assert notification['status'] in content assert notification['template']['name'] in content assert '.csv' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Failed emails and text messages' mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed'], template_type=['email', 'sms']) # noqa +def test_can_view_failed_combination_of_notification_type_and_status( + app_, + service_one, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service, + mock_get_notifications, + mock_has_permissions +): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for( + 'main.view_notifications', + service_id=service_one['id'], + status='failed', + template_type='sms')) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Failed text messages' + + mock_get_notifications.assert_called_with(limit_days=7, page=1, service_id=service_one['id'], status=['failed'], template_type=['sms']) # noqa + + def test_should_show_notifications_for_a_service_with_next_previous(app_, service_one, api_user_active, From 43ef3e86ae9e1578aacc42db07c913ea8d253abd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 29 Apr 2016 15:47:31 +0100 Subject: [PATCH 2/4] Make big numbers on the homepage linkable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The big numbers on the home page relate directly to the notifications on the notification page. So let’s link them. With a _hyper_ link. This commit actually adds two links, one of which is semantically correct, and one of which is visually correct, ie makes the whole black area of the box clickable/hoverable. --- .../stylesheets/components/big-number.scss | 25 +++++++++++++++++++ app/templates/components/big-number.html | 21 +++++++++++++--- app/templates/views/dashboard/today.html | 6 +++-- 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/components/big-number.scss b/app/assets/stylesheets/components/big-number.scss index 5a7d8cd65..8031ad980 100644 --- a/app/assets/stylesheets/components/big-number.scss +++ b/app/assets/stylesheets/components/big-number.scss @@ -16,13 +16,38 @@ @extend %big-number; background: $text-colour; color: $white; + position: relative; .big-number { padding: 15px; + position: relative; + } + + .big-number-overlay-link { + + background: transparent; + position: absolute; + top: 0; + left: 0; + background: transparent; + width: 100%; + height: 100%; + + &:hover { + background: rgba($text-colour, 0.2); + } + } .big-number-label { + padding-bottom: 0; + + &:link, + &:visited { + color: $white; + } + } %big-number-status, diff --git a/app/templates/components/big-number.html b/app/templates/components/big-number.html index 6127c0564..c856544a4 100644 --- a/app/templates/components/big-number.html +++ b/app/templates/components/big-number.html @@ -1,18 +1,31 @@ -{% macro big_number(number, label) %} +{% macro big_number(number, label, label_link=None) %}
{% if number is number %} {{ "{:,}".format(number) }} {% else %} {{ number }} {% endif %} - {{ label }} + {% if label_link %} + {{ label }} + + {% else %} + {{ label }} + {% endif %}
{% endmacro %} -{% macro big_number_with_status(number, label, failures, failure_percentage, danger_zone=False, failure_link=None) %} +{% macro big_number_with_status( + number, + label, + failures, + failure_percentage, + danger_zone=False, + failure_link=None, + label_link=None +) %}
- {{ big_number(number, label) }} + {{ big_number(number, label, label_link) }}
{% if failures %} {% if failure_link %} diff --git a/app/templates/views/dashboard/today.html b/app/templates/views/dashboard/today.html index b12390ccf..42fd196b4 100644 --- a/app/templates/views/dashboard/today.html +++ b/app/templates/views/dashboard/today.html @@ -18,7 +18,8 @@ statistics.emails_failed, statistics.get('emails_failure_rate', 0.0), statistics.get('emails_failure_rate', 0)|float > 3, - failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='email', status='failed') + failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='email', status='failed'), + label_link=url_for(".view_notifications", service_id=current_service.id, template_type='email', status='delivered,failed') ) }}
@@ -28,7 +29,8 @@ statistics.sms_failed, statistics.get('sms_failure_rate', 0.0), statistics.get('sms_failure_rate', 0)|float > 3, - failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='sms', status='failed') + failure_link=url_for(".view_notifications", service_id=current_service.id, template_type='sms', status='failed'), + label_link=url_for(".view_notifications", service_id=current_service.id, template_type='sms', status='delivered,failed') ) }}
From 744064e8403b4b0797135268f88577269957c227 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 29 Apr 2016 15:53:10 +0100 Subject: [PATCH 3/4] Remove activity from the nav MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We saw lots of people in the lab clicking activity hoping for… something. But it’s not really where you go to do a thing, so they weren’t finding what they were looking for. Since you can now get to the activity from the dashboard, let’s remove the link in the nav, to make thing less ambiguous. --- app/templates/main_nav.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index c0f687a27..2d69364bf 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -3,9 +3,6 @@ {{ current_service.name }}