Delete imported files when deactivating version (#10684)

* Delete imported files when deactivating version

Closes #10682

* Remove dead code

We aren't using this, and tests were failing because
of the storage class being overridden by other tests.

Author: stsewd

readthedocs/api/v3/tests/test_versions.py

@@ -3,10 +3,11 @@
 import django_dynamic_fixture as fixture
 from django.test import override_settings
 from django.urls import reverse
+from django_dynamic_fixture import get
 
 from readthedocs.builds.constants import EXTERNAL, TAG
 from readthedocs.builds.models import Version
-from readthedocs.projects.models import Project
+from readthedocs.projects.models import HTMLFile, Project
 
 from .mixins import APIEndpointMixin
 
@@ -266,12 +267,33 @@ def test_activate_version(self, clean_project_resources, trigger_build):
         trigger_build.assert_called_once()
 
     @mock.patch("readthedocs.builds.models.trigger_build")
-    @mock.patch("readthedocs.projects.tasks.utils.clean_project_resources")
-    def test_deactivate_version(self, clean_project_resources, trigger_build):
+    @mock.patch("readthedocs.projects.tasks.search.remove_search_indexes")
+    @mock.patch("readthedocs.projects.tasks.utils.remove_build_storage_paths")
+    def test_deactivate_version(
+        self, remove_build_storage_paths, remove_search_indexes, trigger_build
+    ):
+        another_version = get(Version, project=self.project, active=True)
+        get(
+            HTMLFile,
+            project=self.project,
+            version=another_version,
+            name="index.html",
+            path="index.html",
+        )
+        get(
+            HTMLFile,
+            project=self.project,
+            version=self.version,
+            name="index.html",
+            path="index.html",
+        )
+
         self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}")
         data = {"active": False}
         self.assertTrue(self.version.active)
         self.assertTrue(self.version.built)
