-
Notifications
You must be signed in to change notification settings - Fork 14
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-200] Implement user's enrolments status API #2530
feat: [AXM-200] Implement user's enrolments status API #2530
Conversation
* feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter
* fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting
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, check comments, please
@ddt.data( | ||
(27, True), | ||
(28, True), | ||
(29, True), | ||
(31, False), | ||
(32, False), |
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.
[info] I think it is enough to have before border value, after border value and border value itself. Maybe we need to add 30
also. It is called Boundary Testing 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.
But no action needed here
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.
I agree, to have before border value, after border value it would be enough.
However, it is possible that we would get a problem if we used the 30 day limit and ran the tests at approximately 00:00. So I removed this value. But this can also be easily handled using a decorator @freeze_time('2018-01-01')
.
What do you think? Do I need to refactor these tests? It's pretty simple
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.
no, it is alright
def get(self, request, *args, **kwargs) -> Response: | ||
""" | ||
Gets user's enrollments status. | ||
""" | ||
active_status_date = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30) | ||
user_enrollments = CourseEnrollment.objects.filter( | ||
user__username=kwargs.get('username'), | ||
is_active=True, | ||
) | ||
mobile_available = [ | ||
enrollment for enrollment in user_enrollments | ||
if is_mobile_available_for_user(self.request.user, enrollment.course_overview) | ||
] | ||
user_completions_last_month = BlockCompletion.objects.filter( | ||
user__username=kwargs.get('username'), | ||
created__gte=active_status_date | ||
) | ||
course_ids_where_user_has_completions = [ | ||
str(completion.block_key.course_key) for completion in user_completions_last_month | ||
] | ||
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_where_user_has_completions | ||
or user_enrollment.created > active_status_date | ||
) | ||
} | ||
) | ||
|
||
return Response(enrollments_status) |
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.
def get(self, request, *args, **kwargs) -> Response: | |
""" | |
Gets user's enrollments status. | |
""" | |
active_status_date = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30) | |
user_enrollments = CourseEnrollment.objects.filter( | |
user__username=kwargs.get('username'), | |
is_active=True, | |
) | |
mobile_available = [ | |
enrollment for enrollment in user_enrollments | |
if is_mobile_available_for_user(self.request.user, enrollment.course_overview) | |
] | |
user_completions_last_month = BlockCompletion.objects.filter( | |
user__username=kwargs.get('username'), | |
created__gte=active_status_date | |
) | |
course_ids_where_user_has_completions = [ | |
str(completion.block_key.course_key) for completion in user_completions_last_month | |
] | |
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_where_user_has_completions | |
or user_enrollment.created > active_status_date | |
) | |
} | |
) | |
return Response(enrollments_status) | |
from django.db.models import Q | |
@mobile_view(is_user=True) | |
class UserEnrollmentsStatus(views.APIView): | |
# ... docstring with more context ... | |
def get(self, request, *args, **kwargs) -> Response: | |
"""Gets user's enrollments status.""" | |
enrollments = self._get_enrollments_with_progress(kwargs.get('username')) | |
enrollments_status = self._build_enrollments_status(enrollments) | |
return Response(enrollments_status) | |
def _get_enrollments_with_progress(self, username): | |
active_status_date = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30) | |
return CourseEnrollment.objects.filter( | |
user__username=username, | |
is_active=True, | |
blockcompletion__created__gte=active_status_date | |
).select_related('course_overview') | |
def _build_enrollments_status(self, enrollments): | |
enrollments_status = [] | |
for enrollment in enrollments: | |
if is_mobile_available_for_user(self.request.user, enrollment.course_overview): | |
# ... (rest of the logic) ... | |
return enrollments_status |
This is suggestion from Gemini:
- I agree with them on the point to divide this big function into smaller functions.
- In terms of
Q
and combining queries I think it requires check if this will work as expected. So it is up to you if you think this is worth to change. It is more like FYI how it can be optimized.
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.
Yes, I think it's a good idea. I'm going to divide this big function into smaller functions
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
* feat: [AXM-24] Update structure for course enrollments API (#2515) * feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter * feat: [AXM-47] Add course_status field to primary object (#2517) * feat: [AXM-40] add courses progress to enrollment endpoint (#2519) * fix: workaround for staticcollection introduced in e40a01c * feat: [AXM-40] add courses progress to enrollment endpoint * refactor: [AXM-40] add caching to improve performance * refactor: [AXM-40] add progress only for primary course * refactor: [AXM-40] refactor enrollment caching optimization --------- Co-authored-by: Glib Glugovskiy <[email protected]> * feat: [AXM-53] add assertions for primary course (#2522) * feat: [AXM-53] add assertions for primary course * test: [AXM-53] fix tests * style: [AXM-53] change future_assignment default value to None * refactor: [AXM-53] add some optimization for assignments collecting * feat: [AXM-200] Implement user's enrolments status API * style: [AXM-200] Improve code style * refactor: [AXM-200] Divide get method into smaller methods --------- Co-authored-by: NiedielnitsevIvan <[email protected]> Co-authored-by: Glib Glugovskiy <[email protected]>
…t/upstream_PR_active_inactive_courses_API feat: [FC-0047] Implement user's enrolments status API (#2530)
…t/upstream_PR_active_inactive_courses_API feat: [FC-0047] Implement user's enrolments status API (#2530)
…t/upstream_PR_active_inactive_courses_API feat: [FC-0047] Implement user's enrolments status API (#2530)
Description
UserEnrollmentsStatus
- implement user's enrolments status API.UserEnrollmentsStatus
- add test case for the testing API.YouTrack
https://youtrack.raccoongang.com/issue/AXM-200/Active-inactive-status-for-User-enrollments