Task/sapnamysore/tlt 3253/upgrade django #44

sapnamysore · 2018-02-05T17:03:17Z

Depends on https://github.com/Harvard-University-iCommons/django-canvas-lti-school-permissions/pull/11

Note: Do not merge till we have done some testing for a bit

…pgjson library .

sapnamysore · 2018-02-06T18:37:56Z

@bermudezjd , @cmurtaugh , @Chris-Thornton-Harvard

bermudezjd

Looks good! Just a few changes and minor suggestions.

bermudezjd · 2018-02-07T15:40:22Z

canvas_manage_course/requirements/base.txt

-Django==1.9.12
-cx-Oracle==5.2.1
+Django==1.11.9
+cx-Oracle==6.0.3


Latest version of cx_Oracle is now 6.1 I believe

bermudezjd · 2018-02-07T15:41:21Z

canvas_manage_course/requirements/base.txt

+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 , thgat is still in a commit but is not in the released version. Unable to specify


Minor spelling error for that

bermudezjd · 2018-02-07T16:58:16Z

canvas_manage_course/views.py

-    url = "%s://%s%s" % (request.scheme, request.get_host(),
-                         reverse('lti_launch', exclude_resource_link_id=True))
+
+    if request.is_secure():


FWIW, for our hosted apps, this will always be true since we have a redirect in nginx from http to https.

bermudezjd · 2018-02-07T17:00:59Z

canvas_manage_course/views.py

+    else:
+        host = 'http://' + request.get_host()
+
+    url = host + reverse('lti_launch')


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.

bermudezjd · 2018-02-07T17:03:17Z

canvas_manage_course/urls.py

@@ -15,3 +16,9 @@
    url(r'^not_authorized$', icommons_ui_views.not_authorized, name='not_authorized'),
    url(r'^tool_config$', views.tool_config, name='tool_config'),
 ]
+
+if settings.DEBUG:
+    import debug_toolbar


Can we add put this in a try/except block? We normally don't require the toolbar in our aws environments, although we might want to run in DEBUG mode on a non-prod deployed instance.

bermudezjd · 2018-02-07T17:04:14Z

canvas_manage_course/requirements/base.txt

-Django==1.9.12
-cx-Oracle==5.2.1
+Django==1.11.9
+cx-Oracle==6.0.3
 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


Would you mind removing the ndg-httpsclient library? We don't need it on Xenial.

Chris-Thornton-Harvard · 2018-02-07T18:56:47Z

canvas_manage_course/requirements/base.txt

+# 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 , thgat 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


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

@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.

sapnamysore · 2018-02-07T22:19:57Z

@bermudezjd : Made some changes based on the PR feedback.

bermudezjd

Few minor nitpicks which would be nice to address, but looks good enough to merge.

bermudezjd · 2018-02-08T03:54:03Z

canvas_manage_course/views.py

-                         reverse('lti_launch', exclude_resource_link_id=True))
+
+    host = 'https://' + request.get_host()
+    url = host + reverse('lti_launch')


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'))

bermudezjd · 2018-02-08T03:58:27Z

canvas_manage_course/urls.py

@@ -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:


Can you move the try block under the if?

bermudezjd · 2018-02-08T04:02:01Z

canvas_manage_course/urls.py

+            url(r'^__debug__/', include(debug_toolbar.urls)),
+        ]
+except:
+    pass


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.

…ranch

cmurtaugh

👍

sapnamysore added 3 commits February 1, 2018 13:26

Upgrading Django and other libraries

70d06be

Handled the issue with importing the fix for Django>=1.10 for django-…

945cbaa

…pgjson library .

Merge branch 'develop' into task/sapnamysore/tlt-3253/upgrade-django

5df17e1

sapnamysore requested review from cmurtaugh, bermudezjd and Chris-Thornton-Harvard February 6, 2018 18:56

bermudezjd self-assigned this Feb 6, 2018

bermudezjd requested changes Feb 7, 2018

View reviewed changes

Chris-Thornton-Harvard reviewed Feb 7, 2018

View reviewed changes

Changes per PR feedback

045d12b

bermudezjd approved these changes Feb 8, 2018

View reviewed changes

sapnamysore and others added 6 commits February 16, 2018 11:42

Testing switching to new JSON field

2f81f39

Removed pgjson requirement from base and pointed icommons-common to b…

521bdc4

…ranch

Context instance is deprecated 1.10+

8836bd8

Code cleanup

0c58121

Updated migration to use boolean values

96d3d81

PR touchups

41c28a8

cmurtaugh approved these changes Mar 8, 2018

View reviewed changes

Chris-Thornton-Harvard merged commit 1784f2e into develop Mar 8, 2018

Chris-Thornton-Harvard deleted the task/sapnamysore/tlt-3253/upgrade-django branch April 11, 2018 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/sapnamysore/tlt 3253/upgrade django #44

Task/sapnamysore/tlt 3253/upgrade django #44

sapnamysore commented Feb 5, 2018 •

edited

Loading

sapnamysore commented Feb 6, 2018

bermudezjd left a comment

bermudezjd Feb 7, 2018

bermudezjd Feb 7, 2018

bermudezjd Feb 7, 2018

bermudezjd Feb 7, 2018

bermudezjd Feb 7, 2018

bermudezjd Feb 7, 2018

Chris-Thornton-Harvard Feb 7, 2018 •

edited

Loading

sapnamysore Feb 7, 2018

sapnamysore commented Feb 7, 2018

bermudezjd left a comment

bermudezjd Feb 8, 2018

bermudezjd Feb 8, 2018

bermudezjd Feb 8, 2018

cmurtaugh left a comment

Task/sapnamysore/tlt 3253/upgrade django #44

Task/sapnamysore/tlt 3253/upgrade django #44

Conversation

sapnamysore commented Feb 5, 2018 • edited Loading

sapnamysore commented Feb 6, 2018

bermudezjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Thornton-Harvard Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapnamysore commented Feb 7, 2018

bermudezjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmurtaugh left a comment

Choose a reason for hiding this comment

sapnamysore commented Feb 5, 2018 •

edited

Loading

Chris-Thornton-Harvard Feb 7, 2018 •

edited

Loading