Revert "[ENG-12157] - Wrap merge_user in a transaction"

admin/users/views.py

@@ -432,7 +432,7 @@ def form_valid(self, form):
         guid_to_be_merged = form.cleaned_data['user_guid_to_be_merged']
 
         user_to_be_merged = OSFUser.objects.get(guids___id=guid_to_be_merged, guids___id__isnull=False)
-        merge_users.delay(user._id, user_to_be_merged._id, initiator_guid=self.request.user._id)
+        merge_users.delay(user._id, user_to_be_merged._id)
         messages.success(
             self.request,
             f'Merge of user {user_to_be_merged._id} into {user._id} has been queued and will run in the background.',

admin_tests/users/test_views.py

@@ -717,4 +717,4 @@ def test_merge_user(self, mock_merge_users_delay):
         assert valid_form.is_valid()
 
         view.form_valid(valid_form)
-        mock_merge_users_delay.assert_called_with(user._id, user_merged._id, initiator_guid=view.request.user._id)
+        mock_merge_users_delay.assert_called_with(user._id, user_merged._id)

api/users/tasks.py

@@ -10,54 +10,30 @@
 
 
 @celery_app.task(name='api.users.tasks.merge_users')
-def merge_users(merger_guid: str, mergee_guid: str, initiator_guid: str | None = None):
+def merge_users(merger_guid: str, mergee_guid: str):
     """
     Background task to merge one user into another.
 
     :param merger_guid: GUID of the primary user that will receive content
     :param mergee_guid: GUID of the user being merged into the primary user
-    :param initiator_guid: GUID of the user who started the merge, notified if it fails
     """
-    merger = OSFUser.load(merger_guid)
-    mergee = OSFUser.load(mergee_guid)
-
-    if not merger or not mergee:
-        message = f'User merge task received invalid users: merger={merger_guid}, mergee={mergee_guid}'
-        sentry.log_message(message)
-        _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, message)
-        return
-
-    if merger == mergee:
-        message = f'User merge task attempted to merge a user into itself: {merger_guid}'
-        sentry.log_message(message)
-        _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, message)
-        return
+    from osf.models import OSFUser
 
     try:
-        merger.merge_user(mergee)
-    except Exception as exc:
-        _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, repr(exc))
+        merger = OSFUser.load(merger_guid)
+        mergee = OSFUser.load(mergee_guid)
 
+        if not merger or not mergee:
+            sentry.log_message(f'User merge task received invalid users: merger={merger_guid}, mergee={mergee_guid}')
+            return
 
-def _notify_merge_failed(initiator_guid: str | None, merger_guid: str, mergee_guid: str, error: str):
-    if not initiator_guid:
-        return
-    try:
-        initiator = OSFUser.load(initiator_guid)
-        if not initiator:
+        if merger == mergee:
+            sentry.log_message(f'User merge task attempted to merge a user into itself: {merger_guid}')
             return
-        NotificationTypeEnum.USER_MERGE_FAILED_REPORT.instance.emit(
-            user=initiator,
-            message_frequency='instantly',
-            event_context={
-                'merger_guid': merger_guid,
-                'mergee_guid': mergee_guid,
-                'error': error,
-            },
-            save=False,
-        )
+
+        merger.merge_user(mergee)
     except Exception as exc:
-        logger.exception('Failed to send user merge failure email')
+        logger.exception(f'Unexpected error during background user merge: merger={merger_guid}, mergee={mergee_guid}')
         sentry.log_exception(exc)
 
 

notifications.yaml

@@ -325,13 +325,6 @@ notification_types:
     template: 'website/templates/user_confirm_ham_report.html.mako'
     tests: []
 
-  - name: user_merge_failed_report
-    subject: 'Merge of user {mergee_guid} into {merger_guid} failed'
-    __docs__: Sent to the admin who triggered a user merge when the merge fails.
-    object_content_type_model_name: osfuser
-    template: 'website/templates/user_merge_failed_report.html.mako'
-    tests: []
-
   - name: desk_registration_bulk_upload_product_owner
     subject: 'Registry Could Not Bulk Upload Registrations'
     object_content_type_model_name: osfuser

osf/models/notification_type.py

@@ -81,7 +81,6 @@ class NotificationTypeEnum(str, Enum):
     USER_CROSSREF_DOI_PENDING = 'user_crossref_doi_pending'
     USER_TERMS_OF_USE_UPDATED = 'user_terms_of_use_updated'  # added as a placeholder
     USER_CONFIRM_HAM_REPORT = 'user_confirm_ham_report'
-    USER_MERGE_FAILED_REPORT = 'user_merge_failed_report'
 
     # Node notifications
     NODE_FILE_UPDATED = 'node_file_updated'

osf/models/user.py

@@ -771,24 +771,6 @@ def merge_user(self, user):
         if self == user:
             raise ValueError('Cannot merge a user into itself')
 
-        try:
-            with transaction.atomic():
-                nodes_to_reindex, preprints_to_reindex = self._merge_user(user)
-        except Exception as exc:
-            logger.exception(f'Failed to merge user {user._id} into {self._id}; the merge was rolled back')
-            sentry.log_exception(exc)
-            self.refresh_from_db()
-            user.refresh_from_db()
-            raise
-
-        # Side effects outside the database only run once the merge is committed
-        transaction.on_commit(lambda: self._after_merge_user(user, nodes_to_reindex, preprints_to_reindex))
-
-    def _merge_user(self, user):
-        """Database part of `merge_user`. Must run inside a transaction.
-
-        :return: nodes and preprints of `user` to reindex in SHARE after the merge is committed
-        """
         # Capture content to SHARE reindex BEFORE merge transfers contributors
         # After merge, user.contributed and user.preprints will be empty
         nodes_to_reindex = list(user.contributed)
@@ -915,6 +897,8 @@ def _merge_user(self, user):
         self._merge_user_draft_registrations(user)
 
         # finalize the merge
+        remove_sessions_for_user(user)
+
         # - username is set to the GUID so the merging user can set it primary
         #   in the future (note: it cannot be set to None due to non-null constraint)
         user.set_unusable_username()
@@ -924,12 +908,6 @@ def _merge_user(self, user):
         user.merged_by = self
 
         user.save()
-
-        return nodes_to_reindex, preprints_to_reindex
-
-    def _after_merge_user(self, user, nodes_to_reindex, preprints_to_reindex):
-        """Side effects of `merge_user` outside the database, run after the merge is committed."""
-        remove_sessions_for_user(user)
         signals.user_account_merged.send(user)
 
         from api.share.utils import update_share

osf_tests/test_merging_users.py

@@ -1,11 +1,9 @@
 import pytest
 from unittest import mock
 import datetime as dt
-from django.test import TestCase
 from django.utils import timezone
 from tests.base import OsfTestCase
 
-from api.users.tasks import merge_users
 from framework.celery_tasks import handlers
 from website import settings
 from website.util.metrics import OsfSourceTags
@@ -198,7 +196,7 @@ def is_mrm_field(value):
         mock_client = mock.MagicMock()
         mock_get_mailchimp_api.return_value = mock_client
 
-        with run_celery_tasks(), TestCase.captureOnCommitCallbacks(execute=True):
+        with run_celery_tasks():
             # perform the merge
             with override_flag(ENABLE_GV, active=True):
                 self.user.merge_user(other_user)
@@ -330,52 +328,3 @@ def test_send_confirm_email_emits_merge_notification(self):
         assert len(notifications['emits']) == 1
         assert notifications['emits'][0]['type'] == NotificationTypeEnum.USER_CONFIRM_MERGE
         assert notifications['emits'][0]['kwargs']['destination_address'] == target_email
-
-    @mock.patch('framework.sentry.log_exception')
-    def test_failed_merge_is_rolled_back(self, mock_log_exception):
-        other_user = UserFactory()
-        other_user.add_system_tag('other')
-        project = ProjectFactory(creator=other_user)
-        error = Exception('missing permissions')
-
-        with mock.patch('osf.models.user.OSFUser._merge_users_preprints', side_effect=error):
-            with pytest.raises(Exception, match='missing permissions'):
-                self.user.merge_user(other_user)
-
-        mock_log_exception.assert_called_once_with(error)
-        project.reload()
-        self.user.reload()
-        other_user.reload()
-        assert project.is_contributor(other_user)
-        assert not project.is_contributor(self.user)
-        assert project.creator == other_user
-        assert other_user.merged_by is None
-        assert not other_user.is_merged
-        assert 'other' not in self.user.system_tags
-        assert other_user.emails.filter(address=other_user.username).exists()
-
-    def test_merge_users_task_notifies_initiator_on_failure(self):
-        other_user = UserFactory()
-        initiator = UserFactory()
-
-        with mock.patch('osf.models.user.OSFUser.merge_user', side_effect=Exception('missing permissions')):
-            with capture_notifications() as notifications:
-                merge_users(self.user._id, other_user._id, initiator_guid=initiator._id)
-
-        assert len(notifications['emits']) == 1
-        assert notifications['emits'][0]['type'] == NotificationTypeEnum.USER_MERGE_FAILED_REPORT
-        assert notifications['emits'][0]['kwargs']['user'] == initiator
-        assert notifications['emits'][0]['kwargs']['event_context']['merger_guid'] == self.user._id
-        assert notifications['emits'][0]['kwargs']['event_context']['mergee_guid'] == other_user._id
-        assert 'missing permissions' in notifications['emits'][0]['kwargs']['event_context']['error']
-
-    def test_merge_users_task_does_not_notify_on_success(self):
-        other_user = UserFactory()
-        initiator = UserFactory()
-
-        with capture_notifications(expect_none=True):
-            with override_flag(ENABLE_GV, active=True):
-                merge_users(self.user._id, other_user._id, initiator_guid=initiator._id)
-
-        other_user.reload()
-        assert other_user.merged_by == self.user