+        self.assertTrue(another_version.imported_files.exists())
+        self.assertTrue(self.version.imported_files.exists())
         response = self.client.patch(
             reverse(
                 "projects-versions-detail",
@@ -286,7 +308,10 @@ def test_deactivate_version(self, clean_project_resources, trigger_build):
         self.version.refresh_from_db()
         self.assertFalse(self.version.active)
         self.assertFalse(self.version.built)
-        clean_project_resources.assert_called_once()
+        self.assertFalse(self.version.imported_files.exists())
+        self.assertTrue(another_version.imported_files.exists())
+        remove_build_storage_paths.delay.assert_called_once()
+        remove_search_indexes.delay.assert_called_once()
         trigger_build.assert_not_called()
 
     def test_projects_version_external(self):

readthedocs/builds/models.py

@@ -422,8 +422,11 @@ def clean_resources(self):
         """
         Remove all resources from this version.
 
-        This includes removing files from storage,
-        and removing its search index.
+        This includes:
+
+        - Files from storage
+        - Search index
+        - Imported files
         """
         from readthedocs.projects.tasks.utils import clean_project_resources
 

readthedocs/projects/models.py

@@ -54,9 +54,6 @@
 
 from .constants import (
     DOWNLOADABLE_MEDIA_TYPES,
-    MEDIA_TYPE_EPUB,
-    MEDIA_TYPE_HTMLZIP,
-    MEDIA_TYPE_PDF,
     MEDIA_TYPES,
 )
 
@@ -851,34 +848,6 @@ def has_good_build(self):
             return self._good_build
         return self.builds(manager=INTERNAL).filter(success=True).exists()
 
-    def has_media(self, type_, version_slug=LATEST, version_type=None):
-        storage_path = self.get_storage_path(
-            type_=type_, version_slug=version_slug,
-            version_type=version_type
-        )
-        return build_media_storage.exists(storage_path)
-
-    def has_pdf(self, version_slug=LATEST, version_type=None):
-        return self.has_media(
-            MEDIA_TYPE_PDF,
-            version_slug=version_slug,
-            version_type=version_type
-        )
-
-    def has_epub(self, version_slug=LATEST, version_type=None):
-        return self.has_media(
-            MEDIA_TYPE_EPUB,
-            version_slug=version_slug,
-            version_type=version_type
-        )
-
-    def has_htmlzip(self, version_slug=LATEST, version_type=None):
-        return self.has_media(
-            MEDIA_TYPE_HTMLZIP,
-            version_slug=version_slug,
-            version_type=version_type
-        )
-
     def vcs_repo(
         self,
         environment,

readthedocs/projects/tasks/utils.py

@@ -67,6 +67,7 @@ def clean_project_resources(project, version=None):
 
     - Artifacts from storage.
     - Search indexes from ES.
+    - Imported files.
 
     :param version: Version instance. If isn't given,
                     all resources of `project` will be deleted.
@@ -90,6 +91,12 @@ def clean_project_resources(project, version=None):
         version_slug=version.slug if version else None,
     )
 
+    # Remove imported files
+    if version:
+        version.imported_files.all().delete()
+    else:
+        project.imported_files.all().delete()
+
 
 @app.task()
 def finish_inactive_builds():

readthedocs/rtd_tests/mocks/paths.py

@@ -1,10 +1,9 @@
 """Context managers to patch os.path.exists calls."""
 import os
-import re
 from unittest import mock
 
 
-def fake_paths(check, target="exists"):
+def fake_paths(check):
     """
     Usage:
 
@@ -23,7 +22,7 @@ def patched_exists(path):
             return original_exists(path)
         return result
 
-    return mock.patch.object(os.path, target, patched_exists)
+    return mock.patch.object(os.path, "exists", patched_exists)
 
 
 def fake_paths_lookup(path_dict):
@@ -40,22 +39,3 @@ def check(path):
         return path_dict.get(path, None)
 
     return fake_paths(check)
-
-
-def fake_paths_by_regex(pattern, target="exists", exists=True):
-    r"""
-    Usage:
-
-    >>> with fake_paths_by_regex('\.pdf$', target="lexists"):
-    ...     assert os.path.exists('my.pdf') == True
-    >>> with fake_paths_by_regex('\.pdf$', target="lexists", exists=False):
-    ...     assert os.path.exists('my.pdf') == False
-    """
-    regex = re.compile(pattern)
-
-    def check(path):
-        if regex.search(path):
-            return exists
-        return None
-
-    return fake_paths(check, target)

readthedocs/rtd_tests/tests/test_build_forms.py

@@ -8,7 +8,7 @@
 from readthedocs.builds.forms import VersionForm
 from readthedocs.builds.models import Version
 from readthedocs.projects.constants import PRIVATE, PUBLIC
-from readthedocs.projects.models import Project
+from readthedocs.projects.models import HTMLFile, Project
 
 
 class TestVersionForm(TestCase):
@@ -90,15 +90,31 @@ def test_can_update_privacy_level(self):
         self.assertEqual(version.privacy_level, PRIVATE)
 
     @mock.patch("readthedocs.builds.models.trigger_build", mock.MagicMock())
-    @mock.patch("readthedocs.projects.tasks.utils.clean_project_resources")
+    @mock.patch("readthedocs.projects.tasks.search.remove_search_indexes")
+    @mock.patch("readthedocs.projects.tasks.utils.remove_build_storage_paths")
     def test_resources_are_deleted_when_version_is_inactive(
-        self, clean_project_resources
+        self, remove_build_storage_paths, remove_search_indexes
     ):
         version = get(
             Version,
             project=self.project,
             active=True,
         )
+        another_version = get(Version, project=self.project, active=True)
+        get(
+            HTMLFile,
+            project=self.project,
+            version=version,
+            name="index.html",
+            path="index.html",
+        )
+        get(
+            HTMLFile,
+            project=self.project,
+            version=another_version,
+            name="index.html",
+            path="index.html",
+        )
 
         url = reverse(
             "project_version_detail", args=(version.project.slug, version.slug)
@@ -114,7 +130,10 @@ def test_resources_are_deleted_when_version_is_inactive(
             },
         )
         self.assertEqual(r.status_code, 302)
-        clean_project_resources.assert_not_called()
+        remove_build_storage_paths.delay.assert_not_called()
+        remove_search_indexes.delay.assert_not_called()
+        self.assertTrue(version.imported_files.exists())
+        self.assertTrue(another_version.imported_files.exists())
 
         r = self.client.post(
             url,
@@ -124,4 +143,7 @@ def test_resources_are_deleted_when_version_is_inactive(
             },
         )
         self.assertEqual(r.status_code, 302)
-        clean_project_resources.assert_called_once()
+        remove_build_storage_paths.delay.assert_called_once()
+        remove_search_indexes.delay.assert_called_once()
+        self.assertFalse(version.imported_files.exists())
+        self.assertTrue(another_version.imported_files.exists())

readthedocs/rtd_tests/tests/test_project.py

@@ -33,7 +33,6 @@
 from readthedocs.projects.exceptions import ProjectConfigurationError
 from readthedocs.projects.models import Project
 from readthedocs.projects.tasks.utils import finish_inactive_builds
-from readthedocs.rtd_tests.mocks.paths import fake_paths_by_regex
 
 
 class ProjectMixin:
@@ -63,36 +62,6 @@ def test_subprojects(self):
         self.assertEqual(r.status_code, 200)
         self.assertEqual(resp['subprojects'][0]['id'], 23)
 
-    def test_has_pdf(self):
-        # The project has a pdf if the PDF file exists on disk.
-        with fake_paths_by_regex(r"\.pdf$", target="lexists"):
-            self.assertTrue(self.pip.has_pdf(LATEST))
-
-        # The project has no pdf if there is no file on disk.
-        with fake_paths_by_regex(r"\.pdf$", target="lexists", exists=False):
-            self.assertFalse(self.pip.has_pdf(LATEST))
-
-    def test_has_pdf_with_pdf_build_disabled(self):
-        # The project doesn't depend on `enable_pdf_build`
-        self.pip.enable_pdf_build = False
-        with fake_paths_by_regex(r"\.pdf$", target="lexists"):
-            self.assertTrue(self.pip.has_pdf(LATEST))
-
-    def test_has_epub(self):
-        # The project has a epub if the PDF file exists on disk.
-        with fake_paths_by_regex(r"\.epub$", target="lexists"):
-            self.assertTrue(self.pip.has_epub(LATEST))
-
-        # The project has no epub if there is no file on disk.
-        with fake_paths_by_regex(r"\.epub$", target="lexists", exists=False):
-            self.assertFalse(self.pip.has_epub(LATEST))
-
-    def test_has_epub_with_epub_build_disabled(self):
-        # The project doesn't depend on `enable_epub_build`
-        self.pip.enable_epub_build = False
-        with fake_paths_by_regex(r"\.epub$", target="lexists"):
-            self.assertTrue(self.pip.has_epub(LATEST))
-
     @patch('readthedocs.projects.models.Project.find')
     def test_conf_file_found(self, find_method):
         find_method.return_value = [