Ensure versioned database exists on submission create

Author: Jakoma02

contentcuration/contentcuration/models.py

@@ -2581,6 +2581,9 @@ class CommunityLibrarySubmission(models.Model):
     internal_notes = models.TextField(blank=True, null=True)
 
     def save(self, *args, **kwargs):
+        # Not a top-level import to avoid circular import issues
+        from contentcuration.utils.publish import ensure_versioned_database_exists
+
         # Validate on save that the submission author is an editor of the channel
         # and that the version is not greater than the current channel version.
         # These cannot be expressed as constraints because traversing
@@ -2603,6 +2606,12 @@ def save(self, *args, **kwargs):
                 code="impossibly_high_channel_version",
             )
 
+        if self.pk is None:
+            # When creating a new submission, ensure the channel has a versioned database
+            # (it might not have if the channel was published before versioned databases
+            # were introduced).
+            ensure_versioned_database_exists(self.channel)
+
         super().save(*args, **kwargs)
 
     @classmethod

contentcuration/contentcuration/tests/test_models.py

@@ -549,12 +549,15 @@ def test_make_content_id_unique(self):
         self.assertNotEqual(copied_node_old_content_id, copied_node.content_id)
 
 
+@mock.patch(
+    "contentcuration.utils.publish.ensure_versioned_database_exists", return_value=None
+)
 class CommunityLibrarySubmissionTestCase(PermissionQuerysetTestCase):
     @property
     def base_queryset(self):
         return CommunityLibrarySubmission.objects.all()
 
-    def test_create_submission(self):
+    def test_create_submission(self, mock_ensure_db_exists):
         # Smoke test
         channel = testdata.channel()
         author = testdata.user()
@@ -577,51 +580,69 @@ def test_create_submission(self):
         submission.full_clean()
         submission.save()
 
-    def test_save__author_not_editor(self):
+    def test_save__author_not_editor(self, mock_ensure_db_exists):
         submission = testdata.community_library_submission()
         user = testdata.user("some@email.com")
         submission.author = user
         with self.assertRaises(ValidationError):
             submission.save()
 
-    def test_save__nonpositive_channel_version(self):
+    def test_save__nonpositive_channel_version(self, mock_ensure_db_exists):
         submission = testdata.community_library_submission()
         submission.channel_version = 0
         with self.assertRaises(ValidationError):
             submission.save()
 
-    def test_save__matching_channel_version(self):
+    def test_save__matching_channel_version(self, mock_ensure_db_exists):
         submission = testdata.community_library_submission()
         submission.channel.version = 5
         submission.channel.save()
         submission.channel_version = 5
         submission.save()
 
-    def test_save__impossibly_high_channel_version(self):
+    def test_save__impossibly_high_channel_version(self, mock_ensure_db_exists):
         submission = testdata.community_library_submission()
         submission.channel.version = 5
         submission.channel.save()
         submission.channel_version = 6
         with self.assertRaises(ValidationError):
             submission.save()
 
-    def test_filter_view_queryset__anonymous(self):
+    def test_save__ensure_versioned_database_exists_on_create(
+        self, mock_ensure_db_exists
+    ):
+        submission = testdata.community_library_submission()
+
+        mock_ensure_db_exists.assert_called_once_with(submission.channel)
+
+    def test_save__dont_ensure_versioned_database_exists_on_update(
+        self, mock_ensure_db_exists
+    ):
+        submission = testdata.community_library_submission()
+        mock_ensure_db_exists.reset_mock()
+
+        submission.description = "Updated description"
+        submission.save()
+
+        mock_ensure_db_exists.assert_not_called()
+
+    def test_filter_view_queryset__anonymous(self, mock_ensure_db_exists):
         _ = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_view_queryset(
             self.base_queryset, user=self.anonymous_user
         )
         self.assertFalse(queryset.exists())
 
-    def test_filter_view_queryset__forbidden_user(self):
+    def test_filter_view_queryset__forbidden_user(self, mock_ensure_db_exists):
         _ = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_view_queryset(
             self.base_queryset, user=self.forbidden_user
         )
         self.assertFalse(queryset.exists())
 
-    def test_filter_view_queryset__channel_editor(self):
+    def test_filter_view_queryset__channel_editor(self, mock_ensure_db_exists):
         submission_a = testdata.community_library_submission()
         submission_b = testdata.community_library_submission()
 
@@ -635,31 +656,31 @@ def test_filter_view_queryset__channel_editor(self):
         self.assertQuerysetContains(queryset, pk=submission_a.id)
         self.assertQuerysetDoesNotContain(queryset, pk=submission_b.id)
 
-    def test_filter_view_queryset__admin(self):
+    def test_filter_view_queryset__admin(self, mock_ensure_db_exists):
         submission_a = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_view_queryset(
             self.base_queryset, user=self.admin_user
         )
         self.assertQuerysetContains(queryset, pk=submission_a.id)
 
