From 85aeebcdd51f247a3eff7272f1b30d060150dc75 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 8 Sep 2021 13:42:54 +0100 Subject: [PATCH] Support broadcasts with unidentifiable areas The original code to raise the exception was flawed: if a broadcast only had a single area that was invalid, we would assume it was a custom broadcast [1]. Since the recent changes [2] fixed the flaw we're now getting exceptions for broadcasts of this kind. It's not practical to go and manually fix the invalid broadcasts, and the likelihood is there will be more in future as the set of areas we support changes. This takes a pragmatic approach of simply logging the issue and pretending such broadcasts are custom. [1]: https://github.com/alphagov/notifications-admin/commit/926ada2f218c59b083e99dcf087ae58c1b188d16 [2]: https://github.com/alphagov/notifications-admin/pull/4014/files#diff-2dd8f77d6df281e7674b20263cdf27a3d58b839dc5930c0087ac8b9749b313e4R92 --- app/models/broadcast_message.py | 18 ++++++++++---- tests/app/models/test_broadcast_message.py | 29 ++++++++++++++-------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index b7e6c218e..c5d334e4c 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -1,6 +1,7 @@ import itertools from datetime import datetime, timedelta +from flask import current_app from notifications_utils.polygons import Polygons from notifications_utils.template import BroadcastPreviewTemplate from orderedset import OrderedSet @@ -92,12 +93,19 @@ class BroadcastMessage(JSONModel): if 'ids' in self._dict['areas']: library_areas = self.get_areas(self.area_ids) - if len(library_areas) != len(self.area_ids): - raise RuntimeError( - f'BroadcastMessage has {len(self.area_ids)} areas ' - f'but {len(library_areas)} found in the library' + if len(library_areas) == len(self.area_ids): + return library_areas + else: + # it's possible an old broadcast may refer to areas that + # are no longer part of our area libraries; in this case + # we should just treat the whole thing as a custom broadcast, + # which isn't great as our code doesn't support editing its + # areas, but we don't expect this to happen often + current_app.logger.warn( + f'BroadcastMessage has {len(self._dict["areas"])} areas ' + f'but {len(library_areas)} found in the library. Treating ' + f'{self.id} as a custom broadcast.' ) - return library_areas polygons = self._dict['areas'].get('simple_polygons', []) diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 4e2705b90..8dcce9711 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -1,5 +1,6 @@ import pytest +from app.broadcast_areas.models import CustomBroadcastAreas from app.models.broadcast_message import BroadcastMessage from tests import broadcast_message_json @@ -70,17 +71,23 @@ def test_areas( assert len(list(broadcast_message.areas)) == expected_length -def test_areas_raises_for_missing_areas(): +def test_areas_treats_missing_ids_as_custom_broadcast(notify_admin): broadcast_message = BroadcastMessage(broadcast_message_json( - area_ids=[ - 'wd20-E05009372', - 'something else', - ], + areas={ + 'ids': [ + 'wd20-E05009372', + 'something else', + ], + # although the IDs may no longer be usable, we can + # expect the broadcast to have names and polygons, + # which is enough to show the user something + 'names': [ + 'wd20 name', + 'something else name' + ], + 'simple_polygons': [[[1, 2]]] + } )) - with pytest.raises(RuntimeError) as exception: - broadcast_message.areas - - assert str(exception.value) == ( - 'BroadcastMessage has 2 areas but 1 found in the library' - ) + assert len(list(broadcast_message.areas)) == 2 + assert type(broadcast_message.areas) == CustomBroadcastAreas