backend view to return tree for course outline and any given block. by mushtaqak · Pull Request #14204 · openedx/openedx-platform

mushtaqak · 2016-12-22T14:11:23Z

Description

As a part of Move Epic, this PR adds a functionality to return concise course outline data and ancestor info (parents) of the XBlock .

Below is the sample concise course outline we can get :

{
    'category': 'course',
    'display_name': 'Run 23',
    'id': 'i4x://org.23/course_23/course/Run_23',
    'child_info': {
        'category': 'chapter',
        'display_name': 'Section',
        'children': [{
            'category': 'chapter',
            'display_name': 'Week 1',
            'id': 'i4x://org.23/course_23/chapter/Week_1',
            'child_info': {
                'category': 'sequential',
                'display_name': 'Subsection',
                'children': [{
                    'category': 'sequential',
                    'display_name': 'Lesson 1',
                    'id': 'i4x://org.23/course_23/sequential/Lesson_1',
                    'child_info': {
                        'category': 'vertical',
                        'display_name': 'Unit',
                        'children': [{
                            'category': 'vertical',
                            'display_name': 'Subsection 1',
                            'id': 'i4x://org.23/course_23/vertical/Subsection_1',
                            'child_info': {
                                'children': [{
                                    'category': 'video',
                                    'display_name': 'My Video',
                                    'id': 'i4x://org.23/course_23/video/My_Video'
                                }]
                            }
                        }]
                    }
                }]
            }
        }]
    }
}

Below is the sample ancestor info we can get :

{
    'ancestors': [{
        'category': 'vertical',
        'display_name': 'vertical1',
        'id': 'i4x://org.0/course_0/vertical/4df65ef9c6334c4e93458150bf592349'
    }, {
        'category': 'sequential',
        'display_name': 'seq1',
        'id': 'i4x://org.0/course_0/sequential/fbb1be44e2774d34adf141263c55a13d'
    }, {
        'category': 'chapter',
        'display_name': 'chapter1',
        'id': 'i4x://org.0/course_0/chapter/29604e92469e410fb2431e5c37380d15'
    }, {
        'category': 'course',
        'display_name': 'Run 0',
        'id': 'i4x://org.0/course_0/course/Run_0'
    }]
}

TNL-6061

Testing

Unit

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍 or approve review.

Code review: @muhammad-ammar
Code review: @andy-armstrong

Post-review

Squash commits

mushtaqak · 2016-12-22T17:03:16Z

@cpennington Do you have some time to review this ?

muhammad-ammar

@mushtaqak This is looking great. I left comments about couple of minor changes. I am done with first pass of the review. Let me know once you addressed the feedback.

muhammad-ammar · 2016-12-28T09:59:37Z

cms/djangoapps/contentstore/views/tests/test_item.py



+@ddt.ddt
+class TestMoveItem(ItemTest):


@mushtaqak I think CourseOutlineTreeTest or TestCourseOutlineTree would be more appropriate because we not moving anything right now.

I think this should go in TestCourseOutline which already exists and which covers a lot of the kinds of tests that are needed. I think the course outline should take a concise=true parameter or something to give a more performant response for the move modal.

muhammad-ammar · 2016-12-28T10:00:26Z

cms/djangoapps/contentstore/views/tests/test_item.py

