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-0031] Add new endpoint CourseInfoDetailView #33297

Conversation

oksana-slu
Copy link
Contributor

@oksana-slu oksana-slu commented Sep 20, 2023

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

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 20, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 20, 2023

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@KyryloKireiev KyryloKireiev force-pushed the rg/feat/FC0031/add_new_endpoint_course_info_detail branch from a6cca1c to 86dd6a7 Compare September 22, 2023 11:39
@oksana-slu oksana-slu force-pushed the rg/feat/FC0031/add_new_endpoint_course_info_detail branch from 86dd6a7 to 57c3f5f Compare September 22, 2023 11:40
@oksana-slu oksana-slu force-pushed the rg/feat/FC0031/add_new_endpoint_course_info_detail branch from 57c3f5f to 118a200 Compare September 25, 2023 08:09
@oksana-slu oksana-slu marked this pull request as ready for review September 25, 2023 11:31
@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 2, 2023
Copy link
Contributor

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

@GlugovGrGlib
Copy link
Member

Hi @feanil, when discussing the solution, we considered these two main options: customize CourseDetailView or extend it in mobile APIs application. I agree that is_enrolled option could be better added directly in the CourseDetailView, but because the About page is very frequently extended with custom parameters in the community on the web, we decided to introduce a separate mobile endpoint with a separate URL. We believe it will allow the community provider to easily extend it via Serializer override or override this new URL to introduce customizations for About page for mobile apps only.

What do you think, is it a fair trade-off?

@feanil
Copy link
Contributor

feanil commented Oct 16, 2023

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

@GlugovGrGlib
Copy link
Member

@feanil
Currently, when customizing About Course page on the web, there is no need to modify this REST API at all because the About Course page is still rendered on the backend.

Also, (AFAIK) the main use case for CourseDetailView is to expose an external API to fetch the course data, the data we want to provide to synchronize backend rendered web and mobile views may be redundant in some cases. Of course, it's not the case if operator creates own frontend to completely replace About Course, but I'm not sure how popular that may be.

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 CourseDetailView to load data.

@feanil
Copy link
Contributor

feanil commented Oct 17, 2023

@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 CourseDetailView and a CourseInfoDetailView are confusing names. We're how would you know when to use one over the other? I still don't understand why using this is better than using the existing optional username parameter to populate additional data in the existing view. It sounds like the concern might be that we'll break something? Is it that? Something else?

@GlugovGrGlib
Copy link
Member

@feanil we are working on the suggested long term appropriate solution with extending CourseDetailView, so the response for this view will be extended if an optional parameter username is passed. We will close this PR and open a new PR with the respective changes, thank you.

@openedx-webhooks
Copy link

@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
@openedx-webhooks
Copy link

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

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
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants