launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05178
[Merge] lp:~danilo/launchpad/bug-401618 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-401618 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #401618 in Launchpad itself: "Remove [I]POTMsgSet.sequence"
https://bugs.launchpad.net/launchpad/+bug/401618
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-401618/+merge/78273
= Bug 401618 =
This is a bug about remaining use of POTMsgSet.sequence that stopped us from killing this column in the database: nature of POTMsgSet has changed in the meantime so it can participate in multiple different translation templates, and sequence in each of those templates is stored on TranslationTemplateItem.sequence instead of on POTMsgSet directly.
Biggest issue was the fact that we constructed URLs for TranslationMessages using potmsgset.sequence. To aid with that, we instead introduce TranslationMessage.sequence which uses self.browser_pofile when it's defined, because that's modified according to the current view.
As for all the other uses, I replace potmsgset.sequence with potmsgset.getSequence(proper_template).
The tests previously used to pass because they didn't heavily depend on this change (eg. the fact that potmsgset.setSequence also set potmsgset.sequence was enough for them to pass), so I am not introducing any new tests because plenty would start failing if I did this wrong.
== Tests ==
"bin/test -vvm lp.translations" still passes
== Demo and Q/A ==
Browse around +translate and +filter pages on translations.l.n, especially clicking on the "zoom" icon for each of the messages. Also look for translation suggestions saying "used in" and "suggested in" and follow those links to ensure they point to right pages.
= Launchpad lint =
NOTE: I'll fix lint after the review, to simplify the review.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/doc/vpotexport.txt
lib/lp/translations/browser/configure.zcml
lib/lp/translations/interfaces/translationmessage.py
lib/lp/translations/interfaces/potmsgset.py
lib/lp/translations/tests/test_pofile.py
lib/lp/translations/model/pofile.py
lib/lp/translations/doc/pofile.txt
lib/lp/translations/browser/tests/translationmessage-views.txt
lib/lp/translations/model/potmsgset.py
lib/lp/translations/model/translationmessage.py
lib/lp/translations/templates/pofile-filter.pt
./lib/lp/translations/doc/vpotexport.txt
1: narrative uses a moin header.
18: narrative uses a moin header.
./lib/lp/translations/interfaces/potmsgset.py
196: Line exceeds 78 characters.
198: Line exceeds 78 characters.
343: E251 no spaces around keyword / parameter equals
./lib/lp/translations/model/potmsgset.py
19: 'IntCol' imported but unused
./lib/lp/translations/templates/pofile-filter.pt
32: Line has trailing whitespace.
--
https://code.launchpad.net/~danilo/launchpad/bug-401618/+merge/78273
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-401618 into lp:launchpad.
=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml 2011-08-25 19:48:19 +0000
+++ lib/lp/translations/browser/configure.zcml 2011-10-05 15:50:29 +0000
@@ -528,7 +528,7 @@
layer="lp.translations.publisher.TranslationsLayer"/>
<browser:url
for="lp.translations.interfaces.translationmessage.ITranslationMessage"
- path_expression="string:${potmsgset/sequence}"
+ path_expression="string:${sequence}"
attribute_to_parent="browser_pofile"
rootsite="translations"/>
<browser:menus
=== modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
--- lib/lp/translations/browser/tests/translationmessage-views.txt 2011-01-05 19:32:32 +0000
+++ lib/lp/translations/browser/tests/translationmessage-views.txt 2011-10-05 15:50:29 +0000
@@ -99,8 +99,8 @@
We are at the beginning because this subview is being used for the first
item.
- >>> subview.context.potmsgset.sequence == 1
- True
+ >>> subview.context.potmsgset.getSequence(pofile.potemplate)
+ 1
It does not have a plural message
@@ -530,8 +530,6 @@
>>> pofile = factory.makePOFile('sr')
>>> potemplate = pofile.potemplate
>>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
- >>> potmsgset.sequence
- 1
>>> potmsgset.getSequence(potemplate)
1
>>> translationmessage = factory.makeCurrentTranslationMessage(
=== modified file 'lib/lp/translations/doc/pofile.txt'
--- lib/lp/translations/doc/pofile.txt 2011-05-27 19:53:20 +0000
+++ lib/lp/translations/doc/pofile.txt 2011-10-05 15:50:29 +0000
@@ -73,7 +73,7 @@
We need a helper method to better display test results.
- >>> def print_potmsgsets(potmsgsets, pofile=None):
+ >>> def print_potmsgsets(potmsgsets, pofile):
... for potmsgset in potmsgsets:
... singular = plural = None
... translation = ""
@@ -86,13 +86,16 @@
... if len(plural) > 20:
... plural = plural[:17] + "..."
... if pofile is not None:
- ... translation = potmsgset.getCurrentTranslation(
+ ... message = potmsgset.getCurrentTranslation(
... pofile.potemplate, pofile.language,
- ... pofile.potemplate.translation_side).translations[0]
+ ... pofile.potemplate.translation_side)
+ ... if message is not None:
+ ... translation = message.translations[0]
... if len(translation) > 20:
... translation = translation[:17] + "..."
... print "%2d. %-20s %-20s %-20s" % (
- ... potmsgset.sequence, singular, plural, translation)
+ ... potmsgset.getSequence(pofile.potemplate),
+ ... singular, plural, translation)
getFullLanguageCode
@@ -131,7 +134,7 @@
>>> found_potmsgsets.count()
4
- >>> print_potmsgsets(found_potmsgsets)
+ >>> print_potmsgsets(found_potmsgsets, dummy_pofile)
7. contact's header: None
14. The location and ... None
15. %d contact %d contacts
@@ -144,7 +147,7 @@
>>> found_potmsgsets.count()
4
- >>> print_potmsgsets(found_potmsgsets)
+ >>> print_potmsgsets(found_potmsgsets, dummy_pofile)
7. contact's header: None
14. The location and ... None
15. %d contact %d contacts
@@ -157,7 +160,7 @@
>>> found_potmsgsets.count()
2
- >>> print_potmsgsets(found_potmsgsets)
+ >>> print_potmsgsets(found_potmsgsets, dummy_pofile)
15. %d contact %d contacts
16. Opening %d contac... Opening %d contac...
=== modified file 'lib/lp/translations/doc/vpotexport.txt'
--- lib/lp/translations/doc/vpotexport.txt 2009-07-29 18:53:50 +0000
+++ lib/lp/translations/doc/vpotexport.txt 2011-10-05 15:50:29 +0000
@@ -61,7 +61,7 @@
>>> first.sequence
0
- >>> potmsgset.sequence
+ >>> potmsgset.getSequence(first.potemplate)
0
The POT file header is a template of common information that will get filled
=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py 2011-03-07 07:17:59 +0000
+++ lib/lp/translations/interfaces/potmsgset.py 2011-10-05 15:50:29 +0000
@@ -95,8 +95,6 @@
readonly=True,
schema=IPOMsgID)
- sequence = Attribute("The ordering of this set within its file.")
-
commenttext = Attribute("The manual comments this set has.")
filereferences = Attribute("The files where this set appears.")
=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py 2011-02-23 16:22:52 +0000
+++ lib/lp/translations/interfaces/translationmessage.py 2011-10-05 15:50:29 +0000
@@ -113,6 +113,10 @@
title=_("The translation file from where this translation comes"),
readonly=False, required=False, schema=IPOFile)
+ sequence = Int(
+ title=_("Sequence of a message in the PO file browser_pofile."),
+ readonly=True, required=False)
+
potemplate = Object(
title=_("The template this translation is in"),
readonly=False, required=False, schema=IPOTemplate)
=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py 2011-06-22 09:59:13 +0000
+++ lib/lp/translations/model/pofile.py 2011-10-05 15:50:29 +0000
@@ -1118,7 +1118,7 @@
error_message = error['error-message']
errorsdetails = '%s%d. "%s":\n\n%s\n\n' % (
errorsdetails,
- potmsgset.sequence,
+ potmsgset.getSequence(self.potemplate),
error_message,
pomessage)
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2011-07-26 11:53:25 +0000
+++ lib/lp/translations/model/potmsgset.py 2011-10-05 15:50:29 +0000
@@ -139,7 +139,6 @@
notNull=True)
msgid_plural = ForeignKey(foreignKey='POMsgID', dbName='msgid_plural',
notNull=False, default=DEFAULT)
- sequence = IntCol(dbName='sequence')
commenttext = StringCol(dbName='commenttext', notNull=False)
filereferences = StringCol(dbName='filereferences', notNull=False)
sourcecomment = StringCol(dbName='sourcecomment', notNull=False)
@@ -1217,7 +1216,6 @@
def setSequence(self, potemplate, sequence):
"""See `IPOTMsgSet`."""
- self.sequence = sequence
translation_template_item = TranslationTemplateItem.selectOneBy(
potmsgset=self, potemplate=potemplate)
if translation_template_item is not None:
=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py 2011-02-24 14:31:46 +0000
+++ lib/lp/translations/model/translationmessage.py 2011-10-05 15:50:29 +0000
@@ -114,6 +114,14 @@
"""See `ITranslationMessage`."""
self.browser_pofile = pofile
+ @property
+ def sequence(self):
+ if self.browser_pofile:
+ pofile = self.browser_pofile
+ return self.potmsgset.getSequence(pofile.potemplate)
+ else:
+ return 0
+
def markReviewed(self, reviewer, timestamp=None):
"""See `ITranslationMessage`."""
if timestamp is None:
=== modified file 'lib/lp/translations/templates/pofile-filter.pt'
--- lib/lp/translations/templates/pofile-filter.pt 2009-09-14 10:35:23 +0000
+++ lib/lp/translations/templates/pofile-filter.pt 2011-10-05 15:50:29 +0000
@@ -46,10 +46,10 @@
<tal:message repeat="message messages">
<tr>
<td class="englishstring" style="text-align:right;">
- <tal:obsolete condition="not:message/potmsgset/sequence">
+ <tal:obsolete condition="not:message/context/sequence">
~</tal:obsolete>
- <tal:nonobsolete condition="message/potmsgset/sequence">
- <a tal:content="string:${message/potmsgset/sequence}."
+ <tal:nonobsolete condition="message/context/sequence">
+ <a tal:content="string:${message/context/sequence}."
tal:attributes="href message/context/fmt:url" />
</tal:nonobsolete>
</td>
=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py 2011-08-03 11:00:11 +0000
+++ lib/lp/translations/tests/test_pofile.py 2011-10-05 15:50:29 +0000
@@ -1356,32 +1356,6 @@
self.assertEquals(
[self.potmsgset1, self.potmsgset2], potmsgsets)
- def test_findPOTMsgSetsContaining_ordering(self):
- # As per bug 388473 findPOTMsgSetsContaining still used the old
- # potmsgset.sequence for ordering. Check that this is fixed.
- # This test will go away when potmsgset.sequence goes away.
-
- # Give the method something to search for.
- self.factory.makeCurrentTranslationMessage(
- pofile=self.devel_pofile,
- potmsgset=self.potmsgset1,
- translations=["Shared translation"])
- self.factory.makeCurrentTranslationMessage(
- pofile=self.devel_pofile,
- potmsgset=self.potmsgset2,
- translations=["Another shared translation"])
-
- # Mess with potmsgset.sequence.
- removeSecurityProxy(self.potmsgset1).sequence = 2
- removeSecurityProxy(self.potmsgset2).sequence = 1
-
- potmsgsets = list(
- self.devel_pofile.findPOTMsgSetsContaining("translation"))
-
- # Order ignores potmsgset.sequence.
- self.assertEquals(
- [self.potmsgset1, self.potmsgset2], potmsgsets)
-
class TestPOFileSet(TestCaseWithFactory):
"""Test PO file set methods."""
