Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783) #47

itsjeyd · 2015-07-16T20:25:11Z

This PR improves UX for the "Instructor Tool" XBlock by introducing a dropdown menu for constraining the set of exported answers to a specific block in the course outline:

It also enables client-server communication for pagination functionality introduced in #45. This means that instead of transferring all results to the client when the export process finishes and paginating them there, at most PAGE_SIZE items (currently: 15) are transferred at a time. This is not a user-facing change (except for the fact that navigating between result pages takes a little bit longer now that the client has to communicate with the server to obtain more data).

Testing

From Studio:

Add pb-instructor-tool to advanced modules.
Add one or more MCQs/rating questions/long answer blocks to a unit.
Add an instructor tool block to the same or another unit.

From the LMS:

Provide answers to MCQ/rating questions/long answers blocks.
Switch to instructor tool and select a block from the "Section/Question" dropdown.
Click "Search".
Repeat for other blocks.
Optional: For each result set, use pagination controls to navigate between result pages.

itsjeyd · 2015-07-16T23:33:50Z

@e-kolpakov Please review.

e-kolpakov · 2015-07-17T11:04:31Z

problem_builder/public/js/instructor_tool.js

@itsjeyd beforeSend is probably ok to do such type of processing in general, but there's a 3rd party instance that uses problem builders and have global beforeSend adding csrf and authentication data to AJAX requests. So, since ajax call arguments take precedence of global config this would cause that global beforeSend skipped - as a result the requests would fail due to auth or csrf protection.

I might be wrong because I observed that about half a year from now, but judging by up-to-date code it should still behave like that. So I'm suggesting rewriting this block without using beforeSend or making sure global beforeSend is called.

Let me know if you need more info - can't speak openly here as we have a NDA with those 3rd party - and it's better safe than sorry in such case :)

e-kolpakov · 2015-07-17T13:40:56Z

@itsjeyd Instructor Tool is not editable: "Error: Unable to find view u'studio_view' on block InstructorToolBlockWithMixins ...". It just missing studio_view method. If there are no configurable options it should at least show a message that nothing is configurable, not throw exception.

e-kolpakov · 2015-07-17T13:44:01Z

@itsjeyd I'm afraid I can't review anything here yet - I've got the following exception thrown right after I visit the module with Problem Builder and Instructor Tool.

