Instructor Tool XBlock - Polish (OC-751)

[itsjeyd]

This PR renames the "Data Export" XBlock to "Instructor Tool". It also improves and extends existing functionality. It is now possible to:

1. View the data about to be exported directly within the XBlock. Answers are shown in a paginated table that includes the following columns: section name, subsection name, unit name, question type, question, answer, username. The content of exported files matches what is displayed on the page.
2. Filter answers by content. This works for MCQs, rating questions, and long answers.

Also, the UI has been overhauled completely. It now looks like this:

---

Testing

From Studio:

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

From the LMS:

1. Provide answers to MCQ/rating questions/long answers blocks.
2. Switch to instructor tool and click "Search" (optionally using filtering options to narrow down results).

[itsjeyd]

@bradenmacdonald Please review.

The last build has a single test that's failing. I'm not sure why -- I ran the full test suite locally and was unable to reproduce the failure.

CC @antoviaque

[bradenmacdonald]

Can you please leave pb-dashboard in with a comment saying deprecated, so that existing courses will keep working?

[bradenmacdonald]

Is "Student Answers Dashboard" the best name for this? I think it's confusing because there's another "Dashboard" block in this repo already, which also displays a student's answers.

[bradenmacdonald]

object is a confusing name for this parameter - it sounds like this method is expecting an object as its parameter. How about fetchObject or returnObject or wantsObject?

[bradenmacdonald]

FYI, I clicked the button to make Travis run the build again and now it's passing. That error you were seeing is a weird error that occurs randomly in the Travis environment and I haven't yet figured out the cause. Usually you can just run the test again and it will pass.

[bradenmacdonald]

Nit: I think you can combine this with showResults, since there doesn't seem to be any time when you want to force the resultTable to appear before the last_export_result is ready.

[bradenmacdonald]

@itsjeyd Nice work! I couldn't find any bugs and it works very well. Please rebase this code (since there's now a merge conflict) and address the issues I commented on, then ping me for another review :)

[itsjeyd]

@bradenmacdonald Thanks a lot for your comments! The code is ready for another review. For the inline notes, if I didn't reply directly that means I just applied them.

Is "Student Answers Dashboard" the best name for this? I think it's confusing because there's another "Dashboard" block in this repo already, which also displays a student's answers.

I agree that it's a tad confusing, but renaming from "Data Export" to "Student Answers Dashboard" was an explicit requirement for completing OC-751. It probably came directly from the client? In any case, I'm pretty sure it's not my call to change it :)

Maybe @antoviaque can comment on this?

FYI, I clicked the button to make Travis run the build again and now it's passing. [...] Usually you can just run the test again and it will pass.

Did not notice there was a button that lets you restart a build without pushing. Thanks for prompting me to go hunt for that, I know where to look for it now ;)

[bradenmacdonald]

@itsjeyd Nice improvements! This is great. I only have one final comment, which is about fetching the choices above. I believe the choices will always be the same for each block, so we should avoid loading all the choices from the modulestore for each student.

You could:

1. Move the code that looks for choice children up one level to be in _extract_data outside of the per-student loop.
   or
2. Keep it here but cache the result (this is probably more efficient since it avoids looking up answers that won't be needed).

Actually, after looking into this I found out that you might be able to take advantage of QuestionnaireAbstractBlock.get_submission_display and to make this cleaner:

def _get_answer(block, submission, answer_cache):  # Answer cache is a dict that is reset for each block.
    answer = submission['answer']
    if isinstance(block, QuestionnaireAbstractBlock):
        # Convert from answer ID to answer label
        if answer not in answer_cache:
            answer_cache[answer] = block.get_submission_display(answer)
        return answer_cache[answer]
    return answer

[itsjeyd]

@bradenmacdonald Thanks! dae27b3 implements the optimizations you suggest.

[antoviaque]

@itsjeyd @bradenmacdonald It's true that we already have a Dashboard XBlock. Any suggestions on a better name? This now does more than just exporting data, so it would be good to keep reflecting that. There is no precise requirement from the client on this, we can chose what we think is best.

[bradenmacdonald]

@antoviaque @itsjeyd Maybe rename the other dashboard to "Answer Review" or something? Or call this "Instructor Tool" ?

[antoviaque]

@bradenmacdonald Both approaches would work for me - @itsjeyd up to you : )

[itsjeyd]

@antoviaque OK then! :)

My vote is for renaming "Student Answers Dashboard" to "Instructor Tool", as this emphasizes the fact that unlike the other dashboard, the Instructor Tool is only for instructors/not accessible to students.

[itsjeyd]

@bradenmacdonald This is ready for you to have another look. Thanks!

[bradenmacdonald]

Thanks @itsjeyd - looks great! 👍