[bug 674835] Make dash readouts consider all approved revs between a …

…translation and the latest ready-for-l10n rev when computing max significance.

Also, factor up some repetition of convenience methods in test_readouts.

apps/dashboards/readouts.py

@@ -264,15 +264,16 @@ def _most_visited_query_and_params(self, max, extra_where=''):
         return (
             'SELECT engdoc.slug, engdoc.title, transdoc.slug, '
             'transdoc.title, dashboards_wikidocumentvisits.visits, '
-            # The most significant ready-for-localization, approved change to
-            # the English article since the English revision the current
-            # translated revision is based on:
+            # The most significant approved change to the English article
+            # between {the English revision the current translated revision is
+            # based on} and {the latest ready-for-localization revision}:
             '(SELECT MAX(engrev.significance) '
              'FROM wiki_revision engrev, wiki_revision transrev '
-             'WHERE engrev.is_ready_for_localization '
+             'WHERE engrev.is_approved '
              'AND transrev.id=transdoc.current_revision_id '
              'AND engrev.document_id=transdoc.parent_id '
              'AND engrev.id>transrev.based_on_id '
+             'AND engrev.id<=engdoc.latest_localizable_revision_id'
             '), '
             # Whether there are any unreviewed revs of the translation made
             # since the current one:
@@ -412,6 +413,7 @@ def _query_and_params(self, max):
         return ('SELECT transdoc.slug, transdoc.title, engrev.reviewed, '
             'dashboards_wikidocumentvisits.visits '
             'FROM wiki_document transdoc '
+            'INNER JOIN wiki_document engdoc ON transdoc.parent_id=engdoc.id '
             'INNER JOIN wiki_revision engrev ON engrev.id='
             # The oldest English rev to have an approved, ready-for-
             # localization level-30 change since the translated doc had an
@@ -430,24 +432,26 @@ def _query_and_params(self, max):
                 'AND wiki_revision.significance>=%s '
                 'AND %s='
                 # Completely filter out outer selections where 30 is not the
-                # max signif of english revisions since trans was last
+                # max signif of approved English revisions since trans was last
                 # approved. Other maxes will be shown by other readouts.
                 # Optimize: try "30 IN" if MySQL's statistics gatherer is
                 # stupid/nonexistent; the inner query should be able to bail
                 # out early. [Ed: No effect on EXPLAIN on admittedly light test
                 # corpus.]
                   '(SELECT MAX(engsince.significance) '
-                  'FROM wiki_revision engsince '
-                  'WHERE engsince.document_id=transdoc.parent_id '
-                  # Assumes that any ready (and therefor approved) revision
-                  # became the current revision at some point: we don't let the
-                  # user go back and approve revisions older than the latest
-                  # approved one.
-                  'AND engsince.is_ready_for_localization '
-                  'AND engsince.id>'
-                  # The English revision the current translation's based on:
-                    '(SELECT based_on_id FROM wiki_revision basedonrev '
-                    'WHERE basedonrev.id=transdoc.current_revision_id) '
+                   'FROM wiki_revision engsince '
+                   'WHERE engsince.document_id=transdoc.parent_id '
+                   # Assumes that any approved revision became the current
+                   # revision at some point: we don't let the user go back and
+                   # approve revisions older than the latest approved one.
+                   'AND engsince.is_approved '
+                   # Consider revisions between the one the last translation
+                   # was based on and the latest ready-for-l10n one.
+                   'AND engsince.id>'
+                   # The English revision the current translation's based on:
+                     '(SELECT based_on_id FROM wiki_revision basedonrev '
+                     'WHERE basedonrev.id=transdoc.current_revision_id) '
+                   'AND engsince.id<=engdoc.latest_localizable_revision_id'
                   ')'
                 ') '
             # Join up the visits table for stats:

apps/dashboards/tests/test_readouts.py

@@ -2,7 +2,7 @@
 
 from nose.tools import eq_
 
-from dashboards.readouts import (UnreviewedReadout,
+from dashboards.readouts import (UnreviewedReadout, OutOfDateReadout,
                                  TemplateTranslationsReadout,
                                  MostVisitedTranslationsReadout)
 from sumo.tests import TestCase
@@ -14,18 +14,27 @@ class MockRequest(object):
     locale = 'de'  # Same locale as translated_revision uses by default
 
 
-class UnreviewedChangesTests(TestCase):
+class ReadoutTestCase(TestCase):
+    """Test case for one readout. Provides some convenience methods."""
+
+    def row(self):
+        """Return first row shown by the readout this class tests."""
+        return self.readout(MockRequest()).rows()[0]
+
+    def titles(self):
+        """Return the titles shown by the Unreviewed Changes readout."""
+        return [row['title'] for row in
+                self.readout(MockRequest()).rows()]
+
+
+class UnreviewedChangesTests(ReadoutTestCase):
     """Tests for the Unreviewed Changes readout
 
     I'm not trying to cover every slice of the Venn diagram--just the tricky
     bits.
 
     """
-    @staticmethod
-    def titles():
-        """Return the titles shown by the Unreviewed Changes readout."""
-        return [row['title'] for row in
-                UnreviewedReadout(MockRequest()).rows()]
+    readout = UnreviewedReadout
 
     def test_unrevieweds_after_current(self):
         """Show the unreviewed revisions with later creation dates than the
@@ -48,17 +57,14 @@ def test_rejected_newer_than_current(self):
         assert rejected.document.title not in self.titles()
 
 
-class MostVisitedTranslationsTests(TestCase):
+class MostVisitedTranslationsTests(ReadoutTestCase):
     """Tests for the Most Visited Translations readout
 
     This is an especially tricky readout, since it effectively implements a
     superset of all other readouts' status discriminators.
 
     """
-    @staticmethod
-    def row():
-        """Return first row shown by the Most Visited Translations readout."""
-        return MostVisitedTranslationsReadout(MockRequest()).rows()[0]
+    readout = MostVisitedTranslationsReadout
 
     def test_unreviewed(self):
         """Assert articles in need of review are labeled as such."""
@@ -118,14 +124,73 @@ def test_spam(self):
         r = revision(is_approved=False, save=True)
         eq_([], MostVisitedTranslationsReadout(MockRequest()).rows())
 
+    def test_consider_max_significance(self):
+        """When determining how significantly an article has changed since
+        translation, use the max significance of the approved revisions, not
+        just that of the latest ready-to-localize one."""
+        translation = translated_revision(is_approved=True, save=True)
+        revision(document=translation.document.parent,
+                 is_approved=True,
+                 is_ready_for_localization=False,  # should still count
+                 significance=MAJOR_SIGNIFICANCE,
+                 save=True)
+        revision(document=translation.document.parent,
+                 is_approved=True,
+                 is_ready_for_localization=True,
+                 significance=MEDIUM_SIGNIFICANCE,
+                 save=True)
+        row = self.row()
+        eq_(row['title'], translation.document.title)
+        eq_(unicode(row['status']), 'Immediate Update Needed')
+
+    def test_consider_only_approved_significances(self):
+        """Consider only approved significances when computing the max."""
+        translation = translated_revision(is_approved=True, save=True)
+        revision(document=translation.document.parent,
+                 is_approved=False,  # shouldn't count
+                 is_ready_for_localization=False,
+                 significance=MAJOR_SIGNIFICANCE,
+                 save=True)
+        revision(document=translation.document.parent,
+                 is_approved=True,
+                 is_ready_for_localization=True,
+                 significance=MEDIUM_SIGNIFICANCE,
+                 save=True)
+        row = self.row()
+        eq_(row['title'], translation.document.title)
+        # This should show as medium significance, because the revision with
+        # major significance is unapproved:
+        eq_(unicode(row['status']), 'Update Needed')
+
+
+class OutOfDateTests(ReadoutTestCase):
+    """Tests for OutOfDateReadout and, by dint of factoring,
+    NeedingUpdatesReadout."""
+
+    readout = OutOfDateReadout
+
+    def test_consider_max_significance(self):
+        """When determining how significantly an article has changed since
+        translation, use the max significance of the approved revisions, not
+        just that of the latest ready-to-localize one."""
+        translation = translated_revision(is_approved=True, save=True)
+        revision(document=translation.document.parent,
+                 is_approved=True,
+                 is_ready_for_localization=False,  # should still count
+                 significance=MAJOR_SIGNIFICANCE,
+                 save=True)
+        revision(document=translation.document.parent,
+                 is_approved=True,
+                 is_ready_for_localization=True,
+                 significance=MEDIUM_SIGNIFICANCE,
+                 save=True)
+        eq_([translation.document.title], self.titles())
+
 
-class TemplateTranslationsTests(TestCase):
+class TemplateTranslationsTests(ReadoutTestCase):
     """Tests for the Template Translations readout"""
 
-    @staticmethod
-    def row():
-        """Return first row shown by the Template Translations readout."""
-        return TemplateTranslationsReadout(MockRequest()).rows()[0]
+    readout = TemplateTranslationsReadout
 
     def test_not_template(self):
         """Documents that are not templates shouldn't show up in the list."""