2.6.2 Instructor Tool uses the Course Blocks REST API #129

pomegranited · 2016-11-09T02:56:10Z

Moves the logic that populates the Instructor Tool's drop-down list of course blocks out of the server-side and into the client, using the Course Blocks API as the data source.

Discussions: In #119, @ormsbee raised concerns about performance and maintainability of the old server-side mechanism for traversing the course blocks via the runtime object's parents.

Sandbox URL:

LMS: http://pr13953.sandbox.opencraft.hosting (persistent databases)
Studio: http://studio-pr13953.sandbox.opencraft.hosting

Partner information: DavidsonX

Testing instructions:

Ensure your devstack is updated to current master.
Install the previous version of Problem Builder, v2.6.1.
Create a course, and import course.99Ob05.tar.gz.
Visit the Features > Instructor Tool unit, and note the list of course block options available for "Section/Question".
Install this branch's version of Problem Builder.
Visit the Features > Instructor Tool unit, and note that the list of course block options has not changed.

Alternatively, you can compare the sandbox from this PR with the one for v2.6.1, https://github.com/edx/edx-platform/pull/13938.

Author notes and concerns:

This change removes the python tests related to the course block tree creation, since that logic has been removed from the server-side. However, we don't currently have any JS/Jasmine tests for this xblock, and so no JS tests have been added to test the new client-side code.

Reviewers

@itsjeyd
edX reviewer[s] TBD

itsjeyd

@pomegranited These changes look great overall, I just had a few small comments/questions.

I'll test when you're done addressing them and the sandbox is up-to-date.

itsjeyd · 2016-11-09T08:13:17Z

problem_builder/public/js/instructor_tool.js

