Skip to content

Commit e4611c8

Browse files
authored
Fix 404 links in GH status for PR builds (#12563)
When a PR build is skipped (exit code 183), it reports SUCCESS to GitHub so the PR can be merged, but the version is never marked as built. This caused the status link to point to a 404 "edit version" page.
1 parent 693dcee commit e4611c8

File tree

2 files changed

+85
-2
lines changed

2 files changed

+85
-2
lines changed

readthedocs/builds/models.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,13 +342,23 @@ def get_absolute_url(self):
342342
"""
343343
Get the absolute URL to the docs of the version.
344344
345-
If the version doesn't have a successfully uploaded build, then we return the project's
346-
dashboard page.
345+
For external versions (PR builds), if the version is not built, we return the
346+
build detail page since these versions are ephemeral and read-only.
347+
348+
For internal versions, if not built, we return the project's version dashboard page.
347349
348350
Because documentation projects can be hosted on separate domains, this function ALWAYS
349351
returns with a full "http(s)://<domain>/" prefix.
350352
"""
351353
if not self.built and not self.uploaded:
354+
# External versions (PR builds) should link to the build detail page
355+
# since they're read-only and we can't "edit" them
356+
if self.type == EXTERNAL:
357+
latest_build = self.latest_build
358+
if latest_build:
359+
return latest_build.get_full_url()
360+
361+
# For internal versions, fall back to the project version detail page
352362
# TODO: Stop assuming protocol based on settings.DEBUG
353363
# (this pattern is also used in builds.tasks for sending emails)
354364
protocol = "http" if settings.DEBUG else "https"

readthedocs/rtd_tests/tests/test_oauth.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,47 @@ def test_send_build_status_success(self, request):
983983
"target_url": f"http://{self.project.slug}.readthedocs.io/en/latest/",
984984
}
985985

986+
@requests_mock.Mocker(kw="request")
987+
def test_send_build_status_success_when_not_built(self, request):
988+
"""Test that when status is SUCCESS but version is not built, it links to build detail page.
989+
990+
This is specifically for external versions (PR builds) which are read-only and should
991+
always link to the build detail page when not built.
992+
"""
993+
commit = "1234abc"
994+
# Create an external version (PR build) that is not built
995+
version = get(Version, project=self.project, type=EXTERNAL, built=False)
996+
build = get(
997+
Build,
998+
project=self.project,
999+
version=version,
1000+
)
1001+
request.post(
1002+
f"{self.api_url}/app/installations/1111/access_tokens",
1003+
json=self._get_access_token_json(),
1004+
)
1005+
request.get(
1006+
f"{self.api_url}/repositories/{self.remote_repository.remote_id}/commits/{commit}",
1007+
json=self._get_commit_json(commit=commit),
1008+
)
1009+
status_api_request = request.post(
1010+
f"{self.api_url}/repos/user/repo/statuses/{commit}",
1011+
json={},
1012+
)
1013+
1014+
service = self.installation.service
1015+
assert service.send_build_status(
1016+
build=build, commit=commit, status=BUILD_STATUS_SUCCESS
1017+
)
1018+
assert status_api_request.called
1019+
assert status_api_request.last_request.json() == {
1020+
"context": f"docs/readthedocs:{self.project.slug}",
1021+
"description": "Read the Docs build succeeded!",
1022+
"state": "success",
1023+
# Should link to build detail page, not version docs
1024+
"target_url": f"https://readthedocs.org/projects/{self.project.slug}/builds/{build.pk}/",
1025+
}
1026+
9861027
@requests_mock.Mocker(kw="request")
9871028
def test_send_build_status_failure(self, request):
9881029
commit = "1234abc"
@@ -1561,6 +1602,38 @@ def test_send_build_status_successful(self, session, mock_logger, mock_structlog
15611602
"GitHub commit status created for project.",
15621603
)
15631604

1605+
@mock.patch("readthedocs.oauth.services.github.structlog")
1606+
@mock.patch("readthedocs.oauth.services.github.log")
1607+
@mock.patch("readthedocs.oauth.services.github.GitHubService.session")
1608+
def test_send_build_status_on_pr_builds(self, session, mock_logger, mock_structlog):
1609+
"""Test that when status is SUCCESS but version is not built, it links to build detail page.
1610+
1611+
This happens when a build has exit code 183 (skipped) - it reports SUCCESS
1612+
to GitHub so the PR can be merged, but the version is never marked as built.
1613+
"""
1614+
# external_version.built is False by default
1615+
session.post.return_value.status_code = 201
1616+
success = self.service.send_build_status(
1617+
build=self.external_build,
1618+
commit=self.external_build.commit,
1619+
status=BUILD_STATUS_SUCCESS,
1620+
)
1621+
1622+
self.assertTrue(success)
1623+
# Verify that the target_url points to the build detail page, not the version docs
1624+
call_args = mock_structlog.contextvars.bind_contextvars.call_args_list
1625+
# Find the call with target_url
1626+
target_url = None
1627+
for call in call_args:
1628+
if 'target_url' in call[1]:
1629+
target_url = call[1]['target_url']
1630+
break
1631+
1632+
self.assertIsNotNone(target_url)
1633+
# Should link to build detail page, not version URL
1634+
self.assertIn(f'/projects/{self.project.slug}/builds/{self.external_build.pk}/', target_url)
1635+
self.assertNotIn('.readthedocs.io', target_url)
1636+
15641637
@mock.patch("readthedocs.oauth.services.github.structlog")
15651638
@mock.patch("readthedocs.oauth.services.github.log")
15661639
@mock.patch("readthedocs.oauth.services.github.GitHubService.session")

0 commit comments

Comments
 (0)