Merge pull request #1940 from alphagov/dashboard-day-limit

Sort out the dashboard day limits

app/dao/services_dao.py

@@ -37,7 +37,7 @@ from app.models import (
     SMS_TYPE,
     LETTER_TYPE,
 )
-from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc
+from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, midnight_n_days_ago
 
 DEFAULT_SERVICE_PERMISSIONS = [
     SMS_TYPE,
@@ -247,8 +247,8 @@ def delete_service_and_all_associated_db_objects(service):
 
 @statsd(namespace="dao")
 def dao_fetch_stats_for_service(service_id):
-    # We want 7 days inclusive
+    # We always want between seven and eight days
-    start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6))
+    start_date = midnight_n_days_ago(7)
     return _stats_for_service_query(service_id).filter(
         Notification.created_at >= start_date
     ).all()

app/template_statistics/rest.py

@@ -29,17 +29,18 @@ register_errors(template_statistics)
 
 @template_statistics.route('')
 def get_template_statistics_for_service_by_day(service_id):
+    whole_days = request.args.get('whole_days', request.args.get('limit_days', ''))
     try:
-        limit_days = int(request.args.get('limit_days', ''))
+        whole_days = int(whole_days)
     except ValueError:
-        error = '{} is not an integer'.format(request.args.get('limit_days'))
+        error = '{} is not an integer'.format(whole_days)
-        message = {'limit_days': [error]}
+        message = {'whole_days': [error]}
         raise InvalidRequest(message, status_code=400)
 
-    if limit_days < 1 or limit_days > 7:
+    if whole_days < 0 or whole_days > 7:
-        raise InvalidRequest({'limit_days': ['limit_days must be between 1 and 7']}, status_code=400)
+        raise InvalidRequest({'whole_days': ['whole_days must be between 0 and 7']}, status_code=400)
 
-    return jsonify(data=_get_template_statistics_for_last_n_days(service_id, limit_days))
+    return jsonify(data=_get_template_statistics_for_last_n_days(service_id, whole_days))
 
 
 @template_statistics.route('/<template_id>')
@@ -54,10 +55,12 @@ def get_template_statistics_for_template_id(service_id, template_id):
     return jsonify(data=data)
 
 
-def _get_template_statistics_for_last_n_days(service_id, limit_days):
+def _get_template_statistics_for_last_n_days(service_id, whole_days):
     template_stats_by_id = Counter()
 
-    for day in last_n_days(limit_days):
+    # 0 whole_days = last 1 days (ie since midnight today) = today.
+    # 7 whole days = last 8 days (ie since midnight this day last week) = a week and a bit
+    for day in last_n_days(whole_days + 1):
         # "{SERVICE_ID}-template-usage-{YYYY-MM-DD}"
         key = cache_key_for_service_template_usage_per_day(service_id, day)
         stats = redis_store.get_all_from_hash(key)

tests/app/dao/test_services_dao.py

@@ -620,30 +620,23 @@ def test_fetch_stats_for_today_only_includes_today(notify_db, notify_db_session,
     assert stats['created'] == 1
 
 
-def test_fetch_stats_should_not_gather_notifications_older_than_7_days(notify_db, notify_db_session, sample_template):
+@pytest.mark.parametrize('created_at, rows_returned', [
+    ('Sunday 8th July 2018 12:00', 0),
+    ('Sunday 8th July 2018 22:59', 0),
+    ('Sunday 8th July 2018 23:00', 1),
+    ('Monday 9th July 2018 09:00', 1),
+    ('Monday 9th July 2018 15:00', 1),
+    ('Monday 16th July 2018 12:00', 1),
+])
+def test_fetch_stats_should_not_gather_notifications_older_than_7_days(sample_template, created_at, rows_returned):
+    # It's monday today. Things made last monday should still show
+    with freeze_time(created_at):
+        create_notification_db(sample_template,)
 
-    # 8 days ago
+    with freeze_time('Monday 16th July 2018 12:00'):
-    create_notification(notify_db, None, to_field='1', status='delivered', created_at='2001-04-02T12:00:00')
-    create_notification(notify_db, None, to_field='1', status='delivered', created_at='2001-04-02T22:59:59')
-
-    # 7 days ago BST midnight
-    create_notification(notify_db, None, to_field='1', status='failed', created_at='2001-04-02T23:00:00')
-    # 7 days ago, 2hours ago
-    create_notification(notify_db, None, to_field='2', status='failed', created_at='2001-04-03T10:00:00')
-
-    # 7 days ago
-    create_notification(notify_db, None, to_field='2', status='failed', created_at='2001-04-03T12:00:00')
-
-    # right_now
-    create_notification(notify_db, None, to_field='3', status='created', created_at='2001-04-09T12:00:00')
-
-    with freeze_time('2001-04-09T12:00:00'):
         stats = dao_fetch_stats_for_service(sample_template.service_id)
 
-    stats = {row.status: row.count for row in stats}
+    assert len(stats) == rows_returned
-    assert 'delivered' not in stats
-    assert stats['failed'] == 3
-    assert stats['created'] == 1
 
 
 def test_dao_fetch_todays_total_message_count_returns_count_for_today(notify_db,

tests/app/job/test_rest.py

@@ -619,28 +619,24 @@ def test_get_jobs(client, notify_db, notify_db_session, sample_template):
     assert len(resp_json['data']) == 5
 
 
-def test_get_jobs_with_limit_days(client, notify_db, notify_db_session, sample_template):
+def test_get_jobs_with_limit_days(admin_request, notify_db, notify_db_session, sample_template):
-    create_job(
+    for time in [
-        notify_db,
+        'Sunday 1st July 2018 22:59',
-        notify_db_session,
+        'Sunday 2nd July 2018 23:00',  # beginning of monday morning
-        service=sample_template.service,
+        'Monday 3rd July 2018 12:00'
-        template=sample_template,
+    ]:
-    )
+        with freeze_time(time):
-    create_job(
+            create_job(
-        notify_db,
+                notify_db,
-        notify_db_session,
+                notify_db_session,
-        service=sample_template.service,
+                service=sample_template.service,
-        template=sample_template,
+                template=sample_template,
-        created_at=datetime.now() - timedelta(days=7))
+            )
 
-    service_id = sample_template.service.id
+    with freeze_time('Monday 9th July 2018 12:00'):
+        resp_json = admin_request.get('job.get_jobs_by_service', service_id=sample_template.service_id, limit_days=7)
 
-    path = '/service/{}/job'.format(service_id)
+    assert len(resp_json['data']) == 2
-    auth_header = create_authorization_header()
-    response = client.get(path, headers=[auth_header], query_string={'limit_days': 5})
-    assert response.status_code == 200
-    resp_json = json.loads(response.get_data(as_text=True))
-    assert len(resp_json['data']) == 1
 
 
 def test_get_jobs_should_return_statistics(client, notify_db, notify_db_session, notify_api, sample_service):

tests/app/template_statistics/test_rest.py

@@ -32,10 +32,10 @@ def set_up_get_all_from_hash(mock_redis, side_effect):
 
 @pytest.mark.parametrize('query_string', [
     {},
-    {'limit_days': 0},
+    {'whole_days': -1},
-    {'limit_days': 8},
+    {'whole_days': 8},
-    {'limit_days': 3.5},
+    {'whole_days': 3.5},
-    {'limit_days': 'blurk'},
+    {'whole_days': 'blurk'},
 ])
 def test_get_template_statistics_for_service_by_day_with_bad_arg_returns_400(admin_request, query_string):
     json_resp = admin_request.get(
@@ -45,14 +45,14 @@ def test_get_template_statistics_for_service_by_day_with_bad_arg_returns_400(adm
         _expected_status=400
     )
     assert json_resp['result'] == 'error'
-    assert 'limit_days' in json_resp['message']
+    assert 'whole_days' in json_resp['message']
 
 
 def test_get_template_statistics_for_service_by_day_returns_template_info(admin_request, mocker, sample_notification):
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_notification.service_id,
-        limit_days=1
+        whole_days=1
     )
 
     assert len(json_resp['data']) == 1
@@ -64,6 +64,22 @@ def test_get_template_statistics_for_service_by_day_returns_template_info(admin_
     assert json_resp['data'][0]['is_precompiled_letter'] is False
 
 
+@pytest.mark.parametrize('var_name', ['limit_days', 'whole_days'])
+def test_get_template_statistics_for_service_by_day_accepts_old_query_string(
+    admin_request,
+    mocker,
+    sample_notification,
+    var_name
+):
+    json_resp = admin_request.get(
+        'template_statistics.get_template_statistics_for_service_by_day',
+        service_id=sample_notification.service_id,
+        **{var_name: 1}
+    )
+
+    assert len(json_resp['data']) == 1
+
+
 @freeze_time('2018-01-01 12:00:00')
 def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_available(
     admin_request,
@@ -78,7 +94,7 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_template.service_id,
-        limit_days=1
+        whole_days=0
     )
 
     assert len(json_resp['data']) == 1
@@ -112,7 +128,7 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis(
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_template.service_id,
-        limit_days=2
+        whole_days=1
     )
 
     assert len(json_resp['data']) == 1
@@ -161,7 +177,7 @@ def test_get_template_statistics_for_service_by_day_combines_templates_correctly
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_service.id,
-        limit_days=3
+        whole_days=2
     )
 
     assert len(json_resp['data']) == 2
@@ -185,13 +201,14 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days(
 
     # first time it is called redis returns data, second time returns none
     set_up_get_all_from_hash(mock_redis, [
-        {sample_template.id: 1},
+        {sample_template.id: 1},  # last weds
         None,
         {sample_template.id: 1},
         {sample_template.id: 1},
         {sample_template.id: 1},
+        {sample_template.id: 1},
         None,
-        None,
+        None,  # current day
     ])
     mock_dao = mocker.patch(
         'app.template_statistics.rest.dao_get_template_usage',
@@ -203,25 +220,26 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days(
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_template.service_id,
-        limit_days=7
+        whole_days=7
     )
 
     assert len(json_resp['data']) == 1
-    assert json_resp['data'][0]['count'] == 10
+    assert json_resp['data'][0]['count'] == 11
     assert json_resp['data'][0]['template_id'] == str(sample_template.id)
 
-    assert mock_redis.get_all_from_hash.call_count == 7
+    assert mock_redis.get_all_from_hash.call_count == 8
 
-    assert '2018-03-22' in mock_redis.get_all_from_hash.mock_calls[0][1][0]
+    assert '2018-03-21' in mock_redis.get_all_from_hash.mock_calls[0][1][0]  # last wednesday
-    assert '2018-03-23' in mock_redis.get_all_from_hash.mock_calls[1][1][0]
+    assert '2018-03-22' in mock_redis.get_all_from_hash.mock_calls[1][1][0]
-    assert '2018-03-24' in mock_redis.get_all_from_hash.mock_calls[2][1][0]
+    assert '2018-03-23' in mock_redis.get_all_from_hash.mock_calls[2][1][0]
-    assert '2018-03-25' in mock_redis.get_all_from_hash.mock_calls[3][1][0]
+    assert '2018-03-24' in mock_redis.get_all_from_hash.mock_calls[3][1][0]
-    assert '2018-03-26' in mock_redis.get_all_from_hash.mock_calls[4][1][0]
+    assert '2018-03-25' in mock_redis.get_all_from_hash.mock_calls[4][1][0]
-    assert '2018-03-27' in mock_redis.get_all_from_hash.mock_calls[5][1][0]
+    assert '2018-03-26' in mock_redis.get_all_from_hash.mock_calls[5][1][0]
-    assert '2018-03-28' in mock_redis.get_all_from_hash.mock_calls[6][1][0]
+    assert '2018-03-27' in mock_redis.get_all_from_hash.mock_calls[6][1][0]
+    assert '2018-03-28' in mock_redis.get_all_from_hash.mock_calls[7][1][0]  # current day (wednesday)
 
     mock_dao.mock_calls == [
-        call(ANY, day=datetime(2018, 3, 23)),
+        call(ANY, day=datetime(2018, 3, 22)),
         call(ANY, day=datetime(2018, 3, 27)),
         call(ANY, day=datetime(2018, 3, 28))
     ]
@@ -237,11 +255,11 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem
     json_resp = admin_request.get(
         'template_statistics.get_template_statistics_for_service_by_day',
         service_id=sample_service.id,
-        limit_days=7
+        whole_days=7
     )
 
     assert len(json_resp['data']) == 0
-    assert mock_redis.get_all_from_hash.call_count == 7
+    assert mock_redis.get_all_from_hash.call_count == 8
     # make sure we don't try and set any empty hashes in redis
     assert mock_redis.set_hash_and_expire.call_count == 0