Fixed middleware with tests

Ross Mechanic · Ross Mechanic · commit 3c62c5679988 · 2018-04-05T10:11:31.000-04:00
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -5,6 +5,7 @@ Unreleased
 ----------
 - Dropped support for Django<=1.9 (gh-356)
 - Fix bug where history_view ignored user permissions (gh-361)
+– Fixed HistoryRequestMiddleware which hadn't been working for Django>1.9 (gh-364)
 
 1.9.1 (2018-03-30)
 ------------------
diff --git a/simple_history/middleware.py b/simple_history/middleware.py
@@ -1,20 +1,29 @@
-from django.utils.deprecation import MiddlewareMixin
-
 from .models import HistoricalRecords
 
 
-class HistoryRequestMiddleware(MiddlewareMixin):
+class HistoryRequestMiddleware:
     """Expose request to HistoricalRecords.
 
     This middleware sets request as a local thread variable, making it
     available to the model-level utilities to allow tracking of the
     authenticated user making a change.
     """
 
-    def process_request(self, request):
+    def __init__(self, get_response):
+        self.get_response = get_response
+
+    def __call__(self, request):
+        # Code to be executed for each request before
+        # the view (and later middleware) are called.
+
         HistoricalRecords.thread.request = request
 
-    def process_response(self, request, response):
+        response = self.get_response(request)
+
+        # Code to be executed for each request/response after
+        # the view is called.
+
         if hasattr(HistoricalRecords.thread, 'request'):
             del HistoricalRecords.thread.request
+
         return response
diff --git a/simple_history/tests/models.py b/simple_history/tests/models.py
@@ -2,6 +2,7 @@
 
 from django.apps import apps
 from django.db import models
+from django.urls import reverse
 
 from simple_history import register
 from simple_history.models import HistoricalRecords
@@ -17,6 +18,9 @@ class Poll(models.Model):
 
     history = HistoricalRecords()
 
+    def get_absolute_url(self):
+        return reverse('poll-detail', kwargs={'pk': self.pk})
+
 
 class PollWithExcludeFields(models.Model):
     question = models.CharField(max_length=200)
@@ -309,6 +313,7 @@ class ContactRegister(models.Model):
     name = models.CharField(max_length=30)
     email = models.EmailField(max_length=255, unique=True)
 
+
 register(ContactRegister, table_name='contacts_register_history')
 
 
diff --git a/simple_history/tests/tests/test_admin.py b/simple_history/tests/tests/test_admin.py
@@ -35,6 +35,12 @@ def get_history_url(obj, history_index=None, site="admin"):
             site=site, app=app, model=model), args=[quote(obj.pk)])
 
 
+overridden_settings = {
+    'MIDDLEWARE': settings.MIDDLEWARE +
+                  ['simple_history.middleware.HistoryRequestMiddleware'],
+}
+
+
 class AdminSiteTest(WebTest):
     def setUp(self):
         self.user = User.objects.create_superuser('user_login',
@@ -196,53 +202,37 @@ def test_history_user_not_saved(self):
             "No way to know of request, history_user should be unset.",
         )
 
+    @override_settings(**overridden_settings)
     def test_middleware_saves_user(self):
-        overridden_settings = {
-            'MIDDLEWARE':
-                settings.MIDDLEWARE +
-                ['simple_history.middleware.HistoryRequestMiddleware'],
-        }
-        with override_settings(**overridden_settings):
-            self.login()
-            form = self.app.get(reverse('admin:tests_book_add')).form
-            form["isbn"] = "9780147_513731"
-            form.submit()
-            book = Book.objects.get()
-            historical_book = book.history.all()[0]
-
-            self.assertEqual(historical_book.history_user, self.user,
-                             "Middleware should make the request available to "
-                             "retrieve history_user.")
+        self.login()
+        form = self.app.get(reverse('admin:tests_book_add')).form
+        form["isbn"] = "9780147_513731"
+        form.submit()
+        book = Book.objects.get()
+        historical_book = book.history.all()[0]
+
+        self.assertEqual(historical_book.history_user, self.user,
+                         "Middleware should make the request available to "
+                         "retrieve history_user.")
 
