diff --git a/app/__init__.py b/app/__init__.py index e9f1bc698..e040868a2 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -3,7 +3,6 @@ import os import urllib from datetime import datetime, timedelta, timezone from functools import partial -from numbers import Number from time import monotonic import ago @@ -84,7 +83,7 @@ from app.notify_client.template_statistics_api_client import ( template_statistics_client, ) from app.notify_client.user_api_client import user_api_client -from app.utils import get_logo_cdn_domain, id_safe +from app.utils import format_thousands, get_logo_cdn_domain, id_safe login_manager = LoginManager() csrf = CSRFProtect() @@ -298,12 +297,16 @@ def get_human_day(time): # Add 1 minute to transform 00:00 into ‘midnight today’ instead of ‘midnight tomorrow’ date = (utc_string_to_aware_gmt_datetime(time) - timedelta(minutes=1)).date() - if date == (datetime.utcnow() + timedelta(days=1)).date(): + now = datetime.utcnow() + + if date == (now + timedelta(days=1)).date(): return 'tomorrow' - if date == datetime.utcnow().date(): + if date == now.date(): return 'today' - if date == (datetime.utcnow() - timedelta(days=1)).date(): + if date == (now - timedelta(days=1)).date(): return 'yesterday' + if date.strftime('%Y') != now.strftime('%Y'): + return '{} {}'.format(_format_datetime_short(date), date.strftime('%Y')) return _format_datetime_short(date) @@ -329,6 +332,10 @@ def format_date_short(date): return _format_datetime_short(utc_string_to_aware_gmt_datetime(date)) +def format_date_human(date): + return get_human_day(date) + + def _format_datetime_short(datetime): return datetime.strftime('%d %B').lstrip('0') @@ -351,14 +358,6 @@ def format_delta(date): ) -def format_thousands(value): - if isinstance(value, Number): - return '{:,.0f}'.format(value) - if value is None: - return '' - return value - - def valid_phone_number(phone_number): try: validate_phone_number(phone_number) @@ -690,6 +689,7 @@ def add_template_filters(application): valid_phone_number, linkable_name, format_date, + format_date_human, format_date_normal, format_date_short, format_datetime_relative, diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index ceb233650..c58b6b607 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -75,6 +75,7 @@ $path: '/static/images/'; @import 'views/template'; @import 'views/notification'; @import 'views/send'; +@import 'views/history'; // TODO: break this up @import 'app'; diff --git a/app/assets/stylesheets/views/history.scss b/app/assets/stylesheets/views/history.scss new file mode 100644 index 000000000..b971a1726 --- /dev/null +++ b/app/assets/stylesheets/views/history.scss @@ -0,0 +1,29 @@ +$item-top-padding: $gutter-half; + +.history-list { + + @include core-19; + margin-bottom: $gutter; + + &-item { + + padding: $item-top-padding 0 $gutter-half 0; + border-top: 1px solid $border-colour; + position: relative; + + &:last-child { + border-bottom: 1px solid $border-colour; + } + + } + + &-user { + display: block; + } + + &-time { + display: block; + color: $secondary-text-colour; + } + +} diff --git a/app/main/views/history.py b/app/main/views/history.py index 428d3b966..07614db3a 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -1,7 +1,10 @@ -from flask import render_template +from collections import defaultdict -from app import current_service +from flask import render_template, request + +from app import current_service, format_date_numeric from app.main import main +from app.models.event import APIKeyEvent, APIKeyEvents, ServiceEvents from app.utils import user_has_permissions @@ -9,9 +12,31 @@ from app.utils import user_has_permissions @user_has_permissions('manage_service') def history(service_id): + events = _get_events(current_service.id, request.args.get('selected')) + return render_template( 'views/temp-history.html', - services=current_service.history['service_history'], - api_keys=current_service.history['api_key_history'], - events=current_service.history['events'] + days=_chunk_events_by_day(events), + show_navigation=request.args.get('selected') or any( + isinstance(event, APIKeyEvent) for event in events + ), + user_getter=current_service.active_users.get_name_from_id, ) + + +def _get_events(service_id, selected): + if selected == 'api': + return APIKeyEvents(service_id) + if selected == 'service': + return ServiceEvents(service_id) + return APIKeyEvents(service_id) + ServiceEvents(service_id) + + +def _chunk_events_by_day(events): + + days = defaultdict(list) + + for event in events: + days[format_date_numeric(event.time)].append(event) + + return sorted(days.items(), reverse=True) diff --git a/app/models/__init__.py b/app/models/__init__.py index 2574495e2..def264a98 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -63,8 +63,8 @@ class ModelList(ABC, Sequence): def model(): pass - def __init__(self): - self.items = self.client() + def __init__(self, *args): + self.items = self.client(*args) def __getitem__(self, index): return self.model(self.items[index]) diff --git a/app/models/event.py b/app/models/event.py new file mode 100644 index 000000000..43dfba627 --- /dev/null +++ b/app/models/event.py @@ -0,0 +1,191 @@ +from abc import ABC, abstractmethod + +from notifications_utils.formatters import formatted_list + +from app.models import ModelList +from app.notify_client.service_api_client import service_api_client +from app.utils import format_thousands + + +class Event(ABC): + + def __init__( + self, + item, + key=None, + value_from=None, + value_to=None, + ): + self.item = item + self.time = item['updated_at'] or item['created_at'] + self.user_id = item['created_by_id'] + self.key = key + self.value_from = value_from + self.value_to = value_to + + @abstractmethod + def __str__(self): + pass + + @property + @abstractmethod + def relevant(self): + pass + + +class ServiceCreationEvent(Event): + + relevant = True + + def __str__(self): + return 'Created this service and called it ‘{}’'.format( + self.item['name'] + ) + + +class ServiceEvent(Event): + + @property + def relevant(self): + return self.value_from != self.value_to and bool(self._formatter) + + def __str__(self): + return self._formatter() + + @property + def _formatter(self): + return getattr(self, 'format_{}'.format(self.key), None) + + def format_restricted(self): + if self.value_to is False: + return 'Made this service live' + if self.value_to is True: + return 'Put this service back into trial mode' + + def format_active(self): + if self.value_to is False: + return 'Deleted this service' + if self.value_to is True: + return 'Unsuspended this service' + + def format_contact_link(self): + return 'Set the contact details for this service to ‘{}’'.format( + self.value_to + ) + + def format_email_branding(self): + return 'Updated this service’s email branding' + + def format_inbound_api(self): + return 'Updated the callback for received text messages' + + def format_letter_branding(self): + if self.value_to is None: + return 'Removed the logo from this service’s letters' + return 'Updated the logo on this service’s letters' + + def format_letter_contact_block(self): + return 'Updated the default letter contact block for this service' + + def format_message_limit(self): + return ( + '{} this service’s daily message limit from {} to {}' + ).format( + 'Reduced' if self.value_from > self.value_to else 'Increased', + format_thousands(self.value_from), + format_thousands(self.value_to), + ) + + def format_name(self): + return ( + 'Renamed this service from ‘{}’ to ‘{}’' + ).format( + self.value_from, self.value_to + ) + + def format_permissions(self): + added = list(sorted(set(self.value_to) - set(self.value_from))) + removed = list(sorted(set(self.value_from) - set(self.value_to))) + if removed and added: + return 'Removed {} from this service’s permissions, added {}'.format( + formatted_list(removed), + formatted_list(added), + ) + if added: + return 'Added {} to this service’s permissions'.format( + formatted_list(added) + ) + if removed: + return 'Removed {} from this service’s permissions'.format( + formatted_list(removed) + ) + + def format_prefix_sms(self): + if self.value_to is True: + return 'Set text messages to start with the name of this service' + else: + return 'Set text messages to not start with the name of this service' + + def format_research_mode(self): + if self.value_to is True: + return 'Put this service into research mode' + else: + return 'Took this service out of research mode' + + def format_service_callback_api(self): + return 'Updated the callback for delivery receipts' + + def format_go_live_user(self): + return 'Requested for this service to go live' + + +class APIKeyEvent(Event): + + relevant = True + + def __str__(self): + if self.item['updated_at']: + return ( + 'Revoked the ‘{}’ API key' + ).format(self.item['name']) + else: + return ( + 'Created an API key called ‘{}’' + ).format(self.item['name']) + + +class APIKeyEvents(ModelList): + + model = APIKeyEvent + client = service_api_client.get_service_api_key_history + + +class ServiceEvents(ModelList): + + client = service_api_client.get_service_service_history + + @property + def model(self): + return lambda x: x + + @staticmethod + def splat(events): + for index, item in enumerate(sorted( + events, + key=lambda event: event['updated_at'] or event['created_at'] + )): + if index == 0: + yield ServiceCreationEvent(item) + else: + for key in sorted(item.keys()): + yield ServiceEvent( + item, + key, + events[index - 1][key], + events[index][key], + ) + + def __init__(self, service_id): + self.items = [ + event for event in self.splat(self.client(service_id)) if event.relevant + ] diff --git a/app/models/service.py b/app/models/service.py index 7b43c53fa..3e3e93fd6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -631,7 +631,3 @@ class Service(JSONModel): ): if test: yield BASE + '_incomplete' + tag - - @cached_property - def history(self): - return service_api_client.get_service_history(self.id)['data'] diff --git a/app/models/user.py b/app/models/user.py index 8810719bc..2c9cf5fce 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -610,6 +610,12 @@ class Users(ModelList): def __init__(self, service_id): self.items = self.client(service_id) + def get_name_from_id(self, id): + for user in self: + if user.id == id: + return user.name + return 'Unknown' + class OrganisationUsers(Users): client = user_api_client.get_users_for_organisation diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index f247597be..8c45fb1b3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -311,7 +311,13 @@ class ServiceAPIClient(NotifyAdminAPIClient): # Temp access of service history data. Includes service and api key history def get_service_history(self, service_id): - return self.get('/service/{0}/history'.format(service_id)) + return self.get('/service/{0}/history'.format(service_id))['data'] + + def get_service_service_history(self, service_id): + return self.get_service_history(service_id)['service_history'] + + def get_service_api_key_history(self, service_id): + return self.get_service_history(service_id)['api_key_history'] def get_monthly_notification_stats(self, service_id, year): return self.get(url='/service/{}/notifications/monthly?year={}'.format(service_id, year)) diff --git a/app/templates/views/temp-history.html b/app/templates/views/temp-history.html index d817e9cea..0c1d79f76 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -1,105 +1,52 @@ {% extends "withnav_template.html" %} {% from "components/page-header.html" import page_header %} {% from "components/table.html" import list_table, field %} +{% from "components/pill.html" import pill %} {% block service_page_title %} -Service and API key history + Audit events {% endblock %} {% block maincolumn_content %} - {{ page_header("Service and API key history") }} + {{ page_header("Audit events") }} -
- {% call(item, row_number) list_table( - services, - caption="Service history", - field_headings=['ID','Name','Created at','Updated at','Active','Message limit','Restricted','Created by id'] - )%} - {% call field() %} - {{item.id}} - {% endcall %} - {% call field() %} - {{item.name}} - {% endcall %} - {% call field() %} - {{item.created_at}} - {% endcall %} - {% call field() %} - {{item.updated_at}} - {% endcall %} - {% call field() %} - {{item.active}} - {% endcall %} - {% call field() %} - {{item.message_limit}} - {% endcall %} - {% call field() %} - {{item.restricted}} - {% endcall %} - {% call field() %} - {{item.created_by_id}} - {% endcall %} + {% if show_navigation %} +
+ {{ pill( + [ + ('All', None, url_for('main.history', service_id=current_service.id), None), + ('Service settings', 'service', url_for('main.history', service_id=current_service.id, selected='service'), None), + ('API keys', 'api', url_for('main.history', service_id=current_service.id, selected='api'), None), + ], + request.args.get('selected'), + show_count=False + ) }} +
+ {% endif %} - {% endcall %} + {% for day, events in days %} +

