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.

Sign up for GitHub

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: remove instructor info serializer validation #34802

Merged
merged 7 commits into from
May 16, 2024
Merged

fix: remove instructor info serializer validation #34802

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d17e6e
fix: remove instructor info serializer validation
rayzhou-bit May 15, 2024
10dc9c4
fix: allow_empty and allow_null on instructor info
rayzhou-bit May 15, 2024
4dec4bf
fix: remove allow_null instructor info
rayzhou-bit May 15, 2024
4e23d8a
fix: allow_blank
rayzhou-bit May 15, 2024
b28ebbb
fix: allow_null
rayzhou-bit May 15, 2024
058ae2c
fix: move allow_empty etc to child
rayzhou-bit May 15, 2024
948f08a
fix: remove allow_blank
rayzhou-bit May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions cms/djangoapps/contentstore/rest_api/v1/serializers/course_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,6 @@
from openedx.core.lib.api.serializers import CourseKeyField


class InstructorInfoSerializer(serializers.Serializer):
""" Serializer for instructor info """
name = serializers.CharField(allow_blank=True)
title = serializers.CharField(allow_blank=True, required=False)
organization = serializers.CharField(allow_blank=True, required=False)
image = serializers.CharField(allow_blank=True, required=False)
bio = serializers.CharField(allow_blank=True, required=False)


class InstructorsSerializer(serializers.Serializer):
""" Serializer for instructors """
instructors = InstructorInfoSerializer(many=True, allow_empty=True)


class CourseDetailsSerializer(serializers.Serializer):
""" Serializer for course details """
about_sidebar_html = serializers.CharField(allow_null=True, allow_blank=True)
Expand All @@ -40,7 +26,7 @@ class CourseDetailsSerializer(serializers.Serializer):
entrance_exam_enabled = serializers.CharField(allow_blank=True)
entrance_exam_id = serializers.CharField(allow_blank=True)
entrance_exam_minimum_score_pct = serializers.CharField(allow_blank=True)
instructor_info = InstructorsSerializer()
instructor_info = serializers.DictField(allow_empty=True)
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
instructor_info = serializers.DictField(allow_empty=True)
instructor_info = InstructorsSerializer(allow_empty=True)

I think that it makes sense to keep the serializer validation so that only the expected information comes through when it is present. I agree with allowing the serializer to be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I'm seeing with this is in the case when instructor_info isn't empty, we're not necessarily getting the instructors field.

I've seen the following, so I think it's possible to get instructors in other languages too.

"instructor_info": {
        "Professeurs": []
    }

Copy link
Member

Choose a reason for hiding this comment

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

The MFE version of the schedule and details page does not allow for the instructor key to be saved in other languages. The frontend expects that the key is in English. The key in varying languages is a bug introduced in the legacy page because of how it handled translations.

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 see... we still have some corrupt data due to the legacy page then. If we are turning off the MFE and using legacy from time to time due to issues, would it be better to accept the corrupt data for now? When the serializer runs into an issue, it entirely shuts down course details.

Copy link
Member

Choose a reason for hiding this comment

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

Adding allow_empty should fix the issue for when there is corrupt data. In the edge case that you pointed out, Proffesseurs is filtered out so instructor_info was empty. With this change, an empty dict will be sent via the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that specific code doesn't work but I get the general idea. I'll look into something that will pass checks.

intro_video = serializers.CharField(allow_null=True)
language = serializers.CharField(allow_null=True)
learning_info = serializers.ListField(child=serializers.CharField(allow_blank=True))
Expand Down
Loading