launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/anon-edit-icons-for-translations into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #393922 in Launchpad itself: "Unprivileged users are shown +edit links on Person+translations"
  https://bugs.launchpad.net/launchpad/+bug/393922

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/anon-edit-icons-for-translations/+merge/129028

Summary
=======
There are a variety of links on the translation pages that show up for
unprivileged users. Initially +appoint and +edit on translationsgroup+index
were unprotected, as were the various edit links on person+translations group.

This branch adds tests to prove that the +appoint and +edit on translations
groups are fixed.

It also uses the same method used by that view to fix the links on
person+translations, and adds tests showing they work.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
Tests are added that show the links on TranslationsGroup:+index only show for
the privileged users.

The +edit links in the table in Person:+translations, as well as the
+editmylanguages link in the portlet, are placed behind tal guards that check
if the user has edit priveleges on the context (the person or team). This
check is done in the view's init, creating a property named user_can_edit
which is used in the tal. This is the same methodology used in
TranslationsGroup:+index. Tests are also added to prove this works.

Tests
=====
bin/test -vvc -t TestTranslationGroupViewPermissions -t TestPersonTranslationViewPermissions

QA
==
Confirm that as an anon user you do not see the links on Person:+translation.

LoC
===
I have more than 400 LoC in credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/templates/person-translations.pt
  lib/lp/translations/browser/tests/test_persontranslationview.py
  lib/lp/translations/browser/tests/test_translationgroup.py
  lib/lp/translations/browser/person.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/anon-edit-icons-for-translations/+merge/129028
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/anon-edit-icons-for-translations into lp:launchpad.

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/browser/person.py	2012-10-10 19:20:39 +0000
@@ -40,6 +40,7 @@
     canonical_url,
     Link,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.menu import NavigationMenu
@@ -210,6 +211,7 @@
         # will result in faster queries (cache effects).
         today = now.replace(minute=0, second=0, microsecond=0)
         self.history_horizon = today - timedelta(90, 0, 0)
+        self.user_can_edit = check_permission('launchpad.Edit', self.context)
 
     @property
     def page_title(self):

