Merge lp:~abentley/launchpad/fix-dummy-translations into lp:launchpad

[Aaron Bentley]

Fix credit display for translators who cannot edit.

= Summary =
Fix bug #702477: Translation credits on new POFiles show the dummy string for the other side

== Proposed fix ==
Use the same translation credit display for all viewers, regardless of whether
they can edit the POFile.

== Pre-implementation notes ==
Discussed with henninge

== Implementation details ==
This bug turns out to be simpler than it appeared. We had two ways of
displaying translation credits; one for translators with write access, and one
for translators without write access. The first was suitable for both, and the
second was unsuitable for anyone. So this branch uses the first way for both
cases.

Also, I had to change self.factory.makeTranslator because it got Unauthorized
trying to set .translations_relicensing_agreement. Presumably, it has only
been used with Zopeless tests so far. But LaunchpadFunctionalLayer is a better
simulation of actual user interaction, so I used that.

== Tests ==
bin/test -t test_unwritable_translation_credits test_pofile_view

== Demo and Q/A ==
View a POFile that you do not have write access to. If you are not already a
translator, accept the agreement. The translation credits should be shown
correctly, and you should not be able to edit any translations.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/tests/test_pofile_view.py
  lib/lp/testing/factory.py
  lib/lp/translations/templates/currenttranslationmessage-translate-one.pt

[Benji York (benji)]

This branch looks good.

A thought regarding this bit of code:

    insecure_tx_person = removeSecurityProxy(tx_person)
    insecure_tx_person.translations_relicensing_agreement = license

You might want to consider avoiding storing the reference to the
unproxied object in the hope that it'll avoid someone changing the code
in an insecure way in the future. Like so:

    removeSecurityProxy(tx_person).translations_relicensing_agreement = (
        license)

Although, your variable naming might be just as effective.

review: Approve (code)

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-04-11 05:00 PM, Benji York wrote:
> Review: Approve code
> This branch looks good.
>
> A thought regarding this bit of code:
>
> insecure_tx_person = removeSecurityProxy(tx_person)
> insecure_tx_person.translations_relicensing_agreement = license
>
> You might want to consider avoiding storing the reference to the
> unproxied object in the hope that it'll avoid someone changing the code
> in an insecure way in the future. Like so:
>
> removeSecurityProxy(tx_person).translations_relicensing_agreement = (
> license)

I've certainly used this approach in the past, but in this case, it
produces a multiple-line statement. I believe that impairs readability,
and for me that trumps the slight chance of avoiding security errors.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2kZiEACgkQ0F+nu1YWqI0AsQCfVIv3RRNBP4dFaQcLxOVuoSuw
p6IAni0bzcXvfyKE1ax74zrB4unB89Do
=XHNq
-----END PGP SIGNATURE-----

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-04-07 20:37:22 +0000
+++ lib/lp/testing/factory.py	2011-04-12 18:56:43 +0000
@@ -800,7 +800,8 @@
         if person is None:
             person = self.makePerson()
         tx_person = ITranslationsPerson(person)
-        tx_person.translations_relicensing_agreement = license
+        insecure_tx_person = removeSecurityProxy(tx_person)
+        insecure_tx_person.translations_relicensing_agreement = license
         return getUtility(ITranslatorSet).new(group, language, person)
 
     def makeMilestone(self, product=None, distribution=None,

=== modified file 'lib/lp/translations/browser/tests/test_pofile_view.py'
--- lib/lp/translations/browser/tests/test_pofile_view.py	2010-12-09 19:49:36 +0000
+++ lib/lp/translations/browser/tests/test_pofile_view.py	2011-04-12 18:56:43 +0000
@@ -11,16 +11,22 @@
 import pytz
 
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import ZopelessDatabaseLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.app.errors import UnexpectedFormData
 from lp.testing import (
+    BrowserTestCase,
     login,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.translations.browser.pofile import (
     POFileBaseView,
     POFileTranslateView,
     )
+from lp.translations.enums import TranslationPermission
 
 
 class TestPOFileBaseViewFiltering(TestCaseWithFactory):
@@ -435,3 +441,37 @@
                                            DocumentationScenarioMixin):
     layer = ZopelessDatabaseLayer
     view_class = POFileTranslateView
+
+
+class TestBrowser(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_unwritable_translation_credits(self):
+        """Text of credits should be sane for non-editors."""
+        # Make the user a translator so they can see translations.
+        self.factory.makeTranslator(person=self.user)
+        pofile = self.factory.makePOFile()
+        # Restrict translations so that the translator cannot change it.
+        product = pofile.potemplate.productseries.product
+        with person_logged_in(product.owner):
+            product.translationpermission = TranslationPermission.CLOSED
+        # Add credits so that they show in the UI
+        credits = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, singular='translator-credits')
+        browser = self.getViewBrowser(pofile)
+        self.assertNotIn('This is a dummy translation', browser.contents)
+        self.assertIn('(no translation yet)', browser.contents)
+
+    def test_anonymous_translation_credits(self):
+        """Credits should be hidden for non-logged-in users."""
+        pofile = self.factory.makePOFile()
+        # Restrict translations so that the translator cannot change it.
+        product = pofile.potemplate.productseries.product
+        # Add credits so that they show in the UI
+        credits = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, singular='translator-credits')
+        browser = self.getViewBrowser(pofile, no_login=True)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            'To prevent privacy issues, this translation is not available to'
+            ' anonymous users', browser.contents)

=== modified file 'lib/lp/translations/templates/currenttranslationmessage-translate-one.pt'
--- lib/lp/translations/templates/currenttranslationmessage-translate-one.pt	2010-12-18 09:10:37 +0000
+++ lib/lp/translations/templates/currenttranslationmessage-translate-one.pt	2011-04-12 18:56:43 +0000
@@ -274,7 +274,7 @@
       <tal:locked condition="context/potmsgset/is_translation_credit">
         <td class="icon left right" />
         <td>
-          <tal:form-writeable condition="view/form_is_writeable">
+          <tal:not-hidden condition="not:view/message_must_be_hidden">
             <tal:automatic condition="view/translation_credits">
               <div tal:condition="not: view/user_is_official_translator"
                    tal:attributes="
@@ -305,22 +305,17 @@
                 (no translation yet)
               </label>
             </tal:no-automatic>
-          </tal:form-writeable>
-          <tal:form-not-writeable condition="not:view/form_is_writeable">
-            <div tal:condition="not: view/user_is_official_translator"
-                 tal:attributes="
+          </tal:not-hidden>
+          <tal:hidden condition="view/message_must_be_hidden">
+            <tal:comment condition="nothing">
+              Display the "privacy issues" message generated by the view.
+            </tal:comment>
+            <div tal:attributes="
                    id string:${translation_dictionary/html_id_translation}"
                  tal:content="structure translation_dictionary/current_translation">
               translation credits
             </div>
-            <label tal:condition="view/user_is_official_translator"
-                   tal:attributes="
-                     id string:${translation_dictionary/html_id_translation};
-                     for string:${translation_dictionary/html_id_translation}_radiobutton"
-                   tal:content="structure translation_dictionary/current_translation">
-              translation credits
-            </label>
-          </tal:form-not-writeable>
+          </tal:hidden>
         </td>
       </tal:locked>
     </tr>