Django 1.8 migration in 29 commits #15
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the hard work Vladimir! Please allow us a few days to process this. |
|
Hi Zenon, Just wondering whether you've had a chance to reviews this.... Cheers, PS: And, btw, I've just pushed in one more commit with a minor change - bumping python-social-auth from 0.2.10 up to 0.2.14 - just to get bugfixes. |
|
Hi Zenon, FYI, I have also developed the feature to make the list of login methods configurable (as discussed in #9). I have it so far in the develop branch at REANNZ/djnro - and I've used the django18 branch as the starting point. Once you merge the django18 branch, I'd create a pull request for this feature. In the meantime, you can have a look at the login methods feature at REANNZ/djnro@django18...REANNZ:develop Any feedback (either on the login methods feature or the Django 1.8 migration) would be greatly appreciated. Cheers, |
|
Hi Vladimir, we have a first round of comments that I will be posting shortly... It's a lot of files though and we've been looking at individual commits so I'm just figuring out how to move comments to PR diff context. |
|
Hi Zenon, I hope I answered all of your questions - would you be happy to merge this now? Cheers, |
|
Hi again Vladimir, beyond this first set of minor issues that you have already mostly answered, we want to do some more tests for the migration/upgrade process to Django 1.8, so as to be able to better understand what you have documented in REANNZ/djnro@8c8842e. We will thus need a few more days for that. Regards, |
vladimir-mencl-eresearch
force-pushed
the
django18
branch
from
c1885af to
ff98cc0
Compare
|
Hi Zenon, I have updated commit messages for:
I understand you still need to review the migration instructions. I have successfully tested them on my own DEV systems. Btw, I also have a script that does that in an automated way (as part of our Dockerized deployment) - I'd be happy to contribute that script too if you'd find it useful: Please let me know if you have any further questions - and I look forward to hearing from you once you are able to merge this PR. Cheers, |
|
Hi Zenon, FYI, I have pushed in a minor change to the upgrade step: keeping the door open to add new migrations to the edumanage models without having instructions in place that would tell the user to ignore (fake) any such new migrations. The Cheers, |
|
Vladimir, I have a couple more comments to add for the migration instructions. Stay tuned! |
|
Thanks, keen to hear from you. Cheers, |
|
With regard to the migration script, which could be considered complementary to the upgrade/migration instructions. First of all I think it can be quite useful, and it looks to me that it's not actually tied to use with Docker. However it's obviously not generic enough (e.g. blindly adding the PostgreSQL constraint). I don't think we have any particular experience with tools for managed installations/upgrades/etc. specific to Django projects, yet in our ops environment folks seem to be fond of Fabric. I can see there is a comparison matrix of tools for such a job. I think we could get a better perspective how one may handle such tasks and perhaps revisit this topic (and maybe update/simplify instructions) after merging this PR. |
|
Vladimir, I can now say that we are done with all our comments and are ready to merge this PR, mainly pending your last answers/final touches for the upgrade instructions. We still want to do more testing with real usage, but we are confident we can do this after merging and report+fix any issues that may arise as necessary. Yay! Thanks for bearing with us 👍 |
|
Hi Zenon, Thanks for the patience too! I've just finished replying to your latest comments - I look forward to hearing from you back. Cheers, |
|
Hi Zenon, Just wondering there there are any pending issues to resolve... Cheers, |
| except: | ||
| pass | ||
|
|
||
| admin.site.register(User, UserAdmin) |
There was a problem hiding this comment.
While trying something irrelevant, namely checking if django-registration works with Django 1.7 (not that we expected djnro to work as is in such a setup), we noticed ./manage.py shell borks apparently due to this line with an 'AppRegistryNotReady' exception:
Traceback (most recent call last):
File "./manage.py", line 10, in <module>
execute_from_command_line(sys.argv)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
utility.execute()
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 354, in execute
django.setup()
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/__init__.py", line 21, in setup
apps.populate(settings.INSTALLED_APPS)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/apps/registry.py", line 108, in populate
app_config.import_models(all_models)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/apps/config.py", line 202, in import_models
self.models_module = import_module(models_module_name)
File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
__import__(name)
File "/srv/djnro/accounts/models.py", line 27, in <module>
admin.site.register(User, UserAdmin)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/sites.py", line 101, in register
system_check_errors.extend(admin_class.check(model))
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/options.py", line 153, in check
return cls.checks_class().check(cls, model, **kwargs)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/checks.py", line 494, in check
errors.extend(self._check_list_filter(cls, model))
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/checks.py", line 665, in _check_list_filter
for index, item in enumerate(cls.list_filter)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/checks.py", line 710, in _check_list_filter_item
get_fields_from_path(model, field)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/contrib/admin/utils.py", line 457, in get_fields_from_path
fields.append(parent._meta.get_field_by_name(piece)[0])
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/db/models/options.py", line 416, in get_field_by_name
cache = self.init_name_map()
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/db/models/options.py", line 445, in init_name_map
for f, model in self.get_all_related_m2m_objects_with_model():
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/db/models/options.py", line 563, in get_all_related_m2m_objects_with_model
cache = self._fill_related_many_to_many_cache()
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/db/models/options.py", line 577, in _fill_related_many_to_many_cache
for klass in self.apps.get_models():
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/utils/lru_cache.py", line 101, in wrapper
result = user_function(*args, **kwds)
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/apps/registry.py", line 168, in get_models
self.check_models_ready()
File "/tmp/virtualenv/djnro17/local/lib/python2.7/site-packages/django/apps/registry.py", line 131, in check_models_ready
raise AppRegistryNotReady("Models aren't loaded yet.")We think it's a good idea anyway to move this to accounts/admin.py, with such a patch:
diff --git a/accounts/admin.py b/accounts/admin.py
index ee85a48..79e7d35 100644
--- a/accounts/admin.py
+++ b/accounts/admin.py
@@ -1,8 +1,10 @@
from django.contrib import admin
-from accounts.models import UserProfile
+from accounts.models import User, UserProfile
+from django.contrib.auth.admin import UserAdmin
class UserPrAdmin(admin.ModelAdmin):
list_display = ('user', 'institution')
admin.site.register(UserProfile, UserPrAdmin)
+admin.site.register(User, UserAdmin)
diff --git a/accounts/models.py b/accounts/models.py
index ff03608..510e93b 100644
--- a/accounts/models.py
+++ b/accounts/models.py
@@ -24,8 +24,6 @@ try:
except:
pass
-admin.site.register(User, UserAdmin)
-
class UserProfile(models.Model):
user = models.OneToOneField(settings.AUTH_USER_MODEL)
institution = models.ForeignKey(Institution)Do you agree? If yes, would you mind adding this patch to the PR, or perhaps I can also add this during the merge?
There was a problem hiding this comment.
Hi Zenon,
Good point! I've checked and the Django auth.User model also does the registration from admin.py.
Done, pushed in in 3e00840.
We should be good to go now!
Cheers,
Vlad
|
Hey Vladimir, see my last line note and then we are good to go :) Sorry for not replying earlier, I've gone busy again. |
vladimir-mencl-eresearch
added a commit
to REANNZ/djnro
that referenced
this pull request
Mar 16, 2016
Move the custom user model accounts.User registration with the admin class (django.contrib.auth.admin.UserAdmin) from accounts/models.py into accounts/admin.py. This is done as per discussion in grnet#15 (line note on accounts/models.py).
|
Thanks Vladimir. I will proceed with the merge. I am tempted to squash some (of the 42) commits to clean things up a little. Authorship will be preserved of course. Would you mind terribly? |
|
Hi Zenon, I'll try some squashing. The commits around Django version are good candidates - and so are about the swappable properties. |
|
I was gonna do it myself, but... great, thanks! |
|
Do you have any other candidates for squashing? |
|
Hi Zenon, So, this is my take on Will soon run this and see if all of it can be easily merged... Cheers, |
Use version comparisions to select latest 1.8.x (with current 1.8.11 being the minimum):
Django>=1.8.11,<1.9
(After careful consideration, deciding not to use PIP PEP 440 compatible
notation to stay compatible with PIP 1.5.6 coming with Debian Jessie).
|
Alright, but I think that for some sequences of squashed commits, you want to keep the first one. More specifically: |
... else, loading settings breaks in a call to non-lazy gettext
Remove deprecated django.contrib.markup (removed in Django 1.6). While this package can be replaced with markdown_deux (as e.g. suggested at django-helpdesk/django-helpdesk#219), we can remove it as it is not used at all in the DjNRO code base.
Remove duplicate listing of django.contrib.staticfiles in settings.INSTALLED_APPS
Django 1.5 requires all URL names to be quoted (single quotes). Update all templates accordingly (HTML and also .txt templates for emails).
User profiles have been deprecated in Django 1.5 and removed in Django 1.7 It is now no longer possible to tell Django directly about a user profile model. However, our model already exists in accounts/models.py: UserProfile and is linked into the model as "userprofile". Replace "get_profile()" calls with a "userprofile" reference. (Note this involves removing the (), otherwise the code would attempt to call the UserProfile object itself - which is not callable). Remove the AUTH_PROFILE_MODULE setting, as it no longer has any effect.
In Django 1.5, the django.views.generic.simple module and the direct_to_template function have been removed (already deprecated in Django 1.4) Replace the direct_to_template call with the class-based approach, calling TemplateView.as_view() - and passing the template_name as argument, avoiding the need to create a sub-class.
Django 1.8 has deprecated django.contrib.contenttypes.generic - and this module will be removed in Django 1.9. To safeguard for the future (and remove an ugly deprecation warning), fix these now. The classes have been moved into the fields, forms and admin modules. Make appropriate changes: * edumanage.models only uses classes that are now in the fields module, so replace generic with fields. * edumanage.admin only uses classes that are now in the admin module, so import admin instead - but as django.contrib.admin is already imported as admin, import django.contrib.contenttypes admin as contenttype_admin. * edumanage.views and edumanage.forms use functions from the forms module - also change the import accordingly.
Django 1.7+ use built-in migration instead of South. * Remove South from INSTALLED_APPS in settings.py * Remove South from requirements.txt * Remove south import and call to add_introspection_rules from edumanage/models.py Note that in Django migrations, the replacement code would go into a deconstruct() method, but as the init() arguments are not changed, we can skip adding the deonstruct() method - for further information, please see https://docs.djangoproject.com/en/1.8/howto/custom-model-fields/#custom-field-deconstruct-method
Remove unused field options in edumanage/models.py (max_length on PositiveInteger fields and null=True on a ManyToManyField) to avoid warnings: about unused filed options. Note that while this changes the model, the resulting database is the same, so no migration is added. (Next commit will sort out the change to Django migrations)
The longerusername package is not really compatible with Django 1.7+ - it only hooks into South migrations, and Django 1.7+ does not allow the "monkeypatch" approach that longerusername uses to modify the Auth.User model. Instead, do the proper thing and define a custom user model as per https://docs.djangoproject.com/en/1.8/topics/auth/customizing/#specifying-a-custom-user-model To make the transition easy: * Create a model that inherits from Auth.AbstractUser, only modifying the username length * Make the model use the auth_user DB table so it picks up existing data * Call the model User so that links from other existing tables still work For existing Django 1.4 databases, the model would just inherit the existing table structure. For new Django 1.8 databases, the generated initial migrations would create the tables the same way. Make the following considerations in designing and implementing the model: * Register the model (accounts.User) with the admin class (django.contrib.auth.admin.UserAdmin) in accounts/admin.py, not accounts/models.py (follow conventions in django.contrib.auth and resolve compatibility issues raised in grnet#15). * Mark the model as swappable. * Make sure the migration also keeps the swappable property. * Update imports across the DjNRO codebase to reflect that User is now defined in accounts.models, not django.contrib.auth.models - and if possible, in model definition, refer to the (swappable) user model through settings.AUTH_USER_MODEL. Swappable models: The accounts.User Meta class has the same property "swappable" to as is defined in auth.User: class User(AbstractUser): ... class Meta(AbstractUser.Meta): swappable = 'AUTH_USER_MODEL' The value of the property is the name of a setting from the Django project's settings that defines which model class implements a particular feature (the user model in this case). With this feature on, only the model class where the name of the class matches the value of the setting referenced by the property is active. Without adding this property, and when changing settings.AUTH_USER_MODEL back to auth.User (either explicitly or by removing the setting and letting it default to this value), the accounts.User model class would clash with auth.User - they'd be both adding foreign key relations with the same name to related classes.
Move legacy South migrations from app_name/migrations to app_name/south_migrations.
Generated with:
./manage.py makemigrations --name initial
(__init__.py created manually)
Due to the structure of the accounts tables, there is more than one initial migration.
Note: this also includes the migrations generated for accounts.User, the custom
user model replacing django.contrib.auth.User.
Note that accounts.User is also a swappable model (same as django.contrib.auth.User).
And so it is important to also include the swappable property in the generated
migrations. (Without the property also being set in the generated migrations,
the migrations framework does not consider the class swapped out and attempts
to run the migrations for this class even when e.g. auth.User is used, clashing
with the migrations for auth.User and breaking the database initialization).
Port django.contrib.auth migrations affecting the user Model over to our Custom
User Model accounts.user as
0003_alter_user_email_max_length and
0004_alter_user_last_login_null.
Manually modify 0001_initial to pretend these changes haven't been done yet to
make the migrations framework actually apply these migrations.
Detailed explanation and reasoning:
In Django 1.7, South was dropped to use Django migrations instead.
Any DB history prior to that gets squashed into a new initial migration in the
new system.
In Django 1.8, with the new Django migrations, auth.User evolved to extend the
email address (from 75 to 254) and also adds blank=True to last_login.
For auth.User, these are captured in stand-alone migrations.
When replacing auth.User with account.User, it might be tempting to create just
one initial migration with the final version of the model.
However, this would not work for existing databases. Because we are at the same
time switching from South to Django migrations, we need to fake history on what
was done. Because the tables already exist, we need to run
./manage.py migrate --fake-initial
to fake the initial migration - and in some cases, also fake subsequent
migrations. E.g., because the edumanage package initial state is spread over
multiple migration steps, we do it with full:
./manage.py migrate --fake-initial --fake edumanage
and likewise for account:
./manage.py migrate --fake-initial --fake accounts 0002_initial
But, here we explicitly state that accounts should have the migration history
faked only up to 0002_initial.
At that point, we get the original tables created with Django 1.4 and with
older versions of email and last_login - and this is when we actually apply the
additional two migrations included in this commit:
./manage.py migrate --fake-initial
which lets migrate go wild and apply any unapplied migrations (but still faking initial ones).
The contacts.py command was printing unicode, and after Django 1.8 upgrade, started failing with:
UnicodeDecodeError: 'ascii' codec can't decode byte 0xce in position 1: ordinal not in range(128)
Django versions prior to 1.8 were enforcing the "en-us" locale which was creating the need to wrap stdout with another layer of unicode encoding.
Django 1.8 now just switches off translations - but honours the leave_locale_alone option.
* Set this option and rely on Django default handling of unicode.
* Also remove the no longer needed wrapped sys.stdout
* Add encoding header to indicate the source code is in utf-8
* Normalize encoding name from UTF8 to UTF-8
More information at
https://docs.djangoproject.com/en/1.8/howto/custom-management-commands/#management-commands-and-locales
... so set to a wildcard and add instructions to customize with public hostname
(and put it in uncommented)
In Django 1.8 logging, filter out DisallowedHost SuspiciousOperation notification emails (as these, triggered by spiderbots, would flood the admin mailboxes). Credits: http://stackoverflow.com/questions/15384250/suppress-admin-email-on-django-allowed-hosts-exception
|
The intention was to I'm almost done with the squashing - will push soon. |
vladimir-mencl-eresearch
force-pushed
the
django18
branch
from
3e00840 to
6b3a911
Compare
|
Squashing down to 26 commits done - please let me know whether you're happy with the outcome... Cheers, |
|
For reference, original unsquashed (and un-reordered) branch is archived at https://github.com/REANNZ/djnro/compare/django18-backup-before-squash-2016-03-17 |
|
Great work, thanks a lot, merging at last! |
zmousm
added a commit
that referenced
this pull request
Mar 17, 2016
Django 1.8 migration in 26 commits
zmousm
pushed a commit
to zmousm/djnro
that referenced
this pull request
Sep 5, 2016
The longerusername package is not really compatible with Django 1.7+ - it only hooks into South migrations, and Django 1.7+ does not allow the "monkeypatch" approach that longerusername uses to modify the Auth.User model. Instead, do the proper thing and define a custom user model as per https://docs.djangoproject.com/en/1.8/topics/auth/customizing/#specifying-a-custom-user-model To make the transition easy: * Create a model that inherits from Auth.AbstractUser, only modifying the username length * Make the model use the auth_user DB table so it picks up existing data * Call the model User so that links from other existing tables still work For existing Django 1.4 databases, the model would just inherit the existing table structure. For new Django 1.8 databases, the generated initial migrations would create the tables the same way. Make the following considerations in designing and implementing the model: * Register the model (accounts.User) with the admin class (django.contrib.auth.admin.UserAdmin) in accounts/admin.py, not accounts/models.py (follow conventions in django.contrib.auth and resolve compatibility issues raised in grnet#15). * Mark the model as swappable. * Make sure the migration also keeps the swappable property. * Update imports across the DjNRO codebase to reflect that User is now defined in accounts.models, not django.contrib.auth.models - and if possible, in model definition, refer to the (swappable) user model through settings.AUTH_USER_MODEL. Swappable models: The accounts.User Meta class has the same property "swappable" to as is defined in auth.User: class User(AbstractUser): ... class Meta(AbstractUser.Meta): swappable = 'AUTH_USER_MODEL' The value of the property is the name of a setting from the Django project's settings that defines which model class implements a particular feature (the user model in this case). With this feature on, only the model class where the name of the class matches the value of the setting referenced by the property is active. Without adding this property, and when changing settings.AUTH_USER_MODEL back to auth.User (either explicitly or by removing the setting and letting it default to this value), the accounts.User model class would clash with auth.User - they'd be both adding foreign key relations with the same name to related classes.
zmousm
added a commit
to zmousm/djnro
that referenced
this pull request
Sep 5, 2016
Django 1.8 migration in 26 commits
zmousm
pushed a commit
to zmousm/djnro
that referenced
this pull request
Sep 5, 2016
The longerusername package is not really compatible with Django 1.7+ - it only hooks into South migrations, and Django 1.7+ does not allow the "monkeypatch" approach that longerusername uses to modify the Auth.User model. Instead, do the proper thing and define a custom user model as per https://docs.djangoproject.com/en/1.8/topics/auth/customizing/#specifying-a-custom-user-model To make the transition easy: * Create a model that inherits from Auth.AbstractUser, only modifying the username length * Make the model use the auth_user DB table so it picks up existing data * Call the model User so that links from other existing tables still work For existing Django 1.4 databases, the model would just inherit the existing table structure. For new Django 1.8 databases, the generated initial migrations would create the tables the same way. Make the following considerations in designing and implementing the model: * Register the model (accounts.User) with the admin class (django.contrib.auth.admin.UserAdmin) in accounts/admin.py, not accounts/models.py (follow conventions in django.contrib.auth and resolve compatibility issues raised in grnet#15). * Mark the model as swappable. * Make sure the migration also keeps the swappable property. * Update imports across the DjNRO codebase to reflect that User is now defined in accounts.models, not django.contrib.auth.models - and if possible, in model definition, refer to the (swappable) user model through settings.AUTH_USER_MODEL. Swappable models: The accounts.User Meta class has the same property "swappable" to as is defined in auth.User: class User(AbstractUser): ... class Meta(AbstractUser.Meta): swappable = 'AUTH_USER_MODEL' The value of the property is the name of a setting from the Django project's settings that defines which model class implements a particular feature (the user model in this case). With this feature on, only the model class where the name of the class matches the value of the setting referenced by the property is active. Without adding this property, and when changing settings.AUTH_USER_MODEL back to auth.User (either explicitly or by removing the setting and letting it default to this value), the accounts.User model class would clash with auth.User - they'd be both adding foreign key relations with the same name to related classes.
zmousm
added a commit
to zmousm/djnro
that referenced
this pull request
Sep 5, 2016
Django 1.8 migration in 26 commits
vladimir-mencl-eresearch
added a commit
to REANNZ/djnro
that referenced
this pull request
Oct 10, 2022
While the filter introduced in d658e30 in grnet#15 blocks DisallowedHost events from generating an admin notification email (as these are likely to happen on a public Internet site), there has been an increasing number of emails triggered from POST requests where the `Referer` header also fails the ALLOWED_HOSTS check. These events are sent as a generic `django.request` events with `DisallowedHost` listed as the causing exception. Catch this case and block logging for these events as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Zenon,
This is the Django 1.8 migration - I believe it is complete.
I have run a fairly detailed test coverage (all URLs, aiming for all different actions, also invoking all management commands) and as far as I can tell, everything is working.
I have added documentation for migration. It will need users to first push forward to the latest 1.4 release (including the InstServer M2M change), but after that, the instructions are straightforward. (We will need to tag the tip of the next branch and make it a stable reference point prior to this merge).
Please let me know if you're happy to merge this (or any concerns you may have).
Cheers,
Vlad