From 53fd5b886938fda9ee4377122e2a4af5f44f70b2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Aug 2020 12:02:56 +0100 Subject: [PATCH 1/3] Exclude grouped areas from examples The given examples should match the choices offered when you visit the next page. The choices offered on the next page are either the areas (when a library is not grouped) or the groups (when a library is grouped). This commit makes the examples match the choices by excluding sub-areas, ie those that have a grouping ID. --- app/broadcast_areas/repo.py | 7 +++++-- tests/app/broadcast_areas/test_broadcast_area.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/broadcast_areas/repo.py b/app/broadcast_areas/repo.py index dca7c2188..171e544ba 100644 --- a/app/broadcast_areas/repo.py +++ b/app/broadcast_areas/repo.py @@ -114,8 +114,11 @@ class BroadcastAreasRepository(object): def get_library_description(self, library_id): q = """ WITH - areas AS (SELECT * FROM broadcast_areas - WHERE broadcast_area_library_id = ?), + areas AS ( + SELECT * FROM broadcast_areas + WHERE broadcast_area_library_id = ? + AND broadcast_area_library_group_id IS NULL + ), area_count AS (SELECT COUNT(*) AS c FROM areas), subset_area_count AS (SELECT c - 4 FROM area_count), description_area_names AS (SELECT name FROM areas LIMIT 4), diff --git a/tests/app/broadcast_areas/test_broadcast_area.py b/tests/app/broadcast_areas/test_broadcast_area.py index 4e8459de6..66226ea7d 100644 --- a/tests/app/broadcast_areas/test_broadcast_area.py +++ b/tests/app/broadcast_areas/test_broadcast_area.py @@ -61,7 +61,7 @@ def test_examples(): wards = broadcast_area_libraries.get( 'electoral-wards-of-the-united-kingdom', ).get_examples() - assert wards == 'Abbey, Alibon, Becontree, Chadwell Heath, and 8970 more…' + assert wards == 'Hartlepool, Middlesbrough, Redcar and Cleveland, Stockton-on-Tees, and 375 more…' @pytest.mark.parametrize('id', ( From dfa2ae0ced2663dc88a8da2fe2d400c9538c1042 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Aug 2020 12:03:09 +0100 Subject: [PATCH 2/3] Order areas by name in examples When you click through to the page for a library you see the available areas in alphabetical order. The examples given for each library should match this. --- app/broadcast_areas/repo.py | 2 +- tests/app/broadcast_areas/test_broadcast_area.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/broadcast_areas/repo.py b/app/broadcast_areas/repo.py index 171e544ba..6465e58c1 100644 --- a/app/broadcast_areas/repo.py +++ b/app/broadcast_areas/repo.py @@ -121,7 +121,7 @@ class BroadcastAreasRepository(object): ), area_count AS (SELECT COUNT(*) AS c FROM areas), subset_area_count AS (SELECT c - 4 FROM area_count), - description_area_names AS (SELECT name FROM areas LIMIT 4), + description_area_names AS (SELECT name FROM areas ORDER BY name ASC LIMIT 4), description_areas_joined AS ( SELECT GROUP_CONCAT(name, ", ") FROM description_area_names ) diff --git a/tests/app/broadcast_areas/test_broadcast_area.py b/tests/app/broadcast_areas/test_broadcast_area.py index 66226ea7d..9417442c5 100644 --- a/tests/app/broadcast_areas/test_broadcast_area.py +++ b/tests/app/broadcast_areas/test_broadcast_area.py @@ -51,17 +51,17 @@ def test_examples(): assert countries == 'England, Northern Ireland, Scotland, Wales' regions = broadcast_area_libraries.get('regions-of-england').get_examples() - assert regions == 'North East, North West, Yorkshire and The Humber, East Midlands, and 5 more…' + assert regions == 'East Midlands, East of England, London, North East, and 5 more…' counties = broadcast_area_libraries.get( 'counties-and-unitary-authorities-in-england-and-wales', ).get_examples() - assert counties == 'Hartlepool, Middlesbrough, Redcar and Cleveland, Stockton-on-Tees, and 170 more…' + assert counties == 'Barking and Dagenham, Barnet, Barnsley, Bath and North East Somerset, and 170 more…' wards = broadcast_area_libraries.get( 'electoral-wards-of-the-united-kingdom', ).get_examples() - assert wards == 'Hartlepool, Middlesbrough, Redcar and Cleveland, Stockton-on-Tees, and 375 more…' + assert wards == 'Aberdeen City, Aberdeenshire, Adur, Allerdale, and 375 more…' @pytest.mark.parametrize('id', ( From 424207888599451b1fa403de73929f0570450702 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Aug 2020 12:17:40 +0100 Subject: [PATCH 3/3] Always show 4 items in the example list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a library has lots of items then the first 3 should be shown, with a count of how many more there are, for a total of 4 list items: > a, b, c, and 23 more If the library only has 4 items then all 4 should be shown, with consistent use of conjunction and Oxford comma[1]: > a, b, c, and d This keeps the lengths of the examples nice and consistent. 1. We use an Oxford comma because it helps disambiguate when an area itself has a comma or ‘and’ in it, for example ‘Armagh City, Banbridge and Craigavon’ --- app/broadcast_areas/repo.py | 4 +++- tests/app/broadcast_areas/test_broadcast_area.py | 8 ++++---- tests/app/main/views/test_broadcast.py | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/broadcast_areas/repo.py b/app/broadcast_areas/repo.py index 6465e58c1..668ea3981 100644 --- a/app/broadcast_areas/repo.py +++ b/app/broadcast_areas/repo.py @@ -121,7 +121,7 @@ class BroadcastAreasRepository(object): ), area_count AS (SELECT COUNT(*) AS c FROM areas), subset_area_count AS (SELECT c - 4 FROM area_count), - description_area_names AS (SELECT name FROM areas ORDER BY name ASC LIMIT 4), + description_area_names AS (SELECT name FROM areas ORDER BY name ASC LIMIT 3), description_areas_joined AS ( SELECT GROUP_CONCAT(name, ", ") FROM description_area_names ) @@ -129,6 +129,8 @@ class BroadcastAreasRepository(object): CASE (SELECT * FROM subset_area_count) WHEN 0 THEN (SELECT * FROM description_areas_joined) + || ", and " + || (SELECT name from areas ORDER BY name DESC limit 1) ELSE (SELECT * FROM description_areas_joined) || ", and " diff --git a/tests/app/broadcast_areas/test_broadcast_area.py b/tests/app/broadcast_areas/test_broadcast_area.py index 9417442c5..26a6fb2c2 100644 --- a/tests/app/broadcast_areas/test_broadcast_area.py +++ b/tests/app/broadcast_areas/test_broadcast_area.py @@ -48,20 +48,20 @@ def test_loads_areas_from_library(): def test_examples(): countries = broadcast_area_libraries.get('countries').get_examples() - assert countries == 'England, Northern Ireland, Scotland, Wales' + assert countries == 'England, Northern Ireland, Scotland, and Wales' regions = broadcast_area_libraries.get('regions-of-england').get_examples() - assert regions == 'East Midlands, East of England, London, North East, and 5 more…' + assert regions == 'East Midlands, East of England, London, and 5 more…' counties = broadcast_area_libraries.get( 'counties-and-unitary-authorities-in-england-and-wales', ).get_examples() - assert counties == 'Barking and Dagenham, Barnet, Barnsley, Bath and North East Somerset, and 170 more…' + assert counties == 'Barking and Dagenham, Barnet, Barnsley, and 170 more…' wards = broadcast_area_libraries.get( 'electoral-wards-of-the-united-kingdom', ).get_examples() - assert wards == 'Aberdeen City, Aberdeenshire, Adur, Allerdale, and 375 more…' + assert wards == 'Aberdeen City, Aberdeenshire, Adur, and 375 more…' @pytest.mark.parametrize('id', ( diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 2fd779585..bd0ec1285 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -299,7 +299,7 @@ def test_choose_broadcast_library_page( ]) assert normalize_spaces(page.select('.file-list-hint-large')[1].text) == ( - 'England, Northern Ireland, Scotland, Wales' + 'England, Northern Ireland, Scotland, and Wales' ) assert page.select_one('a.file-list-filename-large.govuk-link')['href'] == url_for(