Added ObjectNotFoundMixin and unit test

letterpress/urls.py

@@ -20,7 +20,7 @@
                   url(r'^accounts/logout/$', logout_view, name='logout'),
                   url(r'^letters/(?P<letter_id>[0-9]+)/sentiment/(?P<sentiment_id>[0-9]+)/$',
                         LetterSentimentView.as_view(), name='letter_sentiment_view'),
-                  url(r'^letters/(?P<letter_id>[0-9]+)/$', LetterDetailView.as_view(), name='letter_detail'),
+                  url(r'^letters/(?P<pk>[0-9]+)/$', LetterDetailView.as_view(), name='letter_detail'),
                   url(r'^letters/', LettersView.as_view(), name='letters_view'),
                   url(r'^search_places/', search_places, name='search_places'),
                   url(r'^search/', SearchView.as_view(), name='search'),

letters/mixins.py

@@ -0,0 +1,22 @@
+from django.http import Http404
+from django.shortcuts import render
+from django.views.generic.detail import SingleObjectMixin
+
+
+class ObjectNotFoundMixin(SingleObjectMixin):
+
+    def dispatch(self, request, *args, **kwargs):
+        pk = self.kwargs.get(self.pk_url_kwarg)
+        try:
+            self.get_object()
+        except Http404:
+            object_type = self.model.__name__
+            return object_not_found(self.request, pk, object_type)
+
+        return super().dispatch(request, *args, **kwargs)
+
+
+def object_not_found(request, pk, object_type):
+    return render(request, 'obj_not_found.html',
+                  {'title': object_type + ' not found', 'object_id': pk, 'object_type': object_type})
+

letters/tests/test_mixins.py

@@ -0,0 +1,25 @@
+from django.test import RequestFactory, TestCase
+
+from letters.mixins import ObjectNotFoundMixin
+from letters.models import Letter
+
+
+class ObjectNotFoundMixinTestCase(TestCase):
+    """
+    ObjectNotFoundMixin.dispatch() should return rendered html giving details about the object that wasn't found
+    """
+
+    def test_dispatch(self):
+        request = RequestFactory()
+        mixin = ObjectNotFoundMixin()
+        mixin.model = Letter
+        mixin.queryset = Letter.objects.all()
+        mixin.request = request
+        mixin.kwargs = {'pk': 1}
+
+        response = mixin.dispatch(request)
+
+        self.assertTrue('<title>Letter not found</title>' in str(response.content),
+                        "ObjectNotFoundMixin response content should include '<object_type> not found'")
+        self.assertTrue('ID 1' in str(response.content),
+                        "ObjectNotFoundMixin) response content should include 'ID <object_id>' if it's for a letter")

letters/tests/test_views.py

@@ -15,8 +15,8 @@
 from letters.models import Correspondent, Letter
 from letters.tests.factories import LetterFactory, PlaceFactory
 from letters.views import export_csv, export_text, get_letter_export_text, GetStatsView, GetTextSentimentView, \
-    GetWordCloudView, highlight_for_sentiment, highlight_letter_for_sentiment, LettersView, object_not_found, \
-    RandomLetterView, SearchView, search_places, show_letter_content, get_highlighted_letter_sentiment
+    GetWordCloudView, highlight_for_sentiment, highlight_letter_for_sentiment, LettersView, RandomLetterView, \
+    SearchView, search_places, show_letter_content, get_highlighted_letter_sentiment
 
 
 class LettersViewTestCase(TestCase):
