[fix] Fixes by @coderabbitai

Author: pandafy

openwisp_radius/migrations/__init__.py

@@ -35,12 +35,45 @@ def _flush_bulk_create(model, objects, batch_size=BATCH_SIZE):
         objects.clear()
 
 
+def _flush_bulk_update(model, objects, fields, batch_size=BATCH_SIZE):
+    if objects:
+        model.objects.bulk_update(objects, fields=fields, batch_size=batch_size)
+        objects.clear()
+
+
 def _registered_user_extra_kwargs(registered_user, extra_fields=()):
     return {
         field_name: getattr(registered_user, field_name) for field_name in extra_fields
     }
 
 
+def _registered_user_method_priority_case():
+    # Strong methods (anything that is not '' or 'email') must rank above the
+    # weak fallbacks so rollback restores the strongest verification state.
+    return Case(
+        When(method="", then=Value(0)),
+        When(method="email", then=Value(1)),
+        default=Value(2),
+        output_field=IntegerField(),
+    )
+
+
+def _registered_user_method_priority(registered_user):
+    if registered_user.method == "":
+        return 0
+    if registered_user.method == "email":
+        return 1
+    return 2
+
+
+def _registered_user_strength(registered_user):
+    return (
+        int(registered_user.is_verified),
+        _registered_user_method_priority(registered_user),
+        registered_user.modified,
+    )
+
+
 def copy_registered_users_ctcr_forward(
     apps,
     schema_editor,
@@ -88,12 +121,7 @@ def copy_registered_users_ctcr_reverse(
     # Annotate each row with an explicit verification priority so that stronger
     # methods (anything that is not '' or 'email') sort before weaker ones.
     # Lexical ordering of 'method' would place '' first, picking the weakest.
-    method_priority = Case(
-        When(method="", then=Value(0)),
-        When(method="email", then=Value(1)),
-        default=Value(2),
-        output_field=IntegerField(),
-    )
+    method_priority = _registered_user_method_priority_case()
     queryset = RegisteredUserNew.objects.annotate(
         method_priority=method_priority
     ).order_by("user_id", "-is_verified", "-method_priority", "-modified")
@@ -187,21 +215,17 @@ def migrate_registered_users_multitenant_reverse(
     for user_id_batch in _batched_iterator(
         user_ids_qs.iterator(chunk_size=BATCH_SIZE), BATCH_SIZE
     ):
-        existing_globals = set(
-            RegisteredUser.objects.filter(
+        existing_globals = {
+            registered_user.user_id: registered_user
+            for registered_user in RegisteredUser.objects.filter(
                 user_id__in=user_id_batch,
                 organization__isnull=True,
-            ).values_list("user_id", flat=True)
-        )
+            )
+        }
         # Annotate each row with an explicit verification priority so that stronger
         # methods (anything that is not '' or 'email') sort before weaker ones.
         # Lexical ordering of 'method' would place '' first, picking the weakest.
-        method_priority = Case(
-            When(method="", then=Value(0)),
-            When(method="email", then=Value(1)),
-            default=Value(2),
-            output_field=IntegerField(),
-        )
+        method_priority = _registered_user_method_priority_case()
         org_records = (
             RegisteredUser.objects.filter(
                 user_id__in=user_id_batch,
@@ -212,28 +236,43 @@ def migrate_registered_users_multitenant_reverse(
         )
 
         to_create = []
+        to_update = []
         to_delete_pks = []
         current_user_id = None
+        update_fields = ["is_verified", "method", "modified", *extra_fields]
 
         for registered_user in org_records.iterator(chunk_size=BATCH_SIZE):
-            to_delete_pks.append(registered_user.pk)
             if registered_user.user_id == current_user_id:
+                to_delete_pks.append(registered_user.pk)
                 continue
             current_user_id = registered_user.user_id
-            if registered_user.user_id in existing_globals:
-                continue
-            restored = RegisteredUser(
-                id=uuid.uuid4(),
-                user_id=registered_user.user_id,
-                organization=None,
-                is_verified=registered_user.is_verified,
-                method=registered_user.method,
-                **_registered_user_extra_kwargs(registered_user, extra_fields),
-            )
-            restored.modified = registered_user.modified
-            to_create.append(restored)
+            existing_global = existing_globals.get(registered_user.user_id)
+            if existing_global is None:
+                restored = RegisteredUser(
+                    id=uuid.uuid4(),
+                    user_id=registered_user.user_id,
+                    organization=None,
+                    is_verified=registered_user.is_verified,
+                    method=registered_user.method,
+                    **_registered_user_extra_kwargs(registered_user, extra_fields),
+                )
+                restored.modified = registered_user.modified
+                to_create.append(restored)
+            elif _registered_user_strength(registered_user) > _registered_user_strength(
+                existing_global
+            ):
+                existing_global.is_verified = registered_user.is_verified
+                existing_global.method = registered_user.method
+                existing_global.modified = registered_user.modified
+                for field_name, value in _registered_user_extra_kwargs(
+                    registered_user, extra_fields
+                ).items():
+                    setattr(existing_global, field_name, value)
+                to_update.append(existing_global)
+            to_delete_pks.append(registered_user.pk)
 
         _flush_bulk_create(RegisteredUser, to_create)
+        _flush_bulk_update(RegisteredUser, to_update, fields=update_fields)
         if to_delete_pks:
             RegisteredUser.objects.filter(pk__in=to_delete_pks).delete()
 

openwisp_radius/tests/test_migrations.py

@@ -0,0 +1,129 @@
+import swapper
+from django.apps.registry import apps
+from django.utils import timezone
+
+from ..migrations import migrate_registered_users_multitenant_reverse
+from ..utils import load_model
+from .mixins import BaseTestCase
+
+RegisteredUser = load_model("RegisteredUser")
+Organization = swapper.load_model("openwisp_users", "Organization")
+User = swapper.load_model("auth", "User")
+
+
+class TestMigrations(BaseTestCase):
+    def test_multitenant_reverse_updates_weaker_existing_global(self):
+        """
+        Test that during migration rollback, a weaker existing global
+        RegisteredUser is updated with data from a stronger org-scoped
+        RegisteredUser instead of being left unchanged.
+        """
+        user = self._create_user(username="rollback-stronger")
+        org1 = self._create_org(name="rollback-org-1", slug="rollback-org-1")
+        org2 = self._create_org(name="rollback-org-2", slug="rollback-org-2")
+        modified_base = timezone.now()
+
+        # Create a weaker existing global (method="email")
+        existing_global = RegisteredUser.objects.create(
+            user=user,
+            organization=None,
+            is_verified=True,
+            method="email",
+        )
+        RegisteredUser.objects.filter(pk=existing_global.pk).update(
+            modified=modified_base
+        )
+        existing_global.refresh_from_db()
+
+        # Create org-scoped email (same strength as global but newer)
+        org_email = RegisteredUser.objects.create(
+            user=user,
+            organization=org1,
+            is_verified=True,
+            method="email",
+        )
+        RegisteredUser.objects.filter(pk=org_email.pk).update(
+            modified=modified_base + timezone.timedelta(minutes=10)
+        )
+        org_email.refresh_from_db()
+
+        # Create org-scoped mobile (strongest due to method priority)
+        org_mobile = RegisteredUser.objects.create(
+            user=user,
+            organization=org2,
+            is_verified=True,
+            method="mobile_phone",
+        )
+        RegisteredUser.objects.filter(pk=org_mobile.pk).update(
+            modified=modified_base - timezone.timedelta(minutes=10)
+        )
+        org_mobile.refresh_from_db()
+
+        # Rollback: should migrate strongest org-scoped (mobile_phone) to global
+        migrate_registered_users_multitenant_reverse(
+            apps, None, app_label="openwisp_radius"
+        )
+
+        existing_global.refresh_from_db()
+        self.assertIsNone(existing_global.organization)
+        self.assertEqual(existing_global.method, "mobile_phone")
+        self.assertTrue(existing_global.is_verified)
+        self.assertEqual(existing_global.modified, org_mobile.modified)
+        self.assertEqual(
+            RegisteredUser.objects.filter(
+                user=user, organization__isnull=False
+            ).count(),
+            0,
+        )
+
+    def test_multitenant_reverse_keeps_stronger_existing_global(self):
+        """
+        Test that during migration rollback, if an existing global
+        RegisteredUser is stronger than all org-scoped candidates,
+        it is left unchanged and org-scoped rows are still cleaned up.
+        """
+        user = self._create_user(username="rollback-global-wins")
+        org = self._create_org(name="rollback-org-3", slug="rollback-org-3")
+        modified_base = timezone.now()
+
+        # Create a stronger existing global (method="mobile_phone", newer timestamp)
+        existing_global = RegisteredUser.objects.create(
+            user=user,
+            organization=None,
+            is_verified=True,
+            method="mobile_phone",
+        )
+        RegisteredUser.objects.filter(pk=existing_global.pk).update(
+            modified=modified_base + timezone.timedelta(minutes=10)
+        )
+        existing_global.refresh_from_db()
+
+        # Create weaker org-scoped (method="social_login", older timestamp)
+        org_specific = RegisteredUser.objects.create(
+            user=user,
+            organization=org,
+            is_verified=True,
+            method="social_login",
+        )
+        RegisteredUser.objects.filter(pk=org_specific.pk).update(modified=modified_base)
+        org_specific.refresh_from_db()
+
+        # Rollback: global should remain unchanged (stronger), org-scoped deleted
+        migrate_registered_users_multitenant_reverse(
+            apps, None, app_label="openwisp_radius"
+        )
+
+        existing_global.refresh_from_db()
+        self.assertIsNone(existing_global.organization)
+        self.assertEqual(existing_global.method, "mobile_phone")
+        self.assertTrue(existing_global.is_verified)
+        self.assertEqual(
+            existing_global.modified,
+            modified_base + timezone.timedelta(minutes=10),
+        )
+        self.assertEqual(
+            RegisteredUser.objects.filter(
+                user=user, organization__isnull=False
+            ).count(),
+            0,
+        )