+ {{ events[0].time|format_date_human|title }} +

+ + {% endfor %} -
- - -
- {% call(item, row_number) list_table( - api_keys, - caption="API key history", - field_headings=['ID','Name','Service ID','Exiry date','Created at','Updated at','Created by id'] - )%} - {% call field() %} - {{item.id}} - {% endcall %} - {% call field() %} - {{item.name}} - {% endcall %} - {% call field() %} - {{item.service_id}} - {% endcall %} - {% call field() %} - {{item.expiry_date}} - {% endcall %} - {% call field() %} - {{item.created_at}} - {% endcall %} - {% call field() %} - {{item.updated_at}} - {% endcall %} - {% call field() %} - {{item.created_by_id}} - {% endcall %} - - {% endcall %} - -
- - {% call(item, row_number) list_table( - events, - caption="Events", - field_headings=['ID','Event type','User ID','IP Address','Event data'] - )%} - {% call field() %} - {{item.id}} - {% endcall %} - {% call field() %} - {{item.event_type}} - {% endcall %} - {% call field() %} - {{item.data.user_id}} - {% endcall %} - {% call field() %} - {{item.data.ip_address}} - {% endcall %} - {% call field() %} - {{item.data}} - {% endcall %} - {% endcall %} - {% endblock %} diff --git a/app/utils.py b/app/utils.py index 9f3d9483e..dffa23ebe 100644 --- a/app/utils.py +++ b/app/utils.py @@ -6,6 +6,7 @@ from datetime import datetime, time, timedelta, timezone from functools import wraps from io import BytesIO, StringIO from itertools import chain +from numbers import Number from os import path from urllib.parse import urlparse @@ -602,3 +603,11 @@ class PermanentRedirect(RequestRedirect): and Windows 8.1, so this class keeps the original status code of 301. """ code = 301 + + +def format_thousands(value): + if isinstance(value, Number): + return '{:,.0f}'.format(value) + if value is None: + return '' + return value diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 21461d960..8b0e64d13 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -1,8 +1,101 @@ -from tests.conftest import SERVICE_ONE_ID +import pytest +from freezegun import freeze_time + +from tests.conftest import SERVICE_ONE_ID, normalize_spaces +@pytest.mark.parametrize('extra_args, expected_headings_and_events', ( + ({}, [ + ( + '12 December', + ( + 'Test User 12:12pm ' + 'Renamed this service from ‘Example service’ to ‘Real service’' + ), + ), + ( + '11 November', + ( + 'Test User 12:12pm ' + 'Revoked the ‘Bad key’ API key' + ), + ), + ( + '11 November 2011', + ( + 'Test User 11:11am ' + 'Created an API key called ‘Bad key’' + ), + ), + ( + '10 October 2010', + ( + 'Test User 11:10am ' + 'Created an API key called ‘Good key’ ' + 'Unknown 2:01am ' + 'Created this service and called it ‘Example service’' + ), + ), + ]), + ({'selected': 'api'}, [ + ( + '11 November', + ( + 'Test User 12:12pm ' + 'Revoked the ‘Bad key’ API key' + ), + ), + ( + '11 November 2011', + ( + 'Test User 11:11am ' + 'Created an API key called ‘Bad key’' + ), + ), + ( + '10 October 2010', + ( + 'Test User 11:10am ' + 'Created an API key called ‘Good key’' + ), + ), + ]), + ({'selected': 'service'}, [ + ( + '12 December', + ( + 'Test User 12:12pm ' + 'Renamed this service from ‘Example service’ to ‘Real service’' + ), + ), + ( + '10 October 2010', + ( + 'Unknown 2:01am ' + 'Created this service and called it ‘Example service’' + ), + ), + ]), +)) +@freeze_time("2012-01-01 01:01:01") def test_history( client_request, mock_get_service_history, + mock_get_users_by_service, + extra_args, + expected_headings_and_events, ): - client_request.get('main.history', service_id=SERVICE_ONE_ID) + page = client_request.get('main.history', service_id=SERVICE_ONE_ID, **extra_args) + + assert page.select_one('h1').text == 'Audit events' + + headings = page.select('main h2.heading-small') + events = page.select('main ul.bottom-gutter') + + assert len(headings) == len(events) == len(expected_headings_and_events) + + for index, expected in enumerate(expected_headings_and_events): + assert ( + normalize_spaces(headings[index].text), + normalize_spaces(events[index].text), + ) == expected diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 745dccd2f..695e27ae2 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -45,11 +45,11 @@ from tests.conftest import ( ), ( 'send_me_later.csv ' - 'Sending 1 January at 11:09am 1' + 'Sending 1 January 2016 at 11:09am 1' ), ( 'even_later.csv ' - 'Sending 1 January at 11:09pm 1' + 'Sending 1 January 2016 at 11:09pm 1' ), ( 'File Sending Delivered Failed' diff --git a/tests/app/models/test_event.py b/tests/app/models/test_event.py new file mode 100644 index 000000000..3b48d88f1 --- /dev/null +++ b/tests/app/models/test_event.py @@ -0,0 +1,89 @@ +import pytest + +from app.models.event import ServiceEvent +from tests.conftest import sample_uuid + + +@pytest.mark.parametrize('key, value_from, value_to, expected', ( + ('restricted', True, False, ( + 'Made this service live' + )), + ('restricted', False, True, ( + 'Put this service back into trial mode' + )), + ('active', False, True, ( + 'Unsuspended this service' + )), + ('active', True, False, ( + 'Deleted this service' + )), + ('contact_link', 'x', 'y', ( + 'Set the contact details for this service to ‘y’' + )), + ('email_branding', 'foo', 'bar', ( + 'Updated this service’s email branding' + )), + ('inbound_api', 'foo', 'bar', ( + 'Updated the callback for received text messages' + )), + ('letter_branding', None, sample_uuid(), ( + 'Updated the logo on this service’s letters' + )), + ('letter_branding', sample_uuid(), None, ( + 'Removed the logo from this service’s letters' + )), + ('letter_contact_block', None, sample_uuid(), ( + 'Updated the default letter contact block for this service' + )), + ('message_limit', 1, 2, ( + 'Increased this service’s daily message limit from 1 to 2' + )), + ('message_limit', 2, 1, ( + 'Reduced this service’s daily message limit from 2 to 1' + )), + ('name', 'Old', 'New', ( + 'Renamed this service from ‘Old’ to ‘New’' + )), + ('permissions', ['a', 'b', 'c'], ['a', 'b', 'c', 'd'], ( + 'Added ‘d’ to this service’s permissions' + )), + ('permissions', ['a', 'b', 'c'], ['a', 'b'], ( + 'Removed ‘c’ from this service’s permissions' + )), + ('permissions', ['a', 'b', 'c'], ['c', 'd', 'e'], ( + 'Removed ‘a’ and ‘b’ from this service’s permissions, added ‘d’ and ‘e’' + )), + ('prefix_sms', True, False, ( + 'Set text messages to not start with the name of this service' + )), + ('prefix_sms', False, True, ( + 'Set text messages to start with the name of this service' + )), + ('research_mode', True, False, ( + 'Took this service out of research mode' + )), + ('research_mode', False, True, ( + 'Put this service into research mode' + )), + ('service_callback_api', 'foo', 'bar', ( + 'Updated the callback for delivery receipts' + )), +)) +def test_service_event( + key, + value_from, + value_to, + expected, +): + event = ServiceEvent( + { + 'created_at': 'foo', + 'updated_at': 'bar', + 'created_by_id': sample_uuid(), + }, + key, + value_from, + value_to, + ) + assert event.relevant is True + assert str(event) == expected diff --git a/tests/conftest.py b/tests/conftest.py index c37a4b8fb..271c6001f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3505,8 +3505,40 @@ def mock_get_service_and_organisation_counts(mocker): @pytest.fixture(scope='function') def mock_get_service_history(mocker): - return mocker.patch('app.service_api_client.get_service_history', return_value={'data': { - 'service_history': [], - 'api_key_history': [], + return mocker.patch('app.service_api_client.get_service_history', return_value={ + 'service_history': [ + { + 'name': 'Example service', + 'created_at': '2010-10-10T01:01:01.000000Z', + 'updated_at': None, + 'created_by_id': uuid4(), + }, + { + 'name': 'Real service', + 'created_at': '2010-10-10T01:01:01.000000Z', + 'updated_at': '2012-12-12T12:12:12.000000Z', + 'created_by_id': sample_uuid(), + }, + ], + 'api_key_history': [ + { + 'name': 'Good key', + 'updated_at': None, + 'created_at': '2010-10-10T10:10:10.000000Z', + 'created_by_id': sample_uuid(), + }, + { + 'name': 'Bad key', + 'updated_at': '2012-11-11T12:12:12.000000Z', + 'created_at': '2011-11-11T11:11:11.000000Z', + 'created_by_id': sample_uuid(), + }, + { + 'name': 'Bad key', + 'updated_at': None, + 'created_at': '2011-11-11T11:11:11.000000Z', + 'created_by_id': sample_uuid(), + }, + ], 'events': [], - }}) + })