@@ -640,15 +640,15 @@ def test_letter_view(self):
         """
 
         # If Letter with letter_id not found, LetterView should return object_not_found()
-        response = self.client.get(reverse('letter_detail', kwargs={'letter_id': '1'}), follow=True)
+        response = self.client.get(reverse('letter_detail', kwargs={'pk': '1'}), follow=True)
 
         expected = {'title': 'Letter not found', 'object_id': '1', 'object_type': 'Letter'}
         for key in expected.keys():
             self.assertEqual(response.context[key], expected[key],
                 "LetterView context '{}' should be '{}', if letter not found".format(key, expected[key]))
 
         # If Letter with letter_id found, LetterView should return show_letter_content()
-        response = self.client.get(reverse('letter_detail', kwargs={'letter_id': LetterFactory().pk}), follow=True)
+        response = self.client.get(reverse('letter_detail', kwargs={'pk': LetterFactory().pk}), follow=True)
         self.assertTemplateUsed(response, 'letter.html')
 
         expected = {'title': 'Letter', 'nbar': 'letters_view'}
@@ -657,26 +657,6 @@ def test_letter_view(self):
                 "LetterView context '{}' should be '{}', if letter found".format(key, expected[key]))
 
 
-class ObjectNotFoundTestCase(SimpleTestCase):
-    """
-    Test object_not_found()
-    """
-
-    def test_object_not_found(self):
-        """
-        object_not_found() should return rendered html giving details about the object that wasn't found
-        """
-
-        request = RequestFactory()
-        response = object_not_found(request, object_id=1, object_type='Letter')
-        content = str(response.content)
-
-        self.assertTrue('<title>Letter not found</title>' in content,
-                        "object_not_found() response content should include '<object_type> not found'")
-        self.assertTrue('ID 1' in content,
-                        "object_not_found() response content should include 'ID <object_id>' if it's for a letter")
-
-
 class ShowLetterContentTestCase(TestCase):
     """
     Test show_letter_content()