osf_tests/test_user.py

@@ -447,7 +447,7 @@ def test_merge_drafts(self, user):
         assert not draft_five.is_contributor(user2)
 
     @mock.patch('api.share.utils.update_share')
-    def test_merge_user_triggers_share_reindex(self, mock_update_share, django_capture_on_commit_callbacks):
+    def test_merge_user_triggers_share_reindex(self, mock_update_share):
         from osf.models import Preprint
 
         user = AuthUserFactory()
@@ -461,8 +461,7 @@ def test_merge_user_triggers_share_reindex(self, mock_update_share, django_captu
         preprint_two = PreprintFactory(title='preprint_two')
         preprint_two.add_contributor(user2)
 
-        with django_capture_on_commit_callbacks(execute=True):
-            user.merge_user(user2)
+        user.merge_user(user2)
 
         # Verify update_share was called for both nodes
         nodes_reindexed = [
@@ -1527,24 +1526,22 @@ def test_dupe_email_is_appended(self, master, merge_dupe):
         assert master.emails.filter(address='joseph123@hotmail.com').exists()
 
     @mock.patch('website.mailchimp_utils.get_mailchimp_api')
-    def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe, django_capture_on_commit_callbacks):
+    def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe):
         dupe.mailchimp_mailing_lists['foo'] = True
         dupe.save()
 
         with capture_signals() as mock_signals:
-            with django_capture_on_commit_callbacks(execute=True):
-                merge_dupe()
+            merge_dupe()
             assert mock_signals.signals_sent() == {user_account_merged}
 
     @pytest.mark.enable_enqueue_task
     @mock.patch('website.mailchimp_utils.unsubscribe_mailchimp_async')
     @mock.patch('website.mailchimp_utils.get_mailchimp_api')
-    def test_merged_user_unsubscribed_from_mailing_lists(self, mock_mailchimp_api, mock_unsubscribe, dupe, merge_dupe, email_subscriptions_enabled, django_capture_on_commit_callbacks):
+    def test_merged_user_unsubscribed_from_mailing_lists(self, mock_mailchimp_api, mock_unsubscribe, dupe, merge_dupe, email_subscriptions_enabled):
         list_name = settings.MAILCHIMP_GENERAL_LIST
         dupe.mailchimp_mailing_lists[list_name] = True
         dupe.save()
-        with django_capture_on_commit_callbacks(execute=True):
-            merge_dupe()
+        merge_dupe()
         assert mock_unsubscribe.called
 
     def test_inherits_projects_contributed_by_dupe(self, dupe, master, merge_dupe):

tests/test_utils.py

@@ -486,15 +486,14 @@ def mock_receiver(user, **kwargs):
         mock_publish_deactivated_user.assert_called_once_with(user)
 
     @mock.patch('osf.external.messages.celery_publishers.publish_merged_user')
-    def test_user_account_merged_signal(self, mock_publish_merged_user, user, old_user, django_capture_on_commit_callbacks):
+    def test_user_account_merged_signal(self, mock_publish_merged_user, user, old_user):
         # Connect a mock receiver to the signal for testing
         @receiver(user_account_merged)
         def mock_receiver(user, **kwargs):
             return mock_publish_merged_user(user)
 
         # Trigger the signal
-        with django_capture_on_commit_callbacks(execute=True):
-            user.merge_user(old_user)
+        user.merge_user(old_user)
 
         # Verify that the mock receiver was called
         mock_publish_merged_user.assert_called_once_with(old_user)
@@ -550,11 +549,10 @@ def test_publish_body_on_merger(
             mock_publish_user_status_change,
             user,
             old_user,
-            account_status_changes_exchange,
-            django_capture_on_commit_callbacks,
+            account_status_changes_exchange
     ):
         with mock.patch.object(settings, 'USE_CELERY', True):
-            with override_flag(features.ENABLE_GV, active=True), django_capture_on_commit_callbacks(execute=True):
+            with override_flag(features.ENABLE_GV, active=True):
                 user.merge_user(old_user)
 
         mock_publish_user_status_change().__enter__().publish.assert_called_once_with(