Merge pull request #11552 from bjester/insufficient-logic

Fix issues with insufficient storage status handling

kolibri/core/content/migrations/0033_content_requests.py

@@ -42,16 +42,16 @@ class Migration(migrations.Migration):
                 (
                     "type",
                     models.CharField(
-                        choices=[("Download", "DOWNLOAD"), ("Removal", "REMOVAL")],
+                        choices=[("DOWNLOAD", "Download"), ("REMOVAL", "Removal")],
                         max_length=8,
                     ),
                 ),
                 (
                     "reason",
                     models.CharField(
                         choices=[
-                            ("SyncInitiated", "SYNC_INITIATED"),
-                            ("UserInitiated", "USER_INITIATED"),
+                            ("SYNC_INITIATED", "SyncInitiated"),
+                            ("USER_INITIATED", "UserInitiated"),
                         ],
                         max_length=14,
                     ),
@@ -60,10 +60,10 @@ class Migration(migrations.Migration):
                     "status",
                     models.CharField(
                         choices=[
-                            ("Completed", "COMPLETED"),
-                            ("Failed", "FAILED"),
-                            ("InProgress", "IN_PROGRESS"),
-                            ("Pending", "PENDING"),
+                            ("COMPLETED", "Completed"),
+                            ("FAILED", "Failed"),
+                            ("IN_PROGRESS", "InProgress"),
+                            ("PENDING", "Pending"),
                         ],
                         max_length=11,
                     ),

kolibri/core/content/test/utils/test_content_request.py

@@ -932,6 +932,23 @@ def test_basic__admin_imported(self):
         self.assertEqual(self.qs.count(), 0)
         self.assertEqual(ContentDownloadRequest.objects.count(), 1)
 
+    def test_basic__has_other_download(self):
+        other_download = ContentDownloadRequest(
+            contentnode_id=self.download.contentnode_id,
+            reason=ContentRequestReason.SyncInitiated,
+            status=ContentRequestStatus.Completed,
+            source_model="test",
+            source_id=uuid.uuid4().hex,
+            facility=self.facility,
+        )
+        other_download.save()
+
+        process_content_removal_requests(self.qs)
+        self.mock_call_command.assert_not_called()
+        # should be marked completed
+        self.assertEqual(self.qs.count(), 0)
+        self.assertEqual(ContentDownloadRequest.objects.count(), 2)
+
 
 class ProcessDownloadRequestTestCase(BaseQuerysetTestCase):
     def setUp(self):

kolibri/core/content/utils/content_request.py

@@ -8,6 +8,7 @@
 from django.db.models import Case
 from django.db.models import Exists
 from django.db.models import OuterRef
+from django.db.models import Q
 from django.db.models import QuerySet
 from django.db.models import Subquery
 from django.db.models import Sum
@@ -94,6 +95,7 @@ def create_content_download_requests(facility, assignments, source_instance_id=N
         # delete any related removal requests
         related_removals.delete()
 
+        logger.debug("Creating content download request for {}".format(assignment))
         ContentDownloadRequest.objects.get_or_create(
             defaults=dict(
                 facility_id=facility.id,
@@ -131,6 +133,9 @@ def create_content_removal_requests(facility, removable_assignments):
             removed_contentnode_ids = [assignment.contentnode_id]
 
         for contentnode_id in removed_contentnode_ids:
+            logger.debug(
+                "Creating content removal request for {}".format(contentnode_id)
+            )
             ContentRemovalRequest.objects.get_or_create(
                 defaults=dict(
                     facility_id=facility.id,
@@ -190,10 +195,11 @@ def synchronize_content_requests(dataset_id, transfer_session=None):
             else sync_session.server_instance_id
         )
 
+    # process removals first
+    create_content_removal_requests(facility, removable_assignments)
     create_content_download_requests(
         facility, assignments, source_instance_id=source_instance_id
     )
-    create_content_removal_requests(facility, removable_assignments)
 
 
 class PreferredDevices(object):
@@ -654,8 +660,8 @@ def incomplete_removals_queryset():
         )
         .exclude(
             # hoping using exclude creates SQL like `NOT EXISTS`
-            has_other_download=True,
-            is_admin_imported=True,
+            Q(has_other_download=True)
+            | Q(is_admin_imported=True)
         )
         .order_by("requested_at")
     )
@@ -941,6 +947,10 @@ def process_content_removal_requests(queryset):
         contentnode_ids = list(
             removable_nodes.filter(channel_id=channel_id).values_list("id", flat=True)
         )
+        # if we somehow have no contentnode_ids, skip, because the deletecontent command will
+        # delete all content for the channel if no node ids are passed
+        if not contentnode_ids:
+            continue
         # queryset unfiltered by status
         channel_requests = ContentRemovalRequest.objects.filter(
             id__in=list(

kolibri/core/device/migrations/0019_syncqueue_and_status.py

@@ -35,13 +35,13 @@ class Migration(migrations.Migration):
             name="status",
             field=models.CharField(
                 choices=[
-                    ("Ineligible", "INELIGIBLE"),
-                    ("Pending", "PENDING"),
-                    ("Queued", "QUEUED"),
-                    ("Ready", "READY"),
-                    ("Stale", "STALE"),
-                    ("Syncing", "SYNCING"),
-                    ("Unavailable", "UNAVAILABLE"),
+                    ("INELIGIBLE", "Ineligible"),
+                    ("PENDING", "Pending"),
+                    ("QUEUED", "Queued"),
+                    ("READY", "Ready"),
+                    ("STALE", "Stale"),
+                    ("SYNCING", "Syncing"),
+                    ("UNAVAILABLE", "Unavailable"),
                 ],
                 default="PENDING",
                 max_length=20,
@@ -57,13 +57,13 @@ class Migration(migrations.Migration):
             name="status",
             field=models.CharField(
                 choices=[
-                    ("Ineligible", "INELIGIBLE"),
-                    ("Pending", "PENDING"),
-                    ("Queued", "QUEUED"),
-                    ("Ready", "READY"),
-                    ("Stale", "STALE"),
-                    ("Syncing", "SYNCING"),
-                    ("Unavailable", "UNAVAILABLE"),
+                    ("INELIGIBLE", "Ineligible"),
+                    ("PENDING", "Pending"),
+                    ("QUEUED", "Queued"),
+                    ("READY", "Ready"),
+                    ("STALE", "Stale"),
+                    ("SYNCING", "Syncing"),
+                    ("UNAVAILABLE", "Unavailable"),
                 ],
                 default="PENDING",
                 max_length=20,

kolibri/core/device/migrations/0020_fix_learner_device_status_choices.py

@@ -0,0 +1,23 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2023-11-21 15:08
+from __future__ import unicode_literals
+
+from django.db import migrations
+from django.db import models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("device", "0019_syncqueue_and_status"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="learnerdevicestatus",
+            name="status_sentiment",
+            field=models.IntegerField(
+                choices=[(-1, "Negative"), (0, "Neutral"), (1, "Positive")]
+            ),
+        ),
+    ]

kolibri/core/device/models.py

@@ -661,7 +661,7 @@ def save_learner_status(cls, learner_user_id, status):
         instance_model = InstanceIDModel.get_or_create_current_instance()[0]
 
         # in order to save a status, it must be defined
-        if not any(status == choice for _, choice in DeviceStatus.choices()):
+        if not any(status == choice for choice, _ in DeviceStatus.choices()):
             raise ValueError("Value '{}' is not a valid status".format(status[0]))
 
         cls.objects.update_or_create(

kolibri/core/device/test/test_models.py

@@ -120,7 +120,7 @@ def test_save_learner_status__updated(self):
         test_status = ("TestStatus", StatusSentiment.Positive)
 
         with mock.patch.object(DeviceStatus, "choices") as mock_choices:
-            mock_choices.return_value = [(test_status[0], test_status)]
+            mock_choices.return_value = [(test_status, test_status[0])]
             LearnerDeviceStatus.save_learner_status(self.user.id, test_status)
 
         self.assertFalse(
@@ -176,7 +176,12 @@ def test_morango_fields(self):
         LearnerDeviceStatus.save_learner_status(
             self.user.id, DeviceStatus.InsufficientStorage
         )
+
         device_status = LearnerDeviceStatus.objects.get(user=self.user)
+        # this is called after deserializing during a sync, so make sure that a model can be
+        # successfully saved after deserializing
+        device_status.full_clean()
+
         self.assertEqual(self.facility.dataset_id, device_status.dataset_id)
         self.assertEqual(
             "{}:{}".format(self.instance.id, self.user.id),

kolibri/core/discovery/migrations/0009_add_location_type.py

@@ -18,9 +18,9 @@ class Migration(migrations.Migration):
             name="location_type",
             field=models.CharField(
                 choices=[
-                    ("Dynamic", "dynamic"),
-                    ("Reserved", "reserved"),
-                    ("Static", "static"),
+                    ("dynamic", "Dynamic"),
+                    ("reserved", "Reserved"),
+                    ("static", "Static"),
                 ],
                 default="static",
                 max_length=8,

kolibri/core/notifications/migrations/0006_fix_choices.py

@@ -0,0 +1,39 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2023-11-21 15:03
+from __future__ import unicode_literals
+
+from django.db import migrations
+from django.db import models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("notifications", "0005_learnerprogressnotification_assignment_collections"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="learnerprogressnotification",
+            name="notification_event",
+            field=models.CharField(
+                blank=True,
+                choices=[
+                    ("Answered", "Answered"),
+                    ("Completed", "Completed"),
+                    ("HelpNeeded", "Help"),
+                    ("Started", "Started"),
+                ],
+                max_length=200,
+            ),
+        ),
+        migrations.AlterField(
+            model_name="learnerprogressnotification",
+            name="reason",
+            field=models.CharField(
+                blank=True,
+                choices=[("MultipleUnsuccessfulAttempts", "Multiple")],
+                max_length=200,
+            ),
+        ),
+    ]

kolibri/utils/data.py

@@ -52,10 +52,10 @@ class ChoicesEnum(object):
     @classmethod
     def choices(cls):
         choices_list = [
-            ("{}".format(m), getattr(cls, m)) for m in cls.__dict__ if m[0] != "_"
+            (getattr(cls, m), "{}".format(m)) for m in cls.__dict__ if m[0] != "_"
         ]
         return tuple(sorted(choices_list))
 
     @classmethod
     def max_length(cls):
-        return max(len(getattr(cls, m)) for m in cls.__dict__ if m[0] != "_")
+        return max(len(str(getattr(cls, m))) for m in cls.__dict__ if m[0] != "_")

kolibri/utils/tests/test_data.py

@@ -6,6 +6,7 @@
 
 from kolibri.utils.data import bytes_for_humans
 from kolibri.utils.data import bytes_from_humans
+from kolibri.utils.data import ChoicesEnum
 
 
 class BytesForHumans(TestCase):
@@ -81,3 +82,14 @@ def test_petabytes_lower_case(self):
             bytes_from_humans("611.77PB".lower()),
             611.77 * 1000 * 1000 * 1000 * 1000 * 1000,
         )
+
+
+class IntegerChoices(ChoicesEnum):
+    ONE = 1
+    TWO = 2
+    THREE = 3
+
+
+def test_choices_enum():
+    assert IntegerChoices.choices() == ((1, "ONE"), (2, "TWO"), (3, "THREE"))
+    assert IntegerChoices.max_length() == 1