Launchpad itself

Merge lp:~henninge/launchpad/devel-bug-638920-private-branch into lp:launchpad

  1. devel-bug-638920-private-branch
  2. Merge into devel
Proposed by Henning Eggers
Status: Merged
Approved by: Henning Eggers
Approved revision: no longer in the source branch.
Merged at revision: 11833
Proposed branch: lp:~henninge/launchpad/devel-bug-638920-private-branch
Merge into: lp:launchpad
Diff against target: 293 lines (+174/-19)
4 files modified
lib/lp/translations/browser/productseries.py (+22/-7)
lib/lp/translations/browser/tests/test_productserieslanguage_views.py (+100/-3)
lib/lp/translations/stories/productseries/xx-productseries-translations.txt (+51/-8)
lib/lp/translations/templates/productseries-translations.pt (+1/-1)
To merge this branch: bzr merge lp:~henninge/launchpad/devel-bug-638920-private-branch
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+39540@code.launchpad.net

Commit message

Only try to show links to private branches to authorized users ond on a product series' translations page.

Description of the change

= Bug 638920 =

The translations page for a product series shows links the import and export Bazaar branches, if there are any. If either of these branches is private and the user no permissions to view them, the page fails with "unauthorized". The main page for a product series also displays a link to the series branch (which is the import branch in case of translations) but if it is private, it pretends that no branch has been set at all. Yes, that's flat out lying ;-). But since that page gets away with, the translations page can (in fact must) do the same.

== Proposed fix ==

Change the has_imports_enabled and has_exports_enabled flags on the view class to check for "View" permission on the branch. If the user lacks permission, the should return "False" just like when no imports or exports have been enabled.

== Implementation details ==

The actual fix is a few lines in lp/translations/browser/productseries.py that add a check_permission call to each of the two properties. It also removes a redundant check from uses_bzr_sync which is now the simple "or" combination of the other two flags.

The rest of the branch consists of tests for the two flags which were missing before and a pagetest that reproduced the bug nicely.

== Tests ==

I learned a little more about test layers because the new test would not work on the ZopelessLayer but needs the FunctionalLayer instead to be able to check permissions. Notice how I removed the security proxy from the productseries to be able to set it up (assign a branch, set import mode) for the test but the view itself is created using the secured copy of the productseries.
LaunchpadZopelessLayer which includes the LibrarianLayer and MemcachedLayer is not needed, though, so I changed that for the other tests.

bin/test -vvcm lp.translations \
    -t TestProductSeriesViewBzrUsage \
    -t xx-productseries-translations.txt

== Demo and QA ==

See my little screen cast that I produced for this just for the fun of it. ;-)

http://people.canonical.com/~henninge/screencasts/private_branch_link.ogv

(Please forgive my blowing into the microphone. ;)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/productseries.py
  lib/lp/translations/browser/tests/test_productserieslanguage_views.py
  lib/lp/translations/stories/productseries/xx-productseries-translations.txt
  lib/lp/translations/templates/productseries-translations.pt

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Henning,

the code changes look overall good, but I am not very happy that we lie to our users. Would it be very difficult to tell people who don't have the permission to view the branch: "yes, there is a branch for synchronization, but sorry, you can't look at it."?

Did you discuss the different options with somebody?

Anyway, if there is an sort of agreement that it is OK to give the somehwat misleading message "no synchronization branch linked", I'll approve the branch. But could you change the doc string for has_import_enabled so that it says that the user permissions are "mixed into" the result value. And could you add such a doc string (or coemment) for has_exports_enabled and uses_bzr_sync?

