From 259db9c271577e95e5df11ea481c6dacc2280374 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Sat, 30 Dec 2017 16:54:39 +0000 Subject: [PATCH 1/3] Put the download csv link on the activity page. Still needs some tests, probably some refactoring as well. --- app/main/views/jobs.py | 56 ++++++--------------- app/main/views/notifications.py | 38 ++++++++++++-- app/templates/views/notifications.html | 6 ++- app/utils.py | 70 ++++++++++++++++++++------ tests/app/test_utils.py | 10 ++-- 5 files changed, 116 insertions(+), 64 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 511716530..7d3be9284 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -1,6 +1,4 @@ # -*- coding: utf-8 -*- -from orderedset import OrderedSet -from itertools import chain from flask import ( render_template, @@ -18,7 +16,6 @@ from notifications_utils.template import ( Template, WithSubjectTemplate, ) -from werkzeug.datastructures import MultiDict from app import ( job_api_client, @@ -35,39 +32,11 @@ from app.utils import ( user_has_permissions, generate_notifications_csv, get_time_left, - REQUESTED_STATUSES, - FAILURE_STATUSES, - SENDING_STATUSES, - DELIVERED_STATUSES, get_letter_timings, -) + parse_filter_args, set_status_filters) from app.statistics_utils import add_rate_to_job -def _parse_filter_args(filter_dict): - if not isinstance(filter_dict, MultiDict): - filter_dict = MultiDict(filter_dict) - - return MultiDict( - ( - key, - (','.join(filter_dict.getlist(key))).split(',') - ) - for key in filter_dict.keys() - if ''.join(filter_dict.getlist(key)) - ) - - -def _set_status_filters(filter_args): - status_filters = filter_args.get('status', []) - return list(OrderedSet(chain( - (status_filters or REQUESTED_STATUSES), - DELIVERED_STATUSES if 'delivered' in status_filters else [], - SENDING_STATUSES if 'sending' in status_filters else [], - FAILURE_STATUSES if 'failed' in status_filters else [] - ))) - - @main.route("/services//jobs") @login_required @user_has_permissions('view_activity', admin_override=True) @@ -104,8 +73,8 @@ def view_job(service_id, job_id): if job['job_status'] == 'cancelled': abort(404) - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) total_notifications = job.get('notification_count', 0) processed_notifications = job.get('notifications_delivered', 0) + job.get('notifications_failed', 0) @@ -146,8 +115,8 @@ def view_job_csv(service_id, job_id): template_id=job['template'], version=job['template_version'] )['data'] - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) return Response( stream_with_context( @@ -206,6 +175,12 @@ def view_notifications(service_id, message_type): page=request.args.get('page', 1), to=request.form.get('to', ''), search_form=SearchNotificationsForm(to=request.form.get('to', '')), + download_link=url_for( + '.download_notifications_csv', + service_id=current_service['id'], + message_type=message_type, + status=request.args.get('status') + ) ) @@ -226,8 +201,8 @@ def get_notifications(service_id, message_type, status_override=None): abort(404, "Invalid page argument ({}) reverting to page 1.".format(request.args['page'], None)) if message_type not in ['email', 'sms', 'letter']: abort(404) - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) if request.path.endswith('csv'): return Response( generate_notifications_csv( @@ -250,7 +225,6 @@ def get_notifications(service_id, message_type, status_override=None): limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'], to=request.form.get('to', ''), ) - url_args = { 'message_type': message_type, 'status': request.args.get('status') @@ -361,8 +335,8 @@ def _get_job_counts(job): def get_job_partials(job, template): - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.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'] ) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index ed6fe756b..1482c68e4 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -1,18 +1,20 @@ # -*- coding: utf-8 -*- +from datetime import datetime + from flask import ( abort, render_template, jsonify, request, url_for, -) + Response, stream_with_context) from flask_login import login_required from app import ( notification_api_client, job_api_client, - current_service -) + current_service, + format_date_numeric) from app.main import main from app.template_previews import TemplatePreview from app.utils import ( @@ -23,7 +25,7 @@ from app.utils import ( get_letter_timings, FAILURE_STATUSES, DELIVERED_STATUSES, -) + generate_notifications_csv, parse_filter_args, set_status_filters) @main.route("/services//notification/") @@ -134,3 +136,31 @@ def get_all_personalisation_from_notification(notification): notification['personalisation']['phone_number'] = notification['to'] return notification['personalisation'] + + +@main.route("/services//download-notifications.csv") +@login_required +@user_has_permissions('view_activity', admin_override=True) +def download_notifications_csv(service_id): + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) + + return Response( + stream_with_context( + generate_notifications_csv( + service_id=service_id, + job_id=None, + status=filter_args.get('status'), + page=request.args.get('page', 1), + page_size=5000, + format_for_csv=True + ) + ), + mimetype='text/csv', + headers={ + 'Content-Disposition': 'inline; filename="{} - {} - {} report.csv"'.format( + format_date_numeric(datetime.now().strftime("%Y-%m-%dT%H:%M:%S.%fZ")), + filter_args['message_type'][0], + current_service['name']) + } + ) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index b76641b34..a03fb4874 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -43,7 +43,11 @@ {% endif %} - +

