From 30af61fd1f0e97fe9ffcb670878a1d1b68f4e4e6 Mon Sep 17 00:00:00 2001 From: Emad Rad Date: Wed, 6 Mar 2024 16:49:44 +0330 Subject: [PATCH 1/3] chore: cleanup - Python & Yaml code highlight added in docs - clean up readme for better readability - Python code reformatted with black and isort --- README.rst | 37 ++++++++++--- openedxscorm/scormxblock.py | 107 ++++++++++++++++++++++++------------ openedxscorm/storage.py | 5 +- openedxscorm/tests.py | 37 ++++++------- 4 files changed, 121 insertions(+), 65 deletions(-) diff --git a/README.rst b/README.rst index 5cd608e..19525c2 100644 --- a/README.rst +++ b/README.rst @@ -27,17 +27,22 @@ Features Installation ------------ -This XBlock was designed to work out of the box with `Tutor `__ (Ironwood release). It comes bundled by default in the official Tutor releases, such that there is no need to install it manually. +This XBlock was designed to work out of the box with `Tutor `__ (Ironwood release). +It comes bundled by default in the official Tutor releases, such that there is no need to install it manually. For non-Tutor platforms, you should install the `Python package from Pypi `__:: pip install openedx-scorm-xblock -In the Open edX native installation, you will have to modify the files ``/edx/etc/lms.yml`` and ``/edx/etc/studio.yml``. Replace:: +In the Open edX native installation, you will have to modify the files ``/edx/etc/lms.yml`` and ``/edx/etc/studio.yml``. Replace + +.. code-block:: yaml X_FRAME_OPTIONS: DENY -By:: +By + +.. code-block:: yaml X_FRAME_OPTIONS: SAMEORIGIN @@ -46,7 +51,9 @@ Usage In the Studio, go to the advanced settings of your course ("Settings" 🡒 "Advanced Settings"). In the "Advanced Module List" add "scorm". Then hit "Save changes". -Go back to your course content. In the "Add New Component" section, click "Advanced", and then "Scorm module". Click "Edit" on the newly-created module: this is where you will upload your content package. It should be a ``.zip`` file containing an ``imsmanifest.xml`` file at the root. The content of the package will be displayed in the Studio and the LMS after you click "Save". +Go back to your course content. In the "Add New Component" section, click "Advanced", and then "Scorm module". +Click "Edit" on the newly-created module: this is where you will upload your content package. It should be a ``.zip`` file containing an ``imsmanifest.xml`` file at the root. +The content of the package will be displayed in the Studio and the LMS after you click "Save". Advanced configuration ---------------------- @@ -54,7 +61,9 @@ Advanced configuration Asset url ~~~~~~~~~ -By default, SCORM modules will be accessible at "/scorm/" urls and static assets will be stored in "scorm" media folders -- either on S3 or in the local storage, depending on your platform configuration. To change this behaviour, modify the xblock-specific ``LOCATION`` setting:: +By default, SCORM modules will be accessible at "/scorm/" urls and static assets will be stored in "scorm" media folders -- either on S3 or in the local storage, depending on your platform configuration. To change this behaviour, modify the xblock-specific ``LOCATION`` setting + +.. code-block:: python XBLOCK_SETTINGS["ScormXBlock"] = { "LOCATION": "alternatevalue", @@ -63,7 +72,10 @@ By default, SCORM modules will be accessible at "/scorm/" urls and static assets Custom storage backends ~~~~~~~~~~~~~~~~~~~~~~~ -By default, static assets are stored in the default Django storage backend. To override this behaviour, you should define a custom storage function. This function must take the xblock instance as its first and only argument. For instance, you can store assets in different directories depending on the XBlock organisation with:: +By default, static assets are stored in the default Django storage backend. To override this behaviour, you should define a custom storage function. This function must take the xblock instance as its first and only argument. +For instance, you can store assets in different directories depending on the XBlock organization with + +.. code-block:: python def scorm_storage(xblock): from django.conf import settings @@ -82,7 +94,9 @@ By default, static assets are stored in the default Django storage backend. To o "STORAGE_FUNC": scorm_storage, } -This should be added both to the LMS and the CMS settings. Instead of a function, a string that points to an importable module may be passed:: +This should be added both to the LMS and the CMS settings. Instead of a function, a string that points to an importable module may be passed + +.. code-block:: python XBLOCK_SETTINGS["ScormXBlock"] = { "STORAGE_FUNC": "my.custom.storage.module.get_scorm_storage_function", @@ -93,7 +107,10 @@ Note that the SCORM XBlock comes with S3 storage support out of the box. See the S3 storage ~~~~~~~~~~ -The SCORM XBlock may be configured to proxy static SCORM assets stored in either public or private S3 buckets. To configure S3 storage, add the following to your LMS and CMS settings:: +The SCORM XBlock may be configured to proxy static SCORM assets stored in either public or private S3 buckets. +To configure S3 storage, add the following to your LMS and CMS settings + +.. code-block:: python XBLOCK_SETTINGS["ScormXBlock"] = { "STORAGE_FUNC": "openedxscorm.storage.s3" @@ -105,7 +122,9 @@ You may define the following additional settings in ``XBLOCK_SETTINGS["ScormXBlo * ``S3_QUERY_AUTH`` (default: ``True``): boolean flag (``True`` or ``False``) for query string authentication in S3 urls. If your bucket is public, set this value to ``False``. But be aware that in such case your SCORM assets will be publicly available to everyone. * ``S3_EXPIRES_IN`` (default: 604800): time duration (in seconds) for the presigned URLs to stay valid. The default is one week. -These settings may be added to Tutor by creating a `plugin `__:: +These settings may be added to Tutor by creating a `plugin `__: + +.. code-block:: python from tutor import hooks diff --git a/openedxscorm/scormxblock.py b/openedxscorm/scormxblock.py index 36f644e..d631c6c 100644 --- a/openedxscorm/scormxblock.py +++ b/openedxscorm/scormxblock.py @@ -67,7 +67,7 @@ class ScormXBlock(XBlock, CompletableXBlockMixin): By default, static assets are stored in the default Django storage backend. To override this behaviour, you should define a custom storage function. This function must take the xblock instance as its first and only argument. For instance, - you can store assets in different directories depending on the XBlock organisation with:: + you can store assets in different directories depending on the XBlock organization with:: def scorm_storage(xblock): from django.conf import settings @@ -158,13 +158,15 @@ def scorm_storage(xblock): ) navigation_menu = String(scope=Scope.settings, default="") - + navigation_menu_width = Integer( display_name=_("Display width of navigation menu(px)"), - help=_("Width of navigation menu. This assumes that Navigation Menu is enabled. (default: 30%)"), + help=_( + "Width of navigation menu. This assumes that Navigation Menu is enabled. (default: 30%)" + ), scope=Scope.settings, ) - + has_author_view = True def render_template(self, template_path, context): @@ -187,9 +189,7 @@ def resource_string(path): def author_view(self, context=None): context = context or {} if not self.index_page_path: - context[ - "message" - ] = "Click 'Edit' to modify this module and upload a new SCORM package." + context["message"] = "Click 'Edit' to modify this module and upload a new SCORM package." context["can_view_student_reports"] = True return self.student_view(context=context) @@ -201,7 +201,7 @@ def student_view(self, context=None): "can_view_student_reports": self.can_view_student_reports, "scorm_xblock": self, "navigation_menu": self.navigation_menu, - "popup_on_launch": self.popup_on_launch + "popup_on_launch": self.popup_on_launch, } student_context.update(context or {}) template = self.render_template("static/html/scormxblock.html", student_context) @@ -242,7 +242,7 @@ def assets_proxy(self, request, suffix): file_name = os.path.basename(suffix) signed_url = self.storage.url(suffix) if request.query_string: - signed_url = '&'.join([signed_url, request.query_string]) + signed_url = "&".join([signed_url, request.query_string]) file_type, _ = mimetypes.guess_type(file_name) with urllib.request.urlopen(signed_url) as response: file_content = response.read() @@ -285,7 +285,9 @@ def studio_submit(self, request, _suffix): self.height = parse_int(request.params["height"], None) self.has_score = request.params["has_score"] == "1" self.enable_navigation_menu = request.params["enable_navigation_menu"] == "1" - self.navigation_menu_width = parse_int(request.params["navigation_menu_width"], None) + self.navigation_menu_width = parse_int( + request.params["navigation_menu_width"], None + ) self.weight = parse_float(request.params["weight"], 1) self.popup_on_launch = request.params["popup_on_launch"] == "1" self.icon_class = "problem" if self.has_score else "video" @@ -323,7 +325,7 @@ def popup_window(self, request, _suffix): "height": self.height or 800, "navigation_menu": self.navigation_menu, "navigation_menu_width": self.navigation_menu_width, - "enable_navigation_menu": self.enable_navigation_menu + "enable_navigation_menu": self.enable_navigation_menu, }, ) return Response(body=rendered) @@ -411,9 +413,9 @@ def extract_folder_base_path(self): Path to the folder where packages will be extracted. """ return os.path.join(self.scorm_location(), self.location.block_id) - - def get_mode(self,data): - if('preview' in data['url']): + + def get_mode(self, data): + if "preview" in data["url"]: return "review" return "normal" @@ -491,7 +493,11 @@ def set_value(self, data): self.success_status = success_status if completion_status == "completed": self.emit_completion(1) - if success_status or completion_status == "completed" or (is_completed and lesson_score): + if ( + success_status + or completion_status == "completed" + or (is_completed and lesson_score) + ): if self.has_score: self.publish_grade() @@ -579,18 +585,23 @@ def extract_navigation_titles(self, root, prefix): root (XMLTag): root of the imsmanifest.xml file prefix (string): namespace to match with in the xml file """ - organizations = root.findall('{prefix}organizations/{prefix}organization'.format(prefix=prefix)) + organizations = root.findall( + "{prefix}organizations/{prefix}organization".format(prefix=prefix) + ) navigation_menu_titles = [] # Get data for all organizations for organization in organizations: - navigation_menu_titles.append(self.find_titles_recursively(organization, prefix, root)) + navigation_menu_titles.append( + self.find_titles_recursively(organization, prefix, root) + ) self.navigation_menu = self.recursive_unorderedlist(navigation_menu_titles) - + def sanitize_input(self, input_str): """Removes script tags from string""" - sanitized_str = re.sub(r']*>(.*?)', '', input_str, flags=re.IGNORECASE) + sanitized_str = re.sub( + r"]*>(.*?)", "", input_str, flags=re.IGNORECASE + ) return sanitized_str - def find_titles_recursively(self, item, prefix, root): """Recursively iterate through the organization tags and extract the title and resources @@ -603,8 +614,8 @@ def find_titles_recursively(self, item, prefix, root): Returns: List: Nested list of all the title tags and their resources """ - children = item.findall('{prefix}item'.format(prefix=prefix)) - item_title = item.find('{prefix}title'.format(prefix=prefix)).text + children = item.findall("{prefix}item".format(prefix=prefix)) + item_title = item.find("{prefix}title".format(prefix=prefix)).text # Sanitizing every title tag to protect against XSS attacks sanitized_title = self.sanitize_input(item_title) item_identifier = item.get("identifierref") @@ -612,19 +623,25 @@ def find_titles_recursively(self, item, prefix, root): if not item_identifier: resource_link = "#" else: - resource = root.find("{prefix}resources/{prefix}resource[@identifier='{identifier}']".format(prefix=prefix, identifier=item_identifier)) + resource = root.find( + "{prefix}resources/{prefix}resource[@identifier='{identifier}']".format( + prefix=prefix, identifier=item_identifier + ) + ) # Attach the storage path with the file path resource_link = urllib.parse.unquote( - self.storage.url(os.path.join(self.extract_folder_path, resource.get("href"))) + self.storage.url( + os.path.join(self.extract_folder_path, resource.get("href")) + ) ) if not children: return [(sanitized_title, resource_link)] child_titles = [] for child in children: - if 'isvisible' in child.attrib and child.attrib['isvisible'] == "true": + if "isvisible" in child.attrib and child.attrib["isvisible"] == "true": child_titles.extend(self.find_titles_recursively(child, prefix, root)) return [(sanitized_title, resource_link), child_titles] - + def recursive_unorderedlist(self, value): """Create an HTML unordered list recursively to display navigation menu @@ -642,29 +659,47 @@ def format(items, tabs=1): if type(items) is tuple: title, resource_url = items[0], items[1] if resource_url != "#": - return "{indent}
  • {title}
    • ".format(indent=indent, resource_url=resource_url, title=title) - return "{indent}
  • {title}
    • ".format(indent=indent, title=title) - + return "{indent}
  • {title}
    • ".format( + indent=indent, resource_url=resource_url, title=title + ) + return ( + "{indent}
  • {title}
    • ".format( + indent=indent, title=title + ) + ) + output = [] # If parent node, create another nested unordered list and return if has_children(items): parent, children = items[0], items[1] title, resource_url = parent[0], parent[1] for child in children: - output.append(format(child, tabs+1)) + output.append(format(child, tabs + 1)) if resource_url != "#": - return "\n{indent}
      \n{indent}
    • {title}
      • \n{indent}
        \n{indent}\n{output}
      \n{indent}
    ".format(indent=indent, resource_url=resource_url, title=title, output="\n".join(output)) - return "\n{indent}
      \n{indent}
    • {title}
      • \n{indent}
        \n{indent}\n{output}
      \n{indent}
    ".format(indent=indent, resource_url=resource_url, title=title, output="\n".join(output)) + return "\n{indent}
      \n{indent}
    • {title}
      • \n{indent}
        \n{indent}\n{output}
      \n{indent}
    ".format( + indent=indent, + resource_url=resource_url, + title=title, + output="\n".join(output), + ) + return "\n{indent}
      \n{indent}
    • {title}
      • \n{indent}
        \n{indent}\n{output}
      \n{indent}
    ".format( + indent=indent, + resource_url=resource_url, + title=title, + output="\n".join(output), + ) else: for item in items: - output.append(format(item, tabs+1)) - return "{indent}\n{indent}
      \n{output}\n{indent}
    ".format(indent=indent, output="\n".join(output)) - + output.append(format(item, tabs + 1)) + return "{indent}\n{indent}
      \n{output}\n{indent}
    ".format( + indent=indent, output="\n".join(output) + ) + unordered_lists = [] # Append navigation menus for all organizations in course for organization in value: unordered_lists.append(format(organization)) - + return "\n".join(unordered_lists) def find_relative_file_path(self, filename): diff --git a/openedxscorm/storage.py b/openedxscorm/storage.py index cd92587..061755b 100644 --- a/openedxscorm/storage.py +++ b/openedxscorm/storage.py @@ -1,6 +1,7 @@ """ Storage backend for scorm metadata export. """ + import os from django.conf import settings @@ -12,7 +13,9 @@ class S3ScormStorage(S3Boto3Storage): S3 backend for scorm metadata export """ - def __init__(self, xblock, bucket_name=None, querystring_auth=None, querystring_expire=None): + def __init__( + self, xblock, bucket_name=None, querystring_auth=None, querystring_expire=None + ): self.xblock = xblock # No need to serve assets from a custom domain. self.custom_domain = None diff --git a/openedxscorm/tests.py b/openedxscorm/tests.py index 3c90c73..8c7cbda 100644 --- a/openedxscorm/tests.py +++ b/openedxscorm/tests.py @@ -129,9 +129,7 @@ def test_build_file_storage_path(self): ) @mock.patch("openedxscorm.scormxblock.default_storage") def test_student_view_data(self, default_storage, file_storage_path): - block = self.make_one( - package_meta={"last_updated": "2018-05-01", "size": 1234} - ) + block = self.make_one(package_meta={"last_updated": "2018-05-01", "size": 1234}) default_storage.configure_mock(url=mock.Mock(return_value="url_zip_file")) student_view_data = block.student_view_data() @@ -245,7 +243,8 @@ def test_scorm_get_status(self, value): self.assertEqual(response.json, {"value": "status"}) @data( - {"name": "cmi.core.score.raw"}, {"name": "cmi.score.raw"}, + {"name": "cmi.core.score.raw"}, + {"name": "cmi.score.raw"}, ) def test_scorm_get_lesson_score(self, value): block = self.make_one(lesson_score=0.2) @@ -277,17 +276,17 @@ def test_get_other_scorm_values(self, value): self.assertEqual(response.json, {"value": block.scorm_data[value["name"]]}) @data( - ({'name': 'cmi.core.student_id'}, 'edx-platform.user_id', 23), - ({'name': 'cmi.core.student_name'}, 'edx-platform.username', 'supername') + ({"name": "cmi.core.student_id"}, "edx-platform.user_id", 23), + ({"name": "cmi.core.student_name"}, "edx-platform.username", "supername"), ) @unpack def test_scorm_12_get_student_data(self, request_data, key, value): service_user_mock = mock.Mock() current_user_mock = mock.Mock() - current_user_mock.opt_attrs = { - key : value - } - service_user_mock.configure_mock(**{'get_current_user.return_value': current_user_mock}) + current_user_mock.opt_attrs = {key: value} + service_user_mock.configure_mock( + **{"get_current_user.return_value": current_user_mock} + ) runtime = mock.Mock() runtime.service.return_value = service_user_mock @@ -297,20 +296,20 @@ def test_scorm_12_get_student_data(self, request_data, key, value): response = block.scorm_get_value( mock.Mock(method="POST", body=json.dumps(request_data)) ) - self.assertEqual(response.json, {'value': value}) - + self.assertEqual(response.json, {"value": value}) + @data( - ({'name': 'cmi.learner_id'}, 'edx-platform.user_id', 23), - ({'name': 'cmi.learner_name'}, 'edx-platform.username', 'supername') + ({"name": "cmi.learner_id"}, "edx-platform.user_id", 23), + ({"name": "cmi.learner_name"}, "edx-platform.username", "supername"), ) @unpack def test_scorm_2004_get_student_data(self, request_data, key, value): service_user_mock = mock.Mock() current_user_mock = mock.Mock() - current_user_mock.opt_attrs = { - key : value - } - service_user_mock.configure_mock(**{'get_current_user.return_value': current_user_mock}) + current_user_mock.opt_attrs = {key: value} + service_user_mock.configure_mock( + **{"get_current_user.return_value": current_user_mock} + ) runtime = mock.Mock() runtime.service.return_value = service_user_mock @@ -320,4 +319,4 @@ def test_scorm_2004_get_student_data(self, request_data, key, value): response = block.scorm_get_value( mock.Mock(method="POST", body=json.dumps(request_data)) ) - self.assertEqual(response.json, {'value': value}) \ No newline at end of file + self.assertEqual(response.json, {"value": value}) From f41cc425c6d446625556431e0e81ba5cecde661a Mon Sep 17 00:00:00 2001 From: Emad Rad Date: Wed, 6 Mar 2024 16:56:48 +0330 Subject: [PATCH 2/3] chore: f-strings used for better readability --- openedxscorm/scormxblock.py | 41 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/openedxscorm/scormxblock.py b/openedxscorm/scormxblock.py index d631c6c..5075c09 100644 --- a/openedxscorm/scormxblock.py +++ b/openedxscorm/scormxblock.py @@ -559,10 +559,10 @@ def update_package_fields(self): prefix = "{" + namespace + "}" if namespace else "" resource = root.find( - "{prefix}resources/{prefix}resource[@href]".format(prefix=prefix) + f"{prefix}resources/{prefix}resource[@href]" ) schemaversion = root.find( - "{prefix}metadata/{prefix}schemaversion".format(prefix=prefix) + f"{prefix}metadata/{prefix}schemaversion" ) self.extract_navigation_titles(root, prefix) @@ -586,7 +586,7 @@ def extract_navigation_titles(self, root, prefix): prefix (string): namespace to match with in the xml file """ organizations = root.findall( - "{prefix}organizations/{prefix}organization".format(prefix=prefix) + f"{prefix}organizations/{prefix}organization" ) navigation_menu_titles = [] # Get data for all organizations @@ -614,8 +614,8 @@ def find_titles_recursively(self, item, prefix, root): Returns: List: Nested list of all the title tags and their resources """ - children = item.findall("{prefix}item".format(prefix=prefix)) - item_title = item.find("{prefix}title".format(prefix=prefix)).text + children = item.findall(f"{prefix}item") + item_title = item.find(f"{prefix}title").text # Sanitizing every title tag to protect against XSS attacks sanitized_title = self.sanitize_input(item_title) item_identifier = item.get("identifierref") @@ -624,9 +624,7 @@ def find_titles_recursively(self, item, prefix, root): resource_link = "#" else: resource = root.find( - "{prefix}resources/{prefix}resource[@identifier='{identifier}']".format( - prefix=prefix, identifier=item_identifier - ) + f"{prefix}resources/{prefix}resource[@identifier='{item_identifier}']" ) # Attach the storage path with the file path resource_link = urllib.parse.unquote( @@ -659,14 +657,9 @@ def format(items, tabs=1): if type(items) is tuple: title, resource_url = items[0], items[1] if resource_url != "#": - return "{indent}
  • {title}
    • ".format( - indent=indent, resource_url=resource_url, title=title - ) - return ( - "{indent}
  • {title}
    • ".format( - indent=indent, title=title - ) - ) + return f"{indent}
  • {title}
    • " + + return f"{indent}
  • {title}
    • " output = [] # If parent node, create another nested unordered list and return @@ -712,9 +705,7 @@ def find_file_path(self, filename): """ path = self.get_file_path(filename, self.extract_folder_path) if path is None: - raise ScormError( - "Invalid package: could not find '{}' file".format(filename) - ) + raise ScormError(f"Invalid package: could not find '{filename}' file") return path def get_file_path(self, filename, root): @@ -795,9 +786,7 @@ def scorm_search_students(self, data, _suffix): [ { "data": {"student_id": enrollment.user.id}, - "value": "{} ({})".format( - enrollment.user.username, enrollment.user.email - ), + "value": f"{enrollment.user.username} ({enrollment.user.email})" } for enrollment in enrollments[:20] ] @@ -812,7 +801,7 @@ def scorm_get_student_state(self, data, _suffix): user_id = int(user_id) except (TypeError, ValueError): return Response( - body="Invalid 'id' parameter {}".format(user_id), status=400 + body=f"Invalid 'id' parameter {user_id}", status=400 ) try: module = StudentModule.objects.filter( @@ -822,7 +811,7 @@ def scorm_get_student_state(self, data, _suffix): ).get() except StudentModule.DoesNotExist: return Response( - body="No data found for student id={}".format(user_id), + body=f"No data found for student id={user_id}", status=404, ) except StudentModule.MultipleObjectsReturned: @@ -904,10 +893,10 @@ def parse_validate_positive_float(value, name): parsed = float(value) except (TypeError, ValueError): raise ValueError( - "Could not parse value of '{}' (must be float): {}".format(name, value) + f"Could not parse value of '{name}' (must be float): {value}" ) if parsed < 0: - raise ValueError("Value of '{}' must not be negative: {}".format(name, value)) + raise ValueError(f"Value of '{name}' must not be negative: {value}") return parsed From 442856f8c1d66c784ca4122a04a2318876a406c6 Mon Sep 17 00:00:00 2001 From: Emad Rad Date: Wed, 6 Mar 2024 17:03:30 +0330 Subject: [PATCH 3/3] ci: don't even try to auto-add PRs to github project Auto-adding PRs to the Github project is not working because the github-token is not available there. --- .github/workflows/auto-add-to-project.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/auto-add-to-project.yml b/.github/workflows/auto-add-to-project.yml index d4ab7a4..21a6ad9 100644 --- a/.github/workflows/auto-add-to-project.yml +++ b/.github/workflows/auto-add-to-project.yml @@ -1,9 +1,7 @@ -name: Auto Add Issues and Pull Requests to Project +name: Auto Add Issues to Project on: - pull_request: - types: - - opened + issues: types: - opened