Domain object changes for topic, skill, question models (oppia#6989)

* Domain object changes for topic, skill, ques models * Fix test * Ensure strings convertible to ckeditor dont fail validation * Fix lint * Address review comments * Change id to misconception id * Add id check * Remove extra attr * Fix test * Fix issues * Fix test * Add test for all commit log models * Update related model to external model * Fix check * Address review comments * entity -> the entity
saketringsia · Jun 25, 2019 · 02e5a7e · 02e5a7e
1 parent dff6a6f
commit 02e5a7e
Show file tree

Hide file tree

Showing 54 changed files with 2,314 additions and 931 deletions.
diff --git a/core/controllers/concept_card_viewer_test.py b/core/controllers/concept_card_viewer_test.py
@@ -37,9 +37,9 @@ def setUp(self):
 
         self.skill_contents = skill_domain.SkillContents(
             state_domain.SubtitledHtml(
-                '1', 'Skill Explanation'), [
-                    state_domain.SubtitledHtml('2', 'Example 1'),
-                    state_domain.SubtitledHtml('3', 'Example 2')],
+                '1', '<p>Skill Explanation</p>'), [
+                    state_domain.SubtitledHtml('2', '<p>Example 1</p>'),
+                    state_domain.SubtitledHtml('3', '<p>Example 2</p>')],
             {'1': {}, '2': {}, '3': {}},
             state_domain.WrittenTranslations.from_dict({
                 'translations_mapping': {'1': {}, '2': {}, '3': {}}
@@ -55,15 +55,15 @@ def test_get_concept_card(self):
             json_response = self.get_json(
                 '%s/%s' % (feconf.CONCEPT_CARD_DATA_URL_PREFIX, self.skill_id))
             self.assertEqual(
-                'Skill Explanation',
+                '<p>Skill Explanation</p>',
                 json_response['concept_card_dict']['explanation']['html'])
             self.assertEqual(
                 [{
                     'content_id': '2',
-                    'html': 'Example 1'
+                    'html': '<p>Example 1</p>'
                 }, {
                     'content_id': '3',
-                    'html': 'Example 2'
+                    'html': '<p>Example 2</p>'
                 }],
                 json_response['concept_card_dict']['worked_examples'])
 

diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py
@@ -1103,7 +1103,7 @@ def setUp(self):
                 'state_name': exploration.init_state_name,
                 'new_value': {
                     'content_id': 'content',
-                    'html': 'ABC'
+                    'html': '<p>ABC</p>'
                 },
             })], 'Change objective and init state content')
 

diff --git a/core/controllers/feedback_test.py b/core/controllers/feedback_test.py
@@ -527,7 +527,7 @@ def test_feedback_threads(self):
 
     def test_feedback_threads_with_suggestions(self):
         new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         change_cmd = {
             'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY,
             'property_name': exp_domain.STATE_PROPERTY_CONTENT,
@@ -598,7 +598,7 @@ def test_post_feedback_threads_with_updated_suggestion_status_raises_400(
         csrf_token = self.get_csrf_token_from_response(response)
 
         new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         change = {
             'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY,
             'property_name': exp_domain.STATE_PROPERTY_CONTENT,

diff --git a/core/controllers/learner_dashboard_test.py b/core/controllers/learner_dashboard_test.py
@@ -395,7 +395,7 @@ def test_get_suggestions_after_updating_suggestion_summary(self):
         self.assertFalse(messages_summary.get('description'))
 
         new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         change_cmd = {
             'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY,
             'property_name': exp_domain.STATE_PROPERTY_CONTENT,
@@ -424,7 +424,7 @@ def test_get_suggestions_after_updating_suggestion_summary(self):
             messages_summary['author_picture_data_url'].startswith(
                 'data:image/png;'))
         self.assertEqual(
-            messages_summary['suggestion_html'], 'new content html')
+            messages_summary['suggestion_html'], '<p>new content html</p>')
         self.assertEqual(
             messages_summary['current_content_html'], current_content_html)
         self.assertEqual(

diff --git a/core/controllers/subtopic_viewer_test.py b/core/controllers/subtopic_viewer_test.py
@@ -64,11 +64,11 @@ def setUp(self):
             'content_ids_to_audio_translations':
                 self.content_ids_to_audio_translations_dict,
             'subtitled_html': {
-                'content_id': 'content', 'html': 'hello world'
+                'content_id': 'content', 'html': '<p>hello world</p>'
             }
         }
         self.subtopic_page.update_page_contents_html({
-            'html': 'hello world',
+            'html': '<p>hello world</p>',
             'content_id': 'content'
         })
         self.subtopic_page.update_page_contents_audio(
@@ -96,7 +96,7 @@ def test_get(self):
                     self.content_ids_to_audio_translations_dict,
                 'subtitled_html': {
                     'content_id': 'content',
-                    'html': 'hello world'
+                    'html': '<p>hello world</p>'
                 },
                 'written_translations': self.written_translations_dict
             }

diff --git a/core/controllers/suggestion_test.py b/core/controllers/suggestion_test.py
@@ -71,7 +71,7 @@ def setUp(self):
                 ['TextInput'], category='Algebra'))
 
         self.old_content = state_domain.SubtitledHtml(
-            'content', 'old content html').to_dict()
+            'content', '<p>old content html</p>').to_dict()
 
         exploration.states['State 1'].update_content(self.old_content)
         exploration.states['State 2'].update_content(self.old_content)
