Implement editing of questions (#5614)

* Save.

* Save.

* Save. UrlInterpolationError topic null.

* Save.

* Modify backend to return associated skills when fetching a question.

* save.

* Saving content works.

* Copy interaction.

* Apply QuestionUpdateService for all changes.

* Circumvent race condition on initialization.

* Lint.

* Clone UndoRedoService for questions.

* Fetch question summaries after edit question.

* Create reusable function in QuestionEditorDirective.

* Lint.

* Add test.

* Fix test.

* Lint.

* Disable flag.

* Review changes.

* Review changes.

* Review comments.

* Lint.

* Review changes.

* Deduplicate UndoRedoService.

* Move updateFunction into QuestionUpdateService.

* Fix test.

* Lint.

* Update documentation.

* Add tests for get_models_by_question_id

* Update setQuestionStateData.

* Fix test.

* Fix test.

* Lint.

* Add test.

* Lint.

* Edit questions in modal.

* Add blank commit message.

* Remove duplicate function; Fix issue with not being able to edit a question's interaction; Fix issue with not validating when no interaction is specified.

* Lint.

* Lint.

* Fix.

* Flip flag.

core/controllers/question_editor.py

@@ -112,8 +112,17 @@ def get(self, question_id):
             raise self.PageNotFoundException(
                 'The question with the given id doesn\'t exist.')
 
+        associated_skill_ids = [link.skill_id for link in (
+            question_services.get_question_skill_links_of_question(
+                question_id))]
+        associated_skills = skill_services.get_multi_skills(
+            associated_skill_ids)
+        associated_skill_dicts = [
+            skill.to_dict() for skill in associated_skills]
+
         self.values.update({
-            'question_dict': question.to_dict()
+            'question_dict': question.to_dict(),
+            'associated_skill_dicts': associated_skill_dicts
         })
         self.render_json(self.values)
 
@@ -154,7 +163,7 @@ def put(self, question_id):
 
         question_dict = question_services.get_question_by_id(
             question_id).to_dict()
-        return self.render_json({
+        self.render_json({
             'question_dict': question_dict
         })
 

core/controllers/question_editor_test.py

@@ -216,6 +216,14 @@ def test_delete(self):
 
 class EditableQuestionDataHandlerTest(BaseQuestionEditorControllerTests):
     """Tests get, put and delete methods of editable questions data handler."""
+    def setUp(self):
+        """Completes the setup for QuestionSkillLinkHandlerTest."""
+        super(EditableQuestionDataHandlerTest, self).setUp()
+        self.skill_id = skill_services.get_new_skill_id()
+        self.save_new_skill(
+            self.skill_id, self.admin_id, 'Skill Description')
+        question_services.create_new_question_skill_link(
+            self.question_id, self.skill_id)
 
     def test_get(self):
         with self.swap(constants, 'ENABLE_NEW_STRUCTURES', True):
@@ -239,6 +247,11 @@ def test_get(self):
             self.assertEqual(
                 response_dict['question_dict']['question_state_data'],
                 self.question.question_state_data.to_dict())
+            self.assertEqual(
+                len(response_dict['associated_skill_dicts']), 1)
+            self.assertEqual(
+                response_dict['associated_skill_dicts'][0]['id'],
+                self.skill_id)
             self.logout()
 
             self.login(self.TOPIC_MANAGER_EMAIL)
@@ -251,6 +264,11 @@ def test_get(self):
             self.assertEqual(
                 response_dict['question_dict']['question_state_data'],
                 self.question.question_state_data.to_dict())
+            self.assertEqual(
+                len(response_dict['associated_skill_dicts']), 1)
+            self.assertEqual(
+                response_dict['associated_skill_dicts'][0]['id'],
+                self.skill_id)
             self.logout()
 
             # Check that question creator can view the data.
@@ -264,6 +282,11 @@ def test_get(self):
             self.assertEqual(
                 response_dict['question_dict']['question_state_data'],
                 self.question.question_state_data.to_dict())
+            self.assertEqual(
+                len(response_dict['associated_skill_dicts']), 1)
+            self.assertEqual(
+                response_dict['associated_skill_dicts'][0]['id'],
+                self.skill_id)
             self.logout()
 
     def test_delete(self):

core/domain/question_services.py

@@ -204,6 +204,23 @@ def get_question_from_model(question_model):
         question_model.created_on, question_model.last_updated)
 
 
+def get_question_skill_link_from_model(question_skill_link_model):
+    """Returns domain object representing the given question skill link model.
+
+    Args:
+        question_skill_link_model: QuestionSkillLinkModel. The question skill
+            link model loaded from the datastore.
+
+    Returns:
+        QuestionSkillLink. The domain object representing the question skill
+            link model.
+    """
+
+    return question_domain.QuestionSkillLink(
+        question_skill_link_model.question_id,
+        question_skill_link_model.skill_id)
+
+
 def get_question_by_id(question_id, strict=True):
     """Returns a domain object representing a question.
 
@@ -226,23 +243,43 @@ def get_question_by_id(question_id, strict=True):
 
 
 def get_question_skill_links_of_skill(skill_id):
-    """Returns a list of QuestionSkillLinkModels of
+    """Returns a list of QuestionSkillLinks of
     a particular skill ID.
 
     Args:
         skill_id: str. ID of the skill.
 
     Returns:
-        list(QuestionSkillLinkModel). The list of question skill link
+        list(QuestionSkillLink). The list of question skill link
         domain objects that are linked to the skill ID or an empty list
-         if the skill does not exist.
+        if the skill does not exist.
     """
 
-    question_skill_link_models = (
+    question_skill_links = [
+        get_question_skill_link_from_model(model) for model in
         question_models.QuestionSkillLinkModel.get_models_by_skill_id(
-            skill_id)
-    )
-    return question_skill_link_models
+            skill_id)]
+    return question_skill_links
+
+
+def get_question_skill_links_of_question(question_id):
+    """Returns a list of QuestionSkillLinks of
+    a particular question ID.
+
+    Args:
+        question_id: str. ID of the question.
+
+    Returns:
+        list(QuestionSkillLink). The list of question skill link
+        domain objects that are linked to the question ID or an empty list
+        if the question does not exist.
+    """
+
+    question_skill_links = [
+        get_question_skill_link_from_model(model) for model in
+        question_models.QuestionSkillLinkModel.get_models_by_question_id(
+            question_id)]
+    return question_skill_links
 
 
 def update_skill_ids_of_questions(curr_skill_id, new_skill_id):
@@ -253,17 +290,20 @@ def update_skill_ids_of_questions(curr_skill_id, new_skill_id):
         curr_skill_id: str. ID of the current skill.
         new_skill_id: str. ID of the superseding skill.
     """
+    old_question_skill_link_models = (
+        question_models.QuestionSkillLinkModel.get_models_by_skill_id(
+            curr_skill_id))
     old_question_skill_links = get_question_skill_links_of_skill(curr_skill_id)
-    new_question_skill_links = []
+    new_question_skill_link_models = []
     for question_skill_link in old_question_skill_links:
-        new_question_skill_links.append(
+        new_question_skill_link_models.append(
             question_models.QuestionSkillLinkModel.create(
                 question_skill_link.question_id, new_skill_id)
             )
     question_models.QuestionSkillLinkModel.delete_multi_question_skill_links(
-        old_question_skill_links)
+        old_question_skill_link_models)
     question_models.QuestionSkillLinkModel.put_multi_question_skill_links(
-        new_question_skill_links)
+        new_question_skill_link_models)
 
 
 def get_question_summaries_linked_to_skills(
@@ -365,9 +405,9 @@ def apply_change_list(question_id, change_list):
                 if (change.property_name ==
                         question_domain.QUESTION_PROPERTY_LANGUAGE_CODE):
                     question.update_language_code(change.new_value)
-                elif (change.cmd ==
+                elif (change.property_name ==
                       question_domain.QUESTION_PROPERTY_QUESTION_STATE_DATA):
-                    question.update_question_data(change.new_value)
+                    question.update_question_state_data(change.new_value)
 
         return question
 
@@ -490,7 +530,7 @@ def save_question_summary(question_summary):
 
 def get_question_summary_from_model(question_summary_model):
     """Returns a domain object for an Oppia question summary given a
-    questioin summary model.
+    question summary model.
 
     Args:
         question_summary_model: QuestionSummaryModel.

core/domain/question_services_test.py

@@ -158,6 +158,8 @@ def test_get_question_skill_links_of_skill(self):
                 'skill_1'))
 
         self.assertEqual(len(question_skill_links), 2)
