Integrations: allow to pass more data about external versions (#7692)

This was extracted from #7664

Author: stsewd

readthedocs/api/v2/views/integrations.py

@@ -3,9 +3,10 @@
 import hashlib
 import hmac
 import json
-import structlog
 import re
+from functools import namedtuple
 
+import structlog
 from django.shortcuts import get_object_or_404
 from rest_framework import permissions, status
 from rest_framework.exceptions import NotFound, ParseError
@@ -14,11 +15,7 @@
 from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.views import APIView
 
-from readthedocs.core.signals import (
-    webhook_bitbucket,
-    webhook_github,
-    webhook_gitlab,
-)
+from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab
 from readthedocs.core.views.hooks import (
     build_branches,
     build_external_version,
@@ -27,7 +24,7 @@
     trigger_sync_versions,
 )
 from readthedocs.integrations.models import HttpExchange, Integration
-from readthedocs.projects.models import Feature, Project
+from readthedocs.projects.models import Project
 
 log = structlog.get_logger(__name__)
 
@@ -55,6 +52,12 @@
 BITBUCKET_PUSH = 'repo:push'
 
 
+ExternalVersionData = namedtuple(
+    "ExternalVersionData",
+    ["id", "source_branch", "base_branch", "commit"],
+)
+
+
 class WebhookMixin:
 
     """Base class for Webhook mixins."""
@@ -228,20 +231,22 @@ def get_external_version_response(self, project):
         :param project: Project instance
         :type project: readthedocs.projects.models.Project
         """
-        identifier, verbose_name = self.get_external_version_data()
+        version_data = self.get_external_version_data()
         # create or get external version object using `verbose_name`.
         external_version = get_or_create_external_version(
-            project, identifier, verbose_name
+            project=project,
+            version_data=version_data,
         )
         # returns external version verbose_name (pull/merge request number)
         to_build = build_external_version(
-            project=project, version=external_version, commit=identifier
+            project=project,
+            version=external_version,
         )
 
         return {
-            'build_triggered': True,
-            'project': project.slug,
-            'versions': [to_build],
+            "build_triggered": bool(to_build),
+            "project": project.slug,
+            "versions": [to_build] if to_build else [],
         }
 
     def get_deactivated_external_version_response(self, project):
@@ -259,9 +264,10 @@ def get_deactivated_external_version_response(self, project):
         :param project: Project instance
         :type project: Project
         """
-        identifier, verbose_name = self.get_external_version_data()
+        version_data = self.get_external_version_data()
         deactivated_version = deactivate_external_version(
-            project, identifier, verbose_name
+            project=project,
+            version_data=version_data,
         )
         return {
             'version_deactivated': bool(deactivated_version),
@@ -320,13 +326,16 @@ def get_data(self):
     def get_external_version_data(self):
         """Get Commit Sha and pull request number from payload."""
         try:
-            identifier = self.data['pull_request']['head']['sha']
-            verbose_name = str(self.data['number'])
-
-            return identifier, verbose_name
-
-        except KeyError:
-            raise ParseError('Parameters "sha" and "number" are required')
+            data = ExternalVersionData(
+                id=str(self.data["number"]),
+                commit=self.data["pull_request"]["head"]["sha"],
+                source_branch=self.data["pull_request"]["head"]["ref"],
+                base_branch=self.data["pull_request"]["base"]["ref"],
+            )
+            return data
+        except KeyError as e:
+            key = e.args[0]
+            raise ParseError(f"Invalid payload. {key} is required.")
 
     def is_payload_valid(self):
         """
@@ -523,13 +532,16 @@ def is_payload_valid(self):
     def get_external_version_data(self):
         """Get commit SHA and merge request number from payload."""
         try:
-            identifier = self.data['object_attributes']['last_commit']['id']
-            verbose_name = str(self.data['object_attributes']['iid'])
-
-            return identifier, verbose_name
-
-        except KeyError:
-            raise ParseError('Parameters "id" and "iid" are required')
+            data = ExternalVersionData(
+                id=str(self.data["object_attributes"]["iid"]),
+                commit=self.data["object_attributes"]["last_commit"]["id"],
+                source_branch=self.data["object_attributes"]["source_branch"],
+                base_branch=self.data["object_attributes"]["target_branch"],
+            )
+            return data
+        except KeyError as e:
+            key = e.args[0]
+            raise ParseError(f"Invalid payload. {key} is required.")
 
     def handle_webhook(self):
         """

readthedocs/core/views/hooks.py

@@ -118,22 +118,22 @@ def trigger_sync_versions(project):
     return None
 
 
-def get_or_create_external_version(project, identifier, verbose_name):
+def get_or_create_external_version(project, version_data):
     """
-    Get or create external versions using `identifier` and `verbose_name`.
+    Get or create version using the ``commit`` as identifier, and PR id as ``verbose_name``.
 
     if external version does not exist create an external version
 
     :param project: Project instance
-    :param identifier: Commit Hash
-    :param verbose_name: pull/merge request number
-    :returns:  External version.
+    :param version_data: A :py:class:`readthedocs.api.v2.views.integrations.ExternalVersionData`
+     instance.
+    :returns: External version.
     :rtype: Version
     """
     external_version, created = project.versions.get_or_create(
-        verbose_name=verbose_name,
+        verbose_name=version_data.id,
         type=EXTERNAL,
-        defaults={'identifier': identifier, 'active': True},
+        defaults={"identifier": version_data.commit, "active": True},
     )
 
     if created:
@@ -144,11 +144,10 @@ def get_or_create_external_version(project, identifier, verbose_name):
         )
     else:
         # Identifier will change if there is a new commit to the Pull/Merge Request.
-        external_version.identifier = identifier
+        external_version.identifier = version_data.commit
         # If the PR was previously closed it was marked as inactive.
         external_version.active = True
         external_version.save()
-
         log.info(
             'External version updated.',
             project_slug=project.slug,
@@ -157,7 +156,7 @@ def get_or_create_external_version(project, identifier, verbose_name):
     return external_version
 
 
-def deactivate_external_version(project, identifier, verbose_name):
+def deactivate_external_version(project, version_data):
     """
     Deactivate external versions using `identifier` and `verbose_name`.
 
@@ -167,14 +166,21 @@ def deactivate_external_version(project, identifier, verbose_name):
     so another celery task will remove it after some days.
 
     :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 = project.versions(manager=EXTERNAL).filter(
-        verbose_name=verbose_name, identifier=identifier
-    ).first()
+    external_version = (
+        project.versions(manager=EXTERNAL)
+        .filter(
+            verbose_name=version_data.id,
+            identifier=version_data.commit,
+        )
+        .first()
+    )
 
     if external_version:
         external_version.active = False
@@ -188,7 +194,7 @@ def deactivate_external_version(project, identifier, verbose_name):
     return None
 
 
-def build_external_version(project, version, commit):
+def build_external_version(project, version):
     """
     Where we actually trigger builds for external versions.
 
@@ -204,6 +210,6 @@ def build_external_version(project, version, commit):
         project_slug=project.slug,
         version_slug=version.slug,
     )
-    trigger_build(project=project, version=version, commit=commit)
+    trigger_build(project=project, version=version, commit=version.identifier)
 
     return version.verbose_name

readthedocs/rtd_tests/tests/test_api.py

@@ -920,9 +920,13 @@ def setUp(self):
             "number": 2,
             "pull_request": {
                 "head": {
-                    "sha": self.commit
-                }
-            }
+                    "sha": self.commit,
+                    "ref": "source_branch",
+                },
+                "base": {
+                    "ref": "master",
+                },
+            },
         }
         self.gitlab_merge_request_payload = {
             "object_kind": GITLAB_MERGE_REQUEST,
@@ -931,7 +935,9 @@ def setUp(self):
                 "last_commit": {
                     "id": self.commit
                 },
-                "action": "open"
+                "action": "open",
+                "source_branch": "source_branch",
+                "target_branch": "master",
             },
         }
         self.gitlab_payload = {
@@ -1526,6 +1532,14 @@ def test_github_dont_trigger_double_sync(self, trigger_build):
         )
         self.assertTrue(resp.json()['versions_synced'])
 
+    def test_github_get_external_version_data(self, trigger_build):
+        view = GitHubWebhookView(data=self.github_pull_request_payload)
+        version_data = view.get_external_version_data()
+        self.assertEqual(version_data.id, "2")
+        self.assertEqual(version_data.commit, self.commit)
+        self.assertEqual(version_data.source_branch, "source_branch")
+        self.assertEqual(version_data.base_branch, "master")
+
     def test_gitlab_webhook_for_branches(self, trigger_build):
         """GitLab webhook API."""
         client = APIClient()
@@ -2041,6 +2055,14 @@ def test_gitlab_merge_request_close_event_invalid_payload(self, trigger_build):
 
         self.assertEqual(resp.status_code, 400)
 
+    def test_gitlab_get_external_version_data(self, trigger_build):
+        view = GitLabWebhookView(data=self.gitlab_merge_request_payload)
+        version_data = view.get_external_version_data()
+        self.assertEqual(version_data.id, "2")
+        self.assertEqual(version_data.commit, self.commit)
+        self.assertEqual(version_data.source_branch, "source_branch")
+        self.assertEqual(version_data.base_branch, "master")
+
     def test_bitbucket_webhook(self, trigger_build):
         """Bitbucket webhook API."""
         client = APIClient()