@@ -785,52 +765,40 @@ class RandomLetterViewTestCase(TestCase):
     Test RandomLetterView
     """
 
-    @patch('letters.views.show_letter_content', autospec=True)
-    @patch('letters.views.object_not_found', autosec=True)
-    def test_random_letter_view(self, mock_object_not_found, mock_show_letter_content):
+    def test_random_letter_view(self):
         """
         RandomLetterView should retrieve a letter with a random index
         between 1 and total Letter objects.count() - 1
         """
 
-        request = RequestFactory().get(reverse('home'), follow=True)
-
-        # If only no Letters, RandomLetterView should return object_not_found()
-        RandomLetterView().dispatch(request)
+        # If only no Letters, RandomLetterView should return object not found page
+        response = self.client.get(reverse('random_letter'), follow=True)
 
-        args, kwargs = mock_object_not_found.call_args
-        self.assertEqual(args, (request, 0, 'Letter'),
-                         'If no Letters, RandomLetterView should return object_not_found()')
+        self.assertTemplateUsed(response, 'obj_not_found.html')
+        self.assertTrue('Letter not found' in str(response.context),
+                         'If no Letters, RandomLetterView should return object not found page')
 
         # If one letter, random_letter() should return that one
         letter = LetterFactory()
 
-        RandomLetterView().dispatch(request)
-
-        args, kwargs = mock_show_letter_content.call_args
-        self.assertEqual(args, (request, letter),
-                         'If one Letter, RandomLetterView should return show_letter_content() with certain args')
-        self.assertEqual(kwargs['title'], 'Random letter',
-                         "If one Letter, RandomLetterView should return show_letter_content() with title 'Random letter")
-        self.assertEqual(kwargs['nbar'], 'random_letter',
-                         "If one Letter, RandomLetterView should return show_letter_content() with title 'random_letter")
-        mock_show_letter_content.reset_mock()
+        response = self.client.get(reverse('random_letter'), follow=True)
+        self.assertIn('Random letter', str(response.content),
+                        "Random letter page should be shown if there's at least one letter in database")
+        self.assertIn(str(letter), str(response.content),
+                      'If one Letter, RandomLetterView should return that letter')
 
         # If more than one letter, RandomLetterView should return one of them
         letter2 = LetterFactory()
 
         with patch('random.randint', autospec=True) as mock_randint:
             mock_randint.return_value = 1
 
-            RandomLetterView().dispatch(request)
+            response = self.client.get(reverse('random_letter'), follow=True)
 
-            args, kwargs = mock_show_letter_content.call_args
-            self.assertEqual(args, (request, letter2),
-                'If more than one Letter, RandomLetterView should return show_letter_content() with certain args')
-            self.assertEqual(kwargs['title'], 'Random letter',
-                "If more than one Letter, RandomLetterView should return show_letter_content() with title 'Random letter")
-            self.assertEqual(kwargs['nbar'], 'random_letter',
-                "If more than one Letter, RandomLetterView should return show_letter_content() with title 'random_letter")
+            self.assertIn('Random letter', str(response.content),
+                          "If more than one Letter, RandomLetterView should return page title 'Random letter")
+            self.assertIn(str(letter2), str(response.content),
+                          "If more than one Letter, RandomLetterView should return randomly chosen letter")
 
 
 class PlacesViewTestCase(TestCase):

letters/views.py

@@ -15,16 +15,19 @@
 
 from django.contrib.auth import logout
 from django.http import HttpRequest, HttpResponse, HttpResponseRedirect
+from django.http import Http404
 from django.shortcuts import render
 from django.template.loader import render_to_string
 from django.utils.html import mark_safe
 from django.views.generic.base import TemplateView, View
+from django.views.generic.detail import DetailView
 from letter_sentiment.custom_sentiment import get_custom_sentiment_for_text, highlight_for_custom_sentiment
 from letter_sentiment.sentiment import get_sentiment, highlight_text_for_sentiment
 
 from letters import letter_search
 from letters import filter as letters_filter
 from letters.charts import make_charts
+from letters.mixins import ObjectNotFoundMixin, object_not_found
 from letters.models import Letter, Place
 from letters.sort_by import DATE, RELEVANCE, get_sentiments_for_sort_by_list
 
@@ -376,24 +379,25 @@ def post(self, request, *args, **kwargs):
             content_type="application/json")
 
 
-class LetterDetailView(View):
+class LetterDetailView(DetailView, ObjectNotFoundMixin):
     """
     Show one letter, by id
     """
 
-    def get(self, request, *args, **kwargs):
-        pk = self.kwargs.get('letter_id')
-        try:
-            letter = Letter.objects.get(pk=pk)
-        except Letter.DoesNotExist:
-            return object_not_found(self.request, pk, 'Letter')
+    model = Letter
+    template_name = 'letter.html'
 
-        return show_letter_content(request, letter, title='Letter', nbar='letters_view')
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+
+        letter = self.object
+        letter.body = mark_safe(letter.body)
+        context['title'] = 'Letter'
+        context['nbar'] = 'letters_view'
+        context['letter'] = letter
 
+        return context
 
-def object_not_found(request, object_id, object_type):
-    return render(request, 'obj_not_found.html',
-                  {'title': object_type + ' not found', 'object_id': object_id, 'object_type': object_type})
 
 
 # show particular letter
@@ -455,10 +459,34 @@ def get(self, request, *args, **kwargs):
         if count >= 1:
             random_idx = random.randint(0, count - 1)
             letter = Letter.objects.all()[random_idx]
-            return show_letter_content(request, letter, title='Random letter', nbar='random_letter')
+            letter.body = mark_safe(letter.body)
+
+            return render(request, 'letter.html',
+                          {'letter': letter, 'title': 'Random letter', 'nbar': 'random_letter'})
+
         return object_not_found(request, 0, 'Letter')
 
 
+class LetterDetailView(DetailView, ObjectNotFoundMixin):
+    """
+    Show one letter, by id
+    """
+
+    model = Letter
+    template_name = 'letter.html'
+
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+
+        letter = self.object
+        letter.body = mark_safe(letter.body)
+        context['title'] = 'Letter'
+        context['nbar'] = 'letters_view'
+        context['letter'] = letter
+
+        return context
+
+
 # Show map of places
 def places_view(request):
     assert isinstance(request, HttpRequest)

templates/place.html

@@ -24,9 +24,9 @@ <h6>Letters written here ({{ letters|length }})</h6>
                         <tbody>
                         {% for letter in letters %}
                             <tr>
-                                <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.list_date }}</a></td>
-                                <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.writer }}</a></td>
-                                <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.recipient }}</a></td>
+                                <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.list_date }}</a></td>
+                                <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.writer }}</a></td>
+                                <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.recipient }}</a></td>
                             </tr>
 
                         {% endfor %}

templates/snippets/search_list.html

@@ -10,10 +10,10 @@
     <tbody>
     {% for letter, highlight, sentiments, score in search_results %}
         <tr>
-            <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.list_date }}</a></td>
-            <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.writer }}</a></td>
-            <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.recipient }}</a></td>
-            <td><a href="{% url 'letter_detail' letter.id %}">{{ letter.place }}</a></td>
+            <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.list_date }}</a></td>
+            <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.writer }}</a></td>
+            <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.recipient }}</a></td>
+            <td><a href="{% url 'letter_detail' letter.pk %}">{{ letter.place }}</a></td>
         </tr>
         {% if sentiments %}
         <tr>