+    @override_settings(**overridden_settings)
     def test_middleware_unsets_request(self):
-        overridden_settings = {
-            'MIDDLEWARE':
-                settings.MIDDLEWARE +
-                ['simple_history.middleware.HistoryRequestMiddleware'],
-        }
-        with override_settings(**overridden_settings):
-            self.login()
-            self.app.get(reverse('admin:tests_book_add'))
-            self.assertFalse(hasattr(HistoricalRecords.thread, 'request'))
+        self.login()
+        self.app.get(reverse('admin:tests_book_add'))
+        self.assertFalse(hasattr(HistoricalRecords.thread, 'request'))
 
+    @override_settings(**overridden_settings)
     def test_rolled_back_user_does_not_lead_to_foreign_key_error(self):
         # This test simulates the rollback of a user after a request (which
         # happens, e.g. in test cases), and verifies that subsequently
         # creating a new entry does not fail with a foreign key error.
+        self.login()
+        self.assertEqual(
+            self.app.get(reverse('admin:tests_book_add')).status_code,
+            200,
+        )
 
-        overridden_settings = {
-            'MIDDLEWARE':
-                settings.MIDDLEWARE +
-                ['simple_history.middleware.HistoryRequestMiddleware'],
-        }
-        with override_settings(**overridden_settings):
-            self.login()
-            self.assertEqual(
-                self.app.get(reverse('admin:tests_book_add')).status_code,
-                200,
-            )
-
-            book = Book.objects.create(isbn="9780147_513731")
+        book = Book.objects.create(isbn="9780147_513731")
 
         historical_book = book.history.all()[0]
 
@@ -251,19 +241,14 @@ def test_rolled_back_user_does_not_lead_to_foreign_key_error(self):
             "No way to know of request, history_user should be unset.",
         )
 
+    @override_settings(**overridden_settings)
     def test_middleware_anonymous_user(self):
