Merge pull request #2635 from niklasmohrin/json-import-idempotent

Make JSONImporter pass assert_no_database_modifications on nop runs

Author: niklasmohrin

evap/cms/json_importer.py

@@ -479,9 +479,13 @@ def _import_evaluation(  # noqa: PLR0912, PLR0915
                 wait_for_grade_upload_before_publishing = True
             else:
                 wait_for_grade_upload_before_publishing = any(grade["scale"] for grade in data["courses"])
-            course.evaluations.all().update(
+
+            if course.evaluations.exclude(
                 wait_for_grade_upload_before_publishing=wait_for_grade_upload_before_publishing
-            )
+            ).exists():
+                course.evaluations.all().update(
+                    wait_for_grade_upload_before_publishing=wait_for_grade_upload_before_publishing
+                )
 
             is_rewarded = False
         else:

evap/cms/tests/test_json_importer.py

@@ -1,4 +1,5 @@
 import json
+from contextlib import nullcontext
 from copy import deepcopy
 from datetime import date, datetime, timedelta
 from pathlib import Path
@@ -23,6 +24,7 @@
     UserProfile,
 )
 from evap.evaluation.models_logging import LogEntry
+from evap.evaluation.tests.tools import assert_no_database_modifications
 
 EXAMPLE_DATA = json.loads(
     Path(evap.cms.fixtures.__file__).with_name("import_example_data.json").read_text(encoding="utf-8")
@@ -313,17 +315,21 @@ class TestImportEvents(TestCase):
     def setUpTestData(cls):
         cls.semester = baker.make(Semester)
 
-    def _import(self, data=None):
+    def _import(self, data=None, assert_nop=False):
         if not data:
             data = EXAMPLE_DATA
         data = json.dumps(data)
-        importer = JSONImporter(self.semester, date(2000, 1, 1))
-        importer.import_json(data)
+
+        cm = assert_no_database_modifications() if assert_nop else nullcontext()
+        with cm:
+            importer = JSONImporter(self.semester, date(2000, 1, 1))
+            importer.import_json(data)
         return importer
 
     @override_settings(EXAM_EVALUATION_DEFAULT_DURATION=timedelta(days=3))
     def test_import_courses(self):
         importer = self._import()
+        self._import(assert_nop=True)
 
         course = Course.objects.get()
 
@@ -632,11 +638,7 @@ def test_import_courses_evaluation_not_new(self):
         evaluation.course.name_en = "Change"
         evaluation.course.save()
 
-        importer = self._import()
-
-        evaluation = Evaluation.objects.get(pk=evaluation.pk)
-        self.assertFalse(evaluation.is_rewarded)
-        self.assertEqual(evaluation.course.name_en, "Change")
+        importer = self._import(assert_nop=True)
         self.assertEqual(len(importer.statistics.attempted_evaluation_changes), 1)
         self.assertEqual(len(importer.statistics.attempted_course_changes), 1)
 
@@ -663,12 +665,8 @@ def test_import_courses_evaluation_approved(self):
         evaluation.is_rewarded = False
         evaluation.save()
 
-        importer = self._import()
-
-        evaluation = Evaluation.objects.get(pk=evaluation.pk)
-        self.assertFalse(evaluation.is_rewarded)
+        importer = self._import(assert_nop=True)
         self.assertEqual(len(importer.statistics.attempted_evaluation_changes), 1)
-        self.assertEqual(evaluation.participants.count(), 2)
 
         evaluation.participants.clear()
         evaluation = Evaluation.objects.get(pk=evaluation.pk)
@@ -695,12 +693,10 @@ def test_import_courses_evaluation_running(self):
 
         self.assertEqual(evaluation.participants.count(), 0)
 
-        importer = self._import()
-
+        importer = self._import(assert_nop=True)
         self.assertEqual(len(importer.statistics.attempted_evaluation_changes), 1)
         self.assertEqual(len(importer.statistics.updated_participants), 0)
         self.assertEqual(len(importer.statistics.attempted_participant_changes), 1)
-        self.assertEqual(evaluation.participants.count(), 0)
 
     def test_import_courses_update(self):
         self._import()
@@ -740,7 +736,8 @@ def test_importer_log_email_sent_no_recipients(self):
     def test_importer_wrong_data(self):
         wrong_data = deepcopy(EXAMPLE_DATA)
         wrong_data["events"][0]["isexam"] = "false"
-        with self.assertRaises(ValidationError):
+        # Note: not using assert_nop because the assert_no_database_modifications should include the assertRaises
+        with assert_no_database_modifications(), self.assertRaises(ValidationError):
             self._import(wrong_data)
 
         data_with_additional_attribute = deepcopy(EXAMPLE_DATA)

evap/evaluation/tests/tools.py

@@ -272,26 +272,27 @@ def assert_no_database_modifications(*args, **kwargs):
     assert len(connections.all()) == 1, "Found more than one connection, so the decorator might monitor the wrong one"
 
     # may be extended with other non-modifying verbs
-    allowed_prefixes = ["select", "savepoint", "release savepoint"]
+    allowed_prefixes = ["select", "savepoint", "release savepoint", "rollback to savepoint"]
 
     conn = connections[DEFAULT_DB_ALIAS]
     with CaptureQueriesContext(conn):
-        yield
-
-        for query in conn.queries_log:
-            if (
-                query["sql"].startswith('INSERT INTO "testing_cache_sessions"')
-                or query["sql"].startswith('UPDATE "testing_cache_sessions"')
-                or query["sql"].startswith('DELETE FROM "testing_cache_sessions"')
-                or query["sql"].startswith('UPDATE "evaluation_userprofile" SET "last_login" = ')
-            ):
-                # These queries are caused by interacting with the test-app (self.app.get()), since that opens a session.
-                # That's not what we want to test for here
-                continue
-
-            lower_sql = query["sql"].lower()
-            if not any(lower_sql.startswith(prefix) for prefix in allowed_prefixes):
-                raise AssertionError("Unexpected modifying query found: " + query["sql"])
+        try:
+            yield
+        finally:
+            for query in conn.queries_log:
+                if (
+                    query["sql"].startswith('INSERT INTO "testing_cache_sessions"')
+                    or query["sql"].startswith('UPDATE "testing_cache_sessions"')
+                    or query["sql"].startswith('DELETE FROM "testing_cache_sessions"')
+                    or query["sql"].startswith('UPDATE "evaluation_userprofile" SET "last_login" = ')
+                ):
+                    # These queries are caused by interacting with the test-app (self.app.get()), since that opens a session.
+                    # That's not what we want to test for here
+                    continue
+
+                lower_sql = query["sql"].lower()
+                if not any(lower_sql.startswith(prefix) for prefix in allowed_prefixes):
+                    raise AssertionError("Unexpected modifying query found: " + query["sql"])
 
 
 class LiveServerTest(SeleniumTestCase):