Merge pull request #102 from open-craft/hide-previous-answer

Problem Builder: Add instance-wide option to hide previous answers for MCQs

problem_builder/mentoring.py

@@ -42,6 +42,7 @@ from .step_review import ReviewStepBlock
 
 from xblockutils.helpers import child_isinstance
 from xblockutils.resources import ResourceLoader
+from xblockutils.settings import XBlockWithSettingsMixin, ThemableXBlockMixin
 from xblockutils.studio_editable import (
     NestedXBlockSpec, StudioEditableXBlockMixin, StudioContainerXBlockMixin, StudioContainerWithNestedXBlocksMixin,
 )
@@ -63,6 +64,10 @@ _default_theme_config = {
     'locations': ['public/themes/lms.css']
 }
 
+_default_options_config = {
+    'pb_mcq_hide_previous_answer': False
+}
+
 
 # Make '_' a no-op so we can scrape strings
 def _(text):
@@ -80,7 +85,8 @@ PARTIAL = 'partial'
 @XBlock.needs("i18n")
 @XBlock.wants('settings')
 class BaseMentoringBlock(
-        XBlock, XBlockWithTranslationServiceMixin, StudioEditableXBlockMixin, MessageParentMixin
+    XBlock, XBlockWithTranslationServiceMixin, XBlockWithSettingsMixin,
+    StudioEditableXBlockMixin, ThemableXBlockMixin, MessageParentMixin,
 ):
     """
     An XBlock that defines functionality shared by mentoring blocks.
@@ -121,6 +127,7 @@ class BaseMentoringBlock(
     icon_class = 'problem'
     block_settings_key = 'mentoring'
     theme_key = 'theme'
+    options_key = 'options'
 
     @property
     def url_name(self):
@@ -156,24 +163,23 @@ class BaseMentoringBlock(
             return [self.display_name]
         return []
 
-    def get_theme(self):
+    def get_options(self):
+        """
+        Get options settings for this block from settings service.
+
+        Fall back on default options if xblock settings have not been customized at all
+        or no customizations for options available.
         """
-        Gets theme settings from settings service. Falls back to default (LMS) theme
-        if settings service is not available, xblock theme settings are not set or does
-        contain mentoring theme settings.
+        xblock_settings = self.get_xblock_settings(_default_options_config)
+        if xblock_settings and self.options_key in xblock_settings:
+            return xblock_settings[self.options_key]
+        return _default_options_config
+
+    def get_option(self, option):
+        """
+        Get value of a specific instance-wide `option`.
         """
-        settings_service = self.runtime.service(self, "settings")
-        if settings_service:
-            xblock_settings = settings_service.get_settings_bucket(self)
-            if xblock_settings and self.theme_key in xblock_settings:
-                return xblock_settings[self.theme_key]
-        return _default_theme_config
-
-    def include_theme_files(self, fragment):
-        theme = self.get_theme()
-        theme_package, theme_files = theme['package'], theme['locations']
-        for theme_file in theme_files:
-            fragment.add_css(ResourceLoader(theme_package).load_unicode(theme_file))
+        return self.get_options()[option]
 
     @XBlock.json_handler
     def view(self, data, suffix=''):
@@ -368,6 +374,8 @@ class MentoringBlock(BaseMentoringBlock, StudioContainerXBlockMixin, StepParentM
         return Score(score, int(round(score * 100)), correct, incorrect, partially_correct)
 
     def student_view(self, context):
+        from .mcq import MCQBlock  # Import here to avoid circular dependency
+
         # Migrate stored data if necessary
         self.migrate_fields()
 
@@ -379,6 +387,8 @@ class MentoringBlock(BaseMentoringBlock, StudioContainerXBlockMixin, StepParentM
         fragment = Fragment()
         child_content = u""
 
+        mcq_hide_previous_answer = self.get_option('pb_mcq_hide_previous_answer')
+
         for child_id in self.children:
             child = self.runtime.get_block(child_id)
             if child is None:  # child should not be None but it can happen due to bugs or permission issues
@@ -388,6 +398,10 @@ class MentoringBlock(BaseMentoringBlock, StudioContainerXBlockMixin, StepParentM
                     if self.is_assessment and isinstance(child, QuestionMixin):
                         child_fragment = child.render('assessment_step_view', context)
                     else:
+                        if mcq_hide_previous_answer and isinstance(child, MCQBlock):
+                            context['hide_prev_answer'] = True
+                        else:
+                            context['hide_prev_answer'] = False
                         child_fragment = child.render('mentoring_view', context)
                 except NoSuchViewError:
                     if child.scope_ids.block_type == 'html' and getattr(self.runtime, 'is_author_mode', False):

problem_builder/public/js/mentoring.js

@@ -8,7 +8,9 @@ function MentoringBlock(runtime, element) {
     var attemptsTemplate = _.template($('#xblock-attempts-template').html());
     var data = $('.mentoring', element).data();
     var children = runtime.children(element);
-    var steps = runtime.children(element).filter(function(c) { return c.element.className.indexOf('assessment_step_view') > -1; });
+    var steps = runtime.children(element).filter(function(c) {
+        return $(c.element).attr("class").indexOf('assessment_step_view') > -1;
+    });
     var step = data.step;
 
     var mentoring = {

problem_builder/public/js/questionnaire.js

@@ -122,7 +122,7 @@ function MCQBlock(runtime, element) {
             var mentoring = this.mentoring;
 
             var messageView = MessageView(element, mentoring);
-            
+
             if (result.message) {
                 var msg = '<div class="message-content">' + result.message + '</div>' +
                           '<div class="close icon-remove-sign fa-times-circle"></div>';
@@ -138,24 +138,27 @@ function MCQBlock(runtime, element) {
                 var choiceResultDOM = $('.choice-result', choiceDOM);
                 var choiceTipsDOM = $('.choice-tips', choiceDOM);
 
-                if (result.status === "correct" && choiceInputDOM.val() === result.submission) {
-                    choiceDOM.addClass('correct');
-                    choiceResultDOM.addClass('checkmark-correct icon-ok fa-check');
-                }
-                else if (choiceInputDOM.val() === result.submission || _.isNull(result.submission)) {
-                    choiceDOM.addClass('incorrect');
-                    choiceResultDOM.addClass('checkmark-incorrect icon-exclamation fa-exclamation');
-                }
-
-                if (result.tips && choiceInputDOM.val() === result.submission) {
-                    mentoring.setContent(choiceTipsDOM, result.tips);
-                }
+                if (choiceInputDOM.prop('checked')) {  // We're showing previous answers,
+                                                       // so go ahead and display results as well
+                    if (result.status === "correct" && choiceInputDOM.val() === result.submission) {
+                        choiceDOM.addClass('correct');
+                        choiceResultDOM.addClass('checkmark-correct icon-ok fa-check');
+                    }
+                    else if (choiceInputDOM.val() === result.submission || _.isNull(result.submission)) {
+                        choiceDOM.addClass('incorrect');
+                        choiceResultDOM.addClass('checkmark-incorrect icon-exclamation fa-exclamation');
+                    }
 
-                choiceResultDOM.off('click').on('click', function() {
-                    if (choiceTipsDOM.html() !== '') {
-                        messageView.showMessage(choiceTipsDOM);
+                    if (result.tips && choiceInputDOM.val() === result.submission) {
+                        mentoring.setContent(choiceTipsDOM, result.tips);
                     }
-                });
+
+                    choiceResultDOM.off('click').on('click', function() {
+                        if (choiceTipsDOM.html() !== '') {
+                            messageView.showMessage(choiceTipsDOM);
+                        }
+                    });
+                }
             });
 
             if (_.isNull(result.submission)) {

problem_builder/tests/integration/test_mentoring.py

@@ -131,6 +131,11 @@ class ProblemBuilderQuestionnaireBlockTest(ProblemBuilderBaseTest):
         self.assertNotIn('checkmark-correct', choice_result_classes)
         self.assertNotIn('checkmark-incorrect', choice_result_classes)
 
+    def _assert_not_checked(self, questionnaire, choice_index):
+        choice = self._get_choice(questionnaire, choice_index)
+        choice_input = choice.find_element_by_css_selector('input')
+        self.assertFalse(choice_input.is_selected())
+
     def _standard_filling(self, answer, mcq, mrq, rating):
         answer.send_keys('This is the answer')
         self.click_choice(mcq, "Yes")
@@ -164,6 +169,32 @@ class ProblemBuilderQuestionnaireBlockTest(ProblemBuilderBaseTest):
         self.assertTrue(messages.is_displayed())
         self.assertEqual(messages.text, "FEEDBACK\nNot done yet")
 
+    def _feedback_customized_checks(self, answer, mcq, mrq, rating, messages):
+        # Long answer: Previous answer and feedback visible
+        self.assertEqual(answer.get_attribute('value'), 'This is the answer')
+        # MCQ: Previous answer and feedback hidden
+        for i in range(3):
+            self._assert_feedback_hidden(mcq, i)
+            self._assert_not_checked(mcq, i)
+        # MRQ: Previous answer and feedback visible
+        self._assert_feedback_showed(
+            mrq, 0, "This is something everyone has to like about this MRQ",
+            click_choice_result=True
+        )
+        self._assert_feedback_showed(
+            mrq, 1, "This is something everyone has to like about beauty",
+            click_choice_result=True, success=False
+        )
+        self._assert_feedback_showed(mrq, 2, "This MRQ is indeed very graceful", click_choice_result=True)
+        self._assert_feedback_showed(mrq, 3, "Nah, there aren't any!", click_choice_result=True, success=False)
+        # Rating: Previous answer and feedback hidden
+        for i in range(5):
+            self._assert_feedback_hidden(rating, i)
+            self._assert_not_checked(rating, i)
+        # Messages
+        self.assertTrue(messages.is_displayed())
+        self.assertEqual(messages.text, "FEEDBACK\nNot done yet")
+
     def reload_student_view(self):
         # Load another page (the home page), then go back to the page we want. This is the only reliable way to reload.
         self.browser.get(self.live_server_url + '/')
@@ -218,6 +249,32 @@ class ProblemBuilderQuestionnaireBlockTest(ProblemBuilderBaseTest):
         self.click_choice(mrq, "Its elegance")
         self.assertTrue(submit.is_enabled())
 
+    def test_does_not_persist_mcq_feedback_on_page_reload_if_disabled(self):
+        with mock.patch("problem_builder.mentoring.MentoringBlock.get_options") as patched_options:
+            patched_options.return_value = {'pb_mcq_hide_previous_answer': True}
+            mentoring = self.load_scenario("feedback_persistence.xml")
+            answer, mcq, mrq, rating = self._get_controls(mentoring)
+            messages = self._get_messages_element(mentoring)
+
+            self._standard_filling(answer, mcq, mrq, rating)
+            self.click_submit(mentoring)
+            self._standard_checks(answer, mcq, mrq, rating, messages)
+
+            # now, reload the page and see if previous answers and results for MCQs are hidden
+            mentoring = self.reload_student_view()
+            answer, mcq, mrq, rating = self._get_controls(mentoring)
+            messages = self._get_messages_element(mentoring)
+            submit = mentoring.find_element_by_css_selector('.submit input.input-main')
+
+            self._feedback_customized_checks(answer, mcq, mrq, rating, messages)
+            # after reloading submit is disabled...
+            self.assertFalse(submit.is_enabled())
+
+            # ... until student answers MCQs again
+            self.click_choice(mcq, "Maybe not")
+            self.click_choice(rating, "2")
+            self.assertTrue(submit.is_enabled())
+
     def test_given_perfect_score_in_past_loads_current_result(self):
         mentoring = self.load_scenario("feedback_persistence.xml")
         answer, mcq, mrq, rating = self._get_controls(mentoring)

problem_builder/tests/integration/test_titles.py

@@ -74,7 +74,7 @@ class StepTitlesTest(SeleniumXBlockTest):
     mcq_template = """
         <problem-builder mode="{{mode}}">
             <pb-mcq name="mcq_1_1" question="Who was your favorite character?"
