Fix github build status reporting bug

readthedocs/core/utils/__init__.py

@@ -133,7 +133,10 @@ def prepare_build(
 
     if build:
         # Send pending Build Status using Git Status API for External Builds.
-        send_external_build_status(version=version, build_pk=build.id, status=BUILD_STATUS_PENDING)
+        send_external_build_status(
+            version_type=version.type, build_pk=build.id,
+            commit=version.identifier, status=BUILD_STATUS_PENDING
+        )
 
     return (
         update_docs_task.signature(

readthedocs/oauth/services/base.py

@@ -250,12 +250,14 @@ def update_webhook(self, project, integration):
         """
         raise NotImplementedError
 
-    def send_build_status(self, build, state):
+    def send_build_status(self, build, commit, state):
         """
         Create commit status for project.
 
         :param build: Build to set up commit status for
         :type build: Build
+        :param commit: commit sha of the pull/merge request
+        :type commit: str
         :param state: build state failure, pending, or success.
         :type state: str
         :returns: boolean based on commit status creation was successful or not.

readthedocs/oauth/services/github.py

@@ -315,21 +315,22 @@ def update_webhook(self, project, integration):
             )
             return (False, resp)
 
-    def send_build_status(self, build, state):
+    def send_build_status(self, build, commit, state):
         """
         Create GitHub commit status for project.
 
         :param build: Build to set up commit status for
         :type build: Build
         :param state: build state failure, pending, or success.
         :type state: str
+        :param commit: commit sha of the pull request
+        :type commit: str
         :returns: boolean based on commit status creation was successful or not.
         :rtype: Bool
         """
         session = self.get_session()
         project = build.project
         owner, repo = build_utils.get_github_username_repo(url=project.repo)
-        build_sha = build.version.identifier
 
         # select the correct state and description.
         github_build_state = SELECT_BUILD_STATUS[state]['github']
@@ -346,7 +347,7 @@ def send_build_status(self, build, state):
 
         try:
             resp = session.post(
-                f'https://api.github.com/repos/{owner}/{repo}/statuses/{build_sha}',
+                f'https://api.github.com/repos/{owner}/{repo}/statuses/{commit}',
                 data=json.dumps(data),
                 headers={'content-type': 'application/json'},
             )

readthedocs/projects/tasks.py

@@ -579,16 +579,25 @@ def run_build(self, docker, record):
         if self.build_env.failed:
             self.send_notifications(self.version.pk, self.build['id'])
             send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_FAILURE
+                version_type=self.version.type,
+                build_pk=self.build['id'],
+                commit=self.build['commit'],
+                status=BUILD_STATUS_FAILURE
             )
         elif self.build_env.successful:
             send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_SUCCESS
+                version_type=self.version.type,
+                build_pk=self.build['id'],
+                commit=self.build['commit'],
+                status=BUILD_STATUS_SUCCESS
             )
         else:
             msg = 'Unhandled Build Status'
             send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_FAILURE
+                version_type=self.version.type,
+                build_pk=self.build['id'],
+                commit=self.build['commit'],
+                status=BUILD_STATUS_FAILURE
             )
             log.warning(
                 LOG_TEMPLATE,
@@ -1822,11 +1831,12 @@ def retry_domain_verification(domain_pk):
 
 
 @app.task(queue='web')
-def send_build_status(build_pk, status):
+def send_build_status(build_pk, commit, status):
     """
     Send Build Status to Git Status API for project external versions.
 
     :param build_pk: Build primary key
+    :param commit: commit sha of the pull/merge request
     :param status: build status failed, pending, or success to be sent.
     """
     build = Build.objects.get(pk=build_pk)
@@ -1838,7 +1848,7 @@ def send_build_status(build_pk, status):
             )
 
             # send Status report using the API.
-            service.send_build_status(build, status)
+            service.send_build_status(build, commit, status)
 
     except RemoteRepository.DoesNotExist:
         log.info('Remote repository does not exist for %s', build.project)
@@ -1849,16 +1859,17 @@ def send_build_status(build_pk, status):
     # TODO: Send build status for other providers.
 
 
