Fix status reporting on PRs with the magic exit code (#9807)

* Fix status reporting on PRs with the magic exit code

This was because we were using this logic to return a relative URL,
and then sending it to GitHub:

* https://github.com/readthedocs/readthedocs.org/blob/f86b5376ffc70242bdd677cb02f016429ff6bd4b/readthedocs/oauth/services/github.py#L459-L460
* https://github.com/readthedocs/readthedocs.org/blob/f86b5376ffc70242bdd677cb02f016429ff6bd4b/readthedocs/builds/models.py#L343-L349

Fixes #9791

* Fix link_to_build defaults

* Fix tests

* Comment on usage

* Fix test

* Use same logic as absolute_url

* VCS commit status: send absolute URL to docs when built successfully

- remove `link_to_build` since it's not required
- base the decition on `Build.state` and `Build.success`

---------

Co-authored-by: Manuel Kaufmann <[email protected]>

Author: ericholscher

readthedocs/builds/tasks.py

@@ -261,7 +261,6 @@ def delete_closed_external_versions(limit=200, days=30 * 3):
                     build_pk=last_build.pk,
                     commit=last_build.commit,
                     status=status,
-                    link_to_build=True,
                 )
         except Exception:
             log.exception(
@@ -377,7 +376,7 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs):
     default_retry_delay=60,
     queue='web'
 )
-def send_build_status(build_pk, commit, status, link_to_build=False):
+def send_build_status(build_pk, commit, status):
     """
     Send Build Status to Git Status API for project external versions.
 
@@ -432,7 +431,6 @@ def send_build_status(build_pk, commit, status, link_to_build=False):
                     build=build,
                     commit=commit,
                     state=status,
-                    link_to_build=link_to_build,
                 )
 
                 if success:
@@ -449,7 +447,11 @@ def send_build_status(build_pk, commit, status, link_to_build=False):
                 # Try to loop through services for users all social accounts
                 # to send successful build status
                 for service in services:
-                    success = service.send_build_status(build, commit, status)
+                    success = service.send_build_status(
+                        build,
+                        commit,
+                        status,
+                    )
                     if success:
                         log.debug(
                             'Build status report sent correctly using an user account.',

readthedocs/core/utils/__init__.py

@@ -106,7 +106,7 @@ def prepare_build(
             version_type=version.type,
             build_pk=build.id,
             commit=commit,
-            status=BUILD_STATUS_PENDING
+            status=BUILD_STATUS_PENDING,
         )
 
     if version.type != EXTERNAL:

readthedocs/oauth/services/base.py

@@ -293,7 +293,7 @@ def update_webhook(self, project, integration):
         """
         raise NotImplementedError
 
-    def send_build_status(self, build, commit, state, link_to_build=False):
+    def send_build_status(self, build, commit, state):
         """
         Create commit status for project.
 
@@ -303,7 +303,6 @@ def send_build_status(self, build, commit, state, link_to_build=False):
         :type commit: str
         :param state: build state failure, pending, or success.
         :type state: str
-        :param link_to_build: If true, link to the build page regardless the state.
         :returns: boolean based on commit status creation was successful or not.
         :rtype: Bool
         """

readthedocs/oauth/services/github.py

@@ -12,7 +12,7 @@
 
 from readthedocs.api.v2.client import api
 from readthedocs.builds import utils as build_utils
-from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS
+from readthedocs.builds.constants import BUILD_FINAL_STATES, SELECT_BUILD_STATUS
 from readthedocs.core.permissions import AdminPermission
 from readthedocs.integrations.models import Integration
 
@@ -431,7 +431,7 @@ def update_webhook(self, project, integration):
         integration.remove_secret()
         return (False, resp)
 
-    def send_build_status(self, build, commit, state, link_to_build=False):
+    def send_build_status(self, build, commit, state):
         """
         Create GitHub commit status for project.
 
@@ -441,7 +441,6 @@ def send_build_status(self, build, commit, state, link_to_build=False):
         :type state: str
         :param commit: commit sha of the pull request
         :type commit: str
-        :param link_to_build: If true, link to the build page regardless the state.
         :returns: boolean based on commit status creation was successful or not.
         :rtype: Bool
         """
@@ -452,12 +451,14 @@ def send_build_status(self, build, commit, state, link_to_build=False):
         # select the correct state and description.
         github_build_state = SELECT_BUILD_STATUS[state]['github']
         description = SELECT_BUILD_STATUS[state]['description']
-
-        target_url = build.get_full_url()
         statuses_url = f'https://api.github.com/repos/{owner}/{repo}/statuses/{commit}'
 
-        if not link_to_build and state == BUILD_STATUS_SUCCESS:
+        if build.state in BUILD_FINAL_STATES and build.success:
+            # Link to the documentation for this version
             target_url = build.version.get_absolute_url()
+        else:
+            # Link to the build detail's page
+            target_url = build.get_full_url()
 
         context = f'{settings.RTD_BUILD_STATUS_API_NAME}:{project.slug}'
 

readthedocs/oauth/services/gitlab.py

@@ -11,7 +11,7 @@
 from requests.exceptions import RequestException
 
 from readthedocs.builds import utils as build_utils
