From a30a3171532fdbbb92ac34c0d4f9466525cfea92 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 18 Feb 2022 12:38:02 +0000 Subject: [PATCH] Fix 500 error due to inconsistent recipient check This strengthens the initial check of what's in the session to make sure it contains some kind of recipient. Without this, we get: Traceback (most recent call last): File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1950, in full_dispatch_request rv = self.dispatch_request() File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1936, in dispatch_request return self.view_functions[rule.endpoint](**req.view_args) File "/home/vcap/app/app/utils/user.py", line 26, in wrap_func return func(*args, **kwargs) File "/home/vcap/app/app/main/views/send.py", line 1041, in send_notification recipient=session['recipient'] or InsensitiveDict(session['placeholders'])['address line 1'], File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_utils/insensitive_dict.py", line 41, in __getitem__ return super().__getitem__(self.make_key(key)) KeyError: 'addressline1' I'm not sure how to reproduce this, but this should at least give the user a better experience, instead of a 500 page. --- app/main/views/send.py | 18 +++++++++++++++--- tests/app/main/views/test_send.py | 8 +++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 10625d9ed..b26e70829 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -1025,7 +1025,9 @@ def get_template_error_dict(exception): @main.route("/services//template//notification/check", methods=['POST']) @user_has_permissions('send_messages', restrict_admin_usage=True) def send_notification(service_id, template_id): - if {'recipient', 'placeholders'} - set(session.keys()): + recipient = get_recipient() + + if not recipient: return redirect(url_for( '.send_one_off', service_id=service_id, @@ -1038,9 +1040,9 @@ def send_notification(service_id, template_id): noti = notification_api_client.send_notification( service_id, template_id=db_template['id'], - recipient=session['recipient'] or InsensitiveDict(session['placeholders'])['address line 1'], + recipient=recipient, personalisation=session['placeholders'], - sender_id=session['sender_id'] if 'sender_id' in session else None + sender_id=session.get('sender_id', None), ) except HTTPError as exception: current_app.logger.info('Service {} could not send notification: "{}"'.format( @@ -1096,3 +1098,13 @@ def get_spreadsheet_column_headings_from_template(template): column_headings.append(column_heading) return column_headings + + +def get_recipient(): + if {'recipient', 'placeholders'} - set(session.keys()): + return None + + return ( + session['recipient'] or + InsensitiveDict(session['placeholders']).get('address line 1') + ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 46bc0f0fe..3dda1a696 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -3969,12 +3969,18 @@ def test_send_notification_clears_session( assert 'placeholders' not in session +@pytest.mark.parametrize('session_data', [ + {'placeholders': {'a': 'b'}}, # missing recipient + {'recipient': '123'}, # missing placeholders + {'placeholders': {}, 'recipient': ''}, # missing address +]) def test_send_notification_redirects_if_missing_data( client_request, fake_uuid, + session_data, ): with client_request.session_transaction() as session: - session['placeholders'] = {'a': 'b'} + session.update(session_data) client_request.post( 'main.send_notification',