Update logic to always preserve client instance id for single user sync.

Update logic to preserve most recent status for full facility sync.
Add tests.

kolibri/core/device/kolibri_plugin.py

@@ -1,5 +1,8 @@
 import time
 
+from django.db.models import OuterRef
+from django.db.models import Subquery
+
 from kolibri.core.auth.hooks import FacilityDataSyncHook
 from kolibri.core.auth.sync_event_hook_utils import get_dataset_id
 from kolibri.core.auth.sync_event_hook_utils import get_user_id_for_single_user_sync
@@ -75,6 +78,8 @@ class LearnerDeviceStatusOperation(KolibriVersionedSyncOperation):
     def downgrade(self, context):
         """
         Delete LearnerDeviceStatus records that might cause an issue when syncing to a previous version.
+        Downgrade only runs when the context is the producer (not the receiver) of the data.
+        For the server in a sync operation, this is when the pull is active, for the client, when a push is active.
         For a single user sync, delete any learner device statuses associated with the single user.
         For a facility sync, delete all learner device statuses for the facility.
         :type context: morango.sync.context.LocalSessionContext
@@ -85,20 +90,26 @@ def downgrade(self, context):
         # if it's not a single user sync, this will be None
         user_id = get_user_id_for_single_user_sync(context)
 
-        # get the instance_id of the remote instance
-        instance_id = (
-            context.sync_session.client_instance_id
-            if context.is_server
-            else context.sync_session.server_instance_id
-        )
-
-        queryset = LearnerDeviceStatus.objects.exclude(instance_id=instance_id)
-
         if user_id is not None:
-            queryset.filter(user=user_id).delete()
+            # get the instance_id of the client instance
+            # as for a single learner sync, the client is the one that we care about the sync status of
+            instance_id = context.sync_session.client_instance_id
+            LearnerDeviceStatus.objects.exclude(instance_id=instance_id).filter(
+                user=user_id
+            ).delete()
         else:
             dataset_id = get_dataset_id(context)
-            queryset.filter(user__dataset_id=dataset_id).delete()
+            # Delete all but the most recent syncs per user for this dataset
+            # As this is a full facility sync, this won't, by itself, be creating any new records
+            # for learner device status, so we should just preserve as much information as we can
+            # while avoiding syncing records that might break older versions.
+            LearnerDeviceStatus.objects.filter(user__dataset_id=dataset_id).exclude(
+                id=Subquery(
+                    LearnerDeviceStatus.objects.filter(user_id=OuterRef("user_id"))
+                    .order_by("-updated_at")
+                    .values("id")[:1]
+                ),
+            ).delete()
 
 
 @register_hook

kolibri/core/device/test/test_sync_operations.py

@@ -0,0 +1,201 @@
+import datetime
+from uuid import uuid4
+
+import mock
+from django.test import TestCase
+from django.utils.timezone import now
+from morango.models.core import InstanceIDModel
+from morango.sync.context import LocalSessionContext
+
+from kolibri.core.auth.models import Facility
+from kolibri.core.auth.models import FacilityUser
+from kolibri.core.device.kolibri_plugin import LearnerDeviceStatusOperation
+from kolibri.core.device.models import DeviceStatus
+from kolibri.core.device.models import LearnerDeviceStatus
+
+
+class LearnerDeviceStatusOperationTestCase(TestCase):
+    def setUp(self):
+        super(LearnerDeviceStatusOperationTestCase, self).setUp()
+        self.facility = Facility.objects.create(name="Test")
+        self.user = FacilityUser.objects.create(username="test", facility=self.facility)
+        self.instance = InstanceIDModel.get_or_create_current_instance()[0]
+        self.other_instance_id = uuid4().hex
+        LearnerDeviceStatus.save_learner_status(
+            self.user.id, DeviceStatus.InsufficientStorage
+        )
+        other_instance_status = LearnerDeviceStatus.objects.create(
+            user=self.user,
+            instance_id=self.other_instance_id,
+            status=DeviceStatus.InsufficientStorage[0],
+            status_sentiment=DeviceStatus.InsufficientStorage[1],
+        )
+        # Do this to bypass the auto_now behaviour
+        LearnerDeviceStatus.objects.filter(id=other_instance_status.id).update(
+            updated_at=now() - datetime.timedelta(days=1)
+        )
+        self.operation = LearnerDeviceStatusOperation()
+        self.context = mock.Mock(spec_set=LocalSessionContext)()
+        self.context.is_server = False
+
+    def _setup_other_user(self):
+        other_user = FacilityUser.objects.create(
+            username="other", facility=self.facility
+        )
+        LearnerDeviceStatus.save_learner_status(
+            other_user.id, DeviceStatus.InsufficientStorage
+        )
+        other_instance_status = LearnerDeviceStatus.objects.create(
+            user=other_user,
+            instance_id=self.other_instance_id,
+            status=DeviceStatus.InsufficientStorage[0],
+            status_sentiment=DeviceStatus.InsufficientStorage[1],
+        )
+        # Do this to bypass the auto_now behaviour
+        LearnerDeviceStatus.objects.filter(id=other_instance_status.id).update(
+            updated_at=now() - datetime.timedelta(days=1)
+        )
+
+    def test_handle__single_user_sync_server__downgrade(self):
+        self.context.is_server = True
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=self.user.id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 1)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                instance_id=self.other_instance_id
+            ).exists()
+        )
+
+    def test_handle__single_user_sync_server__downgrade_other_user(self):
+        self._setup_other_user()
+        self.context.is_server = True
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=self.user.id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 3)
+        self.assertEqual(LearnerDeviceStatus.objects.exclude(user=self.user).count(), 2)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                user=self.user, instance_id=self.other_instance_id
+            ).exists()
+        )
+
+    def test_handle__single_user_sync_client__downgrade(self):
+        self.context.is_server = False
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=self.user.id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 1)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                instance_id=self.other_instance_id
+            ).exists()
+        )
+
+    def test_handle__single_user_sync_client__downgrade_other_user(self):
+        self._setup_other_user()
+        self.context.is_server = False
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=self.user.id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 3)
+        self.assertEqual(LearnerDeviceStatus.objects.exclude(user=self.user).count(), 2)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                user=self.user, instance_id=self.other_instance_id
+            ).exists()
+        )
+
+    def test_handle__full_facility_sync_server__downgrade(self):
+        self.context.is_server = True
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=None,
+        ), mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_dataset_id",
+            return_value=self.facility.dataset_id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 1)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(instance_id=self.instance.id).exists()
+        )
+
+    def test_handle__full_facility_sync_server__downgrade_other_user(self):
+        self._setup_other_user()
+        self.context.is_server = True
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=None,
+        ), mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_dataset_id",
+            return_value=self.facility.dataset_id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 2)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                user=self.user, instance_id=self.instance.id
+            ).exists()
+        )
+        self.assertTrue(
+            LearnerDeviceStatus.objects.exclude(user=self.user)
+            .filter(instance_id=self.instance.id)
+            .exists()
+        )
+
+    def test_handle__full_facility_sync_client__downgrade(self):
+        self.context.is_server = False
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=None,
+        ), mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_dataset_id",
+            return_value=self.facility.dataset_id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 1)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(instance_id=self.instance.id).exists()
+        )
+
+    def test_handle__full_facility_sync_client__downgrade_other_user(self):
+        self._setup_other_user()
+        self.context.is_server = False
+        self.context.sync_session.client_instance_id = self.other_instance_id
+        with mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_user_id_for_single_user_sync",
+            return_value=None,
+        ), mock.patch(
+            "kolibri.core.device.kolibri_plugin.get_dataset_id",
+            return_value=self.facility.dataset_id,
+        ):
+            self.operation.downgrade(self.context)
+        self.assertEqual(LearnerDeviceStatus.objects.count(), 2)
+        self.assertTrue(
+            LearnerDeviceStatus.objects.filter(
+                user=self.user, instance_id=self.instance.id
+            ).exists()
+        )
+        self.assertTrue(
+            LearnerDeviceStatus.objects.exclude(user=self.user)
+            .filter(instance_id=self.instance.id)
+            .exists()
+        )