launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-dummy-translations into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #702477 in Launchpad itself: "Translation credits on new POFiles show the dummy string for the other side"
  https://bugs.launchpad.net/launchpad/+bug/702477

For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-dummy-translations/+merge/57208

= 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
-- 
https://code.launchpad.net/~abentley/launchpad/fix-dummy-translations/+merge/57208
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/fix-dummy-translations into lp:launchpad.

=== 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-11 18:11:22 +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-11 18:11:22 +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,24 @@
                                            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)

=== 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-11 18:11:22 +0000
@@ -274,53 +274,36 @@
       <tal:locked condition="context/potmsgset/is_translation_credit">
         <td class="icon left right" />
         <td>
-          <tal:form-writeable condition="view/form_is_writeable">
-            <tal:automatic condition="view/translation_credits">
-              <div tal:condition="not: view/user_is_official_translator"
-                   tal:attributes="
-                     id string:${translation_dictionary/html_id_translation}"
-                   tal:content="structure view/translation_credits">
-                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 view/translation_credits">
-                translation credits
-              </label>
-            </tal:automatic>
-            <tal:no-automatic condition="not:view/translation_credits">
-              <div class="no-translation"
-                   tal:condition="not:view/user_is_official_translator"
-                   tal:attributes="
-                     id string:${translation_dictionary/html_id_translation}">
-                (no translation yet)
-              </div>
-              <label class="no-translation"
-                     tal:condition="view/user_is_official_translator"
-                     tal:attributes="
-                       id string:${translation_dictionary/html_id_translation};
-                        for string:${translation_dictionary/html_id_translation}_radiobutton">
-                (no translation yet)
-              </label>
-            </tal:no-automatic>
-          </tal:form-writeable>
-          <tal:form-not-writeable condition="not:view/form_is_writeable">
+          <tal:automatic condition="view/translation_credits">
             <div tal:condition="not: view/user_is_official_translator"
                  tal:attributes="
                    id string:${translation_dictionary/html_id_translation}"
-                 tal:content="structure translation_dictionary/current_translation">
+                 tal:content="structure view/translation_credits">
               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">
+                   tal:content="structure view/translation_credits">
               translation credits
             </label>
-          </tal:form-not-writeable>
+          </tal:automatic>
+          <tal:no-automatic condition="not:view/translation_credits">
+            <div class="no-translation"
+                 tal:condition="not:view/user_is_official_translator"
+                 tal:attributes="
+                   id string:${translation_dictionary/html_id_translation}">
+              (no translation yet)
+            </div>
+            <label class="no-translation"
+                   tal:condition="view/user_is_official_translator"
+                   tal:attributes="
+                     id string:${translation_dictionary/html_id_translation};
+                      for string:${translation_dictionary/html_id_translation}_radiobutton">
+              (no translation yet)
+            </label>
+          </tal:no-automatic>
         </td>
       </tal:locked>
     </tr>