[bug 679806] Don't count outdated articles as done on Overview l10n d…

…ash.
mythmon · Sep 30, 2011 · dcf5a05 · dcf5a05
2 parents 233e806 + 7846bd8
commit dcf5a05
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 45 deletions.
diff --git a/apps/dashboards/readouts.py b/apps/dashboards/readouts.py
@@ -38,11 +38,11 @@
     'LEFT JOIN dashboards_wikidocumentvisits ON engdoc.id='
         'dashboards_wikidocumentvisits.document_id '
         'AND dashboards_wikidocumentvisits.period=%s '
+    '{extra_joins} '
     'WHERE engdoc.locale=%s '
         'AND engdoc.is_localizable '
         'AND NOT engdoc.is_archived '
         'AND engdoc.latest_localizable_revision_id IS NOT NULL '
-        '{extra_where} '  # extra WHERE conditions
     'ORDER BY dashboards_wikidocumentvisits.visits DESC, '
              'COALESCE(transdoc.title, engdoc.title) ASC ').format
 
@@ -73,16 +73,24 @@
 # Whether there are any unreviewed revs of the translation made since the
 # current one:
 NEEDS_REVIEW = (
-            '(SELECT EXISTS '
-                '(SELECT * '
-                 'FROM wiki_revision transrev '
-                 'WHERE transrev.document_id=transdoc.id '
-                 'AND transrev.reviewed IS NULL '
-                 'AND (transrev.id>transdoc.current_revision_id OR '
-                      'transdoc.current_revision_id IS NULL)'
-                ')'
-            ') ')
-
+    '(SELECT EXISTS '
+        '(SELECT * '
+         'FROM wiki_revision transrev '
+         'WHERE transrev.document_id=transdoc.id '
+         'AND transrev.reviewed IS NULL '
+         'AND (transrev.id>transdoc.current_revision_id OR '
+              'transdoc.current_revision_id IS NULL)'
+        ')'
+    ') ')
+
+# Any ready-for-l10n, nontrivial-significance revision of the English doc newer
+# than the one our current translation is based on:
+ANY_SIGNIFICANT_UPDATES = (
+    '(SELECT id FROM wiki_revision engrev '
+     'WHERE engrev.document_id=engdoc.id '
+     'AND engrev.id>curtransrev.based_on_id '
+     'AND engrev.is_ready_for_localization '
+     'AND engrev.significance>=%s) ')
 
 def _cursor():
     """Return a DB cursor for reading."""
@@ -120,52 +128,83 @@ def _format_row_with_out_of_dateness(readout_locale, eng_slug, eng_title, slug,
 
 def overview_rows(locale):
     """Return the iterable of dicts needed to draw the Overview table."""
-    def percent_or_100(num, denom):
-        return int(round(num / float(denom) * 100)) if denom else 100
-
     # The Overview table is a special case: it has only a static number of
     # rows, so it has no expanded, all-rows view, and thus needs no slug, no
     # "max" kwarg on rows(), etc. It doesn't fit the Readout signature, so we
     # don't shoehorn it in.
 
+    def percent_or_100(num, denom):
+        return int(round(num / float(denom) * 100)) if denom else 100
+
+    def single_result(sql, params):
+        """Return the first column of the first row returned by a query."""
+        cursor = _cursor()
+        cursor.execute(sql, params)
+        return cursor.fetchone()[0]
+
     total = Document.uncached.filter(
-                locale=settings.WIKI_DEFAULT_LANGUAGE,
-                current_revision__isnull=False,
-                is_localizable=True,
-                latest_localizable_revision__isnull=False,
-                is_archived=False)
+        locale=settings.WIKI_DEFAULT_LANGUAGE,
+        is_archived=False,
+        current_revision__isnull=False,
+        is_localizable=True,
+        latest_localizable_revision__isnull=False)
     total_docs = total.filter(is_template=False).count()
+    total_templates = total.filter(is_template=True).count()
 
-    # How many approved documents are there in German that have parents?
-    # Even though users are technically allowed to translate revisions that
-    # aren't marked as ready-for-l10n, we restrict this set to ready-to-
+    # How many approved, up-to-date documents are there in German that have
+    # parents? Even though users are technically allowed to translate revisions
+    # that aren't marked as ready-for-l10n, we restrict this set to ready-to-
     # localize documents because we restrict the denominator to those.
     translated = Document.uncached.filter(
         locale=locale,
         is_archived=False,
         current_revision__isnull=False,
         parent__isnull=False,
         parent__latest_localizable_revision__isnull=False)
-    translated_docs = translated.filter(is_template=False).count()
-
-    # Of the top 20 most visited English articles, how many are not translated
-    # into German?
+    # Translations whose based_on revision has no >10-significance, ready-for-
+    # l10n revisions after it. It *might* be possible to do this with the ORM
+    # by passing wheres and tables to extra():
+    up_to_date_translation_count = (
+        'SELECT COUNT(*) FROM wiki_document transdoc '
+        'INNER JOIN wiki_document engdoc ON transdoc.parent_id=engdoc.id '
+        'INNER JOIN wiki_revision curtransrev '
+            'ON transdoc.current_revision_id=curtransrev.id '
+        'WHERE transdoc.locale=%s '
+            'AND transdoc.is_template=%s '
+            'AND NOT transdoc.is_archived '
+            'AND engdoc.latest_localizable_revision_id IS NOT NULL '
+            'AND engdoc.is_localizable '
+            'AND NOT EXISTS ' +
+                ANY_SIGNIFICANT_UPDATES)
+    # TODO: Optimize by running the above query just once and separating the
+    # templates from the non-templates with a GROUP BY.
+    translated_docs = single_result(up_to_date_translation_count,
+                                    (locale, False, MEDIUM_SIGNIFICANCE))
+    translated_templates = single_result(up_to_date_translation_count,
+                                         (locale, True, MEDIUM_SIGNIFICANCE))
+
+    # Of the top 20 most visited English articles, how many have up-to-date
+    # translations into German?
+    #
+    # TODO: Be very suspicious of this query. It selects from a subquery (known
+    # to MySQL's EXPLAIN as a "derived" table), and MySQL always materializes
+    # such subqueries and never builds indexes on them. However, it seems to be
+    # fast in practice.
     TOP_N = 20
-    cursor = _cursor()
-    cursor.execute(
-        'SELECT count(*) FROM '
-            '(SELECT transdoc.id '
-              + most_visited_translation_from(extra_where='') +
-             'LIMIT %s) t1 '
-        'WHERE t1.id IS NOT NULL',
-        (locale, THIS_WEEK, settings.WIKI_DEFAULT_LANGUAGE, TOP_N))
-    popular_translated = cursor.fetchone()[0]
-
-    # Template overview
-    total_templates = total.filter(is_template=True).count()
-
-    # How many approved templates are there in German that have parents?
-    translated_templates = translated.filter(is_template=True).count()
+    popular_translated = int(single_result(  # Driver returns a Decimal.
+        'SELECT SUM(istranslated) FROM '
+            '(SELECT transdoc.current_revision_id IS NOT NULL '
+                # And there have been no significant updates since the current
+                # translation:
+                'AND NOT EXISTS ' +
+                ANY_SIGNIFICANT_UPDATES +
+                'AS istranslated ' +
+             most_visited_translation_from(extra_joins=
+                'LEFT JOIN wiki_revision curtransrev '
+                    'ON transdoc.current_revision_id=curtransrev.id ') +
+             'LIMIT %s) t1 ',
+        (MEDIUM_SIGNIFICANCE, locale, THIS_WEEK,
+         settings.WIKI_DEFAULT_LANGUAGE, TOP_N)) or 0)  # SUM can return NULL.
 
     return {'most-visited': dict(
                  title=_('Most-Visited Articles'),
@@ -343,7 +382,7 @@ def _query_and_params(self, max):
             'transdoc.title, dashboards_wikidocumentvisits.visits, ' +
             MOST_SIGNIFICANT_CHANGE_READY_TO_TRANSLATE + ', ' +
             NEEDS_REVIEW +
-            most_visited_translation_from(extra_where='') +
+            most_visited_translation_from(extra_joins='') +
             self._limit_clause(max),
             (self.locale,
              ALL_TIME if self.mode == ALL_TIME else THIS_WEEK,

diff --git a/apps/dashboards/tests/test_readouts.py b/apps/dashboards/tests/test_readouts.py
@@ -90,8 +90,7 @@ def test_counting_unready_parents(self):
 
     def test_templates_and_docs_disjunct(self):
         """Make sure templates aren't included in the All Articles count."""
-        t = translated_revision(is_approved=True,
-                                save=True)
+        t = translated_revision(is_approved=True, save=True)
         # It shows up in All when it's a normal doc:
         eq_(1, overview_rows('de')['all']['numerator'])
         eq_(1, overview_rows('de')['all']['denominator'])
@@ -104,6 +103,47 @@ def test_templates_and_docs_disjunct(self):
         eq_(0, overview_rows('de')['all']['numerator'])
         eq_(0, overview_rows('de')['all']['denominator'])
 
+    def test_not_counting_outdated(self):
+        """Out-of-date translations shouldn't count as "done".
+
+        "Out-of-date" can mean either moderately or majorly out of date. The
+        only thing we don't care about is typo-level outdatedness.
+
+        """
+        t = translated_revision(is_approved=True, save=True)
+        overview = overview_rows('de')
+        eq_(1, overview['most-visited']['numerator'])
+        eq_(1, overview['all']['numerator'])
+
+        # Update the parent with a typo-level revision:
+        revision(document=t.document.parent,
+                 significance=TYPO_SIGNIFICANCE,
+                 is_approved=True,
+                 is_ready_for_localization=True,
+                 save=True)
+        # Assert it still shows up in the numerators:
+        overview = overview_rows('de')
+        eq_(1, overview['most-visited']['numerator'])
+        eq_(1, overview['all']['numerator'])
+
+        # Update the parent with a medium-level revision:
+        revision(document=t.document.parent,
+                 significance=MEDIUM_SIGNIFICANCE,
+                 is_approved=True,
+                 is_ready_for_localization=True,
+                 save=True)
+        # Assert it no longer shows up in the numerators:
+        overview = overview_rows('de')
+        eq_(0, overview['all']['numerator'])
+        eq_(0, overview['most-visited']['numerator'])
+
+    def test_not_counting_untranslated(self):
+        """Translations with no approved revisions shouldn't count as done."""
+        t = translated_revision(is_approved=False, save=True)
+        overview = overview_rows('de')
+        eq_(0, overview['most-visited']['numerator'])
+        eq_(0, overview['all']['numerator'])
+
 
 class UnreviewedChangesTests(ReadoutTestCase):
     """Tests for the Unreviewed Changes readout

diff --git a/apps/wiki/models.py b/apps/wiki/models.py
@@ -184,7 +184,8 @@ class Document(NotificationsMixin, ModelBase, BigVocabTaggableMixin):
     current_revision = models.ForeignKey('Revision', null=True,
                                          related_name='current_for+')
 
-    # Latest revision which both is_approved and is_ready_for_localization
+    # Latest revision which both is_approved and is_ready_for_localization,
+    # This may remain non-NULL even if is_localizable is changed to false.
     latest_localizable_revision = models.ForeignKey(
         'Revision', null=True, related_name='localizable_for+')