diff --git a/.basedpyright/baseline.json b/.basedpyright/baseline.json index 9324164a5..6db295694 100644 --- a/.basedpyright/baseline.json +++ b/.basedpyright/baseline.json @@ -22133,6 +22133,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": { @@ -80970,6 +81026,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": [ 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..f00d7f225 --- /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:" %}

+ +
+ {% endif %} +{% endblock %} diff --git a/course/versioning.py b/course/versioning.py index a45f86f60..6098278ad 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 @@ -332,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: "), + "") + % ("".join( + "
  • %s: %s
    • " + % (w.location, w.text) + for w in warnings))) + + return True + + # {{{ update def is_ancestor_commit( @@ -380,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: @@ -421,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: "), - "") - % ("".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.")) @@ -639,6 +662,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 @@ -652,6 +680,195 @@ def update_course(pctx: CoursePageContext): # }}} +# {{{ branch preview + +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 {} + + 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") + } + + +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 + 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", _("Fetch")) + 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() + + if request.method == "POST": + form = BranchPreviewForm(previewing, request.POST) + + command = None + for cmd in [ + CourseRevisionCommand.fetch, + 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.")) + 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, + _("Branch '%(branch)s' not found. " + "Use 'Fetch and preview' to fetch it first.") + % {"branch": branch_name}) + else: + new_sha = refs[branch_ref] + + if validate_course(request, course, repo, new_sha): + participation.preview_git_commit_sha = \ + new_sha.decode() + participation.save() + messages.add_message(request, messages.INFO, + _("Preview activated.")) + + if navigate_to_modified_flow: + modified_flow_ids = get_modified_flow_ids( + content_repo, + course.active_git_commit_sha.encode(), 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)}) + + elif request.method == "GET": + previewing = bool(participation.preview_git_commit_sha) + form = BranchPreviewForm(previewing) + + else: + raise SuspiciousOperation("unexpected method") + + refs = repo.get_refs() + branch_sha: bytes | None = None + modified_flow_ids: list[str] | None = None + if branch_ref in refs: + 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.decode() if branch_sha is not None else None, + "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 b5286967e..082b6a801 100644 --- a/tests/test_versioning.py +++ b/tests/test_versioning.py @@ -1230,6 +1230,276 @@ def test_repo_is_in_subdir(self): 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, []) + +# }}} + + @python_repo_class class BasicCourse: course_dot_yml: ClassVar[str] = """