Merge lp:~henninge/launchpad/recife-bug-611674-convert-imports into lp:~launchpad/launchpad/recife

[Henning Eggers]

Bug 611674
==========

Although the bug does not say so, this branch is about converting the import code to the new models and use the new methods for adding translations and marking them as current.

The main change is in FileImporter.storeTranslations which used to call updateTranslations. The new scheme is to
  1. Add the new translation as a suggestion
  2. Try to validate it
  3. Approve the suggestion if it validates and has no conflicts.

A large part of the diff is made up of the new property "FileImporter.share_with_other_side" and its tests. The property is a boolean that reflects the sharing policy for translations from the file that is being imported. The policy depends on where the translations are coming from and what they are being imported into and on the permissions of the uploader. This was isolated to be able to change this policy later. (And I admit it could have warranted an extra branch.)

Implementation details
----------------------

rosetta-karma.txt:
Because of the new scheme, import karma is now awarded for adding suggestions, not for approving them. Slightly different but not much.

30-rosetta-pofile-translation-gettext-error.txt:
I don't know why gettextpo mentions "msgid_plural" nowadays. This code will pass for both "msgid" and "msgid_plural".

sanitize.py, test_sanitize.py:
A test failed because a language returned "None" for pluralforms. I am not sure if that is a bug in itself.

translation_import.py:
The new methods translations_are_msgids, _storeCredits, _validateMessage and _approveMessage are all code that used to be in storeTranslationsInDatabase. This should make it more readable (based on a suggestion by jtv).
The flow in storeStoreTranslationsInDatabase has changed slightly but as all tests are passing ...

Tests
-----

I ran all lp.translations tests to find breakage but I think something like this would cover most of the import code:

bin/test -vvcm lp.translations -t poimport.txt -t poimport-script.txt -t test_file_importer -t test_sanitize -t stories.translations

Lint
----

Lint complains about bad indention in 30-rosetta-pofile-translation-gettext-error.txt which is true but which I am not going to fix here.

[Данило Шеган (danilo)]

Hi Henning,

Thanks for tackling this. You've done a part of another card with this
job, so good job there :)

I feel it still needs a few improvements. See below.

  review needs-fixing

> A large part of the diff is made up of the new property
> "FileImporter.share_with_other_side" and its tests. The property is a
> boolean that reflects the sharing policy for translations from the
> file that is being imported. The policy depends on where the
> translations are coming from and what they are being imported into and
> on the permissions of the uploader. This was isolated to be able to
> change this policy later. (And I admit it could have warranted an
> extra branch.)

Interesting approach. We'll need to extract this even further, but
we've already got a separate card for that. You've just made that job
simpler (though, it's not going to be identical, since we'll have to
support the specifics of the web UI as well).

> === modified file 'lib/lp/translations/doc/poimport-script.txt'
> --- lib/lp/translations/doc/poimport-script.txt 2010-09-21 16:19:47
> +0000
> +++ lib/lp/translations/doc/poimport-script.txt 2010-09-30 08:53:45 +0000
> @@ -282,9 +282,9 @@
>
> >>> from lp.services.worlddata.interfaces.language import ILanguageSet
> >>> esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
> - >>> foos = potemplate['Foo %s'].getLocalTranslationMessages(
> + >>> suggestions = potemplate['Foo %
> s'].getLocalTranslationMessages(
> ... potemplate, esperanto)
> - >>> sorted([foo.msgstr0.translation for foo in foos])
> + >>> sorted([sugg.msgstr0.translation for sugg in suggestions])
> [u'Bar', u'Bars']

I don't really like "sugg" much better than "foo" (though, "suggestions"
instead of "foos" IS MUCH better :). How about "suggestion"?

> === modified file 'lib/lp/translations/utilities/sanitize.py'
> --- lib/lp/translations/utilities/sanitize.py 2010-08-16 09:01:35 +0000
> +++ lib/lp/translations/utilities/sanitize.py 2010-09-30 08:53:45 +0000
> @@ -181,6 +181,8 @@
>
> # Expected plural forms should all exist and empty translations should
> # be normalized to None.
> + if pluralforms is None:
> + pluralforms = 2
> for pluralform in range(pluralforms):
> if pluralform not in sanitized_translations:
> sanitized_translations[pluralform] = None

What's the reasoning for this? Why not default to 1 instead (i.e.
assume it's a message with no plurals at all)?

> === modified file 'lib/lp/translations/utilities/tests/test_sanitize.py'
> --- lib/lp/translations/utilities/tests/test_sanitize.py 2010-08-23 08:41:03 +0000
> +++ lib/lp/translations/utilities/tests/test_sanitize.py 2010-09-30 08:53:45 +0000
> @@ -257,6 +257,20 @@
> expected_sanitized,
> sanitize_translations_from_webui(self.english, translations, 3))
>
> + def test_sanitize_translations_pluralforms_none(self):
> + # Some languages don't provide a plural form, so 2 is assumed.
> + translations = {
> + 0: u'Plural form 0 ',
> + }
> + expected_sanitized = {
> + 0: u'Plural form 0',
> + ...

review: Needs Fixing

[Henning Eggers (henninge)]

> > === modified file 'lib/lp/translations/doc/poimport-script.txt'
> > --- lib/lp/translations/doc/poimport-script.txt 2010-09-21 16:19:47
> > +0000
> > +++ lib/lp/translations/doc/poimport-script.txt 2010-09-30 08:53:45 +0000
> > @@ -282,9 +282,9 @@
> >
> > >>> from lp.services.worlddata.interfaces.language import ILanguageSet
> > >>> esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
> > - >>> foos = potemplate['Foo %s'].getLocalTranslationMessages(
> > + >>> suggestions = potemplate['Foo %
> > s'].getLocalTranslationMessages(
> > ... potemplate, esperanto)
> > - >>> sorted([foo.msgstr0.translation for foo in foos])
> > + >>> sorted([sugg.msgstr0.translation for sugg in suggestions])
> > [u'Bar', u'Bars']
>
> I don't really like "sugg" much better than "foo" (though, "suggestions"
> instead of "foos" IS MUCH better :). How about "suggestion"?

Yeah, I didn't think it would fit. It does.

>
> > === modified file 'lib/lp/translations/utilities/sanitize.py'
> > --- lib/lp/translations/utilities/sanitize.py 2010-08-16 09:01:35 +0000
> > +++ lib/lp/translations/utilities/sanitize.py 2010-09-30 08:53:45 +0000
> > @@ -181,6 +181,8 @@
> >
> > # Expected plural forms should all exist and empty translations should
> > # be normalized to None.
> > + if pluralforms is None:
> > + pluralforms = 2
> > for pluralform in range(pluralforms):
> > if pluralform not in sanitized_translations:
> > sanitized_translations[pluralform] = None
>
> What's the reasoning for this? Why not default to 1 instead (i.e.
> assume it's a message with no plurals at all)?

That's what Language does. I could remove it here and use language.guessed_pluralforms when calling the function.
Also, there is something else I was wondering about. The function will fill in the missing plural forms with None, regardless of whether there is a msgid_plural or not. I don't remember if that is intended behavior. It does no harm but seems like a waste.

>
> > === modified file
> 'lib/lp/translations/utilities/tests/test_file_importer.py'
> ...
> > +class FileImporterSharingTest(TestCaseWithFactory):
> > + """Class test for the sharing operation of the FileImporter base
> class."""
> > + layer = LaunchpadZopelessLayer
> > +
> > + UPSTREAM = 0
> > + UBUNTU = 1
> > + LANGCODE = 'eo'
>
> I know this is how we usually used to do stuff, but today we should be
> able to also make languages and not depend on existing ones in the
> sampledata. For instance, if you really want to keep using 'eo', I'd
> suggest you do factory.makeLanguage('eo@test').

No, I am not intend on this. Actually I defined it here so that it does not appear in the code itself. Changed it to use a constructed Language.

>
> > + POFILE = dedent("""\
> > + msgid ""
> > + msgstr ""
> > + "PO-Revision-Date: 2005-05-03 20:41+0100\\n"
> > + "Last-Translator: FULL NAME <EMAIL@ADDRESS>\\n"
> > + "Content-Type: text/plain; charset=UTF-8\\n"
> > + "X-Launchpad-Export-Date: 2009-05-14 08:54+0000\\n"
> > +
> > + msgid "Thank You"
> > + msgstr "Dankon"
> > + ...

[Henning Eggers (henninge)]

Incremental diff.

[Данило Шеган (danilo)]

У пет, 01. 10 2010. у 14:43 +0000, Henning Eggers пише:

> That's what Language does. I could remove it here and use
> language.guessed_pluralforms when calling the function. Also, there is
> something else I was wondering about. The function will fill in the
> missing plural forms with None, regardless of whether there is a
> msgid_plural or not. I don't remember if that is intended behavior. It
> does no harm but seems like a waste.

Let's keep it simple for now and not touch anything. It definitely
sounds a bit wasteful, but it's not a big deal.

> > I know this is how we usually used to do stuff, but today we should be
> > able to also make languages and not depend on existing ones in the
> > sampledata. For instance, if you really want to keep using 'eo', I'd
> > suggest you do factory.makeLanguage('eo@test').
>
> No, I am not intend on this. Actually I defined it here so that it does
> not appear in the code itself. Changed it to use a constructed
> Language.

Cool, thanks.

> > > + if side == self.UPSTREAM:
> > > + potemplate = self.upstream_template
> > > + else:
> > > + # Create a template in a source package and link the source
> > > + # package to the upstream series to enable sharing.
> > > + ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> > > + distroseries =
> > self.factory.makeDistroSeries(distribution=ubuntu)
> > > + ubuntu.translation_focus = distroseries
> > > + sourcepackagename = self.factory.makeSourcePackageName()
> > > + potemplate = self.factory.makePOTemplate(
> > > + distroseries=distroseries,
> > > + sourcepackagename=sourcepackagename,
> > > + name=self.upstream_template.name)
> > > + self.factory.makeSourcePackagePublishingHistory(
> > > + sourcepackagename=sourcepackagename,
> > > + distroseries=distroseries)
> >
> > Why is this necessary? Is it not possible to reach a sourcepackage
> > without it?
>
> For some reason I cannot use sourcepackage.productseries if that has not been set.

Right, it's probably useful knowledge worth sharing across the
translations team at least. Knowing the background would be even better
because then we could reconstruct it in the future :)

> >
> > > + return entry
> > > +
> > > + def test_translator_persmissions(self):
> >
> > s/perSmissions/permissions/

You seem to have forgotten this bit.

> >
> > > + def test_templates_are_sharing(self):
> > > + # Sharing between upstream and Ubuntu was set up correctly.
> > > + entry = self._makeEntry(self.UBUNTU)
> > > + subset = getUtility(IPOTemplateSet).getSharingSubset(
> > > + distribution=entry.distroseries.distribution,
> > > + sourcepackagename=entry.sourcepackagename)
> > > + self.assertContentEqual(
> > > + [entry.potemplate, self.upstream_template],
> > > + list(subset.getSharingPOTemplates(entry.potemplate.name)))
> >
> > This looks a lot like a test for the code that's not really here,
> > doesn't it? :)
> >
> > Or, is it basicall...

review: Approve

[Henning Eggers (henninge)]

Am 05.10.2010 13:34, schrieb Данило Шеган:
>>>> + self.factory.makeSourcePackagePublishingHistory(
>>>> + sourcepackagename=sourcepackagename,
>>>> + distroseries=distroseries)
>>>
>>> Why is this necessary? Is it not possible to reach a sourcepackage
>>> without it?
>>
>> For some reason I cannot use sourcepackage.productseries if that has not been set.
>
> Right, it's probably useful knowledge worth sharing across the
> translations team at least. Knowing the background would be even better
> because then we could reconstruct it in the future :)

Hm, should I put a card in the backlog to research that?

>
>>>
>>>> + return entry
>>>> +
>>>> + def test_translator_persmissions(self):
>>>
>>> s/perSmissions/permissions/
>
> You seem to have forgotten this bit.

I did, thanks. ;)

> It'd still be nice to name the test so it reflects this: eg.
> "test_makeImportEntry_templates_are_sharing".

Renamed.

>
>>> Cool stuff. It's amazing how tests can be so simple for something this
>>> intricate. :)
>>
>> I see the smiley but I am not sure if you are making fun of me or not ... ;-/
>
> No, it's a smiley of happiness. Also a credit to your "factor out stuff
> that is not being tested" (your _makeImportEntry method).

Thank you. :-)

>>>> + @cachedproperty
>>>> + def translations_are_msgids(self):
>>>> + """Are symbolic msgids being used and these are the real ones?"""
>>>
>>> This sentence is a bit confusing. If I had no idea about what this is
>>> about, I'd have even less idea about it. :)
>>
>> OK, I tried better. ;-)
>
> I think "text" is a non-countable noun, so plural "texts" sounds weird:

It can be both. In this case it is countable

>>> This is actually very interesting, and I am happy to see that you
>>> extracted it out. It actually points out that the naming for
>>> "from_upstream" is not really perfect: it should probably be
>>> "from_maintainer" or "from_trusted" or something.
>>
>> Yes, Jeroen and I had talked about that, too. I think I like from_maintainer most.
>
> Let's add a card for that then.

Done.

>>> Oh, I just read up on any() :) It seems you can drop the comparison to
>>> the empty list as well.
>>
>> Good catch! I was a bit worried about using "any" because it tests for
>> "False" but we are looking for empty strings and "None". Although
>> these are by definition "False", too, our coding standards always
>> forbade implicit bool checks like these.
>
> That was kind of my worry as well, but I guess there's no harm since the
> intent is clear enough.

I put it on the reviewer meeting agenda.

>
>>> With the new model, there will be no way to know if a translation
>>> credits message didn't validate, but is the actual one from upstream.
>>> Something to think about.
>>
>> Hm, we could still validate and mark the message accordingly, even if
>> it is used, right? I am not sure, though, if translators expect us to
>> catch bugs like that. Maybe we should discard c-format flags on
>> translation credits POTMsgSets altogether?
>
> Definitely worth thinking about some more. How about you file a bug
> about this to look into after all the "re...

=== modified file 'lib/lp/translations/doc/poimport-script.txt'
--- lib/lp/translations/doc/poimport-script.txt	2010-09-21 16:19:47 +0000
+++ lib/lp/translations/doc/poimport-script.txt	2010-10-06 11:07:49 +0000
@@ -282,9 +282,9 @@
 
     >>> from lp.services.worlddata.interfaces.language import ILanguageSet
     >>> esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
-    >>> foos = potemplate['Foo %s'].getLocalTranslationMessages(
+    >>> suggestions = potemplate['Foo %s'].getLocalTranslationMessages(
     ...     potemplate, esperanto)
-    >>> sorted([foo.msgstr0.translation for foo in foos])
+    >>> sorted([suggestion.msgstr0.translation for suggestion in suggestions])
     [u'Bar', u'Bars']
 
 Since this last upload was not the upstream one, however, its credits

=== modified file 'lib/lp/translations/doc/rosetta-karma.txt'
--- lib/lp/translations/doc/rosetta-karma.txt	2010-08-04 11:00:51 +0000
+++ lib/lp/translations/doc/rosetta-karma.txt	2010-10-06 11:07:49 +0000
@@ -221,14 +221,14 @@
 Tell the PO file to import from the file data it has.  The user has rights
 to edit translations directly, so his suggestion is approved directly.
 Even when the user is also a reviwer, he should not get karma for reviewing
-his own translations. IOW, he'll only get karma for his suggestion being
-approved.
+or approving his own translations. IOW, he'll only get karma for submitting
+his suggestion.
 
     >>> potemplate = POTemplate.get(1)
     >>> entry = translation_import_queue[entry_id]
     >>> pofile = potemplate.getPOFileByLang('es')
     >>> (subject, message) = pofile.importFromQueue(entry)
-    Karma added: action=translationsuggestionapproved, product=evolution
+    Karma added: action=translationsuggestionadded, product=evolution
     >>> transaction.commit()
 
 Let's try the case when a file is uploaded, but no translation is changed.
@@ -384,7 +384,8 @@
 We do the update and see the karma being assigned.
 
     >>> status = potemplate_view.initialize()
-    Karma added: action=translationtemplatedescriptionchanged, product=evolution
+    Karma added:
+    action=translationtemplatedescriptionchanged, product=evolution
 
 And the new one is:
 

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/model/pofile.py	2010-10-06 11:07:49 +0000
@@ -538,7 +538,8 @@
         applicable_template = Coalesce(
             TranslationMessage.potemplateID, self.potemplate.id)
         clauses = [
-            TranslationTemplateItem.potmsgsetID == TranslationMessage.potmsgsetID,
+            TranslationTemplateItem.potmsgsetID == (
+                TranslationMessage.potmsgsetID),
             TranslationTemplateItem.potemplate == self.potemplate,
             TranslationMessage.language == self.language,
             applicable_template == self.potemplate.id,
@@ -1108,8 +1109,7 @@
 
         # Prepare the mail notification.
         msgsets_imported = self.getTranslationMessages(
-            TranslationMessage.was_obsolete_in_last_import == False
-            ).count()
+            TranslationMessage.was_obsolete_in_last_import == False).count()
 
         replacements = collect_import_info(entry_to_import, self, warnings)
         replacements.update({
@@ -1179,8 +1179,6 @@
             # Assign karma to the importer if this is not an automatic import
             # (all automatic imports come from the rosetta expert user) and
             # comes from upstream.
-            celebs = getUtility(ILaunchpadCelebrities)
-            rosetta_experts = celebs.rosetta_experts
             if (entry_to_import.from_upstream and
                 entry_to_import.importer.id != rosetta_experts.id):
                 entry_to_import.importer.assignKarma(

=== modified file 'lib/lp/translations/stories/translations/30-rosetta-pofile-translation-gettext-error.txt'
--- lib/lp/translations/stories/translations/30-rosetta-pofile-translation-gettext-error.txt	2010-09-20 05:32:10 +0000
+++ lib/lp/translations/stories/translations/30-rosetta-pofile-translation-gettext-error.txt	2010-10-06 11:07:49 +0000
@@ -38,38 +38,30 @@
   >>> user_browser.getControl(
   ...     name='msgset_150_es_translation_1_radiobutton').value = [
   ...         'msgset_150_es_translation_1_new']
-  >>> user_browser.getControl(name='msgset_150_es_translation_1_new').value = (
+  >>> user_browser.getControl(
+  ...     name='msgset_150_es_translation_1_new').value = (
   ...     u'Found %s invalid files')
   >>> user_browser.getControl(name='submit_translations').click()
 
 Because of the error, we're still in on the same page.
 
   >>> print user_browser.url
-  http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/+translate?start=20
+  http://.../hoary/+source/evo.../+pots/evo...-2.2/es/+translate?start=20
 
 And we can see the error.
 
   >>> for tag in find_tags_by_class(user_browser.contents, 'error'):
-  ...     print tag
-  <div class="error message">There is an error in a translation you provided.
-    Please correct it before continuing.</div>
-  <tr class="error translation">
-    <th colspan="3">
-      <strong>Error in Translation:</strong>
-    </th>
-    <td>
-    </td><td>
-      <div>
-        format specifications in ... and 'msgstr[0]' for argument 1 are
-        not the sameformat specifications in ... and 'msgstr[1]' for
-        argument 1 are not the same
-      </div>
-    </td>
-  </tr>
+  ...     print extract_text(tag)
+  There is an error in a translation you provided.
+  Please correct it before continuing.
+  Error in Translation:
+  format specifications in 'msgid...' and 'msgstr[0]' for argument 1 are
+  not the sameformat specifications in 'msgid...' and 'msgstr[1]' for
+  argument 1 are not the same
 
 The translation form got an error with the translations we wanted to store,
-and thus we still have that text as part of translations input, otherwise, they
-will be empty waiting for new suggestions/translations.
+and thus we still have that text as part of translations input, otherwise,
+they will be empty waiting for new suggestions/translations.
 
   >>> user_browser.getControl(name='msgset_150_es_translation_0_new').value
   'Found %s invalid file'

=== modified file 'lib/lp/translations/utilities/sanitize.py'
--- lib/lp/translations/utilities/sanitize.py	2010-08-16 09:01:35 +0000
+++ lib/lp/translations/utilities/sanitize.py	2010-10-06 11:07:49 +0000
@@ -181,6 +181,8 @@
 
     # Expected plural forms should all exist and empty translations should
     # be normalized to None.
+    if pluralforms is None:
+        pluralforms = 2
     for pluralform in range(pluralforms):
         if pluralform not in sanitized_translations:
             sanitized_translations[pluralform] = None

=== modified file 'lib/lp/translations/utilities/tests/test_file_importer.py'
--- lib/lp/translations/utilities/tests/test_file_importer.py	2010-09-30 17:57:01 +0000
+++ lib/lp/translations/utilities/tests/test_file_importer.py	2010-10-06 11:07:49 +0000
@@ -5,13 +5,24 @@
 
 __metaclass__ = type
 
+from textwrap import dedent
+
+import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.testing import ZopelessDatabaseLayer
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing import (
+    LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakelibrarian import FakeLibrarian
+from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.translationfileformat import (
+    TranslationFileFormat,
+    )
 from lp.translations.interfaces.translationgroup import TranslationPermission
 from lp.translations.interfaces.translationimporter import (
     OutdatedTranslationError,
@@ -23,6 +34,7 @@
 from lp.translations.utilities.translation_common_format import (
     TranslationMessageData)
 from lp.translations.utilities.translation_import import (
+    importers,
     FileImporter,
     POFileImporter,
     POTFileImporter,
@@ -367,11 +379,11 @@
     def test_FileImporter_importFile_ok(self):
         # Test correct import operation for both
         # exported and upstream files.
-        importers = (
+        used_importers = (
             self._createImporterForExportedEntries(),
             self._createImporterForUpstreamEntries(),
             )
-        for (pot_importer, po_importer) in importers:
+        for (pot_importer, po_importer) in used_importers:
             # Run the import and see if PotMsgSet and TranslationMessage
             # entries are correctly created in the DB.
             errors, warnings = pot_importer.importFile()
@@ -562,3 +574,139 @@
         old_raw_header = pofile.header
         importer = POFileImporter(queue_entry, GettextPOImporter(), None)
         self.assertEqual(old_raw_header, pofile.header)
+
+
+class FileImporterSharingTest(TestCaseWithFactory):
+    """Class test for the sharing operation of the FileImporter base class."""
+    layer = LaunchpadZopelessLayer
+
+    UPSTREAM = 0
+    UBUNTU = 1
+
+    POFILE = dedent("""\
+        msgid ""
+        msgstr ""
+        "PO-Revision-Date: 2005-05-03 20:41+0100\\n"
+        "Last-Translator: FULL NAME <EMAIL@ADDRESS>\\n"
+        "Content-Type: text/plain; charset=UTF-8\\n"
+        "X-Launchpad-Export-Date: 2009-05-14 08:54+0000\\n"
+
+        msgid "Thank You"
+        msgstr "Translation"
+        """)
+
+    def setUp(self):
+        super(FileImporterSharingTest, self).setUp()
+        # Create the upstream series and template with a translator.
+        self.language = self.factory.makeLanguage()
+        self.translator = self.factory.makeTranslator(self.language.code)
+        self.upstream_productseries = self.factory.makeProductSeries()
+        self.upstream_productseries.product.translationgroup = (
+            self.translator.translationgroup)
+        self.upstream_productseries.product.translationpermission = (
+                TranslationPermission.RESTRICTED)
+        self.upstream_template = self.factory.makePOTemplate(
+                productseries=self.upstream_productseries)
+
+    def _makeImportEntry(self, side, from_upstream=False, uploader=None,
+                         no_upstream=False):
+        if side == self.UPSTREAM:
+            potemplate = self.upstream_template
+        else:
+            # Create a template in a source package. 
+            ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+            distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+            ubuntu.translation_focus = distroseries
+            sourcepackagename = self.factory.makeSourcePackageName()
+            potemplate = self.factory.makePOTemplate(
+                distroseries=distroseries,
+                sourcepackagename=sourcepackagename,
+                name=self.upstream_template.name)
+            if not no_upstream:
+                # Link the source package to the upstream series to
+                # enable sharing.
+                self.factory.makeSourcePackagePublishingHistory(
+                    sourcepackagename=sourcepackagename,
+                    distroseries=distroseries)
+                sourcepackage = distroseries.getSourcePackage(
+                    sourcepackagename)
+                sourcepackage.setPackaging(
+                    self.upstream_productseries, self.factory.makePerson())
+        pofile = self.factory.makePOFile(
+            self.language.code, potemplate=potemplate, create_sharing=True)
+        entry = self.factory.makeTranslationImportQueueEntry(
+            potemplate=potemplate, from_upstream=from_upstream,
+            uploader=uploader, content=self.POFILE)
+        entry.potemplate = potemplate
+        entry.pofile = pofile
+        # The uploaded file is only created in the librarian by a commit. 
+        transaction.commit()
+        return entry
+
+    def test_translator_permissions(self):
+        # Sanity check that the translator has the right permissions but
+        # others don't.
+        pofile = self.factory.makePOFile(
+            self.language.code, potemplate=self.upstream_template)
+        self.assertFalse(
+            pofile.canEditTranslations(self.factory.makePerson()))
+        self.assertTrue(
+            pofile.canEditTranslations(self.translator.translator))
+
+    def test_makeImportEntry_templates_are_sharing(self):
+        # Sharing between upstream and Ubuntu was set up correctly.
+        entry = self._makeImportEntry(self.UBUNTU)
+        subset = getUtility(IPOTemplateSet).getSharingSubset(
+                distribution=entry.distroseries.distribution,
+                sourcepackagename=entry.sourcepackagename)
+        self.assertContentEqual(
+            [entry.potemplate, self.upstream_template],
+            list(subset.getSharingPOTemplates(entry.potemplate.name)))
+
+    def test_share_with_other_side_upstream(self):
+        # An upstream queue entry will be shared with ubuntu.
+        entry = self._makeImportEntry(self.UPSTREAM)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertTrue(
+            importer.share_with_other_side,
+            "Upstream import should share with Ubuntu.")
+
+    def test_share_with_other_side_ubuntu(self):
+        # An ubuntu queue entry will not be shared with upstream.
+        entry = self._makeImportEntry(self.UBUNTU)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertFalse(
+            importer.share_with_other_side,
+            "Ubuntu import should not share with upstream.")
+
+    def test_share_with_other_side_ubuntu_no_upstream(self):
+        # An ubuntu queue entry cannot share with a non-existent upstream.
+        entry = self._makeImportEntry(self.UBUNTU, no_upstream=True)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertFalse(
+            importer.share_with_other_side,
+            "Ubuntu import should not share with upstream.")
+
+    def test_share_with_other_side_ubuntu_from_package(self):
+        # An ubuntu queue entry that is imported from an upstream package
+        # will be shared with upstream.
+        entry = self._makeImportEntry(self.UBUNTU, from_upstream=True)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertTrue(
+            importer.share_with_other_side,
+            "Ubuntu import should share with upstream.")
+
+    def test_share_with_other_side_ubuntu_uploader_upstream_translator(self):
+        # If the uploader in ubuntu has rights on upstream as well, the
+        # translations are shared.
+        entry = self._makeImportEntry(
+            self.UBUNTU, uploader=self.translator.translator)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertTrue(
+            importer.share_with_other_side,
+            "Ubuntu import should share with upstream.")

=== modified file 'lib/lp/translations/utilities/tests/test_sanitize.py'
--- lib/lp/translations/utilities/tests/test_sanitize.py	2010-08-23 08:41:03 +0000
+++ lib/lp/translations/utilities/tests/test_sanitize.py	2010-10-06 11:07:49 +0000
@@ -257,6 +257,20 @@
             expected_sanitized,
             sanitize_translations_from_webui(self.english, translations, 3))
 
+    def test_sanitize_translations_pluralforms_none(self):
+        # Some languages don't provide a plural form, so 2 is assumed.
+        translations = {
+            0: u'Plural form 0  ',
+            }
+        expected_sanitized = {
+            0: u'Plural form 0',
+            1: None,
+            }
+        self.assertEqual(
+            expected_sanitized,
+            sanitize_translations_from_webui(
+                self.english, translations, None))
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/utilities/translation_import.py	2010-10-06 11:07:49 +0000
@@ -31,6 +31,11 @@
     PersonCreationRationale,
     )
 from lp.services.propertycache import cachedproperty
+from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.side import (
+    ITranslationSideTraitsSet,
+    TranslationSide,
+    )
 from lp.translations.interfaces.translationexporter import (
     ITranslationExporter,
     )
@@ -45,15 +50,25 @@
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus,
     )
-from lp.translations.interfaces.translationmessage import TranslationConflict
+from lp.translations.interfaces.translationmessage import (
+    RosettaTranslationOrigin,
+    TranslationConflict,
+    TranslationValidationStatus,
+    )
 from lp.translations.interfaces.translations import TranslationConstants
 from lp.translations.utilities.gettext_po_importer import GettextPOImporter
 from lp.translations.utilities.kde_po_importer import KdePOImporter
 from lp.translations.utilities.mozilla_xpi_importer import MozillaXpiImporter
+from lp.translations.utilities.sanitize import (
+    sanitize_translations_from_import,
+    )
 from lp.translations.utilities.translation_common_format import (
     TranslationMessageData,
     )
-from lp.translations.utilities.validate import GettextValidationError
+from lp.translations.utilities.validate import (
+    GettextValidationError,
+    validate_translation,
+    )
 
 
 importers = {
@@ -445,7 +460,89 @@
                 context=message.context))
         return potmsgset
 
-    def storeTranslationsInDatabase(self, message, potmsgset):
+    @cachedproperty
+    def share_with_other_side(self):
+        """Returns True if translations should be shared with the other side.
+        """
+        traits = getUtility(
+            ITranslationSideTraitsSet).getForTemplate(self.potemplate)
+        if traits.side == TranslationSide.UPSTREAM:
+            return True
+        # Check from_upstream.
+        if self.translation_import_queue_entry.from_upstream:
+            return True
+        # Find the sharing POFile and check permissions.
+        productseries = self.potemplate.distroseries.getSourcePackage(
+            self.potemplate.sourcepackagename).productseries
+        if productseries is None:
+            return False
+        upstream_template = getUtility(IPOTemplateSet).getSubset(
+            productseries=productseries).getPOTemplateByName(
+                self.potemplate.name)
+        upstream_pofile = upstream_template.getPOFileByLang(
+            self.pofile.language.code)
+        if upstream_pofile is not None:
+            uploader_person = self.translation_import_queue_entry.importer
+            if upstream_pofile.canEditTranslations(uploader_person):
+                return True
+        # Deny the rest.
+        return False
+
+    @cachedproperty
+    def translations_are_msgids(self):
+        """Are these English strings instead of translations?
+
+        If this template uses symbolic message ids, the English POFile
+        will contain the English original texts that correspond to the 
+        symbols."""
+        return (
+            self.importer.uses_source_string_msgids and
+            self.pofile.language.code == 'en')
+
+    def _storeCredits(self, potmsgset, credits):
+        """Store credits but only from upstream."""
+        if not self.translation_import_queue_entry.from_upstream:
+            return None
+        return potmsgset.setCurrentTranslation(
+            self.pofile, self.last_translator, credits,
+            RosettaTranslationOrigin.SCM, self.share_with_other_side)
+
+    def _validateMessage(self, potmsgset, message,
+                         translations, message_data):
+        """Validate the message and report success or failure."""
+        try:
+            validate_translation(
+                potmsgset.singular_text, potmsgset.plural_text,
+                translations, potmsgset.flags)
+        except GettextValidationError, e:
+            self._addUpdateError(message_data, potmsgset, unicode(e))
+            message.validation_status = (
+                TranslationValidationStatus.UNKNOWNERROR)
+            return False
+
+        message.validation_status = TranslationValidationStatus.OK
+        return True
+
+    def _approveMessage(self, potmsgset, message, message_data):
+        """Try to approve the message, return None on TranslationConflict."""
+        try:
+            message.approve(
+                self.pofile, self.last_translator,
+                self.share_with_other_side, self.lock_timestamp)
+        except TranslationConflict:
+            self._addConflictError(message_data, potmsgset)
+            if self.logger is not None:
+                self.logger.info(
+                    "Conflicting updates on message %d." % potmsgset.id)
+            # The message remains a suggestion.
+            return None
+
+        if self.translations_are_msgids:
+            potmsgset.clearCachedSingularText()
+
+        return message
+
+    def storeTranslationsInDatabase(self, message_data, potmsgset):
         """Try to store translations in the database.
 
         Perform check if a PO file is available and if the message has any
@@ -453,7 +550,8 @@
         is added to the list in self.errors but the translations are stored
         anyway, marked as having an error.
 
-        :param message: The message for which translations will be stored.
+        :param message_data: The message data for which translations will be
+            stored.
         :param potmsgset: The POTMsgSet that this message belongs to.
 
         :return: The updated translation_message entry or None, if no storing
@@ -464,54 +562,39 @@
             # store English strings in an IPOFile.
             return None
 
-        if (not message.translations or
-            set(message.translations) == set([u'']) or
-            set(message.translations) == set([None])):
+        no_translations = (
+            message_data.translations is None or
+            not any(message_data.translations))
+        if no_translations:
             # We don't have anything to import.
             return None
 
-        try:
-            # Do the actual import.
-            translation_message = potmsgset.updateTranslation(
-                self.pofile, self.last_translator, message.translations,
-                self.translation_import_queue_entry.from_upstream,
-                self.lock_timestamp, force_edition_rights=self.is_editor)
-        except TranslationConflict:
-            self._addConflictError(message, potmsgset)
-            if self.logger is not None:
-                self.logger.info(
-                    "Conflicting updates on message %d." % potmsgset.id)
-            return None
-        except GettextValidationError, e:
-            # We got an error, so we submit the translation again but
-            # this time asking to store it as a translation with
-            # errors.
-
-            # Add the pomsgset to the list of pomsgsets with errors.
-            self._addUpdateError(message, potmsgset, unicode(e))
-
-            try:
-                translation_message = potmsgset.updateTranslation(
-                    self.pofile, self.last_translator, message.translations,
-                    self.translation_import_queue_entry.from_upstream,
-                    self.lock_timestamp, ignore_errors=True,
-                    force_edition_rights=self.is_editor)
-            except TranslationConflict:
-                # A conflict on top of a validation error?  Give up.
-                # This message is cursed.
-                if self.logger is not None:
-                    self.logger.info(
-                        "Conflicting updates; ignoring invalid message %d." %
-                            potmsgset.id)
-                return None
-
-        just_replaced_msgid = (
-            self.importer.uses_source_string_msgids and
-            self.pofile.language.code == 'en')
-        if just_replaced_msgid:
-            potmsgset.clearCachedSingularText()
-
-        return translation_message
+        sanitized_translations = sanitize_translations_from_import(
+            potmsgset.singular_text, message_data.translations,
+            self.pofile.language.pluralforms)
+
+        if potmsgset.is_translation_credit:
+            # Translation credits cannot be added as suggestions.
+            return self._storeCredits(potmsgset, sanitized_translations)
+
+        # The message is first stored as a suggestion and only made
+        # current if it validates.
+        new_message = potmsgset.submitSuggestion(
+            self.pofile, self.last_translator, sanitized_translations)
+
+        validation_ok = self._validateMessage(
+            potmsgset, new_message, sanitized_translations, message_data)
+        if validation_ok and self.is_editor:
+            return self._approveMessage(potmsgset, new_message, message_data)
+
+        if self.translation_import_queue_entry.from_upstream:
+            # XXX: henninge 2010-09-21: Mixed models!
+            # This is mimicking the old behavior to still mark these messages
+            # as "imported". Will have to be removed when
+            # getPOTMsgSetsWithErrors is updated to the new model.
+            new_message.makeCurrentUpstream(True)
+
+        return new_message
 
     def importMessage(self, message):
         """Import a single message.