Allow move for content experiment by mushtaqak · Pull Request #14457 · openedx/openedx-platform

mushtaqak · 2017-02-02T16:12:41Z

This PR adds ability to:

Move Content Experiment in other unit.
Move children of content experiment to other unit as well as other Content Experiment.
Don't show move icon on library page

Sandbox Link

@muhammad-ammar @muzaffaryousaf Please review.

mushtaqak · 2017-02-03T13:16:34Z

jenkins run python

muzaffaryousaf · 2017-02-05T13:55:12Z

@mushtaqak Fix the tests first and build a sandbox to look at.

muzaffaryousaf · 2017-02-05T13:54:38Z

cms/djangoapps/contentstore/views/preview.py

            'can_edit': context.get('can_edit', True),
            'can_edit_visibility': context.get('can_edit_visibility', True),
            'can_add': context.get('can_add', True),
+            'can_move': context.get('is_unit_page', False),  # move action is not allowed other than unit page


Where we're passing True for unit page ?

When a page is unit page, it has True set to is_unit_page field of the xblock_info. So context.get('is_unit_page', False) would either give True (if unit page) or False

muzaffaryousaf · 2017-02-06T09:56:21Z

@mushtaqak I'm not able to see "move" icon for Content Experiment.

muzaffaryousaf · 2017-02-07T15:31:27Z

@mushtaqak I'm not able to move the component in a unit which does't have children.

mushtaqak · 2017-02-07T16:32:01Z

@mushtaqak I'm not able to move the component in a unit which does't have children.

Oh I will fix that tomorrow :)

mushtaqak · 2017-02-07T17:34:16Z

@muzaffaryousaf is it working now for emptyunit?

muzaffaryousaf · 2017-02-07T19:03:23Z

have you updated sandbox ?

muzaffaryousaf · 2017-02-07T19:03:56Z

looks like it is.

muzaffaryousaf · 2017-02-09T14:58:00Z

@sstack22 We've added capability to move an content experiment across the units. Would you please give it a quick look. Here is link to sandbox https://studio-moveaction.sandbox.edx.org/course/course-v1:edX+DemoX+Demo_Course which includes everything that we've done so far related to move xblock feature.

@catong FYI ^^

NOTE:
You maybe wondering why we implemented this instead of just hiding the move icon from content experiment, initially that was our plan but it turned out implementing is simpler than hiding the icon.

muzaffaryousaf · 2017-02-09T15:00:25Z

@andy-armstrong Based on your previous feedback, we've added the ability to move content experiment. It would be nice if you can take a look too. Thanks.

mushtaqak · 2017-02-10T13:54:21Z

@sstack22 @andy-armstrong please have a look at the sandbox :)

andy-armstrong

👍 looks great!

One minor nit: the groups are labelled with their internal ids, rather than with the group name.

catong · 2017-02-10T14:36:11Z

Thanks @mushtaqak. It looks great - I'll update the doc PR that is in progress to reflect this.

muzaffaryousaf · 2017-02-14T07:55:34Z

@mushtaqak When i add new component on library page, i'm able to see move icon for the first time.

muzaffaryousaf · 2017-02-14T08:43:55Z

@mushtaqak Have you tried conditional module ?

muzaffaryousaf · 2017-02-14T08:40:30Z

cms/templates/js/move-xblock-list.underscore

-                    <%- xblock.get('display_name') %>
-                </span>
-            <% } else { %>
+            <% if (xblock.get('child_info') || XBlocksCategory !== 'component') { %>


I'd explicitly check if xblock.get('child_info') is not undefined or if it's a list then i'd check len(xblock.get('child_info')) > 0.

muzaffaryousaf · 2017-02-14T09:45:12Z

cms/static/js/views/modals/move_xblock_modal.js

            }
            this.isValidMove = isValidMove || false;
        },
+        validCategoryCheck: function(targetParentType, sourceParentType) {


Can we rename it to isCategoryValid / isValidCategory

muzaffaryousaf · 2017-02-14T09:46:28Z

cms/static/js/views/modals/move_xblock_modal.js

        },
