> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 12.03.2010 16:42, Adi Roiban schrieb:
> Hi Adi,
> thanks for tackling this. There seem to have been some misunderstanding
> about how this option worked, judging by the comments in the bug.
>
> I am a little concerned about the behaviour. Shouldn't the radio button
> in front of the input box need to be activated (see below)? Also, I am
> not sure if really _all_ old suggestions should be brought up again. But
> that may be OK.
This implementation should reset the current translation as there is no other workaround for the following use case:
https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/14
In an ideal world, we would have a "is_reviewed" field for each translation message, a dedicated "reset translation" checkbox and a way to see all previously entered suggestions.
I was thinking there is no need to force a user to add an empty text and mark it as a suggestion and it should be enough to just tick the "needs review" checkbox.
> I also have some other suggestions. Please look at them and we can talk
> again tomorrow.
Many thanks for your review.
Please see my inline comments.
[snip]
> > +Reseting current translation
>
> Resetting ...
Fixed.
> > +-----------------------------
> > +
> > +The current translation can be resetted by submiting an empty translation
> or
> > +a translation similar to the current one, while forcing the suggestions.
>
> The current translation can be reset by submitting an empty translation
> or a translation similar to the current one while forcing suggestions.
Fixed.
> > +
> > + >>> # We need to force timestamp update, since suggestions are
> submitted
> > + >>> # to fast
>
> Please don't put comments in doc tests. Whatever goes into the comment
> can go into the normal text. It's "too fast", btw. ;)
Done.
> > + >>> year_tick = 2100
> > + >>> def submit_translation(translation, force_suggestion=False):
> > + ... global year_tick
> > + ... translationmessage = TranslationMessage.get(1)
>
> I find this very obscure. I think it would be clearer to pass
> translationmessage as a parameter into the function. If you want "any"
> TranslationMessage object, use the factory outside and pass the object
> into the function.
I was getting the translation message with id 1 to simplify the form submission, since
the form names depend on the id, and also since translation permission were already there.
I have rewritten the code to take a translation message as parameter and only use generated data.
[snip]
> > +
> > +Same happens if we force the submission of a suggestion similar with the
>
> ... similar to the ...
Done.
[snip]
> > + if (force_suggestion and
> > + self.user_is_official_translator and
> > + (self._areSuggestionsEmpty(translations) or
> > + (translationmessage is not None and
> > + self._areSuggestionsIdenticalToTranslations(translations,
> > + translationmessage.translations)))):
>
> This is not very easy to read, right? I suggest you create a few
> intermediate boolean values so that they all fit on one line behind the
> "if".
Indeed :)
I have restructured the conditions.
[snip]
> ""):
> > + empty = False
> > + break
> > + return empty
>
> You can save a few lines of code here, if you like:
> for index in suggestions:
> if (suggestions[index] is not None and suggestions[index] != ""):
> return False
> return True
True.
Done
[snip]
> ... using the plural form ...
Done.
> > + as index, while translations are represented as a list ordered by
> > + plural form.
>
> .. the plural form ...
Done.
> > + """
> > +
> > + identical = True
> > + for index in suggestions:
> > + if (suggestions[index] != translations[index]):
> > + identical = False
> > + break
> > + return identical
>
> Same here, you can save code lines. ;-)
Done.
[snip]
> > + def resetCurrentTranslation(pofile, lock_timestamp):
> > + """Reset the currently used translation.
> > +
> > + Reset means setting the "is_current"
>
> Oops, something missing from the sentence here. ;)
Right. Fixed.
[snip]
> > + def _isNoTranslationConflict(self, message, lock_timestamp):
> > + """Checks if there is a translation conflict for message.
>
> ... the message.
Done.
[snip]
> > + elif self._isNoTranslationConflict(current, lock_timestamp):
> > + current.reviewer = reviewer
> > + current.date_reviewed = lock_timestamp
>
> Coding guide lines require this after an elif:
>
> ... else:
> # Comment.
> pass
I have avoided the elif statement.
[snip]
> > + # imported.
> > + if (current.potemplate is not None and
> > + current.is_imported is False):
>
> I think these fit on one line. Also, is that "is False" really on
> purpose? Can is_imported be None?
> if current.potemplate is not None and not current.is_imported:
Done.
[snip]
> >
> > When translation is restricted to a particular set of users
>
> I know this is not your code, but can you please change this to:
>
> "When translating is restricted ..."
Done
[snip]
> > +If the 'Someone should review this translation' checkbox is used without
> > +adding any new suggestions or adding an empty string, the current
> translation
> > +will be resetted causing all suggestions entered for this message to be
> listed
>
> "reset"
Done.
>
> The verb "to set" is irregular, like "to put". Its tenses are "set, set,
> set", just like "put, put, put".
>
>
> ;-) (Forgive me, please.)
Don't worry :)
Many thanks for the friendly reminder.
[snip]
> > +'Someone should review this translation' can be used to reset the
> translation,
> > +and after clicking 'Save & Continue', all translations entered for this
> > +message will be listed as suggestions.
>
> Hm, do I not have to click the radio button to activate the input field?
> I think that would be much clearer. Some javascript should be added to
> move the radio button.
Since we want to reset the current translation, I don't think that this should require entering any text / new suggestion and a simple tick to "Some should review this translation" is enough.
> > +
> > + >>> admin_browser.getControl(
> > + ... 'Someone should review this translation').selected = True
> > + >>> admin_browser.getControl('Save & Continue').click()
> > + >>> print extract_text(find_tag_by_id(admin_browser.contents,
> > + ... 'messages_to_translate'))
> > + 1. English: test man page
> > + Current Spanish: (no translation yet)
> > + Suggestions:
> > + New test translation Suggested by Foo Bar ...
> > + blah, blah, blah Suggested by Carlos ...
> > + lalalala Suggested by Carlos ...
> > + just a translation Suggested by Sample Person ...
>
> So, this brings up all old translations? Is that really what we want?
Maybe the UI is a bit confusion and we should have a dedicated checkbox for reseting the translations.
I think that this is what a reset should do for a potmsgset.
> > + Dismiss all suggestions above.
> > + New translation:
> > + Someone should review this translation
> > + Located in ...
> >
> > === modified file 'lib/lp/translations/tests/test_potmsgset.py'
> > --- lib/lp/translations/tests/test_potmsgset.py 2010-03-06 06:21:04
> +0000
> > +++ lib/lp/translations/tests/test_potmsgset.py 2010-03-15 13:43:34
> +0000
> > @@ -903,6 +903,89 @@
> > include_dismissed=False, include_unreviewed=False))
> >
> >
> > +class TestPOTMsgSetResetTranslation(TestCaseWithFactory):
> > + """Test retrieval and dismissal of translation suggestions."""
>
> Copied doc string?
Yes.
Fixed.
[snip]
> > + self.assertFalse(translation.is_current)
> > + self.assertTrue(translation.is_imported)
> > + self.assertTrue(translation.potemplate is not None)
>
> self.assertFalse(translation.potemplate is None)
Done.
[snip]
Here is the latest diff.
Please let me know how can I improve it.
=== modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
--- lib/lp/translations/browser/tests/translationmessage-views.txt 2010-03-13 02:27:53 +0000
+++ lib/lp/translations/browser/tests/translationmessage-views.txt 2010-03-18 20:04:30 +0000
@@ -691,18 +691,29 @@
True
-Reseting current translation
+Resetting current translation
-----------------------------
-The current translation can be resetted by submiting an empty translation or
-a translation similar to the current one, while forcing the suggestions.
-
- >>> # We need to force timestamp update, since suggestions are submitted
- >>> # to fast
+The current translation can be reset by submitting an empty translation
+or a translation similar to the current one while forcing suggestions.
+
+We need to force timestamp update, since suggestions are submitted too fast.
+
+ >>> from lp.translations.interfaces.translationgroup import (
+ ... TranslationPermission)
+ >>> pofile = factory.makePOFile('es')
+ >>> potemplate = pofile.potemplate
+ >>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
+ >>> translationmessage = factory.makeTranslationMessage(
+ ... potmsgset=potmsgset, pofile=pofile,
+ ... translations = [u"A shared translation"])
+ >>> translationmessage.setPOFile(pofile)
+ >>> product = potemplate.productseries.product
+ >>> product.translationpermission = TranslationPermission.CLOSED
>>> year_tick = 2100
- >>> def submit_translation(translation, force_suggestion=False):
+ >>> def submit_translation(
+ ... translationmessage, translation, force_suggestion=False):
... global year_tick
- ... translationmessage = TranslationMessage.get(1)
... pofile = translationmessage.pofile
... potmsgset = translationmessage.potmsgset
... server_url = '/'.join(
@@ -710,36 +721,32 @@
... now = (str(year_tick) +
... datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
... year_tick += 1
+ ... msgset_stem = 'msgset_' + str(potmsgset.id)
+ ... msgset_stem_lang = msgset_stem + '_' + pofile.language.code
... form = {
... 'lock_timestamp': now,
... 'alt': None,
- ... 'msgset_1': None,
- ... 'msgset_1_es_translation_0_radiobutton':
- ... 'msgset_1_es_translation_0_new',
- ... 'msgset_1_es_translation_0_new': translation,
+ ... msgset_stem : None,
+ ... msgset_stem_lang + '_translation_0_radiobutton':
+ ... msgset_stem_lang + '_translation_0_new',
+ ... msgset_stem_lang + '_translation_0_new': translation,
... 'submit_translations': 'Save & Continue'}
... if force_suggestion:
- ... form['msgset_1_es_needsreview'] = 'force_suggestion'
+ ... form[msgset_stem_lang + '_needsreview'] = 'force_suggestion'
... tm_view = create_view(
... translationmessage, "+translate", form=form,
... layer=TranslationsLayer, server_url=server_url)
... tm_view.request.method = 'POST'
... tm_view.initialize()
- >>> def print_current_translation():
- ... translationmessage = TranslationMessage.get(1)
- ... pofile = translationmessage.pofile
- ... potmsgset = translationmessage.potmsgset
+ >>> def print_current_translation(potmsgset, pofile):
... current_translation = potmsgset.getCurrentTranslationMessage(
... pofile.potemplate, pofile.language)
... if current_translation is not None:
... print current_translation.translations
- >>> def print_local_suggestions():
+ >>> def print_local_suggestions(potmsgset, pofile):
... import operator
- ... translationmessage = TranslationMessage.get(1)
- ... pofile = translationmessage.pofile
- ... potmsgset = translationmessage.potmsgset
... local = sorted(
... potmsgset.getLocalTranslationMessages(
... pofile.potemplate, pofile.language),
@@ -748,49 +755,51 @@
... for suggestion in local:
... print suggestion.translations
-Then we will add a new translation, forcing it as a suggestion.
+We will make an initial translation.
- >>> submit_translation(u'A new suggestion', force_suggestion=True)
- >>> print_local_suggestions()
- [u'A new suggestion']
- [u'Foos']
- >>> print_current_translation()
+ >>> submit_translation(
+ ... translationmessage, u'Foo')
+ >>> print_local_suggestions(potmsgset, pofile)
+ >>> print_current_translation(potmsgset, pofile)
[u'Foo']
Forcing suggestions while submitting an empty translation will reset the
translation for this message and all previously entered translations will be
listed as suggestions.
- >>> submit_translation(u'', force_suggestion=True)
- >>> print_local_suggestions()
- [u'A new suggestion']
- [u'Foos']
+ >>> submit_translation(translationmessage, u'', force_suggestion=True)
+ >>> print_local_suggestions(potmsgset, pofile)
[u'Foo']
- >>> print_current_translation()
+ [u'A shared translation']
+ >>> print_current_translation(potmsgset, pofile)
-Same happens if we force the submission of a suggestion similar with the
+Same happens if we force the submission of a suggestion similar to the
current translation.
- >>> submit_translation(u'New current translation')
- >>> print_current_translation()
- [u'New current translation']
- >>> submit_translation(u'New current translation', force_suggestion=True)
- >>> print_local_suggestions()
- [u'New current translation']
- [u'A new suggestion']
- [u'Foos']
+ >>> submit_translation(translationmessage, u'New current translation')
+ >>> print_local_suggestions(potmsgset, pofile)
+ >>> print_current_translation(potmsgset, pofile)
+ [u'New current translation']
+ >>> submit_translation(
+ ... translationmessage, u'New current translation',
+ ... force_suggestion=True)
+ >>> print_current_translation(potmsgset, pofile)
+ >>> print_local_suggestions(potmsgset, pofile)
+ [u'New current translation']
[u'Foo']
- >>> print_current_translation()
+ [u'A shared translation']
Only users with edit rights are allowed to reset a translation.
- >>> submit_translation(u'New current translation')
- >>> print_current_translation()
+ >>> submit_translation(translationmessage, u'New current translation')
+ >>> print_current_translation(potmsgset, pofile)
[u'New current translation']
- >>> print_local_suggestions()
+ >>> print_local_suggestions(potmsgset, pofile)
>>> login('')
- >>> submit_translation(u'New current translation', force_suggestion=True)
+ >>> submit_translation(
+ ... translationmessage, u'New current translation',
+ ... force_suggestion=True)
>>> login('')
- >>> print_local_suggestions()
- >>> print_current_translation()
+ >>> print_local_suggestions(potmsgset, pofile)
+ >>> print_current_translation(potmsgset, pofile)
[u'New current translation']
=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py 2010-03-13 02:27:53 +0000
+++ lib/lp/translations/browser/translationmessage.py 2010-03-18 17:27:39 +0000
@@ -428,12 +428,14 @@
# If suggestions were forced and user has the rights to do it,
# reset the current translation.
+ identical_suggestions = (translationmessage is not None
+ and self._areSuggestionsIdenticalToTranslations(
+ translations, translationmessage.translations))
+ empty_suggestions = self._areSuggestionsEmpty(translations)
+ resetConditions = empty_suggestions or identical_suggestions
if (force_suggestion and
self.user_is_official_translator and
- (self._areSuggestionsEmpty(translations) or
- (translationmessage is not None and
- self._areSuggestionsIdenticalToTranslations(translations,
- translationmessage.translations)))):
+ resetConditions):
potmsgset.resetCurrentTranslation(
self.pofile, self.lock_timestamp)
@@ -453,29 +455,23 @@
def _areSuggestionsEmpty(self, suggestions):
"""Return true if all suggestions are empty strings or None."""
-
- empty = True
for index in suggestions:
if (suggestions[index] is not None and suggestions[index] != ""):
- empty = False
- break
- return empty
+ return False
+ return True
def _areSuggestionsIdenticalToTranslations(self, suggestions,
translations):
"""Return true if suggestions are identical to translations.
-
- Suggestions are represented as a dictionary using plural form
+
+ Suggestions are represented as a dictionary using the plural form
as index, while translations are represented as a list ordered by
- plural form.
+ the plural form.
"""
-
- identical = True
for index in suggestions:
if (suggestions[index] != translations[index]):
- identical = False
- break
- return identical
+ return False
+ return True
def _prepareView(self, view_class, current_translation_message, error):
"""Collect data and build a TranslationMessageView for display."""
=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py 2010-03-13 02:27:53 +0000
+++ lib/lp/translations/interfaces/potmsgset.py 2010-03-18 17:30:04 +0000
@@ -253,7 +253,8 @@
def resetCurrentTranslation(pofile, lock_timestamp):
"""Reset the currently used translation.
- Reset means setting the "is_current"
+ This will set the "is_current" attribute to False and if the message
+ is diverge, will try to converge it.
:param pofile: a `POFile` to dismiss suggestions from.
:param lock_timestamp: the timestamp when we checked the values we
want to update.
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2010-03-13 02:27:53 +0000
+++ lib/lp/translations/model/potmsgset.py 2010-03-18 20:40:55 +0000
@@ -884,7 +884,7 @@
return matching_message
def _isNoTranslationConflict(self, message, lock_timestamp):
- """Checks if there is a translation conflict for message.
+ """Checks if there is a translation conflict for the message.
If a translation conflict is detected, TranslationConflict is raised.
"""
@@ -910,7 +910,9 @@
# Create an empty translation message.
current = self.updateTranslation(
pofile, reviewer, [], False, lock_timestamp)
- elif self._isNoTranslationConflict(current, lock_timestamp):
+ else:
+ # Check for translation conflicts and update review fields.
+ self._isNoTranslationConflict(current, lock_timestamp)
current.reviewer = reviewer
current.date_reviewed = lock_timestamp
@@ -930,8 +932,7 @@
current.is_current = False
# Converge the current translation only if it is diverged and not
# imported.
- if (current.potemplate is not None and
- current.is_imported is False):
+ if current.potemplate is not None and not current.is_imported:
current.potemplate = None
pofile.date_changed = UTC_NOW
=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt 2010-03-13 02:27:53 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt 2010-03-18 17:36:47 +0000
@@ -4,7 +4,7 @@
Forcing suggestions
-------------------
-When translation is restricted to a particular set of users
+When translating is restricted to a particular set of users
(like Evolution product is restricted for Spanish), other users
don't see a checkbox named 'Someone should review this translation'.
@@ -64,7 +64,7 @@
If the 'Someone should review this translation' checkbox is used without
adding any new suggestions or adding an empty string, the current translation
-will be resetted causing all suggestions entered for this message to be listed
+will be reset causing all suggestions entered for this message to be listed
again.
A new translation is entered and checked that it was saved as the current
=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py 2010-03-15 13:40:18 +0000
+++ lib/lp/translations/tests/test_potmsgset.py 2010-03-18 20:13:53 +0000
@@ -904,7 +904,7 @@
class TestPOTMsgSetResetTranslation(TestCaseWithFactory):
- """Test retrieval and dismissal of translation suggestions."""
+ """Test resetting the current translation."""
layer = ZopelessDatabaseLayer
@@ -931,7 +931,7 @@
self.pofile = self.factory.makePOFile('eo', self.potemplate)
def test_resetCurrentTranslation_shared(self):
- # Reseting a shared current translation will change iscurrent=False
+ # Resetting a shared current translation will change iscurrent=False
# and there will be no other current translations for this POTMsgSet.
translation = self.factory.makeTranslationMessage(
@@ -949,7 +949,7 @@
self.assertTrue(translation.potemplate is None)
def test_resetCurrentTranslation_diverged_not_imported(self):
- # Reseting a diverged current translation that was not imported, will
+ # Resettting a diverged current translation that was not imported, will
# change is_current to False and will make it shared.
translation = self.factory.makeTranslationMessage(
@@ -967,7 +967,7 @@
self.assertTrue(translation.potemplate is None)
def test_resetCurrentTranslation_diverged_imported(self):
- # Reseting a diverged current translation that was imported in
+ # Resetting a diverged current translation that was imported in
# Launchpad will change iscurrent to False but the translation
# message will be still diverged.
@@ -983,7 +983,7 @@
self.assertTrue(current is None)
self.assertFalse(translation.is_current)
self.assertTrue(translation.is_imported)
- self.assertTrue(translation.potemplate is not None)
+ self.assertFalse(translation.potemplate is None)
class TestPOTMsgSetCornerCases(TestCaseWithFactory):