From 47c403f6ab17a108773c6d0229438c6b9edeb2d3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 7 Jan 2019 17:12:00 +0000 Subject: [PATCH] Don't return pagination links for API Message log requests Flask-SQLAlchemy paginate function issues a separate query to get the total count of rows for a given filter. This query (with filters used by the API integration Message log page) is slow for services with large number of notifications. Since Message log page doesn't actually allow users to paginate through the response (it only shows the last 50 messages) we can use limit instead of paginate, which requires passing in another flag from admin to the dao method. `count` flag has been added to `paginate` in March 2018, however there was no release of flask-sqlalchemy since then, so we need to pull the dev version of the package from Github. --- app/dao/notifications_dao.py | 4 +++- app/schemas.py | 1 + app/service/rest.py | 4 ++++ requirements-app.txt | 2 +- requirements.txt | 2 +- .../notification_dao/test_notification_dao.py | 10 +++++++++ tests/app/service/test_rest.py | 22 +++++++++++++++++++ 7 files changed, 42 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index c78582851..9426a28c0 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -255,6 +255,7 @@ def get_notifications_for_service( filter_dict=None, page=1, page_size=None, + count_pages=True, limit_days=None, key_type=None, personalisation=False, @@ -300,7 +301,8 @@ def get_notifications_for_service( return query.order_by(desc(Notification.created_at)).paginate( page=page, - per_page=page_size + per_page=page_size, + count=count_pages ) diff --git a/app/schemas.py b/app/schemas.py index 460b52240..0ebc42dc4 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -596,6 +596,7 @@ class NotificationsFilterSchema(ma.Schema): format_for_csv = fields.String() to = fields.String() include_one_off = fields.Boolean(required=False) + count_pages = fields.Boolean(required=False) @pre_load def handle_multidict(self, in_data): diff --git a/app/service/rest.py b/app/service/rest.py index f1c4f947e..fd03e7d05 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -345,16 +345,20 @@ def get_all_notifications_for_service(service_id): include_from_test_key = data.get('include_from_test_key', False) include_one_off = data.get('include_one_off', True) + count_pages = data.get('count_pages', True) + pagination = notifications_dao.get_notifications_for_service( service_id, filter_dict=data, page=page, page_size=page_size, + count_pages=count_pages, limit_days=limit_days, include_jobs=include_jobs, include_from_test_key=include_from_test_key, include_one_off=include_one_off ) + kwargs = request.args.to_dict() kwargs['service_id'] = service_id diff --git a/requirements-app.txt b/requirements-app.txt index 09c0ee350..f4097456a 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -7,7 +7,7 @@ docopt==0.6.2 Flask-Bcrypt==0.7.1 flask-marshmallow==0.9.0 Flask-Migrate==2.3.0 -Flask-SQLAlchemy==2.3.2 +git+https://github.com/mitsuhiko/flask-sqlalchemy.git@500e732dd1b975a56ab06a46bd1a20a21e682262#egg=Flask-SQLAlchemy==2.3.2.dev20190108 Flask==1.0.2 click-datetime==0.2 eventlet==0.23.0 diff --git a/requirements.txt b/requirements.txt index f711cac44..e232d5b51 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,7 +9,7 @@ docopt==0.6.2 Flask-Bcrypt==0.7.1 flask-marshmallow==0.9.0 Flask-Migrate==2.3.0 -Flask-SQLAlchemy==2.3.2 +git+https://github.com/mitsuhiko/flask-sqlalchemy.git@500e732dd1b975a56ab06a46bd1a20a21e682262#egg=Flask-SQLAlchemy==2.3.2.dev20190108 Flask==1.0.2 click-datetime==0.2 eventlet==0.23.0 diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index ad8875656..197b6556a 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -909,6 +909,16 @@ def test_should_return_notifications_including_one_offs_by_default(sample_user, assert len(include_one_offs_by_default) == 2 +def test_should_not_count_pages_when_given_a_flag(sample_user, sample_template): + create_notification(sample_template) + notification = create_notification(sample_template) + + pagination = get_notifications_for_service(sample_template.service_id, count_pages=False, page_size=1) + assert len(pagination.items) == 1 + assert pagination.total is None + assert pagination.items[0].id == notification.id + + def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excluding_test_key_notifications( notify_db, notify_db_session, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index a1a40b3a8..c12b0e223 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1407,6 +1407,28 @@ def test_get_only_api_created_notifications_for_service( assert resp['notifications'][0]['id'] == str(without_job.id) +def test_get_notifications_for_service_without_page_count( + admin_request, + sample_job, + sample_template, + sample_user, +): + create_notification(sample_template) + without_job = create_notification(sample_template) + + resp = admin_request.get( + 'service.get_all_notifications_for_service', + service_id=sample_template.service_id, + page_size=1, + include_jobs=False, + include_one_off=False, + count_pages=False + ) + assert len(resp['notifications']) == 1 + assert resp['total'] is None + assert resp['notifications'][0]['id'] == str(without_job.id) + + @pytest.mark.parametrize('should_prefix', [ True, False,