@@ -84,9 +84,9 @@ def setUp(self):
             rights_manager.ROLE_EDITOR)
 
         self.new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         self.resubmit_change_content = state_domain.SubtitledHtml(
-            'content', 'resubmit change content html').to_dict()
+            'content', '<p>resubmit change content html</p>').to_dict()
 
         self.logout()
 
@@ -318,7 +318,7 @@ def test_owner_of_exploration_cannot_repond_to_own_suggestion(self):
         self.save_new_default_exploration(exp_id, self.editor_id)
 
         new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         change_cmd = {
             'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY,
             'property_name': exp_domain.STATE_PROPERTY_CONTENT,
@@ -806,7 +806,7 @@ def test_suggestion_to_topic_handler_with_invalid_target_type(self):
         self.save_new_default_exploration(exp_id, self.admin_id)
 
         new_content = state_domain.SubtitledHtml(
-            'content', 'new content html').to_dict()
+            'content', '<p>new content html</p>').to_dict()
         change_cmd = {
             'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY,
             'property_name': exp_domain.STATE_PROPERTY_CONTENT,

diff --git a/core/controllers/topic_editor.py b/core/controllers/topic_editor.py
@@ -238,7 +238,8 @@ def put(self, topic_id):
             'topic_and_subtopic_page_change_dicts')
         topic_and_subtopic_page_change_list = []
         for change in topic_and_subtopic_page_change_dicts:
-            if change['change_affects_subtopic_page']:
+            if change['cmd'] == (
+                    subtopic_page_domain.CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY):
                 topic_and_subtopic_page_change_list.append(
                     subtopic_page_domain.SubtopicPageChange(change))
             else:

