From 13a9e7c3aa5156e9d6974768393b74eff8c00d37 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 29 Apr 2020 12:18:10 -0700 Subject: [PATCH 01/16] Add ability to rebuild a specific build This is most useful for PR builds, but I've enabled it for all builds currently. It might make sense to only enable it for PR builds, or other builds where we care about rebuilding a specific commit. Fixes https://github.com/readthedocs/readthedocs.org/issues/6524 --- readthedocs/builds/views.py | 18 ++++++++++++ readthedocs/core/utils/__init__.py | 29 ++++++++++++------- .../templates/builds/build_detail.html | 15 ++++++++++ 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 3167570752f..75a00d81e06 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -54,14 +54,32 @@ def post(self, request, project_slug): return HttpResponseForbidden() version_slug = request.POST.get('version_slug') + commit = request.POST.get('commit') + build_pk = request.POST.get('build_pk') + version = get_object_or_404( self._get_versions(project), slug=version_slug, ) + # Set either the build or None + build = Build.objects.filter(pk=build_pk).first() + + if build: + + log.info( + 'Rebuilding build. project=%s version=%s commit=%s build=%s', + project.slug, + version.slug, + commit, + build.pk + ) + update_docs_task, build = trigger_build( project=project, version=version, + build=build, + commit=commit, ) if (update_docs_task, build) == (None, None): # Build was skipped diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index c257470406b..0d22512c986 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -64,6 +64,7 @@ def broadcast(type, task, args, kwargs=None, callback=None): # pylint: disable= def prepare_build( project, version=None, + build=None, commit=None, record=True, force=False, @@ -93,8 +94,6 @@ def prepare_build( send_notifications, ) - build = None - if not Project.objects.is_active(project): log.warning( 'Build not triggered because Project is not active: project=%s', @@ -112,7 +111,15 @@ def prepare_build( 'commit': commit, } - if record: + if build: + build.state = BUILD_STATE_TRIGGERED + build.success = True + build.commit = commit + build.commands.all().delete() + build.save() + kwargs['build_pk'] = build.pk + + if record and not build: build = Build.objects.create( project=project, version=version, @@ -197,7 +204,7 @@ def prepare_build( ) -def trigger_build(project, version=None, commit=None, record=True, force=False): +def trigger_build(project, version=None, build=None, commit=None, record=True, force=False): """ Trigger a Build. @@ -213,17 +220,19 @@ def trigger_build(project, version=None, commit=None, record=True, force=False): :rtype: tuple """ log.info( - 'Triggering build. project=%s version=%s commit=%s', + 'Triggering build. project=%s version=%s commit=%s build=%s', project.slug, version.slug if version else None, commit, + build.pk ) update_docs_task, build = prepare_build( - project, - version, - commit, - record, - force, + project=project, + version=version, + build=build, + commit=commit, + record=record, + force=force, immutable=True, ) diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 4124600f3ed..8a6c8f2e0bd 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -119,6 +119,21 @@ style="display: none;"> {% trans "Build failed" %} + + {% if request.user|is_admin:project %} + +
+
+ {% csrf_token %} + + + + +
+
+ + {% endif %} + {% if request.user|is_admin:project %} From d0e715c7d4d0709e2c5e07708c5a7bf226d6b940 Mon Sep 17 00:00:00 2001 From: Eric Holscher <25510+ericholscher@users.noreply.github.com> Date: Thu, 30 Apr 2020 05:55:17 -0700 Subject: [PATCH 02/16] Update readthedocs/core/utils/__init__.py Co-authored-by: Maksudul Haque --- readthedocs/core/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 0d22512c986..d4f3e4093b0 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -224,7 +224,7 @@ def trigger_build(project, version=None, build=None, commit=None, record=True, f project.slug, version.slug if version else None, commit, - build.pk + build.pk if build else None ) update_docs_task, build = prepare_build( project=project, From 1f74b9a679da9a4ed3406664d8636928968b42ae Mon Sep 17 00:00:00 2001 From: Eric Holscher <25510+ericholscher@users.noreply.github.com> Date: Mon, 18 May 2020 16:05:59 -0700 Subject: [PATCH 03/16] Update readthedocs/builds/views.py Co-authored-by: Maksudul Haque --- readthedocs/builds/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 75a00d81e06..cdd4f477110 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -63,7 +63,7 @@ def post(self, request, project_slug): ) # Set either the build or None - build = Build.objects.filter(pk=build_pk).first() + build = version.builds.filter(pk=build_pk).first() if build: From 3a6d1e6780205a12291ebb8bf6c3986378d7c4a5 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Mon, 18 May 2020 16:07:50 -0700 Subject: [PATCH 04/16] Fix rebuild link display --- .../templates/builds/build_detail.html | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 8a6c8f2e0bd..eb14d1fdbe9 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -69,6 +69,22 @@ + + + {% if request.user|is_admin:project %} +
+
+ {% csrf_token %} + + Rebuild commit + + + + +
+
+ {% endif %} +
@@ -119,21 +135,6 @@ style="display: none;"> {% trans "Build failed" %} - - {% if request.user|is_admin:project %} - -
-
- {% csrf_token %} - - - - -
-
- - {% endif %} -
{% if request.user|is_admin:project %} From a2c731a1ab46de9a076b32291658eb2b63dd84eb Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Mon, 18 May 2020 16:24:00 -0700 Subject: [PATCH 05/16] Add test for building by commit --- readthedocs/builds/views.py | 3 ++- readthedocs/rtd_tests/tests/test_views.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index cdd4f477110..88e2007cf70 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -58,7 +58,8 @@ def post(self, request, project_slug): build_pk = request.POST.get('build_pk') version = get_object_or_404( - self._get_versions(project), + # Don't filter by internal/external here so we can build all versions + Version.objects.public(self.request.user), slug=version_slug, ) diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index f9fbb5e8159..cb063ac850f 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -263,6 +263,22 @@ def test_build_list_includes_external_versions(self): self.assertEqual(response.status_code, 200) self.assertIn(external_version_build, response.context['build_qs']) + @mock.patch('readthedocs.projects.tasks.update_docs_task') + def test_build_commit_external_version(self, mock): + ver = self.pip.versions.first() + ver.commit = 'asd324653546' + ver.type = 'external' + ver.save() + build = get(Build, version=ver, project=self.pip) + build.save() + r = self.client.post('/projects/pip/builds/', + {'version_slug': ver.slug, 'commit': ver.commit, 'build_pk': build.pk} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r._headers['location'][1], + '/projects/pip/builds/%s/' % build.pk, + ) class TestSearchAnalyticsView(TestCase): From 423c3d8d6843e17587c67cbc699d35723cecc808 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Mon, 18 May 2020 16:32:41 -0700 Subject: [PATCH 06/16] Add a BuildCommandResult for retriggering ot make the UI nicer --- readthedocs/core/utils/__init__.py | 10 ++++++++-- readthedocs/templates/builds/build_detail.html | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index d4f3e4093b0..55cb27f5c2a 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -1,5 +1,6 @@ """Common utilty functions.""" +from datetime import datetime import errno import logging import os @@ -17,7 +18,6 @@ BUILD_STATUS_PENDING, EXTERNAL, ) -from readthedocs.doc_builder.constants import DOCKER_LIMITS from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError @@ -86,7 +86,7 @@ def prepare_build( :rtype: tuple """ # Avoid circular import - from readthedocs.builds.models import Build + from readthedocs.builds.models import Build, BuildCommandResult from readthedocs.projects.models import Project, Feature from readthedocs.projects.tasks import ( update_docs_task, @@ -119,6 +119,12 @@ def prepare_build( build.save() kwargs['build_pk'] = build.pk + # Show a step in the UI that the build is being rebuilt + BuildCommandResult.objects.create( + build=build, command='Rebuilding build', exit_code=0, + start_time=datetime.utcnow(), end_time=datetime.utcnow(), + ) + if record and not build: build = Build.objects.create( project=project, diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index eb14d1fdbe9..eec4d1de93c 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -76,7 +76,7 @@
{% csrf_token %} - Rebuild commit + Rebuild this build From 0a0221e76ab3b27deaa45aa9037167374be2fd5d Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Tue, 23 Jun 2020 10:50:15 -0700 Subject: [PATCH 07/16] Use new build status to convey retriggered state --- readthedocs/builds/constants.py | 2 ++ readthedocs/core/utils/__init__.py | 10 +++------- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index 9f0f7a1d6bc..499be07e515 100644 --- a/readthedocs/builds/constants.py +++ b/readthedocs/builds/constants.py @@ -129,7 +129,9 @@ BUILD_STATUS_NORMAL = 'normal' BUILD_STATUS_DUPLICATED = 'duplicated' +BUILD_STATUS_RETRIGGERED = 'retriggered' BUILD_STATUS_CHOICES = ( (BUILD_STATUS_NORMAL, 'Normal'), (BUILD_STATUS_DUPLICATED, 'Duplicated'), + (BUILD_STATUS_RETRIGGERED, 'Retriggered'), ) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index eaa2d80380e..25955a6f26c 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -17,6 +17,7 @@ BUILD_STATE_FINISHED, BUILD_STATUS_PENDING, EXTERNAL, + BUILD_STATUS_RETRIGGERED ) from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError, DuplicatedBuildError @@ -86,7 +87,7 @@ def prepare_build( :rtype: tuple """ # Avoid circular import - from readthedocs.builds.models import Build, BuildCommandResult + from readthedocs.builds.models import Build from readthedocs.projects.models import Project, Feature from readthedocs.projects.tasks import ( update_docs_task, @@ -113,18 +114,13 @@ def prepare_build( if build: build.state = BUILD_STATE_TRIGGERED + build.status = BUILD_STATUS_RETRIGGERED build.success = True build.commit = commit build.commands.all().delete() build.save() kwargs['build_pk'] = build.pk - # Show a step in the UI that the build is being rebuilt - BuildCommandResult.objects.create( - build=build, command='Rebuilding build', exit_code=0, - start_time=datetime.utcnow(), end_time=datetime.utcnow(), - ) - if record and not build: build = Build.objects.create( project=project, From 2fe76124967ca10b0c6ea6688f4b36e502ccc466 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 2 Jun 2021 12:12:45 +0200 Subject: [PATCH 08/16] "Rebuild this build" by creating a new `Build` object Instead of re-using the `Build` object, we are creating a new one following the same process that we already have for normal builds. This reduces the complexity of the code by not adding a new special case and also keeps the history of build outputs so users can compare it and figure it out where the potential problem was. --- readthedocs/builds/constants.py | 2 -- readthedocs/builds/views.py | 8 ++--- readthedocs/core/utils/__init__.py | 20 +++-------- readthedocs/rtd_tests/tests/test_views.py | 36 ++++++++++++++----- .../templates/builds/build_detail.html | 1 - 5 files changed, 34 insertions(+), 33 deletions(-) diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index 3cdaf562031..d47f3f3b650 100644 --- a/readthedocs/builds/constants.py +++ b/readthedocs/builds/constants.py @@ -129,11 +129,9 @@ BUILD_STATUS_NORMAL = 'normal' BUILD_STATUS_DUPLICATED = 'duplicated' -BUILD_STATUS_RETRIGGERED = 'retriggered' BUILD_STATUS_CHOICES = ( (BUILD_STATUS_NORMAL, 'Normal'), (BUILD_STATUS_DUPLICATED, 'Duplicated'), - (BUILD_STATUS_RETRIGGERED, 'Retriggered'), ) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 88e2007cf70..160e22c2ff9 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -54,33 +54,31 @@ def post(self, request, project_slug): return HttpResponseForbidden() version_slug = request.POST.get('version_slug') - commit = request.POST.get('commit') build_pk = request.POST.get('build_pk') version = get_object_or_404( # Don't filter by internal/external here so we can build all versions Version.objects.public(self.request.user), slug=version_slug, + project=project, ) # Set either the build or None build = version.builds.filter(pk=build_pk).first() if build: - log.info( 'Rebuilding build. project=%s version=%s commit=%s build=%s', project.slug, version.slug, - commit, + build.commit, build.pk ) update_docs_task, build = trigger_build( project=project, version=version, - build=build, - commit=commit, + commit=build.commit, ) if (update_docs_task, build) == (None, None): # Build was skipped diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 1207c1fa5e1..8ac316b4809 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -17,7 +17,6 @@ BUILD_STATE_FINISHED, BUILD_STATUS_PENDING, EXTERNAL, - BUILD_STATUS_RETRIGGERED ) from readthedocs.doc_builder.constants import DOCKER_LIMITS from readthedocs.doc_builder.exceptions import ( @@ -36,7 +35,6 @@ def prepare_build( project, version=None, - build=None, commit=None, record=True, force=False, @@ -66,6 +64,7 @@ def prepare_build( update_docs_task, ) + build = None if not Project.objects.is_active(project): log.warning( 'Build not triggered because Project is not active: project=%s', @@ -83,16 +82,7 @@ def prepare_build( 'commit': commit, } - if build: - build.state = BUILD_STATE_TRIGGERED - build.status = BUILD_STATUS_RETRIGGERED - build.success = True - build.commit = commit - build.commands.all().delete() - build.save() - kwargs['build_pk'] = build.pk - - if record and not build: + if record: build = Build.objects.create( project=project, version=version, @@ -221,7 +211,7 @@ def prepare_build( ) -def trigger_build(project, version=None, build=None, commit=None, record=True, force=False): +def trigger_build(project, version=None, commit=None, record=True, force=False): """ Trigger a Build. @@ -237,16 +227,14 @@ def trigger_build(project, version=None, build=None, commit=None, record=True, f :rtype: tuple """ log.info( - 'Triggering build. project=%s version=%s commit=%s build=%s', + 'Triggering build. project=%s version=%s commit=%s', project.slug, version.slug if version else None, commit, - build.pk if build else None ) update_docs_task, build = prepare_build( project=project, version=version, - build=build, commit=commit, record=record, force=force, diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index a3bfbf18e1a..df77743aed6 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -266,21 +266,39 @@ def test_build_list_includes_external_versions(self): self.assertIn(external_version_build, response.context['build_qs']) @mock.patch('readthedocs.projects.tasks.update_docs_task') - def test_build_commit_external_version(self, mock): - ver = self.pip.versions.first() - ver.commit = 'asd324653546' - ver.type = 'external' - ver.save() - build = get(Build, version=ver, project=self.pip) + def test_rebuild_specific_commit(self, mock): + builds_count = Build.objects.count() + + version = self.pip.versions.first() + version.type = 'external' + version.save() + + build = get( + Build, + version=version, + project=self.pip, + commit='a1b2c3', + ) build.save() - r = self.client.post('/projects/pip/builds/', - {'version_slug': ver.slug, 'commit': ver.commit, 'build_pk': build.pk} + + r = self.client.post( + '/projects/pip/builds/', + { + 'version_slug': version.slug, + 'build_pk': build.pk, + } ) + self.assertEqual(r.status_code, 302) + self.assertEqual(Build.objects.count(), builds_count + 1) + + newbuild = Build.objects.last() self.assertEqual( r._headers['location'][1], - '/projects/pip/builds/%s/' % build.pk, + f'/projects/pip/builds/{newbuild.pk}/', ) + self.assertEqual(newbuild.commit, 'a1b2c3') + class TestSearchAnalyticsView(TestCase): diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index d4de6e686d5..7c33518f958 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -79,7 +79,6 @@ Rebuild this build - From 8ed2e443fafe8c3ed7b7bc51992e3977f353b3a4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Jun 2021 12:27:07 +0200 Subject: [PATCH 09/16] Show rebuild button only on latest build for external versions Allowing to re-build all the versions and all the past build could end up on publishing outdated docs for a stable/latest version. So, we are restricting this only to External Versions and only to its latest build object. See discussion in https://github.com/readthedocs/readthedocs.org/pull/6995#issuecomment-852918969 --- readthedocs/builds/views.py | 43 +++++++++++++------ .../templates/builds/build_detail.html | 4 +- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 160e22c2ff9..9db57aedc38 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -56,29 +56,37 @@ def post(self, request, project_slug): version_slug = request.POST.get('version_slug') build_pk = request.POST.get('build_pk') - version = get_object_or_404( - # Don't filter by internal/external here so we can build all versions - Version.objects.public(self.request.user), - slug=version_slug, - project=project, - ) - - # Set either the build or None - build = version.builds.filter(pk=build_pk).first() + if build_pk: + # Filter over external versions only when re-triggering a specific build + version = get_object_or_404( + Version.external.public(self.request.user), + slug=version_slug, + project=project, + ) + else: + # Use generic query when triggering a normal build + version = get_object_or_404( + self._get_versions(project), + slug=version_slug, + ) - if build: + # Set either the build to re-trigger it or None + build_to_retrigger = version.builds.filter(pk=build_pk).first() + commit_to_retrigger = None + if build_to_retrigger: + commit_to_retrigger = build_to_retrigger.commit log.info( - 'Rebuilding build. project=%s version=%s commit=%s build=%s', + 'Re-triggering build. project=%s version=%s commit=%s build=%s', project.slug, version.slug, - build.commit, - build.pk + build_to_retrigger.commit, + build_to_retrigger.pk ) update_docs_task, build = trigger_build( project=project, version=version, - commit=build.commit, + commit=commit_to_retrigger, ) if (update_docs_task, build) == (None, None): # Build was skipped @@ -128,6 +136,13 @@ def get_context_data(self, **kwargs): context['project'] = self.project build = self.get_object() + context['is_latest_build'] = ( + build == Build.objects.filter( + project=build.project, + version=build.version, + ).first() + ) + if build.error != BuildEnvironmentError.GENERIC_WITH_BUILD_ID.format(build_id=build.pk): # Do not suggest to open an issue if the error is not generic return context diff --git a/readthedocs/templates/builds/build_detail.html b/readthedocs/templates/builds/build_detail.html index 7c33518f958..6ce1c0dfe7a 100644 --- a/readthedocs/templates/builds/build_detail.html +++ b/readthedocs/templates/builds/build_detail.html @@ -71,7 +71,9 @@ - {% if request.user|is_admin:project %} + {# Show rebuild button only if the version is external and it's the latest build for this version #} + {# see https://github.com/readthedocs/readthedocs.org/pull/6995#issuecomment-852918969 #} + {% if request.user|is_admin:project and build.version.type == "external" and is_latest_build %}
{% csrf_token %} From 54986052fb7f46c653875dfe473e7bd3eaa1b178 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Jun 2021 12:52:54 +0200 Subject: [PATCH 10/16] Check the build is the latest for that version - check at view level (POST) - test case for the check --- readthedocs/builds/views.py | 11 +++++++ readthedocs/rtd_tests/tests/test_views.py | 35 +++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 9db57aedc38..404eb9a387f 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -7,6 +7,7 @@ from django.contrib import messages from django.contrib.auth.decorators import login_required from django.http import ( + HttpResponseBadRequest, HttpResponseForbidden, HttpResponseRedirect, ) @@ -63,6 +64,16 @@ def post(self, request, project_slug): slug=version_slug, project=project, ) + + build = get_object_or_404( + Build.objects.all(), + pk=build_pk, + version=version, + ) + if build != Build.objects.filter(version=version).first(): + # Return bad request if the build re-triggered is not the + # latest for that version + return HttpResponseBadRequest() else: # Use generic query when triggering a normal build version = get_object_or_404( diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index df77743aed6..581292456d8 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -279,7 +279,6 @@ def test_rebuild_specific_commit(self, mock): project=self.pip, commit='a1b2c3', ) - build.save() r = self.client.post( '/projects/pip/builds/', @@ -292,7 +291,7 @@ def test_rebuild_specific_commit(self, mock): self.assertEqual(r.status_code, 302) self.assertEqual(Build.objects.count(), builds_count + 1) - newbuild = Build.objects.last() + newbuild = Build.objects.first() self.assertEqual( r._headers['location'][1], f'/projects/pip/builds/{newbuild.pk}/', @@ -300,6 +299,38 @@ def test_rebuild_specific_commit(self, mock): self.assertEqual(newbuild.commit, 'a1b2c3') + @mock.patch('readthedocs.projects.tasks.update_docs_task') + def test_rebuild_invalid_specific_commit(self, mock): + version = self.pip.versions.first() + version.type = 'external' + version.save() + + for i in range(3): + get( + Build, + version=version, + project=self.pip, + commit=f'a1b2c3-{i}', + ) + + build = Build.objects.filter( + version=version, + project=self.pip, + ).last() + + r = self.client.post( + '/projects/pip/builds/', + { + 'version_slug': version.slug, + 'build_pk': build.pk, + } + ) + + # It should return 400 because we are re-triggering a build of a + # non-lastest build for that version + self.assertEqual(r.status_code, 400) + + class TestSearchAnalyticsView(TestCase): """Tests for search analytics page.""" From 1183f99db33b4889ff3047312c407847e92db808 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Jun 2021 13:07:52 +0200 Subject: [PATCH 11/16] Count builds properly on test --- readthedocs/rtd_tests/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index 581292456d8..2d9df3a5045 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -289,7 +289,7 @@ def test_rebuild_specific_commit(self, mock): ) self.assertEqual(r.status_code, 302) - self.assertEqual(Build.objects.count(), builds_count + 1) + self.assertEqual(Build.objects.count(), builds_count + 2) newbuild = Build.objects.first() self.assertEqual( From 1e349ca7b829c05b53863dd4d9cd5f30e309de5c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Jun 2021 12:51:52 +0200 Subject: [PATCH 12/16] Show a user's notification if the build can't be retriggered Using 400 as response results in a blank page. It's better to clearly communicate what's the problem and redirect to the build listing page. --- readthedocs/builds/views.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 404eb9a387f..074ec5bd443 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -7,7 +7,6 @@ from django.contrib import messages from django.contrib.auth.decorators import login_required from django.http import ( - HttpResponseBadRequest, HttpResponseForbidden, HttpResponseRedirect, ) @@ -71,9 +70,12 @@ def post(self, request, project_slug): version=version, ) if build != Build.objects.filter(version=version).first(): - # Return bad request if the build re-triggered is not the - # latest for that version - return HttpResponseBadRequest() + messages.add_message( + request, + messages.ERROR, + "This build can't be re-triggered because it's not the latest build for this version.", + ) + return HttpResponseRedirect(request.path) else: # Use generic query when triggering a normal build version = get_object_or_404( From 5f663d4fa7bd031f848ebea5d05a1b2820d3545b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Jun 2021 12:52:48 +0200 Subject: [PATCH 13/16] Move "build_to_retrigger" block behind the "if build_pk" conditional --- readthedocs/builds/views.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 074ec5bd443..20d4da09b44 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -48,6 +48,7 @@ class BuildTriggerMixin: @method_decorator(login_required) def post(self, request, project_slug): + commit_to_retrigger = None project = get_object_or_404(Project, slug=project_slug) if not AdminPermission.is_admin(request.user, project): @@ -76,6 +77,18 @@ def post(self, request, project_slug): "This build can't be re-triggered because it's not the latest build for this version.", ) return HttpResponseRedirect(request.path) + + # Set either the build to re-trigger it or None + build_to_retrigger = version.builds.filter(pk=build_pk).first() + if build_to_retrigger: + commit_to_retrigger = build_to_retrigger.commit + log.info( + 'Re-triggering build. project=%s version=%s commit=%s build=%s', + project.slug, + version.slug, + build_to_retrigger.commit, + build_to_retrigger.pk + ) else: # Use generic query when triggering a normal build version = get_object_or_404( @@ -83,19 +96,6 @@ def post(self, request, project_slug): slug=version_slug, ) - # Set either the build to re-trigger it or None - build_to_retrigger = version.builds.filter(pk=build_pk).first() - commit_to_retrigger = None - if build_to_retrigger: - commit_to_retrigger = build_to_retrigger.commit - log.info( - 'Re-triggering build. project=%s version=%s commit=%s build=%s', - project.slug, - version.slug, - build_to_retrigger.commit, - build_to_retrigger.pk - ) - update_docs_task, build = trigger_build( project=project, version=version, From 2f9244ee3fb2e826255954a5aa4ff694ff7941e4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 8 Jun 2021 14:10:42 +0200 Subject: [PATCH 14/16] Small refactor to reuse the build variable and reduce 1 db query --- readthedocs/builds/views.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 20d4da09b44..3722dde24bc 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -65,12 +65,12 @@ def post(self, request, project_slug): project=project, ) - build = get_object_or_404( + build_to_retrigger = get_object_or_404( Build.objects.all(), pk=build_pk, version=version, ) - if build != Build.objects.filter(version=version).first(): + if build_to_retrigger != Build.objects.filter(version=version).first(): messages.add_message( request, messages.ERROR, @@ -79,7 +79,6 @@ def post(self, request, project_slug): return HttpResponseRedirect(request.path) # Set either the build to re-trigger it or None - build_to_retrigger = version.builds.filter(pk=build_pk).first() if build_to_retrigger: commit_to_retrigger = build_to_retrigger.commit log.info( From 0db1c8f5cf4088354ff2432f8bfc1407b7d7e461 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 8 Jun 2021 14:23:58 +0200 Subject: [PATCH 15/16] Fix tests when triggering an invalid build --- readthedocs/rtd_tests/tests/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index 2d9df3a5045..d0929c30ef8 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -326,9 +326,9 @@ def test_rebuild_invalid_specific_commit(self, mock): } ) - # It should return 400 because we are re-triggering a build of a - # non-lastest build for that version - self.assertEqual(r.status_code, 400) + # It should return 302 and show a message to the user because we are + # re-triggering a build of a non-lastest build for that version + self.assertEqual(r.status_code, 302) class TestSearchAnalyticsView(TestCase): From 4db6b3437999295786e78d154a0bc56125532221 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 9 Jun 2021 09:16:07 +0200 Subject: [PATCH 16/16] Lint --- readthedocs/builds/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/builds/views.py b/readthedocs/builds/views.py index 3722dde24bc..83b3cdf57f9 100644 --- a/readthedocs/builds/views.py +++ b/readthedocs/builds/views.py @@ -74,7 +74,8 @@ def post(self, request, project_slug): messages.add_message( request, messages.ERROR, - "This build can't be re-triggered because it's not the latest build for this version.", + "This build can't be re-triggered because it's " + "not the latest build for this version.", ) return HttpResponseRedirect(request.path)