edx-platform
edx-platform copied to clipboard
feat(mobile_api): Add course access object to mobile course info API
Description
This PR is a follow-up to FC-0031 project for Mobile API updates https://github.com/openedx/edx-platform/issues/33304, and extends BlocksInfoInCourseView view with coursewareAccess data for a user in the form:
"coursewareAccess": {
"hasUnmetPrerequisites": <bool>,
"isTooEarly": <bool>,
"isStaff": <bool>
}
The coursewareAccess data should be provided only if it is requested by a staff or admin user, or by the users themselves.
Supporting information
This PR further extends the mobile BlocksInfoInCourseView that was introduced in the https://github.com/openedx/edx-platform/pull/33296
Testing instructions
Run a GET /api/mobile/{api_version}/course_info/blocks/?course_id=<course_id>&username=<username>&return_type=dict
Asses the response, check that fields "coursewareAccess" and "certificate" are in the response body.
Deadline
End of the month, as those changes are required to unblock the further works around mobile courseware enhancements.
Thanks for the pull request, @GlugovGrGlib! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
- supporting documentation
- Open edX discussion forum threads
- timeline information ("this must be merged by XX date", and why that is)
- partner information ("this is a course on edx.org")
- any other information that can help Product understand the context for the PR
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
Also, since this change:
- might require pulling data from different models or queries that need to be done on the db
- and increase the JSON response size
so, is there a plan to do performance & load testing before and after this change?
@miankhalid Thanks for the details, it seems like it shouldn't take long to update the response to the structure you've mentioned.
Also, I have checked the code around has_access, and it appears to be optimized in terms of the number of database queries. However, I appreciate you raising this concern. I will conduct a small load test to compare the performance before and after changes to the /api/mobile/{api_version}/course_info/blocks/ endpoint.
Great @GlugovGrGlib, thanks for making these changes!
Do let us know when the PR is ready for another pass.
@GlugovGrGlib @volodymyr-chekyrta here are some more things that should be added to the Blocks API (/api/mobile/v3/course_info/blocks/) that are currently available in the Enrollments API (/api/mobile/v3/users/{username}/course_enrollments/):
- Elements in red boxes are absolutely needed currently for app's working and especially analytics.
- Elements in green boxes would be needed in future for in-app payments in the apps.
cc @omerhabib26 @saeedbashir @moiz994 @touchapp
Hi @GlugovGrGlib Is this ready for another review?
Hi @GlugovGrGlib Is this ready for another review?
@moeez96 Not yet, I have to do the final changes today, so it should be ready by next week.
@moeez96 @miankhalid The PR is ready for the final review
The final response from the endpoint was extended with all requested fields except dynamic_deadline_upgrade and subscription_id
The course access messages can be accessed using $.course_access_details.courseware_access.has_access
Any specific reason of writing tests for
mobile_api/course_info/*inmobile_api/tests/*rather than the existingmobile_api/course_info/tests.py?
@moeez96 I have done this change to fix the Quality check. If tests are placed in the tests/* directory, it will be ignored during xsslint, this way it's possible to fix quality check without change to xsslint configuration - https://github.com/openedx/edx-platform/blob/master/scripts/xsslint_config.py#L49.
@GlugovGrGlib Can we then add the test code to files named lms/djangoapps/mobile_api/tests/test_course_info_serializers.py
and
lms/djangoapps/mobile_api/tests/test_course_info_views.py?
This way file seggregation will remain intact and xsslint will also test these files. Otherwise the first impression a code reader gets is that these tests are written for mobile_api views and serializers. Rest looks good to me!
@moeez96 Thank you for the suggestion, I have renamed test files
@moeez96 @miankhalid If everything looks good from your side, please merge this PR whenever you're ready.
@GlugovGrGlib 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
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.
2U Release Notice: This PR has been deployed to the edX production environment.