launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/copy-translations-from-anywhere-actually into lp:launchpad.

Commit message:
copy-translations-from-parent.py is now copy-distroseries-translations.py, and it takes explicit --from-distribution/--from-series options. previous_series is still the default.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/copy-translations-from-anywhere-actually/+merge/231329

copy-translations-from-parent.py is now copy-distroseries-translations.py, and it takes explicit --from-distribution/--from-series options. previous_series is still the default.
-- 
https://code.launchpad.net/~wgrant/launchpad/copy-translations-from-anywhere-actually/+merge/231329
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/copy-translations-from-anywhere-actually into lp:launchpad.

=== modified file 'lib/lp/translations/doc/distroseries-translations-copy.txt'
--- lib/lp/translations/doc/distroseries-translations-copy.txt	2014-08-19 03:59:56 +0000
+++ lib/lp/translations/doc/distroseries-translations-copy.txt	2014-08-19 10:19:43 +0000
@@ -10,8 +10,7 @@
     >>> login("foo.bar@xxxxxxxxxxxxx")
     >>> foobuntu = factory.makeDistribution('foobuntu', 'Foobuntu')
     >>> barty = factory.makeDistroSeries(foobuntu, '99.0', name='barty')
-    >>> carty = factory.makeDistroSeries(
-    ...     foobuntu, '99.1', name='carty', previous_series=barty)
+    >>> carty = factory.makeDistroSeries(foobuntu, '99.1', name='carty')
 
 Functions to create source packages, templates and and translations.
 
@@ -66,7 +65,7 @@
 We need a transaction manager (in this case a fake one) to make the copy work.
 
     >>> from lp.testing.faketransaction import FakeTransaction
-    >>> transaction_stub = FakeTransaction()
+    >>> txn = FakeTransaction()
 
 
 Performing the migration
@@ -79,7 +78,8 @@
     >>> from lp.services.log.logger import DevNullLogger
     >>> from lp.translations.model.distroseries_translations_copy import (
     ...     copy_active_translations)
-    >>> copy_active_translations(carty, transaction_stub, DevNullLogger())
+    >>> logger = DevNullLogger()
+    >>> copy_active_translations(barty, carty, txn, logger)
 
 All current templates were copied from the parent series but the deactivated
 template template3 was not copied.
@@ -129,11 +129,11 @@
 again as it only operates on distroseries without any translation templates.
 Because of message sharing incremental copies are no longer needed.
 
-    >>> copy_active_translations(carty, transaction_stub, DevNullLogger())
+    >>> copy_active_translations(barty, carty, txn, logger)
     Traceback (most recent call last):
     ...
     AssertionError:
-    The child series must not yet have any translation templates.
+    The target series must not yet have any translation templates.
 
 
 Running the script
@@ -160,7 +160,7 @@
 
     >>> from lp.testing.script import run_script
     >>> returnvalue, output, error_output = run_script(
-    ...     'scripts/copy-translations-from-parent.py',
+    ...     'scripts/copy-distroseries-translations.py',
     ...     ['--distribution=foobuntu', '--series=darty'])
     >>> returnvalue
     1
@@ -188,7 +188,7 @@
     >>> transaction.abort()
     >>> darty = DistroSeries.get(darty_id)
     >>> returnvalue, output, error_output = run_script(
-    ...     'scripts/copy-translations-from-parent.py',
+    ...     'scripts/copy-distroseries-translations.py',
     ...     ['--distribution=foobuntu', '--series=darty', '--force'])
     >>> returnvalue
     0
@@ -196,7 +196,8 @@
     INFO    Creating lockfile:
       /var/lock/launchpad-copy-missing-translations-foobuntu-darty.lock
     INFO    Starting...
-    INFO    Populating blank distroseries darty with translations from barty.
+    INFO    Populating blank distroseries foobuntu darty with
+            translations from foobuntu barty.
     INFO    Extracting from potemplate into
             "temp_potemplate_holding_foobuntu_darty"...
     INFO    Extracting from translationtemplateitem into
@@ -231,3 +232,40 @@
     >>> print sorted([template.name for template in dartempls])
     [u'template1', u'template2']
 
+The script defaults to copying from the given series' previous_series,
+but that can be overridden.
+
+    >>> grumpy = factory.makeDistroSeries(
+    ...     distribution=factory.makeDistribution(name="notbuntu"),
+    ...     name='grumpy')
+    >>> grumpy_id = grumpy.id
+    >>> transaction.commit()
+
+    >>> returnvalue, output, error_output = run_script(
+    ...     'scripts/copy-distroseries-translations.py',
+    ...     ['--distribution=notbuntu', '--series=grumpy'])
+    >>> returnvalue
+    2
+    >>> print error_output
+    INFO    Creating lockfile:
+      /var/lock/launchpad-copy-missing-translations-notbuntu-grumpy.lock
+    Usage: copy-distroseries-translations.py [options]
+    copy-distroseries-translations.py: error: No source series specified
+    and target has no previous series.
+    <BLANKLINE>
+
+    >>> returnvalue, output, error_output = run_script(
+    ...     'scripts/copy-distroseries-translations.py',
+    ...     ['--distribution=notbuntu', '--series=grumpy',
+    ...      '--from-distribution=foobuntu' , '--from-series=darty'])
+    >>> returnvalue
+    0
+    >>> print error_output
+    INFO    Creating lockfile:
+      /var/lock/launchpad-copy-missing-translations-notbuntu-grumpy.lock
+    INFO    Starting...
+    INFO    Populating blank distroseries notbuntu grumpy with
+            translations from foobuntu darty.
+    ...
+    INFO    Done.
+    <BLANKLINE>

