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: [FC-0047] Implement user's enrolments status API (#2530) #34859

Conversation

KyryloKireiev
Copy link
Contributor

@KyryloKireiev KyryloKireiev commented May 27, 2024

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/

USER_ENROLLMENTS_LIMIT - adds users enrollments query limit to
safe API from possible DDOS attacks.

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",
            "recently_active": true
        },
        {
            "course_id": "course-v1:b+b+b",
            "course_name": "b",
            "recently_active": true
        },
        {
            "course_id": "course-v1:c+c+c",
            "course_name": "c",
            "recently_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
Copy link

openedx-webhooks commented May 27, 2024

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 27, 2024
@KyryloKireiev 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 kdmccormick self-assigned this Jun 13, 2024
@mphilbrick211
Copy link

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

@kdmccormick kdmccormick self-requested a review June 20, 2024 17:52
"""
Builds list with dictionaries with user's enrolments statuses.
"""
user_enrollments = CourseEnrollment.objects.filter(
Copy link
Contributor

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

Copy link
Contributor Author

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):
Copy link
Contributor

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?

Copy link
Contributor Author

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):
Copy link
Contributor

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?

Copy link
Contributor Author

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

@KyryloKireiev 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
* 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 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
@KyryloKireiev
Copy link
Contributor Author

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

@GlugovGrGlib
Copy link
Member

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

@KyryloKireiev
Copy link
Contributor Author

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

@KyryloKireiev
Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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:

  1. The API should be fast and easy;
  2. The API should return all users enrolments without pagination;
  3. 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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

@KyryloKireiev Thanks for that background.

Pagination/limits

In order to keep large Open edX sites safe from DDOS attacks, all HTTP APIs need to be paginated (with a max page size) or limited (for example, only return the 1000 most relevant results).

Without pagination or limiting, a bad-faith actor (or a sufficiently advanced good-faith user) could take down the site by creating a huge number of enrollments and then repeatedly calling the unbounded API.

user_enrollments API versus enrollments_status

With this PR merged, we would have two separate APIs for listing enrollments: user_enrollments and enrollments_status. I want to understand whether we truly need both APIs.

Once enrollments_status is used by the new apps, will they still use user_enrollments too?

  • If no, can user_enrollments be deprecated?
  • If yes, then is user_enrollments always called alongside enrollments_status?
    • If yes, then why does the speed of user_enrollments matter, since you will always need both API responses anyway?

@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 30, 2024
@kdmccormick kdmccormick self-requested a review August 1, 2024 19:52
@kdmccormick kdmccormick removed their request for review August 9, 2024 17:03
@mphilbrick211
Copy link

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

Copy link
Member

@kdmccormick kdmccormick left a 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.

@GlugovGrGlib
Copy link
Member

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

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Thank you for the helpful Google doc, and thank you for your patience.

Comment on lines 628 to 630
'is_active': bool(
course_id in course_ids
or user_enrollment.created > active_status_date
Copy link
Member

Choose a reason for hiding this comment

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

There is already a CourseEnrollment.is_active field, and it has a different meaning than this new field. Please pick a new name for this field that doesn't conflict, and update the API as well as the tests. For example, you could use is_fresh, or recently_active.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, definitely agree rename is necessary in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think recently_active is good idea. So, changed is_active --> recently_active.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Please update your PR description too

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

from .views import UserCourseEnrollmentsList, UserCourseStatus, UserDetail
Copy link
Member

Choose a reason for hiding this comment

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

@KyryloKireiev Thanks for that background.

Pagination/limits

In order to keep large Open edX sites safe from DDOS attacks, all HTTP APIs need to be paginated (with a max page size) or limited (for example, only return the 1000 most relevant results).

Without pagination or limiting, a bad-faith actor (or a sufficiently advanced good-faith user) could take down the site by creating a huge number of enrollments and then repeatedly calling the unbounded API.

user_enrollments API versus enrollments_status

With this PR merged, we would have two separate APIs for listing enrollments: user_enrollments and enrollments_status. I want to understand whether we truly need both APIs.

Once enrollments_status is used by the new apps, will they still use user_enrollments too?

  • If no, can user_enrollments be deprecated?
  • If yes, then is user_enrollments always called alongside enrollments_status?
    • If yes, then why does the speed of user_enrollments matter, since you will always need both API responses anyway?

@KyryloKireiev
Copy link
Contributor Author

Hi! @kdmccormick I will try to answer your question

Once enrollments_status is used by the new apps, will they still use user_enrollments too?

  • If no, can user_enrollments be deprecated?
  • If yes, then is user_enrollments always called alongside enrollments_status?
    • If yes, then why does the speed of user_enrollments matter, since you will always need both API responses anyway?
  1. enrollments_status API and user_enrollments API are used by different applications.
    The next question:
  • If yes, then is user_enrollments always called alongside enrollments_status?

user_enrollments - this is a heavy large query for rendering my courses page.
enrollment_status- this is a simple query to get all courses of a user for calendar sync.

So, we used these two APIs to render two different pages on mobile. I agree, we don't use pagination for enrollments_status API. Because we want to get all user enrollments in one request. Maybe we should really consider adding a limit (for example, only return the 1000 most relevant results)?I don't think any user can have more than 1000 errollments.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

OK, I understand now why this needs to be its own endpoint, thank you @KyryloKireiev .

Maybe we should really consider adding a limit (for example, only return the 1000 most relevant results)?I don't think any user can have more than 1000 enrollments.

When I worked at edX.org, we had some users with 2500+ enrollments. If I remember correctly, they were members of the support team who weren't granted global staff access, but were granted instructor access in every course on the site, and were also enrolled in every course on the site. You could argue that this is a strange way to use the platform. Regardless, it is what happened, so this new API would return end up returning 2500+ enrollment objects for those users.

That is a specific example, but there is a general purpose of me saying it: even if it seems unreasonable for an API to return so many items, Open edX is a flexible and highly-scalable platform, so these very high return counts are still be possible. Therefore, we must use either limits or pagination on all HTTP APIs in order to protect the stability of the platform for highly-scaled environments.

Please choose a reasonable limit (something in the 100 to 1000 range?) and apply it to the new API, and I'll be happy to approve this PR.

@KyryloKireiev
Copy link
Contributor Author

@kdmccormick

When I worked at edX.org, we had some users with 2500+ enrollments. If I remember correctly, they were members of the
support team who weren't granted global staff access, but were granted instructor access in every course on the site, and were also enrolled in every course on the site. You could argue that this is a strange way to use the platform. Regardless, it is what happened, so this new API would return end up returning 2500+ enrollment objects for those users.

I couldn't even imagine that there could be such a large number of enrollments. I added a default maximum value of enrollments that will be given to each user. I think this will cover 99% of all needs and protect this API from DDoS attacks. In any case, the constant USER_ENROLLMENTS_LIMIT can be easily changed if we want to receive more user enrollments.

@KyryloKireiev
Copy link
Contributor Author

Hi @kdmccormick!

Hope you're doing well!

I'm reaching out to see if you have a moment to review my pull request. I'd really appreciate your feedback and insights.

Let me know if you need any additional information or have any questions.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Just one more suggestion, then this is good to go.

I have not manually tested this PR, so I'm trusting that you or your other reviewers have.

Comment on lines 651 to 660
) -> List[str]:
"""
Gets course ids where user has completions.
"""
context_keys = BlockCompletion.objects.filter(
user__username=username,
created__gte=active_status_date
).values_list('context_key', flat=True).distinct()

return [str(context_key) for context_key in context_keys]
Copy link
Member

Choose a reason for hiding this comment

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

First off, thanks for adding type annotations here 👍🏻

Secondly, notice that you are converting these context keys to strings, and you are also converting the user_enrollment.course_overview.id to a string. You could make the whole system simpler and more type-safe by not doing that--just return ContextKey objects from this function, and then check whether the user enrollment's course id (which is also a ContextKey) belongs to that collection of objects.

Lastly, and this is just an optional nit, you could return a set of keys rather than a list, since your use of distinct() guarantees that there are no duplicates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Set will also be a performance win since the loop on line 633 is using that list to determine if the course is active which is using the in operator which is faster for a set than a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdmccormick @xitij2000 Hello again! I made the changes you wrote about above. Please take a look when you have time. Now we use Course ContextKey instead of id sting. Also I used Set with context_keys instead of List.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

LGTM. Do you need me to merge?

@KyryloKireiev
Copy link
Contributor Author

LGTM. Do you need me to merge?

@kdmccormick That would be great. I don't have the rights to merge.

@KyryloKireiev
Copy link
Contributor Author

Hi @kdmccormick!

Hope you're doing well!

I wanted to gently remind you about my pull request :)
Our mobile team needs this pull request to release mobile APPs in master) If you have time, please merge this PR

@cmltaWt0
Copy link
Contributor

cmltaWt0 commented Nov 1, 2024

@kdmccormick two approves are here. Merging it.

@cmltaWt0 cmltaWt0 merged commit bd22449 into openedx:master Nov 1, 2024
49 checks passed
@kdmccormick
Copy link
Member

Thanks Max

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants