diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 49cf3e0675b..82b96c70ce8 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -333,10 +333,9 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ args=[self.get_artifact_paths()], ) - # Remove build artifacts from storage if the version is not external + # Remove resources if the version is not external if self.type != EXTERNAL: - storage_paths = self.get_storage_paths() - tasks.remove_build_storage_paths.delay(storage_paths) + tasks.clean_project_resources(self.project, self) project_pk = self.project.pk super().delete(*args, **kwargs) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index f31560a75fd..759de988e3f 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -480,16 +480,8 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ args=[(self.doc_path,)], ) - # Remove build artifacts from storage - storage_paths = [] - for type_ in MEDIA_TYPES: - storage_paths.append( - '{}/{}'.format( - type_, - self.slug, - ) - ) - tasks.remove_build_storage_paths.delay(storage_paths) + # Remove extra resources + tasks.clean_project_resources(self) super().delete(*args, **kwargs) @@ -534,6 +526,19 @@ def get_subproject_urls(self): return [(proj.child.slug, proj.child.get_docs_url()) for proj in self.subprojects.all()] + def get_storage_paths(self): + """ + Get the paths of all artifacts used by the project. + + :return: the path to an item in storage + (can be used with ``storage.url`` to get the URL). + """ + storage_paths = [ + f'{type_}/{self.slug}' + for type_ in MEDIA_TYPES + ] + return storage_paths + def get_storage_path( self, type_, diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index e359d24eb2e..ce45ce84e25 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1621,8 +1621,9 @@ def _sync_imported_files(version, build, changed_files): # Remove old HTMLFiles from ElasticSearch remove_indexed_files( model=HTMLFile, - version=version, - build=build, + project_slug=version.project.slug, + version_slug=version.slug, + build_id=build, ) # Delete SphinxDomain objects from previous versions @@ -1844,6 +1845,47 @@ def remove_build_storage_paths(paths): storage.delete_directory(storage_path) +@app.task(queue='web') +def remove_search_indexes(project_slug, version_slug=None): + """Wrapper around ``remove_indexed_files`` to make it a task.""" + remove_indexed_files( + model=HTMLFile, + project_slug=project_slug, + version_slug=version_slug, + ) + + +def clean_project_resources(project, version=None): + """ + Delete all extra resources used by `version` of `project`. + + It removes: + + - Artifacts from storage. + - Search indexes from ES. + + :param version: Version instance. If isn't given, + all resources of `project` will be deleted. + + .. note:: + This function is usually called just before deleting project. + Make sure to not depend on the project object inside the tasks. + """ + # Remove storage paths + storage_paths = [] + if version: + storage_paths = version.get_storage_paths() + else: + storage_paths = project.get_storage_paths() + remove_build_storage_paths.delay(storage_paths) + + # Remove indexes + remove_search_indexes.delay( + project_slug=project.slug, + version_slug=version.slug if version else None, + ) + + @app.task(queue='web') def sync_callback(_, version_pk, commit, build, *args, **kwargs): """ diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index b25512f4583..d23bae49ce7 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -203,6 +203,10 @@ def form_valid(self, form): task=tasks.remove_dirs, args=[version.get_artifact_paths()], ) + tasks.clean_project_resources( + version.project, + version, + ) version.built = False version.save() return HttpResponseRedirect(self.get_success_url()) diff --git a/readthedocs/rtd_tests/tests/test_build_forms.py b/readthedocs/rtd_tests/tests/test_build_forms.py index 1e901771632..81da9240b69 100644 --- a/readthedocs/rtd_tests/tests/test_build_forms.py +++ b/readthedocs/rtd_tests/tests/test_build_forms.py @@ -1,6 +1,7 @@ -# -*- coding: utf-8 -*- - +import mock +from django.contrib.auth.models import User from django.test import TestCase +from django.urls import reverse from django_dynamic_fixture import get from readthedocs.builds.forms import VersionForm @@ -12,7 +13,8 @@ class TestVersionForm(TestCase): def setUp(self): - self.project = get(Project) + self.user = get(User) + self.project = get(Project, users=(self.user,)) def test_default_version_is_active(self): version = get( @@ -50,3 +52,35 @@ def test_default_version_is_inactive(self): ) self.assertFalse(form.is_valid()) self.assertIn('active', form.errors) + + @mock.patch('readthedocs.projects.tasks.clean_project_resources') + def test_resources_are_deleted_when_version_is_inactive(self, clean_project_resources): + version = get( + Version, + project=self.project, + active=True, + ) + + url = reverse('project_version_detail', args=(version.project.slug, version.slug)) + + self.client.force_login(self.user) + + r = self.client.post( + url, + data={ + 'active': True, + 'privacy_level': PRIVATE, + }, + ) + self.assertTrue(r.status_code, 200) + clean_project_resources.assert_not_called() + + r = self.client.post( + url, + data={ + 'active': False, + 'privacy_level': PRIVATE, + }, + ) + self.assertTrue(r.status_code, 200) + clean_project_resources.assert_called_once() diff --git a/readthedocs/search/utils.py b/readthedocs/search/utils.py index a747c45f115..97a563d6e88 100644 --- a/readthedocs/search/utils.py +++ b/readthedocs/search/utils.py @@ -43,18 +43,22 @@ def index_new_files(model, version, build): log.exception('Unable to index a subset of files. Continuing.') -def remove_indexed_files(model, version, build): +def remove_indexed_files(model, project_slug, version_slug=None, build_id=None): """ - Remove files from the version from the search index. + Remove files from `version_slug` of `project_slug` from the search index. - This excludes files from the current build. + :param model: Class of the model to be deleted. + :param project_slug: Project slug. + :param version_slug: Version slug. If isn't given, + all index from `project` are deleted. + :param build_id: Build id. If isn't given, all index from `version` are deleted. """ if not DEDConfig.autosync_enabled(): log.info( 'Autosync disabled, skipping removal from the search index for: %s:%s', - version.project.slug, - version.slug, + project_slug, + version_slug, ) return @@ -62,16 +66,18 @@ def remove_indexed_files(model, version, build): document = list(registry.get_documents(models=[model]))[0] log.info( 'Deleting old files from search index for: %s:%s', - version.project.slug, - version.slug, + project_slug, + version_slug, ) - ( + documents = ( document().search() - .filter('term', project=version.project.slug) - .filter('term', version=version.slug) - .exclude('term', build=build) - .delete() + .filter('term', project=project_slug) ) + if version_slug: + documents.filter('term', version=version_slug) + if build_id: + documents.exclude('term', build=build_id) + documents.delete() except Exception: log.exception('Unable to delete a subset of files. Continuing.')