launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04335
[Merge] lp:~wallyworld/launchpad/question-canBeUnsubscribedByUser into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/question-canBeUnsubscribedByUser into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/question-canBeUnsubscribedByUser/+merge/68798
This is the first part of changes to allow users to unsubscribe people from questions without needed to ask an dmin to run SQL. See bug 134577.
== Implementation ==
This branch adds canBeUnsubscribedByUser() to IQuestionSubscription
ie it adds the necessary permission check so that the next branch can add the user interface elements to do the work
Users who can unsubscribe someone from a question:
- lp admins
- the person themselves
- the question owner
- people who can reject questions (eg target owner, answer contacts)
== Tests ==
Add new unit test module: test_questionsubscription.py
There were various subscribe/unsubscribe bits and pieces in doc tests. However, the new unit test specifically tests the expected canBeUnsubscribedByUser behaviour. The doc tests changed due to the API change for unsubscribe() were run also.
== Lint ==
Linting changed files:
lib/lp/answers/browser/question.py
lib/lp/answers/browser/tests/question-subscribe_me.txt
lib/lp/answers/doc/notifications.txt
lib/lp/answers/doc/question.txt
lib/lp/answers/interfaces/question.py
lib/lp/answers/interfaces/questionsubscription.py
lib/lp/answers/model/question.py
lib/lp/answers/model/questionsubscription.py
lib/lp/answers/model/tests/
lib/lp/answers/model/tests/__init__.py
lib/lp/answers/model/tests/test_questionsubscription.py
lib/lp/testing/factory.py
--
https://code.launchpad.net/~wallyworld/launchpad/question-canBeUnsubscribedByUser/+merge/68798
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/question-canBeUnsubscribedByUser into lp:launchpad.
=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py 2011-05-27 21:12:25 +0000
+++ lib/lp/answers/browser/question.py 2011-07-22 07:31:44 +0000
@@ -361,7 +361,7 @@
_("You have subscribed to this question."))
modified_fields.add('subscribers')
elif newsub == 'Unsubscribe':
- self.context.unsubscribe(self.user)
+ self.context.unsubscribe(self.user, self.user)
response.addNotification(
_("You have unsubscribed from this question."))
modified_fields.add('subscribers')
=== modified file 'lib/lp/answers/browser/tests/question-subscribe_me.txt'
--- lib/lp/answers/browser/tests/question-subscribe_me.txt 2010-07-30 12:56:27 +0000
+++ lib/lp/answers/browser/tests/question-subscribe_me.txt 2011-07-22 07:31:44 +0000
@@ -17,7 +17,7 @@
Empty the subscribers list:
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
>>> list(firefox_question.subscriptions)
[]
@@ -36,7 +36,7 @@
>>> workflow_harness.submit('requestinfo', form_data)
>>> firefox_question.isSubscribed(foo_bar)
True
- >>> firefox_question.unsubscribe(foo_bar)
+ >>> firefox_question.unsubscribe(foo_bar, foo_bar)
Subscription is possible when providing more information:
@@ -44,7 +44,7 @@
>>> workflow_harness.submit('giveinfo', form_data)
>>> firefox_question.isSubscribed(sample_person)
True
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
Subscription is possible when providing an answer:
@@ -52,7 +52,7 @@
>>> workflow_harness.submit('answer', form_data)
>>> firefox_question.isSubscribed(foo_bar)
True
- >>> firefox_question.unsubscribe(foo_bar)
+ >>> firefox_question.unsubscribe(foo_bar, foo_bar)
As when confirming an answer (altough this is probably not that common):
@@ -60,28 +60,28 @@
>>> workflow_harness.submit('confirm', dict(answer_id=-1, **form_data))
>>> firefox_question.isSubscribed(sample_person)
True
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
It is also possible when reopening the request.
>>> workflow_harness.submit('reopen', form_data)
>>> firefox_question.isSubscribed(sample_person)
True
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
Self-Answering the request:
>>> workflow_harness.submit('selfanswer', form_data)
>>> firefox_question.isSubscribed(sample_person)
True
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
As well as adding a comment:
>>> workflow_harness.submit('comment', form_data)
>>> firefox_question.isSubscribed(sample_person)
True
- >>> firefox_question.unsubscribe(sample_person)
+ >>> firefox_question.unsubscribe(sample_person, sample_person)
Make sure that whenever the view actions is modified, this test
requires update:
=== modified file 'lib/lp/answers/doc/notifications.txt'
--- lib/lp/answers/doc/notifications.txt 2011-05-13 16:50:50 +0000
+++ lib/lp/answers/doc/notifications.txt 2011-07-22 07:31:44 +0000
@@ -302,7 +302,7 @@
a notification.
>>> login('test@xxxxxxxxxxxxx')
- >>> ubuntu_question.unsubscribe(sample_person)
+ >>> ubuntu_question.unsubscribe(sample_person, sample_person)
>>> message = ubuntu_question.giveInfo('A PowerMac 7200.')
>>> notifications = pop_questionemailjobs()
=== modified file 'lib/lp/answers/doc/question.txt'
--- lib/lp/answers/doc/question.txt 2011-05-13 02:52:50 +0000
+++ lib/lp/answers/doc/question.txt 2011-07-22 07:31:44 +0000
@@ -203,7 +203,7 @@
To remove a person from the subscriptions list, we use the unsubscribe()
method.
- >>> firefox_question.unsubscribe(no_priv)
+ >>> firefox_question.unsubscribe(no_priv, no_priv)
>>> print_subscribers(firefox_question)
Sample Person
=== modified file 'lib/lp/answers/interfaces/question.py'
--- lib/lp/answers/interfaces/question.py 2011-05-21 18:54:42 +0000
+++ lib/lp/answers/interfaces/question.py 2011-07-22 07:31:44 +0000
@@ -440,7 +440,7 @@
def isSubscribed(person):
"""Return a boolean indicating whether the person is subscribed."""
- def unsubscribe(person):
+ def unsubscribe(person, unsubscribed_by):
"""Remove the person's subscription to this question."""
def getDirectSubscribers():
=== modified file 'lib/lp/answers/interfaces/questionsubscription.py'
--- lib/lp/answers/interfaces/questionsubscription.py 2010-08-20 20:31:18 +0000
+++ lib/lp/answers/interfaces/questionsubscription.py 2011-07-22 07:31:44 +0000
@@ -23,3 +23,5 @@
person = Attribute("The subscriber.")
question = Attribute("The question.")
+ def canBeUnsubscribedByUser(user):
+ """Can the user unsubscribe the subscriber from the question?"""
=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py 2011-05-29 21:18:09 +0000
+++ lib/lp/answers/model/question.py 2011-07-22 07:31:44 +0000
@@ -87,6 +87,7 @@
from lp.answers.model.questionreopening import create_questionreopening
from lp.answers.model.questionsubscription import QuestionSubscription
from lp.app.enums import ServiceUsage
+from lp.app.errors import UserCannotUnsubscribePerson
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.bugs.interfaces.buglink import IBugLinkTarget
from lp.bugs.interfaces.bugtask import BugTaskStatus
@@ -527,11 +528,16 @@
Store.of(sub).flush()
return sub
- def unsubscribe(self, person):
+ def unsubscribe(self, person, unsubscribed_by):
"""See `IQuestion`."""
# See if a relevant subscription exists, and if so, delete it.
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))
store = Store.of(sub)
sub.destroySelf()
store.flush()
=== modified file 'lib/lp/answers/model/questionsubscription.py'
--- lib/lp/answers/model/questionsubscription.py 2010-10-03 15:30:06 +0000
+++ lib/lp/answers/model/questionsubscription.py 2011-07-22 07:31:44 +0000
@@ -15,6 +15,7 @@
from canonical.database.sqlbase import SQLBase
from lp.answers.interfaces.questionsubscription import IQuestionSubscription
from lp.registry.interfaces.person import validate_public_person
+from lp.registry.interfaces.role import IPersonRoles
class QuestionSubscription(SQLBase):
@@ -30,3 +31,17 @@
person = ForeignKey(
dbName='person', foreignKey='Person',
storm_validator=validate_public_person, notNull=True)
+
+ def canBeUnsubscribedByUser(self, user):
+ """See `IQuestionSubscription`."""
+ if user is None:
+ return False
+ # The people who can unsubscribe someone are:
+ # - lp admins
+ # - the person themselves
+ # - the question owner
+ # - people who can reject questions (eg target owner, answer contacts)
+ return (user.inTeam(self.question.owner) or
+ user.inTeam(self.person) or
+ IPersonRoles(user).in_admin or
+ self.question.canReject(user))
=== added directory 'lib/lp/answers/model/tests'
=== added file 'lib/lp/answers/model/tests/__init__.py'
=== added file 'lib/lp/answers/model/tests/test_questionsubscription.py'
--- lib/lp/answers/model/tests/test_questionsubscription.py 1970-01-01 00:00:00 +0000
+++ lib/lp/answers/model/tests/test_questionsubscription.py 2011-07-22 07:31:44 +0000
@@ -0,0 +1,140 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the QuestionSubscrption model object.."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.errors import UserCannotUnsubscribePerson
+from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+
+
+class TestQuestionSubscription(TestCaseWithFactory):
+ """Tests relating to question subscriptions in general."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_owner_subscribed(self):
+ # The owner of a question is subscribed to the question.
+ question = self.factory.makeQuestion()
+ [subscription] = list(question.subscriptions)
+ self.assertEqual(question.owner, subscription.person)
+
+ def test_subscribed_by_set(self):
+ """The user subscribing is recorded along the subscriber."""
+ subscriber = self.factory.makePerson()
+ question = self.factory.makeQuestion()
+ with person_logged_in(subscriber):
+ subscription = question.subscribe(subscriber)
+ self.assertEqual(subscriber, subscription.person)
+
+ def test_unsubscribe(self):
+ """Test unsubscribing by the subscriber."""
+ subscription = self.factory.makeQuestionSubscription()
+ subscriber = subscription.person
+ question = subscription.question
+ with person_logged_in(subscriber):
+ question.unsubscribe(subscriber, subscriber)
+ self.assertFalse(question.isSubscribed(subscriber))
+
+ def test_unsubscribe_by_unauthorized(self):
+ """Test unsubscribing someone you shouldn't be able to."""
+ subscription = self.factory.makeQuestionSubscription()
+ question = subscription.question
+ unsubscriber = self.factory.makePerson()
+ with person_logged_in(unsubscriber):
+ self.assertRaises(
+ UserCannotUnsubscribePerson,
+ question.unsubscribe,
+ subscription.person,
+ unsubscriber)
+
+
+class TestQuestionSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
+ """Tests for QuestionSubscription.canBeUnsubscribedByUser."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_none(self):
+ """None for a user always returns False."""
+ subscription = self.factory.makeQuestionSubscription()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(None))
+
+ def test_self_subscriber(self):
+ """The subscriber has permission to unsubscribe."""
+ subscription = self.factory.makeQuestionSubscription()
+ self.assertTrue(
+ subscription.canBeUnsubscribedByUser(subscription.person))
+
+ def test_non_subscriber_fails(self):
+ """An unrelated person can't unsubscribe a user."""
+ subscription = self.factory.makeQuestionSubscription()
+ editor = self.factory.makePerson()
+ self.assertFalse(subscription.canBeUnsubscribedByUser(editor))
+
+ def test_team_member_can_unsubscribe(self):
+ """Any team member can unsubscribe the team from a question."""
+ team = self.factory.makeTeam()
+ member = self.factory.makePerson()
+ with person_logged_in(team.teamowner):
+ team.addMember(member, team.teamowner)
+ subscription = self.factory.makeQuestionSubscription(person=team)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(member))
+
+ def test_question_person_owner_can_unsubscribe(self):
+ """Question owner can unsubscribe someone from a question."""
+ question_owner = self.factory.makePerson()
+ question = self.factory.makeQuestion(owner=question_owner)
+ subscriber = self.factory.makePerson()
+ subscription = self.factory.makeQuestionSubscription(
+ question=question, person=subscriber)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(question_owner))
+
+ def test_question_team_owner_can_unsubscribe(self):
+ """Question team owner can unsubscribe someone from a question.
+
+ If the owner of a question is a team, then the team members can
+ unsubscribe someone.
+ """
+ team_owner = self.factory.makePerson()
+ team_member = self.factory.makePerson()
+ question_owner = self.factory.makeTeam(
+ owner=team_owner, members=[team_member])
+ question = self.factory.makeQuestion(owner=question_owner)
+ subscriber = self.factory.makePerson()
+ subscription = self.factory.makeQuestionSubscription(
+ question=question, person=subscriber)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(team_owner))
+ self.assertTrue(subscription.canBeUnsubscribedByUser(team_member))
+
+ def test_question_target_owner_can_unsubscribe(self):
+ """Question target owner can unsubscribe someone from a question."""
+ target_owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=target_owner)
+ question = self.factory.makeQuestion(target=product)
+ subscriber = self.factory.makePerson()
+ subscription = self.factory.makeQuestionSubscription(
+ question=question, person=subscriber)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(target_owner))
+
+ def test_question_target_answer_contact_can_unsubscribe(self):
+ """Question target answer contact can unsubscribe someone."""
+ answer_contact = self.factory.makePerson()
+ english = getUtility(ILanguageSet)['en']
+ answer_contact.addLanguage(english)
+ distro_owner = self.factory.makePerson()
+ distro = self.factory.makeDistribution(owner=distro_owner)
+ with person_logged_in(distro_owner):
+ distro.addAnswerContact(answer_contact, answer_contact)
+ question = self.factory.makeQuestion(target=distro)
+ subscriber = self.factory.makePerson()
+ subscription = self.factory.makeQuestionSubscription(
+ question=question, person=subscriber)
+ self.assertTrue(subscription.canBeUnsubscribedByUser(answer_contact))
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-07-19 13:51:36 +0000
+++ lib/lp/testing/factory.py 2011-07-22 07:31:44 +0000
@@ -2070,6 +2070,15 @@
owner=owner, title=title, description=description)
return question
+ def makeQuestionSubscription(self, question=None, person=None):
+ """Create a QuestionSubscription."""
+ if question is None:
+ question = self.makeQuestion()
+ if person is None:
+ person = self.makePerson()
+ with person_logged_in(person):
+ return question.subscribe(person)
+
def makeFAQ(self, target=None, title=None):
"""Create and return a new, arbitrary FAQ.
