From ec7ec77b6984eb79b0181f7c7cb27b9e4b44ce18 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 14:45:41 +0000 Subject: [PATCH 1/7] Return the email and sms stats instead of summing them --- app/main/views/platform_admin.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 7c883d7e9..46bb030d8 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -1,7 +1,5 @@ import itertools -from datetime import datetime -import pytz from flask import render_template from flask_login import login_required @@ -63,13 +61,10 @@ def create_global_stats(services): def format_stats_by_service(services): for service in services: - stats = service['statistics'].values() yield { 'id': service['id'], 'name': service['name'], - 'sending': sum((stat['requested'] - stat['delivered'] - stat['failed']) for stat in stats), - 'delivered': sum(stat['delivered'] for stat in stats), - 'failed': sum(stat['failed'] for stat in stats), + 'stats': service['statistics'], 'restricted': service['restricted'], 'research_mode': service['research_mode'], 'created_at': service['created_at'] From 7e269a454a1a80a31cbae481ac0cf0922f580796 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 14:46:26 +0000 Subject: [PATCH 2/7] Show more granular email & sms counts on platform admin page --- app/assets/stylesheets/components/table.scss | 10 ++ app/templates/components/table.html | 18 +++- app/templates/views/platform-admin.html | 107 +++++++++++++------ 3 files changed, 97 insertions(+), 38 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index c748bdbcc..f5cb285c0 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -118,6 +118,10 @@ } + &-noborder { + border: 0px; + } + &-index { width: 15px; } @@ -159,6 +163,12 @@ text-align: right; } + +.table-row-group { + border-top: 1px solid #BFC1C3; + border-bottom: 1px solid #BFC1C3; +} + .table-empty-message { @include core-16; color: $secondary-text-colour; diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 97783ff93..02dcfadd0 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -49,10 +49,20 @@ {%- endmacro %} -{% macro field(align='left', status='') -%} - - {{ caller() }} - +{% macro row_group(id=None) %} + + {{ caller() }} + +{%- endmacro %} + +{% macro field(align='left', status='', border=True) -%} + + {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-center-aligned' %} + {% set border = '' if border else 'table-field-noborder' %} + + + {{ caller() }} + {%- endmacro %} {% macro row_heading() -%} diff --git a/app/templates/views/platform-admin.html b/app/templates/views/platform-admin.html index 38902bc7d..a79f35886 100644 --- a/app/templates/views/platform-admin.html +++ b/app/templates/views/platform-admin.html @@ -2,49 +2,88 @@ {% from "components/big-number.html" import big_number, big_number_with_status %} {% from "components/message-count-label.html" import message_count_label %} {% from "components/browse-list.html" import browse_list %} -{% from "components/table.html" import list_table, field, right_aligned_field_heading, hidden_field_heading, text_field %} +{% from "components/table.html" import mapping_table, field, row_group, row, right_aligned_field_heading, hidden_field_heading, text_field %} {% macro services_table(services, caption) %} - {% call(item, row_number) list_table( - services, + {% call(item, row_number) mapping_table( caption=caption, caption_visible=True, field_headings=[ - 'Service', - hidden_field_heading('Status'), - right_aligned_field_heading('Sending'), - right_aligned_field_heading('Delivered'), - right_aligned_field_heading('Failed') + 'Service', + hidden_field_heading('Type'), + right_aligned_field_heading('Sending'), + right_aligned_field_heading('Delivered'), + right_aligned_field_heading('Failed') ], field_headings_visible=True ) %} - {% call field() %} -
- {{ item['name'] }} -
- {% endcall %} - {% if item['research_mode'] %} - {% call field() %} - research mode + + {% for service in services %} + + {% call row_group() %} + + {% call row() %} + + {% call field(border=False) %} + {{ service['name'] }} + {% endcall %} + + {% call field(border=False) %} + Email + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['email']['requested'], smaller=True) }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['email']['delivered'], smaller=True) }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['email']['failed'], smaller=True) }} + {% endcall %} + + {% endcall %} + + {% call row() %} + + {% if service['research_mode'] %} + {% call field(border=False) %} + research mode + {% endcall %} + {% elif not service['restricted'] %} + {% call field(status='error') %} + + Live + + {% endcall %} + {% else %} + {{ text_field('') }} + {% endif %} + + {% call field(border=False) %} + SMS + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['sms']['requested'], smaller=True) }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['sms']['delivered'], smaller=True) }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(service['stats']['sms']['failed'], smaller=True) }} + {% endcall %} + + {% endcall %} + {% endcall %} - {% elif not item['restricted'] %} - {% call field(status='error') %} - - Live - - {% endcall %} - {% else %} - {{ text_field('') }} - {% endif %} - {% call field(align='right') %} - {{ big_number(item['sending'], smaller=True) }} - {% endcall %} - {% call field(align='right') %} - {{ big_number(item['delivered'], smaller=True) }} - {% endcall %} - {% call field(align='right', status='error' if 0 else '') %} - {{ big_number(item['failed'], smaller=True) }} - {% endcall %} + + {% endfor %} + {% endcall %} {% endmacro %} From c1649e85a61187178b98428eed4e2ac6047a7c35 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 14:46:52 +0000 Subject: [PATCH 3/7] Ensure stats shown on platform admin are correct and refactor original test for new layout --- tests/app/main/views/test_platform_admin.py | 68 +++++++++++++++++++-- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index ca95d704a..e5d374394 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -61,7 +61,9 @@ def test_should_show_research_and_restricted_mode( mock_get_detailed_services.assert_called_once_with({'detailed': True}) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') # get second column, which contains flags as text. - assert page.find_all('tbody')[table_index].find_all('td')[1].text.strip() == displayed + table_body = page.find_all('table')[table_index].find_all('tbody')[0] + service_mode = table_body.find_all('tbody')[0].find_all('tr')[1].find_all('td')[0].text.strip() + assert service_mode == displayed def test_should_render_platform_admin_page( @@ -141,7 +143,7 @@ def create_stats( } -def test_format_stats_by_service_sums_values_for_sending(fake_uuid): +def test_format_stats_by_service_returns_correct_values(fake_uuid): services = [service_json(fake_uuid, 'a', [])] services[0]['statistics'] = create_stats( emails_requested=10, @@ -153,8 +155,62 @@ def test_format_stats_by_service_sums_values_for_sending(fake_uuid): ) ret = list(format_stats_by_service(services)) - assert len(ret) == 1 - assert ret[0]['sending'] == 34 - assert ret[0]['delivered'] == 10 - assert ret[0]['failed'] == 16 + assert ret[0]['stats']['email']['requested'] == 10 + assert ret[0]['stats']['email']['delivered'] == 3 + assert ret[0]['stats']['email']['failed'] == 5 + + assert ret[0]['stats']['sms']['requested'] == 50 + assert ret[0]['stats']['sms']['delivered'] == 7 + assert ret[0]['stats']['sms']['failed'] == 11 + + +@pytest.mark.parametrize('restricted, table_index, research_mode', [ + (True, 1, False), + (False, 0, False) +]) +def test_should_show_email_and_sms_stats_for_all_service_types( + restricted, + table_index, + research_mode, + app_, + platform_admin_user, + mocker, + mock_get_detailed_services, + fake_uuid +): + services = [service_json(fake_uuid, 'My Service', [], restricted=restricted, research_mode=research_mode)] + services[0]['statistics'] = create_stats( + emails_requested=10, + emails_delivered=3, + emails_failed=5, + sms_requested=50, + sms_delivered=7, + sms_failed=11 + ) + + mock_get_detailed_services.return_value = {'data': services} + with app_.test_request_context(): + with app_.test_client() as client: + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + response = client.get(url_for('main.platform_admin')) + + assert response.status_code == 200 + mock_get_detailed_services.assert_called_once_with({'detailed': True}) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + table_body = page.find_all('table')[table_index].find_all('tbody')[0] + service_row_group = table_body.find_all('tbody')[0].find_all('tr') + email_stats = service_row_group[0].find_all('td')[2:] + sms_stats = service_row_group[1].find_all('td')[2:] + + email_sending, email_delivered, email_failed = [int(stat.text.split()[0]) for stat in email_stats] + sms_sending, sms_delivered, sms_failed = [int(stat.text.split()[0]) for stat in sms_stats] + + assert email_sending == 10 + assert email_delivered == 3 + assert email_failed == 5 + assert sms_sending == 50 + assert sms_delivered == 7 + assert sms_failed == 11 From dbbb2f7848083f5d9147ac12217f6ccf50e3b786 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 14:50:36 +0000 Subject: [PATCH 4/7] Fix comment --- tests/app/main/views/test_platform_admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index e5d374394..74ba43a35 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -60,7 +60,7 @@ def test_should_show_research_and_restricted_mode( assert response.status_code == 200 mock_get_detailed_services.assert_called_once_with({'detailed': True}) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - # get second column, which contains flags as text. + # get first column in second row, which contains flags as text. table_body = page.find_all('table')[table_index].find_all('tbody')[0] service_mode = table_body.find_all('tbody')[0].find_all('tr')[1].find_all('td')[0].text.strip() assert service_mode == displayed From eaa246bd26cd3942595bd13f3e89541a1ca0f4fb Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 16:00:45 +0000 Subject: [PATCH 5/7] Use SASS var for border colour and correct closing tbody tag --- app/assets/stylesheets/components/table.scss | 4 ++-- app/templates/components/table.html | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index f5cb285c0..8f908ddde 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -165,8 +165,8 @@ .table-row-group { - border-top: 1px solid #BFC1C3; - border-bottom: 1px solid #BFC1C3; + border-top: 1px solid $border-colour; + border-bottom: 1px solid $border-colour; } .table-empty-message { diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 02dcfadd0..9b0eaf8bc 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -52,7 +52,7 @@ {% macro row_group(id=None) %} {{ caller() }} - + {%- endmacro %} {% macro field(align='left', status='', border=True) -%} From 5ce0cbe1dbda7f762d21074f6b65bf19948e601f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 14 Nov 2016 16:42:08 +0000 Subject: [PATCH 6/7] Refactor stats into a macro and add format number red on failure --- app/main/views/user_profile.py | 2 +- app/templates/views/platform-admin.html | 58 ++++++++++--------------- 2 files changed, 24 insertions(+), 36 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 4107e53cd..b74dcf43e 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -202,7 +202,7 @@ def user_profile_password(): if form.validate_on_submit(): current_user.set_password(form.new_password.data) - user_api_client.update_user(current_user) + user_api_client.update_user_password(current_user.id, form.new_password.data) return redirect(url_for('.user_profile')) return render_template( diff --git a/app/templates/views/platform-admin.html b/app/templates/views/platform-admin.html index a79f35886..d8b8e6569 100644 --- a/app/templates/views/platform-admin.html +++ b/app/templates/views/platform-admin.html @@ -2,7 +2,27 @@ {% from "components/big-number.html" import big_number, big_number_with_status %} {% from "components/message-count-label.html" import message_count_label %} {% from "components/browse-list.html" import browse_list %} -{% from "components/table.html" import mapping_table, field, row_group, row, right_aligned_field_heading, hidden_field_heading, text_field %} +{% from "components/table.html" import mapping_table, field, stats_fields, row_group, row, right_aligned_field_heading, hidden_field_heading, text_field %} + +{% macro stats_fields(channel, data) -%} + + {% call field(border=False) %} + {{ channel.title() }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(data[channel]['requested'], smaller=True) }} + {% endcall %} + + {% call field(align='right', border=False) %} + {{ big_number(data[channel]['delivered'], smaller=True) }} + {% endcall %} + + {% call field(align='right', status='error' if data[channel]['failed'], border=False) %} + {{ big_number(data[channel]['failed'], smaller=True) }} + {% endcall %} + +{%- endmacro %} {% macro services_table(services, caption) %} {% call(item, row_number) mapping_table( @@ -23,31 +43,14 @@ {% call row_group() %} {% call row() %} - {% call field(border=False) %} {{ service['name'] }} {% endcall %} - {% call field(border=False) %} - Email - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['email']['requested'], smaller=True) }} - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['email']['delivered'], smaller=True) }} - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['email']['failed'], smaller=True) }} - {% endcall %} - + {{ stats_fields('email', service['stats']) }} {% endcall %} {% call row() %} - {% if service['research_mode'] %} {% call field(border=False) %} research mode @@ -62,22 +65,7 @@ {{ text_field('') }} {% endif %} - {% call field(border=False) %} - SMS - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['sms']['requested'], smaller=True) }} - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['sms']['delivered'], smaller=True) }} - {% endcall %} - - {% call field(align='right', border=False) %} - {{ big_number(service['stats']['sms']['failed'], smaller=True) }} - {% endcall %} - + {{ stats_fields('sms', service['stats']) }} {% endcall %} {% endcall %} From 49082ab646a0c74221544c98cc4c4e30901bf698 Mon Sep 17 00:00:00 2001 From: imdadahad Date: Mon, 14 Nov 2016 16:49:25 +0000 Subject: [PATCH 7/7] Revert user_profile back original This change shouldn't be in this PR and got in somehow :( --- app/main/views/user_profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index b74dcf43e..4107e53cd 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -202,7 +202,7 @@ def user_profile_password(): if form.validate_on_submit(): current_user.set_password(form.new_password.data) - user_api_client.update_user_password(current_user.id, form.new_password.data) + user_api_client.update_user(current_user) return redirect(url_for('.user_profile')) return render_template(