launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04731
[Merge] lp:~danilo/launchpad/bug-321467 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-321467 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #321467 in Launchpad itself: "Template name is not validated on admin form."
https://bugs.launchpad.net/launchpad/+bug/321467
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-321467/+merge/72553
= Bug 321467: validate POTemplate name =
We recently exposed POTemplate:+admin page to a wider group (namely, project maintainers). However, +admin page was not sufficiently well doing the validation before trying to save the data which caused old OOPSes to become more frequent.
== Proposed fix ==
While just adding a valid_name validation for POTemplate name is a simple affair, I've went a bit further: I've decided to move only the relevant fields to the +edit page and still prohibit access to the +admin page for anyone but admins.
== Pre-implementation notes ==
I discussed Ubuntu-needed fields with David Planella, an Ubuntu Translations Coordinator.
== Implementation details ==
This meant a few things:
1. We use launchpad.TranslationsAdmin where we used to use launchpad.Edit, and launchpad.Admin for launchpad.TranslationsAdmin privilege
2. potemplate name is added to the POTemplateEditView.field_names and sourcepackagename and languagepack fields are added if the template is for a distro
3. most of the validation code is moved from admin view into the edit view
4. newly added check to the validation code is a valid_name(name) check
5. POTemplateAdminView now derives from POTemplateEditView and only implements _validateTargetAndGetTemplates method which validates the specific bits it needs to
6. test cases are expanded so they test all of the above
7. we move the "Change details" (potemplate:+edit) link to be in the "Administration" section next to the "Administer template" (potemplate:+admin) link so it makes more sense
8. _return_url never worked properly when template was moved around (it only worked when just "name" was changed), so I fix that along the way
Note: @@ -320,12 +321,11 @@ in browser/potemplate.py is a drive-by fix to remove the stale XXX and use proper code.
== Tests ==
bin/test -cvvt potemplate_views
(there are a few page test failures which I am fixing right now)
== Demo and Q/A ==
Log-in as one of (anonymous|regular user|project maintainer|rosetta expert|ubuntu translations coordinator team member) and try POTemplate:+edit and POTemplate:+admin pages from eg:
https://translations.qastaging.launchpad.net/ubuntu/natty/+templates
https://translations.qastaging.launchpad.net/openobject-addons/6.0/+templates
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/browser/tests/test_potemplate_views.py
lib/lp/translations/templates/potemplate-index.pt
lib/lp/translations/browser/configure.zcml
lib/lp/translations/interfaces/potemplate.py
lib/canonical/launchpad/security.py
lib/lp/translations/configure.zcml
lib/lp/translations/browser/potemplate.py
--
https://code.launchpad.net/~danilo/launchpad/bug-321467/+merge/72553
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-321467 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2011-08-19 14:57:16 +0000
+++ lib/canonical/launchpad/security.py 2011-08-23 11:39:41 +0000
@@ -1262,6 +1262,11 @@
Product owners does not have administrative privileges.
"""
+ permission = 'launchpad.Admin'
+ usedfor = IPOTemplate
+
+
+class EditPOTemplateDetails(AuthorizationBase):
permission = 'launchpad.TranslationsAdmin'
usedfor = IPOTemplate
@@ -1269,30 +1274,11 @@
template = self.obj
if template.distroseries is not None:
# Template is on a distribution.
- return AdminDistroSeriesTranslations(
- template.distroseries).checkAuthenticated(user)
+ return self.forwardCheckAuthenticated(user, template.distroseries)
else:
# Template is on a product.
- return AdminProductSeriesTranslations(
- template.productseries).checkAuthenticated(user)
-
-
-class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins):
- permission = 'launchpad.Edit'
- usedfor = IPOTemplate
-
- def checkAuthenticated(self, user):
- """Allow anyone with admin rights; owners, product owners and
- distribution owners; and for distros, translation group owners.
- """
- if (self.obj.productseries is not None and
- user.inTeam(self.obj.productseries.product.owner)):
- # The user is the owner of the product.
- return True
-
- return (
- AdminPOTemplateDetails.checkAuthenticated(self, user) or
- EditByOwnersOrAdmins.checkAuthenticated(self, user))
+ return self.forwardCheckAuthenticated(
+ user, template.productseries)
class AddPOTemplate(OnlyRosettaExpertsAndAdmins):
=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml 2011-08-19 15:01:48 +0000
+++ lib/lp/translations/browser/configure.zcml 2011-08-23 11:39:41 +0000
@@ -408,14 +408,14 @@
name="+edit"
for="lp.translations.interfaces.potemplate.IPOTemplate"
class="lp.translations.browser.potemplate.POTemplateEditView"
- permission="launchpad.Edit"
+ permission="launchpad.TranslationsAdmin"
template="../../app/templates/generic-edit.pt"
layer="lp.translations.publisher.TranslationsLayer"/>
<browser:page
name="+admin"
for="lp.translations.interfaces.potemplate.IPOTemplate"
class="lp.translations.browser.potemplate.POTemplateAdminView"
- permission="launchpad.TranslationsAdmin"
+ permission="launchpad.Admin"
template="../../app/templates/generic-edit.pt"
layer="lp.translations.publisher.TranslationsLayer"/>
<browser:page
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2011-07-29 17:01:31 +0000
+++ lib/lp/translations/browser/potemplate.py 2011-08-23 11:39:41 +0000
@@ -63,15 +63,16 @@
ICanonicalUrlData,
ILaunchBag,
)
-from canonical.launchpad.webapp.launchpadform import ReturnToReferrerMixin
from canonical.launchpad.webapp.menu import structured
from canonical.lazr.utils import smartquote
+from lp.app.browser.launchpadform import ReturnToReferrerMixin
from lp.app.browser.tales import DateTimeFormatterAPI
from lp.app.enums import (
service_uses_launchpad,
ServiceUsage,
)
from lp.app.errors import NotFoundError
+from lp.app.validators.name import valid_name
from lp.registry.browser.productseries import ProductSeriesFacets
from lp.registry.browser.sourcepackage import SourcePackageFacets
from lp.registry.interfaces.productseries import IProductSeries
@@ -320,12 +321,11 @@
if (by_source_count > self.SHOW_RELATED_TEMPLATES):
other = by_source_count - self.SHOW_RELATED_TEMPLATES
if (self.context.distroseries):
- # XXX adiroiban 2009-12-21 bug=499058: A canonical_url for
- # SourcePackageName is needed to avoid hardcoding this URL.
- url = (canonical_url(
- self.context.distroseries, rootsite="translations") +
- "/+source/" + self.context.sourcepackagename.name +
- "/+translations")
+ sourcepackage = self.context.distroseries.getSourcePackage(
+ self.context.sourcepackagename)
+ url = canonical_url(
+ sourcepackage, rootsite="translations",
+ view_name='+translations')
else:
url = canonical_url(
self.context.productseries,
@@ -554,13 +554,49 @@
"""View class that lets you edit a POTemplate object."""
schema = IPOTemplate
- field_names = ['translation_domain', 'description', 'priority',
- 'path', 'owner', 'iscurrent']
label = 'Edit translation template details'
page_title = 'Edit details'
PRIORITY_MIN_VALUE = 0
PRIORITY_MAX_VALUE = 100000
+ @property
+ def field_names(self):
+ field_names = [
+ 'name', 'translation_domain', 'description', 'priority',
+ 'path', 'iscurrent']
+ if self.context.distroseries:
+ field_names.extend([
+ 'sourcepackagename',
+ 'languagepack',
+ ])
+ return field_names
+
+ @property
+ def _return_url(self):
+ # We override the ReturnToReferrerMixin _return_url because it might
+ # change when any of the productseries, distroseries,
+ # sourcepackagename or name attributes change, and the basic version
+ # only supports watching changes to a single attribute.
+
+ # The referer header is a hidden input in the form.
+ referrer = self.request.form.get('_return_url')
+ returnChanged = False
+ if referrer is None:
+ # "referer" is misspelled in the HTTP specification.
+ referrer = self.request.getHeader('referer')
+ # If we were looking at the actual template, we want a new
+ # URL constructed.
+ if referrer is not None and '/+pots/' in referrer:
+ returnChanged = True
+
+ if (referrer is not None
+ and not returnChanged
+ and referrer.startswith(self.request.getApplicationURL())
+ and referrer != self.request.getHeader('location')):
+ return referrer
+ else:
+ return canonical_url(self.context)
+
@action(_('Change'), name='change')
def change_action(self, action, data):
context = self.context
@@ -581,7 +617,43 @@
naked_context = removeSecurityProxy(context)
naked_context.date_last_updated = datetime.datetime.now(pytz.UTC)
+ def _validateTargetAndGetTemplates(self, data):
+ """Return a POTemplateSubset corresponding to the chosen target."""
+ sourcepackagename = data.get('sourcepackagename',
+ self.context.sourcepackagename)
+ return getUtility(IPOTemplateSet).getSubset(
+ distroseries=self.context.distroseries,
+ sourcepackagename=sourcepackagename,
+ productseries=self.context.productseries)
+
def validate(self, data):
+ name = data.get('name', None)
+ if name is None or not valid_name(name):
+ self.setFieldError(
+ 'name',
+ 'Template name can only start with lowercase letters a-z '
+ 'or digits 0-9, and other than those characters, can only '
+ 'contain "-", "+" and "." characters.')
+
+ distroseries = data.get('distroseries', self.context.distroseries)
+ sourcepackagename = data.get(
+ 'sourcepackagename', self.context.sourcepackagename)
+ productseries = data.get('productseries', None)
+ sourcepackage_changed = (
+ distroseries is not None and
+ (distroseries != self.context.distroseries or
+ sourcepackagename != self.context.sourcepackagename))
+ productseries_changed = (productseries is not None and
+ productseries != self.context.productseries)
+ similar_templates = self._validateTargetAndGetTemplates(data)
+ if similar_templates is not None:
+ self.validateName(
+ name, similar_templates, sourcepackage_changed,
+ productseries_changed)
+ self.validateDomain(
+ data.get('translation_domain'), similar_templates,
+ sourcepackage_changed, productseries_changed)
+
priority = data.get('priority')
if priority is None:
return
@@ -592,24 +664,6 @@
'priority',
'The priority value must be between %s and %s.' % (
self.PRIORITY_MIN_VALUE, self.PRIORITY_MAX_VALUE))
- return
-
- @property
- def _return_attribute_name(self):
- """See 'ReturnToReferrerMixin'."""
- return "name"
-
-
-class POTemplateAdminView(POTemplateEditView):
- """View class that lets you admin a POTemplate object."""
- field_names = [
- 'name', 'translation_domain', 'description', 'header', 'iscurrent',
- 'owner', 'productseries', 'distroseries', 'sourcepackagename',
- 'from_sourcepackagename', 'sourcepackageversion', 'binarypackagename',
- 'languagepack', 'path', 'source_file_format', 'priority',
- 'date_last_updated']
- label = 'Administer translation template'
- page_title = "Administer"
def validateName(self, name, similar_templates,
sourcepackage_changed, productseries_changed):
@@ -645,8 +699,25 @@
self.setFieldError(
'translation_domain', "Domain is already in use.")
- def validate(self, data):
- super(POTemplateAdminView, self).validate(data)
+ @property
+ def _return_attribute_name(self):
+ """See 'ReturnToReferrerMixin'."""
+ return "name"
+
+
+class POTemplateAdminView(POTemplateEditView):
+ """View class that lets you admin a POTemplate object."""
+ field_names = [
+ 'name', 'translation_domain', 'description', 'header', 'iscurrent',
+ 'owner', 'productseries', 'distroseries', 'sourcepackagename',
+ 'from_sourcepackagename', 'sourcepackageversion', 'binarypackagename',
+ 'languagepack', 'path', 'source_file_format', 'priority',
+ 'date_last_updated']
+ label = 'Administer translation template'
+ page_title = "Administer"
+
+ def _validateTargetAndGetTemplates(self, data):
+ """Return a POTemplateSubset corresponding to the chosen target."""
distroseries = data.get('distroseries')
sourcepackagename = data.get('sourcepackagename')
productseries = data.get('productseries')
@@ -662,27 +733,10 @@
if message is not None:
self.addError(message)
- return
-
- # Validate name and domain; they should be unique within the
- # template's translation target (productseries or source
- # package). Validate against the target selected by the form,
- # not the template's existing target; the form may change the
- # template's target as well.
- similar_templates = getUtility(IPOTemplateSet).getSubset(
+ return None
+ return getUtility(IPOTemplateSet).getSubset(
distroseries=distroseries, sourcepackagename=sourcepackagename,
productseries=productseries)
- sourcepackage_changed = (
- distroseries != self.context.distroseries or
- sourcepackagename != self.context.sourcepackagename)
- productseries_changed = productseries != self.context.productseries
-
- self.validateName(
- data.get('name'), similar_templates,
- sourcepackage_changed, productseries_changed)
- self.validateDomain(
- data.get('translation_domain'), similar_templates,
- sourcepackage_changed, productseries_changed)
class POTemplateExportView(BaseExportView):
@@ -872,10 +926,10 @@
self.distroseries = series
else:
self.productseries = series
- self.can_admin = check_permission(
- 'launchpad.TranslationsAdmin', series)
+ self.can_admin = check_permission('launchpad.Admin', series)
self.can_edit = (
- self.can_admin or check_permission('launchpad.Edit', series))
+ self.can_admin or
+ check_permission('launchpad.TranslationsAdmin', series))
self.user_is_logged_in = (self.user is not None)
=== modified file 'lib/lp/translations/browser/tests/test_potemplate_views.py'
--- lib/lp/translations/browser/tests/test_potemplate_views.py 2011-06-07 12:59:28 +0000
+++ lib/lp/translations/browser/tests/test_potemplate_views.py 2011-08-23 11:39:41 +0000
@@ -9,10 +9,13 @@
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.testing import TestCaseWithFactory
-from lp.translations.browser.potemplate import POTemplateAdminView
-
-
-class TestPOTemplateAdminViewValidation(TestCaseWithFactory):
+from lp.translations.browser.potemplate import (
+ POTemplateAdminView,
+ POTemplateEditView,
+ )
+
+
+class TestPOTemplateEditViewValidation(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -29,6 +32,19 @@
data.update(**kwargs)
return data
+ def test_detects_invalid_names(self):
+ # A template name must be satisfying the valid_name constraint.
+ invalid_name = 'name!'
+ potemplate = self.factory.makePOTemplate()
+ data = self._makeData(potemplate, name=invalid_name)
+ view = POTemplateEditView(potemplate, LaunchpadTestRequest())
+ view.validate(data)
+ self.assertEqual(
+ [u'Template name can only start with lowercase letters a-z '
+ u'or digits 0-9, and other than those characters, can only '
+ u'contain "-", "+" and "." characters.'],
+ view.errors)
+
def test_detects_name_clash_on_name_change(self):
# A template name may not already be used.
existing_name = self.factory.getUniqueString()
@@ -36,7 +52,7 @@
series = existing_potemplate.productseries
potemplate = self.factory.makePOTemplate(productseries=series)
- view = POTemplateAdminView(potemplate, LaunchpadTestRequest())
+ view = POTemplateEditView(potemplate, LaunchpadTestRequest())
data = self._makeData(potemplate, name=existing_name)
view.validate(data)
self.assertEqual([u'Name is already in use.'], view.errors)
@@ -49,11 +65,50 @@
series = existing_potemplate.productseries
potemplate = self.factory.makePOTemplate(productseries=series)
- view = POTemplateAdminView(potemplate, LaunchpadTestRequest())
+ view = POTemplateEditView(potemplate, LaunchpadTestRequest())
data = self._makeData(potemplate, translation_domain=existing_domain)
view.validate(data)
self.assertEqual([u'Domain is already in use.'], view.errors)
+ def test_detects_name_clash_on_sourcepackage_change(self):
+ # Detect changing to a source package that already has a template of
+ # the same name.
+ sourcepackage = self.factory.makeSourcePackage()
+ existing_potemplate = self.factory.makePOTemplate(
+ sourcepackage=sourcepackage)
+ potemplate = self.factory.makePOTemplate(
+ distroseries=sourcepackage.distroseries,
+ name=existing_potemplate.name)
+
+ view = POTemplateEditView(potemplate, LaunchpadTestRequest())
+ data = self._makeData(
+ potemplate, sourcepackagename=sourcepackage.sourcepackagename)
+ view.validate(data)
+ self.assertEqual(
+ [u'Source package already has a template with that same name.'],
+ view.errors)
+
+ def test_detects_domain_clash_on_sourcepackage_change(self):
+ # Detect changing to a source package that already has a template with
+ # the same translation domain.
+ sourcepackage = self.factory.makeSourcePackage()
+ existing_potemplate = self.factory.makePOTemplate(
+ sourcepackage=sourcepackage)
+ potemplate = self.factory.makePOTemplate(
+ distroseries=sourcepackage.distroseries,
+ translation_domain=existing_potemplate.translation_domain)
+
+ view = POTemplateEditView(potemplate, LaunchpadTestRequest())
+ data = self._makeData(
+ potemplate, sourcepackagename=sourcepackage.sourcepackagename)
+ view.validate(data)
+ self.assertEqual(
+ [u'Source package already has a template with that same domain.'],
+ view.errors)
+
+
+class TestPOTemplateAdminViewValidation(TestPOTemplateEditViewValidation):
+
def test_detects_name_clash_on_productseries_change(self):
# Detect changing to a productseries that already has a template of
# the same name.
@@ -86,42 +141,6 @@
[u'Series already has a template with that same domain.'],
view.errors)
- def test_detects_name_clash_on_sourcepackage_change(self):
- # Detect changing to a source package that already has a template of
- # the same name.
- sourcepackage = self.factory.makeSourcePackage()
- existing_potemplate = self.factory.makePOTemplate(
- sourcepackage=sourcepackage)
- potemplate = self.factory.makePOTemplate(
- distroseries=sourcepackage.distroseries,
- name=existing_potemplate.name)
-
- view = POTemplateAdminView(potemplate, LaunchpadTestRequest())
- data = self._makeData(
- potemplate, sourcepackagename=sourcepackage.sourcepackagename)
- view.validate(data)
- self.assertEqual(
- [u'Source package already has a template with that same name.'],
- view.errors)
-
- def test_detects_domain_clash_on_sourcepackage_change(self):
- # Detect changing to a productseries that already has a template with
- # the same translation domain.
- sourcepackage = self.factory.makeSourcePackage()
- existing_potemplate = self.factory.makePOTemplate(
- sourcepackage=sourcepackage)
- potemplate = self.factory.makePOTemplate(
- distroseries=sourcepackage.distroseries,
- translation_domain=existing_potemplate.translation_domain)
-
- view = POTemplateAdminView(potemplate, LaunchpadTestRequest())
- data = self._makeData(
- potemplate, sourcepackagename=sourcepackage.sourcepackagename)
- view.validate(data)
- self.assertEqual(
- [u'Source package already has a template with that same domain.'],
- view.errors)
-
def test_detects_no_sourcepackage_or_productseries(self):
# Detect if no source package or productseries was selected.
potemplate = self.factory.makePOTemplate()
=== modified file 'lib/lp/translations/configure.zcml'
--- lib/lp/translations/configure.zcml 2011-07-30 14:21:23 +0000
+++ lib/lp/translations/configure.zcml 2011-08-23 11:39:41 +0000
@@ -431,15 +431,15 @@
<allow
interface="lp.translations.interfaces.potemplate.IPOTemplate"/>
<require
- permission="launchpad.Edit"
- set_attributes="
- owner priority description translation_domain path
- iscurrent"/>
- <require
permission="launchpad.TranslationsAdmin"
set_attributes="
- name productseries distroseries sourcepackagename
- sourcepackageversion binarypackagename languagepack
+ name priority description translation_domain path
+ iscurrent languagepack sourcepackagename"/>
+ <require
+ permission="launchpad.Admin"
+ set_attributes="
+ owner productseries distroseries
+ sourcepackageversion binarypackagename
source_file_format source_file date_last_updated
from_sourcepackagename header"/>
</class>
=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py 2011-07-28 12:59:26 +0000
+++ lib/lp/translations/interfaces/potemplate.py 2011-08-23 11:39:41 +0000
@@ -33,6 +33,7 @@
from canonical.launchpad import _
from canonical.launchpad.interfaces.librarian import ILibraryFileAlias
from lp.app.errors import NotFoundError
+from lp.app.validators.name import valid_name
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.interfaces.sourcepackagename import ISourcePackageName
@@ -112,7 +113,7 @@
"unique name in its package. It's important to get this "
"correct, because Launchpad will recommend alternative "
"translations based on the name."),
- required=True))
+ constraint=valid_name, required=True))
translation_domain = exported(TextLine(
title=_("Translation domain"),
=== modified file 'lib/lp/translations/templates/potemplate-index.pt'
--- lib/lp/translations/templates/potemplate-index.pt 2011-03-08 09:59:36 +0000
+++ lib/lp/translations/templates/potemplate-index.pt 2011-08-23 11:39:41 +0000
@@ -28,10 +28,6 @@
<strong>Owner:</strong>
<a tal:replace="structure context/owner/fmt:link">owner name</a>
</p>
- <div>
- <a href="+edit" class="edit sprite"
- tal:condition="context/required:launchpad.Edit">Change details</a>
- </div>
</div>
<div class="yui-g">
@@ -119,11 +115,16 @@
translation tarballs.
</p>
<div tal:condition="context/required:launchpad.TranslationsAdmin">
- <a tal:attributes="href
- context/menu:navigation/administer/url"
+ <a href="+edit" class="edit sprite"
+ tal:condition="context/required:launchpad.TranslationsAdmin">
+ Change details</a>
+ <tal:admin condition="context/required:launchpad.Admin">
+ — <a tal:attributes="href
+ context/menu:navigation/administer/url"
class="edit sprite">
- Administer this template
- </a>
+ Administer this template
+ </a>
+ </tal:admin>
</div>
</div>
</div>
Follow ups
