Skip to content

Commit

Permalink
chore: cleanup (#66)
Browse files Browse the repository at this point in the history 
* chore: cleanup

- Python & Yaml code highlight added in docs
- clean up readme for better readability
- Python code reformatted with black and isort

* chore: f-strings used for better readability

* 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.
  • Loading branch information
@CodeWithEmad
CodeWithEmad authored Mar 13, 2024
1 parent 27be75f commit f81b9e7
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 81 deletions.
6 changes: 2 additions & 4 deletions .github/workflows/auto-add-to-project.yml
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down
37 changes: 28 additions & 9 deletions README.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,22 @@ Features
Installation
------------

This XBlock was designed to work out of the box with `Tutor <https://docs.tutor.overhang.io>`__ (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 <https://docs.tutor.overhang.io>`__ (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 <https://pypi.org/project/openedx-scorm-xblock/>`__::

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
Expand All @@ -46,15 +51,19 @@ 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
----------------------

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",
Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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"
Expand All @@ -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 <https://docs.tutor.overhang.io/plugins/>`__::
These settings may be added to Tutor by creating a `plugin <https://docs.tutor.overhang.io/plugins/>`__:

.. code-block:: python
from tutor import hooks
Expand Down
120 changes: 72 additions & 48 deletions openedxscorm/scormxblock.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand All @@ -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)

Expand All @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"

Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -553,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)
Expand All @@ -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(
f"{prefix}organizations/{prefix}organization"
)
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'<script\b[^>]*>(.*?)</script>', '', input_str, flags=re.IGNORECASE)
sanitized_str = re.sub(
r"<script\b[^>]*>(.*?)</script>", "", 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
Expand All @@ -603,28 +614,32 @@ 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")
# If item does not have a resource, we don't need to make it into a link
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(
f"{prefix}resources/{prefix}resource[@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
Expand All @@ -642,29 +657,42 @@ def format(items, tabs=1):
if type(items) is tuple:
title, resource_url = items[0], items[1]
if resource_url != "#":
return "{indent}<li href='{resource_url}' class='navigation-title'>{title}</li>".format(indent=indent, resource_url=resource_url, title=title)
return "{indent}<li class='navigation-title-header'>{title}</li>".format(indent=indent, title=title)

return f"{indent}<li href='{resource_url}' class='navigation-title'>{title}</li>"

return f"{indent}<li class='navigation-title-header'>{title}</li>"

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}<ul>\n{indent}<li href='{resource_url}' class='navigation-title'>{title}</li>\n{indent}<ul>\n{indent}\n{output}</ul>\n{indent}</ul>".format(indent=indent, resource_url=resource_url, title=title, output="\n".join(output))
return "\n{indent}<ul>\n{indent}<li class='navigation-title-header'>{title}</li>\n{indent}<ul>\n{indent}\n{output}</ul>\n{indent}</ul>".format(indent=indent, resource_url=resource_url, title=title, output="\n".join(output))
return "\n{indent}<ul>\n{indent}<li href='{resource_url}' class='navigation-title'>{title}</li>\n{indent}<ul>\n{indent}\n{output}</ul>\n{indent}</ul>".format(
indent=indent,
resource_url=resource_url,
title=title,
output="\n".join(output),
)
return "\n{indent}<ul>\n{indent}<li class='navigation-title-header'>{title}</li>\n{indent}<ul>\n{indent}\n{output}</ul>\n{indent}</ul>".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}<ul>\n{output}\n{indent}</ul>".format(indent=indent, output="\n".join(output))

output.append(format(item, tabs + 1))
return "{indent}\n{indent}<ul>\n{output}\n{indent}</ul>".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):
Expand All @@ -677,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):
Expand Down Expand Up @@ -760,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]
]
Expand All @@ -777,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(
Expand All @@ -787,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:
Expand Down Expand Up @@ -869,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


Expand Down
Loading

0 comments on commit f81b9e7

Please sign in to comment.