review changes

oppia · aks681 · Aug 14, 2019 · Jul 21, 2019 · Jul 21, 2019 · Jul 23, 2019
commit 029526e3750a0fc99701e250b2273db14abfdc2b
diff --git a/core/controllers/skill_mastery.py b/core/controllers/skill_mastery.py
@@ -49,7 +49,7 @@ def put(self):
             try:
                 degree_of_mastery_per_skill[skill_id] = (
                     float(degree_of_mastery_per_skill[skill_id]))
-            except TypeError:
+            except (TypeError, ValueError):
                 raise self.InvalidInputException(
                     'Expected degree of mastery of skill %s to be a number.'
                     % skill_id)
@@ -65,9 +65,7 @@ def put(self):
         except Exception as e:
             raise self.PageNotFoundException(e)
 
-        degrees_of_mastery = degree_of_mastery_per_skill.values()
-
         skill_services.create_multi_user_skill_mastery(
-            self.user_id, skill_ids, degrees_of_mastery)
+            self.user_id, degree_of_mastery_per_skill)
 
         self.render_json(self.values)
diff --git a/core/domain/skill_services.py b/core/domain/skill_services.py
@@ -839,26 +839,18 @@ def save_user_skill_mastery(user_skill_mastery):
     user_skill_mastery_model.put()
 
 
-def create_multi_user_skill_mastery(user_id, skill_ids, degrees_of_mastery):
+def create_multi_user_skill_mastery(user_id, degrees_of_mastery):
     """Creates the mastery of a user in multiple skills.
 
     Args:
         user_id: str. The user ID of the user.
-        skill_ids: list(str). Skill IDs of the requested skills.
-        degrees_of_mastery: list(float). The degrees of mastery of the user in
-            the requested skills.
-
-    Raises:
-        Exception: skill_ids and degrees_of_mastery lists have different
-            lengths.
+        degrees_of_mastery: dict(str, float). The keys are the requested
+            skill IDs. The values are the corresponding mastery degree of
+            the user.
     """
-    if len(skill_ids) != len(degrees_of_mastery):
-        raise Exception(
-            'skill_ids and degrees_of_mastery should have the same length.')
-
     user_skill_mastery_models = []
 
-    for skill_id, degree_of_mastery in zip(skill_ids, degrees_of_mastery):
+    for skill_id, degree_of_mastery in degrees_of_mastery.iteritems():
         user_skill_mastery_models.append(user_models.UserSkillMasteryModel(
             id=user_models.UserSkillMasteryModel.construct_model_id(
                 user_id, skill_id),
@@ -867,7 +859,7 @@ def create_multi_user_skill_mastery(user_id, skill_ids, degrees_of_mastery):
     user_models.UserSkillMasteryModel.put_multi(user_skill_mastery_models)
 
 
-def get_skill_mastery(user_id, skill_id):
+def get_user_skill_mastery(user_id, skill_id):
     """Fetches the mastery of user in a particular skill.
 
     Args:
@@ -890,13 +882,13 @@ def get_skill_mastery(user_id, skill_id):
     return user_skill_mastery_model.degree_of_mastery
 
 
-def get_multi_skill_mastery(user_id, skill_ids):
+def get_multi_user_skill_mastery(user_id, skill_ids):
     """Fetches the mastery of user in multiple skills.
 
     Args:
         user_id: str. The user ID of the user.
-        skill_ids: list(str). Skill IDs of the skill for which mastery degree
-            is requested.
+        skill_ids: list(str). Skill IDs of the skill for which mastery degree is
+            requested.
 
     Returns:
         degrees_of_mastery: dict(str, float|None). The keys are the requested

diff --git a/core/domain/skill_services_test.py b/core/domain/skill_services_test.py
@@ -752,19 +752,19 @@ def setUp(self):
         skill_services.create_user_skill_mastery(
             self.USER_ID, self.SKILL_ID_2, self.DEGREE_OF_MASTERY_2)
 
-    def test_get_skill_mastery(self):
-        degree_of_mastery = skill_services.get_skill_mastery(
+    def test_get_user_skill_mastery(self):
+        degree_of_mastery = skill_services.get_user_skill_mastery(
             self.USER_ID, self.SKILL_ID_1)
 
         self.assertEqual(degree_of_mastery, self.DEGREE_OF_MASTERY_1)
 
-        degree_of_mastery = skill_services.get_skill_mastery(
+        degree_of_mastery = skill_services.get_user_skill_mastery(
             self.USER_ID, self.SKILL_ID_3)
 
         self.assertEqual(degree_of_mastery, None)
 
-    def test_get_multi_skill_mastery(self):
-        degree_of_mastery = skill_services.get_multi_skill_mastery(
+    def test_get_multi_user_skill_mastery(self):
+        degree_of_mastery = skill_services.get_multi_user_skill_mastery(
             self.USER_ID, self.SKILL_IDS)
 
         self.assertEqual(
@@ -778,23 +778,14 @@ def test_create_multi_user_skill_mastery(self):
         skill_id_4 = skill_services.get_new_skill_id()
         skill_id_5 = skill_services.get_new_skill_id()
         skill_services.create_multi_user_skill_mastery(
-            self.USER_ID, [skill_id_4, skill_id_5], [0.3, 0.5])
+            self.USER_ID, {skill_id_4: 0.3, skill_id_5: 0.5})
 
-        degrees_of_mastery = skill_services.get_multi_skill_mastery(
+        degrees_of_mastery = skill_services.get_multi_user_skill_mastery(
             self.USER_ID, [skill_id_4, skill_id_5])
 
         self.assertEqual(
             degrees_of_mastery, {skill_id_4: 0.3, skill_id_5: 0.5})
 
-    def test_create_multi_user_skill_mastery_with_different_length_lists(self):
-        skill_id_4 = skill_services.get_new_skill_id()
-        skill_id_5 = skill_services.get_new_skill_id()
-        with self.assertRaisesRegexp(
-            Exception,
-            'skill_ids and degrees_of_mastery should have the same length.'):
-            skill_services.create_multi_user_skill_mastery(
-                self.USER_ID, [skill_id_4, skill_id_5], [0.3, 0.5, 0.8])
-
 
 # TODO(lilithxxx): Remove this mock class and tests for the mock skill
 # migrations once the actual functions are implemented.

diff --git a/feconf.py b/feconf.py
@@ -670,8 +670,8 @@ def get_empty_ratings():
 SKILL_EDITOR_URL_PREFIX = '/skill_editor'
 SKILL_EDITOR_QUESTION_URL = '/skill_editor_question_handler'
 SKILL_MASTERY_DATA_URL = '/skill_mastery_handler/data'
-SKILL_RIGHTS_URL_PREFIX = '/skill_editor_handler/rights'
 SKILL_PUBLISH_URL_PREFIX = '/skill_editor_handler/publish_skill'
+SKILL_RIGHTS_URL_PREFIX = '/skill_editor_handler/rights'
 SPLASH_URL = '/splash'
 STORY_DATA_HANDLER = '/story_data_handler'
 STORY_EDITOR_URL_PREFIX = '/story_editor'