diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index cd7a5a981da..2b356236eb2 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -15,11 +15,15 @@ from rest_framework.status import HTTP_400_BAD_REQUEST from rest_framework.views import APIView +from readthedocs.builds.constants import ( + EXTERNAL_VERSION_STATE_CLOSED, + EXTERNAL_VERSION_STATE_OPEN, +) from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab from readthedocs.core.views.hooks import ( build_branches, build_external_version, - deactivate_external_version, + close_external_version, get_or_create_external_version, trigger_sync_versions, ) @@ -249,14 +253,14 @@ def get_external_version_response(self, project): "versions": [to_build] if to_build else [], } - def get_deactivated_external_version_response(self, project): + def get_closed_external_version_response(self, project): """ - Deactivate the external version on merge/close events and return the API response. + Close the external version on merge/close events and return the API response. Return a JSON response with the following:: { - "version_deactivated": true, + "closed": true, "project": "project_name", "versions": [verbose_name] } @@ -265,14 +269,14 @@ def get_deactivated_external_version_response(self, project): :type project: Project """ version_data = self.get_external_version_data() - deactivated_version = deactivate_external_version( + version_closed = close_external_version( project=project, version_data=version_data, ) return { - 'version_deactivated': bool(deactivated_version), - 'project': project.slug, - 'versions': [deactivated_version] if deactivated_version else [], + "closed": bool(version_closed), + "project": project.slug, + "versions": [version_closed] if version_closed else [], } @@ -432,7 +436,7 @@ def handle_webhook(self): if action == GITHUB_PULL_REQUEST_CLOSED: # Delete external version when PR is closed - return self.get_deactivated_external_version_response(self.project) + return self.get_closed_external_version_response(self.project) # Sync versions when push event is created/deleted action if all([ @@ -594,7 +598,7 @@ def handle_webhook(self): if action in [GITLAB_MERGE_REQUEST_CLOSE, GITLAB_MERGE_REQUEST_MERGE]: # Handle merge and close merge_request event. - return self.get_deactivated_external_version_response(self.project) + return self.get_closed_external_version_response(self.project) return None def _normalize_ref(self, ref): diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index 2da46a0eef4..a02d5b0d61b 100644 --- a/readthedocs/builds/constants.py +++ b/readthedocs/builds/constants.py @@ -51,6 +51,12 @@ (EXTERNAL, EXTERNAL_TEXT), (UNKNOWN, UNKNOWN_TEXT), ) +EXTERNAL_VERSION_STATE_OPEN = "open" +EXTERNAL_VERSION_STATE_CLOSED = "closed" +EXTERNAL_VERSION_STATES = ( + (EXTERNAL_VERSION_STATE_OPEN, _("Open")), + (EXTERNAL_VERSION_STATE_CLOSED, _("Closed")), +) LATEST = settings.RTD_LATEST LATEST_VERBOSE_NAME = settings.RTD_LATEST_VERBOSE_NAME diff --git a/readthedocs/builds/migrations/0042_version_state.py b/readthedocs/builds/migrations/0042_version_state.py new file mode 100644 index 00000000000..e655b399920 --- /dev/null +++ b/readthedocs/builds/migrations/0042_version_state.py @@ -0,0 +1,40 @@ +# Generated by Django 3.2.12 on 2022-04-21 13:50 + +from django.db import migrations, models + + +def forwards_func(apps, schema_editor): + """ + Migrate external Versions with `active=False` to use `state=Closed` and `active=True`. + """ + Version = apps.get_model("builds", "Version") + # NOTE: we can't use `Version.external` because the Django's __fake__ + # object does not declare it. So, we need to pass `type=external` in the + # filter + Version.objects.filter(type="external", active=False).update( + active=True, + state="closed", + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ("builds", "0041_track_task_id"), + ] + + operations = [ + migrations.AddField( + model_name="version", + name="state", + field=models.CharField( + blank=True, + choices=[("open", "Open"), ("closed", "Closed")], + help_text="State of the PR/MR associated to this version.", + max_length=20, + null=True, + verbose_name="State", + ), + ), + migrations.RunPython(forwards_func), + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 7c662f2c103..b1913a6dc31 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -27,6 +27,7 @@ BUILD_STATUS_CHOICES, BUILD_TYPES, EXTERNAL, + EXTERNAL_VERSION_STATES, INTERNAL, LATEST, NON_REPOSITORY_VERSIONS, @@ -140,8 +141,16 @@ class Version(TimeStampedModel): supported = models.BooleanField(_('Supported'), default=True) active = models.BooleanField(_('Active'), default=False) - built = models.BooleanField(_('Built'), default=False) - uploaded = models.BooleanField(_('Uploaded'), default=False) + state = models.CharField( + _("State"), + max_length=20, + choices=EXTERNAL_VERSION_STATES, + null=True, + blank=True, + help_text=_("State of the PR/MR associated to this version."), + ) + built = models.BooleanField(_("Built"), default=False) + uploaded = models.BooleanField(_("Uploaded"), default=False) privacy_level = models.CharField( _('Privacy Level'), max_length=20, diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index ab8408e280d..41b31d02aac 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -22,6 +22,7 @@ BUILD_STATUS_PENDING, BUILD_STATUS_SUCCESS, EXTERNAL, + EXTERNAL_VERSION_STATE_CLOSED, LOCK_EXPIRE, MAX_BUILD_COMMAND_SIZE, TAG, @@ -228,15 +229,15 @@ def archive_builds_task(self, days=14, limit=200, delete=False): @app.task(queue='web') -def delete_inactive_external_versions(limit=200, days=30 * 3): +def delete_closed_external_versions(limit=200, days=30 * 3): """ - Delete external versions that have been marked as inactive after ``days``. + Delete external versions that have been marked as closed after ``days``. The commit status is updated to link to the build page, as the docs are removed. """ days_ago = timezone.now() - timezone.timedelta(days=days) queryset = Version.external.filter( - active=False, + state=EXTERNAL_VERSION_STATE_CLOSED, modified__lte=days_ago, )[:limit] for version in queryset: diff --git a/readthedocs/builds/tests/test_tasks.py b/readthedocs/builds/tests/test_tasks.py index 0a5975175cb..89f6a4ff592 100644 --- a/readthedocs/builds/tests/test_tasks.py +++ b/readthedocs/builds/tests/test_tasks.py @@ -5,18 +5,24 @@ from django.utils import timezone from django_dynamic_fixture import get -from readthedocs.builds.constants import BRANCH, EXTERNAL, TAG +from readthedocs.builds.constants import ( + BRANCH, + EXTERNAL, + EXTERNAL_VERSION_STATE_CLOSED, + EXTERNAL_VERSION_STATE_OPEN, + TAG, +) from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.builds.tasks import ( archive_builds_task, - delete_inactive_external_versions, + delete_closed_external_versions, ) from readthedocs.projects.models import Project class TestTasks(TestCase): - def test_delete_inactive_external_versions(self): + def test_delete_closed_external_versions(self): project = get(Project) project.versions.all().delete() get( @@ -24,7 +30,7 @@ def test_delete_inactive_external_versions(self): project=project, slug='branch', type=BRANCH, - active=False, + state=EXTERNAL_VERSION_STATE_CLOSED, modified=datetime.now() - timedelta(days=7), ) get( @@ -32,7 +38,7 @@ def test_delete_inactive_external_versions(self): project=project, slug='tag', type=TAG, - active=True, + state=EXTERNAL_VERSION_STATE_OPEN, modified=datetime.now() - timedelta(days=7), ) get( @@ -40,7 +46,7 @@ def test_delete_inactive_external_versions(self): project=project, slug='external-active', type=EXTERNAL, - active=True, + state=EXTERNAL_VERSION_STATE_OPEN, modified=datetime.now() - timedelta(days=7), ) get( @@ -48,7 +54,7 @@ def test_delete_inactive_external_versions(self): project=project, slug='external-inactive', type=EXTERNAL, - active=False, + state=EXTERNAL_VERSION_STATE_CLOSED, modified=datetime.now() - timedelta(days=3), ) get( @@ -56,7 +62,7 @@ def test_delete_inactive_external_versions(self): project=project, slug='external-inactive-old', type=EXTERNAL, - active=False, + state=EXTERNAL_VERSION_STATE_CLOSED, modified=datetime.now() - timedelta(days=7), ) @@ -64,12 +70,12 @@ def test_delete_inactive_external_versions(self): self.assertEqual(Version.external.all().count(), 3) # We don't have inactive external versions from 9 days ago. - delete_inactive_external_versions(days=9) + delete_closed_external_versions(days=9) self.assertEqual(Version.objects.all().count(), 5) self.assertEqual(Version.external.all().count(), 3) # We have one inactive external versions from 6 days ago. - delete_inactive_external_versions(days=6) + delete_closed_external_versions(days=6) self.assertEqual(Version.objects.all().count(), 4) self.assertEqual(Version.external.all().count(), 2) self.assertFalse(Version.objects.filter(slug='external-inactive-old').exists()) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 054cb5b6ab7..d7ed214c0c8 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -2,7 +2,11 @@ import structlog -from readthedocs.builds.constants import EXTERNAL +from readthedocs.builds.constants import ( + EXTERNAL, + EXTERNAL_VERSION_STATE_CLOSED, + EXTERNAL_VERSION_STATE_OPEN, +) from readthedocs.core.utils import trigger_build from readthedocs.projects.models import Feature, Project from readthedocs.projects.tasks.builds import sync_repository_task @@ -133,7 +137,11 @@ def get_or_create_external_version(project, version_data): external_version, created = project.versions.get_or_create( verbose_name=version_data.id, type=EXTERNAL, - defaults={"identifier": version_data.commit, "active": True}, + defaults={ + "identifier": version_data.commit, + "active": True, + "state": EXTERNAL_VERSION_STATE_OPEN, + }, ) if created: @@ -145,8 +153,8 @@ def get_or_create_external_version(project, version_data): else: # Identifier will change if there is a new commit to the Pull/Merge Request. external_version.identifier = version_data.commit - # If the PR was previously closed it was marked as inactive. - external_version.active = True + # If the PR was previously closed it was marked as closed + external_version.state = EXTERNAL_VERSION_STATE_OPEN external_version.save() log.info( 'External version updated.', @@ -156,21 +164,16 @@ def get_or_create_external_version(project, version_data): return external_version -def deactivate_external_version(project, version_data): +def close_external_version(project, version_data): """ - Deactivate external versions using `identifier` and `verbose_name`. + Close external versions using `identifier` and `verbose_name`. - if external version does not exist then returns `None`. - - We mark the version as inactive, - so another celery task will remove it after some days. + We mark the version's state as `closed` so another celery task will remove + it after some days. If external version does not exist then returns `None`. :param project: Project instance :param version_data: A :py:class:`readthedocs.api.v2.views.integrations.ExternalVersionData` instance. - :param identifier: Commit Hash - :param verbose_name: pull/merge request number - :returns: verbose_name (pull/merge request number). :rtype: str """ external_version = ( @@ -183,10 +186,10 @@ def deactivate_external_version(project, version_data): ) if external_version: - external_version.active = False + external_version.state = EXTERNAL_VERSION_STATE_CLOSED external_version.save() log.info( - 'External version marked as inactive.', + "External version marked as closed.", project_slug=project.slug, version_slug=external_version.slug, ) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 846a80ab7f0..fca76a468eb 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -41,6 +41,7 @@ BUILD_STATE_TRIGGERED, BUILD_STATUS_DUPLICATED, EXTERNAL, + EXTERNAL_VERSION_STATE_CLOSED, LATEST, ) from readthedocs.builds.models import Build, BuildCommandResult, Version @@ -1242,12 +1243,12 @@ def test_github_pull_request_closed_event(self, trigger_build, core_trigger_buil manager=EXTERNAL ).get(verbose_name=pull_request_number) - # External version should be inactive. - self.assertFalse(external_version.active) + self.assertTrue(external_version.active) + self.assertEqual(external_version.state, EXTERNAL_VERSION_STATE_CLOSED) self.assertEqual(resp.status_code, status.HTTP_200_OK) - self.assertTrue(resp.data['version_deactivated']) - self.assertEqual(resp.data['project'], self.project.slug) - self.assertEqual(resp.data['versions'], [version.verbose_name]) + self.assertTrue(resp.data["closed"]) + self.assertEqual(resp.data["project"], self.project.slug) + self.assertEqual(resp.data["versions"], [version.verbose_name]) core_trigger_build.assert_not_called() def test_github_pull_request_no_action(self, trigger_build): @@ -1938,12 +1939,12 @@ def test_gitlab_merge_request_close_event(self, trigger_build, core_trigger_buil manager=EXTERNAL ).get(verbose_name=merge_request_number) - # External version should be inactive. - self.assertFalse(external_version.active) + self.assertTrue(external_version.active) + self.assertEqual(external_version.state, EXTERNAL_VERSION_STATE_CLOSED) self.assertEqual(resp.status_code, status.HTTP_200_OK) - self.assertTrue(resp.data['version_deactivated']) - self.assertEqual(resp.data['project'], self.project.slug) - self.assertEqual(resp.data['versions'], [version.verbose_name]) + self.assertTrue(resp.data["closed"]) + self.assertEqual(resp.data["project"], self.project.slug) + self.assertEqual(resp.data["versions"], [version.verbose_name]) core_trigger_build.assert_not_called() @mock.patch('readthedocs.core.utils.trigger_build') @@ -1983,11 +1984,12 @@ def test_gitlab_merge_request_merge_event(self, trigger_build, core_trigger_buil ).get(verbose_name=merge_request_number) # external version should be deleted - self.assertFalse(external_version.active) + self.assertTrue(external_version.active) + self.assertEqual(external_version.state, EXTERNAL_VERSION_STATE_CLOSED) self.assertEqual(resp.status_code, status.HTTP_200_OK) - self.assertTrue(resp.data['version_deactivated']) - self.assertEqual(resp.data['project'], self.project.slug) - self.assertEqual(resp.data['versions'], [version.verbose_name]) + self.assertTrue(resp.data["closed"]) + self.assertEqual(resp.data["project"], self.project.slug) + self.assertEqual(resp.data["versions"], [version.verbose_name]) core_trigger_build.assert_not_called() def test_gitlab_merge_request_no_action(self, trigger_build): diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 391032b6cf7..370c9c89834 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -430,7 +430,7 @@ def TEMPLATES(self): }, }, 'every-day-delete-inactive-external-versions': { - 'task': 'readthedocs.builds.tasks.delete_inactive_external_versions', + 'task': 'readthedocs.builds.tasks.delete_closed_external_versions', 'schedule': crontab(minute=0, hour=1), 'options': {'queue': 'web'}, },