-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: [FC-86] extend courseware api with new fields #36845
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
feat: [FC-86] extend courseware api with new fields #36845
Conversation
|
Thanks for the pull request, @andrii-hantkovskyi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
f389467 to
c43e1be
Compare
|
Hi @diana-villalvazo-wgu, would you have a chance to review this PR when you have a moment? |
|
Hi @andrii-hantkovskyi , why are you tagging me for review on all of these PRs? By the way, thank you for making detailed pull request descriptions--they are very helpful--but you should not call them "ADRs". An ADR is separate piece of documentation which you'd write if you were making some decisions that you wanted to record more formally than a PR descrption. ADR info. |
|
@kdmccormick, sorry for tagging, I thought you were a reviewer for this project as well, have already changed, so sorry Thank you for the clarification about ADR |
|
No problem @andrii-hantkovskyi . Glad to see these pages getting converted, best of luck. |
| advertised_start = serializers.CharField() | ||
| course_price = serializers.CharField() | ||
| pre_requisite_courses = serializers.ListField( | ||
| child=serializers.CharField(), |
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 add a PrerequisiteCourseSerializer that will return serialized data instead of a string with a tuple?
class PrerequisiteCourseSerializer(serializers.Serializer):
"""
Serializer for prerequisite course data with key and display name.
"""
key = serializers.CharField()
display = serializers.CharField()
| child=serializers.CharField(), | |
| child=PrerequisiteCourseSerializer(), |
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.
added
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.
Resolved
| allow_blank=True, | ||
| allow_null=True, | ||
| default=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.
Please add a new field display_number_with_default
display_number_with_default = serializers.CharField()
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.
added
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.
Resolved
| allow_blank=True, | ||
| allow_null=True, | ||
| default=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.
Let's add display_org_with_default
org = serializers.CharField(source='display_org_with_default')
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.
added
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.
Resolved
|
|
||
|
|
||
| @method_decorator(transaction.non_atomic_requests, name='dispatch') | ||
| class CoursewareInformation(RetrieveAPIView): |
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.
Please update the docstring and check if all API fields are actually in it
I found the following fields for which the op is missing:
* content_type_gating_enabled: Whether the content type gating is enabled for the course
* show_calculator: Whether the calculator should be shown in the course details page
* can_access_proctored_exams: Whether the user is eligible to access proctored exams
* notes: An object containing note settings for the course
* enabled: Boolean indicating whether edxnotes feature is enabled for the course
* visible: Boolean indicating whether notes are visible in the course
* marketing_url: The marketing URL for the course
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.
added
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.
Please remember to update this docstring after the final version of the API content is approved
cbc169b to
ca43788
Compare
| * ocw_links: A list of OpenCourseWare links for the course | ||
| * prerequisites: A list of prerequisite courses for the course |
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.
I managed to get data for the prerequisites field through the settings on the Schedule & Details, but on the legacy page nothing is displayed in the sidebar. This may indicate that the API is returning data in an incorrect format. Could you check this?
"prerequisites": [
"course-v1:openedx2+123+2024"
]
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.
fixed
| * studio_url: The URL to the course in Studio, if the user is staff | ||
| * is_cosmetic_price_enabled: Boolean indicating whether the cosmetic price feature is enabled | ||
| * overview: The overview HTML content for the course | ||
| * ocw_links: A list of OpenCourseWare links for the course |
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.
Could you please add some test instructions or an explanation of how to get the data for ocw_links? I'm not entirely sure where the data for ocw_links comes from.
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.
ocw_links is a part of the course about section, but as far as I see, it's not configurable right now
0e1aa04 to
3c47f1c
Compare
PKulkoRaccoonGang
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.
I reviewed this PR and left some comments. Some fields in this API need additional validation (as per Brian's question in [DEPR]: Deprecation of Prerequisites and OCW Links sections) and Catalog About Page API Design.
| allow_blank=True, | ||
| allow_null=True, | ||
| default=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.
Resolved
| advertised_start = serializers.CharField() | ||
| course_price = serializers.CharField() | ||
| pre_requisite_courses = serializers.ListField( | ||
| child=serializers.CharField(), |
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.
Resolved
| start_date_is_still_default = serializers.BooleanField() | ||
| advertised_start = serializers.CharField() | ||
| course_price = serializers.CharField() | ||
| pre_requisite_courses = serializers.ListField( |
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.
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.
After some investigation, I can confirm that there is no BE logic that would populate these two fields. I left a reply in the DEPR and updated this PR accordingly.
| allow_blank=True, | ||
| allow_null=True, | ||
| default=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.
Resolved
| return get_course_about_section(self.request, self.course, "overview") | ||
|
|
||
| @property | ||
| def ocw_links(self): |
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.
Regarding the prerequisites and ocw_links fields, we will need to decide whether we leave them in this view after discussions in this DEPR
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.
I left a reply in the DEPR concerning theses two fields - it looks like we can remove them. I updated the PR accordingly.
| return get_prerequisite_courses_display(self.course) | ||
|
|
||
| @property | ||
| def about_sidebar_html(self): |
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.
According to the discussions in Catalog About Page API Design, it was decided to change sidebar_html_enabled (WaffleSwitch "course_experience.enable_about_sidebar_html) sidebar content via frontend plugin slots. It seems that we don't need this field in the serializer anymore.
|
|
||
|
|
||
| @method_decorator(transaction.non_atomic_requests, name='dispatch') | ||
| class CoursewareInformation(RetrieveAPIView): |
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.
Please remember to update this docstring after the final version of the API content is approved
|
Once the DEPR related work is done, do you need me to merge this or does someone on your team have merge rights? |
447f071 to
84f0690
Compare
|
@feanil looks like it would be great if you could provide the merge when this is ready |
|
@Serj-N looks like the tests weren't updated after the last two commits, could you address? |
c98fd28 to
a41dcb7
Compare
a41dcb7 to
ed69cb5
Compare
|
@PKulkoRaccoonGang @NiedielnitsevIvan are your concerns addressed? |
The comments from the review have been resolved.
|
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. |
|
Hey 👋 just writing to say that I've identified a bug with this PR. I have a resolution here #37338 that I'm rushing out since it is currently causing production outages for us but I'd like to ask your help updating the test suites to cover those cases that weren't caught in unit testing |
Hey @nsprenkle. |
|
@nsprenkle am I correct that what broke was the ecommerce service that is deprecated on the Open edX side? If so I'm not entirely sure how we would even go about testing this. |
|
@cmltaWt0 I think we can add the tests after the fact for possible null values for that field. Especially since it's from a deprecated system, that makes sense that it could be null sometimes. The bug fix has already merged so I think we just need to get the tests landed. It looks like the bug is in this test. We should split this into to tests and explicitly make sure we allow null values like the upstream objects could produce. |

Extend Courseware API with New Fields
This PR extends the Courseware API with several new fields to enhance the course metadata provided to API consumers.
The changes include updates to the serializer, model properties, and comprehensive tests to ensure the correct behavior of each new field.
Key changes:
CourseInfoSerializer:show_courseware_linkis_course_fullcan_enrollinvitation_onlyis_shib_courseallow_anonymousecommerce_checkoutsingle_paid_modeecommerce_checkout_linkcourse_image_urlsstart_date_is_still_defaultadvertised_startcourse_pricepre_requisite_coursessidebar_html_enabledcourse_about_section_htmlCoursewareMetaclass to provide these fields' values.These changes provide richer and more detailed course metadata to frontend clients, supporting improved user experiences and new UI features.
Unit and integration tests cover all new fields and confirm correct values based on various course and user scenarios.