Merge pull request #11310 from Ostap-Zherebetskyi/fix/file_event_notifications

[ENG-8861] File Operations: Email content and subject not correct when file is renamed

Author: Johnetordoff

addons/base/views.py

@@ -611,48 +611,18 @@ def create_waterbutler_log(payload, **kwargs):
     target_node = AbstractNode.load(metadata.get('nid'))
     if target_node and payload['action'] != 'download_file':
         update_storage_usage_with_size(payload)
-
     with transaction.atomic():
-        file_signals.file_updated.send(target=node, user=user, payload=payload)
+        file_signals.file_updated.send(target=node, user=user, event_type=action, payload=payload)
     return {'status': 'success'}
 
 
 @file_signals.file_updated.connect
-def emit_notification(self, target, user, payload, *args, **kwargs):
-    if not isinstance(target, Preprint):
-        return
-    notification_types = {
-        'rename': NotificationType.Type.ADDON_FILE_RENAMED.instance,
-        'copy': NotificationType.Type.ADDON_FILE_COPIED.instance,
-        'create': NotificationType.Type.FILE_ADDED.instance,
-        'move': NotificationType.Type.ADDON_FILE_MOVED.instance,
-        'delete': NotificationType.Type.FILE_REMOVED.instance,
-        'update': NotificationType.Type.FILE_UPDATED.instance,
-        'create_folder': NotificationType.Type.FOLDER_CREATED.instance,
-    }
-    notification_type = notification_types[payload.get('action')]
-    if notification_type not in notification_types.values():
-        raise NotImplementedError(f'Notification type {notification_type} is not supported')
-    notification_type.emit(
-        user=user,
-        subscribed_object=target,
-        event_context={
-            'profile_image_url': user.profile_image_url(),
-            'localized_timestamp': str(timezone.now()),
-            'message': payload.get('message'),
-            'user_fullname': user.fullname,
-            'url': target.absolute_url,
-        }
-    )
-
-@file_signals.file_updated.connect
-def addon_delete_file_node(self, target, user, payload):
+def addon_delete_file_node(self, target, user, event_type, payload):
     """ Get addon BaseFileNode(s), move it into the TrashedFileNode collection
     and remove it from StoredFileNode.
     Required so that the guids of deleted addon files are not re-pointed when an
     addon file or folder is moved or renamed.
     """
-    event_type = payload['action']
     if event_type == 'file_removed' and payload.get('provider', None) != 'osfstorage':
         provider = payload['provider']
         path = payload['metadata']['path']

notifications/file_event_notifications.py

@@ -62,7 +62,8 @@ def perform(self):
                 'url': self.url,
                 'message': self.html_message,
                 'localized_timestamp': str(self.timestamp),
-            }
+            },
+            is_digest=True,
         )
 
     @property
@@ -88,20 +89,20 @@ def event_type(self):
 # -----------------------------
 
 @signal.connect
-def file_updated(self, target=None, user=None, payload=None):
+def file_updated(self, target=None, user=None, event_type=None, payload=None):
     """Signal receiver for addon file updates."""
     if isinstance(target, Preprint):
         return
 
     event = {
-        'rename': NotificationType.Type.ADDON_FILE_RENAMED,
-        'copy': NotificationType.Type.ADDON_FILE_COPIED,
-        'create': NotificationType.Type.FILE_UPDATED,
-        'move': NotificationType.Type.ADDON_FILE_MOVED,
-        'delete': NotificationType.Type.FILE_REMOVED,
-        'update': NotificationType.Type.FILE_UPDATED,
-        'create_folder': NotificationType.Type.FILE_UPDATED,
-    }[payload.get('action')]
+        NodeLog.FILE_RENAMED: NotificationType.Type.ADDON_FILE_RENAMED,
+        NodeLog.FILE_COPIED: NotificationType.Type.ADDON_FILE_COPIED,
+        NodeLog.FILE_ADDED: NotificationType.Type.FILE_ADDED,
+        NodeLog.FILE_MOVED: NotificationType.Type.ADDON_FILE_MOVED,
+        NodeLog.FILE_REMOVED: NotificationType.Type.FILE_REMOVED,
+        NodeLog.FILE_UPDATED: NotificationType.Type.FILE_UPDATED,
+        NodeLog.FOLDER_CREATED: NotificationType.Type.FOLDER_CREATED,
+    }[event_type]
 
     if event not in event_registry:
         raise NotImplementedError(f'{event} not in {event_registry}')
@@ -262,10 +263,6 @@ def source_url(self):
 @register(NodeLog.FILE_RENAMED)
 class AddonFileRenamed(ComplexFileEvent):
     def perform(self):
