Handle mismatching datasets for exams and lessons to prevent syncing issues

kolibri/core/auth/models.py

@@ -261,15 +261,17 @@ def full_clean(self, *args, **kwargs):
         )
         super(AbstractFacilityDataModel, self).full_clean(*args, **kwargs)
 
-    def save(self, *args, **kwargs):
-
-        # before saving, ensure we have a dataset, and convert any validation errors into integrity errors,
-        # since by this point the `clean_fields` method should already have prevented this situation from arising
+    def pre_save(self):
+        # before saving, ensure we have a dataset, and convert any validation errors into integrity
+        # errors, since by this point the `clean_fields` method should already have prevented
+        # this situation from arising
         try:
             self.ensure_dataset()
         except KolibriValidationError as e:
             raise IntegrityError(str(e))
 
+    def save(self, *args, **kwargs):
+        self.pre_save()
         super(AbstractFacilityDataModel, self).save(*args, **kwargs)
 
     def ensure_dataset(self, *args, **kwargs):

kolibri/core/auth/test/test_datasets.py

@@ -1,6 +1,8 @@
 """
 Tests related specifically to the FacilityDataset model.
 """
+import uuid
+
 from django.db.utils import IntegrityError
 from django.test import TestCase
 
@@ -10,26 +12,44 @@
 from ..models import FacilityDataset
 from ..models import FacilityUser
 from ..models import LearnerGroup
+from kolibri.core.auth.constants import role_kinds
 from kolibri.core.exams.models import Exam
+from kolibri.core.exams.models import ExamAssignment
 from kolibri.core.lessons.models import Lesson
+from kolibri.core.lessons.models import LessonAssignment
+
+
+def _create_classroom_data():
+    facility = Facility.objects.create()
+    classroom = Classroom.objects.create(parent=facility)
+    learner_group = LearnerGroup.objects.create(parent=classroom)
+    coach = FacilityUser.objects.create(
+        username="coach_" + uuid.uuid4().hex[:10],
+        password="password",
+        facility=facility,
+    )
+    facility.add_role(coach, role_kinds.COACH)
+    exam = Exam.objects.create(
+        title="", question_count=1, collection=facility, creator=coach
+    )
+    lesson = Lesson.objects.create(
+        created_by=coach, title="lesson", collection=facility
+    )
+    return (facility, classroom, learner_group, coach, exam, lesson)
 
 
 class FacilityDatasetTestCase(TestCase):
     @classmethod
     def setUpTestData(cls):
-        cls.facility = Facility.objects.create()
+        (
+            cls.facility,
+            cls.classroom,
+            cls.learner_group,
+            cls.coach,
+            cls.exam,
+            cls.lesson,
+        ) = _create_classroom_data()
         cls.facility_2 = Facility.objects.create()
