From 1923c5edb1cb859c3376dfb293ec1bf70b1c8871 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 23 Aug 2021 16:35:38 +0100 Subject: [PATCH 1/4] Remove redundant 'filter' and return value 'None' is the implicit return value. Since the filter was operating on a yield that never yield 'None', it was redundant. --- app/broadcast_areas/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/broadcast_areas/models.py b/app/broadcast_areas/models.py index b33ee1ee4..6e8785b61 100644 --- a/app/broadcast_areas/models.py +++ b/app/broadcast_areas/models.py @@ -125,7 +125,7 @@ class BroadcastArea(BaseBroadcastArea, SortableMixin): @cached_property def parents(self): - return list(filter(None, self._parents_iterator)) + return list(self._parents_iterator) @property def _parents_iterator(self): @@ -135,7 +135,7 @@ class BroadcastArea(BaseBroadcastArea, SortableMixin): parent = BroadcastAreasRepository().get_parent_for_area(id) if not parent: - return None + return parent_broadcast_area = BroadcastArea(parent) From d2784d0d8a823d0d9a6dcafe9a57f3e7876ad363 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 23 Aug 2021 16:48:21 +0100 Subject: [PATCH 2/4] Rename "parents" methods to "ancestors" Resolves: https://github.com/alphagov/notifications-admin/pull/3980#discussion_r694002952 A grandparent is not a parent, so the return value of these methods were misleading. This makes it clearer. --- app/broadcast_areas/models.py | 8 +++----- app/models/broadcast_message.py | 10 +++++----- app/templates/views/broadcast/libraries.html | 2 +- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/broadcast_areas/models.py b/app/broadcast_areas/models.py index 6e8785b61..105a456dc 100644 --- a/app/broadcast_areas/models.py +++ b/app/broadcast_areas/models.py @@ -124,11 +124,11 @@ class BroadcastArea(BaseBroadcastArea, SortableMixin): return self._count_of_phones or 0 @cached_property - def parents(self): - return list(self._parents_iterator) + def ancestors(self): + return list(self._ancestors_iterator) @property - def _parents_iterator(self): + def _ancestors_iterator(self): id = self.id while True: @@ -138,9 +138,7 @@ class BroadcastArea(BaseBroadcastArea, SortableMixin): return parent_broadcast_area = BroadcastArea(parent) - yield parent_broadcast_area - id = parent_broadcast_area.id diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index 14c4c9525..112e234f7 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -104,14 +104,14 @@ class BroadcastMessage(JSONModel): ) @property - def parent_areas(self): - return sorted(set(self._parent_areas_iterator)) + def ancestor_areas(self): + return sorted(set(self._ancestor_areas_iterator)) @property - def _parent_areas_iterator(self): + def _ancestor_areas_iterator(self): for area in self.areas: - for parent in area.parents: - yield parent + for ancestor in area.ancestors: + yield ancestor @cached_property def polygons(self): diff --git a/app/templates/views/broadcast/libraries.html b/app/templates/views/broadcast/libraries.html index 502d1d35f..a90b80011 100644 --- a/app/templates/views/broadcast/libraries.html +++ b/app/templates/views/broadcast/libraries.html @@ -14,7 +14,7 @@ {{ page_header("Choose where to send this alert") }} - {% for area in broadcast_message.parent_areas %} + {% for area in broadcast_message.ancestor_areas %} {{ area.name }} {% if loop.last %}
From 427ac0c8c10ab4ba68b814d43b68d98f4db19501 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 23 Aug 2021 16:50:56 +0100 Subject: [PATCH 3/4] Fix misleading name for overlap method Resolves: https://github.com/alphagov/notifications-admin/pull/3980#discussion_r692919874 Previously it was unclear what kinds of areas this method returned, and whether there would be any duplicates (due to the hierarchy of areas we work with). This clarifies that. In addition, the areas returned may not overlap with the custom one [1], so we should reword to avoid falsely implying that. We could do the overlap check as part of the method as an alternative, but that would create extra work when calculating the ratio of intersection. We could always add "overlapping areas" as a complementary method to this one in future. [1]: https://github.com/alphagov/notifications-admin/pull/3980#discussion_r692919874 --- app/broadcast_areas/models.py | 5 +++-- tests/app/broadcast_areas/test_broadcast_area.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/broadcast_areas/models.py b/app/broadcast_areas/models.py index 105a456dc..d7776c4ed 100644 --- a/app/broadcast_areas/models.py +++ b/app/broadcast_areas/models.py @@ -163,10 +163,11 @@ class CustomBroadcastArea(BaseBroadcastArea): simple_polygons = polygons @cached_property - def overlapping_areas(self): + def nearby_electoral_wards(self): if not self.polygons: return [] return broadcast_area_libraries.get_areas_with_simple_polygons([ + # We only index electoral wards in the RTree overlap.data for overlap in rtree_index.query( Rect(*self.polygons.bounds) ) @@ -176,7 +177,7 @@ class CustomBroadcastArea(BaseBroadcastArea): def count_of_phones(self): return sum( area.simple_polygons.ratio_of_intersection_with(self.polygons) * area.count_of_phones - for area in self.overlapping_areas + for area in self.nearby_electoral_wards ) diff --git a/tests/app/broadcast_areas/test_broadcast_area.py b/tests/app/broadcast_areas/test_broadcast_area.py index 4c2af384b..ef862f10d 100644 --- a/tests/app/broadcast_areas/test_broadcast_area.py +++ b/tests/app/broadcast_areas/test_broadcast_area.py @@ -386,7 +386,7 @@ def test_count_of_phones_for_custom_area( ) assert sorted( - overlap.name for overlap in area.overlapping_areas + overlap.name for overlap in area.nearby_electoral_wards ) == expected_possible_overlaps assert close_enough(area.count_of_phones, expected_count_of_phones) From 5ba8387c6de9b0e757966d595f675de24c1859db Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Jul 2021 11:33:48 +0100 Subject: [PATCH 4/4] Bump utils to 44.5.0 Brings in a new `intersects_with()` method of `Polygons` which will come in handy. --- requirements.in | 2 +- requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.in b/requirements.in index b828b23ab..ca9d5f69f 100644 --- a/requirements.in +++ b/requirements.in @@ -26,7 +26,7 @@ fido2==0.9.1 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 # pyup: <2 -git+https://github.com/alphagov/notifications-utils.git@44.4.3#egg=notifications-utils==44.4.3 +git+https://github.com/alphagov/notifications-utils.git@44.5.0#egg=notifications-utils==44.5.0 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.8-alpha#egg=govuk-frontend-jinja==0.5.8-alpha # cryptography 3.4+ incorporates Rust code, which isn't supported on PaaS diff --git a/requirements.txt b/requirements.txt index 4abcbfda2..9d37aad32 100644 --- a/requirements.txt +++ b/requirements.txt @@ -118,7 +118,7 @@ mistune==0.8.4 # via notifications-utils notifications-python-client==6.2.1 # via -r requirements.in -git+https://github.com/alphagov/notifications-utils.git@44.4.3#egg=notifications-utils==44.4.3 +git+https://github.com/alphagov/notifications-utils.git@44.5.0#egg=notifications-utils==44.5.0 # via -r requirements.in openpyxl==3.0.7 # via pyexcel-xlsx