Code review comment for lp:~jtv/launchpad/bug-702228

Jeroen--

This looks good. I have couple comments below, but the one on the test case is the only one I feel is important. This is good to land with that change.

> === modified file 'lib/lp/translations/scripts/po_export_queue.py'
> --- lib/lp/translations/scripts/po_export_queue.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/translations/scripts/po_export_queue.py 2011-01-13 18:25:08 +0000
> @@ -250,6 +252,46 @@
> 'request_url': self.request_url,
> }
>
> + def setExportFile(self, exported_file):
> + """Attach an exported file to the result, for upload to the Librarian.
> +
> + After this is set, `upload` will perform the actual upload. The two
> + actions are separated so as to isolate write access to the database.
> +
> + :param exported_file: An `IExportedTranslationFile` containing the
> + exported data.
> + """
> + self.exported_file = exported_file

Given how very little this method is doing, I'm not sure it needs to be a separate method. It's not hurting anything to have it this way, but seems slightly unnecesary.

> === added file 'lib/lp/translations/tests/test_exportresult.py'
> --- lib/lp/translations/tests/test_exportresult.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/tests/test_exportresult.py 2011-01-13 18:25:08 +0000
> @@ -0,0 +1,67 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for `ExportResult`."""
> +
> +__metaclass__ = type
> +
> +import hashlib
> +import os.path
> +from StringIO import StringIO
> +
> +from canonical.librarian.testing.fake import FakeLibrarian
> +from canonical.testing.layers import ZopelessDatabaseLayer
> +from lp.services.log.logger import DevNullLogger
> +from lp.testing import TestCaseWithFactory
> +from lp.translations.scripts.po_export_queue import ExportResult
> +
> +
> +class FakeExportedTranslationFile:
> + """Fake `IExportedTranslationFile` for testing."""
> +
> + def __init__(self, path, content, content_type):
> + self.path = path
> + base, ext = os.path.splitext(path)
> + self.file_extension = ext
> + self.size = len(content)
> + self.content = content
> + self.file = StringIO(content)
> + self.content_type = content_type
> +
> + def read(self, *args, **kwargs):
> + return self.file.read(*args, **kwargs)
> +
> +
> +class TestExportResult(TestCaseWithFactory):
> +
> + layer = ZopelessDatabaseLayer
> +
> + def makeExportResult(self):
> + request = [self.factory.makePOFile()]
> + requester = self.factory.makePerson()
> + logger = DevNullLogger()
> + return ExportResult(requester, request, logger)
> +
> + def makeExportedTranslationFile(self):
> + filename = self.factory.getUniqueString()
> + content = self.factory.getUniqueString()
> + mime_type = "text/plain"
> + return FakeExportedTranslationFile(filename, content, mime_type)
> +
> + def test_upload_exported_file(self):
> + librarian = self.useFixture(FakeLibrarian())
> + export = self.makeExportedTranslationFile()
> + export_result = self.makeExportResult()
> + export_result.setExportFile(export)
> + export_result.upload()
> +
> + self.assertNotEqual(None, export_result.url)

Asserting equalilty or inequality on None feels odd to me. self.assertIsNot(None, export_result.url) would be better.

> + sha1 = hashlib.sha1(export.content).hexdigest()
> + self.assertEqual(sha1, librarian.aliases.values()[0].content.sha1)
> + alias = librarian.findBySHA1(sha1)
> + self.assertEqual(export.path, alias.filename)
> +
> + def test_upload_without_exported_file_does_nothing(self):
> + export_result = self.makeExportResult()
> + export_result.upload()
> + self.assertIs(None, export_result.url)