-        overridden_settings = {
-            'MIDDLEWARE':
-                settings.MIDDLEWARE +
-                ['simple_history.middleware.HistoryRequestMiddleware'],
-        }
-        with override_settings(**overridden_settings):
-            self.app.get(reverse('admin:index'))
-            poll = Poll.objects.create(question="why?", pub_date=today)
-            historical_poll = poll.history.all()[0]
-            self.assertEqual(historical_poll.history_user, None,
-                             "Middleware request user should be able to "
-                             "be anonymous.")
+        self.app.get(reverse('admin:index'))
+        poll = Poll.objects.create(question="why?", pub_date=today)
+        historical_poll = poll.history.all()[0]
+        self.assertEqual(historical_poll.history_user, None,
+                         "Middleware request user should be able to "
+                         "be anonymous.")
 
     def test_other_admin(self):
         """Test non-default admin instances.
diff --git a/simple_history/tests/tests/test_middleware.py b/simple_history/tests/tests/test_middleware.py
@@ -0,0 +1,173 @@
+from datetime import date
+
+from django.conf import settings
+from django.test import TestCase, override_settings
+from django.urls import reverse
+
+from simple_history.tests.custom_user.models import CustomUser
+from simple_history.tests.models import Poll
+
+overridden_settings = {
+    'MIDDLEWARE': settings.MIDDLEWARE +
+                  ['simple_history.middleware.HistoryRequestMiddleware'],
+}
+
+
+class MiddlewareTest(TestCase):
+    def setUp(self):
+        self.user = CustomUser.objects.create_superuser(
+            'user_login',
+            'u@example.com',
+            'pass'
+        )
+
+    @override_settings(**overridden_settings)
+    def test_user_is_set_on_create_view_when_logged_in(self):
+        self.client.force_login(self.user)
+        data = {
+            'question': 'Test question',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-add'), data=data)
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll_history = polls.first().history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [self.user.id])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_not_set_on_create_view_not_logged_in(self):
+        data = {
+            'question': 'Test question',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-add'), data=data)
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll_history = polls.first().history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [None])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_set_on_update_view_when_logged_in(self):
+        self.client.force_login(self.user)
+        poll = Poll.objects.create(
+            question='Test question',
+            pub_date=date.today()
+        )
+        data = {
+            'question': 'Test question updated',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-update', args=[poll.pk]), data=data)
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll = polls.first()
+        self.assertEqual(poll.question, 'Test question updated')
+
+        poll_history = poll.history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [self.user.id, None])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_not_set_on_update_view_when_not_logged_in(self):
+        poll = Poll.objects.create(
+            question='Test question',
+            pub_date=date.today()
+        )
+        data = {
+            'question': 'Test question updated',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-update', args=[poll.pk]), data=data)
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll = polls.first()
+        self.assertEqual(poll.question, 'Test question updated')
+
+        poll_history = poll.history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [None, None])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_unset_on_update_view_after_logging_out(self):
+        self.client.force_login(self.user)
+        poll = Poll.objects.create(
+            question='Test question',
+            pub_date=date.today()
+        )
+        data = {
+            'question': 'Test question updated',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-update', args=[poll.pk]), data=data)
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll = polls.first()
+        self.assertEqual(poll.question, 'Test question updated')
+
+        self.client.logout()
+
+        new_data = {
+            'question': 'Test question updated part 2',
+            'pub_date': '2010-01-01'
+        }
+        self.client.post(reverse('poll-update', args=[poll.pk]), data=new_data)
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 1)
+
+        poll = polls.first()
+        self.assertEqual(poll.question, 'Test question updated part 2')
+
+        poll_history = poll.history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [None, self.user.id, None])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_set_on_delete_view_when_logged_in(self):
+        self.client.force_login(self.user)
+        poll = Poll.objects.create(
+            question='Test question',
+            pub_date=date.today()
+        )
+
+        self.client.post(reverse('poll-delete', args=[poll.pk]))
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 0)
+
+        poll_history = poll.history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [self.user.id, None])
+
+    @override_settings(**overridden_settings)
+    def test_user_is_not_set_on_delete_view_when_not_logged_in(self):
+        poll = Poll.objects.create(
+            question='Test question',
+            pub_date=date.today()
+        )
+
+        self.client.post(reverse('poll-delete', args=[poll.pk]))
+
+        polls = Poll.objects.all()
+        self.assertEqual(polls.count(), 0)
+
+        poll_history = poll.history.all()
+
+        self.assertListEqual([ph.history_user_id for ph in poll_history],
+                             [None, None])
diff --git a/simple_history/tests/urls.py b/simple_history/tests/urls.py
@@ -3,11 +3,18 @@
 from django.conf.urls import url
 from django.contrib import admin
 
+from simple_history.tests.view import PollCreate, PollUpdate, PollDelete, PollDetail, PollList
 from . import other_admin
 
 admin.autodiscover()
 
 urlpatterns = [
     url(r'^admin/', admin.site.urls),
     url(r'^other-admin/', other_admin.site.urls),
+    url(r'^poll/add/$', PollCreate.as_view(), name='poll-add'),
+    url(r'^poll/(?P<pk>[0-9]+)/$', PollUpdate.as_view(), name='poll-update'),
+    url(r'^poll/(?P<pk>[0-9]+)/delete/$', PollDelete.as_view(),
+        name='poll-delete'),
+    url(r'^polls/(?P<pk>[0-9]+)/$', PollDetail.as_view(), name='poll-detail'),
+    url(r'^polls/$', PollList.as_view(), name='poll-list')
 ]
diff --git a/simple_history/tests/view.py b/simple_history/tests/view.py