=== modified file 'lib/lp/translations/model/distroseries_translations_copy.py'
--- lib/lp/translations/model/distroseries_translations_copy.py	2012-05-08 03:10:09 +0000
+++ lib/lp/translations/model/distroseries_translations_copy.py	2014-08-19 10:19:43 +0000
@@ -1,7 +1,7 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Functions to copy translations from previous to child distroseries."""
+"""Functions to copy translations from one distroseries to another."""
 
 __metaclass__ = type
 
@@ -47,16 +47,18 @@
         """ % params)
 
 
-def copy_active_translations(child, transaction, logger):
-    """Furnish untranslated child `DistroSeries` with previous series's
-    translations.
+def copy_active_translations(source, target, transaction, logger):
+    """Populate target `DistroSeries` with source series' translations.
+
+    The target must not already have any translations.
 
     This method uses `MultiTableCopy` to copy data.
 
-    Translation data for the new series ("child") is first copied into holding
-    tables called e.g. "temp_POTemplate_holding_ubuntu_feisty" and processed
-    there.  Then, near the end of the procedure, the contents of these holding
-    tables are all poured back into the original tables.
+    Translation data for the new series ("target") is first copied into
+    holding tables called e.g. "temp_POTemplate_holding_ubuntu_feisty"
+    and processed there.  Then, near the end of the procedure, the
+    contents of these holding tables are all poured back into the
+    original tables.
 
     If this procedure fails, it may leave holding tables behind.  This was
     done deliberately to leave some forensics information for failures, and
@@ -68,24 +70,19 @@
     process of being poured back into its source table.  In that case the
     sensible thing to do is probably to continue pouring it.
     """
-    previous_series = child.previous_series
-    if previous_series is None:
-        # We don't have a previous series from where we could copy
-        # translations.
-        return
-
     translation_tables = ['potemplate', 'translationtemplateitem', 'pofile']
 
-    full_name = "%s_%s" % (child.distribution.name, child.name)
+    full_name = "%s_%s" % (target.distribution.name, target.name)
     copier = MultiTableCopy(full_name, translation_tables, logger=logger)
 
     # Incremental copy of updates is no longer supported
-    assert not child.has_translation_templates, (
-           "The child series must not yet have any translation templates.")
+    assert not target.has_translation_templates, (
+        "The target series must not yet have any translation templates.")
 
     logger.info(
-        "Populating blank distroseries %s with translations from %s." %
-        (child.name, previous_series.name))
+        "Populating blank distroseries %s %s with translations from %s %s." %
+        (target.distribution.name, target.name, source.distribution.name,
+         source.name))
 
     # 1. Extraction phase--for every table involved (called a "source table"
     # in MultiTableCopy parlance), we create a "holding table."  We fill that
@@ -110,7 +107,7 @@
 
     # Copy relevant POTemplates from existing series into a holding table,
     # complete with their original id fields.
-    where = 'distroseries = %s AND iscurrent' % quote(previous_series)
+    where = 'distroseries = %s AND iscurrent' % quote(source)
     copier.extract('potemplate', [], where)
 
     # Now that we have the data "in private," where nobody else can see it,
@@ -126,14 +123,14 @@
             datecreated =
                 timezone('UTC'::text,
                     ('now'::text)::timestamp(6) with time zone)
-    ''' % (copier.getHoldingTableName('potemplate'), quote(child)))
+    ''' % (copier.getHoldingTableName('potemplate'), quote(target)))
 
     # Copy each TranslationTemplateItem whose template we copied, and let
     # MultiTableCopy replace each potemplate reference with a reference to
     # our copy of the original POTMsgSet's potemplate.
     copier.extract('translationtemplateitem', ['potemplate'], 'sequence > 0')
 
-    # Copy POFiles, making them refer to the child's copied POTemplates.
+    # Copy POFiles, making them refer to the target's copied POTemplates.
     copier.extract(
         'pofile', ['potemplate'],
         batch_pouring_callback=omit_redundant_pofiles)

=== modified file 'lib/lp/translations/scripts/copy_distroseries_translations.py'
--- lib/lp/translations/scripts/copy_distroseries_translations.py	2014-08-19 05:13:06 +0000
+++ lib/lp/translations/scripts/copy_distroseries_translations.py	2014-08-19 10:19:43 +0000
@@ -73,16 +73,16 @@
         series.defer_translation_imports = self.defer_translation_imports
 
 
-def copy_distroseries_translations(distroseries, txn, logger):
-    """Copy `distroseries` translations from its parents.
+def copy_distroseries_translations(source, target, txn, logger):
+    """Copy translations into a new `DistroSeries`.
 
-    Wraps around `DistroSeries.copyMissingTranslationsFromParent`, but also
-    ensures that the `hide_all_translations` and `defer_translation_imports`
-    flags are set.  After copying they are restored to their previous state.
+    Wraps around `copy_active_translations`, but also ensures that the
+    `hide_all_translations` and `defer_translation_imports` flags are
+    set.  After copying they are restored to their previous state.
     """
     statekeeper = SeriesStateKeeper()