-        cls.classroom = Classroom.objects.create(parent=cls.facility)
-        cls.learner_group = LearnerGroup.objects.create(parent=cls.classroom)
-        cls.user = FacilityUser.objects.create(
-            username="user", password="password", facility=cls.facility
-        )
-        cls.exam = Exam.objects.create(
-            title="", question_count=1, collection=cls.facility, creator=cls.user
-        )
-        cls.lesson = Lesson.objects.create(
-            created_by=cls.user, title="lesson", collection=cls.facility
-        )
 
     def setUp(self):
         self.facility_user = FacilityUser.objects.create(
@@ -114,3 +134,165 @@ def test_dataset_incompatible_setting(self):
             FacilityDataset.objects.create(
                 learner_can_edit_password=True, learner_can_login_with_no_password=True
             )
+
+
+class MultipleDatasetAssignmentTestCase(TestCase):
+    """
+    Tests enforcement of correct dataset for creation of lessons, exams, and their assignments,
+    which involves related models and ensuring those are not in the wrong dataset
+    """
+
+    @classmethod
+    def setUpTestData(cls):
+        (
+            cls.facility_1,
+            cls.classroom_1,
+            cls.learner_group_1,
+            cls.coach_1,
+            cls.exam_1,
+            cls.lesson_1,
+        ) = _create_classroom_data()
+        (
+            cls.facility_2,
+            cls.classroom_2,
+            cls.learner_group_2,
+            cls.coach_2,
+            cls.exam_2,
+            cls.lesson_2,
+        ) = _create_classroom_data()
+
+        cls.super_1 = FacilityUser.objects.create_superuser(
+            "super_1",
+            "password",
+            facility=cls.facility_1,
+        )
+        cls.super_2 = FacilityUser.objects.create_superuser(
+            "super_2",
+            "password",
+            facility=cls.facility_2,
+        )
+
+    def assertMatchingDatasets(self, expected_obj, actual_obj):
+        self.assertEqual(expected_obj.dataset_id, actual_obj.infer_dataset())
+        self.assertEqual(expected_obj.dataset_id, actual_obj.dataset_id)
+
+    def test_exam_creation_requires_creator_assigner(self):
+        with self.assertRaises(IntegrityError):
+            Exam.objects.create(
+                title="",
+                question_count=1,
+                collection=self.classroom_1,
+            )
+        with self.assertRaises(IntegrityError):
+            ExamAssignment.objects.create(
+                exam=self.exam_1,
+                collection=self.learner_group_1,
+            )
+
+    def test_exam_creation_across_facilities_by_superuser(self):
+        exam = Exam.objects.create(
+            title="",
+            question_count=1,
+            collection=self.classroom_1,
+            creator=self.super_2,
+        )
+        exam_assignment = ExamAssignment.objects.create(
+            exam=exam, collection=self.learner_group_1, assigned_by=self.super_2
+        )
+        self.assertMatchingDatasets(self.facility_1, exam)
+        self.assertIsNone(exam.creator)
+        self.assertMatchingDatasets(self.facility_1, exam_assignment)
+        self.assertIsNone(exam_assignment.assigned_by)
+
+    def test_exam_creation_across_facilities_by_non_superuser(self):
+        # fails as coach is in wrong facility
+        with self.assertRaises(IntegrityError):
+            Exam.objects.create(
+                title="",
+                question_count=1,
+                collection=self.classroom_1,
+                creator=self.coach_2,
+            )
+        # fails as coach is in wrong facility
+        with self.assertRaises(IntegrityError):
+            ExamAssignment.objects.create(
+                exam=self.exam_1,
+                collection=self.learner_group_1,
+                assigned_by=self.coach_2,
+            )
+        exam_assignment = ExamAssignment.objects.create(
+            exam=self.exam_1, collection=self.learner_group_1, assigned_by=self.coach_1
+        )
+        self.assertMatchingDatasets(self.facility_1, self.exam_1)
+        self.assertEqual(self.coach_1, self.exam_1.creator)
+        self.assertMatchingDatasets(self.facility_1, exam_assignment)
+        self.assertEqual(self.coach_1, exam_assignment.assigned_by)
+
+    def test_exam_assignment_dataset_mismatch(self):
+        # fails as exam is in different facility
+        with self.assertRaises(IntegrityError):
+            ExamAssignment.objects.create(
+                exam=self.exam_1,
+                collection=self.learner_group_2,
+                assigned_by=self.coach_2,
+            )
+
+    def test_lesson_creation_requires_creator_assigner(self):
+        with self.assertRaises(IntegrityError):
+            Lesson.objects.create(
+                title="lesson",
+                collection=self.classroom_2,
+            )
+        with self.assertRaises(IntegrityError):
+            LessonAssignment.objects.create(
+                lesson=self.lesson_2,
+                collection=self.learner_group_2,
+            )
+
+    def test_lesson_creation_across_facilities_by_superuser(self):
+        lesson = Lesson.objects.create(
+            title="lesson",
+            collection=self.classroom_2,
+            created_by=self.super_1,
+        )
+        lesson_assignment = LessonAssignment.objects.create(
+            lesson=lesson, collection=self.learner_group_2, assigned_by=self.super_1
+        )
+        self.assertMatchingDatasets(self.facility_2, lesson)
+        self.assertIsNone(lesson.created_by)
+        self.assertMatchingDatasets(self.facility_2, lesson_assignment)
+        self.assertIsNone(lesson_assignment.assigned_by)
+
+    def test_lesson_creation_across_facilities_by_non_superuser(self):
+        # fails as coach is in wrong facility
+        with self.assertRaises(IntegrityError):
+            Lesson.objects.create(
+                title="lesson",
+                collection=self.classroom_2,
+                created_by=self.coach_1,
+            )
+        # fails as coach is in wrong facility
+        with self.assertRaises(IntegrityError):
+            LessonAssignment.objects.create(
+                lesson=self.lesson_2,
+                collection=self.learner_group_2,
+                assigned_by=self.coach_1,
+            )
+        lesson_assignment = LessonAssignment.objects.create(
+            lesson=self.lesson_2,
+            collection=self.learner_group_2,
+            assigned_by=self.coach_2,
+        )
+        self.assertMatchingDatasets(self.facility_2, self.lesson_2)
+        self.assertEqual(self.coach_2, self.lesson_2.created_by)
+        self.assertMatchingDatasets(self.facility_2, lesson_assignment)
+        self.assertEqual(self.coach_2, lesson_assignment.assigned_by)
+
+    def test_lesson_assignment_dataset_mismatch(self):
+        # fails as lesson is in different facility
+        with self.assertRaises(IntegrityError):
+            LessonAssignment.objects.create(
+                lesson=self.lesson_1,
+                collection=self.learner_group_2,
+                assigned_by=self.coach_2,
+            )

kolibri/core/content/utils/annotation.py

@@ -344,7 +344,7 @@ def set_leaf_node_availability_from_local_file_availability(
             )
         )
         # Only look at files that are required (not supplementary)
