Repository navigation
feat: remove legacy Django unit editor #38985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
2d87434
2d839db
95b84bd
966629f
d354714
db13ea0
12c14ab
8edd15e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -158,10 +158,14 @@ def get_use_new_course_outline_page(self, obj): | |||||||||||||||||||
|
|
||||||||||||||||||||
| def get_use_new_unit_page(self, obj): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Method to get the use_new_unit_page switch | ||||||||||||||||||||
| Method to get the use_new_unit_page switch. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Always returns True. The legacy unit editor has been removed. | ||||||||||||||||||||
| This method will be removed in a follow-up cleanup. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| See https://github.com/openedx/edx-platform/issues/36275 | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. frontend-app-authoring has never read this key (nothing in its history matches
Suggested change
|
||||||||||||||||||||
| """ | ||||||||||||||||||||
| course_key = self.get_course_key() | ||||||||||||||||||||
| return toggles.use_new_unit_page(course_key) | ||||||||||||||||||||
| return True | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_use_new_course_team_page(self, obj): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,5 @@ | ||
| # pylint: disable=missing-module-docstring | ||
|
|
||
| # TODO: Rewrite several of these assertions so that they check the output of the REST or Python | ||
| # APIs rather than parsing HTML from the deprecated legacy frontend pages. In particular, any | ||
| # test case using override_waffle_flag(toggles.LEGACY_STUDIO_*, True) will need to be fixed. | ||
| # Part of https://github.com/openedx/edx-platform/issues/36275. | ||
|
|
||
| import copy | ||
| import re | ||
| import shutil | ||
|
|
@@ -21,7 +16,7 @@ | |
| from django.test import TestCase | ||
| from django.test.utils import override_settings | ||
| from django.urls import reverse | ||
| from edx_toggles.toggles.testutils import override_waffle_flag, override_waffle_switch | ||
| from edx_toggles.toggles.testutils import override_waffle_switch | ||
| from edxval.api import create_video, get_videos_for_course | ||
| from fs.osfs import OSFS | ||
| from lxml import etree | ||
|
|
@@ -30,11 +25,9 @@ | |
| from opaque_keys.edx.locations import CourseLocator | ||
| from path import Path as path | ||
|
|
||
| from cms.djangoapps.contentstore import toggles | ||
| from cms.djangoapps.contentstore.config import waffle | ||
| from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, CourseTestCase, get_url, parse_json | ||
| from cms.djangoapps.contentstore.utils import delete_course, reverse_course_url, reverse_url | ||
| from cms.djangoapps.contentstore.views.component import ADVANCED_COMPONENT_TYPES | ||
| from common.djangoapps.course_action_state.managers import CourseActionStateItemNotFoundError | ||
| from common.djangoapps.course_action_state.models import CourseRerunState, CourseRerunUIStateManager | ||
| from common.djangoapps.student import auth | ||
|
|
@@ -568,44 +561,6 @@ def setUp(self): | |
| ) | ||
| self.course = self.store.publish(self.course.location, self.user.id) | ||
|
|
||
| def check_components_on_page(self, component_types, expected_types): | ||
| """ | ||
| Ensure that the right types end up on the page. | ||
|
|
||
| component_types is the list of advanced components. | ||
|
|
||
| expected_types is the list of elements that should appear on the page. | ||
|
|
||
| expected_types and component_types should be similar, but not | ||
| exactly the same -- for example, 'video' in | ||
| component_types should cause 'Video' to be present. | ||
| """ | ||
| self.course.advanced_modules = component_types | ||
| self.store.update_item(self.course, self.user.id) | ||
|
|
||
| # just pick one vertical | ||
| resp = self.client.get_html(get_url('container_handler', self.vert_loc)) | ||
| for expected in expected_types: | ||
| self.assertContains(resp, expected) | ||
|
|
||
| @override_waffle_flag(toggles.LEGACY_STUDIO_UNIT_EDITOR, True) | ||
| @ddt.data("<script>alert(1)</script>", "alert('hi')", "</script><script>alert(1)</script>") | ||
| def test_container_handler_xss_prevent(self, malicious_code): | ||
| """ | ||
| Test that XSS attack is prevented | ||
| """ | ||
| resp = self.client.get_html(get_url('container_handler', self.vert_loc) + '?action=' + malicious_code) | ||
| # Test that malicious code does not appear in html | ||
| self.assertNotContains(resp, malicious_code) | ||
|
|
||
| @override_waffle_flag(toggles.LEGACY_STUDIO_UNIT_EDITOR, True) | ||
| def test_advanced_components_in_edit_unit(self): | ||
| # This could be made better, but for now let's just assert that we see the advanced modules mentioned in the | ||
| # page response HTML | ||
| self.check_components_on_page( | ||
| ADVANCED_COMPONENT_TYPES, | ||
| ['Word cloud', 'Annotation', 'split_test'], | ||
| ) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: two consecutive blank lines left here after the deleted methods above — worth squashing to one. |
||
| @ddt.data('/Fake/asset/displayname', '\\Fake\\asset\\displayname') | ||
| def test_export_on_invalid_displayname(self, invalid_displayname): | ||
|
|
@@ -700,14 +655,6 @@ def test_assets_overwrite(self): | |
| # Remove tempdir | ||
| shutil.rmtree(root_dir) | ||
|
|
||
| @override_waffle_flag(toggles.LEGACY_STUDIO_UNIT_EDITOR, True) | ||
| def test_advanced_components_require_two_clicks(self): | ||
| self.check_components_on_page(['word_cloud'], ['Word cloud']) | ||
|
|
||
| @override_waffle_flag(toggles.LEGACY_STUDIO_UNIT_EDITOR, True) | ||
| def test_edit_unit(self): | ||
| """Verifies rendering the editor in all the verticals in the given test course""" | ||
| self._check_verticals([self.vert_loc]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the only caller of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: same as above — two consecutive blank lines left after the deleted methods here too. |
||
| def _get_draft_counts(self, item): # pylint: disable=missing-function-docstring | ||
| cnt = 1 if not self.store.has_published_version(item) else 0 | ||
|
|
@@ -1520,11 +1467,10 @@ def test_get_json(handler): | |
| ) | ||
| self.assertEqual(resp.status_code, 200) # noqa: PT009 | ||
|
|
||
| # go look at the Edit page | ||
| # go look at the Edit page — now redirects to MFE unit editor | ||
| unit_key = course_key.make_usage_key('vertical', 'test_vertical') | ||
| with override_waffle_flag(toggles.LEGACY_STUDIO_UNIT_EDITOR, True): | ||
| resp = self.client.get_html(get_url('container_handler', unit_key)) | ||
| self.assertEqual(resp.status_code, 200) # noqa: PT009 | ||
| resp = self.client.get_html(get_url('container_handler', unit_key)) | ||
| self.assertEqual(resp.status_code, 302) # noqa: PT009 | ||
|
|
||
| def delete_item(category, name): | ||
| """ Helper method for testing the deletion of an xblock item. """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -194,23 +194,14 @@ def use_new_video_uploads_page(course_key): | |||
| LEGACY_STUDIO_COURSE_OUTLINE = CourseWaffleFlag('legacy_studio.course_outline', __name__) | ||||
|
|
||||
|
|
||||
| # .. toggle_name: legacy_studio.unit_editor | ||||
| # .. toggle_implementation: WaffleFlag | ||||
| # .. toggle_default: False | ||||
| # .. toggle_description: Temporarily fall back to the old Studio unit editing page. | ||||
| # .. toggle_use_cases: temporary | ||||
| # .. toggle_creation_date: 2025-03-14 | ||||
| # .. toggle_target_removal_date: 2025-09-14 | ||||
| # .. toggle_tickets: https://github.com/openedx/edx-platform/issues/36275 | ||||
| # .. toggle_warning: In Ulmo, this toggle will be removed. Only the new (React-based) experience will be available. | ||||
| LEGACY_STUDIO_UNIT_EDITOR = CourseWaffleFlag('legacy_studio.unit_editor', __name__) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Link #35261 from the description and add a line to the operator notes for the release this lands in, since
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @feanil Could you please elaborate on this comment a bit more? I’m not sure I fully understand what you mean.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. #35261 is the DEPR ticket for the legacy unit editor, add it to the References list in the description so the ticket and the PR that closes it point at each other. Then add a line to the Willow operator notes page, https://openedx.atlassian.net/wiki/spaces/COMM/pages/6476169217/Willow+-+Operator+Dev+Notes, saying the While you're in the description, the "until the MFE confirms it no longer reads this key" lines are out of date, the MFE never read it. |
||||
|
|
||||
|
|
||||
| def use_new_unit_page(course_key): | ||||
| """ | ||||
| Returns a boolean if new studio course outline mfe is enabled | ||||
| Returns a boolean if new studio unit page is enabled. | ||||
|
|
||||
| Always returns True. The legacy unit editor has been removed. | ||||
| This function will be removed in a follow-up cleanup once all callers are updated. | ||||
| """ | ||||
| return not LEGACY_STUDIO_UNIT_EDITOR.is_enabled(course_key) | ||||
| return True | ||||
|
|
||||
|
|
||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||||
| # .. toggle_name: contentstore.mock_video_uploads | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,7 +22,7 @@ | |||||||||||||||||||||||||||||||||||
| from xblock.runtime import Mixologist | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from cms.djangoapps.contentstore.helpers import get_parent_if_split_test, is_library_content, is_unit | ||||||||||||||||||||||||||||||||||||
| from cms.djangoapps.contentstore.toggles import libraries_v2_enabled, use_new_unit_page | ||||||||||||||||||||||||||||||||||||
| from cms.djangoapps.contentstore.toggles import libraries_v2_enabled | ||||||||||||||||||||||||||||||||||||
| from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import load_services_for_studio | ||||||||||||||||||||||||||||||||||||
| from common.djangoapps.edxmako.shortcuts import render_to_response | ||||||||||||||||||||||||||||||||||||
| from common.djangoapps.student.auth import has_course_author_access | ||||||||||||||||||||||||||||||||||||
|
|
@@ -130,44 +130,36 @@ def _load_mixed_class(category): | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| @require_GET | ||||||||||||||||||||||||||||||||||||
| @login_required | ||||||||||||||||||||||||||||||||||||
| def container_handler(request, usage_key_string): # pylint: disable=too-many-statements | ||||||||||||||||||||||||||||||||||||
| def container_handler(request, usage_key_string): | ||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||
| The restful handler for container xblock requests. | ||||||||||||||||||||||||||||||||||||
| Redirects to the MFE unit editor. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| GET | ||||||||||||||||||||||||||||||||||||
| html: returns the HTML page for editing a container | ||||||||||||||||||||||||||||||||||||
| json: not currently supported | ||||||||||||||||||||||||||||||||||||
| The legacy Django-template-based unit editor has been removed. | ||||||||||||||||||||||||||||||||||||
| This view exists for backward compatibility with any existing links to /container/<usage_key>. | ||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||
| from ..utils import get_unit_url | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from ..utils import get_container_handler_context, get_unit_url | ||||||||||||||||||||||||||||||||||||
| if 'text/html' not in request.META.get('HTTP_ACCEPT', 'text/html'): | ||||||||||||||||||||||||||||||||||||
| return HttpResponseBadRequest("Only supports HTML requests") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| usage_key = UsageKey.from_string(usage_key_string) | ||||||||||||||||||||||||||||||||||||
| except InvalidKeyError: | ||||||||||||||||||||||||||||||||||||
| raise Http404 # pylint: disable=raise-missing-from # noqa: B904 | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| with modulestore().bulk_operations(usage_key.course_key): | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| usage_key = UsageKey.from_string(usage_key_string) | ||||||||||||||||||||||||||||||||||||
| except InvalidKeyError: # Raise Http404 on invalid 'usage_key_string' | ||||||||||||||||||||||||||||||||||||
| raise Http404 # pylint: disable=raise-missing-from # noqa: B904 | ||||||||||||||||||||||||||||||||||||
| with modulestore().bulk_operations(usage_key.course_key): | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| course, xblock, lms_link, preview_lms_link = _get_item_in_course(request, usage_key) | ||||||||||||||||||||||||||||||||||||
| except ItemNotFoundError: | ||||||||||||||||||||||||||||||||||||
| return HttpResponseBadRequest() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if use_new_unit_page(course.id): | ||||||||||||||||||||||||||||||||||||
| if is_unit(xblock) or is_library_content(xblock): | ||||||||||||||||||||||||||||||||||||
| return redirect(get_unit_url(course.id, xblock.location)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if split_xblock := get_parent_if_split_test(xblock): | ||||||||||||||||||||||||||||||||||||
| return redirect(get_unit_url(course.id, split_xblock.location)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| container_handler_context = get_container_handler_context(request, usage_key, course, xblock) | ||||||||||||||||||||||||||||||||||||
| container_handler_context.update({ | ||||||||||||||||||||||||||||||||||||
| 'draft_preview_link': preview_lms_link, | ||||||||||||||||||||||||||||||||||||
| 'published_preview_link': lms_link, | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
| return render_to_response('container.html', container_handler_context) | ||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||
| return HttpResponseBadRequest("Only supports HTML requests") | ||||||||||||||||||||||||||||||||||||
| course, xblock, lms_link, preview_lms_link = _get_item_in_course(request, usage_key) | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the LMS links aren't used anymore.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||||||||||||||||||||||||||||||||||||
| except ItemNotFoundError: | ||||||||||||||||||||||||||||||||||||
| return HttpResponseBadRequest() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if is_unit(xblock) or is_library_content(xblock): | ||||||||||||||||||||||||||||||||||||
| return redirect(get_unit_url(course.id, xblock.location)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if split_xblock := get_parent_if_split_test(xblock): | ||||||||||||||||||||||||||||||||||||
| return redirect(get_unit_url(course.id, split_xblock.location)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| raise Http404 | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On master a With this change on a scratch course,
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed
Comment on lines
+156
to
+159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A split test's group verticals 404 now. The
Suggested change
Then add a |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| @require_GET | ||||||||||||||||||||||||||||||||||||
|
|
@@ -176,7 +168,8 @@ def container_handler(request, usage_key_string): # pylint: disable=too-many-st | |||||||||||||||||||||||||||||||||||
| def container_embed_handler(request, usage_key_string): # pylint: disable=too-many-statements | ||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||
| Returns an HttpResponse with HTML content for the container XBlock. | ||||||||||||||||||||||||||||||||||||
| The returned HTML is a chromeless rendering of the XBlock. | ||||||||||||||||||||||||||||||||||||
| The returned HTML is a chromeless rendering of the XBlock, used by the | ||||||||||||||||||||||||||||||||||||
| Authoring MFE to display xblock content in iframes. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| GET | ||||||||||||||||||||||||||||||||||||
| html: returns the HTML page for editing a container | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_new_unit_pageis kept in the serializer response for now as a field returning True. Once the MFE confirms it no longer reads this key from the waffle flags API, the field and its method can be removed in a follow-up PR.