External versions: save state (open / closed)

When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109

Author: humitos

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):

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

readthedocs/builds/migrations/0042_version_state.py

@@ -0,0 +1,25 @@
+# Generated by Django 3.2.12 on 2022-04-21 13:50
+
+from django.db import migrations, models
+
+
+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"), ("merged", "Merged")],
+                help_text="State of the PR/MR associated to this version.",
+                max_length=20,
+                null=True,
+                verbose_name="State",
+            ),
+        ),
+    ]

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,

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:

readthedocs/builds/tests/test_tasks.py

@@ -5,71 +5,77 @@
 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(
             Version,
             project=project,
             slug='branch',
             type=BRANCH,
-            active=False,
+            state=EXTERNAL_VERSION_STATE_CLOSED,
             modified=datetime.now() - timedelta(days=7),
         )
         get(
             Version,
             project=project,
             slug='tag',
             type=TAG,
-            active=True,
+            state=EXTERNAL_VERSION_STATE_OPEN,
             modified=datetime.now() - timedelta(days=7),
         )
         get(
             Version,
             project=project,
             slug='external-active',
             type=EXTERNAL,
-            active=True,
+            state=EXTERNAL_VERSION_STATE_OPEN,
             modified=datetime.now() - timedelta(days=7),
         )
         get(
             Version,
             project=project,
             slug='external-inactive',
             type=EXTERNAL,
-            active=False,
+            state=EXTERNAL_VERSION_STATE_CLOSED,
             modified=datetime.now() - timedelta(days=3),
         )
         get(
             Version,
             project=project,
             slug='external-inactive-old',
             type=EXTERNAL,
-            active=False,
+            state=EXTERNAL_VERSION_STATE_CLOSED,
             modified=datetime.now() - timedelta(days=7),
         )
 
         self.assertEqual(Version.objects.all().count(), 5)
         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())

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,
         )

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):

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'},
         },