-              correct_choices="gaius,adama,starbuck,roslin,six,lee"
+              correct_choices="[gaius,adama,starbuck,roslin,six,lee]"
               {display_name_attr} {show_title_attr}
             >
                 <pb-choice value="gaius">Gaius Baltar</pb-choice>
@@ -90,7 +90,7 @@ class StepTitlesTest(SeleniumXBlockTest):
     mrq_template = """
         <problem-builder mode="{{mode}}">
             <pb-mrq name="mrq_1_1" question="What makes a great MRQ?"
-              ignored_choices="1,2,3"
+              ignored_choices="[1,2,3]"
               {display_name_attr} {show_title_attr}
             >
                 <pb-choice value="1">Lots of choices</pb-choice>
@@ -103,7 +103,7 @@ class StepTitlesTest(SeleniumXBlockTest):
     rating_template = """
         <problem-builder mode="{{mode}}">
             <pb-rating name="rating_1_1" question="How do you rate Battlestar Galactica?"
-              correct_choices="5,6"
+              correct_choices="[5,6]"
               {display_name_attr} {show_title_attr}
             >
                 <pb-choice value="6">More than 5 stars</pb-choice>

problem_builder/tests/unit/test_problem_builder.py

-import unittest
 import ddt
+import unittest
+
 from mock import MagicMock, Mock, patch