+        self.assertTrue(isinstance(question_skill_links[0],
+                                   question_domain.QuestionSkillLink))
         question_ids = [question_skill.question_id for question_skill
                         in question_skill_links]
         self.assertItemsEqual(
@@ -296,3 +298,40 @@ def test_created_question_rights(self):
             Exception, 'Entity for class QuestionRightsModel with id '
             'question_id not found'):
             question_services.get_question_rights('question_id')
+
+    def test_get_question_skill_links_of_question(self):
+        # If the question id doesnt exist at all, it returns an empty list.
+        question_skill_links = (
+            question_services.get_question_skill_links_of_question(
+                'non_existent_question_id'))
+        self.assertEqual(len(question_skill_links), 0)
+
+        question_id_2 = question_services.get_new_question_id()
+        self.save_new_question(
+            question_id_2, self.editor_id,
+            self._create_valid_question_data('ABC'))
+
+        question_id_3 = question_services.get_new_question_id()
+        self.save_new_question(
+            question_id_3, self.editor_id,
+            self._create_valid_question_data('ABC'))
+        question_services.create_new_question_skill_link(
+            self.question_id, 'skill_1')
+        question_services.create_new_question_skill_link(
+            question_id_2, 'skill_1')
+        question_services.create_new_question_skill_link(
+            question_id_2, 'skill_2')
+        question_services.create_new_question_skill_link(
+            question_id_3, 'skill_2')
+
+        question_skill_links = (
+            question_services.get_question_skill_links_of_question(
+                question_id_2))
+
+        self.assertTrue(isinstance(question_skill_links[0],
+                                   question_domain.QuestionSkillLink))
+        self.assertEqual(len(question_skill_links), 2)
+        skill_ids = [question_skill.skill_id for question_skill
+                     in question_skill_links]
+        self.assertItemsEqual(
+            skill_ids, ['skill_1', 'skill_2'])

core/domain/skill_services.py

@@ -196,6 +196,26 @@ def get_multi_skill_summaries(skill_ids):
     return skill_summaries
 
 
+def get_multi_skills(skill_ids):
+    """Returns a list of skills matching the skill IDs provided.
+
+    Args:
+        skill_ids: list(str). List of skill IDs to get skills for.
+
+    Returns:
+        list(Skill). The list of skills matching the provided IDs.
+    """
+    local_skill_models = skill_models.SkillModel.get_multi(skill_ids)
+    for skill_id, skill_model in zip(skill_ids, local_skill_models):
+        if skill_model is None:
+            raise Exception('No skill exists for ID %s' % skill_id)
+    skills = [
+        get_skill_from_model(skill_model)
+        for skill_model in local_skill_models
+        if skill_model is not None]
+    return skills
+
+
 def get_skill_summary_from_model(skill_summary_model):
     """Returns a domain object for an Oppia skill summary given a
     skill summary model.

core/domain/skill_services_test.py

@@ -288,6 +288,28 @@ def test_get_unpublished_skill_rights_by_creator(self):
         skill_ids = [skill_rights_obj.id for skill_rights_obj in skill_rights]
         self.assertListEqual(skill_ids, ['skill_a', 'skill_b'])
 
+    def test_get_multi_skills(self):
+        self.save_new_skill(
+            'skill_a', self.user_id_admin, 'Description A', misconceptions=[],
+            skill_contents=skill_domain.SkillContents(
+                state_domain.SubtitledHtml(
+                    '1', 'Explanation'), [
+                        state_domain.SubtitledHtml('2', 'Example 1')], {}))
+        self.save_new_skill(
+            'skill_b', self.user_id_admin, 'Description B', misconceptions=[],
+            skill_contents=skill_domain.SkillContents(
+                state_domain.SubtitledHtml(
+                    '1', 'Explanation'), [
+                        state_domain.SubtitledHtml('2', 'Example 1')], {}))
+        try:
+            skill_services.get_multi_skills(['skill_a', 'skill_b'])
+        except Exception:
+            self.fail(msg='Unexpected exception raised.')
+
+        with self.assertRaisesRegexp(
+            Exception, 'No skill exists for ID skill_c'):
+            skill_services.get_multi_skills(['skill_a', 'skill_c'])
+
 
 class SkillMasteryServicesUnitTests(test_utils.GenericTestBase):
     """Test the skill mastery services module."""

core/domain/state_domain.py

@@ -402,7 +402,8 @@ def from_dict(cls, interaction_dict):
         solution_dict = (
             Solution.from_dict(
                 interaction_dict['id'], interaction_dict['solution'])
-            if interaction_dict['solution'] else None)
+            if (interaction_dict['solution'] and interaction_dict['id'])
+            else None)
 
         return cls(
             interaction_dict['id'],
@@ -1114,7 +1115,8 @@ def validate(self, exp_param_specs_dict, allow_null_interaction):
         if not set(content_id_list) <= set(available_content_ids):
             raise utils.ValidationError(
                 'Expected state content_ids_to_audio_translations to have all '
-                'of the listed content ids %s' % content_id_list)
+                'of the listed content ids %s, found %s' %
+                (content_id_list, available_content_ids))
         if not isinstance(self.content_ids_to_audio_translations, dict):
             raise utils.ValidationError(
                 'Expected state content_ids_to_audio_translations to be a dict,'

core/storage/question/gae_models.py

@@ -267,9 +267,26 @@ def get_models_by_skill_id(cls, skill_id):
         return QuestionSkillLinkModel.query().filter(
             cls.skill_id == skill_id).fetch()
 
+    @classmethod
+    def get_models_by_question_id(cls, question_id):
+        """Returns a list of QuestionSkillLinkModels of a particular
+        question ID.
+
+        Args:
+            question_id: str. ID of the question.
+
+        Returns:
+            list(QuestionSkillLinkModel)|None. The list of question skill link
+            models that are linked to the question ID, or None if there are no
+            question skill link models associated with the question ID.
+        """
+        return QuestionSkillLinkModel.query().filter(
+            cls.question_id == question_id,
+            cls.deleted == False).fetch() #pylint: disable=singleton-comparison
+
     @classmethod
     def put_multi_question_skill_links(cls, question_skill_links):
-        """Puts multiple question skill links into the datastore.
+        """Puts multiple question skill link models into the datastore.
 
         Args:
             question_skill_links: list(QuestionSkillLink). The list of

core/storage/question/gae_models_test.py

@@ -140,3 +140,32 @@ def test_delete_multi_question_skill_link(self):
         )
         self.assertEqual(len(question_skill_links), 1)
         self.assertEqual(question_skill_links[0].question_id, 'question_id3')
+
+    def test_get_models_by_question_id(self):
+        questionskilllink_model1 = (
+            question_models.QuestionSkillLinkModel.create(
+                'question_id1', 'skill_id1')
+            )
+        questionskilllink_model2 = (
+            question_models.QuestionSkillLinkModel.create(
+                'question_id2', 'skill_id1')
+            )
+        questionskilllink_model3 = (
+            question_models.QuestionSkillLinkModel.create(
+                'question_id2', 'skill_id3')
+            )
+
+        question_models.QuestionSkillLinkModel.put_multi_question_skill_links(
+            [questionskilllink_model1, questionskilllink_model2,
+             questionskilllink_model3])
+
+        question_skill_links = (
+            question_models.QuestionSkillLinkModel.get_models_by_question_id(
+                'question_id2')
+        )
+        self.assertEqual(len(question_skill_links), 2)
+        question_skill_links = (
+            question_models.QuestionSkillLinkModel.get_models_by_question_id(
+                'question_id3')
+        )
+        self.assertEqual(len(question_skill_links), 0)