From b2ba2afbcd896d35bdcd5078067f69f228f96594 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 23 Nov 2017 11:37:35 +0000 Subject: [PATCH] List templates in alphabetical order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently templates are ordered by the newest created first. This made sense when, after creating a new template, you were landed on the page that listed all the templates. In other words, you needed to see confirmation of the thing that you’ve just done. Now (since https://github.com/alphagov/notifications-admin/pull/1195) you get landed on the page for just that template. So you always see the template you’ve just created, no matter how the list of templates is ordered. So we are free to change the order of the templates. Ordering by time created is not great, because it gives users no control over which templates appear first. For example, our research reported this from one team: > One frustration they have is that when you add a new template it > automatically goes to the top of the list. To get round this, whenever > they add a new template they delete all of the existing ones and start > again because they want to keep their templates in numerical order. > They'd like to be able to control the order of templates in the list. We _could_ give people some sort of drag-and-drop template ordering thing. But this feels like overkill. I think that alphabetical order is better because: - it’s easily discoverable – anyone who wants to know how a list is ordered can quickly tell just by looking at it - it’s universal – everyone knows how alphabetical ordering works - it’s familiar – this is how people documents on their computer are ordered; there’s no new UI to learn - it’s what users are doing already – from the same service as above: > They number their templates 1,2a, 2b, 3a etc So this commit changes the ordering from newest created first to alphabetical. Previous changes to template order and navigation: - https://github.com/alphagov/notifications-admin/pull/1163 - https://github.com/alphagov/notifications-admin/pull/1195 - https://github.com/alphagov/notifications-admin/pull/1330 Implementation notes --- I refactored some of the tests here. I still don’t think they’re great tests, but they’re a little more Pythonic now at least. I also added a sort by template type, so that the order is deterministic when you have, for example, an email template and a text message template with the same name. If you have two text message templates with the same name you’re on your own. --- app/dao/templates_dao.py | 8 ++- tests/app/dao/test_templates_dao.py | 12 ++-- tests/app/db.py | 3 +- tests/app/template/test_rest.py | 4 +- tests/app/v2/templates/test_get_templates.py | 62 +++++++++++--------- 5 files changed, 48 insertions(+), 41 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 6cae74c0e..5343f511c 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,7 +1,7 @@ from datetime import datetime import uuid -from sqlalchemy import desc +from sqlalchemy import asc, desc from sqlalchemy.sql.expression import bindparam from app import db @@ -65,14 +65,16 @@ def dao_get_all_templates_for_service(service_id, template_type=None): template_type=template_type, archived=False ).order_by( - desc(Template.created_at) + asc(Template.name), + asc(Template.template_type), ).all() return Template.query.filter_by( service_id=service_id, archived=False ).order_by( - desc(Template.created_at) + asc(Template.name), + asc(Template.template_type), ).all() diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 607330097..62f5c8a8f 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -163,7 +163,7 @@ def test_get_all_templates_for_service(notify_db, notify_db_session, service_fac assert len(dao_get_all_templates_for_service(service_2.id)) == 2 -def test_get_all_templates_for_service_shows_newest_created_first(notify_db, notify_db_session, sample_service): +def test_get_all_templates_for_service_is_alphabetised(notify_db, notify_db_session, sample_service): template_1 = create_sample_template( notify_db, notify_db_session, @@ -190,14 +190,14 @@ def test_get_all_templates_for_service_shows_newest_created_first(notify_db, not ) assert Template.query.count() == 3 - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 3' + assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 1' assert dao_get_all_templates_for_service(sample_service.id)[1].name == 'Sample Template 2' - assert dao_get_all_templates_for_service(sample_service.id)[2].name == 'Sample Template 1' + assert dao_get_all_templates_for_service(sample_service.id)[2].name == 'Sample Template 3' - template_2.name = 'Sample Template 2 (updated)' + template_2.name = 'AAAAA Sample Template 2' dao_update_template(template_2) - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 3' - assert dao_get_all_templates_for_service(sample_service.id)[1].name == 'Sample Template 2 (updated)' + assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'AAAAA Sample Template 2' + assert dao_get_all_templates_for_service(sample_service.id)[1].name == 'Sample Template 1' def test_get_all_returns_empty_list_if_no_templates(sample_service): diff --git a/tests/app/db.py b/tests/app/db.py index 0457b3c0a..32ccc3d7a 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -120,12 +120,13 @@ def create_service_with_defined_sms_sender( def create_template( service, template_type=SMS_TYPE, + template_name=None, subject='Template subject', content='Dear Sir/Madam, Hello. Yours Truly, The Government.', template_id=None ): data = { - 'name': '{} Template Name'.format(template_type), + 'name': template_name or '{} Template Name'.format(template_type), 'template_type': template_type, 'content': content, 'service': service, diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index da72c3dff..6d666bfa3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -301,10 +301,10 @@ def test_should_be_able_to_get_all_templates_for_a_service(client, sample_user, assert response.status_code == 200 update_json_resp = json.loads(response.get_data(as_text=True)) - assert update_json_resp['data'][0]['name'] == 'my template 2' + assert update_json_resp['data'][0]['name'] == 'my template 1' assert update_json_resp['data'][0]['version'] == 1 assert update_json_resp['data'][0]['created_at'] - assert update_json_resp['data'][1]['name'] == 'my template 1' + assert update_json_resp['data'][1]['name'] == 'my template 2' assert update_json_resp['data'][1]['version'] == 1 assert update_json_resp['data'][1]['created_at'] diff --git a/tests/app/v2/templates/test_get_templates.py b/tests/app/v2/templates/test_get_templates.py index 6bc4aabde..1a0f1eacc 100644 --- a/tests/app/v2/templates/test_get_templates.py +++ b/tests/app/v2/templates/test_get_templates.py @@ -1,6 +1,7 @@ import pytest from flask import json +from itertools import product from app.models import TEMPLATE_TYPES, EMAIL_TYPE from tests import create_authorization_header @@ -8,12 +9,15 @@ from tests.app.db import create_template def test_get_all_templates_returns_200(client, sample_service): - num_templates = 3 - templates = [] - for i in range(num_templates): - for tmp_type in TEMPLATE_TYPES: - subject = 'subject_{}'.format(i) if tmp_type == EMAIL_TYPE else '' - templates.append(create_template(sample_service, template_type=tmp_type, subject=subject)) + templates = [ + create_template( + sample_service, + template_type=tmp_type, + subject='subject_{}'.format(name) if tmp_type == EMAIL_TYPE else '', + template_name=name, + ) + for name, tmp_type in product(('A', 'B', 'C'), TEMPLATE_TYPES) + ] auth_header = create_authorization_header(service_id=sample_service.id) @@ -25,25 +29,27 @@ def test_get_all_templates_returns_200(client, sample_service): json_response = json.loads(response.get_data(as_text=True)) - assert len(json_response['templates']) == num_templates * len(TEMPLATE_TYPES) + assert len(json_response['templates']) == len(templates) - # need to reverse index as get all templates returns list sorted by descending date - for i in range(len(json_response['templates'])): - reverse_index = len(json_response['templates']) - 1 - i - assert json_response['templates'][reverse_index]['id'] == str(templates[i].id) - assert json_response['templates'][reverse_index]['body'] == templates[i].content - assert json_response['templates'][reverse_index]['type'] == templates[i].template_type - if templates[i].template_type == EMAIL_TYPE: - assert json_response['templates'][reverse_index]['subject'] == templates[i].subject + for index, template in enumerate(json_response['templates']): + assert template['id'] == str(templates[index].id) + assert template['body'] == templates[index].content + assert template['type'] == templates[index].template_type + if templates[index].template_type == EMAIL_TYPE: + assert template['subject'] == templates[index].subject @pytest.mark.parametrize("tmp_type", TEMPLATE_TYPES) def test_get_all_templates_for_valid_type_returns_200(client, sample_service, tmp_type): - num_templates = 3 - templates = [] - for i in range(num_templates): - subject = 'subject_{}'.format(i) if tmp_type == EMAIL_TYPE else '' - templates.append(create_template(sample_service, template_type=tmp_type, subject=subject)) + templates = [ + create_template( + sample_service, + template_type=tmp_type, + template_name='Template {}'.format(i), + subject='subject_{}'.format(i) if tmp_type == EMAIL_TYPE else '' + ) + for i in range(3) + ] auth_header = create_authorization_header(service_id=sample_service.id) @@ -55,16 +61,14 @@ def test_get_all_templates_for_valid_type_returns_200(client, sample_service, tm json_response = json.loads(response.get_data(as_text=True)) - assert len(json_response['templates']) == num_templates + assert len(json_response['templates']) == len(templates) - # need to reverse index as get all templates returns list sorted by descending date - for i in range(len(json_response['templates'])): - reverse_index = len(json_response['templates']) - 1 - i - assert json_response['templates'][reverse_index]['id'] == str(templates[i].id) - assert json_response['templates'][reverse_index]['body'] == templates[i].content - assert json_response['templates'][reverse_index]['type'] == tmp_type - if templates[i].template_type == EMAIL_TYPE: - assert json_response['templates'][reverse_index]['subject'] == templates[i].subject + for index, template in enumerate(json_response['templates']): + assert template['id'] == str(templates[index].id) + assert template['body'] == templates[index].content + assert template['type'] == tmp_type + if templates[index].template_type == EMAIL_TYPE: + assert template['subject'] == templates[index].subject @pytest.mark.parametrize("tmp_type", TEMPLATE_TYPES)