From 59b4d60c916eea61e3677b1e8832e40b134f0bd1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 18 Oct 2019 16:09:39 +0100 Subject: [PATCH 1/7] Munge stuff into a consistent event data type We store our audit history in two ways: 1. A list of versions of a service 2. A list of events to do with API keys In the future there could be auditing data which we want to display that is stored in other formats (for example the event table). This commit adds some objects which wrap around the different types of auditing data, and expose a consistent interface to them. This architecture will let us: - write clean code in the presentation layer to display these events on a page - add more types of events in the future by subclassing the `Event` data type, without having to rewrite anything in the presentation layer --- app/__init__.py | 11 +- app/main/views/history.py | 9 +- app/models/__init__.py | 4 +- app/models/event.py | 191 ++++++++++++++++++++++++ app/models/service.py | 10 +- app/notify_client/service_api_client.py | 8 +- app/templates/views/temp-history.html | 100 +------------ app/utils.py | 9 ++ tests/app/main/views/test_history.py | 18 ++- tests/app/models/test_event.py | 89 +++++++++++ tests/conftest.py | 39 ++++- 11 files changed, 367 insertions(+), 121 deletions(-) create mode 100644 app/models/event.py create mode 100644 tests/app/models/test_event.py diff --git a/app/__init__.py b/app/__init__.py index e9f1bc698..2b98ab712 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() @@ -351,14 +350,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) diff --git a/app/main/views/history.py b/app/main/views/history.py index 428d3b966..217177ff5 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -1,6 +1,5 @@ from flask import render_template -from app import current_service from app.main import main from app.utils import user_has_permissions @@ -8,10 +7,4 @@ from app.utils import user_has_permissions @main.route("/services//history") @user_has_permissions('manage_service') def history(service_id): - - 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'] - ) + return render_template('views/temp-history.html') 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..341ecf6a7 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,3 +1,5 @@ +from operator import attrgetter + from flask import Markup, abort, current_app from notifications_utils.field import Field from notifications_utils.formatters import nl2br @@ -5,6 +7,7 @@ from notifications_utils.take import Take from werkzeug.utils import cached_property from app.models import JSONModel +from app.models.event import APIKeyEvents, ServiceEvents from app.models.organisation import Organisation from app.models.user import InvitedUsers, User, Users from app.notify_client.api_key_api_client import api_key_api_client @@ -632,6 +635,9 @@ class Service(JSONModel): if test: yield BASE + '_incomplete' + tag - @cached_property + @property def history(self): - return service_api_client.get_service_history(self.id)['data'] + return sorted( + ServiceEvents(self.id) + APIKeyEvents(self.id), + key=attrgetter('time'), + ) 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..6ca2b7764 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -10,96 +10,12 @@ Service and API key history {{ page_header("Service and API key history") }} -
- {% 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 %} - - {% endcall %} - -
- - -
- {% 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..17e8ba7a7 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -1,8 +1,22 @@ -from tests.conftest import SERVICE_ONE_ID +from tests.conftest import SERVICE_ONE_ID, normalize_spaces def test_history( client_request, mock_get_service_history, ): - client_request.get('main.history', service_id=SERVICE_ONE_ID) + page = client_request.get('main.history', service_id=SERVICE_ONE_ID) + + assert normalize_spaces( + page.select_one('main').text + ) == ( + 'Service and API key history ' + '11 November at 12:12pm 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' + ' Revoked the ‘Bad key’ API key ' + '11 November at 11:11am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' + ' Created an API key called ‘Bad key’ ' + '10 October at 11:10am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' + ' Created an API key called ‘Good key’ ' + '10 October at 11:10am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' + ' Created this service and called it ‘Example service’' + ) 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..ca097949f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3505,8 +3505,39 @@ 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-10T10:10:10.000000Z', + 'updated_at': None, + 'created_by_id': sample_uuid(), + }, + { + 'created_at': '2010-10-10T10:10:10.000000Z', + 'updated_at': '2012-12-12T12:12:12.000000Z', + 'created_by_id': uuid4(), + }, + ], + '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': [], - }}) + }) From b2ebaf153abccc974268a39af472d610c11e6465 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 18 Oct 2019 16:43:52 +0100 Subject: [PATCH 2/7] Chunk events by day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scanning the page is difficult at the moment because it’s hard to tell how far apart in time events are, and thereby determine which events might be related. Grouping the events by day quickly lets users narrow their focus to a meaningful subset of the events. --- app/__init__.py | 5 ++ app/assets/stylesheets/main.scss | 1 + app/assets/stylesheets/views/history.scss | 29 ++++++++++++ app/main/views/history.py | 18 +++++++- app/templates/views/temp-history.html | 32 +++++++++---- tests/app/main/views/test_history.py | 56 +++++++++++++++++------ tests/conftest.py | 3 +- 7 files changed, 121 insertions(+), 23 deletions(-) create mode 100644 app/assets/stylesheets/views/history.scss diff --git a/app/__init__.py b/app/__init__.py index 2b98ab712..fb904e4f6 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -328,6 +328,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') @@ -681,6 +685,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 217177ff5..222a44490 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -1,5 +1,8 @@ +from collections import defaultdict + from flask import render_template +from app import current_service, format_date_numeric from app.main import main from app.utils import user_has_permissions @@ -7,4 +10,17 @@ from app.utils import user_has_permissions @main.route("/services//history") @user_has_permissions('manage_service') def history(service_id): - return render_template('views/temp-history.html') + return render_template( + 'views/temp-history.html', + days=_chunk_events_by_day(current_service.history) + ) + + +def _chunk_events_by_day(events): + + days = defaultdict(list) + + for event in reversed(events): + days[format_date_numeric(event.time)].append(event) + + return sorted(days.items(), reverse=True) diff --git a/app/templates/views/temp-history.html b/app/templates/views/temp-history.html index 6ca2b7764..798047c49 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -10,12 +10,28 @@ Service and API key history {{ page_header("Service and API key history") }} -
    - {% for event in current_service.history|reverse %} -
  • - {{ event.time|format_datetime_relative }} {{ event.user_id }}
    - {{ event|string }} -
    • - {% endfor %} -
+ {% for day, events in days %} +

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

+
    + {% for event in events %} +
  • +
    +
    +
    + {{ event.user_id }} +
    +
    + {{ event.time|format_time }} +
    +
    +
    + {{ event }} +
    +
    • + {% endfor%} +
+ {% endfor %} + {% endblock %} diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 17e8ba7a7..6a41898b8 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -7,16 +7,46 @@ def test_history( ): page = client_request.get('main.history', service_id=SERVICE_ONE_ID) - assert normalize_spaces( - page.select_one('main').text - ) == ( - 'Service and API key history ' - '11 November at 12:12pm 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' - ' Revoked the ‘Bad key’ API key ' - '11 November at 11:11am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' - ' Created an API key called ‘Bad key’ ' - '10 October at 11:10am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' - ' Created an API key called ‘Good key’ ' - '10 October at 11:10am 6ce466d0-fd6a-11e5-82f5-e0accb9d11a6' - ' Created this service and called it ‘Example service’' - ) + assert page.select_one('h1').text == 'Service and API key history' + + headings = page.select('main h2') + events = page.select('main ul') + + assert len(headings) == len(events) + assert [ + ( + normalize_spaces(headings[index].text), + normalize_spaces(events[index].text), + ) for index in range(len(headings)) + ] == [ + ( + '12 December', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Renamed this service from ‘Example service’ to ‘Real service’' + ), + ), + ( + '11 November', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Revoked the ‘Bad key’ API key' + ), + ), + ( + '11 November', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:11am ' + 'Created an API key called ‘Bad key’' + ), + ), + ( + '10 October', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' + 'Created an API key called ‘Good key’ ' + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' + 'Created this service and called it ‘Example service’' + ), + ), + ] diff --git a/tests/conftest.py b/tests/conftest.py index ca097949f..b077122bc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3514,9 +3514,10 @@ def mock_get_service_history(mocker): 'created_by_id': sample_uuid(), }, { + 'name': 'Real service', 'created_at': '2010-10-10T10:10:10.000000Z', 'updated_at': '2012-12-12T12:12:12.000000Z', - 'created_by_id': uuid4(), + 'created_by_id': sample_uuid(), }, ], 'api_key_history': [ From d93ebd99d3a69a9242f747948c8ddb1ab248f1f3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Oct 2019 12:14:41 +0100 Subject: [PATCH 3/7] Refactor history off the service model Directly referencing the `ModelList` instances will let us more easily make choices at the view layer about which kinds of events to show, and is one less layer of indirection to jump through. --- app/main/views/history.py | 9 ++++++++- app/models/service.py | 10 ---------- tests/app/main/views/test_history.py | 4 ++-- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/main/views/history.py b/app/main/views/history.py index 222a44490..00669219e 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -4,6 +4,7 @@ from flask import render_template from app import current_service, format_date_numeric from app.main import main +from app.models.event import APIKeyEvents, ServiceEvents from app.utils import user_has_permissions @@ -12,10 +13,16 @@ from app.utils import user_has_permissions def history(service_id): return render_template( 'views/temp-history.html', - days=_chunk_events_by_day(current_service.history) + days=_chunk_events_by_day( + _get_events(current_service.id) + ) ) +def _get_events(service_id): + return APIKeyEvents(service_id) + ServiceEvents(service_id) + + def _chunk_events_by_day(events): days = defaultdict(list) diff --git a/app/models/service.py b/app/models/service.py index 341ecf6a7..3e3e93fd6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,5 +1,3 @@ -from operator import attrgetter - from flask import Markup, abort, current_app from notifications_utils.field import Field from notifications_utils.formatters import nl2br @@ -7,7 +5,6 @@ from notifications_utils.take import Take from werkzeug.utils import cached_property from app.models import JSONModel -from app.models.event import APIKeyEvents, ServiceEvents from app.models.organisation import Organisation from app.models.user import InvitedUsers, User, Users from app.notify_client.api_key_api_client import api_key_api_client @@ -634,10 +631,3 @@ class Service(JSONModel): ): if test: yield BASE + '_incomplete' + tag - - @property - def history(self): - return sorted( - ServiceEvents(self.id) + APIKeyEvents(self.id), - key=attrgetter('time'), - ) diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 6a41898b8..92e1985de 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -44,9 +44,9 @@ def test_history( '10 October', ( '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' - 'Created an API key called ‘Good key’ ' + 'Created this service and called it ‘Example service’ ' '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' - 'Created this service and called it ‘Example service’' + 'Created an API key called ‘Good key’' ), ), ] From 63f6a3ab12e02a2c2ac55669b132f54a9576a8ab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Oct 2019 13:16:48 +0100 Subject: [PATCH 4/7] Let users filter by type of event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment we have two types of event, ‘service’ events and ‘API key’ events. They are munged together which is useful initially, but could get noisy. This commit adds filters (copied from the choose template page) that let users narrow down the list to one of the two types of event. This might help users get a clearer picture of what’s going on. --- app/main/views/history.py | 12 ++-- app/templates/views/temp-history.html | 15 ++++- tests/app/main/views/test_history.py | 87 +++++++++++++++++++++------ tests/conftest.py | 4 +- 4 files changed, 90 insertions(+), 28 deletions(-) diff --git a/app/main/views/history.py b/app/main/views/history.py index 00669219e..08f696494 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -1,6 +1,6 @@ from collections import defaultdict -from flask import render_template +from flask import render_template, request from app import current_service, format_date_numeric from app.main import main @@ -14,12 +14,16 @@ def history(service_id): return render_template( 'views/temp-history.html', days=_chunk_events_by_day( - _get_events(current_service.id) + _get_events(current_service.id, request.args.get('selected')) ) ) -def _get_events(service_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) @@ -27,7 +31,7 @@ def _chunk_events_by_day(events): days = defaultdict(list) - for event in reversed(events): + for event in events: days[format_date_numeric(event.time)].append(event) return sorted(days.items(), reverse=True) diff --git a/app/templates/views/temp-history.html b/app/templates/views/temp-history.html index 798047c49..47c3e24f3 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -1,14 +1,25 @@ {% 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") }} + + {{ pill( + [ + ('All', None, url_for('main.history', service_id=current_service.id), None), + ('Service', '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 + ) }} {% for day, events in days %}

diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 92e1985de..41f746ac4 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -1,24 +1,10 @@ +import pytest + from tests.conftest import SERVICE_ONE_ID, normalize_spaces -def test_history( - client_request, - mock_get_service_history, -): - page = client_request.get('main.history', service_id=SERVICE_ONE_ID) - - assert page.select_one('h1').text == 'Service and API key history' - - headings = page.select('main h2') - events = page.select('main ul') - - assert len(headings) == len(events) - assert [ - ( - normalize_spaces(headings[index].text), - normalize_spaces(events[index].text), - ) for index in range(len(headings)) - ] == [ +@pytest.mark.parametrize('extra_args, expected_headings_and_events', ( + ({}, [ ( '12 December', ( @@ -44,9 +30,70 @@ def test_history( '10 October', ( '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' - 'Created this service and called it ‘Example service’ ' + 'Created an API key called ‘Good key’ ' + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 2:01am ' + 'Created this service and called it ‘Example service’' + ), + ), + ]), + ({'selected': 'api'}, [ + ( + '11 November', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Revoked the ‘Bad key’ API key' + ), + ), + ( + '11 November', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:11am ' + 'Created an API key called ‘Bad key’' + ), + ), + ( + '10 October', + ( '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' 'Created an API key called ‘Good key’' ), ), - ] + ]), + ({'selected': 'service'}, [ + ( + '12 December', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Renamed this service from ‘Example service’ to ‘Real service’' + ), + ), + ( + '10 October', + ( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 2:01am ' + 'Created this service and called it ‘Example service’' + ), + ), + ]), +)) +def test_history( + client_request, + mock_get_service_history, + mock_get_users_by_service, + extra_args, + expected_headings_and_events, +): + 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/conftest.py b/tests/conftest.py index b077122bc..05792f2e7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3509,13 +3509,13 @@ def mock_get_service_history(mocker): 'service_history': [ { 'name': 'Example service', - 'created_at': '2010-10-10T10:10:10.000000Z', + 'created_at': '2010-10-10T01:01:01.000000Z', 'updated_at': None, 'created_by_id': sample_uuid(), }, { 'name': 'Real service', - 'created_at': '2010-10-10T10:10:10.000000Z', + 'created_at': '2010-10-10T01:01:01.000000Z', 'updated_at': '2012-12-12T12:12:12.000000Z', 'created_by_id': sample_uuid(), }, From 9055a33dcab1ba225efc5ad5999e5c323aa51c28 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Oct 2019 13:42:28 +0100 Subject: [PATCH 5/7] Only show the filters if they will have an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you never create any API keys we shouldn’t give you the option to see API-related events – it will only confuse things. And since there’s (currently) only one type of event left once you take API key events out of the picture it doesn’t make sense to show the filters at all. --- app/main/views/history.py | 10 +++++++--- app/templates/views/temp-history.html | 22 +++++++++++++--------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/app/main/views/history.py b/app/main/views/history.py index 08f696494..2512cfe72 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -4,17 +4,21 @@ from flask import render_template, request from app import current_service, format_date_numeric from app.main import main -from app.models.event import APIKeyEvents, ServiceEvents +from app.models.event import APIKeyEvent, APIKeyEvents, ServiceEvents from app.utils import user_has_permissions @main.route("/services//history") @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', - days=_chunk_events_by_day( - _get_events(current_service.id, request.args.get('selected')) + days=_chunk_events_by_day(events), + show_navigation=request.args.get('selected') or any( + isinstance(event, APIKeyEvent) for event in events ) ) diff --git a/app/templates/views/temp-history.html b/app/templates/views/temp-history.html index 47c3e24f3..2047e6236 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -11,15 +11,19 @@ {{ page_header("Audit events") }} - {{ pill( - [ - ('All', None, url_for('main.history', service_id=current_service.id), None), - ('Service', '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 - ) }} + {% 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 %} {% for day, events in days %}

From 600e3affc12947d5a7b957b71e3d2864467d6bc2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Oct 2019 14:08:18 +0100 Subject: [PATCH 6/7] Show user names for events without API changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces a slightly hacky way of putting usernames against events, given that the API only returns user IDs. It does so without: - making changes to the API - making a pages that could potentially fire off dozens of API calls (ie one per user) This comes with the limitation that it can only get names for those team members who are still in the team. Otherwise it will say ‘Unknown’. In the future the API should probably return the name and email address for the user who initiated the event, and whether that user was acting in a platform admin capacity. --- app/main/views/history.py | 3 ++- app/models/user.py | 6 ++++++ app/templates/views/temp-history.html | 2 +- tests/app/main/views/test_history.py | 20 ++++++++++---------- tests/conftest.py | 2 +- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/app/main/views/history.py b/app/main/views/history.py index 2512cfe72..07614db3a 100644 --- a/app/main/views/history.py +++ b/app/main/views/history.py @@ -19,7 +19,8 @@ def history(service_id): 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, ) 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/templates/views/temp-history.html b/app/templates/views/temp-history.html index 2047e6236..0c1d79f76 100644 --- a/app/templates/views/temp-history.html +++ b/app/templates/views/temp-history.html @@ -35,7 +35,7 @@
- {{ event.user_id }} + {{ user_getter(event.user_id) }}
{{ event.time|format_time }} diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 41f746ac4..09da2d0a9 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -8,30 +8,30 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ( '12 December', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Test User 12:12pm ' 'Renamed this service from ‘Example service’ to ‘Real service’' ), ), ( '11 November', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Test User 12:12pm ' 'Revoked the ‘Bad key’ API key' ), ), ( '11 November', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:11am ' + 'Test User 11:11am ' 'Created an API key called ‘Bad key’' ), ), ( '10 October', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' + 'Test User 11:10am ' 'Created an API key called ‘Good key’ ' - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 2:01am ' + 'Unknown 2:01am ' 'Created this service and called it ‘Example service’' ), ), @@ -40,21 +40,21 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ( '11 November', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Test User 12:12pm ' 'Revoked the ‘Bad key’ API key' ), ), ( '11 November', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:11am ' + 'Test User 11:11am ' 'Created an API key called ‘Bad key’' ), ), ( '10 October', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 11:10am ' + 'Test User 11:10am ' 'Created an API key called ‘Good key’' ), ), @@ -63,14 +63,14 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ( '12 December', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 12:12pm ' + 'Test User 12:12pm ' 'Renamed this service from ‘Example service’ to ‘Real service’' ), ), ( '10 October', ( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6 2:01am ' + 'Unknown 2:01am ' 'Created this service and called it ‘Example service’' ), ), diff --git a/tests/conftest.py b/tests/conftest.py index 05792f2e7..271c6001f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3511,7 +3511,7 @@ def mock_get_service_history(mocker): 'name': 'Example service', 'created_at': '2010-10-10T01:01:01.000000Z', 'updated_at': None, - 'created_by_id': sample_uuid(), + 'created_by_id': uuid4(), }, { 'name': 'Real service', From a765920f6571ade405a08cb08f7078f037be5d0b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Oct 2019 14:14:59 +0100 Subject: [PATCH 7/7] Add years to old dates Just to disambiguate things without introducing unnecessary noise. --- app/__init__.py | 10 +++++++--- tests/app/main/views/test_history.py | 12 +++++++----- tests/app/main/views/test_jobs.py | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index fb904e4f6..e040868a2 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -297,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) diff --git a/tests/app/main/views/test_history.py b/tests/app/main/views/test_history.py index 09da2d0a9..8b0e64d13 100644 --- a/tests/app/main/views/test_history.py +++ b/tests/app/main/views/test_history.py @@ -1,4 +1,5 @@ import pytest +from freezegun import freeze_time from tests.conftest import SERVICE_ONE_ID, normalize_spaces @@ -20,14 +21,14 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ), ), ( - '11 November', + '11 November 2011', ( 'Test User 11:11am ' 'Created an API key called ‘Bad key’' ), ), ( - '10 October', + '10 October 2010', ( 'Test User 11:10am ' 'Created an API key called ‘Good key’ ' @@ -45,14 +46,14 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ), ), ( - '11 November', + '11 November 2011', ( 'Test User 11:11am ' 'Created an API key called ‘Bad key’' ), ), ( - '10 October', + '10 October 2010', ( 'Test User 11:10am ' 'Created an API key called ‘Good key’' @@ -68,7 +69,7 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ), ), ( - '10 October', + '10 October 2010', ( 'Unknown 2:01am ' 'Created this service and called it ‘Example service’' @@ -76,6 +77,7 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces ), ]), )) +@freeze_time("2012-01-01 01:01:01") def test_history( client_request, mock_get_service_history, 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'