diff mbox series

models: Remove 'latest_series'

Message ID 20180910204542.28715-1-stephen@that.guru
State Accepted
Headers show
Series models: Remove 'latest_series' | expand

Commit Message

Stephen Finucane Sept. 10, 2018, 8:45 p.m. UTC
This is only used in a single view (where it probably shouldn't be used)
and some tests. It's an anti-pattern that makes it too easy to shoot
yourself in the foot. Remove it.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
 patchwork/models.py                           | 23 ++----------------
 .../templates/patchwork/download_buttons.html | 10 ++++----
 patchwork/tests/test_series.py                | 24 +++++++++----------
 patchwork/views/utils.py                      |  2 +-
 4 files changed, 21 insertions(+), 38 deletions(-)

Comments

Stephen Finucane Sept. 10, 2018, 10:34 p.m. UTC | #1
On Mon, 2018-09-10 at 14:45 -0600, Stephen Finucane wrote:
> This is only used in a single view (where it probably shouldn't be used)
> and some tests. It's an anti-pattern that makes it too easy to shoot
> yourself in the foot. Remove it.
> 
> Signed-off-by: Stephen Finucane <stephen@that.guru>

This was blocking my rebase of the series-patch relationship series so
I've gone ahead and applied it. If anyone sees any issues, we can fix
them with a follow-up.

Stephen
diff mbox series

Patch

diff --git a/patchwork/models.py b/patchwork/models.py
index d2d8f343..8731bb2f 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -393,26 +393,7 @@  class Submission(FilenameMixin, EmailMixin, models.Model):
         ]
 
 
-class SeriesMixin(object):
-
-    @property
-    def latest_series(self):
-        """Get the latest series this is a member of.
-
-        Return the last series that (ordered by date) that this
-        submission is a member of.
-
-        .. warning::
-          Be judicious in your use of this. For example, do not use it
-          in list templates as doing so will result in a new query for
-          each item in the list.
-        """
-        # NOTE(stephenfin): We don't use 'latest()' here, as this can raise an
-        # exception if no series exist
-        return self.series.order_by('-date').first()
-
-
-class CoverLetter(SeriesMixin, Submission):
+class CoverLetter(Submission):
 
     def get_absolute_url(self):
         return reverse('cover-detail', kwargs={'cover_id': self.id})
@@ -422,7 +403,7 @@  class CoverLetter(SeriesMixin, Submission):
 
 
 @python_2_unicode_compatible
-class Patch(SeriesMixin, Submission):
+class Patch(Submission):
     # patch metadata
 
     diff = models.TextField(null=True, blank=True)
diff --git a/patchwork/templates/patchwork/download_buttons.html b/patchwork/templates/patchwork/download_buttons.html
index 4809db54..32acf26b 100644
--- a/patchwork/templates/patchwork/download_buttons.html
+++ b/patchwork/templates/patchwork/download_buttons.html
@@ -15,17 +15,19 @@ 
    class="btn btn-default" role="button" title="Download cover mbox"
    >mbox</a>
   {% endif %}
-  {% if submission.series.all|length == 1 %}
-  <a href="{% url 'series-mbox' series_id=submission.latest_series.id %}"
+  {% if all_series|length == 1 %}
+  {% with all_series|first as series %}
+  <a href="{% url 'series-mbox' series_id=series.id %}"
    class="btn btn-default" role="button"
    title="Download patch mbox with dependencies">series</a>
-  {% elif submission.series.all|length > 1 %}
+  {% endwith %}
+  {% elif all_series|length > 1 %}
   <button type="button" class="btn btn-default dropdown-toggle"
    data-toggle="dropdown">
    series <span class="caret"></span>
   </button>
   <ul class="dropdown-menu" role="menu">
-  {% for series in submission.series.all %}
+  {% for series in all_series %}
    <li><a href="{% url 'series-mbox' series_id=series.id %}"
     >{{ series }}</a></li>
   {% endfor %}
diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
index 9b5c0129..6892a615 100644
--- a/patchwork/tests/test_series.py
+++ b/patchwork/tests/test_series.py
@@ -87,7 +87,7 @@  class _BaseTestCase(TestCase):
 
             patches_ = patches[start_idx:end_idx]
             for patch in patches_:
-                self.assertEqual(patch.latest_series, series[idx])
+                self.assertEqual(patch.series.first(), series[idx])
 
             start_idx = end_idx
 
@@ -531,7 +531,7 @@  class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(patches, [1])
         self.assertSerialized(covers, [1])
 
-        series = patches[0].latest_series
+        series = patches[0].series.first()
         self.assertFalse(series.received_all)
 
     def test_complete(self):
@@ -551,7 +551,7 @@  class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(covers, [1])
         self.assertSerialized(patches, [2])
 
-        series = patches[0].latest_series
+        series = patches[0].series.first()
         self.assertTrue(series.received_all)
 
     def test_extra_patches(self):
@@ -572,7 +572,7 @@  class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(covers, [1])
         self.assertSerialized(patches, [3])
 
-        series = patches[0].latest_series
+        series = patches[0].series.first()
         self.assertTrue(series.received_all)
 
 
@@ -657,13 +657,13 @@  class SeriesNameTestCase(TestCase):
 
         cover = self._parse_mail(mbox[0])
         cover_name = self._format_name(cover)
-        self.assertEqual(cover.latest_series.name, cover_name)
+        self.assertEqual(cover.series.first().name, cover_name)
 
         self._parse_mail(mbox[1])
-        self.assertEqual(cover.latest_series.name, cover_name)
+        self.assertEqual(cover.series.first().name, cover_name)
 
         self._parse_mail(mbox[2])
-        self.assertEqual(cover.latest_series.name, cover_name)
+        self.assertEqual(cover.series.first().name, cover_name)
 
         mbox.close()
 
@@ -681,7 +681,7 @@  class SeriesNameTestCase(TestCase):
         mbox = self._get_mbox('base-no-cover-letter.mbox')
 
         patch = self._parse_mail(mbox[0])
-        series = patch.latest_series
+        series = patch.series.first()
         self.assertEqual(series.name, patch.name)
 
         self._parse_mail(mbox[1])
@@ -705,13 +705,13 @@  class SeriesNameTestCase(TestCase):
         mbox = self._get_mbox('base-out-of-order.mbox')
 
         patch = self._parse_mail(mbox[0])
-        self.assertIsNone(patch.latest_series.name)
+        self.assertIsNone(patch.series.first().name)
 
         patch = self._parse_mail(mbox[1])
-        self.assertEqual(patch.latest_series.name, patch.name)
+        self.assertEqual(patch.series.first().name, patch.name)
 
         cover = self._parse_mail(mbox[2])
-        self.assertEqual(cover.latest_series.name, self._format_name(cover))
+        self.assertEqual(cover.series.first().name, self._format_name(cover))
 
         mbox.close()
 
@@ -730,7 +730,7 @@  class SeriesNameTestCase(TestCase):
         """
         mbox = self._get_mbox('base-out-of-order.mbox')
 
-        series = self._parse_mail(mbox[0]).latest_series
+        series = self._parse_mail(mbox[0]).series.first()
         self.assertIsNone(series.name)
 
         series_name = 'My custom series name'
diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
index 2357ab86..3bf7af3c 100644
--- a/patchwork/views/utils.py
+++ b/patchwork/views/utils.py
@@ -143,7 +143,7 @@  def series_patch_to_mbox(patch, series_id):
         A string for the mbox file.
     """
     if series_id == '*':
-        series = patch.latest_series
+        series = patch.series.order_by('-date').first()
     else:
         try:
             series_id = int(series_id)