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: [AXM-53] add assertions for primary course #2522

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
45 changes: 39 additions & 6 deletions lms/djangoapps/mobile_api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Serializer for user API
"""

from datetime import datetime
from typing import Dict, List, Optional, Tuple

from django.core.cache import cache
Expand All @@ -16,8 +17,10 @@
from lms.djangoapps.certificates.api import certificate_downloadable_status
from lms.djangoapps.courseware.access import has_access
from lms.djangoapps.courseware.block_render import get_block_for_descriptor
from lms.djangoapps.courseware.courses import get_current_child
from lms.djangoapps.courseware.context_processor import get_user_timezone_or_last_seen_timezone_or_utc
from lms.djangoapps.courseware.courses import get_course_assignment_date_blocks, get_current_child
from lms.djangoapps.courseware.model_data import FieldDataCache
from lms.djangoapps.course_home_api.dates.serializers import DateSummarySerializer
from lms.djangoapps.grades.api import CourseGradeFactory
from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager
from openedx.features.course_duration_limits.access import get_user_course_expiration_date
Expand Down Expand Up @@ -160,9 +163,14 @@ class CourseEnrollmentSerializerModifiedForPrimary(CourseEnrollmentSerializer):

course_status = serializers.SerializerMethodField()
progress = serializers.SerializerMethodField()
course_assignments = serializers.SerializerMethodField()

BLOCK_STRUCTURE_CACHE_TIMEOUT = 60 * 60 # 1 hour

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.course = modulestore().get_course(self.instance.course.id)

def get_course_status(self, model: CourseEnrollment) -> Optional[Dict[str, List[str]]]:
"""
Gets course status for the given user's enrollments.
Expand All @@ -186,8 +194,8 @@ def get_course_status(self, model: CourseEnrollment) -> Optional[Dict[str, List[
'last_visited_unit_display_name': unit_name,
}

@staticmethod
def _get_last_visited_block_path_and_unit_name(
self,
request: 'Request', # noqa: F821
model: CourseEnrollment,
) -> Tuple[List[Optional['XBlock']], Optional[str]]: # noqa: F821
Expand All @@ -197,12 +205,10 @@ def _get_last_visited_block_path_and_unit_name(
If there is no such visit, the first item deep enough down the course
tree is used.
"""
course = modulestore().get_course(model.course.id)
field_data_cache = FieldDataCache.cache_for_block_descendents(
course.id, model.user, course, depth=3)
field_data_cache = FieldDataCache.cache_for_block_descendents(self.course.id, model.user, self.course, depth=3)

course_block = get_block_for_descriptor(
model.user, request, course, field_data_cache, course.id, course=course
model.user, request, self.course, field_data_cache, self.course.id, course=self.course
)

unit_name = ''
Expand Down Expand Up @@ -243,6 +249,32 @@ def get_progress(self, model: CourseEnrollment) -> Dict[str, int]:
'num_points_possible': sum(map(lambda x: x.graded_total.possible if x.graded else 0, subsection_grades)),
}

def get_course_assignments(self, model: CourseEnrollment) -> Optional[Dict[str, List[Dict[str, str]]]]:
"""
Returns the future assignment data and past assignments data for the user in the course.
"""
assignments = get_course_assignment_date_blocks(
self.course,
model.user,
self.context.get('request'),
include_past_dates=True
)
next_assignment = None
past_assignment = []
Copy link

Choose a reason for hiding this comment

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

Suggested change
past_assignment = []
past_assignments = [assignment for assignment in sorted_assignments if assignment.date < datetime.now(timezone)]

If we iterate over the same list twice the complexity will be O(2N) that is by O-notation roughly the same as a single iteration.

Copy link
Author

Choose a reason for hiding this comment

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

What benefits will this give us, since this way we will iterate by sorted_assignments 2 times? In addition, we will also worsen the readability, as for me.


Copy link

@monteri monteri Mar 29, 2024

Choose a reason for hiding this comment

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

Suggested change
sorted_assignments = sorted(assignments, key=lambda x: x.date)

create this variable and move it above the past assignments

Copy link
Author

@NiedielnitsevIvan NiedielnitsevIvan Apr 2, 2024

Choose a reason for hiding this comment

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

Do we need this micro-optimization here if there is still only one cycle?

timezone = get_user_timezone_or_last_seen_timezone_or_utc(model.user)
for assignment in sorted(assignments, key=lambda x: x.date):
if assignment.date < datetime.now(timezone):
past_assignment.append(assignment)
else:
next_assignment = DateSummarySerializer(assignment).data
break

return {
'future_assignment': next_assignment,
'past_assignments': DateSummarySerializer(past_assignment, many=True).data,
}

class Meta:
model = CourseEnrollment
fields = (
Expand All @@ -255,6 +287,7 @@ class Meta:
'course_modes',
'course_status',
'progress',
'course_assignments',
)
lookup_field = 'username'

Expand Down
25 changes: 18 additions & 7 deletions lms/djangoapps/mobile_api/users/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ def test_student_dont_have_enrollments(self):
'configs': {
'iap_configs': {}
},
'user_timezone': 'UTC',
'enrollments': {
'next': None,
'previous': None,
Expand All @@ -434,7 +435,8 @@ def test_student_dont_have_enrollments(self):
self.assertDictEqual(expected_result, response.data)
self.assertNotIn('primary', response.data)

def test_student_have_one_enrollment(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_student_have_one_enrollment(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand All @@ -458,7 +460,8 @@ def test_student_have_one_enrollment(self):
self.assertIn('primary', response.data)
self.assertEqual(str(course.id), response.data['primary']['course']['id'])

def test_student_have_two_enrollments(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_student_have_two_enrollments(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand All @@ -477,7 +480,8 @@ def test_student_have_two_enrollments(self):
self.assertIn('primary', response.data)
self.assertEqual(response.data['primary']['course']['id'], str(course_second.id))

def test_student_have_more_then_ten_enrollments(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_student_have_more_then_ten_enrollments(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand All @@ -497,7 +501,8 @@ def test_student_have_more_then_ten_enrollments(self):
self.assertIn('primary', response.data)
self.assertEqual(response.data['primary']['course']['id'], str(latest_enrolment.id))

def test_student_have_progress_in_old_course_and_enroll_newest_course(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_student_have_progress_in_old_course_and_enroll_newest_course(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand Down Expand Up @@ -559,6 +564,7 @@ def test_student_enrolled_only_not_mobile_available_courses(self):
"configs": {
"iap_configs": {}
},
"user_timezone": "UTC",
"enrollments": {
"next": None,
"previous": None,
Expand All @@ -576,7 +582,8 @@ def test_student_enrolled_only_not_mobile_available_courses(self):
self.assertDictEqual(expected_result, response.data)
self.assertNotIn('primary', response.data)

def test_do_progress_in_not_mobile_available_course(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_do_progress_in_not_mobile_available_course(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand Down Expand Up @@ -613,7 +620,8 @@ def test_do_progress_in_not_mobile_available_course(self):
self.assertIn('primary', response.data)
self.assertEqual(response.data['primary']['course']['id'], str(new_course.id))

def test_pagination_for_user_enrollments_api_v4(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_pagination_for_user_enrollments_api_v4(self, cache_mock: MagicMock):
"""
Tests `UserCourseEnrollmentsV4Pagination`, api_version == v4.
"""
Expand All @@ -632,7 +640,8 @@ def test_pagination_for_user_enrollments_api_v4(self):
self.assertIn('previous', response.data['enrollments'])
self.assertIn('primary', response.data)

def test_course_status_in_primary_obj_when_student_doesnt_have_progress(self):
@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
def test_course_status_in_primary_obj_when_student_doesnt_have_progress(self, cache_mock: MagicMock):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
"""
Expand All @@ -645,10 +654,12 @@ def test_course_status_in_primary_obj_when_student_doesnt_have_progress(self):
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.data['primary']['course_status'], None)

@patch('lms.djangoapps.mobile_api.users.serializers.cache.set', return_value=None)
@patch('lms.djangoapps.mobile_api.users.serializers.get_key_to_last_completed_block')
def test_course_status_in_primary_obj_when_student_have_progress(
self,
get_last_completed_block_mock: MagicMock,
cache_mock: MagicMock
):
"""
Testing modified `UserCourseEnrollmentsList` view with api_version == v4.
Expand Down
12 changes: 10 additions & 2 deletions lms/djangoapps/mobile_api/users/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
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 All @@ -28,6 +28,7 @@
from common.djangoapps.student.models import CourseEnrollment, User # lint-amnesty, pylint: disable=reimported
from lms.djangoapps.courseware.access import is_mobile_available_for_user
from lms.djangoapps.courseware.access_utils import ACCESS_GRANTED
from lms.djangoapps.courseware.context_processor import get_user_timezone_or_last_seen_timezone_or_utc
from lms.djangoapps.courseware.courses import get_current_child
from lms.djangoapps.courseware.model_data import FieldDataCache
from lms.djangoapps.courseware.block_render import get_block_for_descriptor
Expand Down Expand Up @@ -358,7 +359,7 @@ def queryset(self):
return CourseEnrollment.objects.all().select_related('course', 'user').filter(
user__username=self.kwargs['username'],
is_active=True
).order_by('created').reverse()
).order_by('-created')

def get_queryset(self):
api_version = self.kwargs.get('api_version')
Expand Down Expand Up @@ -404,6 +405,7 @@ def list(self, request, *args, **kwargs):
if api_version in (API_V2, API_V3, API_V4):
enrollment_data = {
'configs': MobileConfig.get_structured_configs(),
'user_timezone': str(get_user_timezone_or_last_seen_timezone_or_utc(self.get_user())),
'enrollments': response.data
}
if api_version == API_V4:
Expand All @@ -419,6 +421,12 @@ def list(self, request, *args, **kwargs):

return response

def get_user(self) -> User:
"""
Get user object by username.
"""
return get_object_or_404(User, username=self.kwargs['username'])

def get_primary_enrollment_by_latest_enrollment_or_progress(self) -> Optional[CourseEnrollment]:
"""
Gets primary enrollment obj by latest enrollment or latest progress on the course.
Expand Down
Loading