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

Upgrade to Django 3.2 and bump base image to v3.10 #329

Conversation

PatrickMTonne
Copy link
Contributor

@PatrickMTonne PatrickMTonne commented Jun 29, 2022

Resolves TLT-4240 and TLT-4260.

Depends on https://github.com/Harvard-University-iCommons/django-icommons-common/pull/216.

Summary

This PR:

  • Upgrades the app to Django 3.2.
  • Updates the Dockerfile to use v3.10 of the build and base images.
  • Replaces django-sslserver with django-extensions since the former is incompatible with Django 3.x.
  • Uses an updated branch of django-icommons-common that includes minor updates to maintain compatibility with Django 3.2.
  • Uses a forked version of django-angular (for reasons explained below).
  • Removes use of debug_toolbar from local.py (for reasons explained below).

Resolving incompatibilities with django-angular and Django 3.2

After some research, it seems that django-angular does not officially support Django 3.2 (more specifically, any version of Django >3.0). According to an issue on the project's GitHub repo (which I came upon after running into the listed error during the upgrade), this appears to be because the primary project maintainer (and creator?) has since developed a different library (django-formset) to serve a similar purpose and no longer maintains the django-angular project.

Since the project's no longer maintained, there was an initial goal to find a way to remove uses of this library from the application. While this initially seemed promising, it was later discovered that CAAT's AngularJS frontend relies heavily on the library's middleware component for reversing URLs (e.g. var URL_LISTS = djangoUrl.reverse('mailing_list:api_lists');).

So, for now at least, we still need this library to keep things functioning. The next step was to determine if there's a way to continue using this library despite its incompatibility with Django 3.2. Through trial and error, two primary errors were addressed:

  • Running the tool locally, django-angular raised an error on startup when attempting to render a form by request from debug_toolbar.middleware.DebugToolbarMiddleware (see the issue mentioned earlier: module 'django.forms.forms' has no attribute 'BoundField' jrief/django-angular#357). Stopping use of debug_toolbar resolved the error.
  • Using the Bulk Course Settings component of CAAT, clicking "Create New Job" raised the same form error as above. To resolve this, I found that removing FORM_RENDERER = 'djng.forms.renderers.DjangoAngularBootstrap3Templates' from base.py fixed the issue. Note that removing this setting bypasses django-angular's form renderer (which is not compatible with Django 3.2) in favor of Django's default form renderer (note that overriding Django's form renderer is a listed step on the library's installation guide).

The two fixes above (removing debug_toolbar to resolve issues when running locally and removing the FORM_RENDERER override from base.py to resolve the failed rendering of a "Create New Job" form) ultimately resolved all errors.

Note that there was some experimentation to update the forked django-angular library itself to determine how easy it would be to resolve some of the Django incompatibilities. Long story short, barring additional time and research, the only clear fix here is to bypass the library's incompatible form renderer via the method above.

Forking the repo

On a separate note, It was determined in a meeting with Chris that forking the repo would be a good way to ensure we always have a local copy of the code (lest something happens to the original repo), so this particular branch is pointed to a newly-tagged version of the forked repo (v2.3). This tag points to the merge commit for release/2.3 (commit in original source repo here: Harvard-University-iCommons/django-angular@b825b80).

Oddly enough, the django-angular repo did not receive a 2.3 tag (the latest tagged version in the source repo is 2.2.4: https://github.com/jrief/django-angular/tags), yet PyPI provides a 2.3 version of the library (https://pypi.org/project/django-angular/), presumably containing the changes merged in by the release/2.3 branch. I'm not sure of the reason for this discrepancy (maybe the maintainer just got lazy near the end?), but it's an interesting thing to note.

Testing

This branch is currently running on QA. Testing was performed via manual use of each of the project's apps (Search Courses, Search People, etc.).

Example sub-account in QA for testing: https://canvas.qa.tlt.harvard.edu/accounts/11/external_tools/3.

@Chris-Thornton-Harvard
Copy link
Contributor

👍 Looks good. Tested out the cards in QA
Would you mind adding a couple of Jira tickets to track re adding the debug toolbar and maybe another to revisit our forked version of django-angular and see if we can get working? They can probably be part of the same ticket

@PatrickMTonne
Copy link
Contributor Author

Thanks for taking a look @Chris-Thornton-Harvard! I'm testing things out with the latest version of django-debug-toolbar (released a few weeks ago) and things seem to be working more smoothly with this version. There's still an occasional 400 HTTP response from the toolbar, but it doesn't crash the app like before (you can simply close out the toolbar window that pops out).

So I added in a new commit that puts the toolbar back into the app (see 8f9d805). I figure if it works the majority of the time, that's fine. We should only remove it if it inhibits development (as it did in my prior testing). If it's okay with you, I'll include this when I merge the PR.

And yeah, I'll create a JIRA ticket to investigate modifying the forked django-angular project.

@Chris-Thornton-Harvard
Copy link
Contributor

Thats great to hear! I agree, lets keep that included. Thanks for the update on that

@PatrickMTonne PatrickMTonne merged commit b6c90cb into develop Jul 22, 2022
@PatrickMTonne PatrickMTonne deleted the patricktonne/upgrade-to-django-3.2-and-base-image-to-v3.10 branch July 22, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants