From f50ef84c0d8a607c88ab17e7a0abf870df594bd5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 21 Sep 2020 18:55:46 +0100 Subject: [PATCH] Suggest previously-used areas when adding new area MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’re adding another area to your broadcast it’s likely to be close to one of the areas you’ve already added. But we make you start by choosing a library, then you have to find the local authority again from the long list. This is clunky, and it interrupts the task the user is trying to complete. We thought about redirecting you somewhere deep into the hierarchy, perhaps by sending you to either: - the parent of the last area you’d chosen - the common ancestor of all the areas you’d chosen This approach would however mean you’d need a way to navigate back up the hierarchy if we’d dropped you in the wrong place. And we don’t have a pattern for that at the moment. So instead this commit adds some ‘shortcuts’ to the chose library page, giving you a choice of all the parents of the areas you’ve currently selected. In most cases this will be one (unitary authority) or two (county and district) choices, but it will scale to adding areas from multiple different authorities. It does mean an extra click compared to the redirect approach, but this is still fewer, easier clicks compared to now. This meant a couple of under-the-hood changes: - making `BroadcastArea`s hashable so it’s possible to do `set([BroadcastArea(…), BroadcastArea(…), BroadcastArea(…)])` - making `BroadcastArea`s aware of which library they live in, so we can link to the correct _Choose area_ page --- app/broadcast_areas/__init__.py | 31 ++++++- app/broadcast_areas/repo.py | 32 +++++-- app/main/views/broadcast.py | 5 +- app/models/broadcast_message.py | 10 ++ app/templates/views/broadcast/libraries.html | 11 ++- tests/app/main/views/test_broadcast.py | 98 ++++++++++++++++++-- 6 files changed, 167 insertions(+), 20 deletions(-) diff --git a/app/broadcast_areas/__init__.py b/app/broadcast_areas/__init__.py index b24e7ba66..5c10f6228 100644 --- a/app/broadcast_areas/__init__.py +++ b/app/broadcast_areas/__init__.py @@ -17,6 +17,12 @@ class SortableMixin: # method are sortable return self.name < other.name + def __eq__(self, other): + return self.id == other.id + + def __hash__(self): + return hash(self.id) + class GetItemByIdMixin: def get(self, id): @@ -29,10 +35,7 @@ class GetItemByIdMixin: class BroadcastArea(SortableMixin): def __init__(self, row): - self.id, self.name, self._count_of_phones = row - - def __eq__(self, other): - return self.id == other.id + self.id, self.name, self._count_of_phones, self.library_id = row @cached_property def polygons(self): @@ -65,6 +68,26 @@ class BroadcastArea(SortableMixin): # https://www.pivotaltracker.com/story/show/174837293 return self._count_of_phones or 0 + @cached_property + def parents(self): + return list(filter(None, self._parents_iterator)) + + @property + def _parents_iterator(self): + id = self.id + + while True: + parent = BroadcastAreasRepository().get_parent_for_area(id) + + if not parent: + return None + + parent_broadcast_area = BroadcastArea(parent) + + yield parent_broadcast_area + + id = parent_broadcast_area.id + class BroadcastAreaLibrary(SerialisedModelCollection, SortableMixin, GetItemByIdMixin): diff --git a/app/broadcast_areas/repo.py b/app/broadcast_areas/repo.py index fa8491097..d6f4ebcf7 100644 --- a/app/broadcast_areas/repo.py +++ b/app/broadcast_areas/repo.py @@ -123,7 +123,7 @@ class BroadcastAreasRepository(object): def get_areas(self, area_ids): q = """ - SELECT id, name, count_of_phones + SELECT id, name, count_of_phones, broadcast_area_library_id FROM broadcast_areas WHERE id IN ({}) """.format(("?," * len(area_ids))[:-1]) @@ -131,7 +131,7 @@ class BroadcastAreasRepository(object): results = self.query(q, *area_ids) areas = [ - (row[0], row[1], row[2]) + (row[0], row[1], row[2], row[3]) for row in results ] @@ -150,7 +150,7 @@ class BroadcastAreasRepository(object): if is_multi_tier_library: # only interested in areas with children - eg local authorities, counties, unitary authorities. not wards. q = """ - SELECT id, name, count_of_phones + SELECT id, name, count_of_phones, broadcast_area_library_id FROM broadcast_areas JOIN ( SELECT DISTINCT broadcast_area_library_group_id @@ -162,7 +162,7 @@ class BroadcastAreasRepository(object): else: # Countries don't have any children, so the above query wouldn't return anything. q = """ - SELECT id, name, count_of_phones + SELECT id, name, count_of_phones, broadcast_area_library_id FROM broadcast_areas WHERE broadcast_area_library_id = ? """ @@ -170,13 +170,13 @@ class BroadcastAreasRepository(object): results = self.query(q, library_id) return [ - (row[0], row[1], row[2]) + (row[0], row[1], row[2], row[3]) for row in results ] def get_all_areas_for_group(self, group_id): q = """ - SELECT id, name, count_of_phones + SELECT id, name, count_of_phones, broadcast_area_library_id FROM broadcast_areas WHERE broadcast_area_library_group_id = ? """ @@ -184,12 +184,30 @@ class BroadcastAreasRepository(object): results = self.query(q, group_id) areas = [ - (row[0], row[1], row[2]) + (row[0], row[1], row[2], row[3]) for row in results ] return areas + def get_parent_for_area(self, area_id): + q = """ + SELECT id, name, count_of_phones, broadcast_area_library_id + FROM broadcast_areas + WHERE id IN ( + SELECT broadcast_area_library_group_id + FROM broadcast_areas + WHERE id = ? + ) + """ + + results = self.query(q, area_id) + + if not results: + return None + + return (results[0][0], results[0][1], results[0][2], results[0][3]) + def get_polygons_for_area(self, area_id): q = """ SELECT polygons diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index 16b820f65..ffcebde2b 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -102,7 +102,10 @@ def choose_broadcast_library(service_id, broadcast_message_id): return render_template( 'views/broadcast/libraries.html', libraries=BroadcastMessage.libraries, - broadcast_message_id=broadcast_message_id, + broadcast_message=BroadcastMessage.from_id( + broadcast_message_id, + service_id=current_service.id, + ), ) diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index f5f88ec66..87b431b37 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -65,6 +65,16 @@ class BroadcastMessage(JSONModel): def areas(self): return self.get_areas(areas=self._dict['areas']) + @property + def parent_areas(self): + return sorted(set(self._parent_areas_iterator)) + + @property + def _parent_areas_iterator(self): + for area in self.areas: + for parent in area.parents: + yield parent + @property def initial_area_names(self): return [ diff --git a/app/templates/views/broadcast/libraries.html b/app/templates/views/broadcast/libraries.html index e5597d511..5b5981531 100644 --- a/app/templates/views/broadcast/libraries.html +++ b/app/templates/views/broadcast/libraries.html @@ -10,11 +10,18 @@ {{ page_header( "Choose where to broadcast to", - back_link=url_for(".preview_broadcast_areas", service_id=current_service.id, broadcast_message_id=broadcast_message_id) + back_link=url_for(".preview_broadcast_areas", service_id=current_service.id, broadcast_message_id=broadcast_message.id) ) }} + {% for area in broadcast_message.parent_areas %} + {{ area.name }} + {% if loop.last %} +
+ {% endif %} + {% endfor %} + {% for library in libraries|sort %} - {{ library.name }} + {{ library.name }}

