Build: Bug in target_url, failure to add "success" status if no external version exists (#10369)

* Clarifications to state vs status

* Use an absolute URL, including the "https://<domain>" for versions

* Update tests with new Version.get_absolute_url implementation

* Revert a change: Always set `self.data.build["success"] = False` in on_failure

* Don't use URLValidator in tests

Author: benjaoming

readthedocs/builds/constants.py

@@ -3,6 +3,9 @@
 from django.conf import settings
 from django.utils.translation import gettext_lazy as _
 
+# BUILD_STATE is our *INTERNAL* representation of build states.
+# This is not to be confused with external representations of 'status'
+# that are sent back to Git providers.
 BUILD_STATE_TRIGGERED = "triggered"
 BUILD_STATE_CLONING = "cloning"
 BUILD_STATE_INSTALLING = "installing"
@@ -78,7 +81,9 @@
     STABLE,
 )
 
-# General Build Statuses
+# General build statuses, i.e. the status that is reported back to the
+# user on a Git Provider. This not the same as BUILD_STATE which the internal
+# representation.
 BUILD_STATUS_FAILURE = 'failed'
 BUILD_STATUS_PENDING = 'pending'
 BUILD_STATUS_SUCCESS = 'success'

readthedocs/builds/models.py

@@ -377,14 +377,29 @@ def commit_name(self):
         return self.identifier
 
     def get_absolute_url(self):
-        """Get absolute url to the docs of the version."""
+        """
+        Get the absolute URL to the docs of the version.
+
+        If the version doesn't have a successfully uploaded build, then we return the project's
+        dashboard page.
+
+        Because documentation projects can be hosted on separate domains, this function ALWAYS
+        returns with a full "http(s)://<domain>/" prefix.
+        """
         if not self.built and not self.uploaded:
-            return reverse(
-                'project_version_detail',
-                kwargs={
-                    'project_slug': self.project.slug,
-                    'version_slug': self.slug,
-                },
+            # TODO: Stop assuming protocol based on settings.DEBUG
+            # (this pattern is also used in builds.tasks for sending emails)
+            protocol = "http" if settings.DEBUG else "https"
+            return "{}://{}{}".format(
+                protocol,
+                settings.PRODUCTION_DOMAIN,
+                reverse(
+                    "project_version_detail",
+                    kwargs={
+                        "project_slug": self.project.slug,
+                        "version_slug": self.slug,
+                    },
+                ),
             )
         external = self.type == EXTERNAL
         return self.project.get_docs_url(

readthedocs/oauth/services/github.py

@@ -446,8 +446,8 @@ def send_build_status(self, build, commit, status):
         project = build.project
         owner, repo = build_utils.get_github_username_repo(url=project.repo)
 
-        # select the correct state and description.
-        github_build_state = SELECT_BUILD_STATUS[status]["github"]
+        # select the correct status and description.
+        github_build_status = SELECT_BUILD_STATUS[status]["github"]
         description = SELECT_BUILD_STATUS[status]["description"]
         statuses_url = f"https://api.github.com/repos/{owner}/{repo}/statuses/{commit}"
 
@@ -461,17 +461,19 @@ def send_build_status(self, build, commit, status):
         context = f'{settings.RTD_BUILD_STATUS_API_NAME}:{project.slug}'
 
         data = {
-            'state': github_build_state,
-            'target_url': target_url,
-            'description': description,
-            'context': context,
+            "state": github_build_status,
+            "target_url": target_url,
+            "description": description,
+            "context": context,
         }
 
         log.bind(
             project_slug=project.slug,
-            commit_status=github_build_state,
+            commit_status=github_build_status,
             user_username=self.user.username,
             statuses_url=statuses_url,
+            target_url=target_url,
+            status=status,
         )
         resp = None
         try:

readthedocs/projects/tasks/builds.py

@@ -422,13 +422,15 @@ def before_start(self, task_id, args, kwargs):
     def _reset_build(self):
         # Reset build only if it has some commands already.
         if self.data.build.get("commands"):
-            log.info("Reseting build.")
+            log.info("Resetting build.")
             api_v2.build(self.data.build["id"]).reset.post()
 
     def on_failure(self, exc, task_id, args, kwargs, einfo):
         """
         Celery handler to be executed when a task fails.
 
+        Updates build data, adds tasks to send build notifications.
+
         .. note::
 
            Since the task has failed, some attributes from the `self.data`
@@ -520,7 +522,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
             )
 
         # Update build object
-        self.data.build['success'] = False
+        self.data.build["success"] = False
 
     def get_valid_artifact_types(self):
         """

readthedocs/rtd_tests/tests/test_api.py

@@ -208,12 +208,14 @@ def test_response_building(self):
                 'version_slug': version.slug,
             },
         )
+
         build = resp.data
-        self.assertEqual(build['state'], 'cloning')
-        self.assertEqual(build['error'], '')
-        self.assertEqual(build['exit_code'], 0)
-        self.assertEqual(build['success'], True)
-        self.assertEqual(build['docs_url'], dashboard_url)
+        self.assertEqual(build["state"], "cloning")
+        self.assertEqual(build["error"], "")
+        self.assertEqual(build["exit_code"], 0)
+        self.assertEqual(build["success"], True)
+        self.assertTrue(build["docs_url"].endswith(dashboard_url))
+        self.assertTrue(build["docs_url"].startswith("https://"))
 
     @override_settings(DOCROOT="/home/docs/checkouts/readthedocs.org/user_builds")
     def test_response_finished_and_success(self):
@@ -299,11 +301,12 @@ def test_response_finished_and_fail(self):
             },
         )
         build = resp.data
-        self.assertEqual(build['state'], 'finished')
-        self.assertEqual(build['error'], '')
-        self.assertEqual(build['exit_code'], 1)
-        self.assertEqual(build['success'], False)
-        self.assertEqual(build['docs_url'], dashboard_url)
+        self.assertEqual(build["state"], "finished")
+        self.assertEqual(build["error"], "")
+        self.assertEqual(build["exit_code"], 1)
+        self.assertEqual(build["success"], False)
+        self.assertTrue(build["docs_url"].endswith(dashboard_url))
+        self.assertTrue(build["docs_url"].startswith("https://"))
 
     def test_make_build_without_permission(self):
         """Ensure anonymous/non-staff users cannot write the build endpoint."""

readthedocs/rtd_tests/tests/test_oauth.py

@@ -350,6 +350,8 @@ def test_send_build_status_value_error(self, session, mock_logger):
             commit_status='success',
             user_username=self.user.username,
             statuses_url='https://api.github.com/repos/pypa/pip/statuses/1234',
+            target_url=mock.ANY,
+            status="success",
         )
         mock_logger.exception.assert_called_with(
             'GitHub commit status creation failed for project.',