+        validCategoryCheck: function(targetParentType, sourceParentType) {
+            if (!_.contains(['course, chapter, sequential, vertical'], sourceParentType)) {
+                sourceParentType = 'vertical';  // eslint-disable-line no-param-reassign


Is this for children of split_test ?

@mushtaqak a small code comment about why we're doing this would be helpful for others.

muzaffaryousaf · 2017-02-14T09:48:45Z

cms/static/js/views/modals/move_xblock_modal.js

            }
            this.isValidMove = isValidMove || false;
        },
+        validCategoryCheck: function(targetParentType, sourceParentType) {


Let's leave a blank line before this method.

muzaffaryousaf · 2017-02-14T10:03:36Z

cms/static/js/views/move_xblock_list.js

                this.visitedAncestors[this.visitedAncestors.length - 2]
            );
-            this.childrenInfo.category = this.categoryRelationMap[this.parentInfo.category];
+            this.childrenInfo.category = this.categoryRelationMap[this.parentInfo.category] || 'component';


Are we missing any other parent category ? I'm asking because of explicit 'component'.

I think we should avoid || 'component' and add category to map, this will make the code more readable.

muzaffaryousaf · 2017-02-14T10:52:53Z

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

-        drag_handle_html = '<span data-tooltip="Drag to reorder" class="drag-handle action"></span>'
-        self.assertIn(drag_handle_html, html)
+        self.validate_html_for_reorder(html, can_reorder)
+        self.validate_html_for_move_button(html, can_move)


@mushtaqak I think can_move will never get called with False, can you double check that. If that's the case i'd like to see coverage for this scenario too.

Nvm, i just saw the test for library.

catong · 2017-02-23T15:06:17Z

@mushtaqak @sstack22 To summarize, we are supporting moving units from content experiments, but we are NOT supporting moving units out of a content library (Move icon not available). Is this right?

mushtaqak · 2017-02-23T18:10:43Z

@catong Yes you are right

mushtaqak · 2017-02-24T06:32:59Z

@catong Just to let you know that component can not be moved inside itself or inside it's children components.

mushtaqak · 2017-02-24T07:12:51Z

@muhammad-ammar PR is ready for second review, sandbox is updated :)

muzaffaryousaf · 2017-02-24T09:54:14Z

cms/djangoapps/contentstore/views/item.py

