fix state divergence for registration approval

Author: antkryt

osf/models/node.py

@@ -1239,7 +1239,7 @@ def set_privacy(self, permissions, auth=None, log=True, save=True, meeting_creat
                                 action_flag=DOI_CREATION_FAILED
                             )
                         raise NodeStateError(
-                            'Unable to make registration public: DOI creation failed. '
+                            f'Unable to make registration {self._id} public: DOI creation failed. '
                             'This may be due to a temporary DataCite service outage. '
                             'Please try again later or contact support if the issue persists.'
                         )

osf/models/sanctions.py

@@ -1,7 +1,7 @@
 from django.apps import apps
 from django.utils import timezone
 from django.conf import settings
-from django.db import models
+from django.db import models, transaction
 
 from osf.utils.fields import NonNaiveDateTimeField
 
@@ -951,31 +951,32 @@ def _on_complete(self, event_data):
         if registration.is_spammy:
             raise NodeStateError('Cannot approve a spammy registration')
 
-        super()._on_complete(event_data)
-        self.save()
-        registered_from = registration.registered_from
-        # Pass auth=None because the registration initiator may not be
-        # an admin on components (component admins had the opportunity
-        # to disapprove the registration by this point)
-        registration.set_privacy('public', auth=None, log=False)
-        for child in registration.get_descendants_recursive(primary_only=True):
-            child.set_privacy('public', auth=None, log=False)
-        # Accounts for system actions where no `User` performs the final approval
-        auth = Auth(user) if user else None
-        registered_from.add_log(
-            action=NodeLog.REGISTRATION_APPROVAL_APPROVED,
-            params={
-                'node': registered_from._id,
-                'registration': registration._id,
-                'registration_approval_id': self._id,
-            },
-            auth=auth,
-        )
-        for node in registration.root.node_and_primary_descendants():
-            self._add_success_logs(node, user)
-            node.update_search()  # update search if public
+        with transaction.atomic():
+            super()._on_complete(event_data)
+            self.save()
+            registered_from = registration.registered_from
+            # Pass auth=None because the registration initiator may not be
+            # an admin on components (component admins had the opportunity
+            # to disapprove the registration by this point)
+            registration.set_privacy('public', auth=None, log=False)
+            for child in registration.get_descendants_recursive(primary_only=True):
+                child.set_privacy('public', auth=None, log=False)
+            # Accounts for system actions where no `User` performs the final approval
+            auth = Auth(user) if user else None
+            registered_from.add_log(
+                action=NodeLog.REGISTRATION_APPROVAL_APPROVED,
+                params={
+                    'node': registered_from._id,
+                    'registration': registration._id,
+                    'registration_approval_id': self._id,
+                },
+                auth=auth,
+            )
+            for node in registration.root.node_and_primary_descendants():
+                self._add_success_logs(node, user)
+                node.update_search()  # update search if public
 
-        self.save()
+            self.save()
 
     def _on_reject(self, event_data):
         user = event_data.kwargs.get('user')

scripts/approve_registrations.py

@@ -38,6 +38,14 @@ def approve_past_pendings(approvals_past_pending, dry_run=True):
                 f'RegistrationApproval {registration_approval._id} is not attached to a registration'
             )
             continue
+
+        if pending_registration.is_spammy:
+            logger.warning(
+                f'Skipping RegistrationApproval {registration_approval._id} for '
+                f'spammy registration {pending_registration._id}.'
+            )
+            continue
+
         logger.warning(
             'RegistrationApproval {} automatically approved by system. Making registration {} public.'
             .format(registration_approval._id, pending_registration._id)

scripts/tests/test_approve_registrations.py

@@ -4,6 +4,7 @@
 
 from tests.base import OsfTestCase
 from osf.models import NodeLog
+from osf.models.spam import SpamStatus
 from osf_tests.factories import RegistrationFactory, UserFactory
 
 from scripts.approve_registrations import main
@@ -73,3 +74,15 @@ def test_registration_adds_to_parent_projects_log(self):
         assert self.registration.registered_from.logs.filter(
                 action=NodeLog.PROJECT_REGISTERED
             ).exists()
+
+    def test_spammy_registration_is_not_auto_approved_or_made_public(self):
+        self.registration.registration_approval.initiation_date = timezone.now() - timedelta(days=365)
+        self.registration.spam_status = SpamStatus.FLAGGED
+        self.registration.save()
+
+        main(dry_run=False)
+        self.registration.registration_approval.reload()
+        self.registration.reload()
+
+        assert not self.registration.is_registration_approved
+        assert not self.registration.is_public

tests/test_registrations/test_registration_approvals.py

@@ -318,3 +318,22 @@ def test_accept_raises_error_if_project_is_spam(self):
             with pytest.raises(NodeStateError):
                 self.registration.registration_approval.accept()
         assert mock_notify.call_count == 0
+
+    def test_accept_rolls_back_approval_if_publish_fails(self):
+        self.registration.require_approval(self.user)
+        self.registration.save()
+        assert self.registration.is_pending_registration
+
+        with mock.patch.object(
+            self.registration.__class__,
+            'set_privacy',
+            side_effect=NodeStateError('forced publish failure')
+        ):
+            with pytest.raises(NodeStateError):
+                self.registration.registration_approval.accept()
+
+        self.registration.registration_approval.reload()
+        self.registration.reload()
+        assert self.registration.is_pending_registration
+        assert not self.registration.is_registration_approved
+        assert not self.registration.is_public