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
=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py 2010-09-27 21:46:10 +0000
+++ lib/lp/translations/browser/productseries.py 2010-11-02 08:12:51 +0000
@@ -140,8 +140,12 @@
140140
141 @property141 @property
142 def has_imports_enabled(self):142 def has_imports_enabled(self):
143 """Is imports enabled for the series?"""143 """Should information about automatic imports be displayed?
144
145 Will be True if imports are enabled for the series and if the user
146 is allowed to view to the import branch."""
144 return (self.context.branch is not None and147 return (self.context.branch is not None and
148 check_permission("launchpad.View", self.context.branch) and
145 self.context.translations_autoimport_mode !=149 self.context.translations_autoimport_mode !=
146 TranslationsBranchImportMode.NO_IMPORT)150 TranslationsBranchImportMode.NO_IMPORT)
147151
@@ -345,12 +349,23 @@
345 self.has_multiple_templates = (349 self.has_multiple_templates = (
346 self.context.getCurrentTranslationTemplates().count() > 1)350 self.context.getCurrentTranslationTemplates().count() > 1)
347351
348 self.has_exports_enabled = (352 @property
349 self.context.translations_branch is not None)353 def has_exports_enabled(self):
350354 """Should information about automatic exports be displayed?
351 self.uses_bzr_sync = (355
352 (self.context.branch is not None and self.has_imports_enabled) or356 Will be True if an export branch exists for the series and if the
353 self.has_exports_enabled)357 user is allowed to view that branch branch."""
358 return self.context.translations_branch is not None and (
359 check_permission(
360 "launchpad.View", self.context.translations_branch))
361
362 @property
363 def uses_bzr_sync(self):
364 """Should information about automatic imports/exports be displayed?
365
366 Will be True if either imports or exports are enabled and if the user
367 is allowed to view the import or the export branch (or both)."""
368 return self.has_imports_enabled or self.has_exports_enabled
354369
355 @property370 @property
356 def productserieslanguages(self):371 def productserieslanguages(self):
357372
=== modified file 'lib/lp/translations/browser/tests/test_productserieslanguage_views.py'
--- lib/lp/translations/browser/tests/test_productserieslanguage_views.py 2010-10-27 11:44:17 +0000
+++ lib/lp/translations/browser/tests/test_productserieslanguage_views.py 2010-11-02 08:12:51 +0000
@@ -3,20 +3,29 @@
33
4__metaclass__ = type4__metaclass__ = type
55
6from zope.security.proxy import removeSecurityProxy
7
6from canonical.launchpad.webapp.servers import LaunchpadTestRequest8from canonical.launchpad.webapp.servers import LaunchpadTestRequest
7from canonical.testing.layers import LaunchpadZopelessLayer9from canonical.testing.layers import (
10 DatabaseFunctionalLayer,
11 ZopelessDatabaseLayer,
12 )
8from lp.testing import (13from lp.testing import (
9 login_person,14 login_person,
15 person_logged_in,
10 TestCaseWithFactory,16 TestCaseWithFactory,
11 )17 )
12from lp.translations.browser.productseries import ProductSeriesView18from lp.translations.browser.productseries import ProductSeriesView
13from lp.translations.browser.serieslanguage import ProductSeriesLanguageView19from lp.translations.browser.serieslanguage import ProductSeriesLanguageView
20from lp.translations.interfaces.translations import (
21 TranslationsBranchImportMode,
22 )
1423
1524
16class TestProductSeriesView(TestCaseWithFactory):25class TestProductSeriesView(TestCaseWithFactory):
17 """Test ProductSeries view in translations facet."""26 """Test ProductSeries view in translations facet."""
1827
19 layer = LaunchpadZopelessLayer28 layer = ZopelessDatabaseLayer
2029
21 def setUp(self):30 def setUp(self):
22 # Create a productseries that uses translations.31 # Create a productseries that uses translations.
@@ -147,10 +156,98 @@
147 self.assertEquals([], self._getProductserieslanguages(view))156 self.assertEquals([], self._getProductserieslanguages(view))
148157
149158
159class TestProductSeriesViewBzrUsage(TestCaseWithFactory):
160 """Test ProductSeries view in translations facet."""
161
162 layer = DatabaseFunctionalLayer
163
164 def setUp(self):
165 # Create a productseries that uses translations.
166 # Strip off the security proxy to allow customization.
167 super(TestProductSeriesViewBzrUsage, self).setUp()
168 self.secured_productseries = self.factory.makeProductSeries()
169 self.productseries = removeSecurityProxy(self.secured_productseries)
170
171 def _createView(self):
172 # The view operates on the secured product series!
173 view = ProductSeriesView(
174 self.secured_productseries, LaunchpadTestRequest())
175 view.initialize()
176 return view
177
178 def test_has_imports_enabled_no_branch(self):
179 view = self._createView()
180 self.assertFalse(view.has_imports_enabled)
181
182 def test_has_exports_enabled_no_branch(self):
183 view = self._createView()
184 self.assertFalse(view.has_exports_enabled)
185
186 def test_has_imports_enabled_with_branch_imports_disabled(self):
187 self.productseries.branch = self.factory.makeBranch()
188 self.productseries.translations_autoimport_mode = (
189 TranslationsBranchImportMode.NO_IMPORT)
190 view = self._createView()
191 self.assertFalse(view.has_imports_enabled)
192
193 def test_has_imports_enabled_with_branch_template_imports_enabled(self):
194 self.productseries.branch = self.factory.makeBranch()
195 self.productseries.translations_autoimport_mode = (
196 TranslationsBranchImportMode.IMPORT_TEMPLATES)
197 view = self._createView()
198 self.assertTrue(view.has_imports_enabled)
199
200 def test_has_imports_enabled_with_branch_trans_imports_enabled(self):
201 self.productseries.branch = self.factory.makeBranch()
202 self.productseries.translations_autoimport_mode = (
203 TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
204 view = self._createView()
205 self.assertTrue(view.has_imports_enabled)
206
207 def test_has_imports_enabled_private_branch_non_privileged(self):
208 # Private branches are hidden from non-privileged users. The view
209 # pretends that it is not used for imports.
210 self.productseries.branch = self.factory.makeBranch(private=True)
211 self.productseries.translations_autoimport_mode = (
212 TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
213 view = self._createView()
214 self.assertFalse(view.has_imports_enabled)
215
216 def test_has_imports_enabled_private_branch_privileged(self):
217 # Private branches are visible for privileged users.
218 self.productseries.branch = self.factory.makeBranch(private=True)
219 self.productseries.translations_autoimport_mode = (
220 TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
221 with person_logged_in(self.productseries.branch.owner):
222 view = self._createView()
223 self.assertTrue(view.has_imports_enabled)
224
225 def test_has_exports_enabled_with_branch(self):
226 self.productseries.translations_branch = self.factory.makeBranch()
227 view = self._createView()
228 self.assertTrue(view.has_exports_enabled)
229
230 def test_has_exports_enabled_private_branch_non_privileged(self):
231 # Private branches are hidden from non-privileged users. The view
232 # pretends that it is not used for exports.
233 self.productseries.translations_branch = self.factory.makeBranch(
234 private=True)
235 view = self._createView()
236 self.assertFalse(view.has_exports_enabled)
237
238 def test_has_exports_enabled_private_branch_privileged(self):
239 # Private branches are visible for privileged users.
240 self.productseries.translations_branch = self.factory.makeBranch(
241 private=True)
242 with person_logged_in(self.productseries.translations_branch.owner):
243 view = self._createView()
244 self.assertTrue(view.has_exports_enabled)
245
246
150class TestProductSeriesLanguageView(TestCaseWithFactory):247class TestProductSeriesLanguageView(TestCaseWithFactory):
151 """Test ProductSeriesLanguage view."""248 """Test ProductSeriesLanguage view."""
152249
153 layer = LaunchpadZopelessLayer250 layer = ZopelessDatabaseLayer
154251
155 def setUp(self):252 def setUp(self):
156 # Create a productseries that uses translations.253 # Create a productseries that uses translations.
157254
=== modified file 'lib/lp/translations/stories/productseries/xx-productseries-translations.txt'
--- lib/lp/translations/stories/productseries/xx-productseries-translations.txt 2010-10-18 22:24:59 +0000
+++ lib/lp/translations/stories/productseries/xx-productseries-translations.txt 2010-11-02 08:12:51 +0000
@@ -164,7 +164,8 @@
164 >>> print extract_text(164 >>> print extract_text(
165 ... find_tag_by_id(165 ... find_tag_by_id(
166 ... owner_browser.contents, 'translations-explanation'))166 ... owner_browser.contents, 'translations-explanation'))
167 Launchpad allows communities to translate projects using imports or a branch.167 Launchpad allows communities to translate projects using
168 imports or a branch.
168 Getting started with translating your project in Launchpad169 Getting started with translating your project in Launchpad
169 Configure translations170 Configure translations
170171
@@ -186,7 +187,8 @@
186 >>> print extract_text(187 >>> print extract_text(
187 ... find_tag_by_id(188 ... find_tag_by_id(
188 ... admin_browser.contents, 'translations-explanation'))189 ... admin_browser.contents, 'translations-explanation'))
189 Launchpad allows communities to translate projects using imports or a branch.190 Launchpad allows communities to translate projects using
191 imports or a branch.
190 Getting started with translating your project in Launchpad192 Getting started with translating your project in Launchpad
191 Configure translations193 Configure translations
192194
@@ -229,14 +231,23 @@
229Branch synchronization options231Branch synchronization options
230------------------------------232------------------------------
231233
232If automatic import and export are set, but import branch is unset, we only234When no imports or exports have been set up, the page indicates that
233indicate that export is happening.235
236 >>> browser.open(frobnicator_trunk_url)
237 >>> sync_settings = first_tag_by_class(
238 ... browser.contents, 'automatic-synchronization')
239 >>> print extract_text(sync_settings)
240 Automatic synchronization
241 This project is currently not using any synchronization
242 with bazaar branches.
243
244If a translation branch is set we indicate that exports are happening.
245Imports are not mentioned until a series branch has been set.
234246
235 >>> login('foo.bar@canonical.com')247 >>> login('foo.bar@canonical.com')
236 >>> from lp.translations.interfaces.translations import (248 >>> from lp.translations.interfaces.translations import (
237 ... TranslationsBranchImportMode)249 ... TranslationsBranchImportMode)
238 >>> branch = factory.makeBranch(product=frobnicator)250 >>> branch = factory.makeBranch(product=frobnicator)
239 >>> branch_name = branch.name
240 >>> frobnicator_trunk.branch = None251 >>> frobnicator_trunk.branch = None
241 >>> frobnicator_trunk.translations_autoimport_mode = (252 >>> frobnicator_trunk.translations_autoimport_mode = (
242 ... TranslationsBranchImportMode.IMPORT_TEMPLATES)253 ... TranslationsBranchImportMode.IMPORT_TEMPLATES)
@@ -244,10 +255,42 @@
244 >>> logout()255 >>> logout()
245256
246 >>> browser.open(frobnicator_trunk_url)257 >>> browser.open(frobnicator_trunk_url)
247 >>> print extract_text(browser.contents)258 >>> sync_settings = first_tag_by_class(
248 Translations...259 ... browser.contents, 'automatic-synchronization')
260 >>> print extract_text(sync_settings)
261 Automatic synchronization
249 Translations are exported daily to branch262 Translations are exported daily to branch
250 ...263 lp://dev/~person-name.../frobnicator/branch....
264
265If the branch is private, though the page pretends to non-privileged users
266that no synchronization has been set up.
267
268 >>> login('foo.bar@canonical.com')
269 >>> private_branch = factory.makeBranch(product=frobnicator, private=True)
270 >>> frobnicator_trunk.translations_branch = private_branch
271 >>> logout()
272
273 >>> browser.open(frobnicator_trunk_url)
274 >>> sync_settings = first_tag_by_class(
275 ... browser.contents, 'automatic-synchronization')
276 >>> print extract_text(sync_settings)
277 Automatic synchronization
278 This project is currently not using any synchronization
279 with bazaar branches.
280
281Imports are indicated in likewise manner once a series branch has been set.
282
283 >>> login('foo.bar@canonical.com')
284 >>> frobnicator_trunk.branch = branch
285 >>> logout()
286
287 >>> browser.open(frobnicator_trunk_url)
288 >>> sync_settings = first_tag_by_class(
289 ... browser.contents, 'automatic-synchronization')
290 >>> print extract_text(sync_settings)
291 Automatic synchronization
292 Translations are imported with every update from branch
293 lp://dev/frobnicator.
251294
252295
253Translation focus296Translation focus
254297
=== modified file 'lib/lp/translations/templates/productseries-translations.pt'
--- lib/lp/translations/templates/productseries-translations.pt 2010-09-24 15:30:10 +0000
+++ lib/lp/translations/templates/productseries-translations.pt 2010-11-02 08:12:51 +0000
@@ -98,7 +98,7 @@
98 </div>98 </div>
99 <div class="yui-g">99 <div class="yui-g">
100 <div class="yui-u first">100 <div class="yui-u first">
101 <div class="portlet">101 <div class="portlet automatic-synchronization">
102 <h3>Automatic synchronization</h3>102 <h3>Automatic synchronization</h3>
103 <tal:uses-bzr-sync condition="view/uses_bzr_sync">103 <tal:uses-bzr-sync condition="view/uses_bzr_sync">
104 <tal:imports condition="view/has_imports_enabled">104 <tal:imports condition="view/has_imports_enabled">