launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03102
[Merge] lp:~henninge/launchpad/devel-737418-remove-sharing-links into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/devel-737418-remove-sharing-links into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #737418 in Launchpad itself: "Remove details link from productseries pages"
https://bugs.launchpad.net/launchpad/+bug/737418
For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-737418-remove-sharing-links/+merge/55313
= Summary =
An earlier branch had introduced sharing information and links to
the sharing details page onto translation pages for productseries,
sourcepackages and translation templates. At the time it was assumed
that there would be different sharing details pages for productseries
and sourcepackages but now only sourcepackages have such a page.
== Proposed fix ==
This branch replaces links on the productseriespages to the details
page of the linked sourcepackages or removes the link all together if
no such link exists.
Permission checking has also been updated. The editable information on
the sharing details page is all related to the productseries involved
in the sharing, so that really only the permissions on that matter.
== Pre-implementation notes ==
We discussed this a bit within the squad.
== Implementation details ==
Because of its simplifications, all the permission checking can now
happen within sharing_details with out any further information. Thus
can_edit_sharing_details could be removed from most views.
Also, getTranslationTarget was consequently renamed to
getTranslationSourcePackage because the links only point to that.
== Tests ==
bin/test -vvcm lp.translations.browser.tests.test_sharing_information
== Demo and Q/A ==
Go to http://translations.launchpad.dev/ubuntu/hoary/+source/evolution
You can:
- go to the sharing series
(http://translations.launchpad.dev/evolution/trunk)
- go to the details page, both from here and from the sharing series
(http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+sharing-details)
- The link should be named "Edit .." when logged in as
test@xxxxxxxxxxxxx because that user owns the upstream project.
- The link should be named "View .." when logged in as no-priv.
- Remove the packaging link, you will find that another link exists to
Ubuntu warty. Remove that, too, to see how the upstream page looks.
- Restore the link to warty. There are no templates on warty. The
upstream series page should still link to the sourcepackage while from
the upstream template will say that it is not sharing.
(http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2)
= Launchpad lint =
Filed bug 744873 for pocketlint.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/browser/productseries.py
lib/lp/translations/browser/translationsharing.py
lib/lp/translations/browser/tests/test_sharing_information.py
lib/lp/translations/browser/sourcepackage.py
lib/lp/translations/browser/potemplate.py
Traceback (most recent call last):
File "/usr/bin/pocketlint", line 4, in <module>
from pocketlint.formatcheck import main
File "/usr/lib/pymodules/python2.6/pocketlint/formatcheck.py", line 22, in <module>
from xml.etree.ElementTree import ParseError
ImportError: cannot import name ParseError
--
https://code.launchpad.net/~henninge/launchpad/devel-737418-remove-sharing-links/+merge/55313
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-737418-remove-sharing-links into lp:launchpad.
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2011-03-23 06:17:58 +0000
+++ lib/lp/translations/browser/potemplate.py 2011-03-29 11:06:48 +0000
@@ -354,20 +354,14 @@
def sharing_template(self):
return self.context.getOtherSidePOTemplate()
- def getTranslationTarget(self):
+ def getTranslationSourcePackage(self):
"""See `TranslationSharingDetailsMixin`."""
if self.is_upstream_template:
- return self.context.productseries
+ productseries = self.context.productseries
+ return productseries.getUbuntuTranslationFocusPackage()
else:
return self.context.sourcepackage
- def can_edit_sharing_details(self):
- if self.is_upstream_template:
- obj = self.context.productseries
- else:
- obj = self.context.distroseries
- return check_permission('launchpad.Edit', obj)
-
class POTemplateUploadView(LaunchpadView, TranslationsMixin):
"""Upload translations and updated template."""
=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py 2011-03-24 20:48:35 +0000
+++ lib/lp/translations/browser/productseries.py 2011-03-29 11:06:48 +0000
@@ -456,17 +456,15 @@
return check_permission("launchpad.TranslationsAdmin", self.context)
def is_sharing(self):
- return self.context.has_sharing_translation_templates
+ return self.sharing_sourcepackage is not None
@property
def sharing_sourcepackage(self):
return self.context.getUbuntuTranslationFocusPackage()
- def getTranslationTarget(self):
+ def getTranslationSourcePackage(self):
"""See `TranslationSharingDetailsMixin`."""
- return self.context
-
- can_edit_sharing_details = can_configure_translations
+ return self.sharing_sourcepackage
class SettingsRadioWidget(LaunchpadRadioWidgetWithDescription):
=== modified file 'lib/lp/translations/browser/sourcepackage.py'
--- lib/lp/translations/browser/sourcepackage.py 2011-03-24 20:48:35 +0000
+++ lib/lp/translations/browser/sourcepackage.py 2011-03-29 11:06:48 +0000
@@ -36,14 +36,7 @@
from lp.translations.model.translationpackagingjob import TranslationMergeJob
-class SharingDetailsPermissionsMixin:
-
- def can_edit_sharing_details(self):
- return check_permission('launchpad.Edit', self.context.distroseries)
-
-
class SourcePackageTranslationsView(TranslationsMixin,
- SharingDetailsPermissionsMixin,
TranslationSharingDetailsMixin):
@property
@@ -55,13 +48,13 @@
return "Translations for %s" % self.context.displayname
def is_sharing(self):
- return self.context.has_sharing_translation_templates
+ return self.sharing_productseries is not None
@property
def sharing_productseries(self):
return self.context.productseries
- def getTranslationTarget(self):
+ def getTranslationSourcePackage(self):
"""See `TranslationSharingDetailsMixin`."""
return self.context
@@ -108,9 +101,7 @@
return "Download translations for %s" % self.download_description
-class SourcePackageTranslationSharingDetailsView(
- LaunchpadView,
- SharingDetailsPermissionsMixin):
+class SourcePackageTranslationSharingDetailsView(LaunchpadView):
"""Details about translation sharing."""
page_title = "Sharing details"
@@ -118,6 +109,9 @@
def is_sharing(self):
return self.context.has_sharing_translation_templates
+ def can_edit_sharing_details(self):
+ return check_permission('launchpad.Edit', self.context.productseries)
+
def initialize(self):
if not getFeatureFlag('translations.sharing_information.enabled'):
raise NotFound(self.context, '+sharing-details')
=== modified file 'lib/lp/translations/browser/tests/test_sharing_information.py'
--- lib/lp/translations/browser/tests/test_sharing_information.py 2011-03-08 11:01:41 +0000
+++ lib/lp/translations/browser/tests/test_sharing_information.py 2011-03-29 11:06:48 +0000
@@ -66,9 +66,17 @@
"""Get a user that is authorized to edit sharing details on obj."""
raise NotImplementedError
- SHARING_DETAILS_INFO = "View sharing details"
- SHARING_DETAILS_SETUP = "Set up sharing"
- SHARING_DETAILS_EDIT = "Edit sharing details"
+ def getAuthorizedUserForProductseries(self, productseries):
+ """Get a user that has Edit rights on productseries.
+
+ If productseries is None, return an arbritrary user. Used by
+ implementations of getAuthorizedUser.
+ """
+ logged_in_user = self.factory.makePerson(password='test')
+ if productseries is not None:
+ with celebrity_logged_in('admin'):
+ productseries.product.owner = logged_in_user
+ return logged_in_user
def _test_sharing_information(self, obj,
id_under_test, expected_text,
@@ -91,16 +99,27 @@
self.assertTextMatchesExpressionIgnoreWhitespace(
expected_text, extract_text(sharing_info))
- def test_not_sharing(self):
+ def test_not_sharing_info(self):
self._test_sharing_information(
self.makeNotSharingObject(),
'sharing-information', self.NOT_SHARING_TEXT)
- def test_sharing(self):
+ def test_sharing_info(self):
self._test_sharing_information(
self.makeSharingObject(),
'sharing-information', self.SHARING_TEXT)
+
+class TestSharingDetailsLinkMixin:
+ """Test that the link to the sharing details page is present.
+
+ Requires TestSharingInfoMixin.
+ """
+
+ SHARING_DETAILS_INFO = "View sharing details"
+ SHARING_DETAILS_SETUP = "Set up sharing"
+ SHARING_DETAILS_EDIT = "Edit sharing details"
+
def test_sharing_details_info(self):
# For unauthorized users, the link to the sharing details page is
# informational.
@@ -126,7 +145,8 @@
class TestUpstreamPOTemplateSharingInfo(BrowserTestCase,
- TestSharingInfoMixin):
+ TestSharingInfoMixin,
+ TestSharingDetailsLinkMixin):
"""Test display of template sharing info."""
layer = DatabaseFunctionalLayer
@@ -145,8 +165,11 @@
SHARING_TEXT = """
This template is sharing translations with .*"""
+ SHARING_DETAILS_SETUP = None
+
def getAuthorizedUser(self, potemplate):
- return potemplate.productseries.product.owner
+ productseries = potemplate.productseries
+ return self.getAuthorizedUserForProductseries(productseries)
class TestPOFileSharingInfo(BrowserTestCase, TestSharingInfoMixin):
@@ -172,10 +195,6 @@
def getAuthorizedUser(self, productseries):
return None
- SHARING_DETAILS_INFO = None
- SHARING_DETAILS_SETUP = None
- SHARING_DETAILS_EDIT = None
-
class TestDummyPOFileSharingInfo(BrowserTestCase, TestSharingInfoMixin):
"""Test display of DummyPOFile sharing info."""
@@ -196,15 +215,10 @@
SHARING_TEXT = """
These translations are shared with .*"""
- def getAuthorizedUser(self, productseries):
- return None
-
- SHARING_DETAILS_INFO = None
- SHARING_DETAILS_SETUP = None
- SHARING_DETAILS_EDIT = None
-
-
-class TestUpstreamSharingInfo(BrowserTestCase, TestSharingInfoMixin):
+
+class TestUpstreamSharingInfo(BrowserTestCase,
+ TestSharingInfoMixin,
+ TestSharingDetailsLinkMixin):
"""Test display of product series sharing info."""
layer = DatabaseFunctionalLayer
@@ -219,17 +233,22 @@
package."""
def makeSharingObject(self):
- template = self._makePackagingAndTemplates(TranslationSide.UPSTREAM)
- return template.productseries
+ packaging = self.factory.makePackagingLink(in_ubuntu=True)
+ set_translations_usage(packaging.productseries.product)
+ return packaging.productseries
SHARING_TEXT = """
This project series is sharing translations with .*"""
+ SHARING_DETAILS_SETUP = None
+
def getAuthorizedUser(self, productseries):
- return productseries.product.owner
-
-
-class TestUbuntuPOTemplateSharingInfo(BrowserTestCase, TestSharingInfoMixin):
+ return self.getAuthorizedUserForProductseries(productseries)
+
+
+class TestUbuntuPOTemplateSharingInfo(BrowserTestCase,
+ TestSharingInfoMixin,
+ TestSharingDetailsLinkMixin):
"""Test display of template sharing info in an Ubuntu source package."""
layer = DatabaseFunctionalLayer
@@ -252,13 +271,13 @@
This template is sharing translations with .*"""
def getAuthorizedUser(self, potemplate):
- with celebrity_logged_in('admin'):
- potemplate.distroseries.owner = self.factory.makePerson(
- password='test')
- return potemplate.distroseries.owner
-
-
-class TestUbuntuSharingInfo(BrowserTestCase, TestSharingInfoMixin):
+ productseries = potemplate.sourcepackage.productseries
+ return self.getAuthorizedUserForProductseries(productseries)
+
+
+class TestUbuntuSharingInfo(BrowserTestCase,
+ TestSharingInfoMixin,
+ TestSharingDetailsLinkMixin):
"""Test display of source package sharing info."""
layer = DatabaseFunctionalLayer
@@ -274,15 +293,12 @@
project."""
def makeSharingObject(self):
- template = self._makePackagingAndTemplates(TranslationSide.UBUNTU)
- enable_translations_on_distroseries(template.distroseries)
- return template.sourcepackage
+ packaging = self.factory.makePackagingLink(in_ubuntu=True)
+ return packaging.sourcepackage
SHARING_TEXT = """
This source package is sharing translations with .*"""
def getAuthorizedUser(self, sourcepackage):
- with celebrity_logged_in('admin'):
- sourcepackage.distroseries.owner = self.factory.makePerson(
- password='test')
- return sourcepackage.distroseries.owner
+ productseries = sourcepackage.productseries
+ return self.getAuthorizedUserForProductseries(productseries)
=== modified file 'lib/lp/translations/browser/translationsharing.py'
--- lib/lp/translations/browser/translationsharing.py 2011-03-08 17:08:27 +0000
+++ lib/lp/translations/browser/translationsharing.py 2011-03-29 11:06:48 +0000
@@ -11,6 +11,7 @@
from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.authorization import check_permission
class TranslationSharingDetailsMixin:
@@ -24,34 +25,34 @@
"""Whether this object is sharing translations or not."""
raise NotImplementedError
- def can_edit_sharing_details(self):
- """If the current user can edit sharing details."""
- raise NotImplementedError
-
- def getTranslationTarget(self):
- """Return either a productseries or a sourcepackage."""
+ def getTranslationSourcePackage(self):
+ """Return the sourcepackage or None."""
raise NotImplementedError
def sharing_details(self):
"""Construct the link to the sharing details page."""
tag_template = (
'<a class="sprite %(icon)s" id="sharing-details"'
- ' href="%(href)s/+sharing-details">%(text)s</a>')
+ ' href="%(href)s">%(text)s</a>')
- if self.can_edit_sharing_details():
+ sourcepackage = self.getTranslationSourcePackage()
+ if sourcepackage is None:
+ return ""
+ productseries = sourcepackage.productseries
+ can_edit_upstream = (
+ productseries is None or
+ check_permission('launchpad.Edit', productseries))
+ if can_edit_upstream:
icon = 'edit'
if self.is_sharing():
text = "Edit sharing details"
else:
text = "Set up sharing"
else:
- if self.is_sharing():
- icon = 'info'
- text = "View sharing details"
- else:
- return ""
+ icon = 'info'
+ text = "View sharing details"
href = canonical_url(
- self.getTranslationTarget(),
+ sourcepackage,
rootsite='translations',
- )
+ view_name='+sharing-details')
return tag_template % dict(icon=icon, text=text, href=href)
