Skip to content
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

fix: video page api error #34883

Merged
merged 1 commit into from
May 31, 2024
Merged

fix: video page api error #34883

merged 1 commit into from
May 31, 2024

Conversation

rayzhou-bit
Copy link
Contributor

@rayzhou-bit rayzhou-bit commented May 30, 2024

Description

When there is corruption in course video data, we can run into the following error:

File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/video_storage_handlers.py", line 693, in _get_values
    (__, values['course_video_image_url']), = list(current_course[0].items())
IndexError: list index out of range

This causes the api to not return course video data even if one url is missing (and subsequently causes the course video page to not load at all).

This PR ensures that the IndexError is not triggered so the api call can at least return as much information as it can.

Supporting information

https://2u-internal.atlassian.net/browse/TNL-11628

Testing instructions

Test on stage by accessing https://course-authoring.stage.edx.org/course/course-v1:edx+gc_6+2024_T1/videos/.
It should not show an error.

if current_course:
values['course_video_image_url'] = current_course[0][course_id]
else:
values['course_video_image_url'] = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
values['course_video_image_url'] = ''
values['course_video_image_url'] = None

@@ -690,7 +690,10 @@ def _get_values(video, course):
for attr in attrs:
if attr == 'courses':
current_course = [c for c in video['courses'] if course_id in c]
(__, values['course_video_image_url']), = list(current_course[0].items())
if current_course:
values['course_video_image_url'] = current_course[0][course_id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you don't keep the original code and just wrap it in a the if/else that you creates here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed better in terms of clarity. I believe using list() and items() iterates twice as well which is worse for performance but that's not a big deal here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I am still slightly confused why you are indexing the dict with the course_id. Is that how you get the image url? If you could add a comment in to code to clarify this it would help me understand more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the video looked like this from working it out backwards. And current_course is only populated with entries if course_id in c.

video = {
  'courses': [
    {
      'course_id_123': 'img.url',
      ...
    },
  ],
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example! It definitely clears up my confusion

@rayzhou-bit rayzhou-bit force-pushed the 2u/fix/video-page-api-error branch from 1cc23d4 to 046c3cb Compare May 31, 2024 17:14
@rayzhou-bit rayzhou-bit force-pushed the 2u/fix/video-page-api-error branch from 046c3cb to 74079ea Compare May 31, 2024 17:29
@rayzhou-bit rayzhou-bit merged commit f3413fd into master May 31, 2024
46 of 47 checks passed
@rayzhou-bit rayzhou-bit deleted the 2u/fix/video-page-api-error branch May 31, 2024 19:15
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants