-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(mobile_api): Add course access object to mobile course info API #34273
feat(mobile_api): Add course access object to mobile course info API #34273
Conversation
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:
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. |
043f344
to
684161d
Compare
0572981
to
f83dc86
Compare
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.
Apologies for the late review on this PR, I was waiting to have a chat with @volodymyr-chekyrta which we did last night.
Approach reconsideration
The change this PR brings to the Blocks API is altogether a new dictionary with a new name and new key-value pairs i.e:
"coursewareAccess": {
"hasUnmetPrerequisites": <bool>,
"isTooEarly": <bool>,
"isStaff": <bool>
}
while what we wanted was the same courseware_access
access object that is sent as part of the Enrollments API (/api/mobile/v3/users/{username}}/course_enrollments/
) i.e:
{
"courseware_access": {
"has_access": <bool>,
"error_code": <enum> {"course_not_started", "audit_expired", "not_visible_to_user", "unfulfilled_milestones"},
"developer_message": <string>,
"user_message": <string>,
"additional_context_user_message": <string>
}
}
here's a sample response for a real user on one of our sandboxes:
{
"courseware_access": {
"has_access": false,
"error_code": "audit_expired",
"developer_message": "User {username} had access to {course_run_key} until 2024-01-15 13:15:13+00:00",
"user_message": "Access expired on Jan 15, 2024",
"additional_context_user_message": "Access to {course_name} expired on Jan 15, 2024"
}
}
Benefit of the old response structure
We'll be able to easily copy over the previous error handling logic that we had in our prod apps already in the market without missing out any of the edge cases.
Code references
iOS
- https://github.com/openedx/edx-app-ios/blob/a07db33dd0e2ef0d2c30582c79b2e7273c7e1aac/Source/OEXCoursewareAccess.h
- https://github.com/openedx/edx-app-ios/blob/a07db33dd0e2ef0d2c30582c79b2e7273c7e1aac/Source/OEXCoursewareAccess.m
Android
- https://github.com/openedx/edx-app-android/blob/459dee23fc14f76abb4d1e05d31e54032aa6d894/OpenEdXMobile/src/main/java/org/edx/mobile/model/api/CoursewareAccess.java
- https://github.com/openedx/edx-app-android/blob/459dee23fc14f76abb4d1e05d31e54032aa6d894/OpenEdXMobile/src/main/java/org/edx/mobile/model/api/AccessError.java
Also, since this change:
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 |
f83dc86
to
e4239e5
Compare
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 (
|
e4239e5
to
0fd4680
Compare
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. |
303012a
to
88f4d7c
Compare
@moeez96 @miankhalid 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 |
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.
Any specific reason of writing tests for mobile_api/course_info/*
in mobile_api/tests/*
rather than the existing mobile_api/course_info/tests.py
?
@moeez96 I have done this change to fix the Quality check. If tests are placed in the |
@GlugovGrGlib Can we then add the test code to files named 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. |
6fae83d
to
a8b9cbf
Compare
@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. |
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 👍
@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. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
…penedx#34273) * feat: include access serializer into mobile info api view * test: add tests for serializer and view methods * test: move tests to common directory and update test case * fix: cr fixes and use snake case for functions * test: fix additional get call assertion * feat: add required course access messages to mobile endpoint * test: [AXM-229] Improve test coverage * style: [AXM-229] Try to fix linters * fix: remove redundant comment * refactor: change names for the test files --------- Co-authored-by: KyryloKireiev <[email protected]>
…penedx#34273) * feat: include access serializer into mobile info api view * test: add tests for serializer and view methods * test: move tests to common directory and update test case * fix: cr fixes and use snake case for functions * test: fix additional get call assertion * feat: add required course access messages to mobile endpoint * test: [AXM-229] Improve test coverage * style: [AXM-229] Try to fix linters * fix: remove redundant comment * refactor: change names for the test files --------- Co-authored-by: KyryloKireiev <[email protected]>
Description
This PR is a follow-up to FC-0031 project for Mobile API updates #33304, and extends
BlocksInfoInCourseView
view withcourseware_access_details
data for a user in the form: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 #33296Testing 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 "course_access_details" 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.