More fixes to the new structures editors (oppia#7770)

* Added skill selector menu for story editor

* fixed a css issue

* Added create buttons to mainpage for small windows

* Empty notes allowed

* Added rubrics to question creator

* Minor fixes

* lint

* Fixed tests

* made initial review changes

* Review changes

* Renamed function

* Review changes

* Added rubrics to change difficulty modal

* fixed frontend issue

* lint

* fixed coverage

.github/CODEOWNERS

@@ -289,6 +289,7 @@
 /core/storage/skill/ @aks681
 /core/templates/dev/head/components/entity-creation-services/skill-creation.service.ts @aks681
 /core/templates/dev/head/components/rubrics-editor/ @aks681
+/core/templates/dev/head/components/skill-selector/ @aks681
 /core/templates/dev/head/domain/skill/ @aks681
 /core/templates/dev/head/pages/skill-editor-page/ @aks681
 

core/controllers/story_editor.py

@@ -18,6 +18,7 @@
 
 from core.controllers import acl_decorators
 from core.controllers import base
+from core.domain import skill_services
 from core.domain import story_domain
 from core.domain import story_fetchers
 from core.domain import story_services
@@ -62,6 +63,9 @@ def get(self, story_id):
         story = story_fetchers.get_story_by_id(story_id, strict=False)
         topic_id = story.corresponding_topic_id
         topic = topic_fetchers.get_topic_by_id(topic_id, strict=False)
+        skill_ids = topic.get_all_skill_ids()
+        skill_summaries = skill_services.get_multi_skill_summaries(skill_ids)
+        skill_summary_dicts = [summary.to_dict() for summary in skill_summaries]
 
         for story_reference in topic.canonical_story_references:
             if story_reference.story_id == story_id:
@@ -70,7 +74,8 @@ def get(self, story_id):
         self.values.update({
             'story': story.to_dict(),
             'topic_name': topic.name,
-            'story_is_published': story_is_published
+            'story_is_published': story_is_published,
+            'skill_summaries': skill_summary_dicts
         })
 
         self.render_json(self.values)

core/controllers/story_editor_test.py

@@ -352,6 +352,7 @@ def test_editable_story_handler_get(self):
                 feconf.STORY_EDITOR_DATA_URL_PREFIX, self.story_id))
         self.assertEqual(self.story_id, json_response['story']['id'])
         self.assertEqual('Name', json_response['topic_name'])
+        self.assertEqual([], json_response['skill_summaries'])
         self.logout()
 
     def test_editable_story_handler_put(self):

core/controllers/topic_editor.py

@@ -18,6 +18,8 @@
 from __future__ import absolute_import  # pylint: disable=import-only-modules
 from __future__ import unicode_literals  # pylint: disable=import-only-modules
 
+import logging
+
 from core.controllers import acl_decorators
 from core.controllers import base
 from core.domain import email_manager
@@ -163,14 +165,30 @@ def get(self, topic_id):
             raise self.PageNotFoundException(
                 Exception('The topic with the given id doesn\'t exist.'))
 
-        skill_ids = topic.get_all_skill_ids()
-
-        skill_id_to_description_dict = (
-            skill_services.get_skill_descriptions_by_ids(topic_id, skill_ids))
+        skill_id_to_description_dict, deleted_skill_ids = (
+            skill_services.get_descriptions_of_skills(
+                topic.get_all_skill_ids()))
+
+        skill_id_to_rubrics_dict, deleted_skill_ids = (
+            skill_services.get_rubrics_of_skills(topic.get_all_skill_ids())
+        )
+
+        if deleted_skill_ids:
+            deleted_skills_string = ', '.join(deleted_skill_ids)
+            logging.error(
+                'The deleted skills: %s are still present in topic with id %s'
+                % (deleted_skills_string, topic_id)
+            )
+            if feconf.CAN_SEND_EMAILS:
+                email_manager.send_mail_to_admin(
+                    'Deleted skills present in topic',
+                    'The deleted skills: %s are still present in topic with '
+                    'id %s' % (deleted_skills_string, topic_id))
 
         self.values.update({
             'topic_dict': topic.to_dict(),
-            'skill_id_to_description_dict': skill_id_to_description_dict
+            'skill_id_to_description_dict': skill_id_to_description_dict,
+            'skill_id_to_rubrics_dict': skill_id_to_rubrics_dict
         })
 
         self.render_json(self.values)
@@ -210,14 +228,31 @@ def put(self, topic_id):
             raise self.InvalidInputException(e)
 
         topic = topic_fetchers.get_topic_by_id(topic_id, strict=False)
-        skill_ids = topic.get_all_skill_ids()
 
