launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-714521-restore-partial-exports into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #714521 in Launchpad itself: "Partial translation export feature gone"
  https://bugs.launchpad.net/launchpad/+bug/714521

For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-714521-restore-partial-exports/+merge/56713

= Summary =

It took me a little while to realize that this is purely a UI issue. The
checkbox for partial exports needs to be always available for Ubuntu
source packages. The actual export code will DTRT.

== Proposed fix ==

Change the condition in POFileExportView.has_pochanged_option to always
be true for Ubuntu source packages.

== Pre-implementation notes ==

This was neglected when the partial export feature was ported to recife
and even the reviewer missed it at the time. :-P

== Implementation details ==

This would have been a quick fix of well under 100 lines of diff if the
test case had not been using setUp to create test data. I had to
refactor the whole case in order to be able to create a pofile on an
Ubuntu source package. Watch http://brainupdate.wordpress.com/ for a
post on the subject.

== Tests ==

bin/test -vvm lp.translations.browser.tests.test_poexportrequest_views \
  -t TextPOExportView

== Demo and Q/A ==

Go to the pofile export page of any Ubuntu source package. Even though
virtually none have set up upstream translations properly you should
see the option "Only strings that differ ..." on all of them.

Here is an example of a package *with* upstream:
https://translations.launchpad.net/ubuntu/natty/+source/gimp/+pots/gimp20/de/+export

Here is an example of a packge *without* upstream:
https://translations.launchpad.net/ubuntu/natty/+source/gimp/+pots/gimp20/de/+export

Exports on productseries that are not packages in Ubuntu will not show
the option:
https://translations.launchpad.net/mailclipper/trunk/+pots/testcase/de/+export

But packaged product series do:
https://translations.launchpad.net/synaptic/main/+pots/synaptic/de/+export

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/tests/test_poexportrequest_views.py
  lib/lp/translations/browser/pofile.py

./lib/lp/translations/browser/pofile.py
     788: E301 expected 1 blank line, found 2
     904: E301 expected 1 blank line, found 2
-- 
https://code.launchpad.net/~henninge/launchpad/devel-714521-restore-partial-exports/+merge/56713
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-714521-restore-partial-exports into lp:launchpad.

=== modified file 'lib/lp/translations/browser/pofile.py'
--- lib/lp/translations/browser/pofile.py	2011-03-30 11:17:35 +0000
+++ lib/lp/translations/browser/pofile.py	2011-04-07 08:18:36 +0000
@@ -1034,6 +1034,11 @@
 
     @property
     def has_pochanged_option(self):
+        is_ubuntu = (
+            self.context.potemplate.translation_side ==
+                TranslationSide.UBUNTU)
+        if is_ubuntu:
+            return True
         other_side_pofile = self.context.getOtherSidePOFile()
         return other_side_pofile is not None
 

=== modified file 'lib/lp/translations/browser/tests/test_poexportrequest_views.py'
--- lib/lp/translations/browser/tests/test_poexportrequest_views.py	2011-01-04 15:40:56 +0000
+++ lib/lp/translations/browser/tests/test_poexportrequest_views.py	2011-04-07 08:18:36 +0000
@@ -14,6 +14,7 @@
     )
 from lp.translations.browser.pofile import POExportView
 from lp.translations.browser.potemplate import POTemplateExportView
+from lp.translations.interfaces.side import TranslationSide
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
 from lp.translations.model.poexportrequest import POExportRequest
@@ -123,56 +124,52 @@
 
     layer = DatabaseFunctionalLayer
 
-    def setUp(self):
-        super(TestPOExportView, self).setUp()
-        self.pofile = self.factory.makePOFile()
-        self.potemplate = self.pofile.potemplate
+    def _createView(self, pofile, form=None):
         # All exports can be requested by an unprivileged user.
-        self.translator = self.factory.makePerson()
-
-    def _createView(self, form=None):
-        login_person(self.translator)
+        login_person(self.factory.makePerson())
         if form is None:
             request = LaunchpadTestRequest()
         else:
             request = LaunchpadTestRequest(method='POST', form=form)
-        view = POExportView(self.pofile, request)
+        view = POExportView(pofile, request)
         view.initialize()
         return view
 
     def test_request_format_po(self):
         # It is possible to request an export in the PO format.
-        self._createView({'format': 'PO'})
+        pofile = self.factory.makePOFile()
+        self._createView(pofile, {'format': 'PO'})
 
         self.assertContentEqual(
-            [(self.potemplate, self.pofile, TranslationFileFormat.PO)],
+            [(pofile.potemplate, pofile, TranslationFileFormat.PO)],
             get_poexportrequests(include_format=True))
 
     def test_request_format_mo(self):
         # It is possible to request an export in the MO format.
-        self._createView({'format': 'MO'})
+        pofile = self.factory.makePOFile()
+        self._createView(pofile, {'format': 'MO'})
 
         self.assertContentEqual(
-            [(self.potemplate, self.pofile, TranslationFileFormat.MO)],
+            [(pofile.potemplate, pofile, TranslationFileFormat.MO)],
             get_poexportrequests(include_format=True))
 
-    def _makeUbuntuTemplateAndPOFile(self):
+    def _makeUbuntuTemplateAndPOFile(self, upstream_pofile=None):
         """Create a sharing template in Ubuntu with a POFile to go with it."""
-        upstream_series = self.potemplate.productseries
-        template_name = self.potemplate.name
-        language = self.pofile.language
+        if upstream_pofile is None:
+            upstream_pofile = self.factory.makePOFile()
+        upstream_potemplate = upstream_pofile.potemplate
+        upstream_series = upstream_potemplate.productseries
+        template_name = upstream_potemplate.name
+        language = upstream_pofile.language
 