+from random import random
+
 from xblock.field_data import DictFieldData
+
 from problem_builder.mcq import MCQBlock
-from problem_builder.mentoring import MentoringBlock, MentoringMessageBlock, _default_theme_config
+from problem_builder.mentoring import (
+    MentoringBlock, MentoringMessageBlock, _default_theme_config, _default_options_config
+)
 
 
 @ddt.ddt
@@ -63,7 +69,6 @@ class TestMentoringBlock(unittest.TestCase):
             self.assertIn('Unable to load child component', fragment.content)
 
 
-@ddt.ddt
 class TestMentoringBlockTheming(unittest.TestCase):
     def setUp(self):
         self.service_mock = Mock()
@@ -71,76 +76,65 @@ class TestMentoringBlockTheming(unittest.TestCase):
         self.runtime_mock.service = Mock(return_value=self.service_mock)
         self.block = MentoringBlock(self.runtime_mock, DictFieldData({}), Mock())
 
-    def test_theme_uses_default_theme_if_settings_service_is_not_available(self):
-        self.runtime_mock.service = Mock(return_value=None)
-        self.assertEqual(self.block.get_theme(), _default_theme_config)
-
-    def test_theme_uses_default_theme_if_no_theme_is_set(self):
-        self.service_mock.get_settings_bucket = Mock(return_value=None)
-        self.assertEqual(self.block.get_theme(), _default_theme_config)
-        self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(123, object())
-    def test_theme_raises_if_theme_object_is_not_iterable(self, theme_config):
-        self.service_mock.get_settings_bucket = Mock(return_value=theme_config)
-        with self.assertRaises(TypeError):
-            self.block.get_theme()
-        self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(
-        {}, {'mass': 123}, {'spin': {}}, {'parity': "1"}
-    )
-    def test_theme_uses_default_theme_if_no_mentoring_theme_is_set_up(self, theme_config):
-        self.service_mock.get_settings_bucket = Mock(return_value=theme_config)
-        self.assertEqual(self.block.get_theme(), _default_theme_config)
-        self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(
-        {MentoringBlock.theme_key: 123},
-        {MentoringBlock.theme_key: [1, 2, 3]},
-        {MentoringBlock.theme_key: {'package': 'qwerty', 'locations': ['something_else.css']}},
-    )
-    def test_theme_correctly_returns_configured_theme(self, theme_config):
-        self.service_mock.get_settings_bucket = Mock(return_value=theme_config)
-        self.assertEqual(self.block.get_theme(), theme_config[MentoringBlock.theme_key])
-
-    def test_theme_files_are_loaded_from_correct_package(self):
-        fragment = MagicMock()
-        package_name = 'some_package'
-        theme_config = {MentoringBlock.theme_key: {'package': package_name, 'locations': ['lms.css']}}
-        self.service_mock.get_settings_bucket = Mock(return_value=theme_config)
-        with patch("problem_builder.mentoring.ResourceLoader") as patched_resource_loader:
-            self.block.include_theme_files(fragment)
-            patched_resource_loader.assert_called_with(package_name)
-
-    @ddt.data(
-        ('problem_builder', ['public/themes/lms.css']),
-        ('problem_builder', ['public/themes/lms.css', 'public/themes/lms.part2.css']),
-        ('my_app.my_rules', ['typography.css', 'icons.css']),
-    )
-    @ddt.unpack
-    def test_theme_files_are_added_to_fragment(self, package_name, locations):
-        fragment = MagicMock()
-        theme_config = {MentoringBlock.theme_key: {'package': package_name, 'locations': locations}}
-        self.service_mock.get_settings_bucket = Mock(return_value=theme_config)
-        with patch("problem_builder.mentoring.ResourceLoader.load_unicode") as patched_load_unicode:
-            self.block.include_theme_files(fragment)
-            for location in locations:
-                patched_load_unicode.assert_any_call(location)
-
-            self.assertEqual(patched_load_unicode.call_count, len(locations))
-
     def test_student_view_calls_include_theme_files(self):
