Address review comments.

- Refactor: Take advantage of mixins from xblock-utils accessing xblock settings.

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,
 )
@@ -84,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.
@@ -161,45 +163,22 @@ class BaseMentoringBlock(
             return [self.display_name]
         return []
 
-    def get_settings(self, settings_key, default):
-        """
-        Get settings identified by `settings_key` from settings service.
-
-        Fall back on `default` if settings service is unavailable
-        or settings have not been customized.
-        """
-        settings_service = self.runtime.service(self, "settings")
-        if settings_service:
-            xblock_settings = settings_service.get_settings_bucket(self)
-            if xblock_settings and settings_key in xblock_settings:
-                return xblock_settings[settings_key]
-        return default
-
-    def get_theme(self):
-        """
-        Get theme settings for this block from settings service.
-
-        Fall back on default (LMS) theme if settings service is not available,
-        or theme has not been customized.
-        """
-        return self.get_settings(self.theme_key, _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))
-
     def get_options(self):
         """
         Get options settings for this block from settings service.
 
-        Fall back on default options if settings service is not available
-        or options have not been customized.
+        Fall back on default options if xblock settings have not been customized at all
+        or no customizations for options available.
         """
-        return self.get_settings(self.options_key, _default_options_config)
+        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`.
+        """
         return self.get_options()[option]
 
     @XBlock.json_handler

problem_builder/tests/integration/test_mentoring.py

@@ -267,7 +267,6 @@ class ProblemBuilderQuestionnaireBlockTest(ProblemBuilderBaseTest):
             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())
 

problem_builder/tests/unit/test_problem_builder.py

@@ -69,74 +69,6 @@ class TestMentoringBlock(unittest.TestCase):
             self.assertIn('Unable to load child component', fragment.content)
 
 
-@ddt.ddt
-class TestMentoringBlockSettings(unittest.TestCase):
-
-    DEFAULT_SETTINGS = {
-        'get_theme': _default_theme_config,
-        'get_options': _default_options_config,
-    }
-
-    SETTINGS_KEYS = {
-        'get_theme': MentoringBlock.theme_key,
-        'get_options': MentoringBlock.options_key,
-    }
-
-    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_settings_method_returns_default_if_settings_service_is_not_available(self):
-        for settings_method, default_config in self.DEFAULT_SETTINGS.items():
-            self.runtime_mock.service = Mock(return_value=None)
-            self.assertEqual(getattr(self.block, settings_method)(), default_config)
-
-    def test_settings_method_returns_default_if_settings_not_customized(self):
-        for settings_method, default_config in self.DEFAULT_SETTINGS.items():
-            self.service_mock.get_settings_bucket = Mock(return_value=None)
-            self.assertEqual(getattr(self.block, settings_method)(), default_config)
-            self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(123, object())
-    def test_settings_method_raises_if_settings_not_iterable(self, config):
-        for settings_method in self.DEFAULT_SETTINGS:
-            self.service_mock.get_settings_bucket = Mock(return_value=config)
-            with self.assertRaises(TypeError):
-                getattr(self.block, settings_method)()
-            self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(
-        {}, {'mass': 123}, {'spin': {}}, {'parity': "1"}
-    )
-    def test_settings_method_returns_default_if_target_setting_not_customized(self, config):
-        for settings_method, default_config in self.DEFAULT_SETTINGS.items():
-            self.service_mock.get_settings_bucket = Mock(return_value=config)
-            self.assertEqual(getattr(self.block, settings_method)(), default_config)
-            self.service_mock.get_settings_bucket.assert_called_once_with(self.block)
-
-    @ddt.data(
-        {
-            MentoringBlock.theme_key: 123,
-            MentoringBlock.options_key: 123,
-        },
-        {
-            MentoringBlock.theme_key: [1, 2, 3],
-            MentoringBlock.options_key: [1, 2, 3],
-        },
-        {
-            MentoringBlock.theme_key: {'package': 'qwerty', 'locations': ['something_else.css']},
-            MentoringBlock.options_key: {'pb_mcq_hide_previous_answer': False},
-        },
-    )
-    def test_settings_method_correctly_returns_customized_settings(self, config):
-        for settings_method, settings_key in self.SETTINGS_KEYS.items():
-            self.service_mock.get_settings_bucket = Mock(return_value=config)
-            self.assertEqual(getattr(self.block, settings_method)(), config[settings_key])
-
-
-@ddt.ddt
 class TestMentoringBlockTheming(unittest.TestCase):
     def setUp(self):
         self.service_mock = Mock()
@@ -144,32 +76,6 @@ 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_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:
@@ -191,6 +97,29 @@ class TestMentoringBlockOptions(unittest.TestCase):
         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: