launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/bug-196679-language-packs-value-error into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #196679 in Launchpad itself: "ValueError on +language-packs"
  https://bugs.launchpad.net/launchpad/+bug/196679

For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-196679-language-packs-value-error/+merge/60487

= Summary =

The cause of the OOPS described in the bug is pretty clear once you find
it. ;-) The base and the proposed language pack can be identical as well
as the delta and the proposed. Trying to remove the same value twice
from a list triggers this error. 


== Proposed fix ==

Make DistroSeriesLanguagePackView.unused_language_pack react gracefully
to identical language packs. If the pack is in the list, remove it.
That's all.

There is also a chance that unused_language_pack would fail if the
number of language packs exeeded 15. That is quite likely and no fixed
maximum number of language packs can be found to provide a sensible
value to shortlist. So shortlist needs to be removed.

== Pre-implementation notes ==

I talked to danilo about this and he agreed to my suggested fix.

== Tests ==

There was no view test for this view, so I added the module. Also, there
was no factory method for lanaguage packs, so I added that, too.

bin/test -vvcm lp.translations.browser.tests.test_distroseries_views

== Demo and Q/A ==

Go to https://translations.launchpad.dev/ubuntu/hoary/+language-packs
and select the same pack for base and proposed. After clicking "Change
Settings" this package will be displayed as both the active base pack
and the pack that is being tested. Note that it does not appear in the
drop-down box for the proposed pack anymore. I filed bug 780435 about
that.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/distroseries.py
  lib/lp/testing/factory.py
  lib/lp/translations/browser/tests/test_distroseries_views.py
-- 
https://code.launchpad.net/~henninge/launchpad/bug-196679-language-packs-value-error/+merge/60487
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-196679-language-packs-value-error into lp:launchpad.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-05-10 04:03:26 +0000
+++ lib/lp/testing/factory.py	2011-05-10 11:03:34 +0000
@@ -287,7 +287,11 @@
     time_counter,
     with_celebrity_logged_in,
     )
-from lp.translations.enums import RosettaImportStatus
+from lp.translations.enums import (
+    LanguagePackType,
+    RosettaImportStatus,
+    )
+from lp.translations.interfaces.languagepack import ILanguagePackSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.side import TranslationSide
 from lp.translations.interfaces.translationfileformat import (
@@ -1250,8 +1254,8 @@
         # Only branches related to products have related series branches.
         if with_series_branches and naked_product is not None:
             series_branch_info = []
+
             # Add some product series
-
             def makeSeriesBranch(name, is_private=False):
                 branch = self.makeBranch(
                     name=name,
@@ -2255,6 +2259,15 @@
             language_code, name, pluralforms=pluralforms,
             pluralexpression=plural_expression)
 
+    def makeLanguagePack(self, distroseries=None, languagepack_type=None):
+        """Create a language pack."""
+        if distroseries is None:
+            distroseries = self.makeUbuntuDistroSeries()
+        if languagepack_type is None:
+            languagepack_type = LanguagePackType.FULL
+        return getUtility(ILanguagePackSet).addLanguagePack(
+            distroseries, self.makeLibraryFileAlias(), languagepack_type)
+
     def makeLibraryFileAlias(self, filename=None, content=None,
                              content_type='text/plain', restricted=False,
                              expires=None):

=== modified file 'lib/lp/translations/browser/distroseries.py'
--- lib/lp/translations/browser/distroseries.py	2010-10-12 07:16:50 +0000
+++ lib/lp/translations/browser/distroseries.py	2011-05-10 11:03:34 +0000
@@ -16,7 +16,6 @@
 
 from zope.component import getUtility
 
-from canonical.launchpad import helpers
 from canonical.launchpad.webapp import action
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.launchpadform import LaunchpadEditFormView
@@ -115,13 +114,13 @@
 
     @cachedproperty
     def unused_language_packs(self):
-        unused_language_packs = helpers.shortlist(self.context.language_packs)
+        unused_language_packs = list(self.context.language_packs)
 
-        if self.context.language_pack_base is not None:
+        if self.context.language_pack_base in unused_language_packs:
             unused_language_packs.remove(self.context.language_pack_base)
-        if self.context.language_pack_delta is not None:
+        if self.context.language_pack_delta in unused_language_packs:
             unused_language_packs.remove(self.context.language_pack_delta)
-        if self.context.language_pack_proposed is not None:
+        if self.context.language_pack_proposed in unused_language_packs:
             unused_language_packs.remove(self.context.language_pack_proposed)
 
         return unused_language_packs

=== added file 'lib/lp/translations/browser/tests/test_distroseries_views.py'
--- lib/lp/translations/browser/tests/test_distroseries_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/tests/test_distroseries_views.py	2011-05-10 11:03:34 +0000
@@ -0,0 +1,59 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the translations views on a distroseries."""
+
+__metaclass__ = type
+
+
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+from lp.translations.enums import LanguagePackType
+
+
+class TestLanguagePacksView(TestCaseWithFactory):
+    """Test language packs view."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_unused_language_packs_many_language_packs(self):
+        distroseries = self.factory.makeUbuntuDistroSeries()
+        # This is one more than the default for shortlist.
+        number_of_language_packs = 16
+        for i in range(number_of_language_packs):
+            self.factory.makeLanguagePack(distroseries)
+
+        view = create_initialized_view(
+            distroseries, '+language-packs', rootsite='translations')
+        # This should not trigger a shortlist warning.
+        self.assertEqual(
+            number_of_language_packs, len(view.unused_language_packs))
+
+    def test_unused_language_packs_identical_base_proposed_pack(self):
+        distroseries = self.factory.makeUbuntuDistroSeries()
+        pack = self.factory.makeLanguagePack(distroseries)
+        with person_logged_in(distroseries.distribution.owner):
+            distroseries.language_pack_base = pack
+            distroseries.language_pack_proposed = pack
+
+        view = create_initialized_view(
+            distroseries, '+language-packs', rootsite='translations')
+        self.assertEqual(0, len(view.unused_language_packs))
+
+    def test_unused_language_packs_identical_delta_proposed_pack(self):
+        distroseries = self.factory.makeUbuntuDistroSeries()
+        with person_logged_in(distroseries.distribution.owner):
+            distroseries.language_pack_base = self.factory.makeLanguagePack(
+                distroseries)
+            delta_pack = self.factory.makeLanguagePack(
+                distroseries, LanguagePackType.DELTA)
+            distroseries.language_pack_delta = delta_pack
+            distroseries.language_pack_proposed = delta_pack
+
+        view = create_initialized_view(
+            distroseries, '+language-packs', rootsite='translations')
+        self.assertEqual(0, len(view.unused_language_packs))