From 05f66439fe060023fd484cf5e9493075c9df2319 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 27 Dec 2022 18:01:11 +0530 Subject: [PATCH 01/15] feat: implement pipeline skeleton --- requirements/base.in | 1 + requirements/base.txt | 3 ++ requirements/ci.txt | 2 +- requirements/dev.txt | 11 +++++--- requirements/doc.txt | 13 +++++---- requirements/quality.txt | 11 +++++--- requirements/test.txt | 11 +++++--- skill_tagging/pipeline.py | 46 +++++++++++++++++++++++++++++++ skill_tagging/static/tagging.html | 40 +++++++++++++++++++++++++++ 9 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 skill_tagging/pipeline.py create mode 100644 skill_tagging/static/tagging.html diff --git a/requirements/base.in b/requirements/base.in index 5288d49..5d40b3b 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -4,3 +4,4 @@ Django # Web application framework django-model-utils # Provides TimeStampedModel abstract base class git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba#egg=openedx_events +openedx-filters diff --git a/requirements/base.txt b/requirements/base.txt index 3298623..2a882da 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -14,6 +14,7 @@ django==3.2.16 # -r requirements/base.in # django-model-utils # openedx-events + # openedx-filters django-model-utils==4.3.1 # via -r requirements/base.in edx-opaque-keys[django]==2.3.0 @@ -22,6 +23,8 @@ fastavro==1.7.0 # via openedx-events openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/base.in +openedx-filters==0.8.0 + # via -r requirements/base.in pbr==5.11.0 # via stevedore pymongo==3.13.0 diff --git a/requirements/ci.txt b/requirements/ci.txt index 4038c91..7d2dab3 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -10,7 +10,7 @@ charset-normalizer==2.1.1 # via requests codecov==2.1.12 # via -r requirements/ci.in -coverage==7.0.0 +coverage==7.0.1 # via codecov distlib==0.3.6 # via virtualenv diff --git a/requirements/dev.txt b/requirements/dev.txt index acc6aed..6d2e32a 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -34,11 +34,11 @@ boto==2.49.0 # via # -r requirements/quality.txt # xblock-sdk -boto3==1.26.35 +boto3==1.26.37 # via # -r requirements/quality.txt # fs-s3fs -botocore==1.29.35 +botocore==1.29.37 # via # -r requirements/quality.txt # boto3 @@ -85,7 +85,7 @@ cookiecutter==2.1.1 # via # -r requirements/quality.txt # xblock-sdk -coverage[toml]==7.0.0 +coverage[toml]==7.0.1 # via # -r requirements/ci.txt # -r requirements/quality.txt @@ -110,6 +110,7 @@ django==3.2.16 # djangorestframework # edx-i18n-tools # openedx-events + # openedx-filters # xblock-sdk django-model-utils==4.3.1 # via -r requirements/quality.txt @@ -127,7 +128,7 @@ edx-opaque-keys[django]==2.3.0 # via # -r requirements/quality.txt # openedx-events -exceptiongroup==1.0.4 +exceptiongroup==1.1.0 # via # -r requirements/quality.txt # pytest @@ -206,6 +207,8 @@ mock==4.0.3 # via -r requirements/quality.txt openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/quality.txt +openedx-filters==0.8.0 + # via -r requirements/quality.txt packaging==22.0 # via # -r requirements/ci.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index d6986c2..9864ecb 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -35,11 +35,11 @@ boto==2.49.0 # via # -r requirements/test.txt # xblock-sdk -boto3==1.26.35 +boto3==1.26.37 # via # -r requirements/test.txt # fs-s3fs -botocore==1.29.35 +botocore==1.29.37 # via # -r requirements/test.txt # boto3 @@ -73,7 +73,7 @@ cookiecutter==2.1.1 # via # -r requirements/test.txt # xblock-sdk -coverage[toml]==7.0.0 +coverage[toml]==7.0.1 # via # -r requirements/test.txt # pytest-cov @@ -87,6 +87,7 @@ django==3.2.16 # django-pyfs # djangorestframework # openedx-events + # openedx-filters # xblock-sdk django-model-utils==4.3.1 # via -r requirements/test.txt @@ -96,7 +97,7 @@ django-pyfs==3.2.0 # xblock-sdk djangorestframework==3.14.0 # via -r requirements/test.txt -doc8==1.0.0 +doc8==1.1.1 # via -r requirements/doc.in docutils==0.19 # via @@ -108,7 +109,7 @@ edx-opaque-keys[django]==2.3.0 # via # -r requirements/test.txt # openedx-events -exceptiongroup==1.0.4 +exceptiongroup==1.1.0 # via # -r requirements/test.txt # pytest @@ -188,6 +189,8 @@ more-itertools==9.0.0 # via jaraco-classes openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/test.txt +openedx-filters==0.8.0 + # via -r requirements/test.txt packaging==22.0 # via # -r requirements/test.txt diff --git a/requirements/quality.txt b/requirements/quality.txt index efb01d5..9e657de 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -33,11 +33,11 @@ boto==2.49.0 # via # -r requirements/test.txt # xblock-sdk -boto3==1.26.35 +boto3==1.26.37 # via # -r requirements/test.txt # fs-s3fs -botocore==1.29.35 +botocore==1.29.37 # via # -r requirements/test.txt # boto3 @@ -71,7 +71,7 @@ cookiecutter==2.1.1 # via # -r requirements/test.txt # xblock-sdk -coverage[toml]==7.0.0 +coverage[toml]==7.0.1 # via # -r requirements/test.txt # pytest-cov @@ -85,6 +85,7 @@ django==3.2.16 # django-pyfs # djangorestframework # openedx-events + # openedx-filters # xblock-sdk django-model-utils==4.3.1 # via -r requirements/test.txt @@ -100,7 +101,7 @@ edx-opaque-keys[django]==2.3.0 # via # -r requirements/test.txt # openedx-events -exceptiongroup==1.0.4 +exceptiongroup==1.1.0 # via # -r requirements/test.txt # pytest @@ -168,6 +169,8 @@ mock==4.0.3 # via -r requirements/test.txt openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/test.txt +openedx-filters==0.8.0 + # via -r requirements/test.txt packaging==22.0 # via # -r requirements/test.txt diff --git a/requirements/test.txt b/requirements/test.txt index 0f195c6..c7e1619 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -21,9 +21,9 @@ binaryornot==0.4.4 # via cookiecutter boto==2.49.0 # via xblock-sdk -boto3==1.26.35 +boto3==1.26.37 # via fs-s3fs -botocore==1.29.35 +botocore==1.29.37 # via # boto3 # s3transfer @@ -41,7 +41,7 @@ code-annotations==1.3.0 # via -r requirements/test.in cookiecutter==2.1.1 # via xblock-sdk -coverage[toml]==7.0.0 +coverage[toml]==7.0.1 # via pytest-cov # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt @@ -50,6 +50,7 @@ coverage[toml]==7.0.0 # django-pyfs # djangorestframework # openedx-events + # openedx-filters # xblock-sdk django-model-utils==4.3.1 # via -r requirements/base.txt @@ -61,7 +62,7 @@ edx-opaque-keys[django]==2.3.0 # via # -r requirements/base.txt # openedx-events -exceptiongroup==1.0.4 +exceptiongroup==1.1.0 # via pytest fastavro==1.7.0 # via @@ -105,6 +106,8 @@ mock==4.0.3 # via -r requirements/test.in openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/base.txt +openedx-filters==0.8.0 + # via -r requirements/base.txt packaging==22.0 # via pytest pbr==5.11.0 diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py new file mode 100644 index 0000000..ebd7861 --- /dev/null +++ b/skill_tagging/pipeline.py @@ -0,0 +1,46 @@ +""" +Module that contains the openedx_filters pipeline steps. +""" +import logging +import pkg_resources + +from django.template import Context, Template +from openedx_filters import PipelineStep + +logger = logging.getLogger(__name__) + + +class AddVerticalBlockSkillVerificationSection(PipelineStep): + """ + Adds extra HTML to the fragment. + + Example Usage: + + .. code-block:: + + "OPENEDX_FILTERS_CONFIG": { + "org.openedx.learning.vertical_block.render.completed.v1": { + "fail_sliently": false, + "pipeline": [ + "skill_tagging.pipeline.AddVerticalBlockSkillVerificationSection" + ] + } + } + """ + def resource_string(self, path): + """Handy helper for getting resources from our kit.""" + data = pkg_resources.resource_string(__name__, path) + return data.decode("utf8") + + def run_filter(self, block, fragment, context, view): + """Pipeline Step implementing the Filter""" + + fetch_url = block.runtime.handler_url(block, "fetch_tags") + verify_tags_url = block.runtime.handler_url(block, "verify_tags") + html = self.resource_string("static/tagging.html") + data = {"fetch_tags_url": fetch_url, "verify_tags_url": verify_tags_url} + template = Template(html) + context = Context(data) + tags_div = template.render(context) + fragment.content = f"{fragment.content}{tags_div}" + return {"block": block, "fragment": fragment, "context": context, "view": view} diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html new file mode 100644 index 0000000..3648e78 --- /dev/null +++ b/skill_tagging/static/tagging.html @@ -0,0 +1,40 @@ +
+
Debug Info
+ +

+
+ + + From c6d8830a84487a626269be67600b504fcf1c9fa8 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 28 Dec 2022 19:53:52 +0530 Subject: [PATCH 02/15] feat: first working implementation --- skill_tagging/pipeline.py | 15 +++- skill_tagging/static/brainstorming.svg | 1 + skill_tagging/static/tagging.css | 76 +++++++++++++++++++++ skill_tagging/static/tagging.html | 72 ++++++++++---------- skill_tagging/static/tagging.js | 94 ++++++++++++++++++++++++++ 5 files changed, 219 insertions(+), 39 deletions(-) create mode 100644 skill_tagging/static/brainstorming.svg create mode 100644 skill_tagging/static/tagging.css create mode 100644 skill_tagging/static/tagging.js diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index ebd7861..dacfe32 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -35,11 +35,22 @@ def resource_string(self, path): def run_filter(self, block, fragment, context, view): """Pipeline Step implementing the Filter""" + print("====================================================================================================") + print(getattr(block, "has_verified_tags")) + print("====================================================================================================") fetch_url = block.runtime.handler_url(block, "fetch_tags") verify_tags_url = block.runtime.handler_url(block, "verify_tags") html = self.resource_string("static/tagging.html") - data = {"fetch_tags_url": fetch_url, "verify_tags_url": verify_tags_url} - template = Template(html) + css = self.resource_string("static/tagging.css") + js = self.resource_string("static/tagging.js") + image = self.resource_string("static/brainstorming.svg") + data = { + "fetch_tags_url": fetch_url, + "verify_tags_url": verify_tags_url, + "image": image, + } + template_str = f'{html}' + template = Template(template_str) context = Context(data) tags_div = template.render(context) fragment.content = f"{fragment.content}{tags_div}" diff --git a/skill_tagging/static/brainstorming.svg b/skill_tagging/static/brainstorming.svg new file mode 100644 index 0000000..5c1dcbe --- /dev/null +++ b/skill_tagging/static/brainstorming.svg @@ -0,0 +1 @@ + diff --git a/skill_tagging/static/tagging.css b/skill_tagging/static/tagging.css new file mode 100644 index 0000000..115d06f --- /dev/null +++ b/skill_tagging/static/tagging.css @@ -0,0 +1,76 @@ +.tag-verification-container { + margin-top: 20px; + background: #F2F0EF; + border-radius: 6px; + padding: 70px 65px; + display: flex; +} + +.tag-verification-container h1 { + font-style: normal; + font-weight: 700; + font-size: 32px; + line-height: 28px; + margin-bottom: 20px; +} + +.tag-verification-container p { + font-style: normal; + font-weight: 400; + font-size: 18px; + line-height: 28px; +} +.tag-verification-tags-container { + flex-direction: row; + align-items: center; + display: flex; + gap: 11px; + margin-bottom: 20px; + flex-wrap: wrap; +} + +.tag-verification-chip { + display: inline-block; + background-color: white; + font-size: 13px; + padding: 2px 8px; + box-sizing: border-box; + border: 1px solid #F2F0EF; + border-radius: 6px; + color: #002121; + font-style: normal; + font-weight: 400; + font-size: 12px; + line-height: 24px; +} + +.tag-verification-chip.tag-verification-chip-hover:hover { + background: #ccc; +} + +.tag-verification-chip-clickable { + cursor: pointer; +} + +.tag-verification-tags-container input[type=checkbox] { + display: none; +} + +.tag-verification-tags-container input[type=checkbox]:checked + label { + border: 1px solid #00262B; +} + +.tag-verification-none-selected-container { + margin-bottom: 25px; + height: 28px; +} + +.tag-verification-none-selected-container label { + font-weight: 400; + font-size: 14px; + line-height: 28px; +} + +.tag-verification-none-selected-container input { + margin-right: 8px; +} diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index 3648e78..d4e5645 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -1,40 +1,38 @@ -
-
Debug Info
- -

+
+
+

What did you learn?

+

+ Congratulations, you've reached the end of this unit!
+ Help future students by selecting the skills or subjects you learned from this unit. +

+
+
+
+ + +
+ +
+
+ {{ image | safe }} +
+ - + diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js new file mode 100644 index 0000000..d46a3b9 --- /dev/null +++ b/skill_tagging/static/tagging.js @@ -0,0 +1,94 @@ +const tagSkillContainer = document.getElementById("tag-verification-tags-container-id"); +var tagSkillSelectedTags = []; + +function tagVerificationSetNoneToFalse(source) { + if (!source.checked) { + return; + } + checkbox = document.getElementById("tagVerificationUnselectAllId"); + checkbox.checked = false; +} + +function tagVerficationCreateCheckbox(skillValue) { + var checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + checkbox.id = skillValue; + checkbox.name = 'tag-verification-skills'; + checkbox.value = skillValue; + checkbox.setAttribute("onclick", "tagVerificationSetNoneToFalse(this)"); + + var label = document.createElement('label') + label.htmlFor = skillValue; + label.className = "tag-verification-chip tag-verification-chip-clickable tag-verification-chip-hover" + label.appendChild(document.createTextNode(skillValue)); + tagSkillContainer.appendChild(checkbox); + tagSkillContainer.appendChild(label); +} + +function tagVerificationFetchTags(url) { + var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; + + fetch(url, { + method: "POST", + body: JSON.stringify([]), + headers: { + "Content-Type": "application/json", + "X-CSRFToken": csrf_token, + } + }) + .then(res => res.json()) + .then((data) => { + data.forEach((skill) => tagVerficationCreateCheckbox(skill.name)); + }); +} + +tagVerificationFetchTags('{{ fetch_tags_url }}') + +function tagVerificationVerifyTags(url) { + var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; + var checkboxes = document.getElementsByName('tag-verification-skills'); + for(var i=0, n=checkboxes.length; i res.json()) + .then(() => { + document.querySelector("#tag-verification-action-id").style.display = "none"; + document.querySelector("#tag-verification-thankyou-container").style.display = "flex"; + }) + .catch(() => { + document.querySelector("#tag-verification-action-id").style.display = "none"; + document.querySelector("#tag-verification-thankyou-container").style.display = "none"; + document.querySelector("#tag-verification-error-container").style.display = "flex"; + }); +} + +function tagVerificationUnselectAll(source) { + if (!source.checked) { + return; + } + checkboxes = document.getElementsByName('tag-verification-skills'); + for(var i=0, n=checkboxes.length;i Date: Thu, 29 Dec 2022 22:31:57 +0530 Subject: [PATCH 03/15] refactor: simplify mixin and implement check for pipeline --- skill_tagging/pipeline.py | 19 ++++++-- skill_tagging/skill_tagging_mixin.py | 27 +++-------- skill_tagging/static/tagging.html | 69 +++++++++++++++++----------- skill_tagging/static/tagging.js | 48 +++++-------------- 4 files changed, 73 insertions(+), 90 deletions(-) diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index dacfe32..2ebd0ee 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -32,20 +32,29 @@ def resource_string(self, path): data = pkg_resources.resource_string(__name__, path) return data.decode("utf8") + def fetch_related_skills(self, block): + has_verified_tags = getattr(block, "has_verified_tags", None) + if has_verified_tags is None or has_verified_tags is True: + return [] + fetch_tags = getattr(block, "fetch_skill_tags", None) + if fetch_tags is None: + return [] + tags = fetch_tags() + return tags + def run_filter(self, block, fragment, context, view): """Pipeline Step implementing the Filter""" - print("====================================================================================================") - print(getattr(block, "has_verified_tags")) - print("====================================================================================================") - fetch_url = block.runtime.handler_url(block, "fetch_tags") + skills = self.fetch_related_skills(block) + if not skills: + return {"block": block, "fragment": fragment, "context": context, "view": view} verify_tags_url = block.runtime.handler_url(block, "verify_tags") html = self.resource_string("static/tagging.html") css = self.resource_string("static/tagging.css") js = self.resource_string("static/tagging.js") image = self.resource_string("static/brainstorming.svg") data = { - "fetch_tags_url": fetch_url, + "skills": skills, "verify_tags_url": verify_tags_url, "image": image, } diff --git a/skill_tagging/skill_tagging_mixin.py b/skill_tagging/skill_tagging_mixin.py index 4ec21b1..8c1c4f4 100644 --- a/skill_tagging/skill_tagging_mixin.py +++ b/skill_tagging/skill_tagging_mixin.py @@ -35,7 +35,7 @@ class SkillTaggingMixin: scope=Scope.user_state ) - def _fetch_skill_tags(self): + def fetch_skill_tags(self): """ Fetch skill tags for the XBlock by calling taxonomy api. """ @@ -79,32 +79,19 @@ def _fetch_skill_tags(self): return results[0].get('skills', []) @XBlock.json_handler - def fetch_tags(self, data, suffix=''): # pylint: disable=unused-argument - """ - Handler for fetching skill tags associated with this XBlock - """ - return self._fetch_skill_tags() - - @XBlock.json_handler - def verify_tags(self, tags, suffix=''): # pylint: disable=unused-argument + def verify_tags(self, data, suffix=''): # pylint: disable=unused-argument """ Handler to verify tags """ + verified_skills = data.get("verified_skills", []) + ignored_skills = data.get("verified_skills", []) usage_key = str(self.scope_ids.usage_id) - verified_skill_ids = [] - ignored_skill_ids = [] - if not self.has_verified_tags: - skills = self._fetch_skill_tags() - for skill in skills: - if skill['name'] in tags: - verified_skill_ids.append(skill['id']) - else: - ignored_skill_ids.append(skill['id']) + if not self.has_verified_tags and (verified_skills or ignored_skills): XBLOCK_SKILL_VERIFIED.send_event( xblock_info=XBlockSkillVerificationData( usage_key=usage_key, - verified_skills=verified_skill_ids, - ignored_skills=ignored_skill_ids, + verified_skills=verified_skills, + ignored_skills=ignored_skills, ) ) self.has_verified_tags = True diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index d4e5645..750ee82 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -1,38 +1,51 @@
-
-

What did you learn?

-

- Congratulations, you've reached the end of this unit!
- Help future students by selecting the skills or subjects you learned from this unit. -

-
+
+

What did you learn?

+

+ Congratulations, you've reached the end of this unit!
+ Help future students by selecting the skills or subjects you learned from this unit. +

+
+ {% for skill in skills %} + + + {% endfor %} +
+
+ + +
+
-
- - +
+ {{ image | safe }}
- -
-
- {{ image | safe }} -
diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index d46a3b9..c723563 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -1,5 +1,6 @@ const tagSkillContainer = document.getElementById("tag-verification-tags-container-id"); var tagSkillSelectedTags = []; +var tagSkillIgnoredTags = []; function tagVerificationSetNoneToFalse(source) { if (!source.checked) { @@ -9,52 +10,25 @@ function tagVerificationSetNoneToFalse(source) { checkbox.checked = false; } -function tagVerficationCreateCheckbox(skillValue) { - var checkbox = document.createElement('input'); - checkbox.type = 'checkbox'; - checkbox.id = skillValue; - checkbox.name = 'tag-verification-skills'; - checkbox.value = skillValue; - checkbox.setAttribute("onclick", "tagVerificationSetNoneToFalse(this)"); - - var label = document.createElement('label') - label.htmlFor = skillValue; - label.className = "tag-verification-chip tag-verification-chip-clickable tag-verification-chip-hover" - label.appendChild(document.createTextNode(skillValue)); - tagSkillContainer.appendChild(checkbox); - tagSkillContainer.appendChild(label); -} - -function tagVerificationFetchTags(url) { - var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; - - fetch(url, { - method: "POST", - body: JSON.stringify([]), - headers: { - "Content-Type": "application/json", - "X-CSRFToken": csrf_token, - } - }) - .then(res => res.json()) - .then((data) => { - data.forEach((skill) => tagVerficationCreateCheckbox(skill.name)); - }); -} - -tagVerificationFetchTags('{{ fetch_tags_url }}') - function tagVerificationVerifyTags(url) { var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; var checkboxes = document.getElementsByName('tag-verification-skills'); + // clear containers + tagSkillSelectedTags = []; + tagSkillIgnoredTags = []; for(var i=0, n=checkboxes.length; i Date: Fri, 30 Dec 2022 15:03:38 +0530 Subject: [PATCH 04/15] fix: make skill chips accessible --- skill_tagging/skill_tagging_mixin.py | 3 +- skill_tagging/static/tagging.css | 115 +++++++++++++++++---------- skill_tagging/static/tagging.html | 6 +- skill_tagging/static/tagging.js | 5 ++ 4 files changed, 84 insertions(+), 45 deletions(-) diff --git a/skill_tagging/skill_tagging_mixin.py b/skill_tagging/skill_tagging_mixin.py index 8c1c4f4..e36a3e7 100644 --- a/skill_tagging/skill_tagging_mixin.py +++ b/skill_tagging/skill_tagging_mixin.py @@ -83,8 +83,9 @@ def verify_tags(self, data, suffix=''): # pylint: disable=unused-argument """ Handler to verify tags """ + verified_skills = data.get("verified_skills", []) - ignored_skills = data.get("verified_skills", []) + ignored_skills = data.get("ignored_skills", []) usage_key = str(self.scope_ids.usage_id) if not self.has_verified_tags and (verified_skills or ignored_skills): XBLOCK_SKILL_VERIFIED.send_event( diff --git a/skill_tagging/static/tagging.css b/skill_tagging/static/tagging.css index 115d06f..686ab84 100644 --- a/skill_tagging/static/tagging.css +++ b/skill_tagging/static/tagging.css @@ -1,76 +1,107 @@ .tag-verification-container { - margin-top: 20px; - background: #F2F0EF; - border-radius: 6px; - padding: 70px 65px; - display: flex; + margin-top: 20px; + background: #F2F0EF; + border-radius: 6px; + padding: 70px 65px; + display: flex; } .tag-verification-container h1 { - font-style: normal; - font-weight: 700; - font-size: 32px; - line-height: 28px; - margin-bottom: 20px; + font-style: normal; + font-weight: 700; + font-size: 32px; + line-height: 28px; + margin-bottom: 20px; +} + +.tag-verification-container button { + background: #00262B; + padding: 10px 16px; + font-weight: 500; + font-size: 18px; + line-height: 24px; + color: #FFFFFF; + box-shadow: none; +} + +.tag-verification-container button:hover, .tag-verification-container button:focus { + background: #00262B; + color: #FFFFFF; + border: 1px solid #00262B; + box-shadow: none; } .tag-verification-container p { - font-style: normal; - font-weight: 400; - font-size: 18px; - line-height: 28px; + font-style: normal; + font-weight: 400; + font-size: 18px; + line-height: 28px; } .tag-verification-tags-container { - flex-direction: row; - align-items: center; - display: flex; - gap: 11px; - margin-bottom: 20px; - flex-wrap: wrap; + flex-direction: row; + align-items: center; + display: flex; + gap: 11px; + margin-bottom: 20px; + flex-wrap: wrap; } .tag-verification-chip { - display: inline-block; - background-color: white; - font-size: 13px; - padding: 2px 8px; - box-sizing: border-box; - border: 1px solid #F2F0EF; - border-radius: 6px; - color: #002121; - font-style: normal; - font-weight: 400; - font-size: 12px; - line-height: 24px; + display: inline-block; + background-color: white; + font-size: 13px; + padding: 2px 8px; + box-sizing: border-box; + border: 1px solid #F2F0EF; + border-radius: 6px; + color: #002121; + font-style: normal; + font-weight: 400; + font-size: 12px; + line-height: 24px; } .tag-verification-chip.tag-verification-chip-hover:hover { - background: #ccc; + background: #ccc; } .tag-verification-chip-clickable { - cursor: pointer; + cursor: pointer; } .tag-verification-tags-container input[type=checkbox] { - display: none; + position: absolute; + /* set same dimensions as the SVG */ + width: 1em; + height: 1em; + /* hide it */ + opacity: 0.00000; } .tag-verification-tags-container input[type=checkbox]:checked + label { - border: 1px solid #00262B; + border: 1px solid #00262B; +} + +.tag-verification-tags-container input[type=checkbox]:focus + label { + background: #ccc; } .tag-verification-none-selected-container { - margin-bottom: 25px; - height: 28px; + margin-bottom: 25px; + height: 28px; + display: flex; + align-items: center; } .tag-verification-none-selected-container label { - font-weight: 400; - font-size: 14px; - line-height: 28px; + font-weight: 400; + font-size: 14px; + line-height: 28px; + margin: 0px; } .tag-verification-none-selected-container input { - margin-right: 8px; + margin-right: 8px; + width: 18px; + height: 18px; } diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index 750ee82..97611c1 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -17,7 +17,9 @@

What did you learn?

+ > + {{ skill.name }} + {% endfor %}
@@ -26,7 +28,7 @@

What did you learn?

None of these subjects were covered
- +
{{ image | safe }} diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index c723563..200992e 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -13,6 +13,7 @@ function tagVerificationSetNoneToFalse(source) { function tagVerificationVerifyTags(url) { var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; var checkboxes = document.getElementsByName('tag-verification-skills'); + var noneSelectedCheckbox = document.getElementById('tagVerificationUnselectAllId'); // clear containers tagSkillSelectedTags = []; tagSkillIgnoredTags = []; @@ -23,6 +24,10 @@ function tagVerificationVerifyTags(url) { tagSkillIgnoredTags.push(parseInt(checkboxes[i].value)); } } + if (tagSkillSelectedTags.length === 0 && !noneSelectedCheckbox.checked) { + alert("Please select atleast one skill or check 'None of these subjects were covered' checkbox!"); + return; + } fetch(url, { method: "POST", body: JSON.stringify({ From e1487937fe37ed7fec2739472ec6b8ebd8a65b11 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 15:15:02 +0530 Subject: [PATCH 05/15] refactor: fix formatting --- skill_tagging/static/tagging.css | 12 ++++++------ skill_tagging/static/tagging.html | 20 +++++++------------- skill_tagging/static/tagging.js | 9 ++------- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/skill_tagging/static/tagging.css b/skill_tagging/static/tagging.css index 686ab84..1bdd803 100644 --- a/skill_tagging/static/tagging.css +++ b/skill_tagging/static/tagging.css @@ -70,12 +70,12 @@ } .tag-verification-tags-container input[type=checkbox] { - position: absolute; - /* set same dimensions as the SVG */ - width: 1em; - height: 1em; - /* hide it */ - opacity: 0.00000; + position: absolute; + /* set same dimensions as the SVG */ + width: 1em; + height: 1em; + /* hide it */ + opacity: 0.00000; } .tag-verification-tags-container input[type=checkbox]:checked + label { diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index 97611c1..c0741ee 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -3,21 +3,14 @@

What did you learn?

Congratulations, you've reached the end of this unit!
- Help future students by selecting the skills or subjects you learned from this unit. + Help future students by selecting the skills or subjects you learned from this unit.

{% for skill in skills %} - -
diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index 200992e..a013c19 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -17,7 +17,7 @@ function tagVerificationVerifyTags(url) { // clear containers tagSkillSelectedTags = []; tagSkillIgnoredTags = []; - for(var i=0, n=checkboxes.length; i Date: Fri, 30 Dec 2022 15:40:22 +0530 Subject: [PATCH 06/15] feat: add option to configure probability of displaying the form --- README.rst | 9 ++++----- skill_tagging/pipeline.py | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/README.rst b/README.rst index 5e17b24..c3d55c5 100644 --- a/README.rst +++ b/README.rst @@ -16,11 +16,10 @@ Purpose Django app plugin for fetching and verifying tags for xblock skills. -TODO: The ``README.rst`` file should start with a brief description of the repository and its purpose. -It should be described in the context of other repositories under the ``openedx`` -organization. It should make clear where this fits in to the overall Open edX -codebase and should be oriented towards people who are new to the Open edX -project. +Set ``SHOW_SKILL_VERIFICATION_PROBABILITY`` in your django settings to configure +probability of displaying verification form. Values in range 0 to 1 are +allowed, where 0 means never and 1 means always display. Default value is 0.5 +i.e. 50% chance of displaying the form. Getting Started *************** diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index 2ebd0ee..f78ee07 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -2,12 +2,15 @@ Module that contains the openedx_filters pipeline steps. """ import logging -import pkg_resources +import random +import pkg_resources +from django.conf import settings from django.template import Context, Template from openedx_filters import PipelineStep logger = logging.getLogger(__name__) +SHOW_SKILL_VERIFICATION_PROBABILITY = getattr(settings, "SHOW_SKILL_VERIFICATION_PROBABILITY", 0.5) class AddVerticalBlockSkillVerificationSection(PipelineStep): @@ -33,6 +36,7 @@ def resource_string(self, path): return data.decode("utf8") def fetch_related_skills(self, block): + """Checks `has_verified_tags` and fetchs related skills.""" has_verified_tags = getattr(block, "has_verified_tags", None) if has_verified_tags is None or has_verified_tags is True: return [] @@ -42,11 +46,19 @@ def fetch_related_skills(self, block): tags = fetch_tags() return tags + def should_run_filter(self, skills): + """Determines whether we should run filter and display form.""" + # return false if no skills found. + if not skills: + return False + # random returns a number between 0 and 1 (inclusive). + return random.random() < SHOW_SKILL_VERIFICATION_PROBABILITY + def run_filter(self, block, fragment, context, view): """Pipeline Step implementing the Filter""" skills = self.fetch_related_skills(block) - if not skills: + if not self.should_run_filter(skills): return {"block": block, "fragment": fragment, "context": context, "view": view} verify_tags_url = block.runtime.handler_url(block, "verify_tags") html = self.resource_string("static/tagging.html") From cb1e275e226c6777a2017005b6596d3c4571f271 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 15:53:35 +0530 Subject: [PATCH 07/15] fix: mixin tests --- tests/test_skill_tagging_mixin.py | 40 +++++++------------------------ 1 file changed, 9 insertions(+), 31 deletions(-) diff --git a/tests/test_skill_tagging_mixin.py b/tests/test_skill_tagging_mixin.py index dcd7af9..e07989d 100644 --- a/tests/test_skill_tagging_mixin.py +++ b/tests/test_skill_tagging_mixin.py @@ -48,7 +48,7 @@ def test_mixin_fields(self, mock_get_api_client): """ self.tagging_mixin = get_tagging_mixin() self.assertTrue(hasattr(self.tagging_mixin, 'has_verified_tags')) - self.assertTrue(hasattr(self.tagging_mixin, 'fetch_tags')) + self.assertTrue(hasattr(self.tagging_mixin, 'fetch_skill_tags')) self.assertTrue(hasattr(self.tagging_mixin, 'verify_tags')) def test_fetch_tags(self, mock_get_api_client): @@ -79,7 +79,7 @@ def test_fetch_tags(self, mock_get_api_client): ) ) mock_get_api_client.return_value = api_client - resp = self.call_handler(self.FETCH_TAGS_HANDLER, data={}) + resp = self.block.fetch_skill_tags() assert mock_get_api_client.call_count == 1 assert api_client.get.call_count == 1 expected_response = [ @@ -89,39 +89,17 @@ def test_fetch_tags(self, mock_get_api_client): self.assertEqual(resp, expected_response) @patch('skill_tagging.skill_tagging_mixin.XBLOCK_SKILL_VERIFIED') - def test_verify_tags(self, mock_event, mock_get_api_client): + def test_verify_tags(self, mock_event, _): """ Test that verify_tags method works as expected """ - sample_output = { - "results": [{ - "id": 1, - "skills": [ - { - "id": 1, - "name": "SKILL-0" - }, - { - "id": 6, - "name": "SKILL-5" - }, - ], - }] - } - api_client = Mock( - get=Mock( - return_value=self._mock_response( - status.HTTP_200_OK, - sample_output - ) - ) - ) - mock_get_api_client.return_value = api_client mock_event.return_value = Mock(send_event=Mock()) - tags = ['SKILL-0', 'SKILL-5'] + selected_tags = [1, 6] + ignored_tags = [] self.assertFalse(self.block.has_verified_tags) - self.call_handler(self.VERIFY_TAGS_HANDLER, data=tags) - assert mock_get_api_client.call_count == 1 - assert api_client.get.call_count == 1 + self.call_handler(self.VERIFY_TAGS_HANDLER, data={ + "verified_skills": selected_tags, + "ignored_skills": ignored_tags, + }) assert mock_event.send_event.call_count == 1 self.assertTrue(self.block.has_verified_tags) From c9f0b5d05929a44395e8e76b96640d198a43de3d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 16:21:10 +0530 Subject: [PATCH 08/15] fix: tests and lint setup --- Makefile | 10 ++++----- skill_tagging/pipeline.py | 2 +- tests/utils.py => test_utils.py | 0 tests/__init__.py | 0 tests/test_skill_tagging_mixin.py | 2 +- tox.ini | 36 +++++++++++++++++++++++++------ 6 files changed, 35 insertions(+), 15 deletions(-) rename tests/utils.py => test_utils.py (100%) delete mode 100644 tests/__init__.py diff --git a/Makefile b/Makefile index 40befcf..d577d74 100644 --- a/Makefile +++ b/Makefile @@ -63,19 +63,17 @@ piptools: ## install pinned version of pip-compile and pip-sync requirements: piptools ## install development environment requirements pip-sync -q requirements/dev.txt requirements/private.* -test: ## Run the tests - mkdir -p var - rm -rf .coverage - python -m coverage run --rcfile=.coveragerc ./test.py --noinput +test: clean ## run tests in the current virtualenv + DJANGO_SETTINGS_MODULE=test_settings pytest diff_cover: test ## find diff lines that need test coverage diff-cover coverage.xml -test-all: quality pii_check ## run tests on every supported Python/Django combination +test-all: quality ## run tests on every supported Python/Django combination tox tox -e docs -validate: quality pii_check test ## run tests and quality checks +validate: quality test ## run tests and quality checks selfcheck: ## check that the Makefile is well-formed @echo "The Makefile is well-formed." diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index f78ee07..26657bd 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -54,7 +54,7 @@ def should_run_filter(self, skills): # random returns a number between 0 and 1 (inclusive). return random.random() < SHOW_SKILL_VERIFICATION_PROBABILITY - def run_filter(self, block, fragment, context, view): + def run_filter(self, block, fragment, context, view): # pylint: disable=arguments-differ """Pipeline Step implementing the Filter""" skills = self.fetch_related_skills(block) diff --git a/tests/utils.py b/test_utils.py similarity index 100% rename from tests/utils.py rename to test_utils.py diff --git a/tests/__init__.py b/tests/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/tests/test_skill_tagging_mixin.py b/tests/test_skill_tagging_mixin.py index e07989d..dbf6d1f 100644 --- a/tests/test_skill_tagging_mixin.py +++ b/tests/test_skill_tagging_mixin.py @@ -10,7 +10,7 @@ from requests.models import Response from rest_framework import status -from .utils import TestCaseMixin, get_tagging_mixin, make_block +from test_utils import TestCaseMixin, get_tagging_mixin, make_block @patch('skill_tagging.skill_tagging_mixin.get_api_client') diff --git a/tox.ini b/tox.ini index 3a1af48..c5f5569 100644 --- a/tox.ini +++ b/tox.ini @@ -1,26 +1,48 @@ [tox] envlist = py38-django{32,40}, quality +[pycodestyle] +exclude = .git,.tox,migrations +max-line-length = 120 + +[isort] +line_length = 120 +known_edx = +known_django = django +known_djangoapp = model_utils +known_first_party = taxonomy +include_trailing_comma = true +multi_line_output = 3 +sections = FUTURE,STDLIB,THIRDPARTY,DJANGO,DJANGOAPP,EDX,FIRSTPARTY,LOCALFOLDER + +[pytest] +DJANGO_SETTINGS_MODULE = test_settings +addopts = --cov skill_tagging --cov-report term-missing --cov-report xml +norecursedirs = .* docs requirements site-packages + [testenv] -whitelist_externals = +whitelist_externals = make -deps = +deps = django32: Django>=3.2,<4.0 django40: Django>=4.0,<4.1 -r{toxinidir}/requirements/test.txt -commands = - make test +commands = + pytest {posargs} [testenv:quality] -whitelist_externals = +whitelist_externals = make rm touch -deps = + pylint + pycodestyle + isort +deps = -r{toxinidir}/requirements/quality.txt -commands = +commands = touch tests/__init__.py pylint skill_tagging tests rm tests/__init__.py From fb66c227bb0756ff64d8632f96c27fef49b62338 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 18:00:54 +0530 Subject: [PATCH 09/15] refactor: tests for pipeline --- requirements/base.in | 2 +- requirements/base.txt | 2 +- requirements/ci.txt | 4 +-- requirements/dev.txt | 12 +++---- requirements/doc.txt | 12 +++---- requirements/quality.txt | 10 +++--- requirements/test.txt | 8 ++--- skill_tagging/pipeline.py | 5 +-- test_utils.py | 54 ++++++++++++++++++++++++++-- tests/test_pipeline.py | 60 +++++++++++++++++++++++++++++++ tests/test_skill_tagging_mixin.py | 57 +++-------------------------- 11 files changed, 144 insertions(+), 82 deletions(-) create mode 100644 tests/test_pipeline.py diff --git a/requirements/base.in b/requirements/base.in index 5d40b3b..4c78d14 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -4,4 +4,4 @@ Django # Web application framework django-model-utils # Provides TimeStampedModel abstract base class git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba#egg=openedx_events -openedx-filters +git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe#egg=openedx-filters diff --git a/requirements/base.txt b/requirements/base.txt index 2a882da..4c9b9c9 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -23,7 +23,7 @@ fastavro==1.7.0 # via openedx-events openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/base.in -openedx-filters==0.8.0 +openedx-filters @ git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe # via -r requirements/base.in pbr==5.11.0 # via stevedore diff --git a/requirements/ci.txt b/requirements/ci.txt index 7d2dab3..1f895a3 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -14,7 +14,7 @@ coverage==7.0.1 # via codecov distlib==0.3.6 # via virtualenv -filelock==3.8.2 +filelock==3.9.0 # via # tox # virtualenv @@ -22,7 +22,7 @@ idna==3.4 # via requests packaging==22.0 # via tox -platformdirs==2.6.0 +platformdirs==2.6.2 # via virtualenv pluggy==1.0.0 # via tox diff --git a/requirements/dev.txt b/requirements/dev.txt index 6d2e32a..d84864f 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -34,11 +34,11 @@ boto==2.49.0 # via # -r requirements/quality.txt # xblock-sdk -boto3==1.26.37 +boto3==1.26.40 # via # -r requirements/quality.txt # fs-s3fs -botocore==1.29.37 +botocore==1.29.40 # via # -r requirements/quality.txt # boto3 @@ -136,7 +136,7 @@ fastavro==1.7.0 # via # -r requirements/quality.txt # openedx-events -filelock==3.8.2 +filelock==3.9.0 # via # -r requirements/ci.txt # tox @@ -203,11 +203,11 @@ mccabe==0.7.0 # via # -r requirements/quality.txt # pylint -mock==4.0.3 +mock==5.0.0 # via -r requirements/quality.txt openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/quality.txt -openedx-filters==0.8.0 +openedx-filters @ git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe # via -r requirements/quality.txt packaging==22.0 # via @@ -229,7 +229,7 @@ pep517==0.13.0 # build pip-tools==6.12.1 # via -r requirements/pip-tools.txt -platformdirs==2.6.0 +platformdirs==2.6.2 # via # -r requirements/ci.txt # -r requirements/quality.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index 9864ecb..267399c 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -35,11 +35,11 @@ boto==2.49.0 # via # -r requirements/test.txt # xblock-sdk -boto3==1.26.37 +boto3==1.26.40 # via # -r requirements/test.txt # fs-s3fs -botocore==1.29.37 +botocore==1.29.40 # via # -r requirements/test.txt # boto3 @@ -139,7 +139,7 @@ importlib-metadata==5.2.0 # keyring # sphinx # twine -importlib-resources==5.10.1 +importlib-resources==5.10.2 # via keyring iniconfig==1.1.1 # via @@ -183,13 +183,13 @@ markupsafe==2.1.1 # -r requirements/test.txt # jinja2 # xblock -mock==4.0.3 +mock==5.0.0 # via -r requirements/test.txt more-itertools==9.0.0 # via jaraco-classes openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/test.txt -openedx-filters==0.8.0 +openedx-filters @ git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe # via -r requirements/test.txt packaging==22.0 # via @@ -295,7 +295,7 @@ six==1.16.0 # python-dateutil snowballstemmer==2.2.0 # via sphinx -sphinx==5.3.0 +sphinx==6.0.0 # via -r requirements/doc.in sphinxcontrib-applehelp==1.0.2 # via sphinx diff --git a/requirements/quality.txt b/requirements/quality.txt index 9e657de..df7b01f 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -33,11 +33,11 @@ boto==2.49.0 # via # -r requirements/test.txt # xblock-sdk -boto3==1.26.37 +boto3==1.26.40 # via # -r requirements/test.txt # fs-s3fs -botocore==1.29.37 +botocore==1.29.40 # via # -r requirements/test.txt # boto3 @@ -165,11 +165,11 @@ markupsafe==2.1.1 # xblock mccabe==0.7.0 # via pylint -mock==4.0.3 +mock==5.0.0 # via -r requirements/test.txt openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/test.txt -openedx-filters==0.8.0 +openedx-filters @ git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe # via -r requirements/test.txt packaging==22.0 # via @@ -179,7 +179,7 @@ pbr==5.11.0 # via # -r requirements/test.txt # stevedore -platformdirs==2.6.0 +platformdirs==2.6.2 # via pylint pluggy==1.0.0 # via diff --git a/requirements/test.txt b/requirements/test.txt index c7e1619..71067a6 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -21,9 +21,9 @@ binaryornot==0.4.4 # via cookiecutter boto==2.49.0 # via xblock-sdk -boto3==1.26.37 +boto3==1.26.40 # via fs-s3fs -botocore==1.29.37 +botocore==1.29.40 # via # boto3 # s3transfer @@ -102,11 +102,11 @@ markupsafe==2.1.1 # via # jinja2 # xblock -mock==4.0.3 +mock==5.0.0 # via -r requirements/test.in openedx_events @ git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba # via -r requirements/base.txt -openedx-filters==0.8.0 +openedx-filters @ git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe # via -r requirements/base.txt packaging==22.0 # via pytest diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index 26657bd..ab3f397 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -10,7 +10,7 @@ from openedx_filters import PipelineStep logger = logging.getLogger(__name__) -SHOW_SKILL_VERIFICATION_PROBABILITY = getattr(settings, "SHOW_SKILL_VERIFICATION_PROBABILITY", 0.5) +DEFAULT_PROBABILITY = 0.5 class AddVerticalBlockSkillVerificationSection(PipelineStep): @@ -52,7 +52,8 @@ def should_run_filter(self, skills): if not skills: return False # random returns a number between 0 and 1 (inclusive). - return random.random() < SHOW_SKILL_VERIFICATION_PROBABILITY + probability = getattr(settings, "SHOW_SKILL_VERIFICATION_PROBABILITY", DEFAULT_PROBABILITY) + return random.random() < probability def run_filter(self, block, fragment, context, view): # pylint: disable=arguments-differ """Pipeline Step implementing the Filter""" diff --git a/test_utils.py b/test_utils.py index a46e8dc..149becf 100644 --- a/test_utils.py +++ b/test_utils.py @@ -6,7 +6,9 @@ import json import re -from mock import patch +from mock import patch, Mock +from requests.models import Response +from rest_framework.status import HTTP_200_OK from webob import Request from workbench.runtime import WorkbenchRuntime from xblock.core import XBlock @@ -57,9 +59,47 @@ class TestCaseMixin: """ Helpful mixins for unittest TestCase subclasses """ maxDiff = None - FETCH_TAGS_HANDLER = 'fetch_tags' VERIFY_TAGS_HANDLER = 'verify_tags' + def setUp(self): + self.block = make_block() + self.patch_workbench() + fake_user = Mock() + fake_user.opt_attrs = { + 'edx-platform.user_role': 'student', + 'edx-platform.is_authenticated': True, + } + mock_user_service = Mock() + mock_user_service.get_current_user.return_value = fake_user + self.block.runtime.service = Mock(return_value=mock_user_service) + + def get_mock_api_response(self): + """Mock fetch tags response""" + sample_output = { + "results": [{ + "id": 1, + "skills": [ + { + "id": 1, + "name": "SKILL-0" + }, + { + "id": 6, + "name": "SKILL-5" + }, + ], + }] + } + api_client = Mock( + get=Mock( + return_value=self._mock_response( + HTTP_200_OK, + sample_output + ) + ) + ) + return api_client + def patch_workbench(self): """ Apply required patches to workbench @@ -97,3 +137,13 @@ def call_handler(self, handler_name, data=None, self.assertEqual(response.status_code, 200) return json.loads(response.body.decode('utf-8')) return response + + def _mock_response(self, status_code, content=None): + """ + Generates a python core response. + """ + mock_response = Response() + mock_response.status_code = status_code + # pylint: disable=protected-access + mock_response._content = json.dumps(content).encode('utf-8') + return mock_response diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py new file mode 100644 index 0000000..1234ecb --- /dev/null +++ b/tests/test_pipeline.py @@ -0,0 +1,60 @@ +""" +Tests for pipeline.py +""" +from unittest.mock import Mock, patch + +from django.test import TestCase, override_settings +from openedx_filters.learning.filters import VerticalBlockRenderCompleted + +from test_utils import TestCaseMixin + + +@patch('skill_tagging.skill_tagging_mixin.get_api_client') +@override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.vertical_block.render.completed.v1": { + "fail_silently": False, + "pipeline": [ + "skill_tagging.pipeline.AddVerticalBlockSkillVerificationSection" + ] + } + }, + SHOW_SKILL_VERIFICATION_PROBABILITY=0, +) +class TestAddVerticalBlockSkillVerificationSection(TestCaseMixin, TestCase): + """ + Testcase for the AddVerticalBlockSkillVerificationSection openedx-filters pipeline. + """ + + def setUp(self) -> None: + super().setUp() + self.original_fragement = Mock(content="

Some

") + + @override_settings( + 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. + """ + mock_get_api_client.return_value = self.get_mock_api_response() + _, fragment, _, _ = VerticalBlockRenderCompleted.run_filter( + block=self.block, context={}, fragment=self.original_fragement, view={} + ) + self.assertEqual(fragment.content, self.original_fragement.content) + self.assertNotIn("SKILL-0", fragment.content) + + @override_settings( + SHOW_SKILL_VERIFICATION_PROBABILITY=1, + ) + def test_pipeline_adds_edit_link_when_probability_set_to_one(self, mock_get_api_client): + """Check that verification div is added with skills.""" + mock_get_api_client.return_value = self.get_mock_api_response() + _, fragment, _, _ = VerticalBlockRenderCompleted.run_filter( + block=self.block, context={}, fragment=self.original_fragement, view={} + ) + + self.assertIn(self.original_fragement.content, fragment.content) + self.assertIn("SKILL-0", fragment.content) + self.assertIn("SKILL-5", fragment.content) diff --git a/tests/test_skill_tagging_mixin.py b/tests/test_skill_tagging_mixin.py index dbf6d1f..0fbab09 100644 --- a/tests/test_skill_tagging_mixin.py +++ b/tests/test_skill_tagging_mixin.py @@ -3,46 +3,19 @@ """ from __future__ import absolute_import -import json import unittest from unittest.mock import Mock, patch -from requests.models import Response -from rest_framework import status - -from test_utils import TestCaseMixin, get_tagging_mixin, make_block +from test_utils import TestCaseMixin, get_tagging_mixin @patch('skill_tagging.skill_tagging_mixin.get_api_client') -class SkillTaggingMixinTests(unittest.TestCase, TestCaseMixin): +class SkillTaggingMixinTests(TestCaseMixin, unittest.TestCase): """ Test suite for SkillTaggingMixin """ - def setUp(self): - self.block = make_block() - self.patch_workbench() - fake_user = Mock() - fake_user.opt_attrs = { - 'edx-platform.user_role': 'student', - 'edx-platform.is_authenticated': True, - } - mock_user_service = Mock() - mock_user_service.get_current_user.return_value = fake_user - self.block.runtime.service = Mock(return_value=mock_user_service) - - def _mock_response(self, status_code, content=None): - """ - Generates a python core response. - """ - mock_response = Response() - mock_response.status_code = status_code - # pylint: disable=protected-access - mock_response._content = json.dumps(content).encode('utf-8') - return mock_response - - # pylint: disable=unused-argument - def test_mixin_fields(self, mock_get_api_client): + def test_mixin_fields(self, _): """ Test for mixin field and methods """ @@ -55,29 +28,7 @@ def test_fetch_tags(self, mock_get_api_client): """ Test that fetch_tags method works as expected """ - sample_output = { - "results": [{ - "id": 1, - "skills": [ - { - "id": 1, - "name": "SKILL-0" - }, - { - "id": 6, - "name": "SKILL-5" - }, - ], - }] - } - api_client = Mock( - get=Mock( - return_value=self._mock_response( - status.HTTP_200_OK, - sample_output - ) - ) - ) + api_client = self.get_mock_api_response() mock_get_api_client.return_value = api_client resp = self.block.fetch_skill_tags() assert mock_get_api_client.call_count == 1 From a6b15b96f7488f2fca15609cb615da56e06f9627 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 18:18:30 +0530 Subject: [PATCH 10/15] fix: image styling in small screen --- skill_tagging/static/tagging.css | 6 ++++++ skill_tagging/static/tagging.html | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/skill_tagging/static/tagging.css b/skill_tagging/static/tagging.css index 1bdd803..fde57a0 100644 --- a/skill_tagging/static/tagging.css +++ b/skill_tagging/static/tagging.css @@ -6,6 +6,12 @@ display: flex; } +@media (max-width: 767px) { + .tag-verification-image-container { + display: none; + } +} + .tag-verification-container h1 { font-style: normal; font-weight: 700; diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index c0741ee..e5279ae 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -1,5 +1,5 @@
-
+

What did you learn?

Congratulations, you've reached the end of this unit!
@@ -23,7 +23,7 @@

What did you learn?

-
+
{{ image | safe }}
From 3a857400fe0b68ee7aa2984bcf834e076ab1d159 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 30 Dec 2022 20:04:09 +0530 Subject: [PATCH 11/15] fix: tox workbench test --- Makefile | 1 + tox.ini | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d577d74..3334362 100644 --- a/Makefile +++ b/Makefile @@ -64,6 +64,7 @@ requirements: piptools ## install development environment requirements pip-sync -q requirements/dev.txt requirements/private.* test: clean ## run tests in the current virtualenv + mkdir -p var DJANGO_SETTINGS_MODULE=test_settings pytest diff_cover: test ## find diff lines that need test coverage diff --git a/tox.ini b/tox.ini index c5f5569..ce71be9 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,7 @@ deps = django40: Django>=4.0,<4.1 -r{toxinidir}/requirements/test.txt commands = - pytest {posargs} + make test [testenv:quality] From 67d16d6c8816d948b1ee9adbedda836b60bac90a Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 3 Jan 2023 11:43:55 +0530 Subject: [PATCH 12/15] refactor: move skills check and rename variable --- skill_tagging/pipeline.py | 7 ++----- skill_tagging/static/tagging.js | 10 +++++----- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/skill_tagging/pipeline.py b/skill_tagging/pipeline.py index ab3f397..9c7fff7 100644 --- a/skill_tagging/pipeline.py +++ b/skill_tagging/pipeline.py @@ -46,11 +46,8 @@ def fetch_related_skills(self, block): tags = fetch_tags() return tags - def should_run_filter(self, skills): + def should_run_filter(self): """Determines whether we should run filter and display form.""" - # return false if no skills found. - if not skills: - return False # random returns a number between 0 and 1 (inclusive). probability = getattr(settings, "SHOW_SKILL_VERIFICATION_PROBABILITY", DEFAULT_PROBABILITY) return random.random() < probability @@ -59,7 +56,7 @@ def run_filter(self, block, fragment, context, view): # pylint: disable=argumen """Pipeline Step implementing the Filter""" skills = self.fetch_related_skills(block) - if not self.should_run_filter(skills): + if not skills or not self.should_run_filter(): return {"block": block, "fragment": fragment, "context": context, "view": view} verify_tags_url = block.runtime.handler_url(block, "verify_tags") html = self.resource_string("static/tagging.html") diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index a013c19..9d0738e 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -1,5 +1,5 @@ const tagSkillContainer = document.getElementById("tag-verification-tags-container-id"); -var tagSkillSelectedTags = []; +var tagSkillVerifiedTags = []; var tagSkillIgnoredTags = []; function tagVerificationSetNoneToFalse(source) { @@ -15,23 +15,23 @@ function tagVerificationVerifyTags(url) { var checkboxes = document.getElementsByName('tag-verification-skills'); var noneSelectedCheckbox = document.getElementById('tagVerificationUnselectAllId'); // clear containers - tagSkillSelectedTags = []; + tagSkillVerifiedTags = []; tagSkillIgnoredTags = []; for (var i = 0, n = checkboxes.length; i < n; i++) { if (checkboxes[i].checked) { - tagSkillSelectedTags.push(parseInt(checkboxes[i].value)); + tagSkillVerifiedTags.push(parseInt(checkboxes[i].value)); } else { tagSkillIgnoredTags.push(parseInt(checkboxes[i].value)); } } - if (tagSkillSelectedTags.length === 0 && !noneSelectedCheckbox.checked) { + if (tagSkillVerifiedTags.length === 0 && !noneSelectedCheckbox.checked) { alert("Please select atleast one skill or check 'None of these subjects were covered' checkbox!"); return; } fetch(url, { method: "POST", body: JSON.stringify({ - verified_skills: tagSkillSelectedTags, + verified_skills: tagSkillVerifiedTags, ignored_skills: tagSkillIgnoredTags, }), headers: { From 0488147e317a3d392d13eb60a55ff44d53b613c8 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 4 Jan 2023 12:26:18 +0530 Subject: [PATCH 13/15] refactor: disable submit button till input is filled --- skill_tagging/static/tagging.html | 8 +++++--- skill_tagging/static/tagging.js | 20 ++++++++++++++++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/skill_tagging/static/tagging.html b/skill_tagging/static/tagging.html index e5279ae..47cd57b 100644 --- a/skill_tagging/static/tagging.html +++ b/skill_tagging/static/tagging.html @@ -8,7 +8,7 @@

What did you learn?

{% for skill in skills %} + value="{{ skill.id }}" onclick="tagVerificationOnSkillClick(this)">
- +
- +
{{ image | safe }} diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index 9d0738e..8f94207 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -2,7 +2,22 @@ const tagSkillContainer = document.getElementById("tag-verification-tags-contain var tagSkillVerifiedTags = []; var tagSkillIgnoredTags = []; -function tagVerificationSetNoneToFalse(source) { +function tagVerificationToggleSubmitButton() { + var enable = false; + checkboxes = Array.from(document.getElementsByName('tag-verification-skills')); + checkboxes.push(document.getElementById("tagVerificationUnselectAllId")); + for (var i = 0, n = checkboxes.length; i < n; i++) { + if (checkboxes[i].checked) { + enable = true; + break; + }; + } + var submitButton = document.getElementById("tagVerificationSubmitButton"); + submitButton.disabled = !enable; +} + +function tagVerificationOnSkillClick(source) { + tagVerificationToggleSubmitButton(); if (!source.checked) { return; } @@ -51,7 +66,8 @@ function tagVerificationVerifyTags(url) { }); } -function tagVerificationUnselectAll(source) { +function tagVerificationOnNoneCheckboxClick(source) { + tagVerificationToggleSubmitButton(); if (!source.checked) { return; } From 88ddc160f4577e34c5b7bcec49c1496908e2d016 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 4 Jan 2023 12:45:08 +0530 Subject: [PATCH 14/15] refactor: better readCookie function --- skill_tagging/static/tagging.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index 8f94207..c439dc2 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -25,8 +25,24 @@ function tagVerificationOnSkillClick(source) { checkbox.checked = false; } +// https://www.quirksmode.org/js/cookies.html +function readCookie(name) { + var nameEQ = name + "="; + var ca = document.cookie.split(';'); + for (var i = 0; i < ca.length; i++) { + var c = ca[i]; + while (c.charAt(0) == ' ') c = c.substring(1, c.length); + if (c.indexOf(nameEQ) == 0) return c.substring(nameEQ.length, c.length); + } + return null; +} + function tagVerificationVerifyTags(url) { - var csrf_token = document.cookie.split(";").find(c => c.startsWith("csrftoken="))?.split("=")[1]; + var csrftoken = readCookie("csrftoken"); + if (!csrftoken) { + alert("csrftoken not found! Please refresh the page or re login"); + return; + } var checkboxes = document.getElementsByName('tag-verification-skills'); var noneSelectedCheckbox = document.getElementById('tagVerificationUnselectAllId'); // clear containers @@ -51,7 +67,7 @@ function tagVerificationVerifyTags(url) { }), headers: { "Content-Type": "application/json", - "X-CSRFToken": csrf_token, + "X-CSRFToken": csrftoken, } }) .then(res => res.json()) From d3d5975de641269e5f068b4171ec1880f9b57ab5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 5 Jan 2023 11:49:23 +0530 Subject: [PATCH 15/15] refactor: remove unnecessary alert and add todo in requirements file --- requirements/base.in | 1 + skill_tagging/static/tagging.js | 5 ----- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/requirements/base.in b/requirements/base.in index 4c78d14..1caf40d 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -3,5 +3,6 @@ Django # Web application framework django-model-utils # Provides TimeStampedModel abstract base class +# TODO: update below dependencies when its commits are merged to upstream git+https://github.com/open-craft/openedx-events.git@77ad965c4ac5157b861a19b8cb6b9240883008ba#egg=openedx_events git+https://github.com/open-craft/openedx-filters.git@d0206cf84f2e5e22b1a99d06566d88839d40b9fe#egg=openedx-filters diff --git a/skill_tagging/static/tagging.js b/skill_tagging/static/tagging.js index c439dc2..6de4972 100644 --- a/skill_tagging/static/tagging.js +++ b/skill_tagging/static/tagging.js @@ -44,7 +44,6 @@ function tagVerificationVerifyTags(url) { return; } var checkboxes = document.getElementsByName('tag-verification-skills'); - var noneSelectedCheckbox = document.getElementById('tagVerificationUnselectAllId'); // clear containers tagSkillVerifiedTags = []; tagSkillIgnoredTags = []; @@ -55,10 +54,6 @@ function tagVerificationVerifyTags(url) { tagSkillIgnoredTags.push(parseInt(checkboxes[i].value)); } } - if (tagSkillVerifiedTags.length === 0 && !noneSelectedCheckbox.checked) { - alert("Please select atleast one skill or check 'None of these subjects were covered' checkbox!"); - return; - } fetch(url, { method: "POST", body: JSON.stringify({