-    def test_filter_edit_queryset__anonymous(self):
+    def test_filter_edit_queryset__anonymous(self, mock_ensure_db_exists):
         _ = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_edit_queryset(
             self.base_queryset, user=self.anonymous_user
         )
         self.assertFalse(queryset.exists())
 
-    def test_filter_edit_queryset__forbidden_user(self):
+    def test_filter_edit_queryset__forbidden_user(self, mock_ensure_db_exists):
         _ = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_edit_queryset(
             self.base_queryset, user=self.forbidden_user
         )
         self.assertFalse(queryset.exists())
 
-    def test_filter_edit_queryset__channel_editor(self):
+    def test_filter_edit_queryset__channel_editor(self, mock_ensure_db_exists):
         submission = testdata.community_library_submission()
 
         user = testdata.user()
@@ -671,7 +692,7 @@ def test_filter_edit_queryset__channel_editor(self):
         )
         self.assertFalse(queryset.exists())
 
-    def test_filter_edit_queryset__author(self):
+    def test_filter_edit_queryset__author(self, mock_ensure_db_exists):
         submission_a = testdata.community_library_submission()
         submission_b = testdata.community_library_submission()
 
@@ -681,7 +702,7 @@ def test_filter_edit_queryset__author(self):
         self.assertQuerysetContains(queryset, pk=submission_a.id)
         self.assertQuerysetDoesNotContain(queryset, pk=submission_b.id)
 