Traceback (most recent call last):
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 109, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/contrib/auth/decorators.py", line 20, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/utils/decorators.py", line 91, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/views/decorators/cache.py", line 75, in _cache_controlled
    response = viewfunc(request, *args, **kw)
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/views.py", line 40, in inner
    response = view_func(request, *args, **kwargs)
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/db.py", line 42, in wrapper
    return func(*args, **kwargs)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/views.py", line 371, in index
    return _index_bulk_op(request, course_key, chapter, section, position)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/views.py", line 558, in _index_bulk_op
    context['fragment'] = section_module.render(STUDENT_VIEW)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/core.py", line 165, in render
    return self.runtime.render(self, view, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1776, in render
    return self.__getattr__('render')(block, view_name, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1231, in render
    return super(MetricsMixin, self).render(block, view_name, context=context)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/runtime.py", line 806, in render
    frag = view_fn(context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/seq_module.py", line 117, in student_view
    rendered_child = child.render(STUDENT_VIEW, context)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/core.py", line 165, in render
    return self.runtime.render(self, view, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1776, in render
    return self.__getattr__('render')(block, view_name, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1231, in render
    return super(MetricsMixin, self).render(block, view_name, context=context)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/runtime.py", line 806, in render
    frag = view_fn(context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/vertical_block.py", line 46, in student_view
    rendered_child = child.render(STUDENT_VIEW, child_context)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/core.py", line 165, in render
    return self.runtime.render(self, view, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1776, in render
    return self.__getattr__('render')(block, view_name, context)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1231, in render
    return super(MetricsMixin, self).render(block, view_name, context=context)
  File "/edx/app/edxapp/venvs/edxapp/src/xblock/xblock/runtime.py", line 806, in render
    frag = view_fn(context)
  File "/edx/xblocks/problem-builder/problem_builder/instructor_tool.py", line 180, in student_view
    build_tree(child_block, [root_entry])
  File "/edx/xblocks/problem-builder/problem_builder/instructor_tool.py", line 143, in build_tree
    block_type = block.runtime.id_reader.get_block_type(block.scope_ids.def_id)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 93, in get_block_type
    return def_id.block_type
AttributeError: 'ObjectId' object has no attribute 'block_type'

e-kolpakov · 2015-07-17T13:46:45Z

problem_builder/instructor_tool.py

@itsjeyd crashes here with the traceback I've sent earlier. Works if replaced to block.scope_ids.usage_id

e-kolpakov · 2015-07-17T13:58:13Z

@itsjeyd actually I kind of sidestepped the issue and got it working, but not committing it since you'll need to verify it anyway. There are other issues though:

"Explain" is a Question field for "Free Answer" block
Blank line below is an MRQ block with non-empty Question, but empty "Question title" field
Two "None" are MRQ Choice blocks - those only have Choice Text field and shouldn'r appear there at all.

So, it is a bit inconsistent (should probably use Title or Question field, whichever is not empty) and includes XBlocks that are essentially answers, not questions

e-kolpakov · 2015-07-17T14:04:24Z

@itsjeyd maybe it's something on my side, but it doesn't show me MRQ question answers at all.

itsjeyd · 2015-07-17T14:07:24Z

@e-kolpakov Thanks for your comments! I'll address them in separate comments.

maybe it's something on my side, but it doesn't show me MRQ question answers at all.

It's not supposed to. Data export only works for MCQs (pb-mcq), "normal" rating questions (pb-rating) and long answers (pb-answers).

e-kolpakov · 2015-07-17T14:07:36Z

@itsjeyd long block names break styling:

e-kolpakov · 2015-07-17T14:12:21Z

@itsjeyd MRQ - ok, acknowledged. Custom choices in MCQ still shown in dropdown, so screeshot is more or less valid :)

itsjeyd · 2015-07-17T14:13:57Z

@e-kolpakov

Instructor Tool is not editable: "Error: Unable to find view u'studio_view' on block InstructorToolBlockWithMixins ...". It just missing studio_view method. If there are no configurable options it should at least show a message that nothing is configurable, not throw exception.

Could you let me know the steps that lead up to this error? Do you get this when you add an Instructor Tool block to a course in studio?

InstructorToolBlock has an author_view method (and has_author_view is set to True). This method returns a simple fragment that says "This block only works from the LMS". That's what I see when I view a unit that has an Instructor Tool block in Studio.

e-kolpakov · 2015-07-17T14:17:20Z

@itsjeyd if you click on "Edit" button for Instructor Tool it will display a popup message with exception. author_view is a preview view displayed in studio instead of student_view. studio_view should contain controls and stuff to edit a block. Most blocks in mentoring use StudioEditableXBlockMixin from xblock-utils that provides default (and pretty good) studio_view.

itsjeyd · 2015-07-17T14:34:01Z

@e-kolpakov

I've got the following exception thrown right after I visit the module with Problem Builder and Instructor Tool.

Traceback (most recent call last):
....
File "/edx/xblocks/problem-builder/problem_builder/instructor_tool.py", line 180, in student_view
build_tree(child_block, [root_entry])
File "/edx/xblocks/problem-builder/problem_builder/instructor_tool.py", line 143, in build_tree
block_type = block.runtime.id_reader.get_block_type(block.scope_ids.def_id)
File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 93, in get_block_type
return def_id.block_type
AttributeError: 'ObjectId' object has no attribute 'block_type'

I can't reproduce this. If I change block.scope_ids.def_id to block.scope_ids.usage_id, I get a bunch of test failures (which I think have something to do with block.scope_ids.usage_id not being available in the workbench runtime).

itsjeyd · 2015-07-17T14:59:37Z

@e-kolpakov

if you click on "Edit" button for Instructor Tool it will display a popup message with exception. author_view is a preview view displayed in studio instead of student_view. studio_view should contain controls and stuff to edit a block. Most blocks in mentoring use StudioEditableXBlockMixin from xblock-utils that provides default (and pretty good) studio_view.

Thanks for these pointers. I tried extending StudioEditableXBlockMixin first but then settled on defining a custom studio_view method in order to be able to display a message explaining that the block isn't editable in the overlay (fbaf5f0).

itsjeyd · 2015-07-17T20:47:04Z

@e-kolpakov Thanks again for your feedback. The latest commits should address all of your comments (I used both my own test course and the course you uploaded to JIRA to check). Please have another look at this PR :)

…ck ID.

results have been inserted into the DOM.

"question" attribute.

For example, ResultView should not manipulate the status area (but it should maintain meta info about result sets).

InstructorToolBlockWithMixins" error that gets thrown when trying to edit Instructor Tool blocks in Studio.

it works for non-eligible blocks with empty "Question title" (display_name).

…RQs).

This ensures that export results correctly reflect changes to answers associated with Long Answer blocks.

itsjeyd · 2015-07-20T21:12:43Z

@e-kolpakov Compatibility issues with latest changes from master have been fixed. Please review :)

antoviaque · 2015-07-21T06:16:42Z

@itsjeyd Thanks for fixing it! Btw, now that I can review on the sandbox, I noticed a couple of issues with the questions titles:

In the drop down showing the list of all sections/questions, when a question doesn't have a title, and instead shows the question id, it seems to be showing the wrong id (cf screenshots below - it shows pb-answer1, instead of test-drive1)

In the result set below and in the CSV, it shows as empty - could you also show test-drive1 there, to be able to tell the answers from each others when there are multiple ones in a given unit?

Otherwise it looks really good, and seem to work well. Good job on this!

set on block.

if "question" attribute is not set to a meaningful value.

itsjeyd · 2015-07-21T09:24:42Z

@antoviaque Thanks! :) I addressed your comments.

CC @e-kolpakov

e-kolpakov · 2015-07-21T10:45:02Z

@itsjeyd one last thing - looks like "Problem Type" malfunction slightly:

If "Rating Question" is selected it shows only rating questions
If "Multiple Choice Question" is selected it shows both MCQ and Rating questions.

Probably it is caused by the fact that Rating questions inherit from MCQs.

e-kolpakov · 2015-07-21T10:49:20Z

problem_builder/instructor_tool.py

@itsjeyd nit: could be more concise and DRY:

for attribute in ('question', 'name', 'display_name'): if getattr('block', attribute, None): return getattr('block', attribute, None) return get_block_id(block)

Not insisting on this though.

e-kolpakov · 2015-07-21T11:03:34Z

@itsjeyd and one more thing - it might make sense to add unit test coverage for all those issues found during the review. So far there's only one new integration test that tests happy-path (which is totally correct), but there're lots of minor things that went wrong, and CI build didn't catch them as there are no tests for them.

antoviaque · 2015-07-21T11:22:20Z

@itsjeyd Thank you for fixing it! Looks much better on the sandbox now : )

itsjeyd · 2015-07-21T11:32:05Z

@e-kolpakov Thanks for the review!

looks like "Problem Type" malfunction slightly:

If "Rating Question" is selected it shows only rating questions

If "Multiple Choice Question" is selected it shows both MCQ and Rating questions.

I was aware of this but I assumed it was a feature, not a bug :) -- the reason being that this is how "Problem types" worked from the beginning, and the task(s) said nothing about changing this behavior. (The only change I made as part of #45 was to turn "Problem types" from a multi-select into a dropdown.) So before I go ahead and work on this I'd like to just confirm with @antoviaque that the current behavior is buggy?

antoviaque · 2015-07-21T11:36:50Z

@itsjeyd That's fine as is - I can see arguments going both ways, so we can leave as is, and address it later in a dedicated task if this proves to be a problem for the clients.

e-kolpakov · 2015-07-21T12:01:20Z

@itsjeyd 👍 than

Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783)