+        source_item (XBlock): Source xblock.
+        target_parent (XBlock): Target XBlock.
+    """
+    target_ancestors = _create_xblock_ancestor_info(target_parent, is_concise=True)['ancestors']


Is there any chance of no ancestor info for any target_parent ?

hmm, if the target item is course xblock may be.

Confirm this and see how we can avoid such usages which can break the code in some unseen scenario.

Even if the target_parent is a course xblock or (having no parent), this _create_xblock_ancestor_info returns {'ancestors': []} which wouldn't break the code.

However, this can break if target_parent is None which would break already built method _create_xblock_ancestor_info because the None xblock wouldn't have any location or any other xblock related fields. But, I think, we are fin in this case, we would want the code to break if None is passed on (which in our scenario would never be the case).

muzaffaryousaf · 2017-02-24T09:56:05Z

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

+        """
+        Test that a component can not be moved to itself.
+        """
+        lc_block = self.create_xblock(


lc_block --> library_block or library_content

muzaffaryousaf · 2017-02-24T10:05:01Z

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

        self.assertEqual(vertical_0.location, split_test.group_id_to_child['0'])
        self.assertEqual(vertical_1.location, split_test.group_id_to_child['1'])

+    def test_split_xblock_info_group_name(self):


@mushtaqak I think we should move this test with utils test cases where we moved the logic.

It's a lot easier to place it where without any repetition. I can change to test_utils if you want.

muhammad-ammar · 2017-02-24T11:04:22Z

cms/static/js/views/move_xblock_list.js

+                }
+                this.categoryRelationMap[this.parentInfo.category] = childCategory;
+            }
+            this.parentInfo.category = this.parentInfo.category;


@mushtaqak i think we left this mistakenly? assigning to itself?

oh, my bad, nice catch!

muzaffaryousaf · 2017-02-24T11:16:34Z

@mushtaqak The overall functionality looks good to me but i'll give final review soon. Thanks for all the good work.

mushtaqak · 2017-02-24T11:27:58Z

jenkins run lettuce

muzaffaryousaf · 2017-02-27T06:53:01Z

cms/djangoapps/contentstore/views/item.py

+    Returns True if source item is found in target parents otherwise False.
+
+    Arguments:
+        source_item (XBlock): Source xblock.


xblock --> Xblock

muzaffaryousaf · 2017-02-27T09:37:13Z

cms/templates/js/move-xblock-list.underscore

+                    </span>
+                    <% if(currentLocationIndex === i) { %>
+                        <span class="current-location">
+                            (<%- gettext('Current location') %>)


I think we should re-word this to Currently Selected

@catong or @srpearce Please suggest appropriate text here. This text will be shown with selected components e.g. video, html, discussion etc.

@muzaffaryousaf I can take a look; I'll be finishing up this Move feature for doc.

@muzaffaryousaf Agreed that at the level of the item that we are actually moving the gettext should change to "Currently selected"

muzaffaryousaf · 2017-02-27T09:45:56Z

cms/static/js/spec/views/move_xblock_spec.js

+                            has_children: true,
+                            id: 'SPLIT_TEST_ID'
+                        },
+                        outlineOptions = {split_test: 1, unit: 2, component: 1};


how this will add groups in content experiment.

actually group in content experiments are verticals/units, so this will add slit test and 2 units and a component in group 0

muzaffaryousaf · 2017-02-27T09:49:33Z

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

+        parent_vert_usage_key = self._create_vertical(parent_usage_key=self.vert_usage_key)
+        child1_vert_usage_key = self._create_vertical(parent_usage_key=parent_vert_usage_key)
+        child2_vert_usage_key = self._create_vertical(parent_usage_key=child1_vert_usage_key)
+        child3_vert_usage_key = self._create_vertical(parent_usage_key=child2_vert_usage_key)


I would use some real scenario like content experiment.

muzaffaryousaf · 2017-02-27T09:50:43Z

@mushtaqak i'm done with final review and left couple of NITS. 👍

catong · 2017-02-27T15:20:01Z

cms/templates/js/move-xblock-list.underscore

                    <span class="icon fa fa-arrow-right forward-sr-icon" aria-hidden="true"></span>
                    <span class="sr forward-sr-text"><%- gettext("Click for children") %></span>
                </button>
+            <% } else { %>


I thought that we had previously changed "Click for children" to something else? Did it get lost in the previous PR?

"View child components" (5 syllables) is still the best option IMO, perhaps "View child items" to lose one syllable. @cptvitamin?

catong · 2017-02-27T15:30:06Z

cms/templates/js/move-xblock-list.underscore

+                    </span>
+                    <% if(currentLocationIndex === i) { %>
+                        <span class="current-location">
+                            (<%- gettext('Current location') %>)


@muzaffaryousaf Agreed that at the level of the item that we are actually moving the gettext should change to "Currently selected"

mushtaqak · 2017-03-01T13:19:18Z

jenkins run bokchoy

bokchoy tests for move xblocks

muzaffaryousaf · 2017-03-01T18:15:40Z

Awesome!!!

cahrens · 2017-03-16T16:18:03Z

cms/djangoapps/contentstore/utils.py

    return reverse_url(handler_name, 'usage_key_string', usage_key, kwargs)


+def get_group_display_name(user_partitions, xblock_display_name):


@mushtaqak @muzaffaryousaf This is causing a problem for me on a PR I'm working on, and it would appear to be a general bug due to the implementation below. group['id'] is an integer; it can be any integer. xblock_display_name can contain any text. It is quite possible that a xblock's display name contains the integer of a group['id'], without them being related in any way.

As a concrete example, I am adding a new UserPartition based on enrollment track. The group IDs related to this partition will be small number (1, 2, 3, etc.). It's quite possible that an xblock's display name will contain those integers, and in fact, I hit failing tests that pointed out this issue (thank goodness for tests!).

Can you guys look into this tomorrow? I won't be able to merge my PR (#14682) until this is changed. For tracking purposes, I have created https://openedx.atlassian.net/browse/TNL-6731.

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 9530222 to 134bb84 Compare February 3, 2017 12:12

muzaffaryousaf suggested changes Feb 5, 2017

View reviewed changes

mushtaqak force-pushed the mushtaq/tnl-6062-save-move-xblock branch from 2149952 to 486bcde Compare February 7, 2017 10:36

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 0027b5d to 798f5f2 Compare February 7, 2017 14:10

mushtaqak force-pushed the mushtaq/tnl-6062-save-move-xblock branch from 486bcde to 99a3416 Compare February 7, 2017 16:25

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 2ef7f96 to 9bd7b72 Compare February 7, 2017 16:44

mushtaqak force-pushed the mushtaq/tnl-6062-save-move-xblock branch 2 times, most recently from c4c0d45 to cda669e Compare February 9, 2017 08:49

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 9bd7b72 to f99b3f6 Compare February 9, 2017 10:17

muzaffaryousaf requested review from andy-armstrong, cptvitamin, muhammad-ammar and sstack22 February 9, 2017 15:08

muzaffaryousaf changed the title ~~Restrict move action~~ Allow move for content experiment Feb 9, 2017

andy-armstrong approved these changes Feb 10, 2017

View reviewed changes

muzaffaryousaf suggested changes Feb 14, 2017

View reviewed changes

muzaffaryousaf reviewed Feb 24, 2017

View reviewed changes

muhammad-ammar reviewed Feb 24, 2017

View reviewed changes

muzaffaryousaf reviewed Feb 27, 2017

View reviewed changes

muzaffaryousaf approved these changes Feb 27, 2017

View reviewed changes

catong reviewed Feb 27, 2017

View reviewed changes

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 965c31f to 93439b2 Compare March 1, 2017 14:03

mushtaqak force-pushed the mushtaq/move-component branch from a12ee2d to e9b8e17 Compare March 1, 2017 14:28

restrict move action

485ffb1

mushtaqak force-pushed the mushtaq/restrict-move-action branch from 93439b2 to 485ffb1 Compare March 1, 2017 14:30

muhammad-ammar and others added 3 commits March 1, 2017 19:31

bokchoy tests for move xblocks

8a1ec3e

Add content experiment test

83de1eb

Merge pull request #14465 from edx/ammar/bokchoy-tests

0df3624

bokchoy tests for move xblocks

mushtaqak merged commit de29ef9 into mushtaq/move-component Mar 1, 2017

mushtaqak mentioned this pull request Mar 1, 2017

Move a component #14315

Merged

cahrens reviewed Mar 16, 2017

View reviewed changes

mushtaqak deleted the mushtaq/restrict-move-action branch May 10, 2017 08:02

		return reverse_url(handler_name, 'usage_key_string', usage_key, kwargs)


		def get_group_display_name(user_partitions, xblock_display_name):

Conversation

mushtaqak commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mushtaqak commented Feb 3, 2017

Uh oh!

muzaffaryousaf commented Feb 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muzaffaryousaf commented Feb 6, 2017

Uh oh!

muzaffaryousaf commented Feb 7, 2017

Uh oh!

mushtaqak commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mushtaqak commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muzaffaryousaf commented Feb 7, 2017

Uh oh!

muzaffaryousaf commented Feb 7, 2017

Uh oh!

muzaffaryousaf commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muzaffaryousaf commented Feb 9, 2017

Uh oh!

mushtaqak commented Feb 10, 2017

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

catong commented Feb 10, 2017

Uh oh!

muzaffaryousaf commented Feb 14, 2017

Uh oh!

muzaffaryousaf commented Feb 14, 2017

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!

catong commented Feb 23, 2017

Uh oh!

mushtaqak commented Feb 23, 2017

Uh oh!

mushtaqak commented Feb 24, 2017

Uh oh!

mushtaqak commented Feb 24, 2017

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 Feb 2, 2017 •

edited

Loading

mushtaqak commented Feb 7, 2017 •

edited

Loading

mushtaqak commented Feb 7, 2017 •

edited

Loading

muzaffaryousaf commented Feb 9, 2017 •

edited

Loading