launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15406
[Merge] lp:~stevenk/launchpad/validate-milestone-tags into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/validate-milestone-tags into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1159619 in Launchpad itself: "milestonetag validation is only performed at the DB-level"
https://bugs.launchpad.net/launchpad/+bug/1159619
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/validate-milestone-tags/+merge/155171
Validate tags during {Product,Distribution}Series:+addmilestone, Milestone:+edit, and IMilestone.setTags(), rather than relying on the database constraint firing which will result in an OOPS.
I also moved validate_tags() from milestone's browser code, and put it with milestonetag's model code, since the places that were pulling it in import from there anyway.
Deal with some whitespace.
--
https://code.launchpad.net/~stevenk/launchpad/validate-milestone-tags/+merge/155171
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/validate-milestone-tags into lp:launchpad.
=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py 2012-12-20 14:55:13 +0000
+++ lib/lp/registry/browser/milestone.py 2013-03-25 06:17:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Milestone views."""
@@ -21,7 +21,6 @@
'MilestoneView',
'MilestoneViewMixin',
'ObjectMilestonesView',
- 'validate_tags',
]
@@ -46,7 +45,6 @@
LaunchpadFormView,
safe_action,
)
-from lp.app.validators.name import valid_name
from lp.app.widgets.date import DateWidget
from lp.bugs.browser.bugtask import BugTaskListingItem
from lp.bugs.browser.structuralsubscription import (
@@ -72,7 +70,10 @@
from lp.registry.interfaces.milestonetag import IProjectGroupMilestoneTag
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProduct
-from lp.registry.model.milestonetag import ProjectGroupMilestoneTag
+from lp.registry.model.milestonetag import (
+ ProjectGroupMilestoneTag,
+ validate_tags,
+ )
from lp.services.propertycache import cachedproperty
from lp.services.webapp import (
canonical_url,
@@ -259,12 +260,9 @@
tags = self._bug_task_tags.get(bugtask.id, ())
people = self._bug_task_people
return BugTaskListingItem(
- bugtask,
- badge_property['has_branch'],
- badge_property['has_specification'],
- badge_property['has_patch'],
- tags,
- people)
+ bugtask, badge_property['has_branch'],
+ badge_property['has_specification'], badge_property['has_patch'],
+ tags, people)
@cachedproperty
def bugtasks(self):
@@ -346,8 +344,7 @@
a project milestone tag, else return False."""
return (
IProjectGroupMilestone.providedBy(self.context) or
- self.is_project_milestone_tag
- )
+ self.is_project_milestone_tag)
@property
def has_bugs_or_specs(self):
@@ -428,7 +425,27 @@
should_show_bugs_and_blueprints = False
-class MilestoneAddView(LaunchpadFormView):
+class MilestoneTagBase:
+
+ def extendFields(self):
+ """See `LaunchpadFormView`.
+
+ Add a text-entry widget for milestone tags since there is not property
+ on the interface.
+ """
+ tag_entry = TextLine(
+ __name__='tags', title=u'Tags', required=False,
+ constraint=lambda value: validate_tags(value.split()))
+ self.form_fields += form.Fields(
+ tag_entry, render_context=self.render_context)
+ # Make an instance attribute to avoid mutating the class attribute.
+ self.field_names = getattr(self, '_field_names', self.field_names)[:]
+ # Insert the tags field before the summary.
+ summary_index = self.field_names.index('summary')
+ self.field_names.insert(summary_index, tag_entry.__name__)
+
+
+class MilestoneAddView(MilestoneTagBase, LaunchpadFormView):
"""A view for creating a new Milestone."""
schema = IMilestone
@@ -438,22 +455,7 @@
custom_widget('dateexpected', DateWidget)
def extendFields(self):
- """See `LaunchpadFormView`.
-
- Add a text-entry widget for milestone tags since there is not property
- on the interface.
- """
- tag_entry = TextLine(
- __name__='tags',
- title=u'Tags',
- required=False)
- self.form_fields += form.Fields(
- tag_entry, render_context=self.render_context)
- # Make an instance attribute to avoid mutating the class attribute.
- self.field_names = self.field_names[:]
- # Insert the tags field before the summary.
- summary_index = self.field_names.index('summary')
- self.field_names.insert(summary_index, tag_entry.__name__)
+ super(MilestoneAddView, self).extendFields()
@action(_('Register Milestone'), name='register')
def register_action(self, action, data):
@@ -479,7 +481,7 @@
return canonical_url(self.context)
-class MilestoneEditView(LaunchpadEditFormView):
+class MilestoneEditView(MilestoneTagBase, LaunchpadEditFormView):
"""A view for editing milestone properties.
This view supports editing of properties such as the name, the date it is
@@ -516,27 +518,10 @@
@property
def initial_values(self):
- tags = self.context.getTags()
- tagstring = ' '.join(tags)
- return dict(tags=tagstring)
+ return {'tags': u' '.join(self.context.getTags())}
def extendFields(self):
- """See `LaunchpadFormView`.
-
- Add a text-entry widget for milestone tags since there is not property
- on the interface.
- """
- tag_entry = TextLine(
- __name__='tags',
- title=u'Tags',
- required=False)
- self.form_fields += form.Fields(
- tag_entry, render_context=self.render_context)
- # Make an instance attribute to avoid mutating the class attribute.
- self.field_names = self._field_names[:]
- # Insert the tags field before the summary.
- summary_index = self.field_names.index('summary')
- self.field_names.insert(summary_index, tag_entry.__name__)
+ super(MilestoneEditView, self).extendFields()
def setUpFields(self):
"""See `LaunchpadFormView`.
@@ -614,14 +599,6 @@
self.next_url = canonical_url(series)
-def validate_tags(tags):
- """Check that `separator` separated `tags` are valid tag names."""
- return (
- all(valid_name(tag) for tag in tags) and
- len(set(tags)) == len(tags)
- )
-
-
class ISearchMilestoneTagsForm(Interface):
"""Schema for the search milestone tags form."""
=== modified file 'lib/lp/registry/browser/project.py'
--- lib/lp/registry/browser/project.py 2012-10-12 01:56:34 +0000
+++ lib/lp/registry/browser/project.py 2013-03-25 06:17:46 +0000
@@ -76,7 +76,6 @@
IRegistryCollectionNavigationMenu,
RegistryCollectionActionMenuBase,
)
-from lp.registry.browser.milestone import validate_tags
from lp.registry.browser.objectreassignment import ObjectReassignmentView
from lp.registry.browser.pillar import PillarViewMixin
from lp.registry.browser.product import (
@@ -90,7 +89,10 @@
IProjectGroupSeries,
IProjectGroupSet,
)
-from lp.registry.model.milestonetag import ProjectGroupMilestoneTag
+from lp.registry.model.milestonetag import (
+ ProjectGroupMilestoneTag,
+ validate_tags,
+ )
from lp.services.feeds.browser import FeedsMixin
from lp.services.fields import (
PillarAliases,
=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2013-01-25 05:59:00 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2013-03-25 06:17:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test milestone views."""
@@ -156,10 +156,9 @@
layer = DatabaseFunctionalLayer
def setUp(self):
- TestCaseWithFactory.setUp(self)
+ super(TestAddMilestoneViews, self).setUp()
self.product = self.factory.makeProduct()
- self.series = (
- self.factory.makeProductSeries(product=self.product))
+ self.series = self.factory.makeProductSeries(product=self.product)
self.owner = self.product.owner
login_person(self.owner)
@@ -168,8 +167,7 @@
'field.name': '1.1',
'field.actions.register': 'Register Milestone',
}
- view = create_initialized_view(
- self.series, '+addmilestone', form=form)
+ view = create_initialized_view(self.series, '+addmilestone', form=form)
self.assertEqual([], view.errors)
def test_add_milestone_with_good_date(self):
@@ -178,8 +176,7 @@
'field.dateexpected': '2010-10-10',
'field.actions.register': 'Register Milestone',
}
- view = create_initialized_view(
- self.series, '+addmilestone', form=form)
+ view = create_initialized_view(self.series, '+addmilestone', form=form)
# It's important to make sure no errors occured,
# but also confirm that the milestone was created.
self.assertEqual([], view.errors)
@@ -191,8 +188,7 @@
'field.dateexpected': '1010-10-10',
'field.actions.register': 'Register Milestone',
}
- view = create_initialized_view(
- self.series, '+addmilestone', form=form)
+ view = create_initialized_view(self.series, '+addmilestone', form=form)
error_msg = view.errors[0].errors[0]
expected_msg = (
"Date could not be formatted. Provide a date formatted "
@@ -206,19 +202,27 @@
'field.tags': tags,
'field.actions.register': 'Register Milestone',
}
- view = create_initialized_view(
- self.series, '+addmilestone', form=form)
+ view = create_initialized_view(self.series, '+addmilestone', form=form)
self.assertEqual([], view.errors)
expected = sorted(tags.split())
self.assertEqual(expected, self.product.milestones[0].getTags())
+ def test_add_milestone_with_invalid_tags(self):
+ form = {
+ 'field.name': '1.1',
+ 'field.tags': '&%&%*',
+ 'field.actions.register': 'Register Milestone',
+ }
+ view = create_initialized_view(self.series, '+addmilestone', form=form)
+ self.assertEqual(1, len(view.errors))
+
class TestMilestoneEditView(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
def setUp(self):
- TestCaseWithFactory.setUp(self)
+ super(TestMilestoneEditView, self).setUp()
self.product = self.factory.makeProduct()
self.milestone = self.factory.makeMilestone(
name='orig-name', product=self.product)
@@ -226,35 +230,38 @@
login_person(self.owner)
def test_edit_milestone_with_tags(self):
- orig_tags = u'b a c'
+ orig_tags = u'ba ac'
self.milestone.setTags(orig_tags.split(), self.owner)
- new_tags = u'z a B'
+ new_tags = u'za ab'
form = {
'field.name': 'new-name',
'field.tags': new_tags,
'field.actions.update': 'Update',
}
- view = create_initialized_view(
- self.milestone, '+edit', form=form)
+ view = create_initialized_view(self.milestone, '+edit', form=form)
self.assertEqual([], view.errors)
self.assertEqual('new-name', self.milestone.name)
expected = sorted(new_tags.lower().split())
self.assertEqual(expected, self.milestone.getTags())
def test_edit_milestone_clear_tags(self):
- orig_tags = u'b a c'
+ orig_tags = u'ba ac'
self.milestone.setTags(orig_tags.split(), self.owner)
form = {
'field.name': 'new-name',
'field.tags': '',
'field.actions.update': 'Update',
}
+ view = create_initialized_view(self.milestone, '+edit', form=form)
+ self.assertEqual([], view.errors)
+ self.assertEqual('new-name', self.milestone.name)
+ self.assertEqual([], self.milestone.getTags())
+
+ def test_edit_milestone_with_invalid_tags(self):
+ form = {'field.tags': '&%$^', 'field.actions.update': 'Update'}
view = create_initialized_view(
self.milestone, '+edit', form=form)
- self.assertEqual([], view.errors)
- self.assertEqual('new-name', self.milestone.name)
- expected = []
- self.assertEqual(expected, self.milestone.getTags())
+ self.assertEqual(1, len(view.errors))
class TestMilestoneDeleteView(TestCaseWithFactory):
=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py 2013-01-25 04:28:40 +0000
+++ lib/lp/registry/model/milestone.py 2013-03-25 06:17:46 +0000
@@ -139,6 +139,14 @@
super(MultipleProductReleases, self).__init__(msg)
+@error_status(httplib.BAD_REQUEST)
+class InvalidTags(Exception):
+ """Raised when tags are invalid."""
+
+ def __init__(self, msg='Tags are invalid.'):
+ super(InvalidTags, self).__init__(msg)
+
+
class MilestoneData:
implements(IMilestoneData)
@@ -312,7 +320,7 @@
assert self.product_release is None, (
"You cannot delete a milestone which has a product release "
"associated with it.")
- SQLBase.destroySelf(self)
+ super(Milestone, self).destroySelf()
def getBugSummaryContextWhereClause(self):
"""See BugTargetBase."""
@@ -323,9 +331,11 @@
def setTags(self, tags, user):
"""See IMilestone."""
# Circular reference prevention.
- from lp.registry.model.milestonetag import MilestoneTag
+ from lp.registry.model.milestonetag import MilestoneTag, validate_tags
store = Store.of(self)
if tags:
+ if not validate_tags(tags):
+ raise InvalidTags()
current_tags = set(self.getTags())
new_tags = set(tags)
if new_tags == current_tags:
=== modified file 'lib/lp/registry/model/milestonetag.py'
--- lib/lp/registry/model/milestonetag.py 2012-10-18 16:49:57 +0000
+++ lib/lp/registry/model/milestonetag.py 2013-03-25 06:17:46 +0000
@@ -7,6 +7,7 @@
__all__ = [
'MilestoneTag',
'ProjectGroupMilestoneTag',
+ 'validate_tags',
]
@@ -23,6 +24,7 @@
from storm.references import Reference
from zope.interface import implements
+from lp.app.validators.name import valid_name
from lp.registry.interfaces.milestonetag import IProjectGroupMilestoneTag
from lp.registry.model.milestone import (
Milestone,
@@ -93,3 +95,10 @@
Milestone.productID == Product.id,
Product.project == self.target,
tag_constraints))
+
+
+def validate_tags(tags):
+ """Check that `separator` separated `tags` are valid tag names."""
+ return (
+ all(valid_name(tag) for tag in tags) and
+ len(set(tags)) == len(tags))
=== modified file 'lib/lp/registry/tests/test_milestonetag.py'
--- lib/lp/registry/tests/test_milestonetag.py 2012-10-18 16:49:57 +0000
+++ lib/lp/registry/tests/test_milestonetag.py 2013-03-25 06:17:46 +0000
@@ -7,6 +7,7 @@
import datetime
+from lazr.restfulclient.errors import BadRequest
import transaction
from lp.registry.model.milestonetag import (
@@ -243,3 +244,7 @@
self.ws_milestone.lp_save()
transaction.begin()
self.assertEqual(sorted(tags2), self.milestone.getTags())
+
+ def test_set_tags_invalid(self):
+ self.assertRaises(
+ BadRequest, self.ws_milestone.setTags, tags=[u'&%&%^&'])
Follow ups
