-----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.

I also have some other suggestions. Please look at them and we can talk
again tomorrow.

 review needs-fixing

Please find my comments below.

Cheers,
Hennning

> === modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
> --- lib/lp/translations/browser/tests/translationmessage-views.txt	2009-09-11 22:24:51 +0000
> +++ lib/lp/translations/browser/tests/translationmessage-views.txt	2010-03-15 13:43:34 +0000
> @@ -689,3 +689,108 @@
>      False
>      >>> subview.shared_translationmessage == translationmessage
>      True
> +
> +
> +Reseting current translation

Resetting ...

> +-----------------------------
> +
> +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.


> +
> +    >>> # 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. ;)

> +    >>> 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.

> +    ...     pofile = translationmessage.pofile
> +    ...     potmsgset = translationmessage.potmsgset
> +    ...     server_url = '/'.join(
> +    ...         [canonical_url(translationmessage), '+translate'])
> +    ...     now = (str(year_tick) +
> +    ...             datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
> +    ...     year_tick += 1
> +    ...     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,
> +    ...         'submit_translations': 'Save & Continue'}
> +    ...     if force_suggestion:
> +    ...         form['msgset_1_es_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
> +    ...     current_translation = potmsgset.getCurrentTranslationMessage(
> +    ...         pofile.potemplate, pofile.language)
> +    ...     if current_translation is not None:
> +    ...         print current_translation.translations
> +
> +    >>> def print_local_suggestions():
> +    ...     import operator
> +    ...     translationmessage = TranslationMessage.get(1)
> +    ...     pofile = translationmessage.pofile
> +    ...     potmsgset = translationmessage.potmsgset
> +    ...     local = sorted(
> +    ...         potmsgset.getLocalTranslationMessages(
> +    ...             pofile.potemplate, pofile.language),
> +    ...         key=operator.attrgetter("date_created"),
> +    ...         reverse=True)
> +    ...     for suggestion in local:
> +    ...         print suggestion.translations
> +
> +Then we will add a new translation, forcing it as a suggestion.
> +
> +    >>> submit_translation(u'A new suggestion', force_suggestion=True)
> +    >>> print_local_suggestions()
> +    [u'A new suggestion']
> +    [u'Foos']
> +    >>> print_current_translation()
> +    [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']
> +    [u'Foo']
> +    >>> print_current_translation()
> +
> +Same happens if we force the submission of a suggestion similar with the

... 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']
> +    [u'Foo']
> +    >>> print_current_translation()
> +
> +Only users with edit rights are allowed to reset a translation.
> +
> +    >>> submit_translation(u'New current translation')
> +    >>> print_current_translation()
> +    [u'New current translation']
> +    >>> print_local_suggestions()
> +    >>> login('<email address hidden>')
> +    >>> submit_translation(u'New current translation', force_suggestion=True)
> +    >>> login('<email address hidden>')
> +    >>> print_local_suggestions()
> +    >>> print_current_translation()
> +    [u'New current translation']
> 
> === modified file 'lib/lp/translations/browser/translationmessage.py'
> --- lib/lp/translations/browser/translationmessage.py	2010-01-27 07:38:02 +0000
> +++ lib/lp/translations/browser/translationmessage.py	2010-03-15 13:43:34 +0000
> @@ -425,6 +425,18 @@
>                  is_imported=False, lock_timestamp=self.lock_timestamp,
>                  force_suggestion=force_suggestion,
>                  force_diverged=force_diverge)
> +
> +            # If suggestions were forced and user has the rights to do it,
> +            # reset the current translation.
> +            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".

> +                potmsgset.resetCurrentTranslation(
> +                    self.pofile, self.lock_timestamp)
> +
>          except TranslationConflict:
>              return (
>                  u'Somebody else changed this translation since you started.'
> @@ -439,6 +451,32 @@
>              self._observeTranslationUpdate(potmsgset)
>              return None
>  
> +    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

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


> +
> +    def _areSuggestionsIdenticalToTranslations(self, suggestions,
> +                                               translations):
> +        """Return true if suggestions are identical to translations.
> +        
> +        Suggestions are represented as a dictionary using plural form

... 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

Same here, you can save code lines. ;-)

> +
>      def _prepareView(self, view_class, current_translation_message, error):
>          """Collect data and build a TranslationMessageView for display."""
>          # XXX: kiko 2006-09-27:
> 
> === modified file 'lib/lp/translations/interfaces/potmsgset.py'
> --- lib/lp/translations/interfaces/potmsgset.py	2010-03-05 15:31:22 +0000
> +++ lib/lp/translations/interfaces/potmsgset.py	2010-03-15 13:43:34 +0000
> @@ -250,6 +250,17 @@
>          If a translation conflict is detected, TranslationConflict is raised.
>          """
>  
> +    def resetCurrentTranslation(pofile, lock_timestamp):
> +        """Reset the currently used translation.
> +
> +        Reset means setting the "is_current"

Oops, something missing from the sentence here. ;)

> +        :param pofile: a `POFile` to dismiss suggestions from.
> +        :param lock_timestamp: the timestamp when we checked the values we
> +            want to update.
> +
> +        If a translation conflict is detected, TranslationConflict is raised.
> +        """
> +
>      def applySanityFixes(unicode_text):
>          """Return 'unicode_text' or None after doing some sanitization.
>  
> 
> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py	2010-03-05 15:31:22 +0000
> +++ lib/lp/translations/model/potmsgset.py	2010-03-15 13:43:34 +0000
> @@ -883,6 +883,24 @@
>          matching_message.sync()
>          return matching_message
>  
> +    def _isNoTranslationConflict(self, message, lock_timestamp):
> +        """Checks if there is a translation conflict for message.

... the message.

> +
> +        If a translation conflict is detected, TranslationConflict is raised.
> +        """
> +        if message.date_reviewed is not None:
> +            use_date = message.date_reviewed
> +        else:
> +            use_date = message.date_created
> +        if use_date >= lock_timestamp:
> +            raise TranslationConflict(
> +                'While you were reviewing these suggestions, somebody '
> +                'else changed the actual translation. This is not an '
> +                'error but you might want to re-review the strings '
> +                'concerned.')
> +        else:
> +            return True
> +
>      def dismissAllSuggestions(self, pofile, reviewer, lock_timestamp):
>          """See `IPOTMsgSet`."""
>          assert(lock_timestamp is not None)
> @@ -892,20 +910,30 @@
>              # Create an empty translation message.
>              current = self.updateTranslation(
>                  pofile, reviewer, [], False, lock_timestamp)
> -        else:
> -            if current.date_reviewed is not None:
> -                use_date = current.date_reviewed
> -            else:
> -                use_date = current.date_created
> -            if use_date >= lock_timestamp:
> -                raise TranslationConflict(
> -                    'While you were reviewing these suggestions, somebody '
> -                    'else changed the actual translation. This is not an '
> -                    'error but you might want to re-review the strings '
> -                    'concerned.')
> -            else:
> -                current.reviewer = reviewer
> -                current.date_reviewed = lock_timestamp
> +        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

> +
> +    def resetCurrentTranslation(self, pofile, lock_timestamp):
> +        """See `IPOTMsgSet`."""
> +
> +        assert(lock_timestamp is not None)
> +
> +        current = self.getCurrentTranslationMessage(
> +            pofile.potemplate, pofile.language)
> +
> +        # Reset only if we don't have a conflict and 
> +        # if no new suggestions were provided or
> +        # the new suggestions are empty.
> +        if (current is not None and
> +            self._isNoTranslationConflict(current, lock_timestamp)):
> +            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):

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:

> +                current.potemplate = None
> +            pofile.date_changed = UTC_NOW
>  
>      def applySanityFixes(self, text):
>          """See `IPOTMsgSet`."""
> 
> === modified file 'lib/lp/translations/model/translationmessage.py'
> --- lib/lp/translations/model/translationmessage.py	2009-10-28 13:08:05 +0000
> +++ lib/lp/translations/model/translationmessage.py	2010-03-15 13:43:34 +0000
> @@ -478,12 +478,12 @@
>      implements(ITranslationMessageSet)
>  
>      def getByID(self, ID):
> -        """See `ILanguageSet`."""
> +        """See `ITranslationMessageSet`."""

Thank you for spotting these.

>          try:
>              return TranslationMessage.get(ID)
>          except SQLObjectNotFound:
>              return None
>  
>      def selectDirect(self, where=None, order_by=None):
> -        """See `ILanguageSet`."""
> +        """See `ITranslationMessageSet`."""
>          return TranslationMessage.select(where, orderBy=order_by)
> 
> === 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	2009-07-01 20:45:39 +0000
> +++ lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt	2010-03-15 13:43:34 +0000
> @@ -1,4 +1,8 @@
> -= Forcing suggestions =
> +Someone should review this translation usage
> +============================================
> +
> +Forcing suggestions
> +-------------------
>  
>  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 ..."

Thanks.

>  (like Evolution product is restricted for Spanish), other users
> @@ -53,3 +57,53 @@
>      >>> print extract_text(find_tag_by_id(user_browser.contents,
>      ...                                   'msgset_130_es_suggestion_701_0'))
>      New suggestion
> +
> +
> +Reseting translations
> +---------------------
> +
> +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"

<EnglishTeacher>
The verb "to set" is irregular, like "to put". Its tenses are "set, set,
set", just like "put, put, put".
</EnglishTeacher>

;-) (Forgive me, please.)

> +again.
> +
> +A new translation is entered and checked that it was saved as the current
> +translation, while no suggestions are displayed.
> +
> +    >>> admin_browser.open(
> +    ...     'http://translations.launchpad.dev/ubuntu/hoary/+source/'
> +    ...     'evolution/+pots/man/es/1/+translate')
> +    >>> inputradio = admin_browser.getControl(
> +    ...    name='msgset_166_es_translation_0_radiobutton')
> +    >>> inputradio.value = [ 'msgset_166_es_translation_0_new']
> +    >>> inputfield = admin_browser.getControl(name='msgset_166_es_translation_0_new')
> +    >>> inputfield.value = 'New test translation'
> +    >>> 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: New test translation Translated and reviewed ...
> +    New translation:
> +    Someone should review this translation
> +    Located in ...
> +
> +'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.

> +
> +    >>> 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?

> +    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?

> +
> +    layer = ZopelessDatabaseLayer
> +
> +    def gen_now(self):
> +        now = datetime.now(pytz.UTC)
> +        while True:
> +            yield now
> +            now += timedelta(milliseconds=1)
> +
> +    def setUp(self):
> +        # Create a product with all the boilerplate objects to be able to
> +        # create TranslationMessage objects.
> +        super(TestPOTMsgSetResetTranslation, self).setUp()
> +        self.now = self.gen_now().next
> +        self.foo = self.factory.makeProduct()
> +        self.foo_main = self.factory.makeProductSeries(
> +            name='main', product=self.foo)
> +        self.foo.official_rosetta = True
> +
> +        self.potemplate = self.factory.makePOTemplate(
> +            productseries=self.foo_main, name="messages")
> +        self.potmsgset = self.factory.makePOTMsgSet(self.potemplate,
> +                                                    sequence=1)
> +        self.pofile = self.factory.makePOFile('eo', self.potemplate)
> +
> +    def test_resetCurrentTranslation_shared(self):
> +        # Reseting a shared current translation will change iscurrent=False
> +        # and there will be no other current translations for this POTMsgSet.
> +
> +        translation = self.factory.makeTranslationMessage(
> +            self.pofile, self.potmsgset, translations=[u'Shared translation'],
> +            reviewer=self.factory.makePerson(),
> +            is_imported=False, force_diverged=False,
> +            date_updated=self.now())
> +
> +        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
> +        current = self.potmsgset.getCurrentTranslationMessage(
> +            self.potemplate, self.pofile.language)
> +        self.assertTrue(current is None)
> +        self.assertFalse(translation.is_current)
> +        self.assertFalse(translation.is_imported)
> +        self.assertTrue(translation.potemplate is None)
> +
> +    def test_resetCurrentTranslation_diverged_not_imported(self):
> +        # Reseting a diverged current translation that was not imported, will
> +        # change is_current to False and will make it shared.
> +
> +        translation = self.factory.makeTranslationMessage(
> +            self.pofile, self.potmsgset, translations=[u'Diverged text'],
> +            reviewer=self.factory.makePerson(),
> +            is_imported=False, force_diverged=True,
> +            date_updated=self.now())
> +
> +        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
> +        current = self.potmsgset.getCurrentTranslationMessage(
> +            self.potemplate, self.pofile.language)
> +        self.assertTrue(current is None)
> +        self.assertFalse(translation.is_current)
> +        self.assertFalse(translation.is_imported)
> +        self.assertTrue(translation.potemplate is None)
> +
> +    def test_resetCurrentTranslation_diverged_imported(self):
> +        # Reseting a diverged current translation that was imported in
> +        # Launchpad will change iscurrent to False but the translation
> +        # message will be still diverged.
> +
> +        translation = self.factory.makeTranslationMessage(
> +            self.pofile, self.potmsgset, translations=[u'Imported diverged'],
> +            reviewer=self.factory.makePerson(),
> +            is_imported=True, force_diverged=True,
> +            date_updated=self.now())
> +
> +        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
> +        current = self.potmsgset.getCurrentTranslationMessage(
> +            self.potemplate, self.pofile.language)
> +        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):
>      """Test corner cases and constraints."""
>  
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkueczEACgkQBT3oW1L17iiXMQCfV86eQEKCTBgmkB3gYcJdC3PN
VvgAnR2XlgTHEXqrZP2wkxbN8qlnPBwq
=qmw9
-----END PGP SIGNATURE-----