-
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
Open
KyryloKireiev
wants to merge
8
commits into
openedx:master
Choose a base branch
from
raccoongang:kireiev/AXM-549/feat/upstream_PR_active_inactive_courses_API
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+260
−3
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0d0503a
feat: [AXM-200] Implement user's enrolments status API (#2530)
KyryloKireiev bd8b35d
fix: [AXM-549] Added missing import
KyryloKireiev e474abd
style: [AXM-549] Remove unused import
KyryloKireiev 1a7f55b
refactor: [AXM-549] Refactor UserEnrollmentsStatus API
KyryloKireiev 4b4fa5f
Merge branch 'master' into kireiev/AXM-549/feat/upstream_PR_active_in…
GlugovGrGlib 1b369b7
Merge branch 'master' into kireiev/AXM-549/feat/upstream_PR_active_in…
GlugovGrGlib 3bd2031
Merge branch 'master' into kireiev/AXM-549/feat/upstream_PR_active_in…
KyryloKireiev 1eec8b8
fix: [AXM-549] Use more efficient query
KyryloKireiev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: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: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.
@GlugovGrGlib
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?