-    statekeeper.prepare(distroseries)
-    name = distroseries.name
+    statekeeper.prepare(target)
+    name = target.name
     txn.commit()
     txn.begin()
 
@@ -90,15 +90,15 @@
 
     try:
         # Do the actual work.
-        assert distroseries.defer_translation_imports, (
+        assert target.defer_translation_imports, (
             "defer_translation_imports not set!"
             " That would corrupt translation data mixing new imports"
             " with the information being copied.")
-        assert distroseries.hide_all_translations, (
+        assert target.hide_all_translations, (
             "hide_all_translations not set!"
             " That would allow users to see and modify incomplete"
             " translation state.")
-        copy_active_translations(distroseries, txn, logger)
+        copy_active_translations(source, target, txn, logger)
     except:
         copy_failed = True
         # Give us a fresh transaction for proper cleanup.

=== modified file 'lib/lp/translations/tests/test_distroseries_translations_copy.py'
--- lib/lp/translations/tests/test_distroseries_translations_copy.py	2012-06-14 05:18:22 +0000
+++ lib/lp/translations/tests/test_distroseries_translations_copy.py	2014-08-19 10:19:43 +0000
@@ -38,8 +38,7 @@
         # place and does not copy it.  (Nor does it raise an error.)
         existing_series = self.factory.makeDistroSeries(name='existing')
         new_series = self.factory.makeDistroSeries(
-            name='new', distribution=existing_series.distribution,
-            previous_series=existing_series)
+            name='new', distribution=existing_series.distribution)
         template = self.factory.makePOTemplate(distroseries=existing_series)
         pofile = self.factory.makePOFile(potemplate=template)
         self.factory.makeCurrentTranslationMessage(
@@ -62,7 +61,8 @@
         MultiTableCopy._pourTable = pour_or_stop_at_pofile
         try:
             copy_active_translations(
-                new_series, FakeTransaction(), DevNullLogger())
+                existing_series, new_series, FakeTransaction(),
+                DevNullLogger())
         except EarlyExit as e:
             pour_args = e.args
             pour_kwargs = e.kwargs

=== renamed file 'scripts/copy-translations-from-parent.py' => 'scripts/copy-distroseries-translations.py'
--- scripts/copy-translations-from-parent.py	2013-03-25 23:39:42 +0000
+++ scripts/copy-distroseries-translations.py	2014-08-19 10:19:43 +0000
@@ -32,9 +32,17 @@
     def add_my_options(self):
         self.parser.add_option('-d', '--distribution', dest='distro',
             default='ubuntu',
-            help='Name of distribution to copy translations in.')
+            help='The target distribution.')
         self.parser.add_option('-s', '--series', dest='series',
-            help='Name of distroseries whose translations should be updated')
+            help='The target distroseries.')
+        self.parser.add_option('--from-distribution', dest='from_distro',
+            help=(
+                "The source distribution (if omitted, target's previous "
+                "series will be used)."))
+        self.parser.add_option('--from-series', dest='from_series',
+            help=(
+                "The source distroseries (if omitted, target's previous "
+                "series will be used)."))
         self.parser.add_option('-f', '--force', dest='force',
             action="store_true", default=False,
             help="Don't check if target's UI and imports are blocked; "
@@ -46,13 +54,23 @@
         return getUtility(IDistributionSet)[self.options.distro][series]
 
     def main(self):
-        series = self._getTargetSeries()
+        target = getUtility(IDistributionSet)[self.options.distro][
+            self.options.series]
+        if self.options.from_distro:
+            source = getUtility(IDistributionSet)[self.options.from_distro][
+                self.options.from_series]
+        else:
+            source = target.previous_series
+        if source is None:
+            self.parser.error(
+                "No source series specified and target has no previous "
+                "series.")
 
         # Both translation UI and imports for this series should be blocked
         # while the copy is in progress, to reduce the chances of deadlocks or
         # other conflicts.
         blocked = (
-            series.hide_all_translations and series.defer_translation_imports)
+            target.hide_all_translations and target.defer_translation_imports)
         if not blocked and not self.options.force:
             self.txn.abort()
             self.logger.error(
@@ -66,7 +84,7 @@
         self.logger.info('Starting...')
 
         # Actual work is done here.
-        copy_distroseries_translations(series, self.txn, self.logger)
+        copy_distroseries_translations(source, target, self.txn, self.logger)
 
         # We would like to update the DistroRelase statistics, but it takes
         # too long so this should be done after.