From de62ca70ac20f3d380958de338e78d729331a6ad Mon Sep 17 00:00:00 2001 From: Hina Khadim Date: Mon, 24 Mar 2025 16:41:17 +0500 Subject: [PATCH] fix: replace is_locked with linkState to handle various link types in Course Optimizer (#36406) --- .../core/course_optimizer_provider.py | 30 +++++++---- .../tests/test_course_optimizer_provider.py | 17 ++++--- .../v0/serializers/course_optimizer.py | 1 + cms/djangoapps/contentstore/tasks.py | 20 ++++++-- .../contentstore/tests/test_tasks.py | 50 +++++++++++++++++-- 5 files changed, 94 insertions(+), 24 deletions(-) diff --git a/cms/djangoapps/contentstore/core/course_optimizer_provider.py b/cms/djangoapps/contentstore/core/course_optimizer_provider.py index c52ee68656..66353980d5 100644 --- a/cms/djangoapps/contentstore/core/course_optimizer_provider.py +++ b/cms/djangoapps/contentstore/core/course_optimizer_provider.py @@ -5,7 +5,7 @@ import json from user_tasks.conf import settings as user_tasks_settings from user_tasks.models import UserTaskArtifact, UserTaskStatus -from cms.djangoapps.contentstore.tasks import CourseLinkCheckTask +from cms.djangoapps.contentstore.tasks import CourseLinkCheckTask, LinkState from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import get_xblock from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import usage_key_with_run @@ -79,11 +79,12 @@ def generate_broken_links_descriptor(json_content, request_user): Returns a Data Transfer Object for frontend given a list of broken links. ** Example json_content structure ** - Note: is_locked is true if the link is a studio link and returns 403 + Note: link_state is locked if the link is a studio link and returns 403 + link_state is external-forbidden if the link is not a studio link and returns 403 [ - ['block_id_1', 'link_1', is_locked], - ['block_id_1', 'link_2', is_locked], - ['block_id_2', 'link_3', is_locked], + ['block_id_1', 'link_1', link_state], + ['block_id_1', 'link_2', link_state], + ['block_id_2', 'link_3', link_state], ... ] @@ -128,16 +129,16 @@ def generate_broken_links_descriptor(json_content, request_user): for item in json_content: block_id, link, *rest = item if rest: - is_locked_flag = bool(rest[0]) + link_state = rest[0] else: - is_locked_flag = False + link_state = '' usage_key = usage_key_with_run(block_id) block = get_xblock(usage_key, request_user) xblock_node_tree, xblock_dictionary = _update_node_tree_and_dictionary( block=block, link=link, - is_locked=is_locked_flag, + link_state=link_state, node_tree=xblock_node_tree, dictionary=xblock_dictionary ) @@ -145,7 +146,7 @@ def generate_broken_links_descriptor(json_content, request_user): return _create_dto_recursive(xblock_node_tree, xblock_dictionary) -def _update_node_tree_and_dictionary(block, link, is_locked, node_tree, dictionary): +def _update_node_tree_and_dictionary(block, link, link_state, node_tree, dictionary): """ Inserts a block into the node tree and add its attributes to the dictionary. @@ -180,7 +181,8 @@ def _update_node_tree_and_dictionary(block, link, is_locked, node_tree, dictiona 'category': 'chapter', 'url': 'url_1', 'locked_links': [...], - 'broken_links': [...] + 'broken_links': [...], + 'external_forbidden_links': [...], } ..., } @@ -210,8 +212,13 @@ def _update_node_tree_and_dictionary(block, link, is_locked, node_tree, dictiona f'/course/{block.course_id}/editor/{block.category}/{block.location}' ) - if is_locked: + # The link_state == True condition is maintained for backward compatibility. + # Previously, the is_locked attribute was used instead of link_state. + # If is_locked is True, it indicates that the link is locked. + if link_state is True or link_state == LinkState.LOCKED: updated_dictionary[xblock_id].setdefault('locked_links', []).append(link) + elif link_state == LinkState.EXTERNAL_FORBIDDEN: + updated_dictionary[xblock_id].setdefault('external_forbidden_links', []).append(link) else: updated_dictionary[xblock_id].setdefault('broken_links', []).append(link) @@ -268,6 +275,7 @@ def _create_dto_recursive(xblock_node, xblock_dictionary): 'url': xblock_data.get('url', ''), 'brokenLinks': xblock_data.get('broken_links', []), 'lockedLinks': xblock_data.get('locked_links', []), + 'externalForbiddenLinks': xblock_data.get('external_forbidden_links', []) }) else: # Non-leaf node category = xblock_data.get('category', None) diff --git a/cms/djangoapps/contentstore/core/tests/test_course_optimizer_provider.py b/cms/djangoapps/contentstore/core/tests/test_course_optimizer_provider.py index 5dd098956e..72bd412032 100644 --- a/cms/djangoapps/contentstore/core/tests/test_course_optimizer_provider.py +++ b/cms/djangoapps/contentstore/core/tests/test_course_optimizer_provider.py @@ -8,6 +8,7 @@ from cms.djangoapps.contentstore.core.course_optimizer_provider import ( _update_node_tree_and_dictionary, _create_dto_recursive ) +from cms.djangoapps.contentstore.tasks import LinkState class TestLinkCheckProvider(CourseTestCase): @@ -61,7 +62,7 @@ class TestLinkCheckProvider(CourseTestCase): } } result_tree, result_dictionary = _update_node_tree_and_dictionary( - self.mock_block, 'example_link', True, {}, {} + self.mock_block, 'example_link', LinkState.LOCKED, {}, {} ) self.assertEqual(expected_tree, result_tree) @@ -92,7 +93,7 @@ class TestLinkCheckProvider(CourseTestCase): } } result_tree, result_dictionary = _update_node_tree_and_dictionary( - self.mock_block, 'example_link', True, {}, {} + self.mock_block, 'example_link', LinkState.LOCKED, {}, {} ) self.assertEqual(expected_dictionary, result_dictionary) @@ -118,7 +119,8 @@ class TestLinkCheckProvider(CourseTestCase): 'displayName': 'Block Name', 'url': '/block/1', 'brokenLinks': ['broken_link_1', 'broken_link_2'], - 'lockedLinks': ['locked_link'] + 'lockedLinks': ['locked_link'], + 'externalForbiddenLinks': ['forbidden_link_1'], } ] } @@ -143,7 +145,8 @@ class TestLinkCheckProvider(CourseTestCase): 'display_name': 'Block Name', 'url': '/block/1', 'broken_links': ['broken_link_1', 'broken_link_2'], - 'locked_links': ['locked_link'] + 'locked_links': ['locked_link'], + 'external_forbidden_links': ['forbidden_link_1'], } } expected = _create_dto_recursive(mock_node_tree, mock_dictionary) @@ -174,7 +177,8 @@ class TestLinkCheckProvider(CourseTestCase): 'displayName': 'Block Name', 'url': '/block/1', 'brokenLinks': ['broken_link_1', 'broken_link_2'], - 'lockedLinks': ['locked_link'] + 'lockedLinks': ['locked_link'], + 'externalForbiddenLinks': ['forbidden_link_1'], } ] } @@ -211,7 +215,8 @@ class TestLinkCheckProvider(CourseTestCase): 'display_name': 'Block Name', 'url': '/block/1', 'broken_links': ['broken_link_1', 'broken_link_2'], - 'locked_links': ['locked_link'] + 'locked_links': ['locked_link'], + 'external_forbidden_links': ['forbidden_link_1'], } } expected = _create_dto_recursive(mock_node_tree, mock_dictionary) diff --git a/cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py index a7b38e0727..7411192d16 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py @@ -12,6 +12,7 @@ class LinkCheckBlockSerializer(serializers.Serializer): url = serializers.CharField(required=True, allow_null=False, allow_blank=False) brokenLinks = serializers.ListField(required=False) lockedLinks = serializers.ListField(required=False) + externalForbiddenLinks = serializers.ListField(required=False) class LinkCheckUnitSerializer(serializers.Serializer): diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index c9776c15e6..d9bf6a9b1f 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -100,6 +100,15 @@ ALL_ALLOWED_XBLOCKS = frozenset( ) +class LinkState: + """ + Links State Enumeration + """ + BROKEN = 'broken' + LOCKED = 'locked' + EXTERNAL_FORBIDDEN = 'external-forbidden' + + def clone_instance(instance, field_values): """ Clones a Django model instance. @@ -1334,7 +1343,8 @@ def _filter_by_status(results): Statuses: 200: OK. No need to do more - 403: Forbidden. Record as locked link. + 403: Forbidden. Record as locked link if it is studio link. + 403: Forbidden. Record as external-forbidden link if it is external link None: Error. Retry up to 3 times. Other: Failure. Record as broken link. @@ -1347,7 +1357,7 @@ def _filter_by_status(results): Example return: [ - [block_id1, filtered_results_url1, is_locked], + [block_id1, filtered_results_url1, link_state], ... ], [ @@ -1364,9 +1374,11 @@ def _filter_by_status(results): elif status == 200: continue elif status == 403 and _is_studio_url(url): - filtered_results.append([block_id, url, True]) + filtered_results.append([block_id, url, LinkState.LOCKED]) + elif status == 403 and not _is_studio_url(url): + filtered_results.append([block_id, url, LinkState.EXTERNAL_FORBIDDEN]) else: - filtered_results.append([block_id, url, False]) + filtered_results.append([block_id, url, LinkState.BROKEN]) return filtered_results, retry_list diff --git a/cms/djangoapps/contentstore/tests/test_tasks.py b/cms/djangoapps/contentstore/tests/test_tasks.py index 9eae1a518e..98d2b2f008 100644 --- a/cms/djangoapps/contentstore/tests/test_tasks.py +++ b/cms/djangoapps/contentstore/tests/test_tasks.py @@ -31,6 +31,7 @@ from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disa from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory # lint-amnesty, pylint: disable=wrong-import-order from ..tasks import ( + LinkState, export_olx, update_special_exams_and_publish, rerun_course, @@ -239,10 +240,18 @@ class CheckBrokenLinksTaskTest(ModuleStoreTestCase): ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@1", "http://example.com/valid"], ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@2", "http://example.com/invalid"], ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", f'http://{settings.CMS_BASE}/locked'], + ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", 'https://outsider.com/about'], ] self.expected_file_contents = [ - ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@2", "http://example.com/invalid", False], - ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", f"http://{settings.CMS_BASE}/locked", True], + ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@2", "http://example.com/invalid", LinkState.BROKEN], + ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", + f"http://{settings.CMS_BASE}/locked", + LinkState.LOCKED + ], + ["block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", + 'https://outsider.com/about', + LinkState.EXTERNAL_FORBIDDEN + ], ] @mock.patch('cms.djangoapps.contentstore.tasks.UserTaskArtifact', autospec=True) @@ -250,7 +259,7 @@ class CheckBrokenLinksTaskTest(ModuleStoreTestCase): @mock.patch('cms.djangoapps.contentstore.tasks._save_broken_links_file', autospec=True) @mock.patch('cms.djangoapps.contentstore.tasks._write_broken_links_to_file', autospec=True) @mock.patch('cms.djangoapps.contentstore.tasks._validate_urls_access_in_batches', autospec=True) - def test_check_broken_links_stores_broken_and_locked_urls( + def test_check_broken_links_stores_broken_locked_and_forbidden_urls( self, mock_validate_urls, mock_write_broken_links_to_file, @@ -287,6 +296,11 @@ class CheckBrokenLinksTaskTest(ModuleStoreTestCase): "url": f"http://{settings.CMS_BASE}/locked", "status": 403, }, + { + "block_id": "block-v1:edX+DemoX+Demo_Course+type@vertical+block@3", + "url": "https://outsider.com/about", + "status": 403, + } ] _check_broken_links(mock_task, mock_user.id, mock_course_key_string, 'en') # pylint: disable=no-value-for-parameter @@ -473,6 +487,36 @@ class CheckBrokenLinksTaskTest(ModuleStoreTestCase): assert len(retry_list) == 1 # The input with status = None assert retry_list[0][1] == '5' # The only URL fit for a retry operation (status == None) + def test_filter_by_status(self): + """ + Test the _filter_by_status function to ensure it correctly categorize links + based on the given status codes and returns appropriate lists of filtered + results and retry attempts. + """ + # Test data + results = [ + {'status': 200, 'block_id': 'block1', 'url': 'https://example.com'}, + {'status': None, 'block_id': 'block2', 'url': 'https://retry.com'}, + {'status': 403, 'block_id': 'block3', 'url': 'https://' + settings.CMS_BASE}, + {'status': 403, 'block_id': 'block4', 'url': 'https://external.com'}, + {'status': 404, 'block_id': 'block5', 'url': 'https://broken.com'} + ] + + expected_filtered_results = [ + ['block3', 'https://' + settings.CMS_BASE, LinkState.LOCKED], + ['block4', 'https://external.com', LinkState.EXTERNAL_FORBIDDEN], + ['block5', 'https://broken.com', LinkState.BROKEN], + ] + + expected_retry_list = [ + ['block2', 'https://retry.com'] + ] + + filtered_results, retry_list = _filter_by_status(results) + + self.assertEqual(filtered_results, expected_filtered_results) + self.assertEqual(retry_list, expected_retry_list) + @patch("cms.djangoapps.contentstore.tasks._validate_user", return_value=MagicMock()) @patch("cms.djangoapps.contentstore.tasks._scan_course_for_links", return_value=["url1", "url2"]) @patch(