-
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-0031] Add new endpoint CourseInfoDetailView #33297
feat: [FC-0031] Add new endpoint CourseInfoDetailView #33297
Conversation
Thanks for the pull request, @oksana-slu! 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. |
a6cca1c
to
86dd6a7
Compare
86dd6a7
to
57c3f5f
Compare
57c3f5f
to
118a200
Compare
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.
@oksana-slu given that the existing CourseDetailView
already can take a username
as an optional parameter. Why not simply enhance that endpoint to provide enrollment data if a username is passed in? That seems reasonable and more robust than to make a whole new endpoint just for mobile that is highly coupled to that endpoint anyway.
Hi @feanil, when discussing the solution, we considered these two main options: customize What do you think, is it a fair trade-off? |
@GlugovGrGlib so the idea would be that if you wanted to have a custom course about page, you would override the serializer for the Mobile endpoint we are adding and it would not impact the other course detail endpoint? I would think that if you are customizing the mobile view you might also want to customize the non-mobile view of the same info, is that not often the case? It seems like this would introduce drift between what data is available to the mobile app vs the web app which is something I would think we want to reduce. |
@feanil Also, (AFAIK) the main use case for Furthermore, according to the issue #31620, there isn't a decision for transitioning this page into MFE. So it's not clear whether the future MFE page would exist and if it will be decided to use exclusively |
@GlugovGrGlib long term, everything will become an MFE so whatever API we end up using for mobile will hopefully also work for that future MFE. In that future, I would assume the mobile access pattern here wouldn't be too different from the MFE access pattern. Also, having the names |
@feanil we are working on the suggested long term appropriate solution with extending |
@oksana-slu Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
1 similar comment
@oksana-slu Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Add a new mobile endpoint that is based on the CourseDetailView endpoint
but extends it with information about user enrolment.
Supporting information
This contribution is done as a part of the project FC-0031
Testing instructions
GET /api/mobile/v3/course_info/{course_key}/info
See that
"is_enrolled"
is present in the response