-
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: [FC-0047] Implement user's enrolments status API (#2530) #34859
Changes from 13 commits
0d0503a
bd8b35d
e474abd
1a7f55b
4b4fa5f
1b369b7
3bd2031
1eec8b8
3cde9d4
bf073fb
da7187e
2077249
bc71be4
4015aab
e868888
9494ee5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,14 @@ | |
""" | ||
|
||
|
||
import datetime | ||
import logging | ||
from functools import cached_property | ||
from typing import Optional | ||
from typing import Dict, List, Optional | ||
|
||
import pytz | ||
from completion.exceptions import UnavailableCompletionData | ||
from completion.models import BlockCompletion | ||
from completion.utilities import get_key_to_last_completed_block | ||
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user | ||
from django.contrib.auth.signals import user_logged_in | ||
|
@@ -530,6 +533,133 @@ def my_user_info(request, api_version): | |
return redirect("user-detail", api_version=api_version, username=request.user.username) | ||
|
||
|
||
@mobile_view(is_user=True) | ||
class UserEnrollmentsStatus(views.APIView): | ||
""" | ||
**Use Case** | ||
|
||
Get information about user's enrolments status. | ||
|
||
Returns active enrolment status if user was enrolled for the course | ||
less than 30 days ago or has progressed in the course in the last 30 days. | ||
Otherwise, the registration is considered inactive. | ||
|
||
USER_ENROLLMENTS_LIMIT - adds users enrollments query limit to | ||
safe API from possible DDOS attacks. | ||
|
||
**Example Request** | ||
|
||
GET /api/mobile/{api_version}/users/<user_name>/enrollments_status/ | ||
|
||
**Response Values** | ||
|
||
If the request for information about the user's enrolments is successful, the | ||
request returns an HTTP 200 "OK" response. | ||
|
||
The HTTP 200 response has the following values. | ||
|
||
* course_id (str): The course id associated with the user's enrollment. | ||
* course_name (str): The course name associated with the user's enrollment. | ||
* recently_active (bool): User's course enrolment status. | ||
|
||
|
||
The HTTP 200 response contains a list of dictionaries that contain info | ||
about each user's enrolment status. | ||
|
||
**Example Response** | ||
|
||
```json | ||
[ | ||
{ | ||
"course_id": "course-v1:a+a+a", | ||
"course_name": "a", | ||
"recently_active": true | ||
}, | ||
{ | ||
"course_id": "course-v1:b+b+b", | ||
"course_name": "b", | ||
"recently_active": true | ||
}, | ||
{ | ||
"course_id": "course-v1:c+c+c", | ||
"course_name": "c", | ||
"recently_active": false | ||
}, | ||
... | ||
] | ||
``` | ||
""" | ||
|
||
USER_ENROLLMENTS_LIMIT = 500 | ||
|
||
def get(self, request, *args, **kwargs) -> Response: | ||
""" | ||
Gets user's enrollments status. | ||
""" | ||
active_status_date = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30) | ||
username = kwargs.get('username') | ||
course_ids_where_user_has_completions = self._get_course_ids_where_user_has_completions( | ||
username, | ||
active_status_date, | ||
) | ||
enrollments_status = self._build_enrollments_status_dict( | ||
username, | ||
active_status_date, | ||
course_ids_where_user_has_completions | ||
) | ||
return Response(enrollments_status) | ||
|
||
def _build_enrollments_status_dict( | ||
self, | ||
username: str, | ||
active_status_date: datetime, | ||
course_ids: List[str], | ||
) -> List[Dict[str, bool]]: | ||
""" | ||
Builds list with dictionaries with user's enrolments statuses. | ||
""" | ||
user = get_object_or_404(User, username=username) | ||
user_enrollments = ( | ||
CourseEnrollment | ||
.enrollments_for_user(user) | ||
.select_related('course') | ||
[:self.USER_ENROLLMENTS_LIMIT] | ||
) | ||
mobile_available = [ | ||
enrollment for enrollment in user_enrollments | ||
if is_mobile_available_for_user(user, enrollment.course_overview) | ||
] | ||
enrollments_status = [] | ||
for user_enrollment in mobile_available: | ||
course_id = str(user_enrollment.course_overview.id) | ||
enrollments_status.append( | ||
{ | ||
'course_id': course_id, | ||
'course_name': user_enrollment.course_overview.display_name, | ||
'recently_active': bool( | ||
course_id in course_ids | ||
or user_enrollment.created > active_status_date | ||
) | ||
} | ||
) | ||
return enrollments_status | ||
|
||
@staticmethod | ||
def _get_course_ids_where_user_has_completions( | ||
username: str, | ||
active_status_date: datetime, | ||
) -> List[str]: | ||
""" | ||
Gets course ids where user has completions. | ||
""" | ||
context_keys = BlockCompletion.objects.filter( | ||
user__username=username, | ||
created__gte=active_status_date | ||
).values_list('context_key', flat=True).distinct() | ||
|
||
return [str(context_key) for context_key in context_keys] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First off, thanks for adding type annotations here 👍🏻 Secondly, notice that you are converting these context keys to strings, and you are also converting the Lastly, and this is just an optional nit, you could return a set of keys rather than a list, since your use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the Set will also be a performance win since the loop on line 633 is using that list to determine if the course is active which is using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kdmccormick @xitij2000 Hello again! I made the changes you wrote about above. Please take a look when you have time. Now we use Course |
||
|
||
|
||
class UserCourseEnrollmentsV4Pagination(DefaultPagination): | ||
""" | ||
Pagination for `UserCourseEnrollments` API v4. | ||
|
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.
@KyryloKireiev If I understand right, the only new information that the UserEnrollmentsStatus API provides is
is_active
, with this logic:Rather than adding a new API endpoint, could this new field be added to the existing
UserCourseEnrollmentsList
API instead?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.
@kdmccormick yes, you are right, the only new information is
is_active
status. But our architects decided we need to create a new API. A mobile team needs an API with the following properties:course name
,course id
andenrollment status
.So, we decided to create a simple interface with only one responsibility.
UserCourseEnrollmentsList
interface has already become too slow, heavy and universal. Also, pagination has already appeared in versions 3 and 4. That is, to obtain the necessary information, we would have to make an additional request to an older version of this API (v0.5, v1 or v2). It would also be necessary to add the “is_active” field to some of the older versions of this API(v0.5, v1 or v2), which would also violate the Open closed Principle.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.
Hi @kdmccormick! Please give us an answer if possible. What do you think about our architectural solution?
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.
@KyryloKireiev The app is calling UserCourseEnrollmentsList anyway in order to render the course cards, right? Why do you need a separate un-paginated endpoint for the status?
Perhaps I am missing the bigger picture. Do you have a doc that outlines all the new and existing API endpoints you need?
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.
Hi @kdmccormick, thank you for your effort to finalize the reviews
So sorry that the context for FC-0047 is not completely clear. I'll try to address all questions and concerns.
As mentioned by Kyrylo above, this PR adds a new endpoint due to performance concerns with using UserEnrollmentsStatus API endpoint, that was developed and used to display User courses on mobile dashboard. Furthermore UserEnrollmentsStatus API was extended with Primary course entry in previous FC-0047 PR to cover product requirements for Mobile Dashboard.
To find more details on this specific functionality which is supported by this new API endpoint, please refer to the video and screenshots in the description for mobile PR openedx/openedx-app-ios#466.
Basically, this endpoint is used only inside the calendar synchronization view, providing the list of active courses that can be selected by a student to synchronize dates for these courses to Apple / Google calendars through native mobile functionality.
For now we only have a document with new PRs for FC-0047 that shows relation of the edx-platform API PR to Mobile APPs PRs in which user facing functionality is added https://docs.google.com/spreadsheets/d/1ImoFKqZZnP3MDnPe_kUmmmuZBgV2teDqsOJVF8y6NjI/edit?gid=0#gid=0
I'll try to outline a document with all the API endpoints that were used or extended as part of FC-0047 today or tomorrow.
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.
@GlugovGrGlib
That would be really helpful, thanks Glib. Can you make that document somewhere that permits review, like an adr, the wiki, or a google doc? I would like to review the API design as a whole, and once we are aligned there, I should be able to approve the individual PRs much more quickly.
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.
@GlugovGrGlib were you able to make a doc outlining the FC-0047 mobile API endpoints?
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.
@KyryloKireiev Thanks for that background.
Pagination/limits
In order to keep large Open edX sites safe from DDOS attacks, all HTTP APIs need to be paginated (with a max page size) or limited (for example, only return the 1000 most relevant results).
Without pagination or limiting, a bad-faith actor (or a sufficiently advanced good-faith user) could take down the site by creating a huge number of enrollments and then repeatedly calling the unbounded API.
user_enrollments API versus enrollments_status
With this PR merged, we would have two separate APIs for listing enrollments: user_enrollments and enrollments_status. I want to understand whether we truly need both APIs.
Once enrollments_status is used by the new apps, will they still use user_enrollments too?