Merge pull request #3152 from alphagov/history-page-redesign

Redesign the history page as one stream of events

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,

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';

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;
+  }
+
+}

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)

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])

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
+        ]

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']

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

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))

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") }}
 
-  <div class="grid-row">
-    {% 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 %}
+    <div class="bottom-gutter">
+      {{ 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
+      ) }}
+    </div>
+  {% endif %}
 
-    {% endcall %}
+  {% for day, events in days %}
+    <h2 class="heading-small top-gutter">
+      {{ events[0].time|format_date_human|title }}
+    </h2>
+    <ul class="bottom-gutter">
+      {% for event in events %}
+        <li class="history-list-item">
+          <div class="grid-row">
+            <div class="column-one-third">
+              <div class="history-list-user">
+                {{ user_getter(event.user_id) }}
+              </div>
+              <div class="history-list-time">
+                {{ event.time|format_time }}
+              </div>
+            </div>
+            <div class="column-two-thirds">
+              {{ event }}
+            </div>
+        </li>
+      {% endfor%}
+    </ul>
+  {% endfor %}
 
-  </div>
-
-
-  <div class="grid-row">
-    {% 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 %}
-
-  </div
-
-  <div class="grid-row">
-
-     {% 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 %}
-  </div>
 {% endblock %}

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

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

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'

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

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': [],
-    }})
+    })