diff --git a/core/controllers/topic_editor_test.py b/core/controllers/topic_editor_test.py
@@ -362,7 +362,6 @@ def test_editable_topic_handler_put_raises_error_with_invalid_name(self):
             'version': 2,
             'commit_message': 'Changed name',
             'topic_and_subtopic_page_change_dicts': [{
-                'change_affects_subtopic_page': False,
                 'cmd': 'update_topic_property',
                 'property_name': 'name',
                 'old_value': '',
@@ -387,13 +386,11 @@ def test_editable_topic_handler_put(self):
             'version': 2,
             'commit_message': 'Some changes and added a subtopic.',
             'topic_and_subtopic_page_change_dicts': [{
-                'change_affects_subtopic_page': False,
                 'cmd': 'update_topic_property',
                 'property_name': 'name',
                 'old_value': '',
                 'new_value': 'A new name'
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_html',
                 'old_value': {
@@ -406,12 +403,10 @@ def test_editable_topic_handler_put(self):
                     'content_id': 'content'
                 }
             }, {
-                'change_affects_subtopic_page': False,
                 'cmd': 'add_subtopic',
                 'subtopic_id': 2,
                 'title': 'Title2'
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_html',
                 'old_value': {
@@ -424,7 +419,6 @@ def test_editable_topic_handler_put(self):
                 },
                 'subtopic_id': 2
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_audio',
                 'old_value': {
@@ -558,13 +552,11 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self):
             'version': 2,
             'commit_message': 'Some changes and added a subtopic.',
             'topic_and_subtopic_page_change_dicts': [{
-                'change_affects_subtopic_page': False,
                 'cmd': 'update_topic_property',
                 'property_name': 'name',
                 'old_value': '',
                 'new_value': 'A new name'
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_html',
                 'old_value': {
@@ -577,12 +569,10 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self):
                     'content_id': 'content'
                 }
             }, {
-                'change_affects_subtopic_page': False,
                 'cmd': 'add_subtopic',
                 'subtopic_id': 2,
                 'title': 'Title2'
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_html',
                 'old_value': {
@@ -595,7 +585,6 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self):
                 },
                 'subtopic_id': 2
             }, {
-                'change_affects_subtopic_page': True,
                 'cmd': 'update_subtopic_page_property',
                 'property_name': 'page_contents_audio',
                 'old_value': {

diff --git a/core/domain/exp_domain_test.py b/core/domain/exp_domain_test.py
@@ -37,6 +37,22 @@ def mock_get_filename_with_dimensions(filename, unused_exp_id):
         filename, 490, 120)
 
 
+# This function should be only be used while loading v26 textangular
+# exploration. If we do not use a mock there, the loading will
+# not pass the validation, since the current html validation
+# assumes CKEditor formatting.
+def mock_validate_rte_format_for_v26(unused_html_list, unused_rte_format):
+    return {}
+
+
+# This function should be only be used while loading v27 exploration
+# without image caption. If we do not use a mock there, the loading will
+# not pass the validation, since the current html validation
+# requires image tags to have a caption attribute.
+def mock_validate_customization_args_for_v27(unused_html_list):
+    return {}
+
+
 class ExplorationChangeTests(test_utils.GenericTestBase):
 
     def test_exp_change_object_with_missing_cmd(self):
@@ -414,7 +430,7 @@ def test_validation(self):
                 'dest': exploration.init_state_name,
                 'feedback': {
                     'content_id': 'feedback_1',
-                    'html': 'Feedback'
+                    'html': '<p>Feedback</p>'
                 },
                 'labelled_as_correct': False,
                 'param_changes': [],
@@ -4625,9 +4641,13 @@ class HTMLMigrationUnitTests(test_utils.GenericTestBase):
           feedback:
             content_id: feedback_1
             html: Here is the image1 <i><oppia-noninteractive-image
-                filepath-with-value="&amp;quot;startBlue.png&amp;quot;">
+                caption-with-value="&amp;quot;&amp;quot;"
+                filepath-with-value="&amp;quot;startBlue.png&amp;quot;"
+                alt-with-value="&amp;quot;&amp;quot;">
                 </oppia-noninteractive-image></i>Here is the image2
-                <div><oppia-noninteractive-image filepath-with-value="&amp;quot;startBlue.png&amp;quot;">
+                <div><oppia-noninteractive-image caption-with-value="&amp;quot;&amp;quot;"
+                filepath-with-value="&amp;quot;startBlue.png&amp;quot;"
+                alt-with-value="&amp;quot;&amp;quot;">
                 </oppia-noninteractive-image></div>
           labelled_as_correct: false
           missing_prerequisite_skill_id: null
@@ -4854,10 +4874,11 @@ class HTMLMigrationUnitTests(test_utils.GenericTestBase):
           dest: state1
           feedback:
             content_id: feedback_1
-            html: <p>Here is the image1 </p><oppia-noninteractive-image caption-with-value="&amp;quot;&amp;quot;"
-              filepath-with-value="&amp;quot;startBlue_height_490_width_120.png&amp;quot;">
-              </oppia-noninteractive-image><p>Here is the image2 </p><oppia-noninteractive-image
+            html: <p>Here is the image1 </p><oppia-noninteractive-image alt-with-value="&amp;quot;&amp;quot;"
               caption-with-value="&amp;quot;&amp;quot;" filepath-with-value="&amp;quot;startBlue_height_490_width_120.png&amp;quot;">
+              </oppia-noninteractive-image><p>Here is the image2 </p><oppia-noninteractive-image
+              alt-with-value="&amp;quot;&amp;quot;" caption-with-value="&amp;quot;&amp;quot;"
+              filepath-with-value="&amp;quot;startBlue_height_490_width_120.png&amp;quot;">
               </oppia-noninteractive-image>
           labelled_as_correct: false
           missing_prerequisite_skill_id: null
@@ -5158,23 +5179,34 @@ class HTMLMigrationUnitTests(test_utils.GenericTestBase):
 
     def test_load_from_v26_textangular(self):
         """Test direct loading from a v26 yaml file."""
-        with self.swap(
+        mock_get_filename_with_dimensions_context = self.swap(
             html_validation_service, 'get_filename_with_dimensions',
-            mock_get_filename_with_dimensions):
-
-            exploration = exp_domain.Exploration.from_yaml(
-                'eid', self.YAML_CONTENT_V26_TEXTANGULAR)
+            mock_get_filename_with_dimensions)
+        mock_validate_rte_format_for_v26_context = self.swap(
+            html_validation_service, 'validate_rte_format',
+            mock_validate_rte_format_for_v26)
+
+        with mock_get_filename_with_dimensions_context:
+            with mock_validate_rte_format_for_v26_context:
+                exploration = exp_domain.Exploration.from_yaml(
+                    'eid', self.YAML_CONTENT_V26_TEXTANGULAR)
         self.assertEqual(
             exploration.to_yaml(), self.YAML_CONTENT_V34_IMAGE_DIMENSIONS)
 
+
     def test_load_from_v27_without_image_caption(self):
         """Test direct loading from a v27 yaml file."""
-        with self.swap(
+        mock_get_filename_with_dimensions_context = self.swap(
             html_validation_service, 'get_filename_with_dimensions',
-            mock_get_filename_with_dimensions):
-
-            exploration = exp_domain.Exploration.from_yaml(
-                'eid', self.YAML_CONTENT_V27_WITHOUT_IMAGE_CAPTION)
+            mock_get_filename_with_dimensions)
+        mock_validate_customization_args_for_v27_context = self.swap(
+            html_validation_service, 'validate_customization_args',
+            mock_validate_customization_args_for_v27)
+
+        with mock_get_filename_with_dimensions_context:
+            with mock_validate_customization_args_for_v27_context:
+                exploration = exp_domain.Exploration.from_yaml(
+                    'eid', self.YAML_CONTENT_V27_WITHOUT_IMAGE_CAPTION)
         self.assertEqual(
             exploration.to_yaml(), self.YAML_CONTENT_V34_WITH_IMAGE_CAPTION)