=== modified file 'lib/lp/translations/browser/tests/test_persontranslationview.py'
--- lib/lp/translations/browser/tests/test_persontranslationview.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/browser/tests/test_persontranslationview.py	2012-10-10 19:20:39 +0000
@@ -9,8 +9,14 @@
 
 from lp.services.webapp import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing import (
+    BrowserTestCase,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.translations.browser.person import PersonTranslationView
 from lp.translations.model.translator import TranslatorSet
 
@@ -422,3 +428,38 @@
         # languages.
         self.view.context.removeLanguage(self.language)
         self.assertTrue(self.view.requires_preferred_languages)
+
+
+class TestPersonTranslationViewPermissions(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonTranslationViewPermissions, self).setUp()
+        self.context = self.factory.makePerson()
+        self.language = self.factory.makeLanguage()
+        self.context.addLanguage(self.language)
+        owner = self.factory.makePerson()
+        self.translationgroup = self.factory.makeTranslationGroup(owner=owner)
+        TranslatorSet().new(
+            translationgroup=self.translationgroup, language=self.language,
+            translator=self.context)
+
+    def test_links_anon(self):
+        browser = self.getViewBrowser(
+            self.context, "+translations", no_login=True)
+        self.assertFalse("+editmylanguages" in browser.contents)
+        self.assertFalse("+edit" in browser.contents)
+
+    def test_links_unauthorized(self):
+        group = self.factory.makeTranslationGroup()
+        browser = self.getViewBrowser(self.context, "+translations")
+        self.assertFalse("+editmylanguages" in browser.contents)
+        self.assertFalse("+edit" in browser.contents)
+
+    def test_links_authorized(self):
+        group = self.factory.makeTranslationGroup()
+        browser = self.getViewBrowser(
+            self.context, "+translations", user=self.context)
+        self.assertTrue("+editmylanguages" in browser.contents)
+        self.assertTrue("+edit" in browser.contents)

=== modified file 'lib/lp/translations/browser/tests/test_translationgroup.py'
--- lib/lp/translations/browser/tests/test_translationgroup.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/browser/tests/test_translationgroup.py	2012-10-10 19:20:39 +0000
@@ -10,8 +10,14 @@
 
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing import (
+    BrowserTestCase,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.translations.browser.translationgroup import TranslationGroupView
 
 
@@ -21,23 +27,19 @@
     def setUp(self):
         super(TestTranslationGroupView, self).setUp()
 
-    def _makeGroup(self):
-        """Create a translation group."""
-        return self.factory.makeTranslationGroup()
-
-    def _makeView(self, group):
+    def _makeView(self, group, name=None):
         """Create a view for self.group."""
         view = TranslationGroupView(group, LaunchpadTestRequest())
         view.initialize()
         return view
 
     def test_translator_list_empty(self):
-        view = self._makeView(self._makeGroup())
+        view = self._makeView(self.factory.makeTranslationGroup())
         self.assertEqual([], view.translator_list)
 
     def test_translator_list(self):
         # translator_list composes dicts using _makeTranslatorDict.
-        group = self._makeGroup()
+        group = self.factory.makeTranslationGroup()
         tr_translator = self.factory.makeTranslator('tr', group)
         transaction.commit()
         view = self._makeView(group)
@@ -47,7 +49,7 @@
 
     def test_makeTranslatorDict(self):
         # _makeTranslatorDict describes a Translator entry to the UI.
-        group = self._makeGroup()
+        group = self.factory.makeTranslationGroup()
         xhosa = self.factory.makeTranslator('xh', group)
         xhosa.style_guide_url = 'http://xh.example.com/'
         view = self._makeView(group)
@@ -62,3 +64,30 @@
         self.assertEqual(xhosa.datecreated, output['datecreated'])
         self.assertEqual(xhosa.style_guide_url, output['style_guide_url'])
         self.assertEqual(xhosa, output['context'])
+
+
+class TestTranslationGroupViewPermissions(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def _assertLinksFound(self, contents, links_found):
+        for link in ['+edit', '+appoint']:
+            if links_found:
+                self.assertTrue(link in contents)
+            else:
+                self.assertFalse(link in contents)
+
+    def test_links_anon(self):
+        group = self.factory.makeTranslationGroup()
+        browser = self.getViewBrowser(group, "+index", no_login=True)
+        self._assertLinksFound(browser.contents, False)
+
+    def test_links_unauthorized(self):
+        group = self.factory.makeTranslationGroup()
+        browser = self.getViewBrowser(group, "+index")
+        self._assertLinksFound(browser.contents, False)
+
+    def test_links_authorized(self):
+        group = self.factory.makeTranslationGroup()
+        browser = self.getViewBrowser(group, "+index", user=group.owner)
+        self._assertLinksFound(browser.contents, True)

=== modified file 'lib/lp/translations/templates/person-translations.pt'
--- lib/lp/translations/templates/person-translations.pt	2012-06-16 13:12:41 +0000
+++ lib/lp/translations/templates/person-translations.pt	2012-10-10 19:20:39 +0000
@@ -10,7 +10,7 @@
 <body>
 
 <metal:side fill-slot="side">
-    <div class="portlet">
+    <div tal:condition="view/user_can_edit" class="portlet">
       <a href="/+editmylanguages" class="sprite edit">Change your preferred languages</a>
     </div>
 </metal:side>
@@ -132,9 +132,12 @@
                  tal:content="translator/style_guide_url"
                  tal:attributes="href translator/style_guide_url"
               >Documentation URL</a>
+            <tal:user-can-edit condition="view/user_can_edit">
             <a class="edit sprite action-icon"
                tal:attributes="href string:${translator/translationgroup/fmt:url}/${translator/language/code}/+edit"
-            >Edit</a></td>
+               >Edit</a>
+             </tal:user-can-edit>
+           </td>
         </tr>
       </tbody>
     </table>