Integrations: allow to pass more data about external versions

This was extracted from #7664

Author: stsewd

readthedocs/api/v2/views/integrations.py

@@ -5,6 +5,7 @@
 import json
 import logging
 import re
+from functools import namedtuple
 
 from django.shortcuts import get_object_or_404
 from rest_framework import permissions, status
@@ -21,14 +22,13 @@
 )
 from readthedocs.core.views.hooks import (
     build_branches,
-    trigger_sync_versions,
-    get_or_create_external_version,
-    delete_external_version,
     build_external_version,
+    delete_external_version,
+    get_or_create_external_version,
+    trigger_sync_versions,
 )
 from readthedocs.integrations.models import HttpExchange, Integration
-from readthedocs.projects.models import Project, Feature
-
+from readthedocs.projects.models import Feature, Project
 
 log = logging.getLogger(__name__)
 
@@ -56,6 +56,12 @@
 BITBUCKET_PUSH = 'repo:push'
 
 
+ExternalVersionData = namedtuple(
+    'ExternalVersionData',
+    ['id', 'source_branch', 'base_branch', 'commit'],
+)
+
+
 class WebhookMixin:
 
     """Base class for Webhook mixins."""
@@ -227,20 +233,23 @@ 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,
+            version_data=version_data,
         )
 
         return {
-            'build_triggered': True,
+            'build_triggered': bool(to_build),
             'project': project.slug,
-            'versions': [to_build],
+            'versions': [to_build] if to_build else [],
         }
 
     def get_delete_external_version_response(self, project):
@@ -258,10 +267,10 @@ def get_delete_external_version_response(self, project):
         :param project: Project instance
         :type project: Project
         """
-        identifier, verbose_name = self.get_external_version_data()
-        # Delete external version
+        version_data = self.get_external_version_data()
         deleted_version = delete_external_version(
-            project, identifier, verbose_name
+            project=project,
+            version_data=version_data,
         )
         return {
             'version_deleted': deleted_version is not None,
@@ -320,13 +329,15 @@ 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
-
+            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:
-            raise ParseError('Parameters "sha" and "number" are required')
+            raise ParseError('Invalid payload')
 
     def is_payload_valid(self):
         """
@@ -530,13 +541,15 @@ 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
-
+            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:
-            raise ParseError('Parameters "id" and "iid" are required')
+            raise ParseError('Invalid payload')
 
     def handle_webhook(self):
         """

readthedocs/core/views/hooks.py

@@ -4,10 +4,9 @@
 
 from readthedocs.builds.constants import EXTERNAL
 from readthedocs.core.utils import trigger_build
-from readthedocs.projects.models import Project, Feature
+from readthedocs.projects.models import Feature, Project
 from readthedocs.projects.tasks import sync_repository_task
 
-
 log = logging.getLogger(__name__)
 
 
@@ -119,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,32 +143,37 @@ 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
-        if external_version.identifier != identifier:
-            external_version.identifier = identifier
+        if external_version.identifier != version_data.commit:
+            external_version.identifier = version_data.commit
             external_version.save()
 
             log.info(
-                '(Update External Version) Updated Version: [%s] ',
-                external_version.slug
+                'Commit from external version updated. project=%s version=%s',
+                project.slug, external_version.slug,
             )
     return external_version
 
 
-def delete_external_version(project, identifier, verbose_name):
+def delete_external_version(project, version_data):
     """
-    Delete external versions using `identifier` and `verbose_name`.
+    Delete external versions using the id and latest commit.
 
     if external version does not exist then returns `None`.
 
     :param project: Project instance
-    :param identifier: Commit Hash
-    :param verbose_name: pull/merge request number
-    :returns:  verbose_name (pull/merge request number).
+    :param version_data: A :py:class:`readthedocs.api.v2.views.integrations.ExternalVersionData`
+    instance.
+    :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:
         # Delete External Version
@@ -183,11 +187,14 @@ def delete_external_version(project, identifier, verbose_name):
     return None
 
 
-def build_external_version(project, version, commit):
+def build_external_version(project, version, version_data):
     """
     Where we actually trigger builds for external versions.
 
     All pull/merge request webhook logic should route here to call ``trigger_build``.
+
+    :param version_data: A :py:class:`readthedocs.api.v2.views.integrations.ExternalVersionData`
+    instance.
     """
     if not project.has_valid_webhook:
         project.has_valid_webhook = True
@@ -199,6 +206,6 @@ def build_external_version(project, version, commit):
         project.slug,
         version.slug,
     )
-    trigger_build(project=project, version=version, commit=commit, force=True)
+    trigger_build(project=project, version=version, commit=version.identifier, force=True)
 
     return version.verbose_name

readthedocs/rtd_tests/tests/test_api.py

@@ -1,8 +1,8 @@
 import base64
 import datetime
 import json
-
 from unittest import mock
+
 from allauth.socialaccount.models import SocialAccount
 from django.contrib.auth.models import User
 from django.http import QueryDict
@@ -16,13 +16,13 @@
     GITHUB_CREATE,
     GITHUB_DELETE,
     GITHUB_EVENT_HEADER,
-    GITHUB_PUSH,
-    GITHUB_SIGNATURE_HEADER,
     GITHUB_PULL_REQUEST,
+    GITHUB_PULL_REQUEST_CLOSED,
     GITHUB_PULL_REQUEST_OPENED,
     GITHUB_PULL_REQUEST_REOPENED,
-    GITHUB_PULL_REQUEST_CLOSED,
     GITHUB_PULL_REQUEST_SYNC,
+    GITHUB_PUSH,
+    GITHUB_SIGNATURE_HEADER,
     GITLAB_MERGE_REQUEST,
     GITLAB_MERGE_REQUEST_CLOSE,
     GITLAB_MERGE_REQUEST_MERGE,
@@ -37,7 +37,7 @@
     GitLabWebhookView,
 )
 from readthedocs.api.v2.views.task_views import get_status_data
-from readthedocs.builds.constants import LATEST, EXTERNAL
+from readthedocs.builds.constants import EXTERNAL, LATEST
 from readthedocs.builds.models import Build, BuildCommandResult, Version
 from readthedocs.integrations.models import Integration
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
@@ -48,7 +48,6 @@
     Project,
 )
 
-
 super_auth = base64.b64encode(b'super:test').decode('utf-8')
 eric_auth = base64.b64encode(b'eric:test').decode('utf-8')
 
@@ -840,9 +839,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,
@@ -851,7 +854,9 @@ def setUp(self):
                 "last_commit": {
                     "id": self.commit
                 },
-                "action": "open"
+                "action": "open",
+                "source_branch": "source_branch",
+                "target_branch": "master",
             },
         }
         self.gitlab_payload = {