-        '''
-        Currently, WB sends the "move" action for renamed files.
-        This code will remain useless until the correct action is sent.
-        '''
         if self.node == self.source_node:
             super().perform()
             return

notifications/tasks.py

@@ -28,13 +28,16 @@ def send_user_email_task(self, user_id, notification_ids, message_freq):
         )
     except OSFUser.DoesNotExist:
         logger.error(f'OSFUser with id {user_id} does not exist')
-        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, status='NO_USER_FOUND')
+        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+        email_task.status = 'NO_USER_FOUND'
         email_task.error_message = 'User not found or disabled'
         email_task.save()
         return
 
     try:
-        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, user=user, status='STARTED')
+        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+        email_task.user = user
+        email_task.status = 'STARTED'
         if user.is_disabled:
             email_task.status = 'USER_DISABLED'
             email_task.error_message = 'User not found or disabled'
@@ -58,7 +61,6 @@ def send_user_email_task(self, user_id, notification_ids, message_freq):
         NotificationType.Type.USER_DIGEST.instance.emit(
             user=user,
             event_context=event_context,
-            is_digest=True
         )
 
         notifications_qs.update(sent=timezone.now())
@@ -73,11 +75,13 @@ def send_user_email_task(self, user_id, notification_ids, message_freq):
             )
         except OSFUser.DoesNotExist:
             logger.error(f'OSFUser with id {user_id} does not exist')
-            email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, status='NO_USER_FOUND')
+            email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+            email_task.status = 'NO_USER_FOUND'
             email_task.error_message = 'User not found or disabled'
             email_task.save()
             return
-        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, user=user, status='RETRY')
+        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+        email_task.user = user
         email_task.status = 'RETRY'
         email_task.error_message = str(e)
         email_task.save()
@@ -93,13 +97,16 @@ def send_moderator_email_task(self, user_id, provider_id, notification_ids, mess
         )
     except OSFUser.DoesNotExist:
         logger.error(f'OSFUser with id {user_id} does not exist')
-        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, status='NO_USER_FOUND')
+        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+        email_task.status = 'NO_USER_FOUND'
         email_task.error_message = 'User not found or disabled'
         email_task.save()
         return
 
     try:
-        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id, user=user, status='STARTED')
+        email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id)
+        email_task.user = user
+        email_task.status = 'STARTED'
         if user.is_disabled:
             email_task.status = 'USER_DISABLED'
             email_task.error_message = 'User not found or disabled'
@@ -173,7 +180,7 @@ def send_moderator_email_task(self, user_id, provider_id, notification_ids, mess
         logger.exception('Retrying send_moderator_email_task due to exception')
         raise self.retry(exc=e)
 
-@celery_app.task(bind=True, name='notifications.tasks.send_users_digest_email')
+@celery_app.task(name='notifications.tasks.send_users_digest_email')
 def send_users_digest_email(dry_run=False):
     today = date.today()
 
@@ -191,7 +198,7 @@ def send_users_digest_email(dry_run=False):
             if not dry_run:
                 send_user_email_task.delay(user_id, notification_ids, freq)
 
-@celery_app.task(bind=True, name='notifications.tasks.send_moderators_digest_email')
+@celery_app.task(name='notifications.tasks.send_moderators_digest_email')
 def send_moderators_digest_email(dry_run=False):
     today = date.today()
 
@@ -315,7 +322,7 @@ def remove_subscription_task(node_id):
     ).delete()
 
 
-@celery_app.task(bind=True, name='notifications.tasks.send_users_instant_digest_email')
+@celery_app.task(name='notifications.tasks.send_users_instant_digest_email')
 def send_users_instant_digest_email(dry_run):
     """Send pending "instant' digest emails.
     :return:
@@ -327,7 +334,7 @@ def send_users_instant_digest_email(dry_run):
         if not dry_run:
             send_user_email_task.delay(user_id, notification_ids, 'instantly')
 
-@celery_app.task(bind=True, name='notifications.tasks.send_moderators_instant_digest_email')
+@celery_app.task(name='notifications.tasks.send_moderators_instant_digest_email')
 def send_moderators_instant_digest_email(dry_run=False):
     """Send pending "instant' digest emails.
     :return:

osf/models/notification.py

@@ -70,10 +70,10 @@ def mark_seen(self) -> None:
 
     def render(self) -> str:
         """Render the notification message using the event context."""
-        template = self.subscription.notification_type.template
-        if not template:
+        notification_type = self.subscription.notification_type
+        if not notification_type:
             raise ValueError('Notification type must have a template to render the notification.')
