notifications: do not send email in case of error

* Avoids to send an incomplete email in case of error during the notification generation. * Fixes resend email every 15 minutes. * Fixes bad logger messages. * Adds traceback to the notification error logs. * Fixes the use of invenio cache for the monitoring. Co-Authored-by: Johnny Mariéthoz <[email protected]> Co-Authored-by: Peter Weber <[email protected]> Co-Authored-by: Renaud Michotte <[email protected]>
rero · Aug 30, 2021 · b10e0cf · b10e0cf
1 parent f08b4da
commit b10e0cf
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 15 deletions.
diff --git a/rero_ils/modules/monitoring.py b/rero_ils/modules/monitoring.py
@@ -25,7 +25,7 @@
 from flask import Blueprint, current_app, jsonify, request, url_for
 from flask.cli import with_appcontext
 from flask_login import current_user
-from invenio_cache import current_cache
+from invenio_cache.proxies import current_cache
 from invenio_db import db
 from invenio_search import RecordsSearch, current_search_client
 from redis import Redis

diff --git a/rero_ils/modules/notifications/dispatcher.py b/rero_ils/modules/notifications/dispatcher.py
@@ -47,39 +47,39 @@ def dispatch_notifications(cls, notification_pids=None, resend=False,
         :param verbose: Verbose output.
         :returns: dictionary with processed and send count
         """
-
         def get_dispatcher_function(channel):
             try:
                 communication_switcher = current_app.config.get(
                     'RERO_ILS_COMMUNICATION_DISPATCHER_FUNCTIONS', [])
                 return communication_switcher[channel]
             except KeyError:
                 current_app.logger.warning(
-                    'The communication channel of the patron'
-                    f' (pid: {patron["pid"]}) is not yet implemented')
+                    f'The communication channel: {channel}'
+                    ' is not yet implemented')
                 return Dispatcher.not_yet_implemented
 
         sent = not_sent = 0
         aggregated = {}
         pids = notification_pids if notification_pids else []
         notifications = [Notification.get_record_by_pid(pid) for pid in pids]
         notifications = list(filter(None, notifications))
-
         # BUILD AGGREGATED NOTIFICATIONS
         #   Notifications should be aggregated on (in this order):
         #     1. notification_type,
         #     2. communication_channel
         #     3. library
         #     4. patron
+        errors = 0
         for notification in notifications:
             try:
                 cls._process_notification(
                     notification, resend, aggregated)
             except Exception as error:
+                errors += 1
                 current_app.logger.error(
                     f'Notification has not be sent (pid: {notification.pid},'
                     f' type: {notification["notification_type"]}): '
-                    f'{error}')
+                    f'{error}', exc_info=True, stack_info=True)
 
         # SEND AGGREGATED NOTIFICATIONS
         for notification_type, notification_values in aggregated.items():
@@ -105,7 +105,8 @@ def get_dispatcher_function(channel):
         return {
             'processed': len(notifications),
             'sent': sent,
-            'not_sent': not_sent
+            'not_sent': not_sent,
+            'errors': errors
         }
 
     @classmethod
@@ -202,11 +203,6 @@ def _process_notification(cls, notification, resend, aggregated):
         p_pid = patron['pid']
         c_channel = communication_channel
 
-        aggregated.setdefault(n_type, {})
-        aggregated[n_type].setdefault(c_channel, {})
-        aggregated[n_type][c_channel].setdefault(l_pid, {})
-        aggregated[n_type][c_channel][l_pid].setdefault(p_pid, ctx_data)
-
         documents_data = {
             'title_text': loan['document']['title_text'],
             'responsibility_statement':
@@ -262,6 +258,13 @@ def _process_notification(cls, notification, resend, aggregated):
                 if email:
                     ctx_data['location_email'] = email
 
+        # Needs to be at the end of the function to avoid to send notification
+        # in case of error
+        aggregated.setdefault(n_type, {})
+        aggregated[n_type].setdefault(c_channel, {})
+        aggregated[n_type][c_channel].setdefault(l_pid, {})
+        aggregated[n_type][c_channel][l_pid].setdefault(p_pid, ctx_data)
+
         # Add information into correct aggregations
         aggregation = aggregated[n_type][c_channel][l_pid][p_pid]
         aggregation['documents'].append(documents_data)

diff --git a/tests/api/notifications/test_notifications_rest.py b/tests/api/notifications/test_notifications_rest.py
@@ -782,8 +782,7 @@ def test_request_notifications(client, patron_martigny, patron_sion,
 
 
 @mock.patch(
-    'rero_ils.modules.notifications.dispatcher.'
-    'Dispatcher._process_notification',
+    'rero_ils.modules.notifications.dispatcher.num2words',
     mock.MagicMock(side_effect=Exception('Test!')))
 def test_dispatch_error(client, patron_martigny, patron_sion,
                         lib_martigny,
@@ -810,7 +809,7 @@ def test_dispatch_error(client, patron_martigny, patron_sion,
 
     request_loan_pid = data.get(
         'action_applied')[LoanAction.REQUEST].get('pid')
-
+    # check that the email has not been sent
     flush_index(NotificationsSearch.Meta.index)
     assert len(mailbox) == 0
     # cancel request