launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03362
[Merge] lp:~allenap/launchpad/no-diffs-for-underprivileged-bug-760648 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/no-diffs-for-underprivileged-bug-760648 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #760648 in Launchpad itself: "Package diffs should not be displayed if they are not available and the current user has not the ability to request them."
https://bugs.launchpad.net/launchpad/+bug/760648
For more details, see:
https://code.launchpad.net/~allenap/launchpad/no-diffs-for-underprivileged-bug-760648/+merge/58137
There's a section in the template which is a bit confusing at
first. It displays either the package diffs, or links to request
package diff calculation. It's confusing because it's mixed in
together, probably to reduce the line count.
Anyway, this branch doesn't change the template much, it changes the
logic about when the elements are displayed, and moves it into the
view.
--
https://code.launchpad.net/~allenap/launchpad/no-diffs-for-underprivileged-bug-760648/+merge/58137
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/no-diffs-for-underprivileged-bug-760648 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py 2011-04-14 16:40:06 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py 2011-04-18 15:01:17 +0000
@@ -151,10 +151,19 @@
comment in comments]
@property
+ def can_request_diffs(self):
+ """Does the user have permission to request diff calculation?"""
+ return check_permission('launchpad.Edit', self.context)
+
+ @property
def show_edit_options(self):
"""Only show the options if an editor requests via JS."""
- return self.request.is_ajax and check_permission(
- 'launchpad.Edit', self.context)
+ return self.request.is_ajax and self.can_request_diffs
+
+ @property
+ def display_diffs(self):
+ """Only show diffs if there's a base version."""
+ return self.context.base_version is not None
@property
def display_child_diff(self):
@@ -186,11 +195,31 @@
not self.context.package_diff and self.display_child_diff)
parent_diff_computable = (
not self.context.parent_package_diff and self.display_parent_diff)
- return (check_permission('launchpad.Edit', self.context) and
- self.context.base_version and
+ return (self.display_diffs and
+ self.can_request_diffs and
(derived_diff_computable or
parent_diff_computable))
+ @property
+ def display_package_diffs_info(self):
+ """Whether or not to show package differences info.
+
+ Show if:
+
+ There are no diffs yet available AND the base version is set AND
+ either the parent or the derived version differs from the base
+ version AND the user can request diff calculation,
+
+ Or:
+
+ There are diffs.
+
+ """
+ return (
+ self.context.package_diff is not None or
+ self.context.parent_package_diff is not None or
+ self.show_package_diffs_request_link)
+
class DistroSeriesDifferenceDisplayComment:
"""Used simply to provide `IComment` for rendering."""
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-04-14 21:01:38 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-04-18 15:01:17 +0000
@@ -39,6 +39,7 @@
PackagePublishingStatus,
)
from lp.testing import (
+ anonymous_logged_in,
celebrity_logged_in,
person_logged_in,
TestCaseWithFactory,
@@ -185,8 +186,9 @@
'parent': parent_changelog_lfa})
self.assertEqual('0.1-1', ds_diff.base_version)
- view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
- soup = BeautifulSoup(view())
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+ soup = BeautifulSoup(view())
tags = soup.find('ul', 'package-diff-status').findAll('span')
self.assertEqual(2, len(tags))
@@ -205,8 +207,9 @@
'parent': parent_changelog_lfa})
self.assertEqual('0.30-1', ds_diff.base_version)
- view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
- soup = BeautifulSoup(view())
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+ soup = BeautifulSoup(view())
tags = soup.find('ul', 'package-diff-status').findAll('span')
self.assertEqual(1, len(tags))
@@ -225,8 +228,9 @@
'parent': parent_changelog_lfa})
self.assertEqual('0.30-1', ds_diff.base_version)
- view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
- soup = BeautifulSoup(view())
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+ soup = BeautifulSoup(view())
tags = soup.find('ul', 'package-diff-status').findAll('span')
self.assertEqual(1, len(tags))
@@ -333,9 +337,12 @@
ds_diff = self.factory.makeDistroSeriesDifference(
set_base_version=True)
- view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+ with person_logged_in(self.factory.makePerson()):
+ view = create_initialized_view(
+ ds_diff, '+listing-distroseries-extra')#, principal=user)
+ soup = BeautifulSoup(view())
# Both diffs present simple text repr. of proposed diff.
- self.assertEqual(2, self.number_of_request_diff_texts(view()))
+ self.assertEqual(2, self.number_of_request_diff_texts(soup))
def test_source_diff_rendering_diff(self):
# A linked description of the diff is displayed when
@@ -527,6 +534,19 @@
self.assertThat(view(), package_diff_request_matcher)
self.assertTrue(view.show_package_diffs_request_link)
+ package_diff_header_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Package diffs header', 'dt',
+ text=re.compile(
+ '\s*Differences from last common version:')))
+
+ package_diff_info_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Within(
+ soupmatchers.Tag('Package diff container', 'dd'),
+ soupmatchers.Tag(
+ 'Package diffs info', 'ul',
+ attrs={'class': 'package-diff-status'})))
+
def test_package_diff_label(self):
# If base_version is not None the label for the section is
# there.
@@ -540,11 +560,6 @@
}, changelogs={
'derived': changelog_lfa,
'parent': parent_changelog_lfa})
- package_diff_header_matcher = soupmatchers.HTMLContains(
- soupmatchers.Tag(
- 'Package diffs header', 'dt',
- text=re.compile(
- '\s*Differences from last common version:')))
with celebrity_logged_in('admin'):
ds_diff.parent_package_diff = self.factory.makePackageDiff()
@@ -552,7 +567,7 @@
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra')
html = view()
- self.assertThat(html, package_diff_header_matcher)
+ self.assertThat(html, self.package_diff_header_matcher)
def test_package_diff_no_base_version(self):
# If diff's base_version is None packages diffs are not displayed
@@ -573,12 +588,6 @@
'Pending package diff', 'span',
attrs={'class': 'PENDING'}))
- package_diff_header_matcher = soupmatchers.HTMLContains(
- soupmatchers.Tag(
- 'Package diffs header', 'dt',
- text=re.compile(
- '\s*Differences from last common version:')))
-
unknown_base_version = soupmatchers.HTMLContains(
soupmatchers.Tag(
'Unknown base version', 'dd',
@@ -597,4 +606,18 @@
MatchesAny(
package_diff_request_matcher,
pending_package_diff_matcher,
- package_diff_header_matcher)))
+ self.package_diff_header_matcher)))
+
+ def test_package_diffs_hidden_to_unprivileged_user_if_not_available(self):
+ # No diff information is shown if pacakge diffs are not available and
+ # the user does not have permission to request their creation.
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ versions={'base': '0.1', 'derived': '0.1-1derived1',
+ 'parent': '0.1-2'},
+ set_base_version=True)
+ with anonymous_logged_in():
+ view = create_initialized_view(
+ ds_diff, '+listing-distroseries-extra')
+ html = view()
+ self.assertThat(html, Not(self.package_diff_header_matcher))
+ self.assertThat(html, Not(self.package_diff_info_matcher))
=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-04-14 13:50:42 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-04-18 15:01:17 +0000
@@ -44,6 +44,7 @@
<dd tal:condition="not: context/base_version">
Unknown
</dd>
+ <tal:package-diffs-info condition="view/display_package_diffs_info">
<dt
tal:condition="view/show_package_diffs_request_link"
class="package-diff-placeholder">
@@ -53,10 +54,10 @@
</span>
</dt>
<dt
- tal:condition="python: not(view.show_package_diffs_request_link) and context.base_version">
+ tal:condition="not:view/show_package_diffs_request_link">
Differences from last common version:
</dt>
- <dd tal:condition="context/base_version">
+ <dd tal:condition="view/display_diffs">
<ul class="package-diff-status">
<tal:source-diff-option condition="view/display_child_diff">
<li tal:condition="context/package_diff">
@@ -85,7 +86,6 @@
</span>
</li>
</tal:source-diff-option>
-
<tal:parent-diff-option condition="view/display_parent_diff">
<li tal:condition="context/parent_package_diff">
<span tal:condition="context/parent_package_diff/status/enumvalue:PENDING"
@@ -113,9 +113,10 @@
</span>
</li>
</tal:parent-diff-option>
-
</ul>
</dd>
+ </tal:package-diffs-info>
+
</tal:package-diffs>
</dl>