-from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS
+from readthedocs.builds.constants import BUILD_FINAL_STATES, SELECT_BUILD_STATUS
 from readthedocs.integrations.models import Integration
 
 from ..constants import GITLAB
@@ -518,7 +518,7 @@ def update_webhook(self, project, integration):
         integration.remove_secret()
         return (False, resp)
 
-    def send_build_status(self, build, commit, state, link_to_build=False):
+    def send_build_status(self, build, commit, state):
         """
         Create GitLab commit status for project.
 
@@ -528,7 +528,6 @@ def send_build_status(self, build, commit, state, link_to_build=False):
         :type state: str
         :param commit: commit sha of the pull request
         :type commit: str
-        :param link_to_build: If true, link to the build page regardless the state.
         :returns: boolean based on commit status creation was successful or not.
         :rtype: Bool
         """
@@ -545,10 +544,12 @@ def send_build_status(self, build, commit, state, link_to_build=False):
         gitlab_build_state = SELECT_BUILD_STATUS[state]['gitlab']
         description = SELECT_BUILD_STATUS[state]['description']
 
-        target_url = build.get_full_url()
-
-        if not link_to_build and state == BUILD_STATUS_SUCCESS:
+        if build.state in BUILD_FINAL_STATES and build.success:
+            # Link to the documentation for this version
             target_url = build.version.get_absolute_url()
+        else:
+            # Link to the build detail's page
+            target_url = build.get_full_url()
 
         context = f'{settings.RTD_BUILD_STATUS_API_NAME}:{project.slug}'
 

readthedocs/projects/tasks/builds.py

@@ -483,7 +483,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
             )
 
         # NOTE: why we wouldn't have `self.data.build_commit` here?
-        # This attribute is set when we get it after clonning the repository
+        # This attribute is set when we get it after cloning the repository
         #
         # Oh, I think this is to differentiate a task triggered with
         # `Build.commit` than a one triggered just with the `Version` to build

readthedocs/rtd_tests/tests/test_celery.py

@@ -8,11 +8,7 @@
 from messages_extends.models import Message
 
 from readthedocs.builds import tasks as build_tasks
-from readthedocs.builds.constants import (
-    BUILD_STATUS_SUCCESS,
-    EXTERNAL,
-    LATEST,
-)
+from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, EXTERNAL, LATEST
 from readthedocs.builds.models import Build, Version
 from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation
 from readthedocs.projects.models import Project
@@ -105,7 +101,6 @@ def test_send_build_status_with_remote_repo_github(self, send_build_status):
             build=external_build,
             commit=external_build.commit,
             state=BUILD_STATUS_SUCCESS,
-            link_to_build=False,
         )
         self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
@@ -125,7 +120,9 @@ def test_send_build_status_with_social_account_github(self, send_build_status):
         )
 
         send_build_status.assert_called_once_with(
-            external_build, external_build.commit, BUILD_STATUS_SUCCESS
+            external_build,
+            external_build.commit,
+            BUILD_STATUS_SUCCESS,
         )
         self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
@@ -171,7 +168,6 @@ def test_send_build_status_with_remote_repo_gitlab(self, send_build_status):
             build=external_build,
             commit=external_build.commit,
             state=BUILD_STATUS_SUCCESS,
-            link_to_build=False,
         )
         self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
@@ -191,7 +187,9 @@ def test_send_build_status_with_social_account_gitlab(self, send_build_status):
         )
 
         send_build_status.assert_called_once_with(
-            external_build, external_build.commit, BUILD_STATUS_SUCCESS
+            external_build,
+            external_build.commit,
+            BUILD_STATUS_SUCCESS,
         )
         self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
@@ -209,4 +207,3 @@ def test_send_build_status_no_remote_repo_or_social_account_gitlab(self, send_bu
 
         send_build_status.assert_not_called()
         self.assertEqual(Message.objects.filter(user=self.eric).count(), 1)
-

readthedocs/rtd_tests/tests/test_projects_tasks.py

@@ -21,21 +21,25 @@ def setUp(self):
     @patch('readthedocs.projects.tasks.utils.send_build_status')
     def test_send_external_build_status_with_external_version(self, send_build_status):
         send_external_build_status(
-            self.external_version.type, self.external_build.id,
-            self.external_build.commit, BUILD_STATUS_SUCCESS
+            self.external_version.type,
+            self.external_build.id,
+            self.external_build.commit,
+            BUILD_STATUS_SUCCESS,
         )
 
         send_build_status.delay.assert_called_once_with(
             self.external_build.id,
             self.external_build.commit,
-            BUILD_STATUS_SUCCESS
+            BUILD_STATUS_SUCCESS,
         )
 
     @patch('readthedocs.projects.tasks.utils.send_build_status')
     def test_send_external_build_status_with_internal_version(self, send_build_status):
         send_external_build_status(
-            self.internal_version.type, self.internal_build.id,
-            self.external_build.commit, BUILD_STATUS_SUCCESS
+            self.internal_version.type,
+            self.internal_build.id,
+            self.external_build.commit,
+            BUILD_STATUS_SUCCESS,
         )
 
         send_build_status.delay.assert_not_called()