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

Prepare for Django 2.0 #320

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion simple_history/admin.py
Expand Up @@ -6,14 +6,17 @@
from django.contrib import admin
from django.contrib.admin import helpers
from django.contrib.contenttypes.models import ContentType
from django.core.urlresolvers import reverse
from django.shortcuts import get_object_or_404, render
from django.utils.text import capfirst
from django.utils.html import mark_safe
from django.utils.translation import ugettext as _
from django.utils.encoding import force_text
from django.conf import settings

try:
from django.urls import reverse
except ImportError: # Django < 1.10
from django.core.urlresolvers import reverse
try:
from django.contrib.admin.utils import unquote
except ImportError: # Django < 1.7
Expand Down
12 changes: 8 additions & 4 deletions simple_history/models.py
Expand Up @@ -179,7 +179,8 @@ def copy_fields(self, model):
if isinstance(old_field.rel.to, str) and old_field.rel.to == 'self':
object_to = old_field.model
else:
object_to = old_field.rel.to
# required for Django <= 1.8 # required for Django >= 2.0

Choose a reason for hiding this comment

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

minor: doubled up comment

object_to = old_field.rel.to if hasattr(old_field, 'rel') else old_field.remote_field.model
Copy link

Choose a reason for hiding this comment

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

As said before, this is the wrong order to avoid warnings.

Line 179 seems to have the same problem.

Choose a reason for hiding this comment

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

So, you want to see
old_field.remote_field.model if hasattr(old_field, 'remote_field') else old_field.rel.to
?

Choose a reason for hiding this comment

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

Did that edit plus helped out @flesser 's merge to guard new uses of old_field.rel.to here https://github.com/flesser/django-simple-history/pull/4 .


field = FieldType(
object_to,
Expand Down Expand Up @@ -284,11 +285,14 @@ def get_history_user(self, instance):
return instance._history_user
except AttributeError:
try:
if self.thread.request.user.is_authenticated():
return self.thread.request.user
return None
is_authenticated = self.thread.request.user.is_authenticated
except AttributeError:
return None
if not is_authenticated in (True, False):
is_authenticated = is_authenticated() # Django < 1.10
Copy link

Choose a reason for hiding this comment

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

I think this is not correct. In 1.10 or 1.11, is_authenticated is an instance of CallableTrue or CallableFalse, not True or False, so the if not in test will fail and go into line 283 which will raise the deprecation warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that might be. I tested it having the callable boolean in mind but now I saw that none of the implemented tests get to this line at all in Django 1.10+ (because self.thread.request.user.is_authenticated raises the caught AttributeError.

request.user raises an AttributeError in multiple tests in Django 2.0 (AttributeError: 'WSGIRequest' object has no attribute 'user'). For that reason it might be the next step to fix those.

Copy link

Choose a reason for hiding this comment

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

This error might be caused by a wrong ordering of middleware.

if is_authenticated:
return self.thread.request.user
return None


def transform_field(field):
Expand Down
2 changes: 1 addition & 1 deletion simple_history/registry_tests/migration_test_app/models.py
Expand Up @@ -11,5 +11,5 @@ class WhatIMean(DoYouKnow):


class Yar(models.Model):
what = models.ForeignKey(WhatIMean)
what = models.ForeignKey(WhatIMean, on_delete=models.CASCADE)
history = HistoricalRecords()
6 changes: 5 additions & 1 deletion simple_history/tests/models.py
Expand Up @@ -287,7 +287,11 @@ class UserAccessorOverride(models.Model):


class Employee(models.Model):
manager = models.OneToOneField('Employee', null=True)
manager = models.OneToOneField(
'Employee',
on_delete=models.CASCADE,
null=True,
)
history = HistoricalRecords()


Expand Down
5 changes: 4 additions & 1 deletion simple_history/tests/tests/test_admin.py
Expand Up @@ -6,7 +6,10 @@
from django.contrib.messages.storage.fallback import FallbackStorage
from django.test.utils import override_settings
from django.test.client import RequestFactory
from django.core.urlresolvers import reverse
try:
from django.urls import reverse
except ImportError: # Django <1.10
from django.core.urlresolvers import reverse
from django.conf import settings
from django.contrib.auth import get_user_model
from django.utils.encoding import force_text
Expand Down
6 changes: 3 additions & 3 deletions simple_history/tests/urls.py
@@ -1,12 +1,12 @@
from __future__ import unicode_literals

from django.conf.urls import include, url
from django.conf.urls import url
from django.contrib import admin
from . import other_admin

admin.autodiscover()

urlpatterns = [
url(r'^admin/', include(admin.site.urls)),
url(r'^other-admin/', include(other_admin.site.urls)),
url(r'^admin/', admin.site.urls),
url(r'^other-admin/', other_admin.site.urls),

Choose a reason for hiding this comment

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

Is the omission of function include backwards compatible? All other edits seem backwards compatibility except maybe this one.

Copy link
Contributor

@dgilge dgilge Dec 20, 2017

Choose a reason for hiding this comment

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

The tests passed using tox.

Choose a reason for hiding this comment

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

Perfect.

]