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

feat(mobile_api): Add course access object to mobile course info API #34273

Conversation

GlugovGrGlib
Copy link
Member

@GlugovGrGlib GlugovGrGlib commented Feb 21, 2024

Description

This PR is a follow-up to FC-0031 project for Mobile API updates #33304, and extends BlocksInfoInCourseView view with courseware_access_details data for a user in the form:

"course_access_details": {
		"has_unmet_prerequisites": <bool>,
		"is_too_early": <bool>,
		"is_staff": <bool>,
		"audit_access_expires": null,
		"courseware_access": {
			"has_access": <bool>,
			"error_code": <string> or null,
			"developer_message":  <string> or null,
			"user_message":  <string> or null,
			"additional_context_user_message":  <string> or null,
			"user_fragment": null
		}
	},
	
"enrollment_details": {
		"created": <date>,
		"mode": <string>,
		"is_active": <bool>
	},

"course_updates": <str>,
"course_handouts": <str>,
"discussion_url": <str> or null,
	
"course_sharing_utm_parameters": {
		"facebook": "utm_medium=social&utm_campaign=social-sharing-db&utm_source=facebook",
		"twitter": "utm_medium=social&utm_campaign=social-sharing-db&utm_source=twitter"
	},
"course_modes": [
		{
			"slug": <str>,
			"sku": null,
			"android_sku": null,
			"ios_sku": null,
			"min_price": <int>
		}
	]

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 #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 "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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 21, 2024
@openedx-webhooks
Copy link

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.

@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch from 043f344 to 684161d Compare February 22, 2024 00:37
@moeez96 moeez96 self-requested a review February 22, 2024 08:17
@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch from 0572981 to f83dc86 Compare February 26, 2024 21:52
@GlugovGrGlib GlugovGrGlib requested a review from moeez96 February 26, 2024 22:13
Copy link

@miankhalid miankhalid left a 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

  1. https://github.com/openedx/edx-app-ios/blob/a07db33dd0e2ef0d2c30582c79b2e7273c7e1aac/Source/OEXCoursewareAccess.h
  2. https://github.com/openedx/edx-app-ios/blob/a07db33dd0e2ef0d2c30582c79b2e7273c7e1aac/Source/OEXCoursewareAccess.m

Android

  1. https://github.com/openedx/edx-app-android/blob/459dee23fc14f76abb4d1e05d31e54032aa6d894/OpenEdXMobile/src/main/java/org/edx/mobile/model/api/CoursewareAccess.java
  2. https://github.com/openedx/edx-app-android/blob/459dee23fc14f76abb4d1e05d31e54032aa6d894/OpenEdXMobile/src/main/java/org/edx/mobile/model/api/AccessError.java

@miankhalid
Copy link

Also, since this change:

  1. might require pulling data from different models or queries that need to be done on the db
  2. and increase the JSON response size

so, is there a plan to do performance & load testing before and after this change?

@GlugovGrGlib
Copy link
Member Author

@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.

@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch from f83dc86 to e4239e5 Compare February 28, 2024 21:26
@miankhalid
Copy link

Great @GlugovGrGlib, thanks for making these changes!

Do let us know when the PR is ready for another pass.

@miankhalid
Copy link

@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/):

  1. Elements in red boxes are absolutely needed currently for app's working and especially analytics.
  2. Elements in green boxes would be needed in future for in-app payments in the apps.

cc @omerhabib26 @saeedbashir @moiz994 @touchapp

Screenshot 2024-03-08 at 12-43-09 Online JSON Viewer and Formatter

@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch from e4239e5 to 0fd4680 Compare March 17, 2024 22:07
@moeez96
Copy link
Contributor

moeez96 commented Mar 18, 2024

Hi @GlugovGrGlib Is this ready for another review?

@GlugovGrGlib
Copy link
Member Author

GlugovGrGlib commented Mar 18, 2024

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.

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 20, 2024
@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch 2 times, most recently from 303012a to 88f4d7c Compare April 12, 2024 15:18
@GlugovGrGlib
Copy link
Member Author

@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
Response

The course access messages can be accessed using $.course_access_details.courseware_access.has_access

Copy link
Contributor

@moeez96 moeez96 left a 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?

@GlugovGrGlib
Copy link
Member Author

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 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.

@moeez96
Copy link
Contributor

moeez96 commented Apr 23, 2024

@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!

@GlugovGrGlib GlugovGrGlib force-pushed the glugovgrglib/add_course_access_to_mobile_info_api branch from 6fae83d to a8b9cbf Compare April 23, 2024 16:29
@GlugovGrGlib
Copy link
Member Author

GlugovGrGlib commented Apr 23, 2024

@moeez96 Thank you for the suggestion, I have renamed test files

@GlugovGrGlib
Copy link
Member Author

@moeez96 @miankhalid If everything looks good from your side, please merge this PR whenever you're ready.

@GlugovGrGlib GlugovGrGlib removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 24, 2024
Copy link

@miankhalid miankhalid left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@moeez96 moeez96 merged commit c37e976 into openedx:master Apr 25, 2024
66 checks passed
@openedx-webhooks
Copy link

@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.

@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.

OmarIthawi pushed a commit to nelc/edx-platform that referenced this pull request May 31, 2024
…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]>
OmarIthawi pushed a commit to nelc/edx-platform that referenced this pull request Jul 24, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants