Merge pull request #11395 from Ostap-Zherebetskyi/fix/NR_review_comments

[ENG-9162] fix NR review comments

Author: Johnetordoff

addons/boa/tasks.py

@@ -186,7 +186,7 @@ async def submit_to_boa_async(host, username, password, user_guid, project_guid,
     NotificationType.Type.ADDONS_BOA_JOB_COMPLETE.instance.emit(
         user=user,
         event_context={
-            'fullname': user.fullname,
+            'user_fullname': user.fullname,
             'query_file_name': query_file_name,
             'query_file_full_path': file_full_path,
             'output_file_name': output_file_name,

addons/boa/tests/test_tasks.py

@@ -39,9 +39,6 @@ def setUp(self):
         self.output_file_name = 'fake_boa_script_results.txt'
         self.job_id = '1a2b3c4d5e6f7g8'
 
-    def tearDown(self):
-        super().tearDown()
-
     def test_boa_error_code(self):
         assert BoaErrorCode.NO_ERROR == -1
         assert BoaErrorCode.UNKNOWN == 0

addons/osfstorage/tests/test_models.py

@@ -9,7 +9,7 @@
 
 from framework.auth import Auth
 from addons.osfstorage.models import OsfStorageFile, OsfStorageFileNode, OsfStorageFolder
-from osf.models import BaseFileNode
+from osf.models import BaseFileNode, NotificationType
 from osf.exceptions import ValidationError
 from osf.utils.permissions import WRITE, ADMIN
 
@@ -746,8 +746,11 @@ def test_after_fork_copies_versions(self, node, node_settings, auth_obj):
             version = factories.FileVersionFactory()
             record.add_version(version)
 
-        with capture_notifications():
+        with capture_notifications() as notifications:
             fork = node.fork_node(auth_obj)
+
+        assert len(notifications['emits']) == 1
+        assert notifications['emits'][0]['type'] == NotificationType.Type.NODE_CONTRIBUTOR_ADDED_DEFAULT
         fork_node_settings = fork.get_addon('osfstorage')
         fork_node_settings.reload()
 

api/providers/tasks.py

@@ -646,6 +646,7 @@ def bulk_upload_finish_job(upload, row_count, success_count, draft_errors, appro
             notification_type = NotificationType.Type.USER_REGISTRATION_BULK_UPLOAD_FAILURE_ALL
         else:
             logger.error(f'Unexpected job state for upload [{upload.id}]: {upload.state.name}')
+            sentry.log_message(f'Unexpected job state for upload [{upload.id}]: {upload.state.name}')
             return
 
         notification_type.instance.emit(

api_tests/crossref/views/test_crossref_email_response.py

@@ -216,6 +216,7 @@ def test_confirmation_marks_legacy_doi_as_deleted(self, app, url, preprint):
         update_xml = self.update_success_xml(preprint)
 
         context_data = self.make_mailgun_payload(crossref_response=update_xml)
-        app.post(url, context_data)
+        with capture_notifications(expect_none=True):
+            app.post(url, context_data)
 
         assert preprint.identifiers.get(category='legacy_doi').deleted

api_tests/draft_registrations/views/test_draft_registration_contributor_list.py

@@ -315,7 +315,8 @@ def test_add_unregistered_contributor_without_email_no_email(self, app, user, ur
         }
 
         with capture_signals() as mock_signal:
-            res = app.post_json_api(url, payload, auth=user.auth)
+            with capture_notifications(expect_none=True):
+                res = app.post_json_api(url, payload, auth=user.auth)
         assert contributor_added in mock_signal.signals_sent()
         assert res.status_code == 201
 

api_tests/institutions/views/test_institution_auth.py

@@ -410,7 +410,7 @@ def test_user_inactive(self, app, institution, url_auth_institution):
 
         username, fullname, password = '[email protected]', 'Foo Bar', 'FuAsKeEr'
         user = make_user(username, fullname)
-        with capture_notifications() as mock_signals:
+        with capture_notifications():
             user.set_password(password)
         # User must be saved before deactivation
         user.save()

notifications.yaml

@@ -193,13 +193,6 @@ notification_types:
     template: 'website/templates/storage_cap_exceeded_announcement.html.mako'
     tests: []
 
-  - name: user_add_sso_email_osf4i
-    subject: 'Your OSF Account Email Address'
-    __docs__: ...
-    object_content_type_model_name: osfuser
-    template: 'website/templates/add_sso_email_osf4i.html.mako'
-    tests: ['']
-
   - name: user_duplicate_accounts_sso_osf4i
     subject: 'Duplicate Account Detection'
     __docs__: ...

notifications/listeners.py

@@ -44,20 +44,22 @@ def subscribe_contributor(resource, contributor, auth=None, *args, **kwargs):
             return None
 
     if contributor.is_registered:
-        NotificationSubscription.objects.get_or_create(
-            user=contributor,
-            notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
-            object_id=contributor.id,
-            content_type=ContentType.objects.get_for_model(contributor),
-            _is_digest=True,
-        )
-        NotificationSubscription.objects.get_or_create(
-            user=contributor,
-            notification_type=NotificationType.Type.FILE_UPDATED.instance,
-            object_id=resource.id,
-            content_type=ContentType.objects.get_for_model(resource),
-            _is_digest=True,
-        )
+        NotificationSubscription.objects.bulk_create([
+            NotificationSubscription(
+                user=contributor,
+                notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
+                object_id=contributor.id,
+                content_type=ContentType.objects.get_for_model(contributor),
+                _is_digest=True,
+            ),
+            NotificationSubscription(
+                user=contributor,
+                notification_type=NotificationType.Type.FILE_UPDATED.instance,
+                object_id=resource.id,
+                content_type=ContentType.objects.get_for_model(resource),
+                _is_digest=True,
+            )
+        ], ignore_conflicts=True)
 
 
 # Handle email notifications to notify moderators of new submissions.

notifications/tasks.py

@@ -19,31 +19,41 @@
 
 logger = get_task_logger(__name__)
 
-@celery_app.task(bind=True)
-def send_user_email_task(self, user_id, notification_ids, **kwargs):
+
+def get_user_and_email_task(task_id, user_id):
+    """Helper to safely fetch user and initialize EmailTask."""
     try:
         user = OSFUser.objects.get(
             guids___id=user_id,
             deleted__isnull=True
         )
     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)
+        email_task, _ = EmailTask.objects.get_or_create(task_id=task_id)
         email_task.status = 'NO_USER_FOUND'
         email_task.error_message = 'User not found or disabled'
         email_task.save()
+        return None, email_task
+
+    email_task, _ = EmailTask.objects.get_or_create(task_id=task_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'
+        email_task.save()
+        return None, email_task
+
+    return user, email_task
+
+@celery_app.task(bind=True)
+def send_user_email_task(self, user_id, notification_ids, **kwargs):
+    user, email_task = get_user_and_email_task(self.request.id, user_id)
+    if not user:
         return
 
     try:
-        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'
-            email_task.save()
-            return
-
         notifications_qs = Notification.objects.filter(id__in=notification_ids)
         rendered_notifications = [n.render() for n in notifications_qs]
 
@@ -90,29 +100,11 @@ def send_user_email_task(self, user_id, notification_ids, **kwargs):
 
 @celery_app.task(bind=True)
 def send_moderator_email_task(self, user_id, provider_id, notification_ids, **kwargs):
-    try:
-        user = OSFUser.objects.get(
-            guids___id=user_id,
-            deleted__isnull=True
-        )
-    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)
-        email_task.status = 'NO_USER_FOUND'
-        email_task.error_message = 'User not found or disabled'
-        email_task.save()
+    user, email_task = get_user_and_email_task(self.request.id, user_id)
+    if not user:
         return
 
     try:
-        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'
-            email_task.save()
-            return
-
         notifications_qs = Notification.objects.filter(id__in=notification_ids)
         rendered_notifications = [notification.render() for notification in notifications_qs]
 
@@ -302,10 +294,7 @@ def remove_supplemental_node_from_preprints(node_id):
     AbstractNode = apps.get_model('osf.AbstractNode')
 
     node = AbstractNode.load(node_id)
-    for preprint in node.preprints.all():
-        if preprint.node is not None:
-            preprint.node = None
-            preprint.save()
+    node.preprints.filter(node__isnull=False).update(node=None)
 
 
 @run_postcommit(once_per_request=False, celery=True)