+        });
+    }
+    function updateBlockOptions(data) {
+


@pomegranited Nit: Move empty line up so it separates getCourseBlocks and updateBlockOptions functions.

Addressed with a3dd42f.

itsjeyd · 2016-11-09T08:17:38Z

problem_builder/slider.py

@@ -172,3 +172,10 @@ def validate_field_data(self, validation, data):
        Validate this block's field data.
        """
        super(SliderBlock, self).validate_field_data(validation, data)
+
+    def student_view_data(self):


@pomegranited Since SliderBlock isn't an eligible block type, I'm wondering why it would need to define student_view_data?

@itsjeyd So my question is, why isn't SliderBlock an eligible block type? And MRQBlock too for that matter? But I didn't have enough time to look into that..

I'm happy to remove this change though.

Reverted with 78f09e6.

@pomegranited

So my question is, why isn't SliderBlock an eligible block type? And MRQBlock too for that matter?

IIRC, we implemented these data exports for Harvard GSE, and at the time they only wanted support for specific question types. MRQ was not one of them, and sliders didn't exist yet :)

itsjeyd · 2016-11-09T08:21:23Z

problem_builder/public/js/instructor_tool.js

+            return $option;
+        },
+        buildTree = function(block, ancestors) {
+


@pomegranited Same nit here: Empty line above start of function definition would be nice :)

Addressed with a3dd42f.

itsjeyd · 2016-11-09T08:34:15Z

problem_builder/public/js/instructor_tool.js

+
+            // Find the best label attribute available for the block
+            labelAttr = _.find(['question', 'name', 'display_name'],
+                               function(attr) {return block[attr]}),


@pomegranited Nit: This would be even more readable when formatted like this:

labelAttr = _.find( ['question', 'name', 'display_name'], function(attr) { return block[attr] } ),

Or like this:

labelAttr = _.find( ['question', 'name', 'display_name'], function(attr) { return block[attr] } ),

Addressed with a3dd42f.

itsjeyd · 2016-11-09T08:37:37Z

problem_builder/public/js/instructor_tool.js

+    }
+    function updateBlockOptions(data) {
+
+        var appendBlock = function(block) {


@pomegranited You already added some nice comments explaining individual steps to the body of this function. It would be great to have a general comment that summarizes what this function does (and why it needs to return $option).

Addressed with a3dd42f.

itsjeyd

Those additional comments look great, @pomegranited! Thanks for adding them. Found a couple typos that will need to be fixed, but other than that the code is good to go.

itsjeyd · 2016-11-09T11:14:18Z

problem_builder/public/js/instructor_tool.js

@@ -253,8 +253,9 @@ function InstructorToolBlock(runtime, element) {
    // Block types with answers we can export
    var questionBlockTypes = ['pb-mcq', 'pb-rating', 'pb-answer'];

+    // Fetch this course's blocks from the REST API, and adds the to the
+    // list of blocks in the Section/Question drop-down list.


@pomegranited Typos: "Fetch this course's blocks ..., and add the ??? to the list of blocks ..." (perhaps you meant to say "and add them"?)

itsjeyd · 2016-11-09T11:16:14Z

problem_builder/public/js/instructor_tool.js

    function updateBlockOptions(data) {

+        // Constructs an <option> element for the given block to the drop-down


@pomegranited "... to add to the drop-down ..."?

itsjeyd · 2016-11-09T17:43:02Z

@pomegranited Thanks for the updates. The code is good to squash. I'll test when the new sandbox is ready.

itsjeyd

@pomegranited This is looking great! I have two more comments based on looking at the recent updates and testing on the sandbox, but they should be quick to address and this good to go once you've done that (I won't need to review again) 👍

I tested this: Course trees are the same on the different sandboxes.
I read through the code
~~I checked for accessibility issues~~ N/A (refactoring)
~~Includes documentation~~ N/A (refactoring)

itsjeyd · 2016-11-10T13:43:14Z

problem_builder/tests/unit/test_instructor_tool.py

-            )
+            patched_loader.render_template.assert_called_once_with('templates/html/instructor_tool.html', {
+                'block_choices': block_choices,
+                'course_blocks_api': '/api/courses/v1/blocks/',


@pomegranited Instead of hard coding the API URL here, you could just import COURSE_BLOCKS_API from problem_builder.instructor_tool.

@itsjeyd I've been smacked on the wrist for doing that before for some reason.. But I totally agree. Fixed here.

And since I squashed the commits, I also added a comment to the COURSE_BLOCKS_API declaration to explain why the trailing slash is important. Seemed excessive to go into detail about the content switch proxy, but at least made a note.

itsjeyd · 2016-11-10T14:01:36Z

problem_builder/public/js/instructor_tool.js

+        // if it's found to have a descendant that is enabled.
+        var appendBlock = function(block) {
+            var blockId = block.id.split('+block@').pop(),
+                padding = Array(block.depth).join('&nbsp;'),


@pomegranited Can you increase the padding to two spaces? That's what the original implementation used (see instructor_tool.html#L35), and I think the tree looks slightly better/more readable with two spaces.

Sorry for not pointing this out sooner, I really had to see the two versions side-by-side to get a sense of the difference.

@itsjeyd Ahh good catch! Sorry I missed that too. Fixed here.

…d drop-down. * Removes the python course tree traversal logic, and replaces it with Javascript. * Adds student_view_data() to downloadable block types to reveal the "question" text to the Course Blocks API

pomegranited changed the title ~~Use the Course Blocks API instead of traversing runtime parent blocks~~ 2.6.2 Use the Course Blocks API instead of traversing runtime parent blocks Nov 9, 2016

pomegranited changed the title ~~2.6.2 Use the Course Blocks API instead of traversing runtime parent blocks~~ 2.6.2 Instructor Tool uses the Course Blocks REST API Nov 9, 2016

pomegranited mentioned this pull request Nov 9, 2016

Bumps problem-builder to v2.6.2 openedx/edx-platform#13953

Closed

2 tasks

pomegranited force-pushed the jill/course_blocks_api branch from 0a0fd92 to b3be88b Compare November 9, 2016 04:25

itsjeyd requested changes Nov 9, 2016

View reviewed changes

pomegranited force-pushed the jill/course_blocks_api branch from f295ec2 to db6b076 Compare November 9, 2016 22:33

itsjeyd approved these changes Nov 10, 2016

View reviewed changes

pomegranited added 2 commits November 11, 2016 09:00

Uses Course Blocks API to populate the Instructor Tool's root_block_i…

9792c93

…d drop-down. * Removes the python course tree traversal logic, and replaces it with Javascript. * Adds student_view_data() to downloadable block types to reveal the "question" text to the Course Blocks API

Bumps version to 2.6.2

0f143d8

pomegranited force-pushed the jill/course_blocks_api branch from 3369ad5 to 0f143d8 Compare November 10, 2016 22:31

pomegranited merged commit 8906d1b into master Nov 10, 2016

pomegranited deleted the jill/course_blocks_api branch November 10, 2016 22:46

itsjeyd mentioned this pull request Nov 24, 2016

Update problem-builder dependency. edx/edx-solutions-edx-platform#757

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.6.2 Instructor Tool uses the Course Blocks REST API #129

2.6.2 Instructor Tool uses the Course Blocks REST API #129

pomegranited commented Nov 9, 2016 •

edited

itsjeyd left a comment

itsjeyd Nov 9, 2016

pomegranited Nov 9, 2016

itsjeyd Nov 9, 2016

pomegranited Nov 9, 2016

pomegranited Nov 9, 2016

itsjeyd Nov 9, 2016

itsjeyd Nov 9, 2016

pomegranited Nov 9, 2016

itsjeyd Nov 9, 2016

pomegranited Nov 9, 2016

itsjeyd Nov 9, 2016

pomegranited Nov 9, 2016

itsjeyd left a comment

itsjeyd Nov 9, 2016

itsjeyd Nov 9, 2016

itsjeyd commented Nov 9, 2016

itsjeyd left a comment

itsjeyd Nov 10, 2016

pomegranited Nov 10, 2016

itsjeyd Nov 10, 2016

pomegranited Nov 10, 2016

		function updateBlockOptions(data) {

		// Constructs an <option> element for the given block to the drop-down

2.6.2 Instructor Tool uses the Course Blocks REST API #129

2.6.2 Instructor Tool uses the Course Blocks REST API #129

Conversation

pomegranited commented Nov 9, 2016 • edited

itsjeyd 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

itsjeyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsjeyd commented Nov 9, 2016

itsjeyd 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

pomegranited commented Nov 9, 2016 •

edited