Merge pull request #19 from open-craft/navin/fix-performance-issues

refactor: call skills api only when required

CHANGELOG.rst

@@ -16,6 +16,15 @@ Unreleased
 
 *
 
+[0.1.3] - 2023-09-27
+************************************************
+
+Changed
+=======
+
+* Gate skills API call behind probablity check to reduce traffic.
+
+
 [0.1.2] - 2023-08-18
 ************************************************
 

skill_tagging/__init__.py

@@ -2,7 +2,7 @@
 Django app plugin for fetching and verifying tags for xblock skills.
 """
 
-__version__ = '0.1.2'
+__version__ = '0.1.3'
 
 # pylint: disable=invalid-name
 default_app_config = 'skill_tagging.apps.SkillTaggingConfig'

skill_tagging/pipeline.py

@@ -82,8 +82,11 @@ class AddVerticalBlockSkillVerificationSection(VerificationPipelineBase, Pipelin
     def run_filter(self, block, fragment, context, view):  # pylint: disable=arguments-differ
         """Pipeline Step implementing the Filter"""
 
+        # Check whether we need to run this filter and only call the API.
+        if not self.should_run_filter():
+            return {"block": block, "fragment": fragment, "context": context, "view": view}
         skills = self.fetch_related_skills(block)
-        if not skills or not self.should_run_filter():
+        if not skills:
             return {"block": block, "fragment": fragment, "context": context, "view": view}
         usage_id = block.scope_ids.usage_id
         data = self.get_skill_context(usage_id, block, skills)
@@ -116,11 +119,11 @@ class AddVideoBlockSkillVerificationComponent(VerificationPipelineBase, Pipeline
     def run_filter(self, block, context):  # pylint: disable=arguments-differ
         """Pipeline Step implementing the Filter"""
         usage_id = block.scope_ids.usage_id
-        if usage_id.block_type != "video":
+        if usage_id.block_type != "video" or not self.should_run_filter():
             # avoid fetching skills for other xblocks
             return {"block": block, "context": context}
         skills = self.fetch_related_skills(block)
-        if not skills or not self.should_run_filter():
+        if not skills:
             return {"block": block, "context": context}
         data = self.get_skill_context(usage_id, block, skills)
 

skill_tagging/skill_tagging_mixin.py

@@ -71,7 +71,7 @@ def fetch_skill_tags(self):
         usage_id_str = str(self.scope_ids.usage_id)
         XBLOCK_SKILL_TAGS_API = urljoin(
             settings.TAXONOMY_API_BASE_URL,
-            '/taxonomy/api/v1/xblocks'
+            '/taxonomy/api/v1/xblocks/'
         )
         response = api_client.get(
             XBLOCK_SKILL_TAGS_API,

tests/test_pipeline.py

@@ -35,14 +35,12 @@ def setUp(self) -> None:
         SHOW_SKILL_VERIFICATION_PROBABILITY=0,
     )
     def test_pipeline_does_nothing_when_probability_set_to_zero(self, mock_get_api_client):
-        """
-        Check that the input fragment is unchanged when there is no
-        configuration for a course.
-        """
+        """Check that pipeline is not executed if probability is set to zero"""
         mock_get_api_client.return_value = self.get_mock_api_response()
         _, fragment, _, _ = VerticalBlockRenderCompleted.run_filter(
             block=self.block, context={}, fragment=self.original_fragement, view={}
         )
+        mock_get_api_client.assert_not_called()
         self.assertEqual(fragment.content, self.original_fragement.content)
         self.assertNotIn("SKILL-0", fragment.content)
 
@@ -84,6 +82,19 @@ def setUp(self) -> None:
         self.original_fragement = Fragment(content="<div class='video-player'></div>")
         self.block.fragment = self.original_fragement
 
+    @override_settings(
+        SHOW_SKILL_VERIFICATION_PROBABILITY=0,
+    )
+    def test_pipeline_does_nothing_when_probability_set_to_zero(self, mock_get_api_client):
+        """Check that pipeline is not executed if probability is set to zero"""
+        mock_get_api_client.return_value = self.get_mock_api_response()
+        _, fragment, _, _ = VerticalBlockRenderCompleted.run_filter(
+            block=self.block, context={}, fragment=self.original_fragement, view={}
+        )
+        mock_get_api_client.assert_not_called()
+        self.assertEqual(fragment.content, self.original_fragement.content)
+        self.assertNotIn("SKILL-0", fragment.content)
+
     def test_pipeline_adds_required_resources(self, mock_get_api_client):
         """Check that pipeline adds required resources."""
         mock_get_api_client.return_value = self.get_mock_api_response()