From e2ca94e7d4c7cf0cf94ade19571477b7d80f0c2f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 20 Sep 2023 15:09:12 +0200 Subject: [PATCH] refactor: call skills api only when required The skills API was being called for each xblock causing performance issue. Now the API is only called if we cross the probablity check. --- CHANGELOG.rst | 9 +++++++++ skill_tagging/__init__.py | 2 +- skill_tagging/pipeline.py | 9 ++++++--- skill_tagging/skill_tagging_mixin.py | 2 +- tests/test_pipeline.py | 19 +++++++++++++++---- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6adab59..9a4e8f2 100644 --- a/CHANGELOG.rst +++ b/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 ************************************************ diff --git a/skill_tagging/__init__.py b/skill_tagging/__init__.py index 144ff08..e9ccabb 100644 --- a/skill_tagging/__init__.py +++ b/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' diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index 67b2217..2756768 100644 --- a/skill_tagging/pipeline.py +++ b/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) diff --git a/skill_tagging/skill_tagging_mixin.py b/skill_tagging/skill_tagging_mixin.py index f1e412b..5c1ac99 100644 --- a/skill_tagging/skill_tagging_mixin.py +++ b/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, diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 499fdba..c485649 100644 --- a/tests/test_pipeline.py +++ b/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="
") 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()