[ENG-12157] Handle contributors without permission group in merge_user

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)
+        merge_users.delay(user._id, user_to_be_merged._id, initiator_guid=self.request.user._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)
+        mock_merge_users_delay.assert_called_with(user._id, user_merged._id, initiator_guid=view.request.user._id)

api/users/tasks.py

@@ -10,30 +10,54 @@
 
 
 @celery_app.task(name='api.users.tasks.merge_users')
-def merge_users(merger_guid: str, mergee_guid: str):
+def merge_users(merger_guid: str, mergee_guid: str, initiator_guid: str | None = None):
     """
     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
     """
-    from osf.models import OSFUser
+    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
 
     try:
-        merger = OSFUser.load(merger_guid)
-        mergee = OSFUser.load(mergee_guid)
+        merger.merge_user(mergee)
+    except Exception as exc:
+        _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, repr(exc))
 
-        if not merger or not mergee:
-            sentry.log_message(f'User merge task received invalid users: merger={merger_guid}, mergee={mergee_guid}')
-            return
 
-        if merger == mergee:
-            sentry.log_message(f'User merge task attempted to merge a user into itself: {merger_guid}')
+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:
             return
-
-        merger.merge_user(mergee)
+        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,
+        )
     except Exception as exc:
-        logger.exception(f'Unexpected error during background user merge: merger={merger_guid}, mergee={mergee_guid}')
+        logger.exception('Failed to send user merge failure email')
         sentry.log_exception(exc)
 
 

api_tests/users/views/test_user_settings.py

@@ -5,10 +5,12 @@
 from api.base.utils import hashids
 from osf_tests.factories import (
     AuthUserFactory,
+    ProjectFactory,
     UserFactory,
 )
 from osf.models import Email, NotableDomain, NotificationTypeEnum
 from framework.auth.views import auth_email_logout
+from osf.utils.permissions import READ
 from tests.utils import capture_notifications
 
 @pytest.fixture()
@@ -579,6 +581,30 @@ def test_updating_verified_for_merge(self, app, user_one, user_two, payload):
         assert res.json['data']['attributes']['confirmed'] is True
         assert res.json['data']['attributes']['is_merge'] is False
 
+    def test_updating_verified_for_merge_contributor_without_permission_group(self, app, user_one, user_two, payload):
+        """Regression: merging used to fail with `ValueError: None is not in list` (HTTP 500)
+        when both accounts were contributors on a project and one had no permission group."""
+        project = ProjectFactory(creator=user_two)
+        project.add_contributor(user_one, permissions=READ, save=True)
+        project.remove_permission(user_one, READ)
+        assert project.contributor_set.get(user=user_one).permission is None
+
+        payload['data']['attributes'] = {'verified': True}
+        token = user_one.add_unconfirmed_email(user_two.username)
+        user_one.email_verifications[token]['confirmed'] = True
+        user_one.save()
+        url = f'/{API_BASE}users/{user_one._id}/settings/emails/{token}/'
+
+        res = app.patch_json_api(url, payload, auth=user_one.auth, expect_errors=True)
+
+        assert res.status_code == 200
+        assert res.json['data']['attributes']['verified'] is True
+        user_two.reload()
+        assert user_two.merged_by == user_one
+        assert project.is_contributor(user_one)
+        assert not project.is_contributor(user_two)
+        assert project.contributor_set.get(user=user_one).permission == 'admin'
+
     @mock.patch('api.users.views.send_confirm_email_async')
     def test_resend_confirmation_email(self, mock_send_confirm_email_async, app, user_one, unconfirmed_url, confirmed_url):
         url = f'{unconfirmed_url}?resend_confirmation=True'

notifications.yaml

@@ -325,6 +325,13 @@ 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

@@ -82,6 +82,7 @@ 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

@@ -772,6 +772,24 @@ 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)
@@ -862,8 +880,9 @@ def merge_user(self, user):
             # if both accounts are contributor of the same project
             if node.is_contributor(self) and node.is_contributor(user):
                 self_perms = Contributor(node=node, user=self).permission
-                permissions = API_CONTRIBUTOR_PERMISSIONS[max(API_CONTRIBUTOR_PERMISSIONS.index(user_perms), API_CONTRIBUTOR_PERMISSIONS.index(self_perms))]
-                node.set_permissions(user=self, permissions=permissions)
+                permissions = self._highest_permission(node, user, user_perms, self_perms)
+                if permissions:
+                    node.set_permissions(user=self, permissions=permissions)
 
                 visible1 = self._id in node.visible_contributor_ids
                 visible2 = user._id in node.visible_contributor_ids
@@ -873,9 +892,12 @@ def merge_user(self, user):
                 node.contributor_set.filter(user=user).delete()
             else:
                 node.contributor_set.filter(user=user).update(user=self)
