From 8bd1567b8814365165054a262f672f38472a625d Mon Sep 17 00:00:00 2001 From: Benjamin Balder Bach Date: Wed, 31 May 2023 16:36:45 +0200 Subject: [PATCH] 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://" 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 --- readthedocs/builds/constants.py | 7 +++++- readthedocs/builds/models.py | 29 +++++++++++++++++------ readthedocs/oauth/services/github.py | 16 +++++++------ readthedocs/projects/tasks/builds.py | 6 +++-- readthedocs/rtd_tests/tests/test_api.py | 23 ++++++++++-------- readthedocs/rtd_tests/tests/test_oauth.py | 2 ++ 6 files changed, 56 insertions(+), 27 deletions(-) diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index fd3c2a902c5..df1bd6bf7f3 100644 --- a/readthedocs/builds/constants.py +++ b/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' diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index a2cc5bc1348..a8483bc9b6e 100644 --- a/readthedocs/builds/models.py +++ b/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):///" 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( diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index 572f6e6fdba..69fb48a1a5d 100644 --- a/readthedocs/oauth/services/github.py +++ b/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: diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 55b00618351..bbe3fd2d034 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/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): """ diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index d0d7901c1ef..f19355c981c 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/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.""" diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index 8e0a171526f..5759756b723 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/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.',