Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task/sapnamysore/tlt 3253/upgrade django #44

Merged
24 changes: 14 additions & 10 deletions canvas_manage_course/requirements/base.txt
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
boto3==1.4.4
Django==1.9.12
cx-Oracle==5.2.1
Django==1.11.9
cx-Oracle==6.1
django-cached-authentication-middleware==0.2.1
django-redis-cache==1.7.1
hiredis==0.2.0
kitchen==1.2.4
ndg-httpsclient==0.4.2
redis==2.10.5
psycopg2==2.6.2
redis==2.10.6
psycopg2==2.7.3.2
requests==2.13.0

git+ssh://[email protected]/penzance/[email protected]#egg=canvas-python-sdk==0.8.4
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-auth-lti==1.2.5
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-icommons-common[async]==1.18.2
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-icommons-ui==1.3.1
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-canvas-lti-school-permissions==0.5.2
git+ssh://[email protected]/penzance/[email protected]#egg=canvas-python-sdk==0.10.2
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-auth-lti==1.2.9

# NOTE: Including the django-pgjson library here. This is required by 'async' within django-icommons-common, but
# there is fix for a bug for django>=1.10 , that is still in a commit but is not in the released version. Unable to specify
# a commit in the django-icommons-common's setup.py, hence including it in the consuming app till we used switch from pjson to standard json
git+ssh://[email protected]/djangonauts/django-pgjson.git@30463d210a42b2de#egg=django-pgjson
Copy link
Contributor

@Chris-Thornton-Harvard Chris-Thornton-Harvard Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of pgjson uses import_by_path which is deprecated in Django 1.10+. Causes the deploy to fail.
djangonauts/django-pgjson@30463d210a42b2de#diff-09ac573aadd5de6bf33fc0a97964399a

djangonauts/django-pgjson#45

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chris-Thornton-Harvard : So the latest Release (tagged 0.31) of django-pgjson from July 2015 ((https://github.com/djangonauts/django-pgjson/releases/tag/0.3.1) uses the deprecated import_by_path. The second link you have is the reported issue, but is still open.

I found the link to this particular commit - which a workaround to this issue(Details in the last comment here djangonauts/django-pgjson#43 ), it has been merged to master but not ben tagged. It worked locally for me, will check why it's failing while we deploy to dev. Thanks for catching it.

git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-icommons-common[async]==1.33.1
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-icommons-ui==1.5.2
git+ssh://[email protected]/Harvard-University-iCommons/[email protected]#egg=django-canvas-lti-school-permissions==0.6
2 changes: 1 addition & 1 deletion canvas_manage_course/requirements/local.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# below are requirements specific to the local environment

ddt==1.1.1
django-debug-toolbar==1.5
django-debug-toolbar==1.8
django-sslserver==0.19
mock==2.0.0
oauthlib==1.1.1
Expand Down
11 changes: 11 additions & 0 deletions canvas_manage_course/urls.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.conf.urls import include, url
from django.conf import settings

from canvas_manage_course import views
from icommons_ui import views as icommons_ui_views
Expand All @@ -15,3 +16,13 @@
url(r'^not_authorized$', icommons_ui_views.not_authorized, name='not_authorized'),
url(r'^tool_config$', views.tool_config, name='tool_config'),
]

try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the try block under the if?

if settings.DEBUG:
import debug_toolbar
urlpatterns += [
url(r'^__debug__/', include(debug_toolbar.urls)),
]
except:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to have a small comment here so we can remember why we don't care about handling this exception in case we revisit the file.


9 changes: 5 additions & 4 deletions canvas_manage_course/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
import logging

from django.contrib.auth.decorators import login_required
from django.core.urlresolvers import reverse
from django.http import HttpResponse
from django.shortcuts import redirect, render
from django.urls import reverse
from django.views.decorators.csrf import csrf_exempt
from django.views.decorators.http import require_http_methods

from ims_lti_py.tool_config import ToolConfig

from isites_migration.utils import get_previous_isites
Expand All @@ -21,8 +20,10 @@

@require_http_methods(['GET'])
def tool_config(request):
url = "%s://%s%s" % (request.scheme, request.get_host(),
reverse('lti_launch', exclude_resource_link_id=True))

host = 'https://' + request.get_host()
url = host + reverse('lti_launch')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that we'd always want our tool launch urls (this one, at least) to be over ssl. IIRC, Canvas prefers/demands that LTI tools be running over secure connections - maybe it's actually browsers that enforce that due to mixed content warnings/errors. In any case, I'd be tempted to simply make the url https here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, although using string formatting reads better to me because I can more easily visualize the resulting string. That said, it's equivalent to the concats in place so no pressure to change:
url = "https://{0}{1}".format(request.get_host(), reverse('lti_launch'))


title = 'Manage Course'
lti_tool_config = ToolConfig(
title=title,
Expand Down