-        skill_id_to_description_dict = (
-            skill_services.get_skill_descriptions_by_ids(topic_id, skill_ids))
+        skill_id_to_description_dict, deleted_skill_ids = (
+            skill_services.get_descriptions_of_skills(
+                topic.get_all_skill_ids()))
+
+        skill_id_to_rubrics_dict, deleted_skill_ids = (
+            skill_services.get_rubrics_of_skills(topic.get_all_skill_ids())
+        )
+
+        if deleted_skill_ids:
+            deleted_skills_string = ', '.join(deleted_skill_ids)
+            logging.error(
+                'The deleted skills: %s are still present in topic with id %s'
+                % (deleted_skills_string, topic_id)
+            )
+            if feconf.CAN_SEND_EMAILS:
+                email_manager.send_mail_to_admin(
+                    'Deleted skills present in topic',
+                    'The deleted skills: %s are still present in topic with '
+                    'id %s' % (deleted_skills_string, topic_id))
 
         self.values.update({
             'topic_dict': topic.to_dict(),
-            'skill_id_to_description_dict': skill_id_to_description_dict
+            'skill_id_to_description_dict': skill_id_to_description_dict,
+            'skill_id_to_rubrics_dict': skill_id_to_rubrics_dict
         })
 
         self.render_json(self.values)

core/controllers/topic_editor_test.py

@@ -278,6 +278,7 @@ def test_access_topic_editor_page(self):
 
 
     def test_editable_topic_handler_get(self):
+        skill_services.delete_skill(self.admin_id, self.skill_id_2)
         # Check that non-admins cannot access the editable topic data.
         self.login(self.NEW_USER_EMAIL)
         self.get_json(
@@ -288,14 +289,29 @@ def test_editable_topic_handler_get(self):
 
         # Check that admins can access the editable topic data.
         self.login(self.ADMIN_EMAIL)
+        with self.swap(feconf, 'CAN_SEND_EMAILS', True):
+            messages = self.mail_stub.get_sent_messages(
+                to=feconf.ADMIN_EMAIL_ADDRESS)
+            self.assertEqual(len(messages), 0)
+            json_response = self.get_json(
+                '%s/%s' % (
+                    feconf.TOPIC_EDITOR_DATA_URL_PREFIX, self.topic_id))
+            self.assertEqual(self.topic_id, json_response['topic_dict']['id'])
+            self.assertEqual(
+                'Skill Description',
+                json_response['skill_id_to_description_dict'][self.skill_id])
+
+            messages = self.mail_stub.get_sent_messages(
+                to=feconf.ADMIN_EMAIL_ADDRESS)
+            expected_email_html_body = (
+                'The deleted skills: %s are still'
+                ' present in topic with id %s' % (
+                    self.skill_id_2, self.topic_id))
+            self.assertEqual(len(messages), 1)
+            self.assertIn(
+                expected_email_html_body,
+                messages[0].html.decode())
 
-        json_response = self.get_json(
-            '%s/%s' % (
-                feconf.TOPIC_EDITOR_DATA_URL_PREFIX, self.topic_id))
-        self.assertEqual(self.topic_id, json_response['topic_dict']['id'])
-        self.assertEqual(
-            'Skill Description',
-            json_response['skill_id_to_description_dict'][self.skill_id])
         self.logout()
 
         # Check that editable topic handler is accessed only when a topic id
@@ -392,17 +408,33 @@ def test_editable_topic_handler_put(self):
         }
         self.login(self.ADMIN_EMAIL)
         csrf_token = self.get_new_csrf_token()
+        skill_services.delete_skill(self.admin_id, self.skill_id_2)
 
-        json_response = self.put_json(
-            '%s/%s' % (
-                feconf.TOPIC_EDITOR_DATA_URL_PREFIX, self.topic_id),
-            change_cmd, csrf_token=csrf_token)
-        self.assertEqual(self.topic_id, json_response['topic_dict']['id'])
-        self.assertEqual('A new name', json_response['topic_dict']['name'])
-        self.assertEqual(2, len(json_response['topic_dict']['subtopics']))
-        self.assertEqual(
-            'Skill Description',
-            json_response['skill_id_to_description_dict'][self.skill_id])
+        with self.swap(feconf, 'CAN_SEND_EMAILS', True):
+            messages = self.mail_stub.get_sent_messages(
+                to=feconf.ADMIN_EMAIL_ADDRESS)
+            self.assertEqual(len(messages), 0)
+            json_response = self.put_json(
+                '%s/%s' % (
+                    feconf.TOPIC_EDITOR_DATA_URL_PREFIX, self.topic_id),
+                change_cmd, csrf_token=csrf_token)
+            self.assertEqual(self.topic_id, json_response['topic_dict']['id'])
+            self.assertEqual('A new name', json_response['topic_dict']['name'])
+            self.assertEqual(2, len(json_response['topic_dict']['subtopics']))
+            self.assertEqual(
+                'Skill Description',
+                json_response['skill_id_to_description_dict'][self.skill_id])
+
+            messages = self.mail_stub.get_sent_messages(
+                to=feconf.ADMIN_EMAIL_ADDRESS)
+            expected_email_html_body = (
+                'The deleted skills: %s are still'
+                ' present in topic with id %s' % (
+                    self.skill_id_2, self.topic_id))
+            self.assertEqual(len(messages), 1)
+            self.assertIn(
+                expected_email_html_body,
+                messages[0].html.decode())
 
         # Test if the corresponding subtopic pages were created.
         json_response = self.get_json(

core/controllers/topic_viewer.py

@@ -16,9 +16,12 @@
 from __future__ import absolute_import  # pylint: disable=import-only-modules
 from __future__ import unicode_literals  # pylint: disable=import-only-modules
 
+import logging
+
 from constants import constants
 from core.controllers import acl_decorators
 from core.controllers import base
+from core.domain import email_manager
 from core.domain import skill_services
 from core.domain import story_fetchers
 from core.domain import topic_fetchers
@@ -78,8 +81,21 @@ def get(self, topic_name):
         subtopics = topic.get_all_subtopics()
 
         assigned_skill_ids = topic.get_all_skill_ids()
-        skill_descriptions = skill_services.get_skill_descriptions_by_ids(
-            topic.id, assigned_skill_ids)
+        skill_descriptions, deleted_skill_ids = (
+            skill_services.get_descriptions_of_skills(
+                assigned_skill_ids))
+
+        if deleted_skill_ids:
+            deleted_skills_string = ', '.join(deleted_skill_ids)
+            logging.error(
+                'The deleted skills: %s are still present in topic with id %s'
+                % (deleted_skills_string, topic.id)
+            )
+            if feconf.CAN_SEND_EMAILS:
+                email_manager.send_mail_to_admin(
+                    'Deleted skills present in topic',
+                    'The deleted skills: %s are still present in topic with '
+                    'id %s' % (deleted_skills_string, topic.id))
 
         if self.user_id:
             degrees_of_mastery = skill_services.get_multi_user_skill_mastery(

core/controllers/topic_viewer_test.py

@@ -134,34 +134,49 @@ def test_get_with_no_user_logged_in(self):
             self.assertDictContainsSubset(expected_dict, json_response)
 
     def test_get_with_user_logged_in(self):
+        skill_services.delete_skill(self.admin_id, self.skill_id_1)
         with self.swap(constants, 'ENABLE_NEW_STRUCTURE_PLAYERS', True):
             self.login(self.NEW_USER_EMAIL)
-            json_response = self.get_json(
-                '%s/%s' % (feconf.TOPIC_DATA_HANDLER, 'public_topic_name'))
-            expected_dict = {
-                'topic_name': 'public_topic_name',
-                'topic_id': self.topic_id,
-                'canonical_story_dicts': [{
-                    'id': self.story.id,
-                    'title': self.story.title,
-                    'description': self.story.description
-                }],
-                'additional_story_dicts': [],
-                'uncategorized_skill_ids': [self.skill_id_1],
-                'subtopics': [{
-                    u'skill_ids': [self.skill_id_2],
-                    u'id': 1,
-                    u'title': u'subtopic_name'}],
-                'degrees_of_mastery': {
-                    self.skill_id_1: 0.3,
-                    self.skill_id_2: 0.5
-                },
-                'skill_descriptions': {
-                    self.skill_id_1: 'Skill Description 1',
-                    self.skill_id_2: 'Skill Description 2'
+            with self.swap(feconf, 'CAN_SEND_EMAILS', True):
+                messages = self.mail_stub.get_sent_messages(
+                    to=feconf.ADMIN_EMAIL_ADDRESS)
+                self.assertEqual(len(messages), 0)
+                json_response = self.get_json(
+                    '%s/%s' % (feconf.TOPIC_DATA_HANDLER, 'public_topic_name'))
+                messages = self.mail_stub.get_sent_messages(
+                    to=feconf.ADMIN_EMAIL_ADDRESS)
+                expected_email_html_body = (
+                    'The deleted skills: %s are still'
+                    ' present in topic with id %s' % (
+                        self.skill_id_1, self.topic_id))
+                self.assertEqual(len(messages), 1)
+                self.assertIn(
+                    expected_email_html_body,
+                    messages[0].html.decode())
+                expected_dict = {
+                    'topic_name': 'public_topic_name',
+                    'topic_id': self.topic_id,
+                    'canonical_story_dicts': [{
+                        'id': self.story.id,
+                        'title': self.story.title,
+                        'description': self.story.description
+                    }],
+                    'additional_story_dicts': [],
+                    'uncategorized_skill_ids': [self.skill_id_1],
+                    'subtopics': [{
+                        u'skill_ids': [self.skill_id_2],
+                        u'id': 1,
+                        u'title': u'subtopic_name'}],
+                    'degrees_of_mastery': {
+                        self.skill_id_1: 0.3,
+                        self.skill_id_2: 0.5
+                    },
+                    'skill_descriptions': {
+                        self.skill_id_1: None,
+                        self.skill_id_2: 'Skill Description 2'
+                    }
                 }
-            }
-            self.assertDictContainsSubset(expected_dict, json_response)
+                self.assertDictContainsSubset(expected_dict, json_response)
 
             self.logout()