feat: [FC-0031] Add new endpoint BlocksInfoInCourseView #33296

oksana-slu · 2023-09-20T11:16:44Z

Description

Add a new mobile endpoint that extends BlocksInCourseView, and add more data about a course as media, course dates, certificate etc.

This endpoint was added into mobile_api LMS application. In the future, this endpoint can be extended to be, for example, a coursee home endpoint for a mobile.

Supporting information

This contribution is done as a part of the project FC-0031

Testing instructions

GET /api/mobile/{api_version}/course_info/blocks/?course_id=<course_id>
Check that fields start, media, certificate are in the reponse.

openedx-webhooks · 2023-09-20T11:16:48Z

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.

jawad-khan · 2023-10-06T14:19:59Z

lms/djangoapps/mobile_api/course_info/views.py

+
+    **Example requests**:
+
+        This api works with all versions {api_version}, you can use: v0.5, v1, v2 or v3


Why introduce a new version if it can work with previous versions?

Hello @jawad-khan, the version 3 was introduced first of all in PR: #33294
Here we have added version 3 support as per comment https://openedx.atlassian.net/wiki/spaces/COMM/pages/3853910042?focusedCommentId=3856859206 for general consistency in new mobile applications.

Ed's suggestion was to make it more intuitive for using mobile API with new and old apps:

one could assume that any call to the API version n or above would be for the new apps, < n for the legacy apps

jawad-khan · 2023-10-06T14:22:41Z

lms/djangoapps/mobile_api/course_info/views.py

+            &block_counts=video
+            &student_view_data=video
+            &block_types_filter=problem,html
+


Can you add information about the params this view accpets?

Yes, I have done that

You can refer to the parent class about all params not used here. We are only using course_id and return_type here.

jawad-khan · 2023-10-06T14:38:28Z

lms/djangoapps/mobile_api/course_info/views.py

+            information about each block. Each block contains the following
+            fields.
+
+        id: (str) The Course's id (Course Run key)


what if we call the view with list param, will we still get these fields?

Ok, it's done. Add description to "return_type=list"

If we call this view with return_type=list we will not get certificate and all other details processed in this view. Please update view according to the desired behavior.

@jawad-khan, the docstring was updated to explicitly include data about the response when return_type=list is provided

jawad-khan · 2023-10-06T14:51:29Z

lms/djangoapps/mobile_api/course_info/views.py

