Merge pull request #3961 from broadinstitute/clearer-mme-notification…

…-error

specify which notification failed when reporting notification error

matchmaker/views/external_api.py

@@ -79,22 +79,15 @@ 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),
         '_disclaimer': MME_DISCLAIMER,
     })
 
 
-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.
@@ -151,7 +144,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 +173,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 +190,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 = """

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()
@@ -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: [email protected].
         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}')