Otherwise, developers might also be surprised if they use these properties without careful looking ;) (OK, I know that the code documented by the doc string/comment is just two lines below -- but at least I read somehwat selectively and could easily miss the additional clauses...

review: Needs Information
Revision history for this message
Abel Deuring (adeuring) wrote :

(16:54:46) henninge: adeuring: oh sorry, sound turned off and not paying attention.
(16:55:20) henninge: adeuring: you are raising a basic question there that will need discussion on the mailing list, I think.
(16:56:53) henninge: adeuring: Are you ok with the branch if I just add the comments as you requested and leave it to a future branch to reflect the outcome of that discusstion?
(17:00:11) adeuring: henninge: yes, that's fine, I think

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/browser/productseries.py'
2--- lib/lp/translations/browser/productseries.py 2010-09-27 21:46:10 +0000
3+++ lib/lp/translations/browser/productseries.py 2010-11-02 08:12:51 +0000
4@@ -140,8 +140,12 @@
5
6 @property
7 def has_imports_enabled(self):
8- """Is imports enabled for the series?"""
9+ """Should information about automatic imports be displayed?
10+
11+ Will be True if imports are enabled for the series and if the user
12+ is allowed to view to the import branch."""
13 return (self.context.branch is not None and
14+ check_permission("launchpad.View", self.context.branch) and
15 self.context.translations_autoimport_mode !=
16 TranslationsBranchImportMode.NO_IMPORT)
17
18@@ -345,12 +349,23 @@
19 self.has_multiple_templates = (
20 self.context.getCurrentTranslationTemplates().count() > 1)
21
22- self.has_exports_enabled = (
23- self.context.translations_branch is not None)
24-
25- self.uses_bzr_sync = (
26- (self.context.branch is not None and self.has_imports_enabled) or
27- self.has_exports_enabled)
28+ @property
29+ def has_exports_enabled(self):
30+ """Should information about automatic exports be displayed?
31+
32+ Will be True if an export branch exists for the series and if the
33+ user is allowed to view that branch branch."""
34+ return self.context.translations_branch is not None and (
35+ check_permission(
36+ "launchpad.View", self.context.translations_branch))
37+
38+ @property
39+ def uses_bzr_sync(self):
40+ """Should information about automatic imports/exports be displayed?
41+
42+ Will be True if either imports or exports are enabled and if the user
43+ is allowed to view the import or the export branch (or both)."""
44+ return self.has_imports_enabled or self.has_exports_enabled
45
46 @property
47 def productserieslanguages(self):
48
49=== modified file 'lib/lp/translations/browser/tests/test_productserieslanguage_views.py'
50--- lib/lp/translations/browser/tests/test_productserieslanguage_views.py 2010-10-27 11:44:17 +0000
51+++ lib/lp/translations/browser/tests/test_productserieslanguage_views.py 2010-11-02 08:12:51 +0000
52@@ -3,20 +3,29 @@
53
54 __metaclass__ = type
55
56+from zope.security.proxy import removeSecurityProxy
57+
58 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
59-from canonical.testing.layers import LaunchpadZopelessLayer
60+from canonical.testing.layers import (
61+ DatabaseFunctionalLayer,
62+ ZopelessDatabaseLayer,
63+ )
64 from lp.testing import (
65 login_person,
66+ person_logged_in,
67 TestCaseWithFactory,
68 )
69 from lp.translations.browser.productseries import ProductSeriesView
70 from lp.translations.browser.serieslanguage import ProductSeriesLanguageView
71+from lp.translations.interfaces.translations import (
72+ TranslationsBranchImportMode,
73+ )
74
75
76 class TestProductSeriesView(TestCaseWithFactory):
77 """Test ProductSeries view in translations facet."""
78
79- layer = LaunchpadZopelessLayer
80+ layer = ZopelessDatabaseLayer
81
82 def setUp(self):
83 # Create a productseries that uses translations.
84@@ -147,10 +156,98 @@
85 self.assertEquals([], self._getProductserieslanguages(view))
86
87
88+class TestProductSeriesViewBzrUsage(TestCaseWithFactory):
89+ """Test ProductSeries view in translations facet."""
90+
91+ layer = DatabaseFunctionalLayer
92+
93+ def setUp(self):
94+ # Create a productseries that uses translations.
95+ # Strip off the security proxy to allow customization.
96+ super(TestProductSeriesViewBzrUsage, self).setUp()
97+ self.secured_productseries = self.factory.makeProductSeries()
98+ self.productseries = removeSecurityProxy(self.secured_productseries)
99+
100+ def _createView(self):
101+ # The view operates on the secured product series!
102+ view = ProductSeriesView(
103+ self.secured_productseries, LaunchpadTestRequest())
104+ view.initialize()
105+ return view
106+
107+ def test_has_imports_enabled_no_branch(self):
108+ view = self._createView()
109+ self.assertFalse(view.has_imports_enabled)
110+
111+ def test_has_exports_enabled_no_branch(self):
112+ view = self._createView()
113+ self.assertFalse(view.has_exports_enabled)
114+
115+ def test_has_imports_enabled_with_branch_imports_disabled(self):
116+ self.productseries.branch = self.factory.makeBranch()
117+ self.productseries.translations_autoimport_mode = (
118+ TranslationsBranchImportMode.NO_IMPORT)
119+ view = self._createView()
120+ self.assertFalse(view.has_imports_enabled)
121+
122+ def test_has_imports_enabled_with_branch_template_imports_enabled(self):
123+ self.productseries.branch = self.factory.makeBranch()
124+ self.productseries.translations_autoimport_mode = (
125+ TranslationsBranchImportMode.IMPORT_TEMPLATES)
126+ view = self._createView()
127+ self.assertTrue(view.has_imports_enabled)
128+
129+ def test_has_imports_enabled_with_branch_trans_imports_enabled(self):
130+ self.productseries.branch = self.factory.makeBranch()
131+ self.productseries.translations_autoimport_mode = (
132+ TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
133+ view = self._createView()
134+ self.assertTrue(view.has_imports_enabled)
135+
136+ def test_has_imports_enabled_private_branch_non_privileged(self):
137+ # Private branches are hidden from non-privileged users. The view
138+ # pretends that it is not used for imports.
139+ self.productseries.branch = self.factory.makeBranch(private=True)
140+ self.productseries.translations_autoimport_mode = (
141+ TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
142+ view = self._createView()
143+ self.assertFalse(view.has_imports_enabled)
144+
145+ def test_has_imports_enabled_private_branch_privileged(self):
146+ # Private branches are visible for privileged users.
147+ self.productseries.branch = self.factory.makeBranch(private=True)
148+ self.productseries.translations_autoimport_mode = (
149+ TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
150+ with person_logged_in(self.productseries.branch.owner):
151+ view = self._createView()
152+ self.assertTrue(view.has_imports_enabled)
153+
154+ def test_has_exports_enabled_with_branch(self):
155+ self.productseries.translations_branch = self.factory.makeBranch()
156+ view = self._createView()
157+ self.assertTrue(view.has_exports_enabled)
158+
159+ def test_has_exports_enabled_private_branch_non_privileged(self):
160+ # Private branches are hidden from non-privileged users. The view
161+ # pretends that it is not used for exports.
162+ self.productseries.translations_branch = self.factory.makeBranch(
163+ private=True)
164+ view = self._createView()
165+ self.assertFalse(view.has_exports_enabled)
166+
167+ def test_has_exports_enabled_private_branch_privileged(self):
168+ # Private branches are visible for privileged users.
169+ self.productseries.translations_branch = self.factory.makeBranch(
170+ private=True)
171+ with person_logged_in(self.productseries.translations_branch.owner):
172+ view = self._createView()
173+ self.assertTrue(view.has_exports_enabled)
174+
175+
176 class TestProductSeriesLanguageView(TestCaseWithFactory):
177 """Test ProductSeriesLanguage view."""
178
179- layer = LaunchpadZopelessLayer
180+ layer = ZopelessDatabaseLayer
181
182 def setUp(self):
183 # Create a productseries that uses translations.
184
185=== modified file 'lib/lp/translations/stories/productseries/xx-productseries-translations.txt'
186--- lib/lp/translations/stories/productseries/xx-productseries-translations.txt 2010-10-18 22:24:59 +0000
187+++ lib/lp/translations/stories/productseries/xx-productseries-translations.txt 2010-11-02 08:12:51 +0000
188@@ -164,7 +164,8 @@
189 >>> print extract_text(
190 ... find_tag_by_id(
191 ... owner_browser.contents, 'translations-explanation'))
192- Launchpad allows communities to translate projects using imports or a branch.
193+ Launchpad allows communities to translate projects using
194+ imports or a branch.
195 Getting started with translating your project in Launchpad
196 Configure translations
197
198@@ -186,7 +187,8 @@
199 >>> print extract_text(
200 ... find_tag_by_id(
201 ... admin_browser.contents, 'translations-explanation'))
202- Launchpad allows communities to translate projects using imports or a branch.
203+ Launchpad allows communities to translate projects using
204+ imports or a branch.
205 Getting started with translating your project in Launchpad
206 Configure translations
207
208@@ -229,14 +231,23 @@
209 Branch synchronization options
210 ------------------------------
211
212-If automatic import and export are set, but import branch is unset, we only
213-indicate that export is happening.
214+When no imports or exports have been set up, the page indicates that
215+
216+ >>> browser.open(frobnicator_trunk_url)
217+ >>> sync_settings = first_tag_by_class(
218+ ... browser.contents, 'automatic-synchronization')
219+ >>> print extract_text(sync_settings)
220+ Automatic synchronization
221+ This project is currently not using any synchronization
222+ with bazaar branches.
223+
224+If a translation branch is set we indicate that exports are happening.
225+Imports are not mentioned until a series branch has been set.
226
227 >>> login('foo.bar@canonical.com')
228 >>> from lp.translations.interfaces.translations import (
229 ... TranslationsBranchImportMode)
230 >>> branch = factory.makeBranch(product=frobnicator)
231- >>> branch_name = branch.name
232 >>> frobnicator_trunk.branch = None
233 >>> frobnicator_trunk.translations_autoimport_mode = (
234 ... TranslationsBranchImportMode.IMPORT_TEMPLATES)
235@@ -244,10 +255,42 @@
236 >>> logout()
237
238 >>> browser.open(frobnicator_trunk_url)
239- >>> print extract_text(browser.contents)
240- Translations...
241+ >>> sync_settings = first_tag_by_class(
242+ ... browser.contents, 'automatic-synchronization')
243+ >>> print extract_text(sync_settings)
244+ Automatic synchronization
245 Translations are exported daily to branch
246- ...
247+ lp://dev/~person-name.../frobnicator/branch....
248+
249+If the branch is private, though the page pretends to non-privileged users
250+that no synchronization has been set up.
251+
252+ >>> login('foo.bar@canonical.com')
253+ >>> private_branch = factory.makeBranch(product=frobnicator, private=True)
254+ >>> frobnicator_trunk.translations_branch = private_branch
255+ >>> logout()
256+
257+ >>> browser.open(frobnicator_trunk_url)
258+ >>> sync_settings = first_tag_by_class(
259+ ... browser.contents, 'automatic-synchronization')
260+ >>> print extract_text(sync_settings)
261+ Automatic synchronization
262+ This project is currently not using any synchronization
263+ with bazaar branches.
264+
265+Imports are indicated in likewise manner once a series branch has been set.
266+
267+ >>> login('foo.bar@canonical.com')
268+ >>> frobnicator_trunk.branch = branch
269+ >>> logout()
270+
271+ >>> browser.open(frobnicator_trunk_url)
272+ >>> sync_settings = first_tag_by_class(
273+ ... browser.contents, 'automatic-synchronization')
274+ >>> print extract_text(sync_settings)
275+ Automatic synchronization
276+ Translations are imported with every update from branch
277+ lp://dev/frobnicator.
278
279
280 Translation focus
281
282=== modified file 'lib/lp/translations/templates/productseries-translations.pt'
283--- lib/lp/translations/templates/productseries-translations.pt 2010-09-24 15:30:10 +0000
284+++ lib/lp/translations/templates/productseries-translations.pt 2010-11-02 08:12:51 +0000
285@@ -98,7 +98,7 @@
286 </div>
287 <div class="yui-g">
288 <div class="yui-u first">
289- <div class="portlet">
290+ <div class="portlet automatic-synchronization">
291 <h3>Automatic synchronization</h3>
292 <tal:uses-bzr-sync condition="view/uses_bzr_sync">
293 <tal:imports condition="view/has_imports_enabled">