+            course_data = {
+                # identifiers
+                'id': course_id,
+                'name': course_overview.display_name,


Can we move course overview data in another function and update dict here?
This will clean this view method.

Yes, I moved course overview data to new method "compose_course_info"

GlugovGrGlib · 2023-11-06T10:07:42Z

@jawad-khan kindly ask you for another round of review

feanil

I'm a little confused by this PR, the one new view simply returns courseoverviews data and I don't see how it does anything with pulling block data out of the system.

feanil · 2023-11-16T19:47:58Z

lms/djangoapps/mobile_api/course_info/views.py

+    """
+
+    def get_certificate(self, request, course_id):
+        """Returns the information about the user's certificate in the course."""


Given that this is a public function, can you document the input parameters and the format of the output data?

input and output docs still missing.

feanil · 2023-11-16T19:50:25Z

lms/djangoapps/mobile_api/course_info/views.py

+        Method for obtaining additional information about the course.
+
+        Arguments:
+            request - Django request object


This is incorrect, update to say that it expects a CourseOverviews object as input.

Thanks, this is a mistake, should be fixed

This function was replaced with serializer as suggested

feanil · 2023-11-16T19:51:03Z

lms/djangoapps/mobile_api/course_info/views.py

+        return {}
+
+    @staticmethod
+    def compose_course_info(course_overview):


Why have this function instead of using a serializer? For the places where names differ you can just set them explicitly.

We will check what to do with this

We are already extracting most of the info related to course info and certificate in this serializer.
Can we use that api, or serializer for this case?

@feanil @jawad-khan This function was replaced with CourseInfoOverviewSerializer, please review the latest changes

jawad-khan · 2023-12-05T09:21:09Z

lms/djangoapps/mobile_api/course_info/views.py

+        response = super().list(request, kwargs)
+
+        if request.GET.get('return_type', 'dict') == 'dict':
+            course_id = request.query_params.get('course_id', None)


What if course_id is None?
Please handle all these cases in tests.

Hi Jawad!
The course_id is a required field and validated in the parent class - here https://github.com/raccoongang/edx-platform/blob/master/lms/djangoapps/course_api/blocks/views.py#L305

Furthermore, all the test cases are covered because TestBlocksInfoInCourseView class is extending TestBlocksInCourseView which has those test cases covered.

jawad-khan · 2023-12-05T09:28:50Z

lms/djangoapps/mobile_api/course_info/views.py

+            &block_counts=video
+            &student_view_data=video
+            &block_types_filter=problem,html
+


You can refer to the parent class about all params not used here. We are only using course_id and return_type here.

jawad-khan · 2023-12-05T09:31:34Z

lms/djangoapps/mobile_api/course_info/tests.py

+
+
+@ddt.ddt
+class TestBlocksInfoInCourseView(TestBlocksInCourseView):  # lint-amnesty, pylint: disable=test-inherits-tests


Add test cases for all the response types mentioned in response status codes returned by this view.

Just because we are extending TestBlocksInCourseView we decided and agreed to focus only on testing the changes in response (new fields as media, course dates, certificate) exist in response

jawad-khan · 2023-12-05T09:34:00Z

lms/djangoapps/mobile_api/course_info/views.py

+            information about each block. Each block contains the following
+            fields.
+
+        id: (str) The Course's id (Course Run key)


If we call this view with return_type=list we will not get certificate and all other details processed in this view. Please update view according to the desired behavior.

jawad-khan · 2023-12-05T09:35:24Z

lms/djangoapps/mobile_api/course_info/views.py

+    """
+
+    def get_certificate(self, request, course_id):
+        """Returns the information about the user's certificate in the course."""


input and output docs still missing.

jawad-khan · 2023-12-05T09:53:18Z

lms/djangoapps/mobile_api/course_info/views.py

+        return {}
+
+    @staticmethod
+    def compose_course_info(course_overview):


We are already extracting most of the info related to course info and certificate in this serializer.
Can we use that api, or serializer for this case?

GlugovGrGlib · 2023-12-15T21:23:55Z

@feanil This is ready for review, the function was replaced with serializer, also docstrings were updated

feanil

@GlugovGrGlib I think instead of adding this as a new mobile view, why not add this as more parameters on the existing block view? We could provide the new data behind a query param so as not to break the existing behavior if necessary. But this feels like a lot of copying just to add additional data. It looks like the only difference between this endpoint and the original one is the addition or the image urls, is that right?

volodymyr-chekyrta · 2023-12-19T10:36:17Z

Hi @feanil, while designing, @GlugovGrGlib and I determined that creating a dedicated endpoint for the mobile course home screen could be advantageous in the long run.
This would allow us to easily extend this endpoint with features such as progress, highlighted discussions, and so forth in the future.

cc @marcotuts

feanil

Making a specific backend for the mobile frontend makes sense, I'm concerned that changes in the upstream classes will be harder to make or cause downstream issues here but I think the tests you have will hopefully catch that. I think we should update the docstring for the new class to make it clear that this API endpoint is specifically meant to optimize the mobile homepage. Once we've updated that, I'm okay with merging this.

feanil · 2023-12-19T14:00:15Z

lms/djangoapps/mobile_api/course_info/views.py

+    """
+    **Use Case**
+
+        Returns the blocks in the course according to the requesting user's access level.


I think if this is going to be an API optimized for the mobile home page, we should update the comment to say so.

The docstring is updated.
Thank you!

openedx-webhooks · 2023-12-19T18:02:08Z

@oksana-slu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-12-19T19:17:30Z

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

edx-pipeline-bot · 2023-12-19T19:39:03Z

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

edx-pipeline-bot · 2023-12-19T19:50:50Z

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

edx-pipeline-bot · 2023-12-19T20:09:56Z

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

edx-pipeline-bot · 2023-12-19T20:09:56Z

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

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

KyryloKireiev force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch from 835815e to 36dd2cd Compare September 22, 2023 10:52

oksana-slu force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch from 36dd2cd to 0807140 Compare September 22, 2023 10:58

oksana-slu marked this pull request as ready for review September 25, 2023 11:49

marcotuts mentioned this pull request Sep 27, 2023

Mobile API Updates (FC-0031) #33304

Closed

jawad-khan self-requested a review October 4, 2023 11:11

jawad-khan reviewed Oct 6, 2023

View reviewed changes

marcotuts added the FC Relates to an Axim Funded Contribution project label Oct 13, 2023

KyryloKireiev force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch from ed315cb to 4e20865 Compare October 30, 2023 15:27

GlugovGrGlib requested a review from feanil November 15, 2023 21:51

feanil reviewed Nov 16, 2023

View reviewed changes

jawad-khan reviewed Dec 5, 2023

View reviewed changes

KyryloKireiev force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch 2 times, most recently from 6e21bc1 to 022d508 Compare December 15, 2023 12:41

feanil reviewed Dec 18, 2023

View reviewed changes

feanil approved these changes Dec 19, 2023

View reviewed changes

GlugovGrGlib force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch from e0e4b34 to 774dd6b Compare December 19, 2023 16:50

KyryloKireiev and others added 4 commits December 19, 2023 19:15

feat: [AXIM-26] Extended BlocksInCourseView API

c4fe8d3

fix: [FC-0031] Add parameters description, refactor list method

81c9929

refactor: [FC-0031] Use serializer instead of custom function

003f6d9

docs: [FC-0031] Update docstring

cae4091

GlugovGrGlib force-pushed the rg/feat/FC0031/extended_blocks_in_course_view branch from 774dd6b to cae4091 Compare December 19, 2023 17:15

feanil approved these changes Dec 19, 2023

View reviewed changes

feanil merged commit 1b54e91 into openedx:master Dec 19, 2023
64 checks passed

This was referenced Feb 21, 2024

feat(mobile_api): Add course access object to mobile course info API raccoongang/edx-platform#2507

Closed

feat(mobile_api): Add course access object to mobile course info API #34273

Merged

OmarIthawi mentioned this pull request May 31, 2024

feat: cherry pick mobile apis nelc/edx-platform#29

Merged

10 tasks


		Example requests:

		This api works with all versions {api_version}, you can use: v0.5, v1, v2 or v3



		@ddt.ddt
		class TestBlocksInfoInCourseView(TestBlocksInCourseView): # lint-amnesty, pylint: disable=test-inherits-tests

feat: [FC-0031] Add new endpoint BlocksInfoInCourseView #33296

feat: [FC-0031] Add new endpoint BlocksInfoInCourseView #33296

Conversation

oksana-slu commented Sep 20, 2023 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Sep 20, 2023 • edited Loading

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

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

GlugovGrGlib commented Nov 6, 2023

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlugovGrGlib commented Dec 15, 2023

feanil left a comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Dec 19, 2023

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Dec 19, 2023

edx-pipeline-bot commented Dec 19, 2023

edx-pipeline-bot commented Dec 19, 2023

edx-pipeline-bot commented Dec 19, 2023

edx-pipeline-bot commented Dec 19, 2023

edx-pipeline-bot commented Dec 19, 2023

oksana-slu commented Sep 20, 2023 •

edited

Loading

openedx-webhooks commented Sep 20, 2023 •

edited

Loading