feat: [FC-0047] Extend mobile API with course progress and primary courses on dashboard view #34848
Conversation
|
Thanks for the pull request, @KyryloKireiev! 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:
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. |
openedx-webhooks
added
the
open-source-contribution
KyryloKireiev
changed the title
KyryloKireiev
force-pushed
the
kireiev/AXM-506/feat/update_UserCourseEnrollmentsList_and_BlocksInfoInCourseView_APIs
branch
from
a959abe
to
104b142
Compare
|
I find the content of the API response to be confusing. I'll spend some more time to try to grok it better, but I have a couple of questions immediately.
Can you explain a bit more about why we need the primary indicator in the first place? I seems to combine to things, recency of registration and recency of activity, into one concept. Given that it is hiding some pretty specific business logic behind the API. I'm not deeply familiar with the new mobile APIs, but this would seem to indicate we have a "backend-for-frontend" here, not really a general purpose API. Is that right? |
|
Hi Edward! This explains some of the things you asked about.
|
|
Hi @KyryloKireiev , I can review this soon. In the meantime, could you help me understand the background of this PR better? I see https://openedx.atlassian.net/wiki/spaces/COMM/pages/3935928321/FC-0047+-+Mobile+Development+Phase+3, but that page is mostly blank. |
|
Hi @kdmccormick thank you!
|
|
Hi @kdmccormick would you be able to take a look this week? |
|
Hey @jawad-khan If you could provide a review from your side as well, it would be really helpful. Thanks! |
|
@GlugovGrGlib thanks for the context. Yes, I should be able to review this week. |
| @staticmethod | ||
| def _get_last_visited_block_path_and_unit_name( | ||
| block_id: str | ||
| ) -> Union[Tuple[None, None], Tuple[List['XBlock'], str]]: # noqa: F821 |
There was a problem hiding this comment.
Rather than noqa codes could you use the human-readable pylint: ignore=... ?
There was a problem hiding this comment.
Hi, sure we will be able to update the format of linter directives, but what do you think about switching to other linters, for example ruff in the future?
Maybe it's worth discussing smooth transition from pylint only supported comments to flake8 like # noqa, that are supported by most python linters nowadays, so it might ease the migration?
There was a problem hiding this comment.
That's an interesting point, and I think it could be worth discussing separately. Currently, though, we almost exclusively use pylint directives instead of noqa directives, so I'd like to stay consistent rather than starting a new pattern in this PR.
If we want to switch from pylint directives to noqa in the future, we could use a script to do it in bulk.
There was a problem hiding this comment.
Yes, I see, I'm going to change # noqa to pylint: ignore=...
| """ | ||
| return self.calculate_progress(model) | ||
|
|
||
| def get_course_assignments(self, model: CourseEnrollment) -> Dict[str, Optional[List[Dict[str, str]]]]: |
There was a problem hiding this comment.
I would like this logic to exist in a central platform Python API instead of the Mobile API. If the Learning MFE or Learner Dash MFE ever needs this same data, they should be able to call this function instead of reimplementing it. Could you move it? Maybe lms/djangoapps/courseware/courses.py is the right place?
In general, when implementing Mobile API features, try to keep the student/grading/progress/access logic in core applications, and then just use the mobile_api as a thin wrapper for presenting that information to the mobile apps. That way, both the Web and Mobile versions of the platform can have consistent behavior and we will have less business logic to test.
There was a problem hiding this comment.
@kdmccormick I agree, it sounds logical. I moved get_course_assignments here: lms/djangoapps/courseware/courses.py
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def calculate_progress( |
There was a problem hiding this comment.
Same as my comment on get_course_assignments, this is core business logic that needs to be implemented in core APIs for consistency with the Web views.
There was a problem hiding this comment.
And this calculate_progress function I also move to lms/djangoapps/courseware/courses.py. Everything works correctly and I'm going to push these changes
There was a problem hiding this comment.
Sounds good. Let me know when you're ready for another review pass.
There was a problem hiding this comment.
@kdmccormick the comments were addressed, It's ready for the second round now
There was a problem hiding this comment.
@KyryloKireiev I see that you moved one definition of calculate_progress to lms/djangoapps/courseware/courses.py, thank you. However, there is this second definition of calculate_progress below, still in the mobile API.
Firstly, I think this function should be called something else, as it returns subsection grades, not progress.
Secondly, and more importantly, I am concerned whether this implementation matches the other ways in which we show a user's subsection grades in edx-platform. For example, I assume that the gradebook and the progress page on the Learning MFE both show subsection grades. Does those implementations perform the same steps that we do here (load the block structure, cache it, calculate grades, and then re-calculate from visible-only) ? If so, why not use a shared API function? If not, why?
cc @jawad-khan
There was a problem hiding this comment.
- @kdmccormick I renamed and moved all
calculate_progressmethods tolms/djangoapps/courseware/courses.py; - @GlugovGrGlib will be able to answer the second question better. This decision was made taking into account the further development of the mobile API.
There was a problem hiding this comment.
@kdmccormick that's true this approach for the assignments completions and progress calculation is not similar to the one on progress page in LMS. This was a product decision made in collaboration with @marcotuts
Please refer to FIGMA designs and to the product requirements write up for more info.
We discussed that now data about assignments on progress page, and course home in web differs from mobile course home but in the future iterations the same feture may be applied to the web course home as well.
KyryloKireiev
force-pushed
the
kireiev/AXM-506/feat/update_UserCourseEnrollmentsList_and_BlocksInfoInCourseView_APIs
branch
2 times, most recently
from
bb91f42
to
cdd2d7e
Compare
|
@KyryloKireiev @GlugovGrGlib I have started reviewing this PR. |
|
Hey @jawad-khan, thank you, so much! |
KyryloKireiev
force-pushed
the
kireiev/AXM-506/feat/update_UserCourseEnrollmentsList_and_BlocksInfoInCourseView_APIs
branch
2 times, most recently
from
fa9f50f
to
bbfb34d
Compare
| @@ -81,7 +81,7 @@ def get_user_course_expiration_date(user, course): | |||
| if access_duration is None: | |||
| return None | |||
|
|
|||
| enrollment = CourseEnrollment.get_enrollment(user, course.id) | |||
| enrollment = CourseEnrollment.get_enrollment(user, course.id) if not enrollment else enrollment | |||
There was a problem hiding this comment.
Another approach(optional).
enrollment = enrollment or CourseEnrollment.get_enrollment(user, course.id)
| @@ -97,7 +107,7 @@ def get_audit_access_expires(self, model): | |||
| """ | |||
| Returns expiration date for a course audit expiration, if any or null | |||
| """ | |||
| return get_user_course_expiration_date(model.user, model.course) | |||
| return get_user_course_expiration_date(model.user, model.course, model) | |||
There was a problem hiding this comment.
why there is a need to send another param when function can work with two params?
There was a problem hiding this comment.
I think we can pass CourseEnrollment object into get_user_course_expiration_date function and avoid the additional query to the database here:
enrollment = enrollment or CourseEnrollment.get_enrollment(user, course.id)
| @@ -124,6 +134,17 @@ def get_course_modes(self, obj): | |||
| for mode in course_modes | |||
| ] | |||
|
|
|||
| def to_representation(self, instance: CourseEnrollment) -> 'OrderedDict': # lint-amnesty, pylint: disable=unused-variable, line-too-long | |||
There was a problem hiding this comment.
why don't we add course_progress as serializer field?
There was a problem hiding this comment.
We didn't want to have a course_progress field for each user's enrollment by default. So, we have made a decision to add course_progress field into each user's enrollment only when course_progress is passed into requested_fields query param.
So if we were to add this field course_progress as serializer field - we would have this field in every version (v0.5, v1, v2, v3, v4) of this API. We would receive something like this: "course_progress": null. We didn't want to change the response for older versions of the API, so we added this field as an additional one to to_representation method.
For primary user enrollment we added course_progress as serializer field. Because this is new functionality and we don't change old functionality.
| @@ -396,6 +517,8 @@ def paginator(self): | |||
|
|
|||
| if self._paginator is None and api_version == API_V3: | |||
| self._paginator = DefaultPagination() | |||
| if self._paginator is None and api_version == API_V4: | |||
There was a problem hiding this comment.
Are we expecting a default paginator for v4?
If no then why api_version == API_V isn't enough here?
There was a problem hiding this comment.
We need to use something like this here:
if self._paginator is None and api_version == API_V4:
Because the method paginator is called 4 times when we requesting the API. And only the first time we don't have pagination object. Then we have pagination object:
<lms.djangoapps.mobile_api.users.views.UserCourseEnrollmentsV4Pagination object at 0x7f57017f5950>
And we can't change pagination object, we need only return pagination object for the next method calls.
| return list(mobile_available) | ||
| return mobile_available | ||
|
|
||
| def get_mobile_available_enrollments(self) -> List[Optional[CourseEnrollment]]: |
There was a problem hiding this comment.
This method also filters same organization check, can we add this in method name?
maybe:
get_same_org_mobile_available_enrollments
There was a problem hiding this comment.
The return type should be simply list[CourseEnrollment] -- the list items will never be None.
There was a problem hiding this comment.
@kdmccormick yes, you are right. It should be list[CourseEnrollment] return type. I lost this comment. I'll add this fix to the next push.
|
|
||
| mobile_available_course_ids = [enrollment.course_id for enrollment in mobile_available] | ||
|
|
||
| latest_enrollment = self.queryset.filter( |
There was a problem hiding this comment.
Haven't we already filtered is_activer and username in queryset method?
will it work the same way if we don't apply username and is_active check here?
There was a problem hiding this comment.
Yes, you are right. It was extra filtration. I removed these filtrations.
| course__id__in=mobile_available_course_ids, | ||
| ).order_by('-created').first() | ||
|
|
||
| if not latest_enrollment: |
There was a problem hiding this comment.
latest_enrollment code looks duplicated, we are doing same thing in queryset?
can we avoid this?
There was a problem hiding this comment.
furthermore, could we centralize the latest_enrollment calculation in a Python API function outside of the mobile_api? For example, common/djangoapps/student/api.py:get_last_active_enrollment
There was a problem hiding this comment.
But we don't filter user's enrollments by mobile_available in queryset . Should we do this filtration is queryset method?
There was a problem hiding this comment.
We need to redesign our queryset and get_queryset approach. once its finalized we can apply mobile_available filter in get_queryset and use it everywhere else.
| @@ -341,47 +362,147 @@ def get_serializer_class(self): | |||
| return CourseEnrollmentSerializerv05 | |||
| return CourseEnrollmentSerializer | |||
|
|
|||
| def get_queryset(self): | |||
| @cached_property | |||
| def queryset(self): | |||
There was a problem hiding this comment.
You should be very careful when using a class level attribute.
Unless we have a strong reason of doing this I'll advise that we move instance or request level logic to get_queryset.
There was a problem hiding this comment.
Yes, you are right. For the first time we try to put all logic into get_queryset , but we received more then 400 SQL queries for the one request (!). So, we need to use property method for received queryset. Caching helps maximize performance, so we need to @cached_property decorator to make this API faster. Now we have about 120 SQL queries for the one API request. This is the maximum number of SQL queries when requesting additional fields and a large number of the user's enrollments.
There was a problem hiding this comment.
This stackflow answer the best example of when to use queryset and get_queryset.
In our case we should move all request level logic to get_queryset and use caching there. queryset is already cached since it is evaluated only once.
| 'enrollments': response.data | ||
| } | ||
| if api_version == API_V4 and status not in EnrollmentStatuses.values(): | ||
| if status in EnrollmentStatuses.values(): |
There was a problem hiding this comment.
Doesn't this condition have a conflict with above condition?
We are here because we already know that status is not in EnrollmentStatuses.values()
There was a problem hiding this comment.
Yes, this is actually not a necessary condition. I removed it.
Here the logic may be a little unclear in isolation from mobile applications. If the status parameter is passed to the query params, then the primary enrollment object field should not exist. Only enrollments filtered by status should be returns by this API.
| 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]]]: |
There was a problem hiding this comment.
Seems like most of this logic is already written in this view.
Can we reuse this logic without duplicating it?
There was a problem hiding this comment.
Yes, I see. This is exactly what we did initially. We tried to use methods from this view. We used functions from this method _last_visited_block_path. We also moved the repetitive logic into a separate function. But our QAs found out that the method _get_course_info returns the wrong path to the last visited block. So, we decided to create new method that returns correct path to the last visited block and don't use logic from _last_visited_block_path method.
Maybe some problems are in this function:
def get_current_child(xblock, min_depth=None, requested_child=None):
"""
Get the xblock.position's display item of an xblock that has a position and
children. If xblock has no position or is out of bounds, return the first
child with children of min_depth.
We may not understand how this function is supposed to work. But the path to last visited block that is built using this function is not correct
There was a problem hiding this comment.
@KyryloKireiev Does get_current_child return the wrong block, or is the right block with the wrong path?
There was a problem hiding this comment.
@kdmccormick get_current_child function is used in UserCourseStatus._last_visited_block_path method. This method _last_visited_block_path returns incorrect path to the last visited block. Maybe get_current_child function works correctly. But I think it is not correct to use it to find path to the last visited block.
|
I have added my first review, will deploy these changes and test it locally once you give feedback on this. |
There was a problem hiding this comment.
I agree with the requests @jawad-khan has raised and I've added a couple comments to his threads. Otherwise, my only other request is that under all PRs' "Testing Instructions" you describe which flags/data you used to manually smoke-test test the changes.
|
Regarding TNL/Aurora review-- if someone from that teams wants to jump in and review as well they are welcome to, but between you and I @jawad-khan, I think our approvals are enough to merge this. |
@kdmccormick I added detail instruction how make manual API testing to the PR's description. |
* feat: [AXM-24] Update structure for course enrollments API * style: [AXM-24] Improve code style * fix: [AXM-24] Fix student's latest enrollment filter
| """ | ||
| return self.filter(is_active=True) | ||
|
|
||
| def without_certificates(self, user_username): |
| from lms.djangoapps.certificates.models import GeneratedCertificate # pylint: disable=import-outside-toplevel | ||
| course_ids_with_certificates = GeneratedCertificate.objects.filter( | ||
| user__username=user_username | ||
| ).values_list('course_id', flat=True) |
There was a problem hiding this comment.
These two lines are duplicated, can we write a method for user_course_ids_with_certificates?
| @@ -341,47 +362,147 @@ def get_serializer_class(self): | |||
| return CourseEnrollmentSerializerv05 | |||
| return CourseEnrollmentSerializer | |||
|
|
|||
| def get_queryset(self): | |||
| @cached_property | |||
| def queryset(self): | |||
There was a problem hiding this comment.
This stackflow answer the best example of when to use queryset and get_queryset.
In our case we should move all request level logic to get_queryset and use caching there. queryset is already cached since it is evaluated only once.
| course__id__in=mobile_available_course_ids, | ||
| ).order_by('-created').first() | ||
|
|
||
| if not latest_enrollment: |
There was a problem hiding this comment.
We need to redesign our queryset and get_queryset approach. once its finalized we can apply mobile_available filter in get_queryset and use it everywhere else.
| not_duration_limited = ( | ||
| enrollment for enrollment in mobile_available | ||
| if check_course_expired(self.request.user, enrollment.course) == ACCESS_GRANTED | ||
| ) | ||
|
|
||
| if api_version == API_V4 and status not in EnrollmentStatuses.values(): |
There was a problem hiding this comment.
Sorry, I added comment on the wrong line.
I was just suggesting why not we send mobile_available to get_primary_enrollment_by_latest_enrollment_or_progress in the following line.
|
@KyryloKireiev Once we agree on queryset implementation apparoach I'll test PR locally and will approve it. |
Hi @jawad-khan, I pushed new The main problem of the queryset implementation is @jawad-khan, if my approach seems wrong to you, can you explain in more detail how it should be? |
This implementation can be another of not using class level attribute(which is recommended). I'll test it locally. Meanwhile can you please fix failing test cases? |
|
@jawad-khan pipelines is passed successfully, ready to local review |
|
@kdmccormick I have approved these changes and its clear from my end, can you please finalize your review because we need theses changes asap. |
|
Hi @kdmccormick, would you be able to do another round of review? |
|
I am out on holiday and unable to review until 16 Jul, but I do trust @jawad-khan 's review so don't hesistate to merge if this is ready to go. |
|
@kdmccormick Ok, thanks for the update, have a nice holiday! @jawad-khan Let's merge it as soon as you're ready? |
|
@kdmccormick can you please resolve your concerns if they are addressed? |
|
I have checked over the |
|
was trying to see if removing Kyle's review would enable the merge button 👆 |
miankhalid
dismissed
kdmccormick’s stale review
Dimissing based on this comment from Kyle: I am out on holiday and unable to review until 16 Jul, but I do trust @jawad-khan 's review so don't hesistate to merge if this is ready to go.
|
@KyryloKireiev 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
…urses on dashboard view (openedx#34848) * feat: [AXM-24] Update structure for course enrollments API (openedx#2515) --------- Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> * feat: [AXM-53] add assertions for primary course (openedx#2522) --------- Co-authored-by: monteri <36768631+monteri@users.noreply.github.com> * feat: [AXM-297] Add progress to assignments in BlocksInfoInCourseView API (openedx#2546) --------- Co-authored-by: NiedielnitsevIvan <81557788+NiedielnitsevIvan@users.noreply.github.com> Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> Co-authored-by: monteri <36768631+monteri@users.noreply.github.com> Conflicts: lms/djangoapps/courseware/courses.py
…urses on dashboard view (openedx#34848) * feat: [AXM-24] Update structure for course enrollments API (openedx#2515) --------- Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> * feat: [AXM-53] add assertions for primary course (openedx#2522) --------- Co-authored-by: monteri <36768631+monteri@users.noreply.github.com> * feat: [AXM-297] Add progress to assignments in BlocksInfoInCourseView API (openedx#2546) --------- Co-authored-by: NiedielnitsevIvan <81557788+NiedielnitsevIvan@users.noreply.github.com> Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> Co-authored-by: monteri <36768631+monteri@users.noreply.github.com>
…urses on dashboard view (openedx#34848) * feat: [AXM-24] Update structure for course enrollments API (openedx#2515) --------- Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> * feat: [AXM-53] add assertions for primary course (openedx#2522) --------- Co-authored-by: monteri <36768631+monteri@users.noreply.github.com> * feat: [AXM-297] Add progress to assignments in BlocksInfoInCourseView API (openedx#2546) --------- Co-authored-by: NiedielnitsevIvan <81557788+NiedielnitsevIvan@users.noreply.github.com> Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com> Co-authored-by: monteri <36768631+monteri@users.noreply.github.com> Conflicts: lms/djangoapps/courseware/courses.py lms/djangoapps/mobile_api/users/tests.py
|
@GlugovGrGlib Can you help me understand how does this progress bar track progress? Are there any specific xblock completion it is tracking? |
Description
Two APIs was updated:
UserCourseEnrollmentsListandBlocksInfoInCourseView:UserCourseEnrollmentsList- In v4 we added to the response primary object. Primary object contains the latest user's enrollment or course where user has the latest progress. Primary object has been cut from user's enrolments array and inserted into separated section with keyprimary.Also we add to Primary enrollment object additional fields:
* course_progress: Contains information about how many assignments are in the course
and how many assignments the student has completed.
* total_assignments_count: Total course's assignments count.
* assignments_completed: Assignments witch the student has completed.
We added
?requested_fields=course_progressquery param to GET request:GET /api/mobile/v4/users/kyrylo/course_enrollments/?requested_fields=course_progressWith using this query param we can getcourse_progressfield for the each student enrollment.Also, for the 4th version of this API, we changed the pagination - now the API returns 5 elements per page. This is necessary to improve performance.
Response example:
BlocksInfoInCourseView- we added to response 2 new fields:assignment_progress:course_progress:All new functionality was covered with unit tests.
Supporting information
https://openedx.atlassian.net/wiki/spaces/COMM/pages/3935928321/FC-0047+-+Mobile+Development+Phase+3
Testing instructions
pytest lms/djangoapps/mobile_api/tests- this command runs all tests related to mobile_api aplication. This command should be running in the LMS container.Also, this API can be tested manual with Postman or another API client:
JwtAuthentication, BearerAuthentication, SessionAuthenticationcan be used;{{LMS}}/api/mobile/<API_version>/users/<username>/course_enrollments/;?requested_fields=course_progress- returnscourse_progressfor each user's enrollment.?status=in_progress- returns user's enrollments filtered bystatus. It can by next statuses:?status=in_progressquery param is used - the response contains only filtered user's enrollments by status. And doesn't containprimaryenrollment object.Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Related mobile PRs: