From 73364712ba5e424c845e5d724b9e2c2c4e6f5733 Mon Sep 17 00:00:00 2001 From: cef Date: Wed, 4 Sep 2024 09:48:22 -0500 Subject: [PATCH 1/5] feat: set links for CourseAuthoring discussion alert --- .../rest_api/v1/views/tests/test_course_index.py | 5 +++-- cms/envs/common.py | 8 +++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py index eafc2b37aa0c..189f2496a427 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py @@ -1,6 +1,7 @@ """ Unit tests for course index outline. """ +from django.conf import settings from django.test import RequestFactory from django.urls import reverse from rest_framework import status @@ -62,7 +63,7 @@ def test_course_index_response(self): "advance_settings_url": f"/settings/advanced/{self.course.id}" }, "discussions_incontext_feedback_url": "", - "discussions_incontext_learnmore_url": "", + "discussions_incontext_learnmore_url": settings.DISCUSSIONS_INCONTEXT_LEARNMORE_URL, "is_custom_relative_dates_active": True, "initial_state": None, "initial_user_clipboard": { @@ -103,7 +104,7 @@ def test_course_index_response_with_show_locators(self): "advance_settings_url": f"/settings/advanced/{self.course.id}" }, "discussions_incontext_feedback_url": "", - "discussions_incontext_learnmore_url": "", + "discussions_incontext_learnmore_url": settings.DISCUSSIONS_INCONTEXT_LEARNMORE_URL, "is_custom_relative_dates_active": False, "initial_state": { "expanded_locators": [ diff --git a/cms/envs/common.py b/cms/envs/common.py index 8daa08aeb1c8..46f7d2a3d2a5 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2814,8 +2814,14 @@ BRAZE_COURSE_ENROLLMENT_CANVAS_ID = '' +######################## Discussion Forum settings ######################## + +# Feedback link in upgraded discussion notification alert DISCUSSIONS_INCONTEXT_FEEDBACK_URL = '' -DISCUSSIONS_INCONTEXT_LEARNMORE_URL = '' + +# Learn More link in upgraded discussion notification alert +# pylint: disable=line-too-long +DISCUSSIONS_INCONTEXT_LEARNMORE_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/manage_discussions/discussions.html" #### django-simple-history## # disable indexing on date field its coming django-simple-history. From 45d328f9fd75b0d8a761de419df1e25dd32a435a Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Thu, 26 Sep 2024 13:15:28 +0000 Subject: [PATCH 2/5] fix: Delete flaky test `test_get_user_group_id_for_partition` (#35545) This test failed on 2024-08-06 and 2024-09-24 but passed on re-run. Deleted according to flaky test process: https://openedx.atlassian.net/wiki/spaces/AC/pages/4306337795/Flaky+Test+Process Flaky test ticket: https://2u-internal.atlassian.net/browse/CR-7071 --- xmodule/partitions/tests/test_partitions.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/xmodule/partitions/tests/test_partitions.py b/xmodule/partitions/tests/test_partitions.py index fde6afd3141b..41f26b14db52 100644 --- a/xmodule/partitions/tests/test_partitions.py +++ b/xmodule/partitions/tests/test_partitions.py @@ -462,21 +462,6 @@ class TestPartitionService(PartitionServiceBaseClass): Test getting a user's group out of a partition """ - def test_get_user_group_id_for_partition(self): - # assign the first group to be returned - user_partition_id = self.user_partition.id - groups = self.user_partition.groups - self.user_partition.scheme.current_group = groups[0] - - # get a group assigned to the user - group1_id = self.partition_service.get_user_group_id_for_partition(self.user, user_partition_id) - assert group1_id == groups[0].id - - # switch to the second group and verify that it is returned for the user - self.user_partition.scheme.current_group = groups[1] - group2_id = self.partition_service.get_user_group_id_for_partition(self.user, user_partition_id) - assert group2_id == groups[1].id - def test_caching(self): username = "psvc_cache_user" user_partition_id = self.user_partition.id From 740921ae21b158acbd18ebbfc3f7bc8c5fe674ab Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 25 Sep 2024 12:48:49 -0400 Subject: [PATCH 3/5] build: Manually pull some RTD Context. See https://about.readthedocs.com/blog/2024/07/addons-by-default/ for details but essentially RTD is changing how it's building docs and this will let us handle the change gracefully. --- docs/conf.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/conf.py b/docs/conf.py index f37fc32f6160..01280c6cd214 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -258,6 +258,16 @@ epub_exclude_files = ['search.html'] +# -- Read the Docs Specific Configuration +# Define the canonical URL if you are using a custom domain on Read the Docs +html_baseurl = os.environ.get("READTHEDOCS_CANONICAL_URL", "") + +# Tell Jinja2 templates the build is running on Read the Docs +if os.environ.get("READTHEDOCS", "") == "True": + if "html_context" not in globals(): + html_context = {} + html_context["READTHEDOCS"] = True + # -- Extension configuration ------------------------------------------------- # -- Options for intersphinx extension --------------------------------------- From 8f47c0b2744801b667d752ab5f066253b8f74582 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 26 Sep 2024 09:34:58 -0700 Subject: [PATCH 4/5] fix: whitespace issues in some capa problem index_dictionary content (#35543) --- .../content/search/tests/test_api.py | 4 +- .../content/search/tests/test_handlers.py | 2 +- xmodule/capa_block.py | 6 +- xmodule/tests/test_capa_block.py | 57 ++++++++++++------- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 2f43896c197e..4c6227af309f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -141,7 +141,7 @@ def setUp(self): "context_key": "lib:org1:lib", "org": "org1", "breadcrumbs": [{"display_name": "Library"}], - "content": {"problem_types": [], "capa_content": " "}, + "content": {"problem_types": [], "capa_content": ""}, "type": "library_block", "access_id": lib_access.id, "last_published": None, @@ -157,7 +157,7 @@ def setUp(self): "context_key": "lib:org1:lib", "org": "org1", "breadcrumbs": [{"display_name": "Library"}], - "content": {"problem_types": [], "capa_content": " "}, + "content": {"problem_types": [], "capa_content": ""}, "type": "library_block", "access_id": lib_access.id, "last_published": None, diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 8a6627e3902d..bdc4814d1c8f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -148,7 +148,7 @@ def test_create_delete_library_block(self, meilisearch_client): "context_key": "lib:orgA:lib_a", "org": "orgA", "breadcrumbs": [{"display_name": "Library Org A"}], - "content": {"problem_types": [], "capa_content": " "}, + "content": {"problem_types": [], "capa_content": ""}, "access_id": lib_access.id, "last_published": None, "created": created_date.timestamp(), diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 54ca0cbc312f..c1c650144b05 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -616,11 +616,15 @@ def index_dictionary(self): "", capa_content ) + # Strip out all other tags, leaving their content. But we want spaces between adjacent tags, so that + #
Option A
Option B
 + # becomes "Option A Option B" not "Option AOption B" (these will appear in search results) + capa_content = re.sub(r"<([^>]+)>", r" <\2>", capa_content) capa_content = re.sub( r"(\s| |//)+", " ", nh3.clean(capa_content, tags=set()) - ) + ).strip() capa_body = { "capa_content": capa_content, diff --git a/xmodule/tests/test_capa_block.py b/xmodule/tests/test_capa_block.py index d1c01e109718..c81b137f1b23 100644 --- a/xmodule/tests/test_capa_block.py +++ b/xmodule/tests/test_capa_block.py @@ -3290,7 +3290,7 @@ def test_response_types_ignores_non_response_tags(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['multiplechoiceresponse'], - 'content': {'display_name': name, 'capa_content': ' Label Some comment Apple Banana Chocolate Donut '}} + 'content': {'display_name': name, 'capa_content': 'Label Some comment Apple Banana Chocolate Donut'}} def test_response_types_multiple_tags(self): xml = textwrap.dedent(""" @@ -3328,7 +3328,7 @@ def test_response_types_multiple_tags(self): 'problem_types': {"optionresponse", "multiplechoiceresponse"}, 'content': { 'display_name': name, - 'capa_content': " Label Some comment Donut Buggy '1','2' " + 'capa_content': "Label Some comment Donut Buggy '1','2'" }, } ) @@ -3369,7 +3369,7 @@ def test_solutions_not_indexed(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': [], - 'content': {'display_name': name, 'capa_content': ' '}} + 'content': {'display_name': name, 'capa_content': ''}} def test_indexing_checkboxes(self): name = "Checkboxes" @@ -3390,7 +3390,7 @@ def test_indexing_checkboxes(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['choiceresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_dropdown(self): name = "Dropdown" @@ -3405,7 +3405,7 @@ def test_indexing_dropdown(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['optionresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_multiple_choice(self): name = "Multiple Choice" @@ -3424,7 +3424,7 @@ def test_indexing_multiple_choice(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['multiplechoiceresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_numerical_input(self): name = "Numerical Input" @@ -3446,7 +3446,7 @@ def test_indexing_numerical_input(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['numericalresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_text_input(self): name = "Text Input" @@ -3465,7 +3465,7 @@ def test_indexing_text_input(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['stringresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_non_latin_problem(self): sample_text_input_problem_xml = textwrap.dedent(""" @@ -3476,7 +3476,7 @@ def test_indexing_non_latin_problem(self): """) name = "Non latin Input" block = self._create_block(sample_text_input_problem_xml, name=name) - capa_content = " Δοκιμή με μεταβλητές με Ελληνικούς χαρακτήρες μέσα σε python: $FX1_VAL " + capa_content = "Δοκιμή με μεταβλητές με Ελληνικούς χαρακτήρες μέσα σε python: $FX1_VAL" block_dict = block.index_dictionary() assert block_dict['content']['capa_content'] == smart_str(capa_content) @@ -3503,7 +3503,7 @@ def test_indexing_checkboxes_with_hints_and_feedback(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['choiceresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_dropdown_with_hints_and_feedback(self): name = "Dropdown with Hints and Feedback" @@ -3523,7 +3523,7 @@ def test_indexing_dropdown_with_hints_and_feedback(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['optionresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_multiple_choice_with_hints_and_feedback(self): name = "Multiple Choice with Hints and Feedback" @@ -3543,7 +3543,7 @@ def test_indexing_multiple_choice_with_hints_and_feedback(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['multiplechoiceresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_numerical_input_with_hints_and_feedback(self): name = "Numerical Input with Hints and Feedback" @@ -3561,7 +3561,7 @@ def test_indexing_numerical_input_with_hints_and_feedback(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['numericalresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_text_input_with_hints_and_feedback(self): name = "Text Input with Hints and Feedback" @@ -3579,7 +3579,7 @@ def test_indexing_text_input_with_hints_and_feedback(self): assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': ['stringresponse'], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ').strip()}} def test_indexing_problem_with_html_tags(self): sample_problem_xml = textwrap.dedent(""" @@ -3598,14 +3598,33 @@ def test_indexing_problem_with_html_tags(self): """) name = "Mixed business" block = self._create_block(sample_problem_xml, name=name) - capa_content = textwrap.dedent(""" - This has HTML comment in it. - HTML end. - """) + capa_content = "This has HTML comment in it. HTML end." assert block.index_dictionary() ==\ {'content_type': ProblemBlock.INDEX_CONTENT_TYPE, 'problem_types': [], - 'content': {'display_name': name, 'capa_content': capa_content.replace('\n', ' ')}} + 'content': {'display_name': name, 'capa_content': capa_content}} + + def test_indexing_problem_with_no_whitespace_between_tags(self): + """ + The new (MFE) visual editor for capa problems renders the OLX without spaces between the tags. + We want to make sure the index description is still readable and has whitespace. + """ + sample_problem_xml = ( + "" + "
Question text here.
" + "
Option A
" + "
Option B
" + "" + "" + ) + name = "No spaces" + block = self._create_block(sample_problem_xml, name=name) + capa_content = "Question text here. Option A Option B" + assert block.index_dictionary() == { + 'content_type': ProblemBlock.INDEX_CONTENT_TYPE, + 'problem_types': ['choiceresponse'], + 'content': {'display_name': name, 'capa_content': capa_content}, + } def test_invalid_xml_handling(self): """ From 67b490cab46c6725528af2d8b0e53be8a45e79e0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 26 Sep 2024 09:35:17 -0700 Subject: [PATCH 5/5] fix: suppress errors+warnings when video is used in a content library (#35544) --- .../contentstore/views/transcripts_ajax.py | 3 +++ xmodule/video_block/transcripts_utils.py | 2 +- xmodule/video_block/video_block.py | 18 ++++++++++++------ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/transcripts_ajax.py b/cms/djangoapps/contentstore/views/transcripts_ajax.py index 892b76caae72..8cb7f455013b 100644 --- a/cms/djangoapps/contentstore/views/transcripts_ajax.py +++ b/cms/djangoapps/contentstore/views/transcripts_ajax.py @@ -649,6 +649,9 @@ def _get_item(request, data): Returns the item. """ usage_key = UsageKey.from_string(data.get('locator')) + if not usage_key.context_key.is_course: + # TODO: implement transcript support for learning core / content libraries. + raise TranscriptsRequestValidationException(_('Transcripts are not yet supported in content libraries.')) # This is placed before has_course_author_access() to validate the location, # because has_course_author_access() raises r if location is invalid. item = modulestore().get_item(usage_key) diff --git a/xmodule/video_block/transcripts_utils.py b/xmodule/video_block/transcripts_utils.py index e41f925295f0..132b8cff1e14 100644 --- a/xmodule/video_block/transcripts_utils.py +++ b/xmodule/video_block/transcripts_utils.py @@ -1074,7 +1074,7 @@ def get_transcript_from_learning_core(video_block, language, output_format, tran """ # TODO: Update to use Learning Core data models once static assets support # has been added. - raise NotImplementedError("Transcripts not supported.") + raise NotFoundError("No transcript - transcripts not supported yet by learning core components.") def get_transcript(video, lang=None, output_format=Transcript.SRT, youtube_id=None): diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index b4fddb63fa7a..782645c373b0 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -482,7 +482,7 @@ def get_html(self, view=STUDENT_VIEW, context=None): # lint-amnesty, pylint: di 'hide_downloads': is_public_view or is_embed, 'id': self.location.html_id(), 'block_id': str(self.location), - 'course_id': str(self.location.course_key), + 'course_id': str(self.context_key), 'video_id': str(self.edx_video_id), 'user_id': self.get_user_id(), 'is_embed': is_embed, @@ -510,8 +510,10 @@ def get_course_video_sharing_override(self): """ Return course video sharing options override or None """ + if not self.context_key.is_course: + return False # Only courses support this feature at all (not libraries) try: - course = get_course_by_id(self.course_id) + course = get_course_by_id(self.context_key) return getattr(course, 'video_sharing_options', None) # In case the course / modulestore does something weird @@ -523,11 +525,13 @@ def is_public_sharing_enabled(self): """ Is public sharing enabled for this video? """ + if not self.context_key.is_course: + return False # Only courses support this feature at all (not libraries) try: # Video share feature must be enabled for sharing settings to take effect - feature_enabled = PUBLIC_VIDEO_SHARE.is_enabled(self.location.course_key) + feature_enabled = PUBLIC_VIDEO_SHARE.is_enabled(self.context_key) except Exception as err: # pylint: disable=broad-except - log.exception(f"Error retrieving course for course ID: {self.location.course_key}") + log.exception(f"Error retrieving course for course ID: {self.context_key}") return False if not feature_enabled: return False @@ -552,11 +556,13 @@ def is_transcript_feedback_enabled(self): """ Is transcript feedback enabled for this video? """ + if not self.context_key.is_course: + return False # Only courses support this feature at all (not libraries) try: # Video transcript feedback must be enabled in order to show the widget - feature_enabled = TRANSCRIPT_FEEDBACK.is_enabled(self.location.course_key) + feature_enabled = TRANSCRIPT_FEEDBACK.is_enabled(self.context_key) except Exception as err: # pylint: disable=broad-except - log.exception(f"Error retrieving course for course ID: {self.location.course_key}") + log.exception(f"Error retrieving course for course ID: {self.context_key}") return False return feature_enabled