-        notification = email.render_notification(template, self.event_context)
+        notification = email._render_email_html(notification_type, self.event_context)
         return notification
 
     def __str__(self) -> str:

tests/test_addons.py

@@ -1569,7 +1569,7 @@ def test_delete_action_creates_trashed_file_node(self):
                 'materialized': '/test/Test'
             }
         }
-        views.addon_delete_file_node(self=None, target=self.project, user=self.user, payload=payload)
+        views.addon_delete_file_node(self=None, target=self.project, user=self.user, event_type='file_removed', payload=payload)
         assert not GithubFileNode.load(file_node._id)
         assert TrashedFileNode.load(file_node._id)
 
@@ -1590,7 +1590,7 @@ def test_delete_action_for_folder_deletes_subfolders_and_creates_trashed_file_no
                 'materialized': '/test/'
             }
         }
-        views.addon_delete_file_node(self=None, target=self.project, user=self.user, payload=payload)
+        views.addon_delete_file_node(self=None, target=self.project, user=self.user, event_type='file_removed', payload=payload)
         assert not GithubFileNode.load(subfolder._id)
         assert TrashedFileNode.load(file_node._id)
 

tests/test_preprints.py

@@ -2174,8 +2174,7 @@ def test_add_log(self):
         url = self.preprint.api_url_for('create_waterbutler_log')
         payload = self.build_payload(metadata={'nid': self.preprint._id, 'materialized': path, 'kind': 'file', 'path': path})
         nlogs = self.preprint.logs.count()
-        with capture_notifications():
-            self.app.put(url, json=payload)
+        self.app.put(url, json=payload)
         self.preprint.reload()
         assert self.preprint.logs.count() == nlogs + 1
 
@@ -2244,14 +2243,11 @@ def test_action_file_rename(self):
                 'kind': 'file',
             },
         )
-        with capture_notifications() as notifications:
-            self.app.put(
-                url,
-                json=payload,
-            )
+        self.app.put(
+            url,
+            json=payload,
+        )
         self.preprint.reload()
-        assert len(notifications['emits']) == 1
-        assert notifications['emits'][0]['type'] == NotificationType.Type.ADDON_FILE_RENAMED
         assert self.preprint.logs.latest().action == 'osf_storage_addon_file_renamed'
 
     def test_action_downloads_contrib(self):
@@ -2279,11 +2275,8 @@ def test_add_file_osfstorage_log(self):
         url = self.preprint.api_url_for('create_waterbutler_log')
         payload = self.build_payload(metadata={'nid': self.preprint._id, 'materialized': path, 'kind': 'file', 'path': path})
         nlogs = self.preprint.logs.count()
-        with capture_notifications() as notifications:
-            self.app.put(url, json=payload)
+        self.app.put(url, json=payload)
         self.preprint.reload()
-        assert len(notifications['emits']) == 1
-        assert notifications['emits'][0]['type'] == NotificationType.Type.FILE_ADDED
         assert self.preprint.logs.count() == nlogs + 1
         assert ('urls' in self.preprint.logs.filter(action='osf_storage_file_added')[0].params)
 
@@ -2292,11 +2285,8 @@ def test_add_folder_osfstorage_log(self):
         url = self.preprint.api_url_for('create_waterbutler_log')
         payload = self.build_payload(metadata={'nid': self.preprint._id, 'materialized': path, 'kind': 'folder', 'path': path})
         nlogs = self.preprint.logs.count()
-        with capture_notifications() as notifications:
-            self.app.put(url, json=payload)
+        self.app.put(url, json=payload)
         self.preprint.reload()
-        assert len(notifications['emits']) == 1
-        assert notifications['emits'][0]['type'] == NotificationType.Type.FILE_ADDED
         assert self.preprint.logs.count() == nlogs + 1
         assert ('urls' not in self.preprint.logs.filter(action='osf_storage_file_added')[0].params)
 

website/project/views/comment.py

@@ -14,7 +14,7 @@
 
 
 @file_updated.connect
-def update_file_guid_referent(self, target, payload, user=None):
+def update_file_guid_referent(self, target, event_type, payload, user=None):
     event_type = payload['action']
     if event_type not in ('addon_file_moved', 'addon_file_renamed'):
         return  # Nothing to do

website/settings/defaults.py

@@ -623,7 +623,7 @@ class CeleryConfig:
             'kwargs': {'dry_run': False},
         },
         '5-minute-user-emails': {
-            'task': 'notifications.tasks.send_moderators_instant_digest_email',
+            'task': 'notifications.tasks.send_users_instant_digest_email',
             'schedule': crontab(minute='*/5'),
             'kwargs': {'dry_run': False},
         },