-
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
base: master
Are you sure you want to change the base?
feat: [FC-0047] Implement user's enrolments status API (#2530) #34859
Conversation
Thanks for the pull request, @KyryloKireiev! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Hi @jawad-khan! Following up on this :) |
""" | ||
Builds list with dictionaries with user's enrolments statuses. | ||
""" | ||
user_enrollments = CourseEnrollment.objects.filter( |
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.
Instead of filtering objects directly we can use class method CourseEnrollment.enrollments_for_user
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.
Ok, used this class method enrollments_for_user
here
|
||
|
||
@ddt.ddt | ||
class UserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin): |
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.
Can we rename it to TestUserEnrollmentsStatus?
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, of course, there was some kind of failure with the name for the test case. Changed to TestUserEnrollmentsStatus
@@ -410,3 +414,128 @@ 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) | |||
|
|||
|
|||
class UserCourseEnrollmentsV4Pagination(DefaultPagination): |
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.
Are we using it anywhere?
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.
This probably got here from another commit. Removed
5080c82
to
ee862b3
Compare
* 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 <glib.glugovskiy@raccoongang.com> * 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 <81557788+NiedielnitsevIvan@users.noreply.github.com> Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com>
a66288f
to
1a7f55b
Compare
Hi @jawad-khan! I fixed code review issues. It's ready for the second round now |
Hi @kdmccormick, could you please review this PR as well? |
Hi @kdmccormick! You said you would be available after July 16th) Could you please review this PR as well? |
Hi @jawad-khan! Apparently @kdmccormick a little busy right now. However, at the previous PR he wrote that he trusts you completely) Maybe you have the opportunity to merge this PR without waiting?) |
@@ -6,7 +6,7 @@ | |||
from django.conf import settings | |||
from django.urls import re_path | |||
|
|||
from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail |
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:
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?
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:
- The API should be fast and easy;
- The API should return all users enrolments without pagination;
- It would be nice if the API did not return anything unnecessary except
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.
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.
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?
…active_courses_API
Hi @GlugovGrGlib @kdmccormick - just checking in on this to see if there's any updates? |
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.
As mentioned above, I'm looking for an outline of this FC's API endpoints so that I can review this PR and any other backend PR in the FC.
Hi @kdmccormick, I have prepared the document with the list of mobile API endpoints used or updated in this FC. Please check it out, you should have commenter access to it - https://docs.google.com/document/d/1IBd2kZqaWJuNFcr_80yWK6x6Nxc1yeYoJkRALn7mLZU |
…active_courses_API
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] |
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.
Each completion also stores the course key so it would be more efficient to directly query that as follows:
context_keys = BlockCompletion.objects.filter(
user__username=username,
created__gte=active_status_date
).values_list('context_key', flat=True).distinct()
Additionally, a user might have dozens or even hundreds of completions in a course, one for each block each for the same course so without adding distinct, it would return a lot of duplicates.
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.
@xitij2000 you are right, it will be more efficient and correct query. I apply your suggestion. Thanks for your code review!
Description
UserEnrollmentsStatus
API was implemented.API gets 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/
The HTTP 200 response contains a list of dictionaries that contain info
about each user's enrolment status.
Example Response
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
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: