From 6c70faed314eaa85ac5a5360d048b3b93d7f4f52 Mon Sep 17 00:00:00 2001 From: Shea Silverman Date: Mon, 4 Nov 2019 16:16:19 -0500 Subject: [PATCH 1/6] Python 3 Conversion --- lti.py | 3 +-- requirements.txt | 2 +- tests.py | 42 +++++++++++++++++++++--------------------- 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/lti.py b/lti.py index c80fa52..3e3afc9 100644 --- a/lti.py +++ b/lti.py @@ -300,8 +300,7 @@ def status(): app.logger.exception("Dev Key check failed.") # Overall health check - if all checks are True - status["healthy"] = all(v is True for k, v in status["checks"].items()) - + status["healthy"] = all(v is True for k, v in list(status["checks"].items())) return Response(json.dumps(status), mimetype="application/json") diff --git a/requirements.txt b/requirements.txt index 7512fff..2482caa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,6 @@ canvasapi==0.14.0 Flask==1.1.1 Flask-SQLAlchemy==2.4.0 -mysql-python==1.2.5 +mysqlclient -e git+https://github.com/ucfcdl/pylti.git@roles#egg=PyLTI requests==2.22.0 diff --git a/tests.py b/tests.py index 5059e0a..5d21fc4 100644 --- a/tests.py +++ b/tests.py @@ -1,6 +1,6 @@ import logging import unittest -from urllib import urlencode +from urllib.parse import urlencode import canvasapi import oauthlib.oauth1 @@ -220,7 +220,7 @@ def test_index_no_auth(self, m): self.assert_template_used("error.html") self.assertIn( - "Authentication error, please refresh and try again", response.data + b"Authentication error, please refresh and try again", response.data ) def test_index_api_key_none(self, m): @@ -234,7 +234,7 @@ def test_index_api_key_none(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Authentication error: missing API key. Please refresh and try again.", + b"Authentication error: missing API key. Please refresh and try again.", response.data, ) @@ -276,7 +276,7 @@ def test_index_api_key_invalid(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "You are not enrolled in this course as a Teacher or Designer.", + b"You are not enrolled in this course as a Teacher or Designer.", response.data, ) @@ -322,7 +322,7 @@ def test_index_no_canvas_conn(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Couldn't connect to Canvas, please refresh and try again", + b"Couldn't connect to Canvas, please refresh and try again", response.data, ) @@ -366,7 +366,7 @@ def test_index_whitelist_error(self, m, filter_tool_list): self.assert_template_used("error.html") self.assertIn( - "There is something wrong with the whitelist.json file", response.data + b"There is something wrong with the whitelist.json file", response.data ) @patch("lti.filter_tool_list") @@ -410,7 +410,7 @@ def test_index_canvas_error(self, m, filter_tool_list): response = self.client.get(self.generate_launch_request(url_for("index"))) self.assert_template_used("error.html") - self.assertIn("Couldn't connect to Canvas", response.data) + self.assertIn(b"Couldn't connect to Canvas", response.data) def test_index(self, m): with self.client.session_transaction() as sess: @@ -472,7 +472,7 @@ def test_status_healthy(self, m): self.assertIn("checks", json_response) self.assertIsInstance(json_response["checks"], dict) self.assertEqual(len(json_response["checks"]), 4) - for check, is_ok in json_response["checks"].items(): + for check, is_ok in list(json_response["checks"].items()): self.assertTrue(is_ok) self.assertIn("healthy", json_response) self.assertTrue(json_response["healthy"]) @@ -499,7 +499,7 @@ def test_status_failures(self, m): self.assertIn("checks", json_response) self.assertIsInstance(json_response["checks"], dict) self.assertEqual(len(json_response["checks"]), 4) - for check, is_ok in json_response["checks"].items(): + for check, is_ok in list(json_response["checks"].items()): self.assertFalse(is_ok) self.assertIn("healthy", json_response) self.assertFalse(json_response["healthy"]) @@ -530,7 +530,7 @@ def test_oauth_login_cancelled(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Authentication error, please refresh and try again.", response.data + b"Authentication error, please refresh and try again.", response.data ) def test_oauth_login_no_access_token(self, m): @@ -552,7 +552,7 @@ def test_oauth_login_no_access_token(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Authentication error, please refresh and try again.", response.data + b"Authentication error, please refresh and try again.", response.data ) def test_oauth_login_new_user(self, m): @@ -650,7 +650,7 @@ def test_oauth_login_new_user_db_error(self, m): self.assert_template_used("error.html") self.assertIn( - "Authentication error, please refresh and try again.", response.data + b"Authentication error, please refresh and try again.", response.data ) def test_oauth_login_existing_user(self, m): @@ -758,7 +758,7 @@ def test_oauth_login_existing_user_db_error(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Authentication error, please refresh and try again.", response.data + b"Authentication error, please refresh and try again.", response.data ) # refresh_access_token @@ -1149,7 +1149,7 @@ def test_get_sessionless_url_is_course_nav_fail(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Error in a response from Canvas, please refresh and try again.", + b"Error in a response from Canvas, please refresh and try again.", response.data, ) @@ -1178,7 +1178,7 @@ def test_get_sessionless_url_is_course_nav_succeed(self, m): ) self.assert_200(response) - self.assertEqual(response.data, launch_url) + self.assertEqual(response.data, launch_url.encode()) def test_get_sessionless_url_not_course_nav_fail(self, m): with self.client.session_transaction() as sess: @@ -1204,7 +1204,7 @@ def test_get_sessionless_url_not_course_nav_fail(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - "Error in a response from Canvas, please refresh and try again.", + b"Error in a response from Canvas, please refresh and try again.", response.data, ) @@ -1233,7 +1233,7 @@ def test_get_sessionless_url_not_course_nav_succeed(self, m): ) self.assert_200(response) - self.assertEqual(response.data, launch_url) + self.assertEqual(response.data, launch_url.encode()) class UtilsTests(unittest.TestCase): @@ -1243,13 +1243,13 @@ def setUpClass(cls): settings.whitelist = "whitelist.json" def test_filter_tool_list_empty_file(self): - with self.assertRaisesRegexp(ValueError, r"No JSON object could be decoded"): - with patch("__builtin__.open", mock_open(read_data="")): + with self.assertRaisesRegex(ValueError, r"No JSON object could be decoded"): + with patch("builtins.open", mock_open(read_data="")): utils.filter_tool_list(1, "password") def test_filter_tool_list_empty_data(self): - with self.assertRaisesRegexp(ValueError, r"whitelist\.json is empty"): - with patch("__builtin__.open", mock_open(read_data="{}")): + with self.assertRaisesRegex(ValueError, r"whitelist\.json is empty"): + with patch("builtins.open", mock_open(read_data="{}")): utils.filter_tool_list(1, "password") @patch("canvasapi.canvas.Canvas.get_course") From b56c279789b84725bcf421061b46a13b6798c4e2 Mon Sep 17 00:00:00 2001 From: Matthew Emond Date: Tue, 5 Nov 2019 11:03:42 -0500 Subject: [PATCH 2/6] small test tweaks to reach py2 pairty --- .gitignore | 1 + requirements.txt | 2 +- setup.cfg | 2 ++ tests.py | 13 +++++++------ 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 9adaf5f..0e980ad 100644 --- a/.gitignore +++ b/.gitignore @@ -110,6 +110,7 @@ celerybeat-schedule .env .venv env/ +env2/ env3/ venv/ ENV/ diff --git a/requirements.txt b/requirements.txt index 2482caa..e6c8fdd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,6 @@ canvasapi==0.14.0 Flask==1.1.1 -Flask-SQLAlchemy==2.4.0 +Flask-SQLAlchemy==2.4.1 mysqlclient -e git+https://github.com/ucfcdl/pylti.git@roles#egg=PyLTI requests==2.22.0 diff --git a/setup.cfg b/setup.cfg index e3fa53f..b9a1755 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,7 @@ [flake8] exclude= env/* + env2/* env3/* src/* max_line_length=99 @@ -9,5 +10,6 @@ ignore = W503, E203 [coverage:run] omit= env/* + env2/* env3/* src/* diff --git a/tests.py b/tests.py index 5d21fc4..c7c09e4 100644 --- a/tests.py +++ b/tests.py @@ -1,3 +1,4 @@ +from json.decoder import JSONDecodeError import logging import unittest from urllib.parse import urlencode @@ -322,7 +323,7 @@ def test_index_no_canvas_conn(self, m): self.assert_200(response) self.assert_template_used("error.html") self.assertIn( - b"Couldn't connect to Canvas, please refresh and try again", + b"Couldn't connect to Canvas, please refresh and try again", response.data, ) @@ -472,7 +473,7 @@ def test_status_healthy(self, m): self.assertIn("checks", json_response) self.assertIsInstance(json_response["checks"], dict) self.assertEqual(len(json_response["checks"]), 4) - for check, is_ok in list(json_response["checks"].items()): + for check, is_ok in json_response["checks"].items(): self.assertTrue(is_ok) self.assertIn("healthy", json_response) self.assertTrue(json_response["healthy"]) @@ -499,7 +500,7 @@ def test_status_failures(self, m): self.assertIn("checks", json_response) self.assertIsInstance(json_response["checks"], dict) self.assertEqual(len(json_response["checks"]), 4) - for check, is_ok in list(json_response["checks"].items()): + for check, is_ok in json_response["checks"].items(): self.assertFalse(is_ok) self.assertIn("healthy", json_response) self.assertFalse(json_response["healthy"]) @@ -1178,7 +1179,7 @@ def test_get_sessionless_url_is_course_nav_succeed(self, m): ) self.assert_200(response) - self.assertEqual(response.data, launch_url.encode()) + self.assertEqual(response.data, launch_url.encode("utf-8")) def test_get_sessionless_url_not_course_nav_fail(self, m): with self.client.session_transaction() as sess: @@ -1233,7 +1234,7 @@ def test_get_sessionless_url_not_course_nav_succeed(self, m): ) self.assert_200(response) - self.assertEqual(response.data, launch_url.encode()) + self.assertEqual(response.data, launch_url.encode("utf-8")) class UtilsTests(unittest.TestCase): @@ -1243,7 +1244,7 @@ def setUpClass(cls): settings.whitelist = "whitelist.json" def test_filter_tool_list_empty_file(self): - with self.assertRaisesRegex(ValueError, r"No JSON object could be decoded"): + with self.assertRaises(JSONDecodeError): with patch("builtins.open", mock_open(read_data="")): utils.filter_tool_list(1, "password") From 816b8eff42c910d0ca1df025d8962721d01ae08e Mon Sep 17 00:00:00 2001 From: Matthew Emond Date: Wed, 6 Nov 2019 15:38:43 -0500 Subject: [PATCH 3/6] Increase log file size from 10kB to 5MB --- settings.py.template | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/settings.py.template b/settings.py.template index f8bc57e..3a4754b 100644 --- a/settings.py.template +++ b/settings.py.template @@ -41,8 +41,8 @@ oauth2_id = "" oauth2_key = "" # Logging configuration -LOG_MAX_BYTES = 10000 -LOG_BACKUP_COUNT = 1 +LOG_MAX_BYTES = 1024 * 1024 * 5, # 5 MB +LOG_BACKUP_COUNT = 2 ERROR_LOG = "logs/faculty-tools.log" whitelist = "whitelist.json" From 1cf52e687179d393b1777245a0e2e87206d15900 Mon Sep 17 00:00:00 2001 From: Shea Silverman Date: Wed, 13 Nov 2019 12:33:16 -0500 Subject: [PATCH 4/6] setup.cfg --- setup.cfg | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.cfg b/setup.cfg index b9a1755..494349c 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,5 +1,6 @@ [flake8] exclude= + venv*/* env/* env2/* env3/* @@ -9,6 +10,7 @@ ignore = W503, E203 [coverage:run] omit= + venv*/* env/* env2/* env3/* From 0972ec99eb5b495934358780c3527e2462631c7e Mon Sep 17 00:00:00 2001 From: Matthew Emond Date: Wed, 11 Dec 2019 11:11:35 -0500 Subject: [PATCH 5/6] Order of tools and categories is now directly tied to the order in the whitelist. --- lti.py | 5 ++- templates/lti_list.html | 3 +- templates/main_template.html | 22 +++++----- utils.py | 80 +++++++++++++++++++++++------------- whitelist.json.template | 31 +++++++------- 5 files changed, 83 insertions(+), 58 deletions(-) diff --git a/lti.py b/lti.py index 3e3afc9..964cccf 100644 --- a/lti.py +++ b/lti.py @@ -226,7 +226,9 @@ def index(lti=lti): ) try: - tools_by_category = filter_tool_list(session["course_id"], session["api_key"]) + tools_by_category, cagetory_order = filter_tool_list( + session["course_id"], session["api_key"] + ) except CanvasException: app.logger.exception("Couldn't connect to Canvas") return return_error( @@ -243,6 +245,7 @@ def index(lti=lti): return render_template( "main_template.html", tools_by_category=tools_by_category, + category_order=cagetory_order, course=session["course_id"], ) diff --git a/templates/lti_list.html b/templates/lti_list.html index 7a31a4a..c2cc9e9 100644 --- a/templates/lti_list.html +++ b/templates/lti_list.html @@ -44,9 +44,8 @@

