Never migrate partial translations (#44)

stasm · web-flow · commit e208161b7537 · 2018-02-06T17:26:39.000+01:00
Partial translations may break the AST because they produce `TextElements` with `None` values. For now, explicitly skip any transforms which depend on at least one missing legacy string. In the future we might be able to allow partial migrations in some situations, like migrating a subset of attributes of a message. See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271.
diff --git a/fluent/migrate/context.py b/fluent/migrate/context.py
@@ -285,7 +285,15 @@ def merge_changeset(self, changeset=None):
 
         The input data must be configured earlier using the `add_*` methods.
         if given, `changeset` must be a set of (path, key) tuples describing
-        which legacy translations are to be merged.
+        which legacy translations are to be merged. If `changeset` is None,
+        all legacy translations will be allowed to be migrated in a single
+        changeset.
+
+        The inner `in_changeset` function is used to determine if a message
+        should be migrated for the given changeset. It compares the legacy
+        dependencies of the transform defined for the message with legacy
+        translations available in the changeset. If all dependencies are
+        present, the message will be migrated.
 
         Given `changeset`, return a dict whose keys are resource paths and
         values are `FTL.Resource` instances.  The values will also be used to
@@ -306,10 +314,18 @@ def merge_changeset(self, changeset=None):
             transforms = self.transforms.get(path, [])
 
             def in_changeset(ident):
-                """Check if entity should be merged.
+                """Check if a message should be migrated.
+
+                A message will be migrated only if all of its dependencies
+                are present in the currently processed changeset.
+
+                If a transform defined for this message points to a missing
+                legacy translation, this message will not be merged. The
+                missing legacy dependency won't be present in the changeset.
 
-                If at least one dependency of the entity is in the current
-                set of changeset, merge it.
+                This also means that partially translated messages (e.g.
+                constructed from two legacy strings out of which only one is
+                avaiable) will never be migrated.
                 """
                 message_deps = self.dependencies.get((path, ident), None)
 
@@ -324,9 +340,11 @@ def in_changeset(ident):
                 if len(message_deps) == 0:
                     return True
 
-                # If the intersection of the dependencies and the current
-                # changeset is non-empty, merge this message.
-                return message_deps & changeset
+                # Make sure all the dependencies are present in the current
+                # changeset. Partial migrations are not currently supported.
+                # See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271
+                available_deps = message_deps & changeset
+                return message_deps == available_deps
 
             # Merge legacy translations with the existing ones using the
             # reference as a template.
diff --git a/tests/migrate/test_context.py b/tests/migrate/test_context.py
@@ -16,7 +16,7 @@
     EmptyLocalizationError, NotSupportedError, UnreadableReferenceError)
 from fluent.migrate.util import ftl, ftl_resource_to_json, to_json
 from fluent.migrate.context import MergeContext
-from fluent.migrate.transforms import COPY
+from fluent.migrate.transforms import CONCAT, COPY
 
 
 def here(*parts):
@@ -251,7 +251,7 @@ def test_missing_reference_file(self):
 
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
-class TestEmptyLocalization(unittest.TestCase):
+class TestMissingLocalizationFiles(unittest.TestCase):
     def setUp(self):
         # Silence all logging.
         logging.disable(logging.CRITICAL)
@@ -266,7 +266,40 @@ def tearDown(self):
         # Resume logging.
         logging.disable(logging.NOTSET)
 
-    def test_all_localization_missing(self):
+    def test_missing_file(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=COPY(
+                    'aboutDownloads.dtd',
+                    'aboutDownloads.title'
+                )
+            ),
+            FTL.Message(
+                id=FTL.Identifier('header'),
+                value=COPY(
+                    'missing.dtd',
+                    'missing'
+                )
+            ),
+        ])
+
+        expected = {
+            'aboutDownloads.ftl': ftl_resource_to_json('''
+        # This Source Code Form is subject to the terms of the Mozilla Public
+        # License, v. 2.0. If a copy of the MPL was not distributed with this
+        # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+        title = Pobrane pliki
+            ''')
+        }
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            expected
+        )
+
+    def test_all_files_missing(self):
         pattern = ('No localization files were found')
         with self.assertRaisesRegexp(EmptyLocalizationError, pattern):
             self.ctx.add_transforms('existing.ftl', 'existing.ftl', [
@@ -281,7 +314,9 @@ def test_all_localization_missing(self):
 
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
-class TestIncompleteLocalization(unittest.TestCase):
+class TestMissingLocalizationStrings(unittest.TestCase):
+    maxDiff = None
+
     def setUp(self):
         # Silence all logging.
         logging.disable(logging.CRITICAL)
@@ -296,20 +331,116 @@ def tearDown(self):
         # Resume logging.
         logging.disable(logging.NOTSET)
 
-    def test_missing_localization_file(self):
-        self.ctx.add_transforms('existing.ftl', 'existing.ftl', [
+    def test_missing_string_in_simple_value(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
             FTL.Message(
-                id=FTL.Identifier('foo'),
+                id=FTL.Identifier('title'),
                 value=COPY(
-                    'existing.dtd',
-                    'foo'
+                    'aboutDownloads.dtd',
+                    'missing'
                 )
             ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_only_variant(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
             FTL.Message(
-                id=FTL.Identifier('bar'),
-                value=COPY(
-                    'missing.dtd',
-                    'bar'
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing'
+                                )
+                            ),
+                        ]
+                    ),
+                )
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_all_variants(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('windows'),
+                                default=False,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.windows'
+                                )
+                            ),
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.other'
+                                )
+                            ),
+                        ]
+                    ),
+                )
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_one_of_variants(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('windows'),
+                                default=False,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'aboutDownloads.title'
+                                )
+                            ),
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.other'
+                                )
+                            ),
+                        ]
+                    ),
                 )
             ),
         ])
@@ -319,6 +450,83 @@ def test_missing_localization_file(self):
             {}
         )
 
+    def test_missing_string_in_only_attribute(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('one'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_all_attributes(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('one'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing.one'
+                        )
+                    ),
+                    FTL.Attribute(
+                        FTL.Identifier('two'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing.two'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_one_of_attributes(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('title'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'aboutDownloads.title'
+                        )
+                    ),
+                    FTL.Attribute(
+                        FTL.Identifier('missing'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
 class TestExistingTarget(unittest.TestCase):
@@ -360,7 +568,6 @@ def test_existing_target_ftl_missing_string(self):
             ''')
         }
 
-        self.maxDiff = None
         self.assertDictEqual(
             to_json(self.ctx.merge_changeset()),
             expected
@@ -399,7 +606,6 @@ def test_existing_target_ftl_existing_string(self):
             ''')
         }
 
-        self.maxDiff = None
         self.assertDictEqual(
             to_json(self.ctx.merge_changeset()),
             expected
diff --git a/tests/migrate/test_context_real_examples.py b/tests/migrate/test_context_real_examples.py
@@ -281,6 +281,8 @@ def test_merge_context_some_messages(self):
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
 class TestMergeAboutDialog(unittest.TestCase):
+    maxDiff = None
+
     def setUp(self):
         self.ctx = MergeContext(
             lang='pl',
@@ -363,6 +365,8 @@ def test_merge_context_all_messages(self):
     def test_merge_context_some_messages(self):
         changeset = {
             ('aboutDialog.dtd', 'update.failed.start'),
+            ('aboutDialog.dtd', 'update.failed.linkText'),
+            ('aboutDialog.dtd', 'update.failed.end'),
         }
 
         expected = {
@@ -379,3 +383,14 @@ def test_merge_context_some_messages(self):
             to_json(self.ctx.merge_changeset(changeset)),
             expected
         )
+
+    def test_merge_context_too_few_messages(self):
+        changeset = {
+            ('aboutDialog.dtd', 'update.failed.start'),
+            ('aboutDialog.dtd', 'update.failed.linkText'),
+        }
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset(changeset)),
+            {}
+        )
diff --git a/tests/migrate/test_merge.py b/tests/migrate/test_merge.py