previous PR (#47). The new tests check if: - preferred block attrs are used when building block tree for current course - block tree excludes pb-choice blocks - blocks are correctly marked as (in)eligible - new-style keys are handled correctly - student_view passes calls rendering method with correct template args - author_view and studio_view show appropriate messages

bradenmacdonald · 2015-08-20T23:21:06Z

@itsjeyd FYI, this PR seems to be causing me trouble as I'm reviewing #57 for you. When I try to view my existing Instructor Tool block, it tries to load my existing results but fails - it just shows a spinner and hangs forever.

The browser console shows an AJAX error because the get_result_page is returning a 500 Internal Server Error. The LMS console shows this error:

...
  File "/edx/xblocks/xblock-problem-builder/problem_builder/instructor_tool.py", line 127, in get_result_page
    'display_data': paginator.page(page).object_list,
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/paginator.py", line 37, in page
    number = self.validate_number(number)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/paginator.py", line 28, in validate_number
    if number > self.num_pages:
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/paginator.py", line 60, in _get_num_pages
    if self.count == 0 and not self.allow_empty_first_page:
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/paginator.py", line 53, in _get_count
    self._count = len(self.object_list)
TypeError: object of type 'NoneType' has no len(

Eventually I figured out why: I have an existing result in last_export_result but the new display_data field is None. I fixed it by pressing the "delete results" button, and then it worked fine after that.

This is a pretty minor bug, and you likely don't need to fix it, but I wanted to record this here in case any one else runs into it.

e-kolpakov reviewed Jul 17, 2015
View reviewed changes

itsjeyd added 15 commits July 20, 2015 16:59

Catch IndexError that gets thrown when user provides invalid root blo…

5be4fa3

…ck ID.

Implement client-server communication for pagination.

3fc5716

Turn "Root block ID" field into user-friendly dropdown.

45bb18a

Make sure spinner stays visible and result table stays hidden until

9b2a4a1

results have been inserted into the DOM.

Make dropdown work in workbench.

6fcbba2

Fix failing tests.

75994ad

Default to question ID (name) if question block does not have a

6e6f31c

"question" attribute.

Refactor to create better separation of concerns in client-side code.

a2eb5c7

For example, ResultView should not manipulate the status area (but it should maintain meta info about result sets).

Add integration test for deleting export results.

466b172

Make sure not to override global beforeSend handler.

5822565

Get rid of "Unable to find view u'studio_view' on block

74e027c

InstructorToolBlockWithMixins" error that gets thrown when trying to edit Instructor Tool blocks in Studio.

Move code that computes block ID into separate method.

a689bd2

Take into account different key styles when computing block_type.

69b0a80

Move code that computes names of blocks to separate method and make sure

4cc0b2e

it works for non-eligible blocks with empty "Question title" (display_name).

Make sure dropdown excludes all choice fields (not just children of M…

a856c87

…RQs).

itsjeyd added 3 commits July 20, 2015 16:59

Make sure long block names can't break layout of export options.

418c55f

Handle cases where "Question title" is set but "question" isn't.

435382f

Look up "Long Answer" submissions by question ID instead of block ID.

51d02cf

This ensures that export results correctly reflect changes to answers associated with Long Answer blocks.

itsjeyd force-pushed the dropdown-pagination branch from 46dab9e to 51d02cf Compare July 20, 2015 20:40

itsjeyd added 2 commits July 21, 2015 09:43

Store question ID in "Question" column if "question" attribute is not

bb2ea26

set on block.

In dropdown listing blocks, show question ID (name) instead of block ID

e0fbd14

if "question" attribute is not set to a meaningful value.

e-kolpakov reviewed Jul 21, 2015
View reviewed changes

Address review comments.

c6ab809

itsjeyd added a commit that referenced this pull request Jul 21, 2015

Merge pull request #47 from open-craft/dropdown-pagination

7da902e

Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783)

itsjeyd merged commit 7da902e into master Jul 21, 2015

itsjeyd mentioned this pull request Jul 22, 2015

Add unit tests for instructor tool #50

Merged

itsjeyd mentioned this pull request Aug 9, 2016

Problem Builder - Review for release on edx.org #119

Merged

3 tasks

xitij2000 deleted the dropdown-pagination branch December 13, 2019 06:42

Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783) #47

Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783) #47

Uh oh!

Conversation

itsjeyd commented Jul 16, 2015

Testing

Uh oh!

itsjeyd commented Jul 16, 2015

Uh oh!

e-kolpakov Jul 17, 2015

Choose a reason for hiding this comment

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

e-kolpakov Jul 17, 2015

Choose a reason for hiding this comment

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 17, 2015

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 17, 2015

Uh oh!

e-kolpakov commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 17, 2015

Uh oh!

itsjeyd commented Jul 20, 2015

Uh oh!

antoviaque commented Jul 21, 2015

Uh oh!

itsjeyd commented Jul 21, 2015

Uh oh!

e-kolpakov commented Jul 21, 2015

Uh oh!

e-kolpakov Jul 21, 2015

Choose a reason for hiding this comment

Uh oh!

e-kolpakov commented Jul 21, 2015

Uh oh!

antoviaque commented Jul 21, 2015

Uh oh!

itsjeyd commented Jul 21, 2015

Uh oh!

antoviaque commented Jul 21, 2015

Uh oh!

e-kolpakov commented Jul 21, 2015

Uh oh!

bradenmacdonald commented Aug 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants