> > === 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 \\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" > > + """) > > + > > + def setUp(self): > > + super(FileImporterSharingTest, self).setUp() > > + # Create the upstream series and template with a translator. > > + self.translator = self.factory.makeTranslator(self.LANGCODE) > > + 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 _makeEntry(self, side, from_upstream=False, uploader=None): > > How about "_makeImportEntry" just so it's slightly clearer? Well, as it is only used locally, I did not think it needs much explanation. Changed. > > > + 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. > > > + sourcepackage = > distroseries.getSourcePackage(sourcepackagename) > > + sourcepackage.setPackaging( > > + self.upstream_productseries, self.factory.makePerson()) > > + pofile = self.factory.makePOFile( > > + self.LANGCODE, 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 > > + transaction.commit() > > It'd be nice to add a comment about why is a commit necessary. Added. > > > + return entry > > + > > + def test_translator_persmissions(self): > > s/perSmissions/permissions/ > > > + # Sanity check that the translator has the right permissions but > > + # others don't. > > + pofile = self.factory.makePOFile( > > + self.LANGCODE, potemplate=self.upstream_template) > > + self.assertFalse( > > + pofile.canEditTranslations(self.factory.makePerson())) > > + self.assertTrue( > > + pofile.canEditTranslations(self.translator.translator)) > > + > > Do note that Jeroen has started on cleaning up these checks as well. > It'd be good to point him at this code as well since you are probably > going to land this first :) He already saw this and talked to me about it. So he should be aware of it. > > > + 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 basically a test for _makeEntry? Perhaps it should be marked > as such (i.e. test_makeEntry_templates_are_sharing for the name). It is > good that you want to have at least a basic sanity check for your helper > method. Yes, that's kind of what it is. When I first wrote the tests some things did not work right, so I added these. When everything was working, I just did not remove them because, as you said, they are still useful. > > > + def test_share_with_other_side_upstream(self): > > + # An upstream queue entry will be shared with ubuntu. > > + entry = self._makeEntry(self.UPSTREAM) > > + importer = POFileImporter( > > + entry, importers[TranslationFileFormat.PO], None) > > + self.assertTrue( > > + importer.share_with_other_side, > > + "Upstream import should share with Ubuntu.") > > That's simple enough :) Yeah ... > > > + def test_share_with_other_side_ubuntu(self): > > + # An ubuntu queue entry will not be shared with upstream. > > + entry = self._makeEntry(self.UBUNTU) > > + importer = POFileImporter( > > + entry, importers[TranslationFileFormat.PO], None) > > + self.assertFalse( > > + importer.share_with_other_side, > > + "Ubuntu import should not share with upstream.") > > As is this and the following tests: > > > + def test_share_with_other_side_ubuntu_from_package(self): > ... > > + def > test_share_with_other_side_ubuntu_uploader_upstream_translator(self): > ... > > 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 ... ;-/ > > > === modified file 'lib/lp/translations/utilities/translation_import.py' > > ... > > > > importers = { > > @@ -445,7 +460,86 @@ > > 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 > > This doesn't seem to be tested (i.e. when there's no link between a > sourcepackage and productseries, we should return True if it's for > UPSTREAM import, and return False for UBUNTU import: I know it doesn't > make much sense to test for all of this with the code as-is, but as soon > as we change the code, it will make more sense). Well, all UPSTREAM imports return True anyway, that's the first thing this method checks. So, only UBUNTU imports will get this far and then False is returned, like you described. But you are right about this not being tested, so I added a test. > > > + upstream_template = getUtility(IPOTemplateSet).getSubset( > > + productseries=productseries).getPOTemplateByName( > > + self.potemplate.name) > > + upstream_pofile = upstream_template.getPOFileByLang( > > + self.pofile.language.code) > > When we move this code for use on views as well, we'll have to take more > care with query count (I know it's a cached property, but at least these > two could be a single query on the POTemplateSet). Not for this branch, > though. Phew! > > > + @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. ;-) > > > + 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) > > 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. > > I thought setCurrentTranslation didn't check for identical message > first, but now I see that it does (I was going to warn about using it > blindly). Na, don't worry. I knew that ... > > > + 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 > > This looks very similar to what potmsgset._validate_translations does. > It'd be nice if we actually shared this code as well. Yes, that may be true. I don't know yet what I would do with _addUpdateError, though. I regard potmsgset._v_t as part of updateTranslations (its only call site AFAIK) which will eventually be removed. > > > + 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() > > Should this not be in the "finally:" block? No, AFAIUI this should only happen if the translation is actually approved. [...] > > + message_data.translations == [] or > > + message_data.translations is None or > > + not any(message_data.translations)) > > 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. > > > + if no_translations: > > # We don't have anything to import. > > return None > > [...] > > + 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) > > Note that this might actually introduce a bug where we ignore bugs in > software. For example, a translation credits message might have a > c-format flag on, and a translation might have a percentage sign in, and > we wouldn't bork out. Still, exported PO files would end up containing > such a message with c-format flag and msgfmt couldn't compile them. > > 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? > > >+ > > + # 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) > > getPOTMsgSetsWithErrors is not really part of our POFile:+filters, now > that I think about it. So, I've added another card for that. Thank you. And thank you for your review! I hope my reply finds your approval ;-) Cheers, Henning