-                node.add_permission(self, user_perms)
+                self._warn_if_no_permission(node, user, user_perms)
+                if user_perms:
+                    node.add_permission(self, user_perms)
 
-            node.remove_permission(user, user_perms)
+            if user_perms:
+                node.remove_permission(user, user_perms)
             node.save()
 
         # Skip bookmark collections
@@ -898,8 +920,6 @@ 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()
@@ -909,6 +929,12 @@ 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
@@ -925,6 +951,22 @@ def merge_user(self, user):
             except Exception as e:
                 logger.exception(f'Failed to SHARE reindex preprint {preprint._id} during user merge: {e}')
 
+    def _warn_if_no_permission(self, resource, user, permission):
+        """Contributor rows without a permission group are inconsistent data. Keep them visible without failing the merge."""
+        if permission is None:
+            message = f'Merging user {user._id} into {self._id}: contributor of {resource._id} has no permission group'
+            logger.warning(message)
+            sentry.log_message(message)
+
+    def _highest_permission(self, resource, user, *permissions):
+        """Return the highest of `permissions`, ignoring missing (`None`) ones. `None` if none are set."""
+        if any(permission is None for permission in permissions):
+            self._warn_if_no_permission(resource, user, None)
+        known = [permission for permission in permissions if permission is not None]
+        if not known:
+            return None
+        return API_CONTRIBUTOR_PERMISSIONS[max(API_CONTRIBUTOR_PERMISSIONS.index(permission) for permission in known)]
+
     def _merge_users_preprints(self, user):
         """
         Preprints use guardian.  The PreprintContributor table stores order and bibliographic information.
@@ -943,9 +985,10 @@ def _merge_users_preprints(self, user):
                 self_contributor = PreprintContributor.objects.get(preprint=preprint, user=self)
                 self_perms = self_contributor.permission
 
-                max_perms_index = max(API_CONTRIBUTOR_PERMISSIONS.index(self_perms), API_CONTRIBUTOR_PERMISSIONS.index(user_perms))
                 # Add the highest of `self` perms or `user` perms to `self`
-                preprint.set_permissions(user=self, permissions=API_CONTRIBUTOR_PERMISSIONS[max_perms_index])
+                highest_perms = self._highest_permission(preprint, user, self_perms, user_perms)
+                if highest_perms:
+                    preprint.set_permissions(user=self, permissions=highest_perms)
 
                 if not self_contributor.visible and user_contributor.visible:
                     # if `self` is not visible, but `user` is visible, make `self` visible.
@@ -957,12 +1000,15 @@ def _merge_users_preprints(self, user):
                 # `self` is not a contributor, but `user` is.  Transfer `user` permissions and
                 # contributor information to `self`.  Remove permissions from `user`.
                 preprint.contributor_set.filter(user=user).update(user=self)
-                preprint.add_permission(self, user_perms)
+                self._warn_if_no_permission(preprint, user, user_perms)
+                if user_perms:
+                    preprint.add_permission(self, user_perms)
 
             if preprint.creator == user:
                 preprint.creator = self
 
-            preprint.remove_permission(user, user_perms)
+            if user_perms:
+                preprint.remove_permission(user, user_perms)
             preprint.save()
 
     @property
@@ -999,9 +1045,10 @@ def _merge_user_draft_registrations(self, user):
                 self_contributor = DraftRegistrationContributor.objects.get(draft_registration=draft_reg, user=self)
                 self_perms = self_contributor.permission
 
-                max_perms_index = max(API_CONTRIBUTOR_PERMISSIONS.index(self_perms), API_CONTRIBUTOR_PERMISSIONS.index(user_perms))
                 # Add the highest of `self` perms or `user` perms to `self`
-                draft_reg.set_permissions(user=self, permissions=API_CONTRIBUTOR_PERMISSIONS[max_perms_index])
+                highest_perms = self._highest_permission(draft_reg, user, self_perms, user_perms)
+                if highest_perms:
+                    draft_reg.set_permissions(user=self, permissions=highest_perms)
 
                 if not self_contributor.visible and user_contributor.visible:
                     # if `self` is not visible, but `user` is visible, make `self` visible.
@@ -1013,12 +1060,15 @@ def _merge_user_draft_registrations(self, user):
                 # `self` is not a contributor, but `user` is.  Transfer `user` permissions and
                 # contributor information to `self`.  Remove permissions from `user`.
                 draft_reg.contributor_set.filter(user=user).update(user=self)
-                draft_reg.add_permission(self, user_perms)
+                self._warn_if_no_permission(draft_reg, user, user_perms)
+                if user_perms:
+                    draft_reg.add_permission(self, user_perms)
 
             if draft_reg.initiator == user:
                 draft_reg.initiator = self
 
-            draft_reg.remove_permission(user, user_perms)
+            if user_perms:
+                draft_reg.remove_permission(user, user_perms)
             draft_reg.save()
 
     @property