{{ library.get_examples() }}

{% endfor %} diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 0ee2a41ed..9ba3a1707 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -356,13 +356,65 @@ def test_preview_broadcast_areas_page( ] +@pytest.mark.parametrize('areas, expected_list', ( + ([], [ + 'Countries', + 'Local authorities', + ]), + ([ + # Countries have no parent areas + 'ctry19-E92000001', + 'ctry19-S92000003', + ], [ + 'Countries', + 'Local authorities', + ]), + ([ + # If you’ve chosen the whole of a county or unitary authority + # there’s no reason to also pick districts of it + 'ctyua19-E10000013', # Gloucestershire, a county + 'lad20-E06000052', # Cornwall, a unitary authority + ], [ + 'Countries', + 'Local authorities', + ]), + ([ + 'wd20-E05004299', # Pitville, in Cheltenham, in Gloucestershire + 'wd20-E05004290', # Benhall and the Reddings, in Cheltenham, in Gloucestershire + 'wd20-E05010951', # Abbeymead, in Gloucester, in Gloucestershire + 'wd20-S13002775', # Shetland Central, in Shetland Isles + 'lad20-E07000037', # High Peak, a district in Derbyshire + ], [ + 'Cheltenham', + 'Derbyshire', + 'Gloucester', + 'Gloucestershire', + 'Shetland Islands', + # --- + 'Countries', + 'Local authorities', + ]), +)) def test_choose_broadcast_library_page( + mocker, client_request, service_one, - mock_get_draft_broadcast_message, fake_uuid, + areas, + expected_list, ): service_one['permissions'] += ['broadcast'] + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + template_id=fake_uuid, + created_by_id=fake_uuid, + service_id=SERVICE_ONE_ID, + status='draft', + areas=areas, + ), + ) page = client_request.get( '.choose_broadcast_library', service_id=SERVICE_ONE_ID, @@ -371,11 +423,8 @@ def test_choose_broadcast_library_page( assert [ normalize_spaces(title.text) - for title in page.select('.file-list-filename-large') - ] == [ - 'Countries', - 'Local authorities', - ] + for title in page.select('main a.govuk-link') + ] == expected_list assert normalize_spaces(page.select('.file-list-hint-large')[0].text) == ( 'England, Northern Ireland, Scotland and Wales' @@ -389,6 +438,43 @@ def test_choose_broadcast_library_page( ) +def test_suggested_area_has_correct_link( + mocker, + client_request, + service_one, + fake_uuid, +): + service_one['permissions'] += ['broadcast'] + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + template_id=fake_uuid, + created_by_id=fake_uuid, + service_id=SERVICE_ONE_ID, + status='draft', + areas=[ + 'wd20-E05004299', # Pitville, a ward of Cheltenham + ], + ), + ) + page = client_request.get( + '.choose_broadcast_library', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + ) + link = page.select_one('main a.govuk-link') + + assert link.text == 'Cheltenham' + assert link['href'] == url_for( + 'main.choose_broadcast_sub_area', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + library_slug='wd20-lad20-ctyua19', + area_slug='lad20-E07000078', + ) + + def test_choose_broadcast_area_page( client_request, service_one,