+ Download this report +   + Data available for 7 days +

{{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), diff --git a/app/utils.py b/app/utils.py index ba19eddc7..3812a936f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,5 +1,7 @@ import re import csv +from itertools import chain + import pytz from io import StringIO from os import path @@ -28,7 +30,8 @@ from notifications_utils.template import ( LetterImageTemplate, LetterPreviewTemplate, ) - +from orderedset._orderedset import OrderedSet +from werkzeug.datastructures import MultiDict SENDING_STATUSES = ['created', 'pending', 'sending'] DELIVERED_STATUSES = ['delivered', 'sent'] @@ -132,21 +135,36 @@ def generate_notifications_csv(**kwargs): yield ','.join(fieldnames) + '\n' while kwargs['page']: + # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - for notification in notifications: - values = [ - notification['row_number'], - notification['recipient'], - notification['template_name'], - notification['template_type'], - notification['job_name'], - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line - + if kwargs['job_id']: + for notification in notifications: + values = [ + notification['row_number'], + notification['recipient'], + notification['template_name'], + notification['template_type'], + notification['job_name'], + notification['status'], + notification['created_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line + else: + # Change here + for notification in notifications: + values = [ + notification.get('row_number', None), + notification['to'], + notification['template']['name'], + notification['template']['template_type'], + notification.get('job_name', None), + notification['status'], + notification['created_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: @@ -381,3 +399,27 @@ def get_cdn_domain(): domain = parsed_uri.netloc[len(subdomain + '.'):] return "static-logos.{}".format(domain) + + +def parse_filter_args(filter_dict): + if not isinstance(filter_dict, MultiDict): + filter_dict = MultiDict(filter_dict) + + return MultiDict( + ( + key, + (','.join(filter_dict.getlist(key))).split(',') + ) + for key in filter_dict.keys() + if ''.join(filter_dict.getlist(key)) + ) + + +def set_status_filters(filter_args): + status_filters = filter_args.get('status', []) + return list(OrderedSet(chain( + (status_filters or REQUESTED_STATUSES), + DELIVERED_STATUSES if 'delivered' in status_filters else [], + SENDING_STATUSES if 'sending' in status_filters else [], + FAILURE_STATUSES if 'failed' in status_filters else [] + ))) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 980f7a6c7..4dcb2bd15 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,3 +1,4 @@ +import uuid from pathlib import Path from io import StringIO from collections import OrderedDict @@ -58,7 +59,8 @@ def _get_notifications_csv( @pytest.fixture(scope='function') def _get_notifications_csv_mock( mocker, - api_user_active + api_user_active, + job_id=None ): return mocker.patch( 'app.notification_api_client.get_notifications_for_service', @@ -122,13 +124,13 @@ def test_can_create_spreadsheet_from_dict_with_filename(): def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): - csv_content = generate_notifications_csv(service_id='1234') + csv_content = generate_notifications_csv(service_id='1234', job_id=uuid.uuid4()) csv_file = DictReader(StringIO('\n'.join(csv_content))) assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): - list(generate_notifications_csv(service_id='1234')) + list(generate_notifications_csv(service_id='1234', job_id=uuid.uuid4())) assert _get_notifications_csv_mock.call_count == 1 @@ -146,7 +148,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): ] ) - csv_content = generate_notifications_csv(service_id=service_id) + csv_content = generate_notifications_csv(service_id=service_id, job_id=uuid.uuid4()) csv = DictReader(StringIO('\n'.join(csv_content))) assert len(list(csv)) == 10 From e71bbc1bb595e18c2207575b3b2be6c6e93a7e0f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 3 Jan 2018 13:21:00 +0000 Subject: [PATCH 2/3] [WIp] --- tests/app/test_utils.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 4dcb2bd15..bd3fdaf8d 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,4 +1,3 @@ -import uuid from pathlib import Path from io import StringIO from collections import OrderedDict @@ -16,6 +15,7 @@ from app.utils import ( get_letter_timings, get_cdn_domain ) +from tests.conftest import fake_uuid def _get_notifications_csv( @@ -29,7 +29,8 @@ def _get_notifications_csv( status='Delivered', created_at='Thursday 19 April at 12:00', rows=1, - with_links=False + with_links=False, + job_id=fake_uuid ): links = {} if with_links: @@ -60,7 +61,7 @@ def _get_notifications_csv( def _get_notifications_csv_mock( mocker, api_user_active, - job_id=None + job_id=fake_uuid ): return mocker.patch( 'app.notification_api_client.get_notifications_for_service', @@ -124,13 +125,13 @@ def test_can_create_spreadsheet_from_dict_with_filename(): def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): - csv_content = generate_notifications_csv(service_id='1234', job_id=uuid.uuid4()) + csv_content = generate_notifications_csv(service_id='1234', job_id=fake_uuid) csv_file = DictReader(StringIO('\n'.join(csv_content))) assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): - list(generate_notifications_csv(service_id='1234', job_id=uuid.uuid4())) + list(generate_notifications_csv(service_id='1234', job_id=fake_uuid)) assert _get_notifications_csv_mock.call_count == 1 @@ -148,7 +149,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): ] ) - csv_content = generate_notifications_csv(service_id=service_id, job_id=uuid.uuid4()) + csv_content = generate_notifications_csv(service_id=service_id, job_id=fake_uuid) csv = DictReader(StringIO('\n'.join(csv_content))) assert len(list(csv)) == 10 From 08e5f6846153d340e2ea4cc1936e58b5eb41c19d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 12 Jan 2018 12:06:07 +0000 Subject: [PATCH 3/3] A little refactoring --- app/main/views/jobs.py | 3 +- app/utils.py | 62 ++++++++++++++++++++++-------------------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 7d3be9284..8beb59f44 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -33,7 +33,8 @@ from app.utils import ( generate_notifications_csv, get_time_left, get_letter_timings, - parse_filter_args, set_status_filters) + parse_filter_args, set_status_filters +) from app.statistics_utils import add_rate_to_job diff --git a/app/utils.py b/app/utils.py index 95e881249..98f87f5bc 100644 --- a/app/utils.py +++ b/app/utils.py @@ -128,43 +128,20 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(**kwargs): from app import notification_api_client + fieldnames, list_of_keys = _create_key_and_fieldnames_for_csv(kwargs) if 'page' not in kwargs: kwargs['page'] = 1 - fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] yield ','.join(fieldnames) + '\n' while kwargs['page']: - # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - if kwargs['job_id']: - for notification in notifications: - values = [ - notification['row_number'], - notification['recipient'], - notification['template_name'], - notification['template_type'], - notification['job_name'], - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line - else: - # Change here - for notification in notifications: - values = [ - notification.get('row_number', None), - notification['to'], - notification['template']['name'], - notification['template']['template_type'], - notification.get('job_name', None), - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line + + for notification in notifications: + values = [notification[x] for x in list_of_keys] + line = ','.join(str(i) for i in values) + '\n' + yield line if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: @@ -172,6 +149,33 @@ def generate_notifications_csv(**kwargs): raise Exception("Should never reach here") +def _create_key_and_fieldnames_for_csv(kwargs): + # if job_id then response looks different + if kwargs['job_id']: + list_of_keys = [ + 'row_number', + 'recipient', + 'template_name', + 'template_type', + 'job_name', + 'status', + 'created_at' + ] + fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + else: + list_of_keys = [ + 'recipient', + 'template_name', + 'template_type', + 'job_name', + 'status', + 'created_at', + 'updated_at' + ] + fieldnames = ['Recipient', 'Template', 'Type', 'Job', 'Status', 'Time', 'Completed at'] + return fieldnames, list_of_keys + + def get_page_from_request(): if 'page' in request.args: try: