diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index d3207e649..1aa958ca3 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -105,7 +105,7 @@ def get_sms_thread(service_id, user_number): notification_api_client.get_notifications_for_service(service_id, to=user_number, template_type='sms')['notifications'] + - service_api_client.get_inbound_sms(service_id, user_number=user_number) + service_api_client.get_inbound_sms(service_id, user_number=user_number)['data'] ), key=lambda notification: notification['created_at']): is_inbound = ('notify_number' in notification) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index d5e699eff..aa5f97276 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -12,7 +12,6 @@ from flask import ( url_for, ) from flask_login import login_required -from notifications_utils.recipients import format_phone_number_human_readable from werkzeug.utils import redirect from app import ( @@ -31,6 +30,8 @@ from app.utils import ( FAILURE_STATUSES, REQUESTED_STATUSES, Spreadsheet, + generate_next_dict, + generate_previous_dict, get_current_financial_year, user_has_permissions, ) @@ -195,7 +196,7 @@ def inbox(service_id): return render_template( 'views/dashboard/inbox.html', partials=get_inbox_partials(service_id), - updates_url=url_for('.inbox_updates', service_id=service_id), + updates_url=url_for('.inbox_updates', service_id=service_id, page=request.args.get('page')), ) @@ -221,7 +222,7 @@ def inbox_download(service_id): message['user_number'], message['content'].lstrip(('=+-@')), format_datetime_numeric(message['created_at']), - ] for message in service_api_client.get_inbound_sms(service_id)] + ] for message in service_api_client.get_inbound_sms(service_id)['data']] ).as_csv_data, mimetype='text/csv', headers={ @@ -233,33 +234,32 @@ def inbox_download(service_id): def get_inbox_partials(service_id): - + page = int(request.args.get('page', 1)) if 'inbound_sms' not in current_service['permissions']: abort(403) - inbound_messages = service_api_client.get_inbound_sms(service_id) - - messages_to_show = {} - # get the most recent message for each number - for message in inbound_messages: - human_readable = format_phone_number_human_readable(message['user_number']) - if human_readable not in messages_to_show: - messages_to_show[human_readable] = message - - count_of_users = len(messages_to_show) - messages_to_show = sorted(messages_to_show.values(), key=lambda x: x['created_at'], reverse=True) + inbound_messages_data = service_api_client.get_most_recent_inbound_sms(service_id, page=page) + inbound_messages = inbound_messages_data['data'] if not inbound_messages: inbound_number = inbound_number_client.get_inbound_sms_number_for_service(service_id)['data']['number'] else: inbound_number = None + prev_page = None + if page > 1: + prev_page = generate_previous_dict('main.inbox', service_id, page) + next_page = None + if inbound_messages_data['has_next']: + next_page = generate_next_dict('main.inbox', service_id, page) + return {'messages': render_template( 'views/dashboard/_inbox_messages.html', - messages=list(messages_to_show), - count_of_messages=len(inbound_messages), - count_of_users=count_of_users, + messages=inbound_messages, inbound_number=inbound_number, + prev_page=prev_page, + next_page=next_page + )} diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 5635dcb5f..8908fc8a6 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -261,13 +261,26 @@ class ServiceAPIClient(NotifyAdminAPIClient): def update_whitelist(self, service_id, data): return self.put(url='/service/{}/whitelist'.format(service_id), data=data) - def get_inbound_sms(self, service_id, user_number=''): + def get_inbound_sms(self, service_id, user_number='', page=None): return self.get( - '/service/{}/inbound-sms?user_number={}'.format( + '/service/{}/inbound-sms'.format( service_id, - user_number, - ) - )['data'] + ), + params={ + 'user_number': user_number, + 'page': page + } + ) + + def get_most_recent_inbound_sms(self, service_id, page=None): + return self.get( + '/service/{}/inbound-sms/most-recent'.format( + service_id, + ), + params={ + 'page': page + } + ) def get_inbound_sms_by_id(self, service_id, notification_id): return self.get( diff --git a/app/templates/views/dashboard/_inbox_messages.html b/app/templates/views/dashboard/_inbox_messages.html index bba1e741d..37d9a317b 100644 --- a/app/templates/views/dashboard/_inbox_messages.html +++ b/app/templates/views/dashboard/_inbox_messages.html @@ -1,4 +1,5 @@ {% from "components/table.html" import list_table, field, hidden_field_heading, right_aligned_field_heading, row_heading %} +{% from "components/previous-next-navigation.html" import previous_next_navigation %} {% from "components/message-count-label.html" import message_count_label %}
@@ -33,10 +34,7 @@ {% endcall %} {% endcall %} - {% if messages %} -

- {{ count_of_messages }} message{{ '' if 1 == count_of_messages else 's' }} - from {{ count_of_users }} user{{ '' if 1 == count_of_users else 's' }} -

- {% endif %} + + {{ previous_next_navigation(prev_page, next_page) }}
+ diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index fbd24fbcf..c8d7df5f9 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -196,13 +196,16 @@ def test_view_conversation_with_empty_inbound( ): mock_get_inbound_sms = mocker.patch( 'app.main.views.conversation.service_api_client.get_inbound_sms', - return_value=[{ - 'user_number': '07900000001', - 'notify_number': '07900000002', - 'content': '', - 'created_at': datetime.utcnow().isoformat(), - 'id': fake_uuid - }] + return_value={ + 'has_next': False, + 'data': [{ + 'user_number': '07900000001', + 'notify_number': '07900000002', + 'content': '', + 'created_at': datetime.utcnow().isoformat(), + 'id': fake_uuid + }] + } ) page = client_request.get( diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 562c0ea8b..960ab33e7 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -164,10 +164,13 @@ def test_inbound_messages_shows_count_of_messages( @pytest.mark.parametrize('index, expected_row', enumerate([ '07900 900000 message-1 1 hour ago', + '07900 900000 message-2 1 hour ago', + '07900 900000 message-3 1 hour ago', '07900 900002 message-4 3 hours ago', '07900 900004 message-5 5 hours ago', '07900 900006 message-6 7 hours ago', '07900 900008 message-7 9 hours ago', + '07900 900008 message-8 9 hours ago', ])) def test_inbox_showing_inbound_messages( logged_in_client, @@ -177,7 +180,7 @@ def test_inbox_showing_inbound_messages( mock_get_detailed_service, mock_get_template_statistics, mock_get_usage, - mock_get_inbound_sms, + mock_get_most_recent_inbound_sms, index, expected_row, ): @@ -189,17 +192,35 @@ def test_inbox_showing_inbound_messages( assert response.status_code == 200 rows = page.select('tbody tr') - assert len(rows) == 5 + assert len(rows) == 8 assert normalize_spaces(rows[index].text) == expected_row - assert normalize_spaces(page.select('.table-show-more-link')) == ( - '8 messages from 5 users' - ) assert page.select_one('a[download]')['href'] == url_for( 'main.inbox_download', service_id=SERVICE_ONE_ID, ) +def test_get_inbound_sms_shows_page_links( + logged_in_client, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_most_recent_inbound_sms, + mock_get_inbound_number_for_service, +): + service_one['permissions'] = ['inbound_sms'] + + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID, page=2)) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert 'Next page' in page.find('li', {'class': 'next-page'}).text + assert 'Previous page' in page.find('li', {'class': 'previous-page'}).text + + def test_empty_inbox( logged_in_client, service_one, @@ -208,7 +229,7 @@ def test_empty_inbox( mock_get_detailed_service, mock_get_template_statistics, mock_get_usage, - mock_get_inbound_sms_with_no_messages, + mock_get_most_recent_inbound_sms_with_no_messages, mock_get_inbound_number_for_service, ): @@ -222,6 +243,8 @@ def test_empty_inbox( 'When users text your service’s phone number (0781239871) you’ll see the messages here' ) assert not page.select('a[download]') + assert not page.select('li.next-page') + assert not page.select('li.previous-page') @pytest.mark.parametrize('endpoint', [ @@ -244,7 +267,7 @@ def test_anyone_can_see_inbox( api_user_active, service_one, mocker, - mock_get_inbound_sms_with_no_messages, + mock_get_most_recent_inbound_sms_with_no_messages, mock_get_inbound_number_for_service, ): @@ -266,7 +289,7 @@ def test_view_inbox_updates( logged_in_client, service_one, mocker, - mock_get_inbound_sms_with_no_messages, + mock_get_most_recent_inbound_sms_with_no_messages, ): mock_get_partials = mocker.patch( @@ -333,13 +356,16 @@ def test_download_inbox_strips_formulae( mocker.patch( 'app.service_api_client.get_inbound_sms', - return_value=[{ - 'user_number': 'elevenchars', - 'notify_number': 'foo', - 'content': message_content, - 'created_at': datetime.utcnow().isoformat(), - 'id': fake_uuid, - }], + return_value={ + 'has_next': False, + 'data': [{ + 'user_number': 'elevenchars', + 'notify_number': 'foo', + 'content': message_content, + 'created_at': datetime.utcnow().isoformat(), + 'id': fake_uuid, + }] + }, ) response = logged_in_client.get( url_for('main.inbox_download', service_id=SERVICE_ONE_ID) diff --git a/tests/conftest.py b/tests/conftest.py index e4b16f647..a506122f2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1862,14 +1862,18 @@ def mock_get_inbound_sms(mocker): def _get_inbound_sms( service_id, user_number=None, + page=1 ): - return [{ - 'user_number': '0790090000' + str(i), - 'notify_number': '07900000002', - 'content': 'message-{}'.format(index + 1), - 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1), seconds=index)).isoformat(), - 'id': sample_uuid(), - } for index, i in enumerate([0, 0, 0, 2, 4, 6, 8, 8])] + return { + 'has_next': True, + 'data': [{ + 'user_number': '0790090000' + str(i), + 'notify_number': '07900000002', + 'content': 'message-{}'.format(index + 1), + 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1), seconds=index)).isoformat(), + 'id': sample_uuid(), + } for index, i in enumerate([0, 0, 0, 2, 4, 6, 8, 8])] + } return mocker.patch( 'app.service_api_client.get_inbound_sms', @@ -1878,15 +1882,44 @@ def mock_get_inbound_sms(mocker): @pytest.fixture(scope='function') -def mock_get_inbound_sms_with_no_messages(mocker): - def _get_inbound_sms( +def mock_get_most_recent_inbound_sms(mocker): + def _get_most_recent_inbound_sms( service_id, + user_number=None, + page=1 ): - return [] + return { + 'has_next': True, + 'data': [{ + 'user_number': '0790090000' + str(i), + 'notify_number': '07900000002', + 'content': 'message-{}'.format(index + 1), + 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1), seconds=index)).isoformat(), + 'id': sample_uuid(), + } for index, i in enumerate([0, 0, 0, 2, 4, 6, 8, 8])] + } return mocker.patch( - 'app.service_api_client.get_inbound_sms', - side_effect=_get_inbound_sms, + 'app.service_api_client.get_most_recent_inbound_sms', + side_effect=_get_most_recent_inbound_sms, + ) + + +@pytest.fixture(scope='function') +def mock_get_most_recent_inbound_sms_with_no_messages(mocker): + def _get_most_recent_inbound_sms( + service_id, + user_number=None, + page=1 + ): + return { + 'has_next': False, + 'data': [] + } + + return mocker.patch( + 'app.service_api_client.get_most_recent_inbound_sms', + side_effect=_get_most_recent_inbound_sms, )