-        distroseries = self.factory.makeUbuntuDistroSeries()
-        naked_distribution = removeSecurityProxy(distroseries.distribution)
-        naked_distribution.translation_focus = distroseries
-        sourcepackagename = self.factory.makeSourcePackageName()
-        sourcepackage = self.factory.makeSourcePackage(
-            sourcepackagename, distroseries)
-        sourcepackage.setPackaging(upstream_series, self.factory.makePerson())
+        packaging = self.factory.makePackagingLink(
+            productseries=upstream_series, in_ubuntu=True)
+        naked_distribution = removeSecurityProxy(
+            packaging.distroseries.distribution)
+        naked_distribution.translation_focus = packaging.distroseries
 
         potemplate = self.factory.makePOTemplate(
-            distroseries=distroseries, sourcepackagename=sourcepackagename,
-            name=template_name)
+            sourcepackage=packaging.sourcepackage, name=template_name)
 
         # The sharing POFile is created automatically when the template is
         # created because self.pofile already exists.
@@ -184,7 +181,8 @@
         # corresponding (same language, sharing template) on the Ubuntu side.
         ubuntu_potemplate, ubuntu_pofile = self._makeUbuntuTemplateAndPOFile()
 
-        self._createView({'format': 'PO', 'pochanged': 'POCHANGED'})
+        self._createView(
+            ubuntu_pofile, {'format': 'PO', 'pochanged': 'POCHANGED'})
 
         expected = (
             ubuntu_potemplate,
@@ -197,42 +195,64 @@
     def test_request_partial_po_ubuntu(self):
         # Partial po exports are requested by an extra check box.
         # For an Ubuntu package, the export is requested on the file itself.
-        ubuntu_potemplate, ubuntu_pofile = self._makeUbuntuTemplateAndPOFile()
-        self.potemplate = ubuntu_potemplate
-        self.pofile = ubuntu_pofile
+        upstream_pofile = self.factory.makePOFile()
+        ubuntu_potemplate, ubuntu_pofile = self._makeUbuntuTemplateAndPOFile(
+            upstream_pofile)
 
-        self._createView({'format': 'PO', 'pochanged': 'POCHANGED'})
+        self._createView(
+            upstream_pofile, {'format': 'PO', 'pochanged': 'POCHANGED'})
 
         self.assertContentEqual(
-            [(self.potemplate, self.pofile, TranslationFileFormat.POCHANGED)],
+            [(ubuntu_potemplate, ubuntu_pofile,
+                TranslationFileFormat.POCHANGED)],
             get_poexportrequests(include_format=True))
 
     def test_request_partial_po_no_link(self):
         # Partial po exports are requested by an extra check box.
         # Without a packaging link, no request is created.
-        self._createView({'format': 'PO', 'pochanged': 'POCHANGED'})
+        pofile = self.factory.makePOFile()
+        self._createView(pofile, {'format': 'PO', 'pochanged': 'POCHANGED'})
 
         self.assertContentEqual([], get_poexportrequests())
 
     def test_request_partial_mo(self):
         # With the MO format, the partial export check box is ignored.
-        self._createView({'format': 'MO', 'pochanged': 'POCHANGED'})
+        pofile = self.factory.makePOFile()
+        self._createView(pofile, {'format': 'MO', 'pochanged': 'POCHANGED'})
 
         self.assertContentEqual(
-            [(self.potemplate, self.pofile, TranslationFileFormat.MO)],
+            [(pofile.potemplate, pofile, TranslationFileFormat.MO)],
             get_poexportrequests(include_format=True))
 
-    def test_pochanged_option_available(self):
-        # The option for partial exports is only available if a POFile can
-        # be found on the other side.
-        self._makeUbuntuTemplateAndPOFile()
-        view = self._createView()
-
-        self.assertTrue(view.has_pochanged_option)
-
-    def test_pochanged_option_not_available(self):
-        # The option for partial exports is not available if no POFile can
-        # be found on the other side.
-        view = self._createView()
+    def test_pochanged_option_available_ubuntu(self):
+        # The option for partial exports is always available on a source
+        # package.
+        ubuntu_potemplate, ubuntu_pofile = self._makeUbuntuTemplateAndPOFile()
+        view = self._createView(ubuntu_pofile)
+
+        self.assertTrue(view.has_pochanged_option)
+
+    def test_pochanged_option_available_ubuntu_no_upstream(self):
+        # The option for partial exports is always available on a source
+        # package, even if there is no upstream pofile.
+        ubuntu_pofile = self.factory.makePOFile(side=TranslationSide.UBUNTU)
+        view = self._createView(ubuntu_pofile)
+
+        self.assertTrue(view.has_pochanged_option)
+
+    def test_pochanged_option_available_upstream(self):
+        # The option for partial exports is only available upstream if a
+        # POFile can be found in Ubuntu.
+        upstream_pofile = self.factory.makePOFile()
+        self._makeUbuntuTemplateAndPOFile(upstream_pofile)
+        view = self._createView(upstream_pofile)
+
+        self.assertTrue(view.has_pochanged_option)
+
+    def test_pochanged_option_not_available_upstream(self):
+        # The option for partial exports is not available upstream if no
+        # POFile can be found in Ubuntu.
+        pofile = self.factory.makePOFile()
+        view = self._createView(pofile)
 
         self.assertFalse(view.has_pochanged_option)