+@ddt.ddt
+class TestMoveItem(ItemTest):
+    """
+    Tests for move item.


Tests for course outline tree.?

muhammad-ammar · 2016-12-28T10:01:38Z

cms/djangoapps/contentstore/views/tests/test_item.py

+    """
+    def setUp(self):
+        """
+        Creates the test course structure and a few components to 'move'.


what about to build course outline tree instead of to move?

muhammad-ammar · 2016-12-28T10:04:33Z

cms/djangoapps/contentstore/views/tests/test_item.py

+                                  boilerplate='multiplechoice.yaml')
+        self.problem_usage_key = self.response_usage_key(resp)
+
+        resp = self.create_xblock(parent_usage_key=self.vert_usage_key, display_name='html1', category='html')


What about giving contextual names to above variables instead of resp? For example

chapter1 = self.create_xblock(parent_usage_key=self.usage_key, display_name='chapter1', category='chapter')

muhammad-ammar · 2016-12-28T10:05:51Z

cms/djangoapps/contentstore/views/tests/test_item.py

+        for child in xblock_children:
+            child_info = get_xblock_outline(child)
+            self.assert_xblock_info(child, child_info)
+            self.assert_xblock_child_info(child, child_info)


It would be nice if we can add docstrings for positional arguments in all of the above util methods as per edX coding guideline.

muhammad-ammar · 2016-12-28T10:57:17Z

cms/djangoapps/contentstore/views/item.py

+def get_xblock_outline(xblock):
+    """
+    Returns the complete outline of of an xblock.
+    """


Please add docstring with sample dict structure that will be returned.

muhammad-ammar · 2016-12-28T11:05:44Z

cms/djangoapps/contentstore/views/tests/test_item.py

+        for usage_key in (self.problem_usage_key, self.vert_usage_key, self.seq_usage_key):
+            xblock = self.get_item_from_modulestore(usage_key)
+            url = reverse('contentstore.views.xblock_handler', kwargs={'usage_key_string': unicode(usage_key)})
+            url = url + '?course_tree=' + str(fetch_course_tree) if fetch_course_tree else url


Why we need
if fetch_course_tree else url
why not just
url = url + '?course_tree={}'.format(etch_course_tree)?

muhammad-ammar · 2016-12-28T11:08:11Z

cms/djangoapps/contentstore/views/tests/test_item.py

+        """
+        for usage_key in (self.problem_usage_key, self.vert_usage_key, self.seq_usage_key):
+            xblock = self.get_item_from_modulestore(usage_key)
+            url = reverse('contentstore.views.xblock_handler', kwargs={'usage_key_string': unicode(usage_key)})


I think we can remove kwargs

muhammad-ammar · 2016-12-28T11:08:42Z

cms/djangoapps/contentstore/views/tests/test_item.py

