launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02328
[Merge] lp:~allenap/launchpad/sub-search-ui-bug-656823-5 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/sub-search-ui-bug-656823-5 into lp:launchpad with lp:~allenap/launchpad/sub-search-ui-bug-656823-4-cleanup as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#656823 Subscribing to a search lacks a UI
https://bugs.launchpad.net/bugs/656823
For more details, see:
https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-5/+merge/46179
This branch adds links to create new bug subscription filters, and the
forms necessary to populate filters initially.
- BugSubscriptionFilterCreateView is the form, and is registered as
IStructuralSubscription:+new-filter. It is subclassed from the +edit
view.
For some reason the actions from the +edit view are not inherited. I
am a bit suspicious about that. I have one more branch in this
series and I suspect I'll refactor everything but the actions into a
new base class just to make it unambiguous.
- I have fixed the "(edit)" links to only appear when the logged-in
user actually has the ability to edit the bug filter.
- I have removed the template for +edit and used generic-edit.pt
instead.
- I discovered that StructuralSubscription.newBugFilter() could be
called by any logged-in user, which isn't right. I've split
IStructuralSubscription up into *Public and *Restricted interfaces,
changed the security declaration zcml, and added a new security
adapter - EditStructuralSubscription - to take care of this.
This security adapter is in a new lp.registry.security module, which
I have also registered. This module is now ready and waiting for
anyone to move other Registry security adapters over from
canonical.launchpad.security.
- The new links to create bug filters have been added to the template
and only appear when the user has permission to do so; i.e. to call
newBugFilter().
--
https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-5/+merge/46179
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/sub-search-ui-bug-656823-5 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/bugsubscriptionfilter.py 2011-01-10 15:23:31 +0000
+++ lib/lp/bugs/browser/bugsubscriptionfilter.py 2011-01-13 20:41:59 +0000
@@ -104,3 +104,18 @@
self.user, view_name="+structural-subscriptions")
cancel_url = next_url
+
+
+class BugSubscriptionFilterCreateView(BugSubscriptionFilterEditView):
+
+ page_title = u"Create new filter"
+
+ # The context does not correspond to the thing we're creating - which,
+ # somewhat obviously, doesn't exist yet - so don't even try to render it.
+ render_context = False
+
+ @action("Create", name="create")
+ def create_action(self, action, data):
+ """Create a new bug filter with the form data."""
+ bug_filter = self.context.newBugFilter()
+ self.updateContextFromData(data, context=bug_filter)
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml 2011-01-10 14:20:13 +0000
+++ lib/lp/bugs/browser/configure.zcml 2011-01-13 20:41:59 +0000
@@ -1192,9 +1192,15 @@
<browser:page
for="lp.bugs.interfaces.bugsubscriptionfilter.IBugSubscriptionFilter"
class=".bugsubscriptionfilter.BugSubscriptionFilterEditView"
- template="../templates/bug-subscription-filter-edit.pt"
+ template="../../app/templates/generic-edit.pt"
permission="launchpad.Edit"
name="+edit" />
+ <browser:page
+ for="lp.registry.interfaces.structuralsubscription.IStructuralSubscription"
+ class=".bugsubscriptionfilter.BugSubscriptionFilterCreateView"
+ template="../../app/templates/generic-edit.pt"
+ permission="launchpad.View"
+ name="+new-filter" />
</facet>
<!-- Widgets -->
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-01-13 20:41:58 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-01-13 20:41:59 +0000
@@ -25,11 +25,11 @@
BugTaskImportance,
BugTaskStatus,
)
-from lp.bugs.publisher import BugsLayer
from lp.registry.browser.structuralsubscription import (
StructuralSubscriptionNavigation,
)
from lp.testing import (
+ anonymous_logged_in,
login_person,
normalize_whitespace,
person_logged_in,
@@ -308,7 +308,7 @@
# the absense of conditions.
self.assertRender(
u"This filter does not allow mail through.",
- u"There are no filter conditions! (edit)")
+ u"There are no filter conditions!")
def test_render_with_no_description_and_conditions(self):
# If conditions are set but no description, the rendered description
@@ -327,7 +327,7 @@
self.subscription_filter.tags = [u"foo", u"bar"]
self.assertRender(
u"This filter allows mail through when:",
- u" and ".join(self.view.conditions) + u" (edit)")
+ u" and ".join(self.view.conditions))
def test_render_with_description_and_no_conditions(self):
# If a description is set it appears in the content of the dt tag,
@@ -336,7 +336,7 @@
self.subscription_filter.description = u"The Wait"
self.assertRender(
u"\u201cThe Wait\u201d does not allow mail through.",
- u"There are no filter conditions! (edit)")
+ u"There are no filter conditions!")
def test_render_with_description_and_conditions(self):
# If a description is set it appears in the content of the dt tag,
@@ -346,7 +346,34 @@
self.subscription_filter.tags = [u"foo"]
self.assertRender(
u"\u201cThe Wait\u201d allows mail through when:",
- u" and ".join(self.view.conditions) + u" (edit)")
+ u" and ".join(self.view.conditions))
+
+ def findEditLinks(self, view):
+ root = html.fromstring(view.render())
+ return [
+ node for node in root.findall("dd//a")
+ if node.get("href").endswith("/+edit")]
+
+ def test_edit_link_for_subscriber(self):
+ # A link to edit the filter is rendered for the subscriber.
+ with person_logged_in(self.subscription.subscriber):
+ subscriber_view = create_initialized_view(
+ self.subscription_filter, "+definition")
+ self.assertNotEqual([], self.findEditLinks(subscriber_view))
+
+ def test_edit_link_for_non_subscriber(self):
+ # A link to edit the filter is *not* rendered for anyone but the
+ # subscriber.
+ with person_logged_in(self.factory.makePerson()):
+ non_subscriber_view = create_initialized_view(
+ self.subscription_filter, "+definition")
+ self.assertEqual([], self.findEditLinks(non_subscriber_view))
+
+ def test_edit_link_for_anonymous(self):
+ # A link to edit the filter is *not* rendered for anyone but the
+ # subscriber.
+ with anonymous_logged_in():
+ self.assertEqual([], self.findEditLinks(self.view))
class TestBugSubscriptionFilterEditView(
@@ -359,8 +386,7 @@
# subscription overview page.
login_person(self.owner)
view = create_initialized_view(
- self.subscription_filter, name="+edit",
- layer=BugsLayer, principal=self.owner)
+ self.subscription_filter, name="+edit")
self.assertEqual([], view.errors)
path = "/~%s/+structural-subscriptions" % self.owner.name
self.assertEqual(path, urlparse(view.cancel_url).path)
@@ -378,8 +404,7 @@
}
with person_logged_in(self.owner):
view = create_initialized_view(
- self.subscription_filter, name="+edit",
- form=form, layer=BugsLayer, principal=self.owner)
+ self.subscription_filter, name="+edit", form=form)
self.assertEqual([], view.errors)
# The subscription filter has been updated.
self.assertEqual(
@@ -396,3 +421,60 @@
self.subscription_filter.tags)
self.assertTrue(
self.subscription_filter.find_all_tags)
+
+
+class TestBugSubscriptionFilterCreateView(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestBugSubscriptionFilterCreateView, self).setUp()
+ self.owner = self.factory.makePerson(name=u"foo")
+ self.structure = self.factory.makeProduct(
+ owner=self.owner, name=u"bar")
+ with person_logged_in(self.owner):
+ self.subscription = self.structure.addBugSubscription(
+ self.owner, self.owner)
+
+ def test_view_properties(self):
+ # The cancel url and next url will both point to the user's structural
+ # subscription overview page.
+ login_person(self.owner)
+ view = create_initialized_view(
+ self.subscription, name="+new-filter")
+ self.assertEqual([], view.errors)
+ path = "/~%s/+structural-subscriptions" % self.owner.name
+ self.assertEqual(path, urlparse(view.cancel_url).path)
+ self.assertEqual(path, urlparse(view.next_url).path)
+
+ def test_create(self):
+ # New filters can be created with +new-filter.
+ self.assertEqual([], list(self.subscription.bug_filters))
+ form = {
+ "field.description": "New description",
+ "field.statuses": ["NEW", "INCOMPLETE"],
+ "field.importances": ["LOW", "MEDIUM"],
+ "field.tags": u"foo bar",
+ "field.find_all_tags": "on",
+ "field.actions.create": "Create",
+ }
+ with person_logged_in(self.owner):
+ view = create_initialized_view(
+ self.subscription, name="+new-filter", form=form)
+ self.assertEqual([], view.errors)
+ # The subscription filter has been created.
+ subscription_filter = self.subscription.bug_filters.one()
+ self.assertEqual(
+ u"New description",
+ subscription_filter.description)
+ self.assertEqual(
+ frozenset([BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE]),
+ subscription_filter.statuses)
+ self.assertEqual(
+ frozenset([BugTaskImportance.LOW, BugTaskImportance.MEDIUM]),
+ subscription_filter.importances)
+ self.assertEqual(
+ frozenset([u"foo", u"bar"]),
+ subscription_filter.tags)
+ self.assertTrue(
+ subscription_filter.find_all_tags)
=== removed file 'lib/lp/bugs/templates/bug-subscription-filter-edit.pt'
--- lib/lp/bugs/templates/bug-subscription-filter-edit.pt 2011-01-07 17:38:18 +0000
+++ lib/lp/bugs/templates/bug-subscription-filter-edit.pt 1970-01-01 00:00:00 +0000
@@ -1,14 +0,0 @@
-<html
- xmlns="http://www.w3.org/1999/xhtml";
- xmlns:tal="http://xml.zope.org/namespaces/tal";
- xmlns:metal="http://xml.zope.org/namespaces/metal";
- xmlns:i18n="http://xml.zope.org/namespaces/i18n";
- xml:lang="en" lang="en" dir="ltr"
- metal:use-macro="view/macro:page/main_only"
- i18n:domain="malone">
- <body>
- <div metal:fill-slot="main">
- <div metal:use-macro="context/@@launchpad_form/form" />
- </div>
- </body>
-</html>
=== modified file 'lib/lp/bugs/templates/bug-subscription-filter.pt'
--- lib/lp/bugs/templates/bug-subscription-filter.pt 2011-01-11 09:45:13 +0000
+++ lib/lp/bugs/templates/bug-subscription-filter.pt 2011-01-13 20:41:59 +0000
@@ -27,10 +27,14 @@
<b>and</b> <br />
</tal:conjunction>
</tal:conditions>
- <br /><a tal:attributes="href context/fmt:url/+edit">(edit)</a>
+ <tal:editable condition="context/required:launchpad.Edit">
+ <br /><a tal:attributes="href context/fmt:url/+edit">(edit)</a>
+ </tal:editable>
</dd>
<dd tal:condition="not:conditions">
There are no filter conditions!
- <br /><a tal:attributes="href context/fmt:url/+edit">(edit)</a>
+ <tal:editable condition="context/required:launchpad.Edit">
+ <br /><a tal:attributes="href context/fmt:url/+edit">(edit)</a>
+ </tal:editable>
</dd>
</dl>
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-01-05 08:10:10 +0000
+++ lib/lp/registry/configure.zcml 2011-01-13 20:41:59 +0000
@@ -10,6 +10,7 @@
xmlns:lp="http://namespaces.canonical.com/lp";
xmlns:webservice="http://namespaces.canonical.com/webservice";
i18n_domain="launchpad">
+ <authorizations module=".security" />
<include
file="vocabularies.zcml"/>
<include
@@ -1862,10 +1863,13 @@
<class
class="lp.registry.model.structuralsubscription.StructuralSubscription">
<allow
- interface="lp.registry.interfaces.structuralsubscription.IStructuralSubscription"/>
- <require
- permission="zope.Public"
- set_schema="lp.registry.interfaces.structuralsubscription.IStructuralSubscription"/>
+ interface=".interfaces.structuralsubscription.IStructuralSubscriptionPublic" />
+ <require
+ set_schema=".interfaces.structuralsubscription.IStructuralSubscriptionPublic"
+ permission="zope.Public" />
+ <require
+ interface=".interfaces.structuralsubscription.IStructuralSubscriptionRestricted"
+ permission="launchpad.Edit" />
</class>
<!-- PersonNotification -->
=== modified file 'lib/lp/registry/interfaces/structuralsubscription.py'
--- lib/lp/registry/interfaces/structuralsubscription.py 2010-12-21 09:42:47 +0000
+++ lib/lp/registry/interfaces/structuralsubscription.py 2011-01-13 20:41:59 +0000
@@ -83,9 +83,8 @@
""")
-class IStructuralSubscription(Interface):
- """A subscription to a Launchpad structure."""
- export_as_webservice_entry()
+class IStructuralSubscriptionPublic(Interface):
+ """The public parts of a subscription to a Launchpad structure."""
id = Int(title=_('ID'), readonly=True, required=True)
product = Int(title=_('Product'), required=False, readonly=True)
@@ -134,11 +133,22 @@
readonly=True, required=False,
value_type=Reference(schema=Interface)))
+
+class IStructuralSubscriptionRestricted(Interface):
+ """The restricted parts of a subscription to a Launchpad structure."""
+
@export_factory_operation(Interface, [])
def newBugFilter():
"""Returns a new `BugSubscriptionFilter` for this subscription."""
+class IStructuralSubscription(
+ IStructuralSubscriptionPublic, IStructuralSubscriptionRestricted):
+ """A subscription to a Launchpad structure."""
+
+ export_as_webservice_entry()
+
+
class IStructuralSubscriptionTargetRead(Interface):
"""A Launchpad Structure allowing users to subscribe to it.
=== added file 'lib/lp/registry/security.py'
--- lib/lp/registry/security.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/security.py 2011-01-13 20:41:59 +0000
@@ -0,0 +1,23 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Security adapters for Registry."""
+
+__metaclass__ = type
+__all__ = []
+
+from canonical.launchpad.security import AuthorizationBase
+from lp.registry.interfaces.structuralsubscription import (
+ IStructuralSubscription,
+ )
+
+
+class EditStructuralSubscription(AuthorizationBase):
+ """Edit permissions for `IStructuralSubscription`."""
+
+ usedfor = IStructuralSubscription
+ permission = "launchpad.Edit"
+
+ def checkAuthenticated(self, user):
+ """Subscribers can edit their own structural subscriptions."""
+ return user.inTeam(self.obj.subscriber)
=== modified file 'lib/lp/registry/stories/person/xx-person-subscriptions.txt'
--- lib/lp/registry/stories/person/xx-person-subscriptions.txt 2011-01-11 21:14:36 +0000
+++ lib/lp/registry/stories/person/xx-person-subscriptions.txt 2011-01-13 20:41:59 +0000
@@ -171,7 +171,7 @@
>>> subscriptions = find_tag_by_id(
... admin_browser.contents, 'structural-subscriptions')
>>> for subscription in subscriptions.findAll("li"):
- ... structure_link, modify_link = subscription.findAll("a")
+ ... structure_link, modify_link = subscription.findAll("a")[:2]
... print "%s <%s>" % (
... extract_text(structure_link), structure_link.get("href"))
... print "--> %s" % modify_link.get("href")
@@ -188,7 +188,7 @@
>>> subscriptions = find_tag_by_id(
... subscriber_browser.contents, 'structural-subscriptions')
>>> for subscription in subscriptions.findAll("li"):
- ... [structure_link] = subscription.findAll("a")
+ ... structure_link = subscription.find("a")
... print "%s <%s>" % (
... extract_text(structure_link), structure_link.get("href"))
mozilla-firefox in ubuntu </ubuntu/+source/mozilla-firefox>
@@ -210,6 +210,48 @@
Webster does not have any structural subscriptions.
+Creating Bug Filters
+~~~~~~~~~~~~~~~~~~~~
+
+Every structural subscription also has a link to create a new bug
+subscription filter.
+
+ >>> import re
+
+ >>> def show_create_links(browser):
+ ... subscriptions = find_tag_by_id(
+ ... browser.contents, 'structural-subscriptions')
+ ... for subscription in subscriptions.findAll("li"):
+ ... structure_link = subscription.find("a")
+ ... print extract_text(structure_link)
+ ... create_text = subscription.find(text=re.compile("Create"))
+ ... if create_text is None:
+ ... print "* No create link."
+ ... else:
+ ... print "* %s --> %s" % (
+ ... create_text.strip(),
+ ... create_text.parent.get("href"))
+
+ >>> admin_browser.open(
+ ... "http://launchpad.dev/people/+me/+structural-subscriptions";)
+ >>> show_create_links(admin_browser)
+ mozilla-firefox in ubuntu
+ * Create a new filter --> /ubuntu/.../name16/+new-filter
+ pmount in ubuntu
+ * Create a new filter --> /ubuntu/.../name16/+new-filter
+
+If the user does not have the necessary rights to create new bug
+filters the "Create" link is not shown.
+
+ >>> subscriber_browser.open(
+ ... "http://launchpad.dev/~name16/+structural-subscriptions";)
+ >>> show_create_links(subscriber_browser)
+ mozilla-firefox in ubuntu
+ * No create link.
+ pmount in ubuntu
+ * No create link.
+
+
Subscriptions with Bug Filters
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -234,7 +276,9 @@
... subscriptions = find_tag_by_id(
... nigel_browser.contents, 'structural-subscriptions')
... for subscription in subscriptions.findAll("li"):
- ... print extract_text(subscription.div)
+ ... print extract_text(subscription.p)
+ ... if subscription.dl is not None:
+ ... print extract_text(subscription.dl)
>>> show_nigels_subscriptions()
Bug mail is not filtered; mail for Nigel about Scofflaw will
=== modified file 'lib/lp/registry/templates/person-structural-subscriptions.pt'
--- lib/lp/registry/templates/person-structural-subscriptions.pt 2011-01-07 16:01:55 +0000
+++ lib/lp/registry/templates/person-structural-subscriptions.pt 2011-01-13 20:41:59 +0000
@@ -55,12 +55,21 @@
</dl>
</tal:filtered>
<tal:unfiltered condition="not:bug_filters">
- Bug mail is <b>not</b> filtered; mail for <span
- tal:content="subscription/subscriber/fmt:displayname" />
- about
- <span tal:content="subscription/target/displayname" />
- will always be sent.
+ <p>
+ Bug mail is <b>not</b> filtered; mail for <span
+ tal:content="subscription/subscriber/fmt:displayname" />
+ about
+ <span tal:content="subscription/target/displayname" />
+ will always be sent.
+ </p>
</tal:unfiltered>
+ <div style="margin-top: 1em"
+ tal:condition="subscription/required:launchpad.Edit">
+ <a class="sprite add new-filter"
+ tal:attributes="href subscription/fmt:url/+new-filter">
+ Create a new filter
+ </a>
+ </div>
</div>
</li>
</ul>
=== modified file 'lib/lp/registry/tests/test_structuralsubscription.py'
--- lib/lp/registry/tests/test_structuralsubscription.py 2010-10-01 16:22:14 +0000
+++ lib/lp/registry/tests/test_structuralsubscription.py 2011-01-13 20:41:59 +0000
@@ -5,37 +5,61 @@
__metaclass__ = type
-from canonical.testing.layers import LaunchpadZopelessLayer
+from zope.security.interfaces import Unauthorized
+
+from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ anonymous_logged_in,
+ person_logged_in,
+ TestCaseWithFactory,
+ )
class TestStructuralSubscription(TestCaseWithFactory):
- layer = LaunchpadZopelessLayer
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestStructuralSubscription, self).setUp()
+ self.product = self.factory.makeProduct()
+ with person_logged_in(self.product.owner):
+ self.subscription = self.product.addSubscription(
+ self.product.owner, self.product.owner)
def test_bug_filters_empty(self):
- # StructuralSubscription.filters returns the BugSubscriptionFilter
- # records associated with this subscription. It's empty to begin with.
- product = self.factory.makeProduct()
- subscription = product.addSubscription(product.owner, product.owner)
- self.assertEqual([], list(subscription.bug_filters))
+ # The bug_filters attribute is empty to begin with.
+ self.assertEqual([], list(self.subscription.bug_filters))
def test_bug_filters(self):
- # StructuralSubscription.filters returns the BugSubscriptionFilter
- # records associated with this subscription.
- product = self.factory.makeProduct()
- subscription = product.addSubscription(product.owner, product.owner)
+ # The bug_filters attribute returns the BugSubscriptionFilter records
+ # associated with this subscription.
subscription_filter = BugSubscriptionFilter()
- subscription_filter.structural_subscription = subscription
- self.assertEqual([subscription_filter], list(subscription.bug_filters))
+ subscription_filter.structural_subscription = self.subscription
+ self.assertEqual(
+ [subscription_filter],
+ list(self.subscription.bug_filters))
def test_newBugFilter(self):
- # Structural_Subscription.newBugFilter() creates a new subscription
- # filter linked to the subscription.
- product = self.factory.makeProduct()
- subscription = product.addSubscription(product.owner, product.owner)
- subscription_filter = subscription.newBugFilter()
- self.assertEqual(
- subscription, subscription_filter.structural_subscription)
- self.assertEqual([subscription_filter], list(subscription.bug_filters))
+ # newBugFilter() creates a new subscription filter linked to the
+ # subscription.
+ with person_logged_in(self.product.owner):
+ subscription_filter = self.subscription.newBugFilter()
+ self.assertEqual(
+ self.subscription,
+ subscription_filter.structural_subscription)
+ self.assertEqual(
+ [subscription_filter],
+ list(self.subscription.bug_filters))
+
+ def test_newBugFilter_by_anonymous(self):
+ # newBugFilter() is not available to anonymous users.
+ with anonymous_logged_in():
+ self.assertRaises(
+ Unauthorized, lambda: self.subscription.newBugFilter)
+
+ def test_newBugFilter_by_other_user(self):
+ # newBugFilter() is only available to the subscriber.
+ with person_logged_in(self.factory.makePerson()):
+ self.assertRaises(
+ Unauthorized, lambda: self.subscription.newBugFilter)