-        .where(FileTable.c.supplementary == False)
+        .where(FileTable.c.supplementary == False)  # noqa
         # Correlate between the contentnode id and the foreign key
         # to the content node on the file table to complete the
         # many to many lookup

kolibri/core/exams/migrations/0006_nullable_creator_assigned_by.py

@@ -0,0 +1,38 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2021-10-19 20:51
+from __future__ import unicode_literals
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations
+from django.db import models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("exams", "0005_individualsyncableexam"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="exam",
+            name="creator",
+            field=models.ForeignKey(
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="exams",
+                to=settings.AUTH_USER_MODEL,
+            ),
+        ),
+        migrations.AlterField(
+            model_name="examassignment",
+            name="assigned_by",
+            field=models.ForeignKey(
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="assigned_exams",
+                to=settings.AUTH_USER_MODEL,
+            ),
+        ),
+    ]

kolibri/core/exams/models.py

@@ -1,4 +1,5 @@
 from django.db import models
+from django.db.utils import IntegrityError
 from django.utils import timezone
 
 from .permissions import UserCanReadExamAssignmentData
@@ -75,7 +76,7 @@ class Exam(AbstractFacilityDataModel):
     question_sources = JSONField(default=[], blank=True)
 
     """
-    This field is interpretted differently depending on the 'data_model_version' field.
+    This field is interpreted differently depending on the 'data_model_version' field.
 
     V1:
         Used to help select new questions from exercises at quiz creation time
@@ -101,7 +102,7 @@ class Exam(AbstractFacilityDataModel):
         Collection, related_name="exams", blank=False, null=False
     )
     creator = models.ForeignKey(
-        FacilityUser, related_name="exams", blank=False, null=False
+        FacilityUser, related_name="exams", blank=False, null=True
     )
 
     # To be set True when the quiz is first set to active=True
@@ -120,6 +121,21 @@ def delete(self, using=None, keep_parents=False):
         LearnerProgressNotification.objects.filter(quiz_id=self.id).delete()
         super(Exam, self).delete(using, keep_parents)
 
+    def pre_save(self):
+        super(Exam, self).pre_save()
+
+        # maintain stricter enforcement on when creator is allowed to be null
+        if self._state.adding and self.creator is None:
+            raise IntegrityError("Exam must be saved with an creator")
+
+        # validate that datasets match so this would be syncable
+        if self.creator and self.creator.dataset_id != self.dataset_id:
+            # the only time creator can be null is if it's a superuser
+            # and if we set it to none HERE
+            if not self.creator.is_superuser:
+                raise IntegrityError("Exam must have creator in the same dataset")
+            self.creator = None
+
     def save(self, *args, **kwargs):
         # If archive is True during the save op, but there is no date_archived then
         # this is the save that is archiving the object and we need to datestamp it
@@ -173,11 +189,41 @@ class ExamAssignment(AbstractFacilityDataModel):
         Collection, related_name="assigned_exams", blank=False, null=False
     )
     assigned_by = models.ForeignKey(
-        FacilityUser, related_name="assigned_exams", blank=False, null=False
+        FacilityUser, related_name="assigned_exams", blank=False, null=True
     )
 
+    def pre_save(self):
+        super(ExamAssignment, self).pre_save()
+
+        # this shouldn't happen
+        if (
+            self.exam
+            and self.collection
+            and self.exam.dataset_id != self.collection.dataset_id
+        ):
+            raise IntegrityError(
+                "Exam assignment foreign models must be in same dataset"
+            )
+
+        # maintain stricter enforcement on when assigned_by is allowed to be null
+        # assignments aren't usually updated, but ensure only during creation
+        if self._state.adding and self.assigned_by is None:
+            raise IntegrityError("Exam assignment must be saved with an assigner")
+
+        # validate that datasets match so this would be syncable
+        if self.assigned_by and self.assigned_by.dataset_id != self.dataset_id:
+            # the only time assigned_by can be null is if it's a superuser
+            # and if we set it to none HERE
+            if not self.assigned_by.is_superuser:
+                # maintain stricter enforcement on when assigned_by is allowed to be null
+                raise IntegrityError(
+                    "Exam assignment must have assigner in the same dataset"
+                )
+            self.assigned_by = None
+
     def infer_dataset(self, *args, **kwargs):
-        return self.cached_related_dataset_lookup("assigned_by")
+        # infer from exam so assignments align with exams
+        return self.cached_related_dataset_lookup("exam")
 
     def calculate_source_id(self):
         return "{exam_id}:{collection_id}".format(