-    def test_filter_edit_queryset__admin(self):
+    def test_filter_edit_queryset__admin(self, mock_ensure_db_exists):
         submission_a = testdata.community_library_submission()
 
         queryset = CommunityLibrarySubmission.filter_edit_queryset(

contentcuration/contentcuration/tests/utils/test_publish.py

@@ -0,0 +1,92 @@
+import os
+import tempfile
+from unittest import mock
+
+from django.conf import settings
+from django.core.files.storage import FileSystemStorage
+
+from contentcuration.tests import testdata
+from contentcuration.tests.base import StudioTestCase
+from contentcuration.utils.publish import ensure_versioned_database_exists
+
+
+class EnsureVersionedDatabaseTestCase(StudioTestCase):
+    def setUp(self):
+        super().setUp()
+
+        self._temp_directory_ctx = tempfile.TemporaryDirectory()
+        self.test_db_root_dir = self._temp_directory_ctx.__enter__()
+
+        storage = FileSystemStorage(location=self.test_db_root_dir)
+
+        self._storage_patch_ctx = mock.patch(
+            "contentcuration.utils.publish.storage",
+            new=storage,
+        )
+        self._storage_patch_ctx.__enter__()
+
+        os.makedirs(
+            os.path.join(self.test_db_root_dir, settings.DB_ROOT), exist_ok=True
+        )
+
+        self.channel = testdata.channel()
+        self.channel.version = 2
+        self.channel.save()
+
+        self.versioned_db_path = os.path.join(
+            self.test_db_root_dir,
+            settings.DB_ROOT,
+            f"{self.channel.id}-{self.channel.version}.sqlite3",
+        )
+        self.unversioned_db_path = os.path.join(
+            self.test_db_root_dir, settings.DB_ROOT, f"{self.channel.id}.sqlite3"
+        )
+
+    def tearDown(self):
+        self._temp_directory_ctx.__exit__(None, None, None)
+        self._storage_patch_ctx.__exit__(None, None, None)
+
+        super().tearDown()
+
+    def test_versioned_database_exists(self):
+        # In reality, the versioned database for the current version
+        # and the unversioned database would have the same content,
+        # but here we provide different content so that we can test
+        # that the versioned database is not overwritten.
+        versioned_db_content = "Versioned content"
+        unversioned_db_content = "Unversioned content"
+
+        with open(self.versioned_db_path, "w") as f:
+            f.write(versioned_db_content)
+        with open(self.unversioned_db_path, "w") as f:
+            f.write(unversioned_db_content)
+
+        ensure_versioned_database_exists(self.channel)
+
+        with open(self.versioned_db_path) as f:
+            read_versioned_content = f.read()
+        self.assertEqual(read_versioned_content, versioned_db_content)
+
+    def test_versioned_database_does_not_exist(self):
+        unversioned_db_content = "Unversioned content"
+
+        with open(self.unversioned_db_path, "w") as f:
+            f.write(unversioned_db_content)
+
+        ensure_versioned_database_exists(self.channel)
+
+        with open(self.versioned_db_path) as f:
+            read_versioned_content = f.read()
+        self.assertEqual(read_versioned_content, unversioned_db_content)
+
+    def test_not_published(self):
+        self.channel.version = 0
+        self.channel.save()
+        self.versioned_db_path = os.path.join(
+            self.test_db_root_dir,
+            settings.DB_ROOT,
+            f"{self.channel.id}-{self.channel.version}.sqlite3",
+        )
+
+        with self.assertRaises(ValueError):
+            ensure_versioned_database_exists(self.channel)

contentcuration/contentcuration/utils/publish.py

@@ -4,6 +4,7 @@
 import logging as logmodule
 import os
 import re
+import shutil
 import tempfile
 import time
 import traceback
@@ -1368,3 +1369,35 @@ def publish_channel(  # noqa: C901
         except SlowPublishError as e:
             report_exception(e)
     return channel
+
+
+def ensure_versioned_database_exists(channel):
+    """
+    Ensures that the versioned database exists, and if not, copies the unversioned database to the versioned path.
+    This happens if the channel was published back when versioned databases were not used.
+    """
+    if channel.version == 0:
+        raise ValueError("An unpublished channel cannot have a versioned database.")
+
+    unversioned_db_storage_path = os.path.join(
+        settings.DB_ROOT, "{id}.sqlite3".format(id=channel.id)
+    )
+    versioned_db_storage_path = os.path.join(
+        settings.DB_ROOT,
+        "{id}-{version}.sqlite3".format(id=channel.id, version=channel.version),
+    )
+
+    if not storage.exists(versioned_db_storage_path):
+        if not storage.exists(unversioned_db_storage_path):
+            # This should never happen, a published channel should always have an unversioned database
+            raise FileNotFoundError(
+                f"Neither unversioned nor versioned database found for channel {channel.id}."
+            )
+
+        with storage.open(unversioned_db_storage_path, "rb") as unversioned_db_file:
+            with storage.open(versioned_db_storage_path, "wb") as versioned_db_file:
+                shutil.copyfileobj(unversioned_db_file, versioned_db_file)
+
+        logging.info(
+            f"Versioned database for channel {channel.id} did not exist, copied the unversioned database to {versioned_db_storage_path}."
+        )

contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py

@@ -152,7 +152,7 @@ def tearDown(self):
         super().tearDown()
 
     @mock.patch("kolibri_public.utils.export_channel_to_kolibri_public.ChannelMapper")
-    def test_export_channel_to_kolibri_public__existing_version__versioned(
+    def test_export_channel_to_kolibri_public__existing_version(
         self, mock_channel_mapper
     ):
         categories = {
@@ -180,29 +180,6 @@ def test_export_channel_to_kolibri_public__existing_version__versioned(
         )
         mock_channel_mapper.return_value.run.assert_called_once_with()
 
-    @mock.patch("kolibri_public.utils.export_channel_to_kolibri_public.ChannelMapper")
-    def test_export_channel_to_kolibri_public__existing_version__unversioned(
-        self, mock_channel_mapper
-    ):
-        os.remove(self.versioned_db_path)
-
-        export_channel_to_kolibri_public(
-            channel_id=self.channel_id,
-            channel_version=1,
-            public=True,
-            categories=None,
-            countries=None,
-        )
-
-        mock_channel_mapper.assert_called_once_with(
-            channel=self.exported_channel_metadata,
-            channel_version=1,
-            public=True,
-            categories=None,
-            countries=None,
-        )
-        mock_channel_mapper.return_value.run.assert_called_once_with()
-
     @mock.patch("kolibri_public.utils.export_channel_to_kolibri_public.ChannelMapper")
     def test_export_channel_to_kolibri_public__without_version(
         self, mock_channel_mapper

contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py

@@ -75,12 +75,6 @@ def export_channel_to_kolibri_public(
         db_storage_path = unversioned_db_storage_path
     else:
         db_storage_path = versioned_db_storage_path
-        _possibly_migrate_unversioned_database(
-            channel_id=channel_id,
-            channel_version=channel_version,
-            unversioned_db_storage_path=unversioned_db_storage_path,
-            versioned_db_storage_path=versioned_db_storage_path,
-        )
 
     with using_temp_migrated_database(db_storage_path):
         channel = ExportedChannelMetadata.objects.get(id=channel_id)
@@ -95,35 +89,3 @@ def export_channel_to_kolibri_public(
             countries=countries,
         )
         mapper.run()
-
-
-def _possibly_migrate_unversioned_database(
-    channel_id,
-    channel_version,
-    unversioned_db_storage_path,
-    versioned_db_storage_path,
-):
-    """
-    Older channels may only have a single database file and not
-    versioned databases. If this is the case and the requested channel version
-    is present in the single database file, this function copies the database to a file
-    containing the version in the filename.
-    """
-    if storage.exists(versioned_db_storage_path) or not storage.exists(
-        unversioned_db_storage_path
-    ):
-        return
-
-    with using_temp_migrated_database(unversioned_db_storage_path):
-        contains_requested_version = ExportedChannelMetadata.objects.filter(
-            id=channel_id, version=channel_version
-        ).exists()
-
-    if contains_requested_version:
-        logger.info(
-            f"Migrating unversioned database {unversioned_db_storage_path} to versioned database {versioned_db_storage_path}"
-        )
-
-        with storage.open(unversioned_db_storage_path, "rb") as unversioned_db_file:
-            with storage.open(versioned_db_storage_path, "wb") as versioned_db_file:
-                shutil.copyfileobj(unversioned_db_file, versioned_db_file)