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 1.9 #283

Merged
merged 1 commit into from
Jul 27, 2016
Merged

Conversation

IllyaMoskvin
Copy link
Member

@IllyaMoskvin IllyaMoskvin commented May 2, 2016

This is a fairly extensive commit, since most of the requirements had
to be upgraded as well. Dependencies have been locked down to specific
versions for consistency and troubleshooting. Notable changes:

  • Moved wsgi.py to a more standard location, updated apache config.
    Now referenced in roundware.settings.WSGI_APPLICATION
  • For roundware.api2, implemented the new Application Configuration
    structure. See http://stackoverflow.com/q/32795227/1943591
  • django-guardian now creates its own AnonymousUser in the database.
    Removed AnonymousUser references from fixtures to avoid conflicts.
    http://django-guardian.readthedocs.io/en/stable/configuration.html
  • django-guardian assign is being depricated for assign_perm
    See django-guardian/django-guardian@1419048
  • ManyToManyField no longer accepts null=True
  • django.contrib.auth.models.User should not be accessed directly.
    Use django.conf.settings.AUTH_USER_MODEL instead
  • Django changed the way it loads models, which broke streaming.
    Moving django.setup() in roundwared.rwstreamd fixed the issue.
  • django_chartit is no longer maintained. Moved to django_chartit2
    This may require us to install jQuery and Highcharts manually
  • django.forms.models.save_instance has been removed.
    We are now calling save() instead, but it might need more testing.
    django/django@8656cf
    django/django@b11564
  • Fixed errors in Travis install script (thanks @hburgund)
  • Fixed Missing core models from admin after adding adminplus jsocol/django-adminplus#42
  • Fixed django-admin-bootstrapped dependency
    See roundware/django-admin-bootstrapped
  • Fixed queryset filtering in ProjectProtectedThrough classes in admin.py
    Added explicit permission-based queryset filtering to ProjectAdmin
  • Fixed duplicate rows in auth_permission, removed default_auth fixtures.
  • Removed fix-m2m-deserial.patch

This closes #229, fixes #291, and supersedes #282.

@@ -19,6 +19,47 @@ Done!
The following instructions describe modifications to the standard upgrade process required due to
specific changes. Items are listed in reverse chronological order.

## 4/28/16 - Upgrade Django from 1.7 to 1.9
Copy link
Member

Choose a reason for hiding this comment

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

Please use three ### to keep the formatting consistent with the other upgrade headers.

@hburgund
Copy link
Member

hburgund commented May 2, 2016

Anyone know what the Travis error is all about?

AttributeError: 'module' object has no attribute 'packaging'

@13rac1
Copy link
Member

13rac1 commented May 2, 2016

The AttributeError looks to be incompatible versions of pkg_resources and setuptools:

  File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/setuptools/dist.py", line 21, in <module>
    packaging = pkg_resources.packaging

http://stackoverflow.com/questions/28967785/attribute-error-installing-with-pip

@hburgund
Copy link
Member

hburgund commented May 4, 2016

thx @eosrei - I'm a bit confused as to how this can be fixed since I assume it isn't happening anywhere other than in Travis, correct @IllyaMoskvin? I am thinking perhaps it is due to Travis using an old version of Ubuntu (12.04) by default. @jslootbeek since you did great work fixing Travis for #273, perhaps you can weigh in on this issue?

@13rac1
Copy link
Member

13rac1 commented May 4, 2016

@hburgund Definitely could be 12.04. Add

dist: trusty

to switch to the beta for 14.04: https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments

@IllyaMoskvin
Copy link
Member Author

@eosrei thanks for the link, Brad! I'm going to attempt some squashing + force pushing to get this all sorted out. Just give me a few hours, venturing into some unfamiliar territory here with Travis.

@jslootbeek
Copy link
Contributor

Switching to the 14.04 beta branch is not a bad idea moving forward, but I would be surprised if it worked cleanly out of the box. We'll likely need to fix some other things as well.
What version of setuptools are you using locally? You could specify that version to run in Travis. As it is we had to do some creative pip installing in the .travis.yml to cover for some dependency issues.

@IllyaMoskvin
Copy link
Member Author

@jslootbeek I'm using setuptools 20.10.1 locally. The tests produce some errors, which I'll need to address, but they do run. I'm looking into how to specify that version in Travis currently...

@jslootbeek
Copy link
Contributor

You can either specify it in requirements/dev.txt by adding setuptools==20.10.1 or on the line after pip install -r requirements/dev.txt add pip install setuptools==20.10.1

@IllyaMoskvin
Copy link
Member Author

Well, no luck so far. If someone has other ideas, please let me know. Still erroring out on setuptools. You can check the commit history for things I've tried.

ImportError: No module named extern

@IllyaMoskvin
Copy link
Member Author

IllyaMoskvin commented May 13, 2016

@hburgund is (1) awesome, and (2) managed to fix the Travis error! I'm getting the same failed tests on my vagrant instance. Hopefully this is just a case of deprecated methods. I'll look into this either tomorrow or on Monday.

@hburgund
Copy link
Member

@IllyaMoskvin what is the status on the error in the unit tests? Now that you are getting it locally as well as in Travis, it should be fairly easy to track down, I am hoping. I want to get this PR fully tested and integrated ASAP.

@IllyaMoskvin
Copy link
Member Author

I've discussed this on our Slack, but I'd like to post it here for reference, too. There are still three failed tests. All three test cases are related and trace back to the ProjectProtected classes and the project_restricted_queryset_through function in roundware.rw.admin. See TestProtectedAdmin in tests/roundware/test_admin.py for the tests. I've been picking away at the problem for a while, but I cannot figure out what is going wrong. Someone who has worked with these specific aspects of the code will have to fix this issue.

@IllyaMoskvin IllyaMoskvin force-pushed the upgrade/django-1-9 branch 3 times, most recently from ee2092b to 4cb5843 Compare June 6, 2016 17:25
@IllyaMoskvin
Copy link
Member Author

IllyaMoskvin commented Jun 9, 2016

Good news! Gray and I worked through the problem together. Apparently, queryset as "shorthand" for get_queryset must have been deprecated somewhere along the way. Changing queryset is just like setting a regular variable now; it has no effect on Django's processing of data. We changed it on a hunch, and it works! Let me rebase the commits + related branches, and this PR should be ready for merging.

@IllyaMoskvin
Copy link
Member Author

IllyaMoskvin commented Jul 14, 2016

Added explicit permission-based queryset filtering to ProjectAdmin. Going forward, we should keep in mind that using django-guardian's custom permissions seems to require us to define get_queryset explicitly, possibly everywhere such permissions are used, but for sure on the admin side.

See #291 for a subtle little problem caused by our default_auth.json fixtures. This resulted in errors similar to the following:

File "/var/www/roundware/source/roundware/rw/admin.py", line 99, in get_queryset
  return qset.filter(project__in=get_objects_for_user(request.user, 'rw.access_project'))
File "/usr/local/lib/python2.7/dist-packages/guardian/shortcuts.py", line 441, in get_objects_for_user
  permission__codename=codename)
File "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py", line 122, in manager_method
  return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 391, in get
  (self.model._meta.object_name, num)
MultipleObjectsReturned: get() returned more than one ContentType -- it returned 2!

Therefore, I am removing these fixtures and incorporating a table flush command into the upgrade instructions.

@hburgund
Copy link
Member

I have done an upgrade test on a fresh develop production install to this branch following the instructions in UPGRADING.md and generally it looks good. I do have a few questions:

  • can we set DEFAULT_SESSION_ID = “1” in roundware/settings/common.py to match current fixture which includes this session so that adding assets through the admin doesn't fail?
  • google maps in asset admin not working apparently due to some gmaps api key issue. But it is working on devapi server, so I'm not sure if you did something there with a key or not? We didn't used to have to supply a key of any sort, I don't think? https://developers.google.com/maps/documentation/javascript/get-api-key
  • why does deploy.sh uninstall and reinstall django every time even with same exact version? This is clearly inefficient. Maybe it has to do with a > version number specification rather than a specific version number somehow?

After we deal with these small issues, it's ready to merge as best I can tell.

@IllyaMoskvin
Copy link
Member Author

  • DEFAULT_SESSION_ID = “1” has been changed.
  • Good catch! Please see Resolve Google Maps API error: MissingKeyMapError #293. Important issue, but unrelated to the upgrade.
  • You were right, Django>=1.9 in django-admin-bootstrapped caused this. I removed the zip and forked that repo into IMAmuseum/django-admin-bootstrapped as a dependency. There, I can tweak the requirements to be more conservative.

I'll wait on rebasing the APIv2 and devapi branches until this one is merged.

@hburgund
Copy link
Member

hburgund commented Jul 26, 2016

Agreed that #293 isn't an upgrade issue, so doesn't need to hold up this merge.

Regarding django-admin-bootstrapped, I forked that repo into the roundware Github account:

https://github.com/roundware/django-admin-bootstrapped-1.9-compatible

This will be a better and more central spot to use than the IMAMuseum fork.

@IllyaMoskvin
Copy link
Member Author

Excellent. I updated (and renamed) roundware/django-admin-bootstrapped to match IMAmuseum/django-admin-bootstrapped and will delete the latter after this merge.

@IllyaMoskvin
Copy link
Member Author

Do we still need the fix-m2m-deserial.patch call in deploy.sh (Line 57)? I'm pretty sure it just fails without any apparent consequences nowadays.

# Apply patch to fix M2M field deserializing for Tag relationships, force command to return true.
# Details: https://code.djangoproject.com/ticket/17946
# TODO: Remove when fixed in Django core, probably when upgrading to Django 1.8.
patch -N $WWW_PATH/lib/python2.7/site-packages/django/core/serializers/python.py < $CODE_PATH/files/fix-m2m-deserial.patch || true
patching file /var/www/roundware/lib/python2.7/site-packages/django/core/serializers/python.py
Hunk #1 FAILED at 116.
1 out of 1 hunk FAILED -- saving rejects to file /var/www/roundware/lib/python2.7/site-packages/django/core/serializers/python.py.rej

@13rac1
Copy link
Member

13rac1 commented Jul 26, 2016

😄 That's why I linked the issue: https://code.djangoproject.com/ticket/17946 The issue is " closed Bug (fixed)", therefore the solution is in Django. The code and file should be removed.

This is a fairly extensive commit, since most of the requirements had
to be upgraded as well. Dependencies have been locked down to specific
versions for consistency and troubleshooting. Notable changes:

  - Moved wsgi.py to a more standard location, updated apache config.
    Now referenced in roundware.settings.WSGI_APPLICATION

  - For roundware.api2, implemented the new Application Configuration
    structure. See http://stackoverflow.com/q/32795227/1943591

  - django-guardian now creates its own AnonymousUser in the database.
    Removed AnonymousUser references from fixtures to avoid conflicts.
    http://django-guardian.readthedocs.io/en/stable/configuration.html

  - django-guardian assign is being depricated for assign_perm
    See django-guardian/django-guardian@1419048

  - ManyToManyField no longer accepts null=True

  - django.contrib.auth.models.User should not be accessed directly.
    Use django.conf.settings.AUTH_USER_MODEL instead

  - Django changed the way it loads models, which broke streaming.
    Moving django.setup() in roundwared.rwstreamd fixed the issue.

  - django_chartit is no longer maintained. Moved to django_chartit2
    This may require us to install jQuery and Highcharts manually

  - django.forms.models.save_instance has been removed.
    We are now calling save() instead, but it might need more testing.
    django/django@8656cf
    django/django@b11564

  - Fixed errors in Travis install script (thanks @hburgund)

  - Fixed jsocol/django-adminplus/issues/42

  - Fixed django-admin-bootstrapped dependency
    See IMAmuseum/django-admin-bootstrapped

  - Fixed queryset filtering in ProjectProtectedThrough classes in admin.py
    Added explicit permission-based queryset filtering to ProjectAdmin

  - Fixed duplicate rows in auth_permission, removed default_auth fixtures.

  - Removed fix-m2m-deserial.patch

This closes roundware#229, fixes roundware#291, and supersedes roundware#282.
@IllyaMoskvin
Copy link
Member Author

Google Images: ripping off a bandaid

Healing doesn't mean the damage never existed. It means the damage no longer controls our lives.

Please tell me this is ready for merging. 😭

@hburgund
Copy link
Member

yes, indeed, I believe we are merge-ready. Is it possible?!?!?

@hburgund hburgund merged commit 546e8d1 into roundware:develop Jul 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants