Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

API V3: clean version when deactivated and build version when activated #10308

Merged
merged 11 commits into from
May 22, 2023
3 changes: 3 additions & 0 deletions docs/user/api/v3.rst
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,9 @@ Version update

Update a version.

When a version is deactivated, its documentation is removed,
and when it's activated, a new build is triggered.
stsewd marked this conversation as resolved.
Show resolved Hide resolved

**Example request**:

.. tabs::
Expand Down
55 changes: 54 additions & 1 deletion readthedocs/api/v3/tests/test_versions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from unittest import mock

import django_dynamic_fixture as fixture
from django.test import override_settings
from django.urls import reverse
Expand Down Expand Up @@ -139,6 +141,9 @@ def test_projects_versions_detail_unique(self):
)
self.assertEqual(response.status_code, 200)

@mock.patch(
"readthedocs.projects.tasks.utils.clean_project_resources", new=mock.MagicMock
)
def test_projects_versions_partial_update(self):
self.assertTrue(self.version.active)
self.assertFalse(self.version.hidden)
Expand Down Expand Up @@ -166,7 +171,7 @@ def test_projects_versions_partial_update(self):
self.assertEqual(self.version.identifier, 'a1b2c3')
self.assertFalse(self.version.active)
self.assertTrue(self.version.hidden)
self.assertTrue(self.version.built)
self.assertFalse(self.version.built)
self.assertEqual(self.version.type, TAG)

def test_projects_versions_partial_update_privacy_levels_disabled(self):
Expand Down Expand Up @@ -227,6 +232,54 @@ def test_projects_versions_partial_update_invalid_privacy_levels(self):
self.assertEqual(response.status_code, 400)
self.assertEqual(self.version.privacy_level, "public")

@mock.patch("readthedocs.api.v3.views.trigger_build")
@mock.patch("readthedocs.projects.tasks.utils.clean_project_resources")
def test_activate_version(self, clean_project_resources, trigger_build):
self.version.active = False
self.version.save()
self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}")
self.assertFalse(self.version.active)
data = {"active": True}
response = self.client.patch(
reverse(
"projects-versions-detail",
kwargs={
"parent_lookup_project__slug": self.project.slug,
"version_slug": self.version.slug,
},
),
data,
)
self.assertEqual(response.status_code, 204)
self.version.refresh_from_db()
self.assertTrue(self.version.active)
clean_project_resources.assert_not_called()
trigger_build.assert_called_once()

@mock.patch("readthedocs.api.v3.views.trigger_build")
@mock.patch("readthedocs.projects.tasks.utils.clean_project_resources")
def test_deactivate_version(self, clean_project_resources, trigger_build):
self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}")
data = {"active": False}
self.assertTrue(self.version.active)
self.assertTrue(self.version.built)
response = self.client.patch(
reverse(
"projects-versions-detail",
kwargs={
"parent_lookup_project__slug": self.project.slug,
"version_slug": self.version.slug,
},
),
data,
)
self.assertEqual(response.status_code, 204)
self.version.refresh_from_db()
self.assertFalse(self.version.active)
self.assertFalse(self.version.built)
clean_project_resources.assert_called_once()
trigger_build.assert_not_called()

def test_projects_version_external(self):
self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}")
self.version.type = EXTERNAL
Expand Down
28 changes: 28 additions & 0 deletions readthedocs/api/v3/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from rest_framework_extensions.mixins import NestedViewSetMixin

from readthedocs.builds.models import Build, Version
from readthedocs.builds.signals import version_changed
from readthedocs.core.utils import trigger_build
from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.oauth.models import (
Expand Down Expand Up @@ -297,6 +298,33 @@ def get_serializer_class(self):
return VersionSerializer
return VersionUpdateSerializer

def update(self, request, *args, **kwargs):
"""
Run extra steps after updating a version.

- When a version is deactivated, we need to clean up its
files from storage, and search index.
- When a version is activated, we need to trigger a build.
- We also need to purge the cache from the CDN,
since the version could have been activated/deactivated,
or its privacy level could have changed.
"""
# Get the current value before updating.
version = self.get_object()
was_active = version.active
result = super().update(request, *args, **kwargs)
# Get the updated version.
version = self.get_object()
# If the version is deactivated, we need to clean up the files.
if was_active and not version.active:
version.clean_resources()
# If the version is activated, we need to trigger a build.
if not was_active and version.active:
trigger_build(project=version.project, version=version)
stsewd marked this conversation as resolved.
Show resolved Hide resolved
# Purge the cache from the CDN.
version_changed.send(sender=self.__class__, version=version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like an additional bug fix in the mix 馃憤

return result


class BuildsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin,
FlexFieldsMixin, ReadOnlyModelViewSet):
Expand Down
18 changes: 18 additions & 0 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,24 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
clean_project_resources(self.project, self)
super().delete(*args, **kwargs)

def clean_resources(self):
"""
Remove all resources from this version.

This includes removing files from storage,
and removing its search index.
"""
from readthedocs.projects.tasks.utils import clean_project_resources

log.info(
"Removing files for version.",
project_slug=self.project.slug,
version_slug=self.slug,
)
clean_project_resources(project=self.project, version=self)
self.built = False
self.save()

@property
def identifier_friendly(self):
"""Return display friendly identifier."""
Expand Down
11 changes: 1 addition & 10 deletions readthedocs/projects/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,16 +218,7 @@ def form_valid(self, form):
version = form.save()
if form.has_changed():
if 'active' in form.changed_data and version.active is False:
log.info(
'Removing files for version.',
version_slug=version.slug,
)
clean_project_resources(
version.project,
version,
)
version.built = False
version.save()
version.clean_resources()
return HttpResponseRedirect(self.get_success_url())


Expand Down
8 changes: 5 additions & 3 deletions readthedocs/rtd_tests/tests/test_build_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,11 @@ def test_can_update_privacy_level(self):
self.assertTrue(form.is_valid())
self.assertEqual(version.privacy_level, PRIVATE)

@mock.patch('readthedocs.builds.forms.trigger_build', mock.MagicMock())
@mock.patch('readthedocs.projects.views.private.clean_project_resources')
def test_resources_are_deleted_when_version_is_inactive(self, clean_project_resources):
@mock.patch("readthedocs.builds.forms.trigger_build", mock.MagicMock())
@mock.patch("readthedocs.projects.tasks.utils.clean_project_resources")
def test_resources_are_deleted_when_version_is_inactive(
self, clean_project_resources
):
version = get(
Version,
project=self.project,
Expand Down