-def send_external_build_status(version, build_pk, status):
+def send_external_build_status(version_type, build_pk, commit, status):
     """
     Check if build is external and Send Build Status for project external versions.
 
-     :param version: Version instance
+     :param version_type: Version type e.g EXTERNAL, BRANCH, TAG
      :param build_pk: Build pk
+     :param commit: commit sha of the pull/merge request
      :param status: build status failed, pending, or success to be sent.
     """
 
     # Send status reports for only External (pull/merge request) Versions.
-    if version.type == EXTERNAL:
+    if version_type == EXTERNAL:
         # call the task that actually send the build status.
-        send_build_status.delay(build_pk, status)
+        send_build_status.delay(build_pk, commit, status)

readthedocs/rtd_tests/tests/test_celery.py

@@ -343,16 +343,22 @@ def test_send_build_status_task(self, send_build_status):
         external_build = get(
             Build, project=self.project, version=external_version
         )
-        tasks.send_build_status(external_build.id, BUILD_STATUS_SUCCESS)
+        tasks.send_build_status(
+            external_build.id, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
-        send_build_status.assert_called_once_with(external_build, BUILD_STATUS_SUCCESS)
+        send_build_status.assert_called_once_with(
+            external_build, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
     @patch('readthedocs.projects.tasks.GitHubService.send_build_status')
     def test_send_build_status_task_without_remote_repo(self, send_build_status):
         external_version = get(Version, project=self.project, type=EXTERNAL)
         external_build = get(
             Build, project=self.project, version=external_version
         )
-        tasks.send_build_status(external_build.id, BUILD_STATUS_SUCCESS)
+        tasks.send_build_status(
+            external_build.id, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
         send_build_status.assert_not_called()

readthedocs/rtd_tests/tests/test_config_integration.py

@@ -368,7 +368,11 @@ def get_update_docs_task(self):
             config=load_yaml_config(self.version),
             project=self.project,
             version=self.version,
-            build={'id': 99, 'state': BUILD_STATE_TRIGGERED}
+            build={
+                'id': 99,
+                'state': BUILD_STATE_TRIGGERED,
+                'commit': 'abc859dada4faf'
+            }
         )
         return update_docs
 

readthedocs/rtd_tests/tests/test_oauth.py

@@ -156,6 +156,7 @@ def test_send_build_status_successful(self, session, mock_logger):
         session().post.return_value.status_code = 201
         success = self.service.send_build_status(
             self.external_build,
+            self.external_build.commit,
             BUILD_STATUS_SUCCESS
         )
 
@@ -172,6 +173,7 @@ def test_send_build_status_404_error(self, session, mock_logger):
         session().post.return_value.status_code = 404
         success = self.service.send_build_status(
             self.external_build,
+            self.external_build.commit,
             BUILD_STATUS_SUCCESS
         )
 
@@ -187,7 +189,7 @@ def test_send_build_status_404_error(self, session, mock_logger):
     def test_send_build_status_value_error(self, session, mock_logger):
         session().post.side_effect = ValueError
         success = self.service.send_build_status(
-            self.external_build, BUILD_STATUS_SUCCESS
+            self.external_build, self.external_build.commit, BUILD_STATUS_SUCCESS
         )
 
         self.assertFalse(success)

readthedocs/rtd_tests/tests/test_projects_tasks.py

@@ -138,14 +138,22 @@ def setUp(self):
 
     @patch('readthedocs.projects.tasks.send_build_status')
     def test_send_external_build_status_with_external_version(self, send_build_status):
-        send_external_build_status(self.external_version,
-                                   self.external_build.id, BUILD_STATUS_SUCCESS)
+        send_external_build_status(
+            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, BUILD_STATUS_SUCCESS)
+        send_build_status.delay.assert_called_once_with(
+            self.external_build.id,
+            self.external_build.commit,
+            BUILD_STATUS_SUCCESS
+        )
 
     @patch('readthedocs.projects.tasks.send_build_status')
     def test_send_external_build_status_with_internal_version(self, send_build_status):
-        send_external_build_status(self.internal_version,
-                                   self.internal_build.id, BUILD_STATUS_SUCCESS)
+        send_external_build_status(
+            self.internal_version.type, self.internal_build.id,
+            self.external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
         send_build_status.delay.assert_not_called()