+        self.service_mock.get_settings_bucket = Mock(return_value={})
         with patch.object(self.block, 'include_theme_files') as patched_include_theme_files:
             fragment = self.block.student_view({})
             patched_include_theme_files.assert_called_with(fragment)
 
     def test_author_preview_view_calls_include_theme_files(self):
+        self.service_mock.get_settings_bucket = Mock(return_value={})
         with patch.object(self.block, 'include_theme_files') as patched_include_theme_files:
             fragment = self.block.author_preview_view({})
             patched_include_theme_files.assert_called_with(fragment)
 
 
+@ddt.ddt
+class TestMentoringBlockOptions(unittest.TestCase):
+    def setUp(self):
+        self.service_mock = Mock()
+        self.runtime_mock = Mock()
+        self.runtime_mock.service = Mock(return_value=self.service_mock)
+        self.block = MentoringBlock(self.runtime_mock, DictFieldData({}), Mock())
+
+    def test_get_options_returns_default_if_xblock_settings_not_customized(self):
+        self.block.get_xblock_settings = Mock(return_value=None)
+        self.assertEqual(self.block.get_options(), _default_options_config)
+        self.block.get_xblock_settings.assert_called_once_with(_default_options_config)
+
+    @ddt.data(
+        {}, {'mass': 123}, {'spin': {}}, {'parity': "1"}
+    )
+    def test_get_options_returns_default_if_options_not_customized(self, xblock_settings):
+        self.block.get_xblock_settings = Mock(return_value=xblock_settings)
+        self.assertEqual(self.block.get_options(), _default_options_config)
+        self.block.get_xblock_settings.assert_called_once_with(_default_options_config)
+
+    @ddt.data(
+        {MentoringBlock.options_key: 123},
+        {MentoringBlock.options_key: [1, 2, 3]},
+        {MentoringBlock.options_key: {'pb_mcq_hide_previous_answer': False}},
+     )
+    def test_get_options_correctly_returns_customized_options(self, xblock_settings):
+        self.block.get_xblock_settings = Mock(return_value=xblock_settings)
+        self.assertEqual(self.block.get_options(), xblock_settings[MentoringBlock.options_key])
+        self.block.get_xblock_settings.assert_called_once_with(_default_options_config)
+
+    def test_get_option(self):
+        random_key, random_value = random(), random()
+        with patch.object(self.block, 'get_options') as patched_get_options:
+            patched_get_options.return_value = {random_key: random_value}
+            option = self.block.get_option(random_key)
+            patched_get_options.assert_called_once_with()
+            self.assertEqual(option, random_value)
+
+    def test_student_view_calls_get_option(self):
+        self.service_mock.get_settings_bucket = Mock(return_value={})
+        with patch.object(self.block, 'get_option') as patched_get_option:
+            self.block.student_view({})
+            patched_get_option.assert_called_with('pb_mcq_hide_previous_answer')
+
+
 class TestMentoringBlockJumpToIds(unittest.TestCase):
     def setUp(self):
         self.service_mock = Mock()