Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: replace is_locked with linkState to handle various link types in Course Optimizer (#36406)

Browse Source
This commit is contained in:
Hina Khadim
2025-03-24 16:41:17 +05:00
committed by GitHub
parent 65cf7a9288
commit de62ca70ac
5 changed files with 94 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
cms/djangoapps/contentstore/core/course_optimizer_provider.py
View File

@@ -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)

17
cms/djangoapps/contentstore/core/tests/test_course_optimizer_provider.py
View File

@@ -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)

1
cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py
View File

@@ -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):

20
cms/djangoapps/contentstore/tasks.py
View File

@@ -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

50
cms/djangoapps/contentstore/tests/test_tasks.py
View File

@@ -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(