From 51d5af11ac5a263040b11cc4093baec236c5e511 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Feb 2026 19:07:53 +0000 Subject: [PATCH 1/4] Add preview-to-flow workflow: branch preview view, helper, template, and tests Co-authored-by: inducer <352067+inducer@users.noreply.github.com> Show branch preview links in the update course page (git-sha-table) Co-authored-by: inducer <352067+inducer@users.noreply.github.com> --- course/templates/course/git-sha-table.html | 12 + course/templates/course/update-branch.html | 48 ++++ course/versioning.py | 230 ++++++++++++++++++ relate/urls.py | 7 + tests/test_versioning.py | 270 +++++++++++++++++++++ 5 files changed, 567 insertions(+) create mode 100644 course/templates/course/update-branch.html diff --git a/course/templates/course/git-sha-table.html b/course/templates/course/git-sha-table.html index 0d9bdb2ac..f7cd67fe6 100644 --- a/course/templates/course/git-sha-table.html +++ b/course/templates/course/git-sha-table.html @@ -26,4 +26,16 @@ {{ git_url }} ({% trans "Manage access tokens" %}) + {% if remote_branches %} + + {% trans "Branches (click to preview)" %} + + {% for branch in remote_branches %} + + {{ branch }} » + {% endfor %} + + + {% endif %} diff --git a/course/templates/course/update-branch.html b/course/templates/course/update-branch.html new file mode 100644 index 000000000..3b1b60ef1 --- /dev/null +++ b/course/templates/course/update-branch.html @@ -0,0 +1,48 @@ +{% extends "course/course-base.html" %} +{% load i18n %} +{% load crispy_forms_tags %} + +{% block title %} + {{ form_description }} - {{ relate_site_name }} +{% endblock %} + +{% block content %} +

{{ form_description }}

+ + + + + + + + + + +
{% trans "Branch" %}{{ branch_name }}
{% trans "Branch SHA" %} + {% if branch_sha %} + {{ branch_sha }} + {% else %} + {% trans "Not found locally – use 'Fetch and preview' to retrieve it." %} + {% endif %} +
+ +
+ {% crispy form %} +
+ + {% if modified_flow_ids %} +
+

{% trans "Modified flows" %}

+

{% trans "The following flows were modified in this branch compared to the currently-live revision. Click a flow to preview it:" %}

+ +
+ {% endif %} +{% endblock %} diff --git a/course/versioning.py b/course/versioning.py index 128da1edf..ce334cc4c 100644 --- a/course/versioning.py +++ b/course/versioning.py @@ -36,6 +36,7 @@ ) import dulwich.client +import dulwich.objects import dulwich.repo import dulwich.web import paramiko @@ -601,6 +602,11 @@ def update_course(pctx: CoursePageContext): args=(course.identifier, ""))), "token_url": reverse("relate-manage_authentication_tokens", args=(course.identifier,)), + "remote_branches": sorted( + _remove_prefix(b"refs/remotes/origin/", ref).decode("utf-8", + errors="replace") + for ref in repo.get_refs() + if ref.startswith(b"refs/remotes/origin/")), }) assert form is not None @@ -614,6 +620,230 @@ def update_course(pctx: CoursePageContext): # }}} +# {{{ branch preview + + +def get_modified_flow_ids( + content_repo: Repo | SubdirRepoWrapper, + old_sha: bytes, + new_sha: bytes) -> list[str]: + """Return flow IDs that were added or modified between *old_sha* and *new_sha*.""" + + from django.core.exceptions import ObjectDoesNotExist + + from course.repo import get_repo_tree + + def get_flow_entry_map(commit_sha: bytes) -> dict[str, bytes]: + try: + flows_tree = get_repo_tree(content_repo, "flows", commit_sha) + except ObjectDoesNotExist: + return {} + + assert isinstance(flows_tree, dulwich.objects.Tree) + return { + entry.path[:-4].decode("utf-8"): entry.sha + for entry in flows_tree.items() + if entry.path.endswith(b".yml") + } + + old_entries = get_flow_entry_map(old_sha) + new_entries = get_flow_entry_map(new_sha) + + return sorted( + flow_id + for flow_id, sha in new_entries.items() + if old_entries.get(flow_id) != sha + ) + + +class BranchPreviewForm(StyledForm): + + def __init__(self, previewing: bool, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + + self.fields["navigate_to_modified_flow"] = forms.BooleanField( + label=_("Go directly to modified flow after preview"), + initial=True, + required=False) + + def add_button(desc: str, label: str | object) -> None: + self.helper.add_input(Submit(desc, label)) + + if previewing: + add_button("end_preview", _("End preview")) + + add_button("fetch_preview", _("Fetch and preview")) + add_button("preview", _("Preview")) + + +@login_required +@course_view +def update_course_from_branch( + pctx: CoursePageContext, + branch_name: str) -> http.HttpResponse: + + if not pctx.has_permission(PPerm.preview_content): + raise PermissionDenied() + + course = pctx.course + request = pctx.request + content_repo = pctx.repo + + from course.repo import SubdirRepoWrapper + if isinstance(content_repo, SubdirRepoWrapper): + repo = content_repo.repo + elif isinstance(content_repo, Repo): + repo = content_repo + else: + raise AssertionError("only actual dulwich repos allowed") + assert isinstance(repo, dulwich.repo.Repo) + + participation = pctx.participation + assert participation is not None + + previewing = bool(participation.preview_git_commit_sha) + + branch_ref = ("refs/remotes/origin/" + branch_name).encode() + + modified_flow_ids: list[str] | None = None + + form = None + response_form = None + if request.method == "POST": + form = BranchPreviewForm(previewing, request.POST) + + command = None + for cmd in [ + CourseRevisionCommand.fetch_preview, + CourseRevisionCommand.preview, + CourseRevisionCommand.end_preview]: + if cmd.value in form.data: + command = cmd + break + + if command is None: + raise SuspiciousOperation(_("invalid command")) + + if form.is_valid(): + navigate_to_modified_flow = form.cleaned_data.get( + "navigate_to_modified_flow", True) + + try: + if command == CourseRevisionCommand.end_preview: + participation.preview_git_commit_sha = None + participation.save() + messages.add_message(request, messages.INFO, + _("Preview ended.")) + else: + if command == CourseRevisionCommand.fetch_preview: + client, remote_path = \ + get_dulwich_client_and_remote_path_from_course(course) + fetch_pack_result = client.fetch(remote_path, repo) + assert isinstance( + fetch_pack_result, dulwich.client.FetchPackResult) + transfer_remote_refs(repo, fetch_pack_result) + messages.add_message(request, messages.SUCCESS, + _("Fetch successful.")) + + refs = repo.get_refs() + if branch_ref not in refs: + messages.add_message(request, messages.ERROR, + _("Branch '%(branch)s' not found. " + "Use 'Fetch and preview' to fetch it first.") + % {"branch": branch_name}) + response_form = form + else: + new_sha = refs[branch_ref] + + from course.validation import ( + ValidationError, + validate_course_content, + ) + try: + warnings = validate_course_content( + content_repo, + course.course_file, course.events_file, + new_sha, course=course) + except ValidationError as e: + messages.add_message(request, messages.ERROR, + _("Course content did not validate " + "successfully:
%s
" + "Update not applied.") % str(e)) + response_form = form + else: + if not warnings: + messages.add_message(request, messages.SUCCESS, + _("Course content validated " + "successfully.")) + else: + messages.add_message(request, messages.WARNING, + string_concat( + _("Course content validated OK, " + "with warnings: "), + "") + % ("".join( + "
  • %s: %s
    • " + % (w.location, w.text) + for w in warnings))) + + participation.preview_git_commit_sha = \ + new_sha.decode() + participation.save() + messages.add_message(request, messages.INFO, + _("Preview activated.")) + + if navigate_to_modified_flow: + active_sha = \ + course.active_git_commit_sha.encode() + modified_flow_ids = get_modified_flow_ids( + content_repo, active_sha, new_sha) + + if len(modified_flow_ids) == 1: + return redirect( + "relate-view_start_flow", + course.identifier, + modified_flow_ids[0]) + + except Exception as e: + import traceback + traceback.print_exc() + + messages.add_message(request, messages.ERROR, + string_concat( + pgettext("Starting of Error message", "Error"), + ": %(err_type)s %(err_str)s") + % {"err_type": type(e).__name__, + "err_str": str(e)}) + else: + response_form = form + + elif request.method == "GET": + pass + else: + raise SuspiciousOperation("unexpected method") + + if response_form is None: + previewing = bool(participation.preview_git_commit_sha) + form = BranchPreviewForm(previewing) + + assert form is not None + + refs = repo.get_refs() + branch_sha: str | None = None + if branch_ref in refs: + branch_sha = refs[branch_ref].decode() + + return render_course_page(pctx, "course/update-branch.html", { + "form": form, + "branch_name": branch_name, + "branch_sha": branch_sha, + "modified_flow_ids": modified_flow_ids, + "form_description": gettext("Preview Branch"), + }) + +# }}} + + # {{{ git endpoint diff --git a/relate/urls.py b/relate/urls.py index 390285e28..0ffb6a3b6 100644 --- a/relate/urls.py +++ b/relate/urls.py @@ -360,6 +360,13 @@ + "/update/$", course.versioning.update_course, name="relate-update_course"), + re_path(r"^course" + "/" + COURSE_ID_REGEX + + "/update/branch" + r"/(?P[-a-zA-Z0-9_./@]+)" + "/$", + course.versioning.update_course_from_branch, + name="relate-update_course_from_branch"), re_path(r"^course" "/" + COURSE_ID_REGEX + "/git" diff --git a/tests/test_versioning.py b/tests/test_versioning.py index 43579ebc4..29f65cbf0 100644 --- a/tests/test_versioning.py +++ b/tests/test_versioning.py @@ -1222,3 +1222,273 @@ def test_repo_is_in_subdir(self): from course.content import SubdirRepoWrapper self.assertIsInstance(mock_run_update.call_args[0][1], Repo) + + +# {{{ UpdateCourseFromBranchTest + +class UpdateCourseFromBranchTest(SingleCourseTestMixin, MockAddMessageMixing, + TestCase): + + default_branch_name = "main" + + def get_branch_preview_url(self, branch_name=None, course_identifier=None): + if branch_name is None: + branch_name = self.default_branch_name + if course_identifier is None: + course_identifier = self.course.identifier + from django.urls import reverse + return reverse("relate-update_course_from_branch", + args=[course_identifier, branch_name]) + + def post_branch_preview(self, branch_name=None, command="preview", + navigate_to_modified_flow=True, + force_login_instructor=True): + if branch_name is None: + branch_name = self.default_branch_name + data = {command: "on"} + if navigate_to_modified_flow: + data["navigate_to_modified_flow"] = "on" + url = self.get_branch_preview_url(branch_name) + if force_login_instructor: + user = self.instructor_participation.user + else: + user = self.get_logged_in_user(self.client) + with self.temporarily_switch_to_user(user): + return self.client.post(url, data) + + def test_no_permission(self): + with self.temporarily_switch_to_user(self.student_participation.user): + resp = self.client.get(self.get_branch_preview_url()) + self.assertEqual(resp.status_code, 403) + + def test_get(self): + with self.temporarily_switch_to_user(self.instructor_participation.user): + resp = self.client.get(self.get_branch_preview_url()) + self.assertEqual(resp.status_code, 200) + + def test_get_with_existing_branch(self): + # The repo has refs we can look up + from course.utils import CoursePageContext + rf = RequestFactory() + request = rf.get(self.get_branch_preview_url()) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + repo = pctx.repo + refs = repo.get_refs() + # Check that some refs exist (from the test course setup) + self.assertGreater(len(refs), 0) + + def test_unknown_command(self): + with self.temporarily_switch_to_user(self.instructor_participation.user): + resp = self.client.post( + self.get_branch_preview_url(), + {"unknown_command": "on"}) + self.assertEqual(resp.status_code, 400) + + @suppress_stdout_decorator(suppress_stderr=True) + def test_preview_branch_not_found(self): + # Branch doesn't exist locally → should show error + resp = self.post_branch_preview( + branch_name="nonexistent-branch", + command="preview", + navigate_to_modified_flow=False) + self.assertEqual(resp.status_code, 200) + self.assertAddMessageCalledWith("not found", reset=False) + + @suppress_stdout_decorator(suppress_stderr=True) + def test_end_preview(self): + # Set up a preview SHA first + self.instructor_participation.preview_git_commit_sha = ( + self.course.active_git_commit_sha) + self.instructor_participation.save() + + resp = self.post_branch_preview(command="end_preview") + self.assertEqual(resp.status_code, 200) + self.instructor_participation.refresh_from_db() + self.assertIsNone(self.instructor_participation.preview_git_commit_sha) + self.assertAddMessageCalledWith("Preview ended") + + @suppress_stdout_decorator(suppress_stderr=True) + def test_preview_with_branch(self): + # Set up a branch ref in the local repo + from course.utils import CoursePageContext + rf = RequestFactory() + request = rf.get(self.get_branch_preview_url()) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + from course.repo import SubdirRepoWrapper + repo = pctx.repo + if isinstance(repo, SubdirRepoWrapper): + real_repo = repo.repo + else: + real_repo = repo + head_sha = real_repo.head() + branch_ref = b"refs/remotes/origin/" + self.default_branch_name.encode() + real_repo[branch_ref] = head_sha + + with mock.patch( + "course.validation.validate_course_content", + return_value=[]): + resp = self.post_branch_preview( + command="preview", + navigate_to_modified_flow=False) + self.assertEqual(resp.status_code, 200) + self.instructor_participation.refresh_from_db() + self.assertIsNotNone( + self.instructor_participation.preview_git_commit_sha) + self.assertAddMessageCalledWith("Preview activated") + + @suppress_stdout_decorator(suppress_stderr=True) + def test_preview_navigate_single_modified_flow(self): + # Set up branch ref pointing to same SHA as active (no modifications) + # but mock get_modified_flow_ids to return a single flow + from course.utils import CoursePageContext + rf = RequestFactory() + request = rf.get(self.get_branch_preview_url()) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + from course.repo import SubdirRepoWrapper + repo = pctx.repo + if isinstance(repo, SubdirRepoWrapper): + real_repo = repo.repo + else: + real_repo = repo + head_sha = real_repo.head() + branch_ref = b"refs/remotes/origin/" + self.default_branch_name.encode() + real_repo[branch_ref] = head_sha + + with mock.patch( + "course.validation.validate_course_content", + return_value=[]): + with mock.patch( + "course.versioning.get_modified_flow_ids", + return_value=["quiz-test"]): + resp = self.post_branch_preview( + command="preview", + navigate_to_modified_flow=True) + + # Should redirect to the single modified flow + self.assertEqual(resp.status_code, 302) + from django.urls import reverse + expected_url = reverse("relate-view_start_flow", + args=[self.course.identifier, "quiz-test"]) + self.assertRedirects(resp, expected_url, fetch_redirect_response=False) + + @suppress_stdout_decorator(suppress_stderr=True) + def test_preview_navigate_multiple_modified_flows(self): + # Branch ref pointing to same SHA + from course.utils import CoursePageContext + rf = RequestFactory() + request = rf.get(self.get_branch_preview_url()) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + from course.repo import SubdirRepoWrapper + repo = pctx.repo + if isinstance(repo, SubdirRepoWrapper): + real_repo = repo.repo + else: + real_repo = repo + head_sha = real_repo.head() + branch_ref = b"refs/remotes/origin/" + self.default_branch_name.encode() + real_repo[branch_ref] = head_sha + + with mock.patch( + "course.validation.validate_course_content", + return_value=[]): + with mock.patch( + "course.versioning.get_modified_flow_ids", + return_value=["flow-a", "flow-b"]): + resp = self.post_branch_preview( + command="preview", + navigate_to_modified_flow=True) + + # Should render page (200) with modified_flow_ids in context + self.assertEqual(resp.status_code, 200) + self.assertIn("flow-a", resp.context["modified_flow_ids"]) + self.assertIn("flow-b", resp.context["modified_flow_ids"]) + + @suppress_stdout_decorator(suppress_stderr=True) + def test_form_not_valid(self): + with mock.patch( + "course.versioning.BranchPreviewForm.is_valid", + return_value=False): + resp = self.post_branch_preview( + command="preview", + navigate_to_modified_flow=False) + self.assertEqual(resp.status_code, 200) + + @suppress_stdout_decorator(suppress_stderr=True) + def test_fetch_preview_calls_fetch(self): + from dulwich.client import FetchPackResult + + from course.utils import CoursePageContext + rf = RequestFactory() + request = rf.get(self.get_branch_preview_url()) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + from course.repo import SubdirRepoWrapper + repo = pctx.repo + if isinstance(repo, SubdirRepoWrapper): + real_repo = repo.repo + else: + real_repo = repo + head_sha = real_repo.head() + + mock_client = mock.MagicMock() + mock_client.fetch.return_value = FetchPackResult( + refs={b"HEAD": head_sha, + b"refs/heads/main": head_sha}, + symrefs={}, + agent="Git") + + with mock.patch( + "course.versioning." + "get_dulwich_client_and_remote_path_from_course", + return_value=(mock_client, "/remote")): + with mock.patch( + "course.versioning.transfer_remote_refs") as mock_transfer: + with mock.patch( + "course.validation.validate_course_content", + return_value=[]): + resp = self.post_branch_preview( + command="fetch_preview", + navigate_to_modified_flow=False) + + self.assertEqual(mock_client.fetch.call_count, 1) + self.assertEqual(mock_transfer.call_count, 1) + # Response may be 200 (branch not found after mock) or 200 (found & previewed) + self.assertEqual(resp.status_code, 200) + + +class GetModifiedFlowIdsTest(SingleCourseTestMixin, TestCase): + + def test_same_commit_no_changes(self): + from course.utils import CoursePageContext + rf = RequestFactory() + from django.urls import reverse + request = rf.get(reverse("relate-update_course", + args=[self.course.identifier])) + request.user = self.instructor_participation.user + with CoursePageContext(request, self.course.identifier) as pctx: + repo = pctx.repo + head_sha = repo.get_refs()[b"HEAD"] + result = versioning.get_modified_flow_ids(repo, head_sha, head_sha) + # Same commit → no modifications + self.assertEqual(result, []) + + def test_no_flows_tree(self): + # If flows tree doesn't exist in old or new commit, + # all flows in new are "modified" + old_sha = b"nonexistent000000000000000000000000000000" + new_sha = b"nonexistent000000000000000000000000000001" + + mock_repo = mock.MagicMock() + + from django.core.exceptions import ObjectDoesNotExist + with mock.patch( + "course.repo.get_repo_tree", + side_effect=ObjectDoesNotExist): + result = versioning.get_modified_flow_ids(mock_repo, old_sha, new_sha) + self.assertEqual(result, []) + +# }}} From ff48fa583462bd226686665f37956bc1d6c5d39e Mon Sep 17 00:00:00 2001 From: Andreas Kloeckner Date: Fri, 27 Feb 2026 15:06:12 -0600 Subject: [PATCH 2/4] Update baseline --- .basedpyright/baseline.json | 104 ++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/.basedpyright/baseline.json b/.basedpyright/baseline.json index 3e07b0c24..6523e3e9c 100644 --- a/.basedpyright/baseline.json +++ b/.basedpyright/baseline.json @@ -22515,6 +22515,62 @@ "lineCount": 1 } }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 12, + "endColumn": 22, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 12, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 45, + "endColumn": 54, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 15, + "endColumn": 25, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 15, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 8, + "endColumn": 24, + "lineCount": 1 + } + }, + { + "code": "reportUnknownMemberType", + "range": { + "startColumn": 12, + "endColumn": 33, + "lineCount": 1 + } + }, { "code": "reportUnknownMemberType", "range": { @@ -82072,6 +82128,54 @@ "endColumn": 56, "lineCount": 1 } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 37, + "endColumn": 48, + "lineCount": 1 + } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 55, + "endColumn": 72, + "lineCount": 1 + } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 34, + "endColumn": 45, + "lineCount": 1 + } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 52, + "endColumn": 59, + "lineCount": 1 + } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 28, + "endColumn": 53, + "lineCount": 1 + } + }, + { + "code": "reportMissingParameterType", + "range": { + "startColumn": 28, + "endColumn": 50, + "lineCount": 1 + } } ], "./tests/test_views.py": [ From 8fecdf9347aef436a40821fb2171078a49538242 Mon Sep 17 00:00:00 2001 From: Andreas Kloeckner Date: Tue, 21 Apr 2026 18:58:20 -0500 Subject: [PATCH 3/4] Clean up the logic a bit --- course/templates/course/update-branch.html | 2 +- course/versioning.py | 33 ++++++++-------------- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/course/templates/course/update-branch.html b/course/templates/course/update-branch.html index 3b1b60ef1..f00d7f225 100644 --- a/course/templates/course/update-branch.html +++ b/course/templates/course/update-branch.html @@ -33,7 +33,7 @@

    {{ form_description }}

    {% if modified_flow_ids %}

    {% trans "Modified flows" %}

    -

    {% trans "The following flows were modified in this branch compared to the currently-live revision. Click a flow to preview it:" %}

    +

    {% trans "The following flows were modified in this branch compared to the currently-live revision. Click a flow to preview:" %}

      {% for flow_id in modified_flow_ids %}
    • diff --git a/course/versioning.py b/course/versioning.py index 21c3d8385..cd780ab2f 100644 --- a/course/versioning.py +++ b/course/versioning.py @@ -660,7 +660,6 @@ def update_course(pctx: CoursePageContext): # {{{ branch preview - def get_modified_flow_ids( content_repo: Repo | SubdirRepoWrapper, old_sha: bytes, @@ -743,10 +742,6 @@ def update_course_from_branch( branch_ref = ("refs/remotes/origin/" + branch_name).encode() - modified_flow_ids: list[str] | None = None - - form = None - response_form = None if request.method == "POST": form = BranchPreviewForm(previewing, request.POST) @@ -789,7 +784,6 @@ def update_course_from_branch( _("Branch '%(branch)s' not found. " "Use 'Fetch and preview' to fetch it first.") % {"branch": branch_name}) - response_form = form else: new_sha = refs[branch_ref] @@ -807,7 +801,6 @@ def update_course_from_branch( _("Course content did not validate " "successfully:
      %s
      " "Update not applied.") % str(e)) - response_form = form else: if not warnings: messages.add_message(request, messages.SUCCESS, @@ -831,10 +824,9 @@ def update_course_from_branch( _("Preview activated.")) if navigate_to_modified_flow: - active_sha = \ - course.active_git_commit_sha.encode() modified_flow_ids = get_modified_flow_ids( - content_repo, active_sha, new_sha) + content_repo, + course.active_git_commit_sha.encode(), new_sha) if len(modified_flow_ids) == 1: return redirect( @@ -852,29 +844,28 @@ def update_course_from_branch( ": %(err_type)s %(err_str)s") % {"err_type": type(e).__name__, "err_str": str(e)}) - else: - response_form = form elif request.method == "GET": - pass - else: - raise SuspiciousOperation("unexpected method") - - if response_form is None: previewing = bool(participation.preview_git_commit_sha) form = BranchPreviewForm(previewing) - assert form is not None + else: + raise SuspiciousOperation("unexpected method") refs = repo.get_refs() - branch_sha: str | None = None + branch_sha: bytes | None = None + modified_flow_ids: list[str] | None = None if branch_ref in refs: - branch_sha = refs[branch_ref].decode() + branch_sha = refs[branch_ref] + + modified_flow_ids = get_modified_flow_ids( + content_repo, + course.active_git_commit_sha.encode(), branch_sha) return render_course_page(pctx, "course/update-branch.html", { "form": form, "branch_name": branch_name, - "branch_sha": branch_sha, + "branch_sha": branch_sha.decode() if branch_sha is not None else None, "modified_flow_ids": modified_flow_ids, "form_description": gettext("Preview Branch"), }) From 4ec600af27119701b4baadffc9a20f845ca34d30 Mon Sep 17 00:00:00 2001 From: Andreas Kloeckner Date: Sat, 2 May 2026 09:03:35 -0500 Subject: [PATCH 4/4] Factor out fetch and validate --- course/versioning.py | 190 +++++++++++++++++++++---------------------- 1 file changed, 93 insertions(+), 97 deletions(-) diff --git a/course/versioning.py b/course/versioning.py index cd780ab2f..6098278ad 100644 --- a/course/versioning.py +++ b/course/versioning.py @@ -333,6 +333,62 @@ def set_up_new_course(request: http.HttpRequest) -> http.HttpResponse: # }}} +def fetch_course( + request: http.HttpRequest, + course: Course, + repo: Repo, + ): + client, remote_path = \ + get_dulwich_client_and_remote_path_from_course(course) + fetch_pack_result = client.fetch(remote_path, repo) + transfer_remote_refs(repo, fetch_pack_result) + messages.add_message(request, messages.SUCCESS, + _("Fetch successful.")) + + return fetch_pack_result + + +def validate_course( + request: http.HttpRequest, + course: Course, + content_repo: Repo | SubdirRepoWrapper, + new_sha: bytes, + ): + from course.validation import ( + ValidationError, + validate_course_content, + ) + try: + warnings = validate_course_content( + content_repo, + course.course_file, course.events_file, + new_sha, course=course) + except ValidationError as e: + messages.add_message(request, messages.ERROR, + _("Course content did not validate " + "successfully:
      %s
      " + "Update not applied.") % str(e)) + + return False + else: + if not warnings: + messages.add_message(request, messages.SUCCESS, + _("Course content validated " + "successfully.")) + else: + messages.add_message(request, messages.WARNING, + string_concat( + _("Course content validated OK, " + "with warnings: "), + "
        %s
      ") + % ("".join( + "
    • %s: %s
      • " + % (w.location, w.text) + for w in warnings))) + + return True + + # {{{ update def is_ancestor_commit( @@ -381,14 +437,8 @@ def run_course_update_command( CourseRevisionCommand.fetch, CourseRevisionCommand.fetch_update, CourseRevisionCommand.fetch_preview]: - client, remote_path = \ - get_dulwich_client_and_remote_path_from_course(pctx.course) - - fetch_pack_result = client.fetch(remote_path, repo) - - assert isinstance(fetch_pack_result, dulwich.client.FetchPackResult) - transfer_remote_refs(repo, fetch_pack_result) + fetch_pack_result = fetch_course(request, pctx.course, repo) remote_head_sha = cast("bytes", fetch_pack_result.refs[b"HEAD"]) if prevent_discarding_revisions: # Guard against bad scenario: @@ -422,37 +472,9 @@ def run_course_update_command( return - # {{{ validate - - from course.validation import ValidationError, validate_course_content - try: - warnings = validate_course_content( - content_repo, pctx.course.course_file, pctx.course.events_file, - new_sha, course=pctx.course) - except ValidationError as e: - from traceback import print_exc - print_exc() - - messages.add_message(request, messages.ERROR, - _("Course content did not validate successfully:
      %s
      " - "Update not applied.") % str(e)) + if not validate_course(request, pctx.course, content_repo, new_sha): return - else: - if not warnings: - messages.add_message(request, messages.SUCCESS, - _("Course content validated successfully.")) - else: - messages.add_message(request, messages.WARNING, - string_concat( - _("Course content validated OK, with warnings: "), - "
        %s
      ") - % ("".join( - f"
    • {w.location}: {w.text}
      • " - for w in warnings))) - - # }}} - if command in [CourseRevisionCommand.fetch_preview, CourseRevisionCommand.preview]: messages.add_message(request, messages.INFO, _("Preview activated.")) @@ -660,31 +682,34 @@ def update_course(pctx: CoursePageContext): # {{{ branch preview -def get_modified_flow_ids( - content_repo: Repo | SubdirRepoWrapper, - old_sha: bytes, - new_sha: bytes) -> list[str]: - """Return flow IDs that were added or modified between *old_sha* and *new_sha*.""" - +def get_flow_entry_map( + content_repo: Repo | SubdirRepoWrapper, + commit_sha: bytes + ) -> dict[str, bytes]: from django.core.exceptions import ObjectDoesNotExist from course.repo import get_repo_tree + try: + flows_tree = get_repo_tree(content_repo, "flows", commit_sha) + except ObjectDoesNotExist: + return {} - def get_flow_entry_map(commit_sha: bytes) -> dict[str, bytes]: - try: - flows_tree = get_repo_tree(content_repo, "flows", commit_sha) - except ObjectDoesNotExist: - return {} + assert isinstance(flows_tree, dulwich.objects.Tree) + return { + entry.path[:-4].decode("utf-8"): entry.sha + for entry in flows_tree.items() + if entry.path.endswith(b".yml") + } - assert isinstance(flows_tree, dulwich.objects.Tree) - return { - entry.path[:-4].decode("utf-8"): entry.sha - for entry in flows_tree.items() - if entry.path.endswith(b".yml") - } - old_entries = get_flow_entry_map(old_sha) - new_entries = get_flow_entry_map(new_sha) +def get_modified_flow_ids( + content_repo: Repo | SubdirRepoWrapper, + old_sha: bytes, + new_sha: bytes) -> list[str]: + """Return flow IDs that were added or modified between *old_sha* and *new_sha*.""" + + old_entries = get_flow_entry_map(content_repo, old_sha) + new_entries = get_flow_entry_map(content_repo, new_sha) return sorted( flow_id @@ -694,7 +719,6 @@ def get_flow_entry_map(commit_sha: bytes) -> dict[str, bytes]: class BranchPreviewForm(StyledForm): - def __init__(self, previewing: bool, *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) @@ -709,6 +733,7 @@ def add_button(desc: str, label: str | object) -> None: if previewing: add_button("end_preview", _("End preview")) + add_button("fetch", _("Fetch")) add_button("fetch_preview", _("Fetch and preview")) add_button("preview", _("Preview")) @@ -747,6 +772,7 @@ def update_course_from_branch( command = None for cmd in [ + CourseRevisionCommand.fetch, CourseRevisionCommand.fetch_preview, CourseRevisionCommand.preview, CourseRevisionCommand.end_preview]: @@ -767,17 +793,16 @@ def update_course_from_branch( participation.save() messages.add_message(request, messages.INFO, _("Preview ended.")) - else: - if command == CourseRevisionCommand.fetch_preview: - client, remote_path = \ - get_dulwich_client_and_remote_path_from_course(course) - fetch_pack_result = client.fetch(remote_path, repo) - assert isinstance( - fetch_pack_result, dulwich.client.FetchPackResult) - transfer_remote_refs(repo, fetch_pack_result) - messages.add_message(request, messages.SUCCESS, - _("Fetch successful.")) - + elif command in [ + CourseRevisionCommand.fetch, + CourseRevisionCommand.fetch_preview + ]: + fetch_course(request, course, repo) + + if command in [ + CourseRevisionCommand.fetch_preview, + CourseRevisionCommand.preview + ]: refs = repo.get_refs() if branch_ref not in refs: messages.add_message(request, messages.ERROR, @@ -787,36 +812,7 @@ def update_course_from_branch( else: new_sha = refs[branch_ref] - from course.validation import ( - ValidationError, - validate_course_content, - ) - try: - warnings = validate_course_content( - content_repo, - course.course_file, course.events_file, - new_sha, course=course) - except ValidationError as e: - messages.add_message(request, messages.ERROR, - _("Course content did not validate " - "successfully:
      %s
      " - "Update not applied.") % str(e)) - else: - if not warnings: - messages.add_message(request, messages.SUCCESS, - _("Course content validated " - "successfully.")) - else: - messages.add_message(request, messages.WARNING, - string_concat( - _("Course content validated OK, " - "with warnings: "), - "
        %s
      ") - % ("".join( - "
    • %s: %s
      • " - % (w.location, w.text) - for w in warnings))) - + if validate_course(request, course, repo, new_sha): participation.preview_git_commit_sha = \ new_sha.decode() participation.save()