From bc2ed7e66927399ab78f829e082cc296e0c8c367 Mon Sep 17 00:00:00 2001 From: Hana Snow Date: Mon, 11 Mar 2024 16:59:06 -0400 Subject: [PATCH 1/2] specify which notification failed when reporting notification error --- matchmaker/views/external_api.py | 11 ++++------- matchmaker/views/external_api_tests.py | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/matchmaker/views/external_api.py b/matchmaker/views/external_api.py index 007278a886..f2cd6320e7 100644 --- a/matchmaker/views/external_api.py +++ b/matchmaker/views/external_api.py @@ -151,7 +151,7 @@ def _generate_notification_for_incoming_match(results, incoming_query, incoming_ emails = [email.strip() for email in submission.contact_href.replace('mailto:', '').split(',')] send_emails = emails if len(emails) < 2 else [email for email in emails if email!= MME_DEFAULT_CONTACT_EMAIL] all_emails.update(send_emails) - match_results.append((result_text, send_emails)) + match_results.append((result_text, send_emails, submission.submission_id)) match_results = sorted(match_results, key=lambda result_tuple: result_tuple[0]) base_message = """Dear collaborators, @@ -180,12 +180,11 @@ def _generate_notification_for_incoming_match(results, incoming_query, incoming_ We sent this email alert to: {email_addresses_alert_sent_to}\n{footer}.""" safe_post_to_slack(MME_SLACK_MATCH_NOTIFICATION_CHANNEL, message_template.format( - base_message=base_message, match_results='\n'.join([text for text, _ in match_results]), + base_message=base_message, match_results='\n'.join([result[0] for result in match_results]), email_addresses_alert_sent_to=', '.join(sorted(all_emails)), footer=MME_EMAIL_FOOTER )) - email_errors = [] - for result_text, emails in match_results: + for result_text, emails, submission_id in match_results: try: email_message = EmailMessage( subject='Received new MME match', @@ -198,9 +197,7 @@ def _generate_notification_for_incoming_match(results, incoming_query, incoming_ ) email_message.send() except Exception as e: - email_errors.append(str(e)) - if email_errors: - raise ValueError('\n'.join(email_errors)) + logger.error(f'Unable to send notification email for incoming MME match with {submission_id}: {e}') MME_EMAIL_FOOTER = """ diff --git a/matchmaker/views/external_api_tests.py b/matchmaker/views/external_api_tests.py index 7ec0e164ad..0956b5ee8d 100644 --- a/matchmaker/views/external_api_tests.py +++ b/matchmaker/views/external_api_tests.py @@ -284,8 +284,8 @@ def test_mme_match_proxy(self, mock_post_to_slack, mock_email, mock_logger): mock.call().send(), ]) - mock_logger.error.assert_called_with( - 'Unable to create notification for incoming MME match request for 3 matches (NA19675_1_01, P0004515, P0004517): Email error') + mock_logger.error.assert_called_once_with( + 'Unable to send notification email for incoming MME match with NA19675_1_01: Email error') # Test receive same request again mock_post_to_slack.reset_mock() From 4d15bacb3f3cc5992b5ba8fad965ade4a8dc888b Mon Sep 17 00:00:00 2001 From: Hana Snow Date: Tue, 12 Mar 2024 16:19:17 -0400 Subject: [PATCH 2/2] remove unused error hndler --- matchmaker/views/external_api.py | 11 ++--------- matchmaker/views/external_api_tests.py | 13 +++++++------ 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/matchmaker/views/external_api.py b/matchmaker/views/external_api.py index f2cd6320e7..629ca4ba88 100644 --- a/matchmaker/views/external_api.py +++ b/matchmaker/views/external_api.py @@ -79,14 +79,7 @@ def mme_match_proxy(request, originating_node_name): patient_data=query_patient_data, origin_request_host=originating_node_name, ) - try: - _generate_notification_for_incoming_match(results, incoming_query, originating_node_name, query_patient_data) - except Exception as e: - logger.error('Unable to create notification for incoming MME match request for {} matches{}: {}'.format( - len(results), - ' ({})'.format(', '.join(sorted([result.get('patient', {}).get('id') for result in results]))) if results else '', - str(e)), - ) + _safe_generate_notification_for_incoming_match(results, incoming_query, originating_node_name, query_patient_data) return create_json_response({ 'results': sorted(results, key=lambda result: result['score']['patient'], reverse=True), @@ -94,7 +87,7 @@ def mme_match_proxy(request, originating_node_name): }) -def _generate_notification_for_incoming_match(results, incoming_query, incoming_request_node, incoming_patient): +def _safe_generate_notification_for_incoming_match(results, incoming_query, incoming_request_node, incoming_patient): """ Generate a SLACK notifcation to say that a VALID match request came in and the following results were sent back. If Slack is not supported, a message is not sent, but details persisted. diff --git a/matchmaker/views/external_api_tests.py b/matchmaker/views/external_api_tests.py index 0956b5ee8d..491cb4458f 100644 --- a/matchmaker/views/external_api_tests.py +++ b/matchmaker/views/external_api_tests.py @@ -399,9 +399,9 @@ def test_mme_match_proxy_phenotype_only(self, mock_post_to_slack, mock_email): mock.call().send(), ]) - @mock.patch('matchmaker.views.external_api.logger') + @mock.patch('seqr.utils.communication_utils.logger') @mock.patch('matchmaker.views.external_api.EmailMessage') - @mock.patch('matchmaker.views.external_api.safe_post_to_slack') + @mock.patch('seqr.utils.communication_utils._post_to_slack') def test_mme_match_proxy_no_results(self, mock_post_to_slack, mock_email, mock_logger): url = '/api/matchmaker/v1/match' request_body = { @@ -424,11 +424,12 @@ def test_mme_match_proxy_no_results(self, mock_post_to_slack, mock_email, mock_l self.assertEqual(incoming_query_q.count(), 1) self.assertIsNone(incoming_query_q.first().patient_id) - mock_post_to_slack.assert_called_with( - 'matchmaker_matches', - """A match request for 12345 came in from Test Institute today. + slack_message = """A match request for 12345 came in from Test Institute today. The contact information given was: test@test.com. We didn't find any individuals in matchbox that matched that query well, *so no results were sent back*.""" + mock_post_to_slack.assert_called_with( + 'matchmaker_matches', + slack_message ) mock_email.assert_not_called() @@ -440,6 +441,6 @@ def test_mme_match_proxy_no_results(self, mock_post_to_slack, mock_email, mock_l self.assertListEqual(response.json()['results'], []) self.assertEqual(MatchmakerIncomingQuery.objects.filter(institution='Test Institute').count(), 2) mock_logger.error.assert_called_with( - 'Unable to create notification for incoming MME match request for 0 matches: Slack connection error') + f'Slack error: Slack connection error: Original message in channel (matchmaker_matches) - {slack_message}')