feat: [FC-0047] Implement user's enrolments status API (#2530) #34859

KyryloKireiev · 2024-05-27T09:51:27Z

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

    ```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
        },
        ...
    ]
    ```

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:

will be open soon

openedx-webhooks · 2024-05-27T09:51:32Z

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If 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 @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mphilbrick211 · 2024-06-18T11:40:04Z

Hi @jawad-khan! Following up on this :)

jawad-khan · 2024-06-21T04:56:35Z

lms/djangoapps/mobile_api/users/views.py

+        """
+        Builds list with dictionaries with user's enrolments statuses.
+        """
+        user_enrollments = CourseEnrollment.objects.filter(


Instead of filtering objects directly we can use class method CourseEnrollment.enrollments_for_user

Ok, used this class method enrollments_for_user here

jawad-khan · 2024-06-21T05:13:14Z

lms/djangoapps/mobile_api/users/tests.py

+
+
+@ddt.ddt
+class UserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin):


Can we rename it to TestUserEnrollmentsStatus?

Yes, of course, there was some kind of failure with the name for the test case. Changed to TestUserEnrollmentsStatus

jawad-khan · 2024-06-21T05:25:15Z

lms/djangoapps/mobile_api/users/views.py

@@ -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):


Are we using it anywhere?

This probably got here from another commit. Removed

* 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]>

KyryloKireiev · 2024-06-21T19:52:15Z

Hi @jawad-khan! I fixed code review issues. It's ready for the second round now

GlugovGrGlib · 2024-07-08T19:27:08Z

Hi @kdmccormick, could you please review this PR as well?

KyryloKireiev · 2024-07-17T08:07:24Z

Hi @kdmccormick! You said you would be available after July 16th) Could you please review this PR as well?

KyryloKireiev · 2024-07-22T08:27:45Z

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?)

kdmccormick · 2024-07-22T15:15:19Z

lms/djangoapps/mobile_api/users/urls.py

@@ -6,7 +6,7 @@
 from django.conf import settings
 from django.urls import re_path

-from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail


@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?

@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 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.

Hi @kdmccormick! Please give us an answer if possible. What do you think about our architectural solution?

@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?

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.

@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.

…active_courses_API

mphilbrick211 · 2024-09-17T15:45:22Z

Hi @GlugovGrGlib @kdmccormick - just checking in on this to see if there's any updates?

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 27, 2024

KyryloKireiev changed the title ~~feat: [AXM-200] Implement user's enrolments status API (#2530)~~ feat: [FC-0047] Implement user's enrolments status API (#2530) May 28, 2024

kdmccormick self-assigned this Jun 13, 2024

GlugovGrGlib mentioned this pull request Jun 13, 2024

feat: [FC-0047] Extend mobile API with course progress and primary courses on dashboard view #34848

Merged

miankhalid requested a review from jawad-khan June 13, 2024 22:43

kdmccormick self-requested a review June 20, 2024 17:52

jawad-khan reviewed Jun 21, 2024

View reviewed changes

KyryloKireiev force-pushed the kireiev/AXM-549/feat/upstream_PR_active_inactive_courses_API branch from 5080c82 to ee862b3 Compare June 21, 2024 13:54

KyryloKireiev and others added 4 commits June 21, 2024 19:37

fix: [AXM-549] Added missing import

bd8b35d

style: [AXM-549] Remove unused import

e474abd

refactor: [AXM-549] Refactor UserEnrollmentsStatus API

1a7f55b

KyryloKireiev force-pushed the kireiev/AXM-549/feat/upstream_PR_active_inactive_courses_API branch from a66288f to 1a7f55b Compare June 21, 2024 16:37

jawad-khan approved these changes Jun 24, 2024

View reviewed changes

GlugovGrGlib mentioned this pull request Jul 3, 2024

feat: [FC-0047] Calendar synchronization openedx/openedx-app-ios#466

Merged

kdmccormick reviewed Jul 22, 2024

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 30, 2024

kdmccormick self-requested a review August 1, 2024 19:52

Merge branch 'master' into kireiev/AXM-549/feat/upstream_PR_active_in…

4b4fa5f

…active_courses_API

kdmccormick removed their request for review August 9, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [FC-0047] Implement user's enrolments status API (#2530) #34859

feat: [FC-0047] Implement user's enrolments status API (#2530) #34859

KyryloKireiev commented May 27, 2024 •

edited

Loading

openedx-webhooks commented May 27, 2024 •

edited

Loading

mphilbrick211 commented Jun 18, 2024

jawad-khan Jun 21, 2024

KyryloKireiev Jun 21, 2024

jawad-khan Jun 21, 2024

KyryloKireiev Jun 21, 2024

jawad-khan Jun 21, 2024

KyryloKireiev Jun 21, 2024

KyryloKireiev commented Jun 21, 2024

GlugovGrGlib commented Jul 8, 2024

KyryloKireiev commented Jul 17, 2024

KyryloKireiev commented Jul 22, 2024

kdmccormick Jul 22, 2024

KyryloKireiev Jul 23, 2024

KyryloKireiev Jul 29, 2024

kdmccormick Aug 5, 2024

GlugovGrGlib Aug 5, 2024

kdmccormick Aug 9, 2024

mphilbrick211 commented Sep 17, 2024



		@ddt.ddt
		class UserEnrollmentsStatus(MobileAPITestCase, MobileAuthUserTestMixin):

feat: [FC-0047] Implement user's enrolments status API (#2530) #34859

Are you sure you want to change the base?

feat: [FC-0047] Implement user's enrolments status API (#2530) #34859

Conversation

KyryloKireiev commented May 27, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented May 27, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

mphilbrick211 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyryloKireiev commented Jun 21, 2024

GlugovGrGlib commented Jul 8, 2024

KyryloKireiev commented Jul 17, 2024

KyryloKireiev commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mphilbrick211 commented Sep 17, 2024

KyryloKireiev commented May 27, 2024 •

edited

Loading

openedx-webhooks commented May 27, 2024 •

edited

Loading