-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: Enhance course optimizer to detect previous run links and expand scanning scope #37104
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
Conversation
1e03b7c to
3d680b5
Compare
3d680b5 to
cf48844
Compare
e9a1b00 to
760d224
Compare
cms/djangoapps/contentstore/rest_api/v0/serializers/course_optimizer.py
Outdated
Show resolved
Hide resolved
| from user_tasks.models import UserTaskArtifact, UserTaskStatus | ||
|
|
||
| from cms.djangoapps.contentstore.tasks import CourseLinkCheckTask, LinkState | ||
| from cms.djangoapps.contentstore.tasks import CourseLinkCheckTask, LinkState, _get_urls |
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.
_ variables should be avoided to import under PEP8 coding guidelines
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.
let's rename _get_urls to extract_content_URLs_from_course may be to avoid PEP8 violiation.
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.
It's updated as per PEP-8
| # .. toggle_default: False | ||
| # .. toggle_description: When enabled, allows the Course Optimizer to detect and update links pointing to previous course runs. | ||
| # This feature enables instructors to fix internal course links that still point to old course runs | ||
| # after creating a course rerun. |
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.
Description says its still pointing to old run, whereas its a feature to show previous run links on new run. Correct me if i am wrong @Faraz32123
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.
Descriptions seems right to me, but little changes can be made here.
# .. toggle_description: When enabled, allows the Course Optimizer to detect and update links pointing to previous course runs.
# This feature allows instructors to see & update internal course links that are still pointing to previous course run
# in a new course run.
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.
Updated description as per above.
| @@ -10,7 +10,8 @@ def cms_api_filter(endpoints): | |||
| """ | |||
| filtered = [] | |||
| CMS_PATH_PATTERN = re.compile( | |||
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.
cms_paths = [
"xblock",
"videos",
"video_transcripts",
"file_assets",
"youtube_transcripts",
"link_check",
"link_check_status"
]
CMS_PATH_PATTERN = re.compile(r"^/api/contentstore/v0/(" + "|".join(cms_paths) + r")")
is it possible to write this way to avoid multiline regexp
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.
Reverted the file, as these APIs are not intended to be exposed in the Swagger UI
760d224 to
d34c891
Compare
Faraz32123
left a comment
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.
LGTM! 👌
0253811 to
585bb9d
Compare
| main_content = None | ||
|
|
||
| if main_content is None: | ||
| main_content = {"sections": []} |
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.
can we directly add main_content = {"sections": []} in except block.
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.
It is updated as per above.
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.
It is updated as per above.
585bb9d to
b8bd541
Compare
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
| # after creating a course rerun. | ||
| # .. toggle_use_cases: temporary | ||
| # .. toggle_creation_date: 2025-07-21 | ||
| # .. toggle_target_removal_date: None |
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.
@djoseph-apphelix @Faraz32123 If this is a temporary toggle, why is there no removal date?
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.
We'll handle this in upcoming PR!
|
@djoseph-apphelix @Faraz32123 Could you provide me the broader context of this pull request? For example, is there a public document outlining the plan, or are there related PRs you could link to? The TNL ticket is internal to 2U so it is not accessible to me. |
Problem: When we create a rerun of a course through publisher, internal links of a course inside course updates, handouts, custom pages or even course content still points to older run instead of newly created run. Due to which learners are redirected to previous run/course instead of staying inside the new run/course. So, basically the purpose of this PR is to enhance the functionality of the course optimizer APIs to also detect prevRunLinks as mentioned in the PR description within a course with expanded scope and it also detects broken links. There will be more PRs to update these links & we'll link those PRs with this. @jristau1984 Do we have any public doc for this? |
Description
This PR extends the functionality of the Course Optimizer page with the following enhancements:
Previous Run Link Detection:
contentstore.enable_course_optimizer_check_prev_run_links. If the flag is disabled, the scan covers only broken, external forbidden, and locked links.Plain Text Link Scanning:
hrefandsrcattributes.Expanded Scanning Scope:
Course Updates,Handouts, andCustom Pagesin the link scan process, in addition to the existing course content.Supporting information
Waffle Flag:
contentstore.enable_course_optimizer_check_prev_run_linkscontrols previous run detection.Enhanced API Response: New
course_updatesandcustom_pagesfields with link categorization.Improved URL Detection: Advanced regex patterns detect URLs in HTML attributes and plain text content.
Previous Run Link Detection: Identifies and categorizes links pointing to previous course runs using
CourseRerunStateJira
Testing Instructions
CourseRerunStaterecord is created.course_updatesandcustom_pagesarrays with the expected data.