launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03871
[Merge] lp:~wallyworld/launchpad/expose-blueprint-subscribe-methods into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/expose-blueprint-subscribe-methods into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/expose-blueprint-subscribe-methods/+merge/63865
This is the first of a series of branches to add an ajax implementation for subscribing/unsubscribing to/from blueprints. The work is needed to fix bug 50875 - allow a team to be unsubscribed from a blueprint.
== Implementation ==
The work done in this branch exposes blueprint methods via the web service api so they can be invoked using xhr calls:
ISpecification.subscribe
ISpecification.unsubscribe
ISpecificationSubscription.canBeUnsubscribedByUser
No code uses the above methods via the web api yet - that is done in subsequent branches.
== Tests ==
Add new tests:
For the canBeUnsubscribedByUser method:
lib/lp/blueprints/model/tests/test_subscription.py
For the web service apis:
lib/lp/blueprints/tests/test_webservice.py
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/blueprints/interfaces/specificationsubscription.py
lib/lp/blueprints/model/specification.py
lib/lp/blueprints/model/specificationsubscription.py
lib/lp/blueprints/model/tests/test_subscription.py
lib/lp/blueprints/tests/test_webservice.py
./lib/lp/blueprints/tests/test_webservice.py
186: local variable 'spec_object' is assigned to but never used
195: local variable 'spec_object' is assigned to but never used
206: local variable 'spec_object' is assigned to but never used
219: local variable 'spec_object' is assigned to but never used
--
https://code.launchpad.net/~wallyworld/launchpad/expose-blueprint-subscribe-methods/+merge/63865
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/expose-blueprint-subscribe-methods into lp:launchpad.
=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py 2011-06-07 04:39:03 +0000
+++ lib/lp/blueprints/browser/specification.py 2011-06-08 13:47:36 +0000
@@ -593,7 +593,7 @@
self.context.subscribe(self.user, self.user, essential)
self.notices.append('Your subscription has been updated.')
elif unsub is not None:
- self.context.unsubscribe(self.user)
+ self.context.unsubscribe(self.user, self.user)
self.notices.append(
"You have unsubscribed from this blueprint.")
=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py 2011-05-22 21:10:25 +0000
+++ lib/lp/blueprints/interfaces/specification.py 2011-06-08 13:47:36 +0000
@@ -55,6 +55,9 @@
SpecificationLifecycleStatus,
SpecificationPriority,
)
+from lp.blueprints.interfaces.specificationsubscription import (
+ ISpecificationSubscription,
+ )
from lp.blueprints.interfaces.specificationtarget import (
IHasSpecifications,
ISpecificationTarget,
@@ -63,6 +66,7 @@
from lp.bugs.interfaces.buglink import IBugLinkTarget
from lp.code.interfaces.branchlink import IHasLinkedBranches
from lp.registry.interfaces.milestone import IMilestone
+from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.projectgroup import IProjectGroup
from lp.registry.interfaces.role import IHasOwner
from lp.services.fields import (
@@ -467,10 +471,22 @@
def subscription(person):
"""Return the subscription for this person to this spec, or None."""
- def subscribe(person, essential=False):
+ @operation_parameters(
+ person=Reference(IPerson, title=_('Person'), required=True),
+ essential=copy_field(
+ ISpecificationSubscription['essential'], required=False))
+ @call_with(subscribed_by=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version('devel')
+ def subscribe(person, subscribed_by=None, essential=False):
"""Subscribe this person to the feature specification."""
- def unsubscribe(person):
+ @operation_parameters(
+ person=Reference(IPerson, title=_('Person'), required=False))
+ @call_with(unsubscribed_by=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version('devel')
+ def unsubscribe(person, unsubscribed_by):
"""Remove the person's subscription to this spec."""
def getSubscriptionByName(name):
=== modified file 'lib/lp/blueprints/interfaces/specificationsubscription.py'
--- lib/lp/blueprints/interfaces/specificationsubscription.py 2011-05-22 21:10:25 +0000
+++ lib/lp/blueprints/interfaces/specificationsubscription.py 2011-06-08 13:47:36 +0000
@@ -11,6 +11,13 @@
'ISpecificationSubscription',
]
+from lazr.restful.declarations import (
+ call_with,
+ export_as_webservice_entry,
+ export_read_operation,
+ operation_for_version,
+ REQUEST_USER,
+ )
from zope.interface import (
Attribute,
Interface,
@@ -27,6 +34,8 @@
class ISpecificationSubscription(Interface):
"""A subscription for a person to a specification."""
+ export_as_webservice_entry(publish_web_link=False, as_of='devel')
+
id = Int(
title=_('ID'), required=True, readonly=True)
person = PublicPersonChoice(
@@ -47,3 +56,8 @@
'attends meetings about this feature.'),
default=False)
+ @call_with(user=REQUEST_USER)
+ @export_read_operation()
+ @operation_for_version("devel")
+ def canBeUnsubscribedByUser(user):
+ """Can the user unsubscribe the subscriber from the specification?"""
=== modified file 'lib/lp/blueprints/interfaces/webservice.py'
--- lib/lp/blueprints/interfaces/webservice.py 2010-11-25 14:11:56 +0000
+++ lib/lp/blueprints/interfaces/webservice.py 2011-06-08 13:47:36 +0000
@@ -12,10 +12,14 @@
__all__ = [
'ISpecification',
'ISpecificationBranch',
+ 'ISpecificationSubscription',
]
from lp.blueprints.interfaces.specification import ISpecification
from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
+from lp.blueprints.interfaces.specificationsubscription import (
+ ISpecificationSubscription,
+ )
from lp.blueprints.interfaces.specificationtarget import ISpecificationTarget
# XXX: JonathanLange 2010-11-09 bug=673083: Legacy work-around for circular
# import bugs. Break this up into a per-package thing.
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2011-05-23 20:00:39 +0000
+++ lib/lp/blueprints/model/specification.py 2011-06-08 13:47:36 +0000
@@ -52,10 +52,7 @@
from canonical.launchpad.helpers import (
get_contact_email_addresses,
)
-from lp.services.propertycache import (
- cachedproperty,
- get_property_cache,
- )
+from lp.app.errors import UserCannotUnsubscribePerson
from lp.blueprints.adapters import SpecificationDelta
from lp.blueprints.enums import (
NewSpecificationDefinitionStatus,
@@ -93,6 +90,10 @@
from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.interfaces.product import IProduct
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
def recursive_blocked_query(spec):
@@ -534,9 +535,12 @@
return sub
return None
- def subscribe(self, person, user, essential):
- """Create or modify a user's subscription to this blueprint."""
- # first see if a relevant subscription exists, and if so, return it
+ def subscribe(self, person, subscribed_by=None, essential=False):
+ """See ISpecification."""
+ if subscribed_by is None:
+ subscribed_by = person
+ # Create or modify a user's subscription to this blueprint.
+ # First see if a relevant subscription exists, and if so, return it
sub = self.subscription(person)
if sub is not None:
if sub.essential != essential:
@@ -550,7 +554,7 @@
# that we can get away with not examining the attribute
# at all - it's a boolean!
notify(ObjectModifiedEvent(
- sub, sub, ['essential'], user=user))
+ sub, sub, ['essential'], user=subscribed_by))
return sub
# since no previous subscription existed, create and return a new one
sub = SpecificationSubscription(specification=self,
@@ -560,14 +564,21 @@
property_cache.subscriptions.append(sub)
property_cache.subscriptions.sort(
key=lambda sub: sub.person.displayname)
- notify(ObjectCreatedEvent(sub, user=user))
+ notify(ObjectCreatedEvent(sub, user=subscribed_by))
return sub
- def unsubscribe(self, person):
+ def unsubscribe(self, person, unsubscribed_by):
"""See ISpecification."""
# see if a relevant subscription exists, and if so, delete it
+ if person is None:
+ person = unsubscribed_by
for sub in self.subscriptions:
if sub.person.id == person.id:
+ if not sub.canBeUnsubscribedByUser(unsubscribed_by):
+ raise UserCannotUnsubscribePerson(
+ '%s does not have permission to unsubscribe %s.' % (
+ unsubscribed_by.displayname,
+ person.displayname))
get_property_cache(self).subscriptions.remove(sub)
SpecificationSubscription.delete(sub.id)
return
=== modified file 'lib/lp/blueprints/model/specificationsubscription.py'
--- lib/lp/blueprints/model/specificationsubscription.py 2011-05-23 16:57:13 +0000
+++ lib/lp/blueprints/model/specificationsubscription.py 2011-06-08 13:47:36 +0000
@@ -33,4 +33,10 @@
storm_validator=validate_public_person, notNull=True)
essential = BoolCol(notNull=True, default=False)
-
+ def canBeUnsubscribedByUser(self, user):
+ """See `ISpecificationSubscription`."""
+ if user is None:
+ return False
+ if self.person.is_team:
+ return user.inTeam(self.person)
+ return user == self.person
=== added file 'lib/lp/blueprints/model/tests/test_subscription.py'
--- lib/lp/blueprints/model/tests/test_subscription.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/model/tests/test_subscription.py 2011-06-08 13:47:36 +0000
@@ -0,0 +1,86 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.errors import UserCannotUnsubscribePerson
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+
+
+class TestSpecificationSubscription(TestCaseWithFactory):
+ """ Test whether a user can unsubscribe someone
+
+ As user can't unsubscribe just anyone from a spec. To check whether
+ someone can be unusubscribed, the canBeUnsubscribedByUser() method on
+ the SpecificationSubscription object is used.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def _make_subscription(self):
+ spec = self.factory.makeSpecification()
+ subscriber = self.factory.makePerson()
+ subscribed_by = self.factory.makePerson()
+ subscription = spec.subscribe(subscriber, subscribed_by)
+ return spec, subscriber, subscribed_by, subscription
+
+ def test_can_unsubscribe_self(self):
+ # The user can of course unsubscribe himself, even if someone else
+ # subscribed him.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ self.assertTrue(subscription.canBeUnsubscribedByUser(subscriber))
+
+ def test_subscriber_cannot_unsubscribe_user(self):
+ # The one who subscribed the subscriber doesn't have permission to
+ # unsubscribe him.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(subscribed_by))
+
+ def test_anonymous_cannot_unsubscribe(self):
+ # The anonymous user (represented by None) can't unsubscribe anyone.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(None))
+
+ def test_can_unsubscribe_team(self):
+ # A user can unsubscribe a team he's a member of.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ team = self.factory.makeTeam()
+ member = self.factory.makePerson()
+ with person_logged_in(member):
+ member.join(team)
+ subscription = spec.subscribe(team, subscribed_by)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(member))
+
+ non_member = self.factory.makePerson()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(non_member))
+
+ def test_cannot_unsubscribe_team(self):
+ # A user cannot unsubscribe a team he's a not member of.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ team = self.factory.makeTeam()
+ member = self.factory.makePerson()
+ with person_logged_in(member):
+ member.join(team)
+ subscription = spec.subscribe(team, subscribed_by)
+ non_member = self.factory.makePerson()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(non_member))
+
+ def test_unallowed_unsubscribe_raises(self):
+ # A spec's unsubscribe method uses canBeUnsubscribedByUser to check
+ # that the unsubscribing user has the appropriate permissions.
+ # unsubscribe will raise an exception if the user does not have
+ # permission.
+ (spec, subscriber,
+ subscribed_by, subscription) = self._make_subscription()
+ person = self.factory.makePerson()
+ self.assertRaises(
+ UserCannotUnsubscribePerson, spec.unsubscribe, subscriber, person)
=== modified file 'lib/lp/blueprints/tests/test_webservice.py'
--- lib/lp/blueprints/tests/test_webservice.py 2011-03-08 18:55:46 +0000
+++ lib/lp/blueprints/tests/test_webservice.py 2011-06-08 13:47:36 +0000
@@ -5,10 +5,19 @@
__metaclass__ = type
+import transaction
+
from zope.security.management import endInteraction
-from canonical.testing import DatabaseFunctionalLayer
-from canonical.launchpad.testing.pages import webservice_for_person
+from canonical.launchpad.testing.pages import (
+ LaunchpadWebServiceCaller,
+ webservice_for_person,
+ )
+from canonical.launchpad.webapp.interaction import ANONYMOUS
+from canonical.testing import (
+ AppServerLayer,
+ DatabaseFunctionalLayer,
+ )
from lp.blueprints.enums import SpecificationDefinitionStatus
from lp.testing import (
launchpadlib_for,
@@ -20,25 +29,21 @@
class SpecificationWebserviceTestCase(TestCaseWithFactory):
- def getLaunchpadlib(self):
- user = self.factory.makePerson()
- return launchpadlib_for("testing", user, version='devel')
-
def getSpecOnWebservice(self, spec_object):
- launchpadlib = self.getLaunchpadlib()
+ launchpadlib = self.factory.makeLaunchpadService()
return launchpadlib.load(
'/%s/+spec/%s' % (spec_object.target.name, spec_object.name))
def getPillarOnWebservice(self, pillar_obj):
# XXX: 2010-11-26, salgado, bug=681767: Can't use relative URLs here.
- launchpadlib = self.getLaunchpadlib()
+ launchpadlib = self.factory.makeLaunchpadService()
return launchpadlib.load(
str(launchpadlib._root_uri) + '/' + pillar_obj.name)
class SpecificationAttributeWebserviceTests(SpecificationWebserviceTestCase):
"""Test accessing specification attributes over the webservice."""
- layer = DatabaseFunctionalLayer
+ layer = AppServerLayer
def test_representation_is_empty_on_1_dot_0(self):
# ISpecification is exposed on the 1.0 version so that they can be
@@ -174,7 +179,7 @@
class SpecificationTargetTests(SpecificationWebserviceTestCase):
"""Tests for accessing specifications via their targets."""
- layer = DatabaseFunctionalLayer
+ layer = AppServerLayer
def test_get_specification_on_product(self):
product = self.factory.makeProduct(name="fooix")
@@ -266,3 +271,61 @@
distro_on_webservice = self.getPillarOnWebservice(distribution)
self.assertNamesOfSpecificationsAre(
["spec1"], distro_on_webservice.valid_specifications)
+
+
+class TestSpecificationSubscription(SpecificationWebserviceTestCase):
+
+ layer = AppServerLayer
+
+ def test_subscribe(self):
+ # Test subscribe() API.
+ with person_logged_in(ANONYMOUS):
+ db_spec = self.factory.makeSpecification()
+ db_person = self.factory.makePerson()
+ launchpad = self.factory.makeLaunchpadService()
+
+ spec = ws_object(launchpad, db_spec)
+ person = ws_object(launchpad, db_person)
+ spec.subscribe(person=person, essential=True)
+ transaction.commit()
+
+ # Check the results.
+ sub = db_spec.subscription(db_person)
+ self.assertIsNot(None, sub)
+ self.assertEqual(sub.essential, True)
+
+ def test_unsubscribe(self):
+ # Test unsubscribe() API.
+ with person_logged_in(ANONYMOUS):
+ db_spec = self.factory.makeBlueprint()
+ db_person = self.factory.makePerson()
+ db_spec.subscribe(person=db_person)
+ launchpad = self.factory.makeLaunchpadService()
+
+ spec = ws_object(launchpad, db_spec)
+ person = ws_object(launchpad, db_person)
+ spec.unsubscribe(person=person)
+ transaction.commit()
+
+ # Check the results.
+ self.assertFalse(db_spec.isSubscribed(db_person))
+
+ def test_canBeUnsubscribedByUser(self):
+ # Test canBeUnsubscribedByUser() API.
+ webservice = LaunchpadWebServiceCaller(
+ 'launchpad-library', 'salgado-change-anything',
+ domain='api.launchpad.dev:8085')
+
+ with person_logged_in(ANONYMOUS):
+ db_spec = self.factory.makeSpecification()
+ db_person = self.factory.makePerson()
+ launchpad = self.factory.makeLaunchpadService()
+
+ spec = ws_object(launchpad, db_spec)
+ person = ws_object(launchpad, db_person)
+ subscription = spec.subscribe(person=person, essential=True)
+ transaction.commit()
+
+ result = webservice.named_get(
+ subscription['self_link'], 'canBeUnsubscribedByUser').jsonBody()
+ self.assertFalse(result)
Follow ups
