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: [FC-0047] Implement user's enrolments status API (#2530) #34859

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions lms/djangoapps/mobile_api/users/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import ddt
import pytz
from completion.models import BlockCompletion
from completion.test_utils import CompletionWaffleTestMixin, submit_completions_for_testing
from django.conf import settings
from django.db import transaction
Expand All @@ -18,6 +19,7 @@
from django.utils.timezone import now
from milestones.tests.utils import MilestonesTestCaseMixin
from opaque_keys.edx.keys import CourseKey
from rest_framework import status

from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.models import CourseEnrollment
Expand Down Expand Up @@ -768,3 +770,137 @@ def test_discussion_tab_url(self, discussion_tab_enabled):
assert isinstance(discussion_url, str)
else:
assert discussion_url is None


@ddt.ddt
class TestUserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin):
"""
Tests for /api/mobile/{api_version}/users/<user_name>/enrollments_status/
"""

REVERSE_INFO = {'name': 'user-enrollments-status', 'params': ['username', 'api_version']}

def test_no_mobile_available_courses(self) -> None:
self.login()
courses = [CourseFactory.create(org="edx", mobile_available=False) for _ in range(3)]
for course in courses:
self.enroll(course.id)

response = self.api_response(api_version=API_V1)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, [])

def test_no_enrollments(self) -> None:
self.login()
for _ in range(3):
CourseFactory.create(org="edx", mobile_available=True)

response = self.api_response(api_version=API_V1)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, [])

def test_user_have_only_active_enrollments_and_no_completions(self) -> None:
self.login()
courses = [CourseFactory.create(org="edx", mobile_available=True) for _ in range(3)]
for course in courses:
self.enroll(course.id)

response = self.api_response(api_version=API_V1)

expected_response = [
{'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True},
{'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True},
{'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True},
]

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, expected_response)

def test_user_have_active_and_inactive_enrollments_and_no_completions(self) -> None:
self.login()
courses = [CourseFactory.create(org="edx", mobile_available=True) for _ in range(3)]
for course in courses:
self.enroll(course.id)
old_course = CourseFactory.create(org="edx", mobile_available=True)
self.enroll(old_course.id)
old_enrollment = CourseEnrollment.objects.filter(user=self.user, course=old_course.course_id).first()
old_enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=31)
old_enrollment.save()

response = self.api_response(api_version=API_V1)

expected_response = [
{'course_id': str(courses[0].course_id), 'course_name': courses[0].display_name, 'is_active': True},
{'course_id': str(courses[1].course_id), 'course_name': courses[1].display_name, 'is_active': True},
{'course_id': str(courses[2].course_id), 'course_name': courses[2].display_name, 'is_active': True},
{'course_id': str(old_course.course_id), 'course_name': old_course.display_name, 'is_active': False}
]

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, expected_response)

@ddt.data(
(27, True),
(28, True),
(29, True),
(31, False),
(32, False),
)
@ddt.unpack
def test_different_enrollment_dates(self, enrolled_days_ago: int, is_active_status: bool) -> None:
self.login()
course = CourseFactory.create(org="edx", mobile_available=True, run='1001')
self.enroll(course.id)
enrollment = CourseEnrollment.objects.filter(user=self.user, course=course.course_id).first()
enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=enrolled_days_ago)
enrollment.save()

response = self.api_response(api_version=API_V1)

expected_response = [
{'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status}
]

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, expected_response)

@ddt.data(
(27, True),
(28, True),
(29, True),
(31, False),
(32, False),
)
@ddt.unpack
def test_different_completion_dates(self, completed_days_ago: int, is_active_status: bool) -> None:
self.login()
course = CourseFactory.create(org="edx", mobile_available=True, run='1010')
section = BlockFactory.create(
parent=course,
category='chapter',
)
self.enroll(course.id)
enrollment = CourseEnrollment.objects.filter(user=self.user, course=course.course_id).first()
# make enrollment older 30 days ago
enrollment.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=50)
enrollment.save()
completion = BlockCompletion.objects.create(
user=self.user,
context_key=course.context_key,
block_type='course',
block_key=section.location,
completion=0.5,
)
completion.created = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=completed_days_ago)
completion.save()

response = self.api_response(api_version=API_V1)

expected_response = [
{'course_id': str(course.course_id), 'course_name': course.display_name, 'is_active': is_active_status}
]

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertListEqual(response.data, expected_response)
7 changes: 5 additions & 2 deletions lms/djangoapps/mobile_api/users/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from django.conf import settings
from django.urls import re_path

from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail
Copy link
Member

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:

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.

Rather than adding a new API endpoint, could this new field be added to the existing UserCourseEnrollmentsList API instead?

Copy link
Contributor Author

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:

  1. The API should be fast and easy;
  2. The API should return all users enrolments without pagination;
  3. It would be nice if the API did not return anything unnecessary except course name, course id and enrollment 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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@GlugovGrGlib

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.

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.

from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail, UserEnrollmentsStatus

urlpatterns = [
re_path('^' + settings.USERNAME_PATTERN + '$', UserDetail.as_view(), name='user-detail'),
Expand All @@ -17,5 +17,8 @@
),
re_path(f'^{settings.USERNAME_PATTERN}/course_status_info/{settings.COURSE_ID_PATTERN}',
UserCourseStatus.as_view(),
name='user-course-status')
name='user-course-status'),
re_path(f'^{settings.USERNAME_PATTERN}/enrollments_status/',
UserEnrollmentsStatus.as_view(),
name='user-enrollments-status')
]
121 changes: 120 additions & 1 deletion lms/djangoapps/mobile_api/users/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@
"""


import datetime
import logging
from typing import Dict, List

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
from django.db import transaction
from django.shortcuts import redirect
from django.shortcuts import get_object_or_404, redirect
from django.utils import dateparse
from django.utils.decorators import method_decorator
from opaque_keys import InvalidKeyError
Expand Down Expand Up @@ -410,3 +414,118 @@ def my_user_info(request, api_version):
# updating it from the oauth2 related code is too complex
user_logged_in.send(sender=User, user=request.user, request=request)
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.

**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.
* is_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",
"is_active": true
},
{
"course_id": "course-v1:b+b+b",
"course_name": "b",
"is_active": true
},
{
"course_id": "course-v1:c+c+c",
"course_name": "c",
"is_active": false
},
...
]
```
"""
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')
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,
'is_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.
"""
user_completions_last_month = BlockCompletion.objects.filter(
user__username=username,
created__gte=active_status_date
)
return [str(completion.block_key.course_key) for completion in user_completions_last_month]
Loading