Merge pull request #53 from edx/hammad/ENT-4202

ENT-4202 | Handled edge cases in `refresh_course_skills` command.

Author: HammadAhmadWaqas

CHANGELOG.rst

@@ -14,6 +14,11 @@ Change Log
 Unreleased
 --------------------
 
+[1.6.2] - 2021-03-12
+--------------------
+
+* Handled edge cases in `refresh_course_skills` command.
+
 [1.6.1] - 2021-03-10
 --------------------
 

taxonomy/__init__.py

@@ -15,6 +15,6 @@
 # 2. MINOR version when you add functionality in a backwards compatible manner, and
 # 3. PATCH version when you make backwards compatible bug fixes.
 # More details can be found at https://semver.org/
-__version__ = '1.6.1'
+__version__ = '1.6.2'
 
 default_app_config = 'taxonomy.apps.TaxonomyConfig'  # pylint: disable=invalid-name

taxonomy/management/commands/refresh_course_skills.py

@@ -90,15 +90,4 @@ def handle(self, *args, **options):
             raise InvalidCommandOptionsError('Either course or all argument must be provided.')
 
         LOGGER.info('[TAXONOMY] Refresh course skills process started.')
-        success_courses_count, skipped_courses_count, failures = utils.refresh_course_skills(courses, options['commit'])
-        LOGGER.info(
-            '[TAXONOMY] Refresh course skills process completed. \n'
-            'Failures: %s \n'
-            'Total Courses Updated Successfully: %s \n'
-            'Total Courses Skipped: %s \n'
-            'Total Failures: %s \n',
-            failures,
-            success_courses_count,
-            skipped_courses_count,
-            len(failures),
-        )
+        utils.refresh_course_skills(courses, options['commit'])

taxonomy/utils.py

@@ -76,20 +76,38 @@ def refresh_course_skills(courses, should_commit_to_db):
         if course_description:
             try:
                 course_skills = client.get_course_skills(course_description)
-                LOGGER.info('[TAXONOMY] Skills data received from EMSI. Skills: [%s]', course_skills)
+            except TaxonomyAPIError:
+                message = f'[TAXONOMY] API Error for course_key: {course["key"]}'
+                LOGGER.error(message)
+                all_failures.append((course['uuid'], message))
+                continue
+
+            try:
                 failures = process_skills_data(course, course_skills, should_commit_to_db)
                 if failures:
+                    LOGGER.info('[TAXONOMY] Skills data received from EMSI. Skills: [%s]', course_skills)
                     all_failures += failures
                 else:
                     success_courses_count += 1
-            except TaxonomyAPIError:
-                message = f'[TAXONOMY] API Error for course_key: {course["key"]}'
+            except Exception as ex:  # pylint: disable=broad-except
+                LOGGER.info('[TAXONOMY] Skills data received from EMSI. Skills: [%s]', course_skills)
+                message = f'[TAXONOMY] Exception for course_key: {course["key"]} Error: {ex}'
                 LOGGER.error(message)
                 all_failures.append((course['uuid'], message))
         else:
             skipped_courses_count += 1
 
-    return success_courses_count, skipped_courses_count, all_failures
+    LOGGER.info(
+        '[TAXONOMY] Refresh course skills process completed. \n'
+        'Failures: %s \n'
+        'Total Courses Updated Successfully: %s \n'
+        'Total Courses Skipped: %s \n'
+        'Total Failures: %s \n',
+        all_failures,
+        success_courses_count,
+        skipped_courses_count,
+        len(all_failures),
+    )
 
 
 def blacklist_course_skill(course_key, skill_id):

tests/management/test_refresh_course_skills.py

@@ -260,10 +260,10 @@ def test_course_skill_not_saved_for_key_error(self, get_course_skills_mock, get_
                 [
                     '[TAXONOMY] Refresh Course Skills. Options: [%s]',
                     '[TAXONOMY] Refresh course skills process started.',
-                    '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     f'[TAXONOMY] Missing keys in skills data for course_key: {self.course_1.key}',
                     '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     f'[TAXONOMY] Missing keys in skills data for course_key: {self.course_2.key}',
+                    '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     '[TAXONOMY] Refresh course skills process completed. \n'
                     'Failures: %s \n'
                     'Total Courses Updated Successfully: %s \n'
@@ -301,10 +301,10 @@ def test_course_skill_not_saved_for_type_error(self, get_course_skills_mock, get
                 [
                     '[TAXONOMY] Refresh Course Skills. Options: [%s]',
                     '[TAXONOMY] Refresh course skills process started.',
-                    '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     f'[TAXONOMY] Invalid type for `confidence` in course skills for course_key: {self.course_1.key}',
                     '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     f'[TAXONOMY] Invalid type for `confidence` in course skills for course_key: {self.course_2.key}',
+                    '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
                     '[TAXONOMY] Refresh course skills process completed. \n'
                     'Failures: %s \n'
                     'Total Courses Updated Successfully: %s \n'
@@ -315,3 +315,46 @@ def test_course_skill_not_saved_for_type_error(self, get_course_skills_mock, get
 
         self.assertEqual(skill.count(), 0)
         self.assertEqual(course_skill.count(), 0)
+
+    @responses.activate
+    @mock.patch('taxonomy.management.commands.refresh_course_skills.get_course_metadata_provider')
+    @mock.patch('taxonomy.management.commands.refresh_course_skills.utils.EMSISkillsApiClient.get_course_skills')
+    @mock.patch('taxonomy.management.commands.refresh_course_skills.utils.process_skills_data')
+    def test_course_skill_not_saved_for_exception(
+            self,
+            mock_process_skills_data,
+            get_course_skills_mock,
+            get_course_provider_mock,
+
+    ):
+        """
+        Test that the command does not create any records when a record value error occurs.
+        """
+        get_course_skills_mock.return_value = self.skills_emsi_client_response
+        get_course_provider_mock.return_value = DiscoveryCourseMetadataProvider([self.course_1])
+        mock_process_skills_data.side_effect = Exception("UNKNOWN ERROR.")
+        skill = Skill.objects.all()
+        course_skill = CourseSkills.objects.all()
+        self.assertEqual(skill.count(), 0)
+        self.assertEqual(course_skill.count(), 0)
+
+        with LogCapture(level=logging.INFO) as log_capture:
+            call_command(self.command, '--course', self.course_1.uuid, '--commit')
+            # Validate a descriptive and readable log message.
+            self.assertEqual(len(log_capture.records), 5)
+            messages = [record.msg for record in log_capture.records]
+            self.assertEqual(
+                messages,
+                ['[TAXONOMY] Refresh Course Skills. Options: [%s]',
+                 '[TAXONOMY] Refresh course skills process started.',
+                 '[TAXONOMY] Skills data received from EMSI. Skills: [%s]',
+                 f'[TAXONOMY] Exception for course_key: {self.course_1.key} Error: UNKNOWN ERROR.',
+                 '[TAXONOMY] Refresh course skills process completed. \n'
+                 'Failures: %s \n'
+                 'Total Courses Updated Successfully: %s \n'
+                 'Total Courses Skipped: %s \n'
+                 'Total Failures: %s \n']
+            )
+
+        self.assertEqual(skill.count(), 0)
+        self.assertEqual(course_skill.count(), 0)