+    @ddt.data(1, 0)
+    def test_get_course_tree(self, fetch_course_tree):
+        """
+        Test that we get course tree.


Please add docstring for fetch_course_tree

muhammad-ammar · 2016-12-28T11:09:32Z

cms/djangoapps/contentstore/views/tests/test_item.py

+                self.assert_xblock_info(xblock, response['xblock_info'])
+                self.assert_course_outline(response['course_outline'])
+            else:
+                self.assertNotIn('xblock_info', response)


What else is included in the response? Can we just check if response is {}?

We don't get the empty response but we get _get_module_info of the block.

mushtaqak · 2016-12-28T17:29:11Z

@andy-armstrong Do you have some time to review this PR. Thanks :)

andy-armstrong · 2016-12-28T17:41:18Z

@mushtaqak I implemented a very similar API two years ago when we were first going to implement moving of XBlocks. Can you look at this and see if you can reuse it, and if not then remove my work since I don't believe it was ever used (we moved onto other projects).

https://github.com/edx/edx-platform/pull/2216

mushtaqak · 2016-12-28T17:47:58Z

@andy-armstrong sure, I can think we can use/re-use some of the code like _xmodule_json and _course_json methods that I have created too that have kind of similar logic :)

andy-armstrong · 2016-12-28T18:48:30Z

@mushtaqak Just to confirm, you are planning to use the same course tree as used in the main course outline, right? We built it so that it could be reused in situations like this. For this to work, your API should return the data in the same JSON structure

mushtaqak · 2016-12-29T11:24:12Z

@andy-armstrong The methods in PR that you mentioned above, had been reworked in this commit f061bbc and now it is create_xblock_info which calculates a lot of data and returns a complex json.

However, we just need the basic data display name, id/location, category and children.

andy-armstrong · 2016-12-29T16:58:30Z

Thanks for the update, @mushtaqak. I recommend that you refactor the code so that the API can take a parameter to control how much data is returned. I don't think it makes sense to have multiple REST APIs to return course tree information.

mushtaqak · 2016-12-29T17:23:51Z

@andy-armstrong Instead of reusing create_xblock_info, We have decided to not use the #2216, but to go on with this new approach i.e to write new util methods that will be fast and would not need unnecessary information.

If we are using the create_xblock_info we would still need to do all the work that we have done and this method would not make much sense.

mushtaqak · 2016-12-30T11:48:20Z

@muhammad-ammar Feedback addressed.

muhammad-ammar · 2017-01-03T06:33:31Z

cms/djangoapps/contentstore/views/item.py

+    Arguments:
+        xblock (Xblock): An XBlock whose summary is to be made.
+        include_children (bool): If True, includes child XBlocks information.
+        child_info (List): A list of info of children of the XBlock


@mushtaqak this is not an argument. we only have two arguments.

@muhammad-ammar addressed this

mushtaqak · 2017-01-03T13:25:39Z

@andy-armstrong May you please have a look :) Thanks

andy-armstrong

@mushtaqak after reviewing this PR, I'm more of the opinion that adding a course outline to the XBlock API is too confusing when we already have a course outline API. IMO you could use the existing API as is and the only issue would be that it returns unnecessary information. It could then be an optimization to allow the client to request a more concise version. This was always the intention of the course outline API, although I can see that it has become bloated as new features have been added.

If you disagree, it may be more efficient to have a quick meeting to discuss the options here. Let me know if you want to do this.

andy-armstrong · 2017-01-03T22:28:11Z

cms/djangoapps/contentstore/views/item.py

    GET
        json: returns representation of the xblock (locator id, data, and metadata).
              if ?fields=graderType, it returns the graderType for the unit instead of the above.
+              if ?course_tree=1, it returns the a course_outline summary and XBlock summary.


I don't like 0/1 for boolean parameters. Our standard convention is true/false, such as here:

https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/views.py#L2038

Interestingly, this isn't documented in our current API best practices, so I added a comment about it on @efagin's OEP: openedx/openedx-proposals#36

In fact, I'd prefer that we not add a new boolean parameter each time we think of a new format. I'd rather you have a more generic option such as format=course_tree. This also seems very similar to @nasthagiri's course blocks API, so maybe that can be shared between the LMS and Studio.

Nit: "the a course_outline"

andy-armstrong · 2017-01-03T22:31:11Z

cms/djangoapps/contentstore/views/item.py

                if 'graderType' in fields:
                    # right now can't combine output of this w/ output of _get_module_info, but worthy goal
                    return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key))
+                if int(request.GET.get('course_tree', 0)):


I'd prefer:

if bool(request.GET.get('course_tree', False)):

andy-armstrong · 2017-01-03T22:33:22Z

cms/djangoapps/contentstore/views/item.py

+                    course = store.get_course(xblock.location.course_key)   # pylint: disable=no-member
+                    xblock_info = xblock_summary(xblock)
+                    course_outline = xblock_summary(course, include_children=True)
+                    return JsonResponse({'course_outline': course_outline, 'xblock_info': xblock_info})


IMO it would make more sense to have this logic live in _get_module_info, so why not pass that parameter to it. That would simplify this logic, and it would also have it wrapped inside bulk_operations.

andy-armstrong · 2017-01-03T22:43:08Z

cms/djangoapps/contentstore/views/item.py

+                    xblock = _get_xblock(usage_key, request.user)
+                    course = store.get_course(xblock.location.course_key)   # pylint: disable=no-member
+                    xblock_info = xblock_summary(xblock)
+                    course_outline = xblock_summary(course, include_children=True)


Why does this API need to return both the course outline and the info about a given XBlock? It makes it into a confusing API, IMO. Presumably the move modal already has the XBlock info, so I think all it needs is the course outline. I would rather see a single course outline API that can be used by both the full course outline and by this new modal.

andy-armstrong · 2017-01-03T22:44:13Z

cms/djangoapps/contentstore/views/item.py

+        'category': xblock.category
+    }
+    if include_children and xblock.has_children:
+        xblock_info['child_info'] = [xblock_summary(child) for child in xblock.get_children()]


Shouldn't this pass include_children to the xblock_summary in order to get the full tree? The default is not to include the children.

andy-armstrong · 2017-01-03T22:49:34Z

cms/djangoapps/contentstore/views/tests/test_item.py



+@ddt.ddt
+class TestMoveItem(ItemTest):


I think this should go in TestCourseOutline which already exists and which covers a lot of the kinds of tests that are needed. I think the course outline should take a concise=true parameter or something to give a more performant response for the move modal.

mushtaqak · 2017-01-04T15:08:04Z

@andy-armstrong I am taking on the approach you have requested. Please see these are the draft changes but you would be able to understand what I am going to do. This may have some optimisation issues but I will compare them once this new approach is ready.

Please comment/suggest if I am going in wrong direction. Thanks.

mushtaqak · 2017-01-04T15:08:51Z

cms/djangoapps/contentstore/views/item.py

        json: returns representation of the xblock (locator id, data, and metadata).
              if ?fields=graderType, it returns the graderType for the unit instead of the above.
-              if ?course_tree=1, it returns the a course_outline summary and XBlock summary.
+              if ?format=course_tree, it returns a course_outline summary.


I will change this to format=consise

mushtaqak · 2017-01-04T15:10:03Z

cms/djangoapps/contentstore/views/item.py

    """
    Returns the current publish state for the specified xblock and its children
    """
+    child_info = None   # TODO remove when resolved


This method was throwing exception, for now I have bypassed it, but I look at fixing later.

mushtaqak · 2017-01-05T10:54:55Z

cms/djangoapps/contentstore/views/item.py

+                        include_children_predicate=lambda xblock: xblock.has_children,
+                        is_concise=format
+                    )
+                    ancestor_info = _create_xblock_ancestor_info(xblock, course_outline, is_concise=format)


@andy-armstrong Should we handle the course-outline concise=true request in course_handler or xblock_handler view? What do you suggest?

We need the concise course-outline data from backend. We also need parents of the source xblock?

Here is the scenario why we need parents info :

For course outline dialog box, we need to know the parents of the source xblock( the component/item being moved). And then when showing the course-outline tree top-level items, we need to let user know which is the parent element of the source tree item and label it as 'Current Location'. You can see this all in this invisionapp screen.

So, we have two options here.

We return ancestor_info using _create_xblock_ancestor_info from backend. OR

We don't return ancestor info from backend but after receiving the data in JS we do the same kind of work _create_xblock_ancestor_info deos in front-end side to know the parents of the source item.

If we go with the (1) approach, I think, this would be the perfect place to handle the request. And have it's test written in where they are currently.

If we go with the (2) approach, We need to handle this request in course_handler. And have it's test written in TestCourseOutline.

What do you suggest, which approach we move forward with ?

If you think we need further discussion on this, please feel free to comment/HipChat me today, so we can discuss this further.

My recommendation would be to use the course_handler, as that is the natural API for returning the course outline. I don't recommend extending it to support additionally returning ancestor info for a block, as IMO that isn't a natural behavior for the API.

One option would be to pass the current parent down to the client in Mako, because its location is known. Then a new request doesn't need to be issued when moving it. I suppose this leaves a window of error where someone else could move the item but that seems unlikely.

Another option would be to issue two requests: one to get the course outline, and a second one to get the current location. In some ways this is better because the two pieces of information are used by different parts of the UI, so it improves the modularity.

Another option would be to issue two requests: one to get the course outline, and a second one to get the current location. In some ways this is better because the two pieces of information are used by different parts of the UI, so it improves the modularity.

This looks a good option.

But the thing is ancestor_info = _create_xblock_ancestor_info(xblock, course_outline, is_concise=format) requires course_outline. So would we be sending the course_outline back from JS ? I don't think we should do that.

Also, another option that I looked for is that we calculate this in front-end but that would be checking entire course_outline each time a new listing of the tree nodes is shown, it may be a bit expensive when compared to one-time backend call ?

@mushtaqak I did a quick analysis of _create_xblock_ancestor_info and I don't see how it is using the course outline. It is passing it around to various methods but none of them seem to do much with it. You could try passing None and see what happens.

mushtaqak · 2017-01-10T13:50:26Z

@andy-armstrong @muhammad-ammar Please review

andy-armstrong

@mushtaqak thanks for switching the approach as I think it is much clearer now. I have a few minor comments and nits, so let me know when I should take a final look.

andy-armstrong · 2017-01-10T18:35:43Z

cms/djangoapps/contentstore/views/course.py

    """
    Returns a JSON representation of the course module and recursively all of its children.
    """
+    is_concise = True if request.GET.get('format', False) == 'concise' else False


Nit: the inner expression is already a boolean, so this can just be:

is_concise = request.GET.get('format', False) == 'concise'

Nit: it would be clearer if the default were None or '' rather than False, since the result of the get is a string.

andy-armstrong · 2017-01-10T19:29:26Z

cms/djangoapps/contentstore/views/item.py

                if 'graderType' in fields:
                    # right now can't combine output of this w/ output of _get_module_info, but worthy goal
                    return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key))
+                elif info_type == 'ancestor':


Nit: I wonder if you should pass this in fields in the same way as graderType above, rather than introducing a new info_type parameter.

andy-armstrong · 2017-01-10T19:29:56Z

cms/djangoapps/contentstore/views/item.py

    There are three optional boolean parameters:
      include_ancestor_info - if true, ancestor info is added to the response
      include_child_info - if true, direct child info is included in the response
+      is_concise - if true, returns the concise version of xblock info.


Nit: I'd add (default is false).

andy-armstrong · 2017-01-10T19:32:25Z

cms/djangoapps/contentstore/views/item.py

        else:
            xblock_info["staff_only_message"] = False

+    if is_concise:


It looks like most of the code before this is unnecessary as it is building up an xblock_info dict and then replacing it. I think this if should be earlier before any code starts updating the xblock_info.

It might be easier to refactor this method into two or more methods, because the logic is hard to read when it is this long.

mushtaqak · 2017-01-11T14:58:02Z

@andy-armstrong and @muhammad-ammar This is also ready for review :)

@catong Can you please check error messages on this PR ?

andy-armstrong

👍 great stuff, @mushtaqak!

muhammad-ammar

👍

Get ancestor info for the given xblock - TNL-6061

mushtaqak · 2017-01-12T08:16:30Z

jenkins run python

mushtaqak force-pushed the mushtaq/move-course-tree-nodes branch from c50ed61 to a1f03ed Compare December 22, 2016 16:36

mushtaqak changed the title ~~xblock summary data of any given xblock~~ backend view to return tree for course outline and any given block. Dec 26, 2016

mushtaqak requested a review from muhammad-ammar December 26, 2016 13:37

mushtaqak force-pushed the mushtaq/move-course-tree-nodes branch from 92b82f4 to fda38d1 Compare December 27, 2016 07:09

muhammad-ammar reviewed Dec 28, 2016

View reviewed changes

mushtaqak requested a review from andy-armstrong December 28, 2016 17:29

muhammad-ammar reviewed Jan 3, 2017

View reviewed changes

andy-armstrong mentioned this pull request Jan 3, 2017

Add first draft of OEP-13 (API conventions) openedx/openedx-proposals#36

Merged

andy-armstrong suggested changes Jan 3, 2017

View reviewed changes

mushtaqak commented Jan 4, 2017

View reviewed changes

mushtaqak commented Jan 5, 2017

View reviewed changes

andy-armstrong suggested changes Jan 10, 2017

View reviewed changes

andy-armstrong approved these changes Jan 11, 2017

View reviewed changes

muhammad-ammar approved these changes Jan 12, 2017

View reviewed changes

mushtaqak force-pushed the mushtaq/move-course-tree-nodes branch from e924384 to ec875f3 Compare January 12, 2017 07:36

Get concise course outline data for move dialog box

54f962c

Get ancestor info for the given xblock - TNL-6061

mushtaqak force-pushed the mushtaq/move-course-tree-nodes branch from ec875f3 to 54f962c Compare January 12, 2017 07:36

mushtaqak merged commit 4804978 into mushtaq/move-component Jan 12, 2017

mushtaqak deleted the mushtaq/move-course-tree-nodes branch January 12, 2017 11:52

mushtaqak mentioned this pull request Mar 1, 2017

Move a component #14315

Merged



		@ddt.ddt
		class TestMoveItem(ItemTest):



		@ddt.ddt
		class TestMoveItem(ItemTest):

Conversation

mushtaqak commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Reviewers

Post-review

Uh oh!

mushtaqak commented Dec 22, 2016

Uh oh!

muhammad-ammar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushtaqak commented Dec 28, 2016

Uh oh!

andy-armstrong commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mushtaqak commented Dec 28, 2016

Uh oh!

andy-armstrong commented Dec 28, 2016

Uh oh!

mushtaqak commented Dec 29, 2016

Uh oh!

andy-armstrong commented Dec 29, 2016

Uh oh!

mushtaqak commented Dec 29, 2016

Uh oh!

mushtaqak commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushtaqak commented Jan 3, 2017

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushtaqak commented Jan 4, 2017

mushtaqak commented Dec 22, 2016 •

edited

Loading

andy-armstrong commented Dec 28, 2016 •

edited

Loading