{{lti.display_name}}

{% endfor %} {% else %}
-
+

No LTIs available in this category.

-
{% endif %} diff --git a/templates/main_template.html b/templates/main_template.html index ee2c50f..8dd5c5e 100644 --- a/templates/main_template.html +++ b/templates/main_template.html @@ -4,7 +4,7 @@

Jump to section:

    - {% for category in tools_by_category %} + {% for category in category_order %}
  • {{ category }}
    • {% endfor %} {% include "extra_links.html" ignore missing %} @@ -14,18 +14,16 @@

    Jump to section:

    {% block content %} -{% for category, tools in tools_by_category.items() %} -
    -
    -

    {{ category }}

    -
    +{% for category in category_order %} +
    +
    +

    {{ category }}

    +
    +
    -
    - -{% with ltis=tools %} - {% include "lti_list.html" %} -{% endwith %} - + {% with ltis=tools_by_category[category] %} + {% include "lti_list.html" %} + {% endwith %} {% endfor %} {% include "custom_tools.html" ignore missing %} diff --git a/utils.py b/utils.py index ac7f083..dcaee44 100644 --- a/utils.py +++ b/utils.py @@ -7,6 +7,23 @@ import settings +def get_tool_info(whitelist, tool_name): + """ + Search the whitelist by tool name. + + :returns: A dictionary , or none if no tool matching that name was found. + :rtype: dict or None + """ + for category, category_tools in whitelist.items(): + for tool_info in category_tools: + print(tool_info) + if tool_info.get("name") == tool_name: + tool_info.update({"category": category}) + return tool_info + + return None + + def filter_tool_list(course_id, access_token): """ Filter tool list down to those on whitelist and sort by category. @@ -31,38 +48,45 @@ def filter_tool_list(course_id, access_token): course = canvas.get_course(course_id) installed_tools = course.get_external_tools(include_parents=True) - tools_by_category = defaultdict(list) for installed_tool in installed_tools: - for tool in whitelist: - if installed_tool.name != tool.get("name"): - continue - - is_course_navigation = hasattr(installed_tool, "course_navigation") - - if tool.get("is_launchable", False): - if is_course_navigation: - sessionless_launch_url = installed_tool.get_sessionless_launch_url( - launch_type="course_navigation" - ) - else: - sessionless_launch_url = installed_tool.get_sessionless_launch_url() - else: - sessionless_launch_url = None + tool_info = get_tool_info(whitelist, installed_tool.name) + if not tool_info: + continue - tool_info = tool - tool_info.update( - { - "id": installed_tool.id, - "lti_course_navigation": is_course_navigation, - "sessionless_launch_url": sessionless_launch_url, - "screenshot": "screenshots/" + tool["screenshot"], - } - ) + is_course_navigation = hasattr(installed_tool, "course_navigation") - tools_by_category[tool.get("category", "Uncategorized")].append(tool_info) - - return tools_by_category + if tool_info.get("is_launchable", False): + if is_course_navigation: + sessionless_launch_url = installed_tool.get_sessionless_launch_url( + launch_type="course_navigation" + ) + else: + sessionless_launch_url = installed_tool.get_sessionless_launch_url() + else: + sessionless_launch_url = None + + tool_info.update( + { + "id": installed_tool.id, + "lti_course_navigation": is_course_navigation, + "sessionless_launch_url": sessionless_launch_url, + "screenshot": "screenshots/" + tool_info["screenshot"], + } + ) + + tools_by_category[tool_info.get("category", "Uncategorized")].append(tool_info) + + for category, tools in tools_by_category.items(): + # Determine tool order based on order in whitelist + order = { + tool.get("display_name"): i for i, tool in enumerate(whitelist[category]) + } + tools_by_category[category] = sorted( + tools, key=lambda k: order[k["display_name"]] + ) + + return tools_by_category, list(whitelist.keys()) def slugify(value): diff --git a/whitelist.json.template b/whitelist.json.template index c717551..6f86482 100644 --- a/whitelist.json.template +++ b/whitelist.json.template @@ -1,15 +1,16 @@ -[ - { - "display_name": "Name to Display", - "name": "Installed Tool Name", - "tool_id": "tool_id", - "allowed_roles": [""], - "desc": "Tool Description", - "screenshot": "screenshot.png", - "logo": "logo.svg", - "filter_by": ["all"], - "docs_url": "https://example.com/tool/docs/", - "is_launchable": true, - "category": "Course Tool" - } -] +{ + "Course Tool": [ + { + "display_name": "Name to Display", + "name": "Installed Tool Name", + "tool_id": "tool_id", + "allowed_roles": [""], + "desc": "Tool Description", + "screenshot": "screenshot.png", + "logo": "logo.svg", + "filter_by": ["all"], + "docs_url": "https://example.com/tool/docs/", + "is_launchable": true + } + ] +} From e92231278b2062b558de8f4cee9549a87b5501e3 Mon Sep 17 00:00:00 2001 From: Matthew Emond Date: Mon, 16 Dec 2019 13:52:56 -0500 Subject: [PATCH 6/6] bump canvasapi version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index e6c8fdd..4543aad 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ -canvasapi==0.14.0 +canvasapi==0.15.0 Flask==1.1.1 Flask-SQLAlchemy==2.4.1 mysqlclient