Changes Required for Compatibility with Django 2.0 #154

rodmanning · 2017-12-06T03:42:41Z

Changes as noted in #153 for compatibility with django v2.0. All tox tests I can run are passing (but py27-django-111 is refusing to run for me due to a dependancy problem as functools apparently doesn't like me today ...)

Still have to add in the migrations — let me know if you want them sequential or squashed.

Made the following changes to ensure compatibility with django 2.0: 1. Replaced function calls to `is_authenticated()` with reference to property `is_authenticated`. 2. Added try/except call to import `django.core.urlresolvers` (now called `django.urls`. Also added an `... as ...` statement to ensure that references in the code to `urlresolvers` don't need to be changed. 3. Fixed calls statement of `on_delete` arg to all ForeignKey fields. Note that previously a kwarg was acceptable, but this is now a positional arg, and the selected `on_delete` method has been retained. Signed-off-by: Rod Manning <[email protected]>

Updated tox.ini to also test django 2.0 on python 3+. Some changes made to previous commits required to ensure all tests passed: - Added `compat.py` to have a `is_authenticated()` function to check authentication. This was necessary as the property/method call for `is_authenticated` is no compatible between django 1.8 (LTS) and 2.0. Changed AuditLogMiddleware to call this compatibility function instead of the django built-ins as a result. - Changes made to `auditlog/models.py` to apply kwargs to both `to=` and `on_delete=` for consistency of handling in all version tested. Signed-off-by: Rod Manning <[email protected]>

Incorrect django version specified for tox.ini. Now fixed. Signed-off-by: Rod Manning <[email protected]>

codecov-io · 2017-12-06T04:11:44Z

Codecov Report

Merging #154 into master will decrease coverage by 0.32%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   69.15%   68.82%   -0.33%     
==========================================
  Files          18       19       +1     
  Lines         483      494      +11     
==========================================
+ Hits          334      340       +6     
- Misses        149      154       +5

Impacted Files	Coverage Δ
src/auditlog/migrations/0001_initial.py	`100% <ø> (ø)`	⬆️
src/auditlog/mixins.py	`0% <0%> (ø)`	⬆️
src/auditlog/middleware.py	`71.42% <100%> (+0.69%)`	⬆️
src/auditlog/models.py	`81.21% <100%> (ø)`	⬆️
src/auditlog/compat.py	`71.42% <71.42%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e33aef8...e56fe55. Read the comment docs.

Added and re-arranged 'on_delete' and 'to' kwargs in initial migration to ensure compatbility with later versions of Django. Also included updated manifest with changes required due to django 2.0 work. Signed-off-by: Rod Manning <[email protected]>

Added simple test case for compat.py file. Signed-off-by: Rod Manning <[email protected]>

audiolion · 2017-12-16T04:35:03Z

hey, been on vacation, looks like everything here is done in a sane way and how I would do it. Gonna ping @jjkester for a review

jjkester

Looks great, thanks! I spotted one minor think I would like to have fixed, plus there are some questions that don't necessarily require a change.

jjkester · 2017-12-20T15:49:00Z

src/auditlog/compat.py

+
+    Function provides compatibility following deprecation of method call to
+    is_authenticated() in Django 2.0.
+


Superfluous new line

At the end of the block? Fixed/removed.

jjkester · 2017-12-20T15:50:52Z

src/auditlog/compat.py

+
+    """
+
+    if django.VERSION < (1, 10):


This works perfectly, however, checking if user.is_authenticated is callable might work too.

No need to change this (obviously) unless you'd like to, kind of a personal preference I guess.

No problem to change it (tested change and all OK).

Thought behind the original version was to make it clear why the code existed — that way when you drop support for <1.10 in the future it's more obvious what code can be removed from compat.py.

personally id prefer the if django.VERSION < (1, 10) code for the reason stated, it lets you know when you can drop it in the compat, whereas looking at the new code I have no idea why it was added or what versions of django it makes compatible without added comments.

jjkester · 2017-12-20T15:51:50Z

src/auditlog/middleware.py

@@ -41,7 +42,7 @@ def process_request(self, request):
            threadlocal.auditlog['remote_addr'] = request.META.get('HTTP_X_FORWARDED_FOR').split(',')[0]

        # Connect signal for automatic logging
-        if hasattr(request, 'user') and hasattr(request.user, 'is_authenticated') and request.user.is_authenticated():
+        if hasattr(request, 'user') and hasattr(request.user, 'is_authenticated') and is_authenticated(request.user):


Maybe the attribute check can be combined with the compat function (and just return False if we don't know)? Just an idea, not sure if this would be practical.

As above, have changed as suggested and all tested OK.

Consideration was to avoid adding logic to compat.py so that it can eventually be removed when <1.10 isn't supported.

jjkester · 2017-12-20T15:52:45Z

src/auditlog/migrations/0001_initial.py

@@ -24,8 +24,8 @@ class Migration(migrations.Migration):
                ('action', models.PositiveSmallIntegerField(verbose_name='action', choices=[(0, 'create'), (1, 'update'), (2, 'delete')])),
                ('changes', models.TextField(verbose_name='change message', blank=True)),
                ('timestamp', models.DateTimeField(auto_now_add=True, verbose_name='timestamp')),
-                ('actor', models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.SET_NULL, verbose_name='actor', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
-                ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+                ('actor', models.ForeignKey(on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL, related_name='+', verbose_name='actor', blank=True, null=True)),


Where does this change come from? It kind of makes no sense to me.

jjkester · 2017-12-20T15:53:01Z

src/auditlog/migrations/0001_initial.py

-                ('actor', models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.SET_NULL, verbose_name='actor', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
-                ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+                ('actor', models.ForeignKey(on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL, related_name='+', verbose_name='actor', blank=True, null=True)),
+                ('content_type', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='contenttypes.ContentType', related_name='+', verbose_name='content type')),


Any reason this migration is edited?

content_type requires on_delete kwargs in django 2.0 (see Django 2.0 changelog)

File "/home/rod/src/git/django-auditlog.git/src/auditlog/migrations/0001_initial.py", line 30, in Migration ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')), TypeError: __init__() missing 1 required positional argument: 'on_delete' ⋮ ERROR: py34-django-20: commands failed ERROR: py35-django-20: commands failed ERROR: py36-django-20: commands failed

actor changes not intentional, so have reverted that line (think that might've been how it was auto generated by manage.py makemigrations, which I copied-pasted =)

Django 2.0 changelog suggests squashing migrations to manage this, and I'd support that approach.

Thanks for the explanation. I'm also for squashing migrations, however, I'm not sure about the implications of squashing for existing installs. Should be a separate issue anyways I guess.

Signed-off-by: Rod Manning <[email protected]>

jjkester · 2017-12-21T08:19:57Z

src/auditlog/compat.py

-    if django.VERSION < (1, 10):
+    if not hasattr(user, 'is_authenticated'):
+        return False
+    if hasattr(user.is_authenticated, "__call__"):


It would be better to use callable() for this check: https://bugs.python.org/issue10518#msg122584

Ok, no problem. Am away for a few days but will either see if I can edit it through github or do it when I’m back.

(You’re sure you want to check callable() instead of the version?)

Didn't hear back, so changed as suggested.

jjkester · 2017-12-21T08:21:53Z

src/auditlog/migrations/0001_initial.py

-                ('actor', models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.SET_NULL, verbose_name='actor', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
-                ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+                ('actor', models.ForeignKey(on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL, related_name='+', verbose_name='actor', blank=True, null=True)),
+                ('content_type', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='contenttypes.ContentType', related_name='+', verbose_name='content type')),


Thanks for the explanation. I'm also for squashing migrations, however, I'm not sure about the implications of squashing for existing installs. Should be a separate issue anyways I guess.

jjkester · 2017-12-21T08:23:06Z

src/auditlog/compat.py

@@ -6,10 +6,10 @@ def is_authenticated(user):

    Function provides compatibility following deprecation of method call to
    is_authenticated() in Django 2.0.


As per your comment, maybe add info here that this is only required for Django 1.10 and older.

Ok. Added detail to both comment and in-line in the code as well.

Note that this is only needed for 1.9 and earlier (django 1.10 accepts is_authenticated as a property (1.10 release notes)).

vshlapakov · 2017-12-25T13:40:08Z

src/auditlog/migrations/0001_initial.py

@@ -25,7 +25,8 @@ class Migration(migrations.Migration):
                ('changes', models.TextField(verbose_name='change message', blank=True)),
                ('timestamp', models.DateTimeField(auto_now_add=True, verbose_name='timestamp')),
                ('actor', models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.SET_NULL, verbose_name='actor', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
-                ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+                ('content_type', models.ForeignKey(on_delete=django.db.models.deletion.SET_NULL, related_name='+', verbose_name='content type', to='contenttypes.ContentType')),


@rodmanning There should be the same deletion mode (CASCADE, I guess) as in the model itself, otherwise it leads to an additional redundant migration.

Thanks! 👍 Didn't notice that — now fixed.

1. Added detailed commentary to `compat.py` to ensure reason why `is_authenticated()` compatibility function is needed 2. Changed `hasattr` to `callable` in compat.is_authenticated() 3. Fixed typo in migration 0001 to use correct `on_delete` function Signed-off-by: Rod Manning <[email protected]>

jjkester · 2017-12-28T09:36:47Z

src/auditlog/compat.py

+
+    This is *only* required to support Django < v1.10 (i.e. v1.9 and earlier),
+    as `is_authenticated` was introduced as a property in v1.10.
+


Another one of those newlines

jjkester · 2017-12-28T09:37:29Z

src/auditlog/migrations/0001_initial.py

@@ -25,7 +25,8 @@ class Migration(migrations.Migration):
                ('changes', models.TextField(verbose_name='change message', blank=True)),
                ('timestamp', models.DateTimeField(auto_now_add=True, verbose_name='timestamp')),
                ('actor', models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.SET_NULL, verbose_name='actor', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
-                ('content_type', models.ForeignKey(related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+                ('content_type', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='+', verbose_name='content type', to='contenttypes.ContentType')),
+


Superfluous newline

all review comments have been addressed

lamby · 2018-01-02T19:55:54Z

Thanks :)

robguttman · 2018-01-04T21:10:45Z

Thank you for this! When can we expect a 0.4.5 with these changes?

jjkester · 2018-01-08T12:35:29Z

I'll try to get it released this week.

robguttman · 2018-01-12T19:34:28Z

Ping - any chance that a 0.4.5 can still happen this week?

jjkester · 2018-01-12T22:43:40Z

Thanks for the ping. I had not forgotten about it. I'm just having a bit of a overwhelming week. I have just pushed 0.4.5 to PyPI, it should appear shortly on GitHub as well. Thanks for your patience!

robguttman · 2018-01-14T02:50:21Z

Thanks, Jan-Jelle! This allows us to upgrade to Django 2.0 fully now.

rodmanning added 3 commits December 6, 2017 08:15

Fix version error in tox test.

b74b441

Incorrect django version specified for tox.ini. Now fixed. Signed-off-by: Rod Manning <[email protected]>

rodmanning added 2 commits December 7, 2017 07:13

Add TestCase for compat.py

a620e17

Added simple test case for compat.py file. Signed-off-by: Rod Manning <[email protected]>

audiolion requested a review from jjkester December 16, 2017 04:35

jjkester requested changes Dec 20, 2017

View reviewed changes

Changes follow code review 2017-12-21

43e87c5

Signed-off-by: Rod Manning <[email protected]>

jjkester reviewed Dec 21, 2017

View reviewed changes

jjkester previously requested changes Dec 21, 2017

View reviewed changes

vshlapakov reviewed Dec 25, 2017

View reviewed changes

jjkester reviewed Dec 28, 2017

View reviewed changes

\n ...

e56fe55

audiolion approved these changes Jan 2, 2018

View reviewed changes

audiolion merged commit 4dee034 into jazzband:master Jan 2, 2018

This was referenced Jan 2, 2018

Using User.is_authenticated() as a callable is deprecated since Django 1.10 and removed in Django 2.x. #148

Closed

Add missing on_delete to LogEntry initial migration #146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes Required for Compatibility with Django 2.0 #154

Changes Required for Compatibility with Django 2.0 #154

rodmanning commented Dec 6, 2017

codecov-io commented Dec 6, 2017 •

edited

Loading

audiolion commented Dec 16, 2017

jjkester left a comment

jjkester Dec 20, 2017

rodmanning Dec 20, 2017

jjkester Dec 20, 2017

rodmanning Dec 20, 2017

audiolion Dec 21, 2017

jjkester Dec 20, 2017

rodmanning Dec 20, 2017

jjkester Dec 20, 2017

rodmanning Dec 20, 2017

jjkester Dec 20, 2017

rodmanning Dec 20, 2017

jjkester Dec 21, 2017

jjkester Dec 21, 2017

rodmanning Dec 22, 2017

rodmanning Dec 28, 2017

jjkester Dec 21, 2017

jjkester Dec 21, 2017

rodmanning Dec 28, 2017

vshlapakov Dec 25, 2017

rodmanning Dec 28, 2017

jjkester Dec 28, 2017

rodmanning Jan 1, 2018

jjkester Dec 28, 2017

rodmanning Jan 1, 2018

lamby commented Jan 2, 2018

robguttman commented Jan 4, 2018

jjkester commented Jan 8, 2018

robguttman commented Jan 12, 2018

jjkester commented Jan 12, 2018

robguttman commented Jan 14, 2018


		Function provides compatibility following deprecation of method call to
		is_authenticated() in Django 2.0.

		@@ -6,10 +6,10 @@ def is_authenticated(user):

		Function provides compatibility following deprecation of method call to
		is_authenticated() in Django 2.0.


		This is only required to support Django < v1.10 (i.e. v1.9 and earlier),
		as `is_authenticated` was introduced as a property in v1.10.

Changes Required for Compatibility with Django 2.0 #154

Changes Required for Compatibility with Django 2.0 #154

Conversation

rodmanning commented Dec 6, 2017

codecov-io commented Dec 6, 2017 • edited Loading

Codecov Report

audiolion commented Dec 16, 2017

jjkester 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

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

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

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

lamby commented Jan 2, 2018

robguttman commented Jan 4, 2018

jjkester commented Jan 8, 2018

robguttman commented Jan 12, 2018

jjkester commented Jan 12, 2018

robguttman commented Jan 14, 2018

codecov-io commented Dec 6, 2017 •

edited

Loading