launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03693
[Merge] lp:~danilo/launchpad/bug-772763-remove-unmute-dialog-part1 into lp:launchpad/db-devel
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-772763-remove-unmute-dialog-part1 into lp:launchpad/db-devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Stuart Bishop (stub): db
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-772763-remove-unmute-dialog-part1/+merge/61775
= Bug 772763: remove unmute dialog, part1 =
As part of solving 772763, we remove the pop-up unmute dialog (to directly unmute instead). To prepare for that, we first do some server-side cleanups and make unmute() method return the previously masked subscription (if any).
== Proposed fix ==
This branch does a few things:
- Make "static" (i.e. non-JS) page IBugTask:+mute not redirect to +subscribe to offer unmute but instead allow direct unmuting
- Ensures Mute/Unmute link is shown when bug is muted
- Takes mutes into consideration for all subscribers-calculation (direct, duplicate, also-notified) (changes in model/bug.py)
- Make unmute() method return a previous subscription (if any)
== Pre-implementation notes ==
This is mostly Gary's branch. I am only shepherding it and providing a few tests of my own.
== Implementation details ==
Not cleaning up the lint for the model/bug.py since it'd taint the diff.
== Tests ==
bin/test -cvvt TestBugSubscriptionInfo -t TestBugSubscriptionMethods -t BugMuteSelfViewTestCase -t TestBugPortletSubscribers
== Demo and Q/A ==
Check that IBugTask:+mute page behaves as expected (iow, allows unmuting as well). Rest of the QA will be possible only when the follow-up branch lands.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
lib/lp/bugs/browser/tests/test_bug_views.py
lib/lp/bugs/browser/bug.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/browser/tests/test_bugsubscription_views.py
lib/lp/bugs/browser/bugsubscription.py
lib/lp/bugs/tests/test_bug.py
lib/lp/bugs/interfaces/bug.py
./lib/lp/bugs/model/bug.py
575: E225 missing whitespace around operator
747: E225 missing whitespace around operator
751: E225 missing whitespace around operator
766: E225 missing whitespace around operator
1461: E225 missing whitespace around operator
1674: E261 at least two spaces before inline comment
1676: E261 at least two spaces before inline comment
1687: E261 at least two spaces before inline comment
1689: E261 at least two spaces before inline comment
1707: E225 missing whitespace around operator
2262: E225 missing whitespace around operator
2277: E225 missing whitespace around operator
2287: E261 at least two spaces before inline comment
2325: E225 missing whitespace around operator
2587: E225 missing whitespace around operator
2628: E225 missing whitespace around operator
--
https://code.launchpad.net/~danilo/launchpad/bug-772763-remove-unmute-dialog-part1/+merge/61775
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-772763-remove-unmute-dialog-part1 into lp:launchpad/db-devel.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-05-17 14:54:56 +0000
+++ lib/lp/bugs/browser/bug.py 2011-05-20 14:30:32 +0000
@@ -292,7 +292,7 @@
return Link(
link, text, icon='remove', summary=(
- "Mute this bug so that you will never receive emails "
+ "Mute this bug so that you will not receive emails "
"about it."))
def nominate(self):
@@ -538,9 +538,7 @@
"""Return True if the user should see the Mute link."""
if features.getFeatureFlag('malone.advanced-subscriptions.enabled'):
user_is_subscribed = (
- # Note that we don't have to check for isMuted(), since
- # if isMuted() is True isSubscribed() will also be
- # True.
+ self.context.isMuted(self.user) or
self.context.isSubscribed(self.user) or
self.context.isSubscribedToDupes(self.user) or
self.context.personIsAlsoNotifiedSubscriber(self.user))
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2011-05-18 08:17:35 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2011-05-20 14:30:32 +0000
@@ -630,7 +630,10 @@
@property
def label(self):
- return "Mute bug mail for bug %s" % self.context.bug.id
+ if self.context.bug.isMuted(self.user):
+ return "Unmute bug mail for bug %s" % self.context.bug.id
+ else:
+ return "Mute bug mail for bug %s" % self.context.bug.id
page_title = label
@@ -641,15 +644,21 @@
cancel_url = next_url
def initialize(self):
+ self.is_muted = self.context.bug.isMuted(self.user)
super(BugMuteSelfView, self).initialize()
- # If the user is already muted, redirect them to the +subscribe
- # page, since there's no point doing its work twice.
- if self.context.bug.isMuted(self.user):
- self.request.response.redirect(
- canonical_url(self.context, view_name="+subscribe"))
- @action('Mute bug mail', name='mute')
+ @action('Mute bug mail',
+ name='mute',
+ condition=lambda form, action: not form.is_muted)
def mute_action(self, action, data):
self.context.bug.mute(self.user, self.user)
self.request.response.addInfoNotification(
"Mail for bug #%s has been muted." % self.context.bug.id)
+
+ @action('Unmute bug mail',
+ name='unmute',
+ condition=lambda form, action: form.is_muted)
+ def unmute_action(self, action, data):
+ self.context.bug.unmute(self.user, self.user)
+ self.request.response.addInfoNotification(
+ "Mail for bug #%s has been unmuted." % self.context.bug.id)
=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-17 14:54:56 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-20 14:30:32 +0000
@@ -173,7 +173,30 @@
self.assertTrue('mute_subscription' in html)
# The template uses user_should_see_mute_link to decide
# whether or not to display the mute link.
- soup = BeautifulSoup(html)
self.assertTrue(
self._hasCSSClass(html, 'mute-link-container', 'hidden'),
'No "hidden" CSS class in mute-link-container.')
+
+ def test_mute_subscription_link_shown_if_muted(self):
+ # If a person is muted but not otherwise subscribed, they should still
+ # see the (un)mute link.
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ with FeatureFixture({self.feature_flag_1: 'on'}):
+ self.bug.mute(person, person)
+ # The user isn't subscribed already, but is muted.
+ self.assertFalse(self.bug.isSubscribed(person))
+ self.assertFalse(
+ self.bug.personIsAlsoNotifiedSubscriber(
+ person))
+ self.assertTrue(self.bug.isMuted(person))
+ view = create_initialized_view(
+ self.bug, name="+portlet-subscribers")
+ self.assertTrue(view.user_should_see_mute_link,
+ "User should see mute link.")
+ contents = view.render()
+ self.assertTrue('mute_subscription' in contents,
+ "'mute_subscription' not in contents.")
+ self.assertFalse(
+ self._hasCSSClass(
+ contents, 'mute-link-container', 'hidden'))
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-05-17 11:44:33 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-05-20 14:30:32 +0000
@@ -8,7 +8,6 @@
import transaction
from canonical.launchpad.ftests import LaunchpadFormHarness
-from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import LaunchpadFunctionalLayer
from lp.bugs.browser.bugsubscription import (
@@ -409,6 +408,44 @@
self.bug = self.factory.makeBug()
self.person = self.factory.makePerson()
+ def test_is_muted_false(self):
+ # BugMuteSelfView initialization sets the is_muted property.
+ # When the person has not muted the bug, it's false.
+ with person_logged_in(self.person):
+ self.assertFalse(self.bug.isMuted(self.person))
+ view = create_initialized_view(
+ self.bug.default_bugtask, name="+mute")
+ self.assertFalse(view.is_muted)
+
+ def test_is_muted_true(self):
+ # BugMuteSelfView initialization sets the is_muted property.
+ # When the person has muted the bug, it's true.
+ with person_logged_in(self.person):
+ self.bug.mute(self.person, self.person)
+ self.assertTrue(self.bug.isMuted(self.person))
+ view = create_initialized_view(
+ self.bug.default_bugtask, name="+mute")
+ self.assertTrue(view.is_muted)
+
+ def test_label_nonmuted(self):
+ # Label to use for the button.
+ with person_logged_in(self.person):
+ self.assertFalse(self.bug.isMuted(self.person))
+ expected_label = "Mute bug mail for bug %s" % self.bug.id
+ view = create_initialized_view(
+ self.bug.default_bugtask, name="+mute")
+ self.assertEqual(expected_label, view.label)
+
+ def test_label_muted(self):
+ # Label to use for the button.
+ with person_logged_in(self.person):
+ self.bug.mute(self.person, self.person)
+ self.assertTrue(self.bug.isMuted(self.person))
+ expected_label = "Unmute bug mail for bug %s" % self.bug.id
+ view = create_initialized_view(
+ self.bug.default_bugtask, name="+mute")
+ self.assertEqual(expected_label, view.label)
+
def test_bug_mute_self_view_mutes_bug(self):
# The BugMuteSelfView mutes bug mail for the current user when
# its form is submitted.
@@ -419,17 +456,13 @@
form={'field.actions.mute': 'Mute bug mail'})
self.assertTrue(self.bug.isMuted(self.person))
- def test_bug_mute_self_view_redirects_muted_users(self):
- # The BugMuteSelfView redirects muted users to the +subscribe
- # page, where they can remove their muted subscription or change
- # their BugNotificationLevel.
+ def test_bug_mute_self_view_unmutes_bug(self):
+ # The BugMuteSelfView unmutes bug mail for the current user when
+ # its form is submitted and the bug was already muted.
with person_logged_in(self.person):
self.bug.mute(self.person, self.person)
- mute_view = create_initialized_view(
- self.bug.default_bugtask, name="+mute")
- response = mute_view.request.response
- self.assertEqual(302, response.getStatus())
- self.assertEqual(
- canonical_url(self.bug.default_bugtask,
- view_name="+subscribe"),
- response.getHeader('Location'))
+ self.assertTrue(self.bug.isMuted(self.person))
+ create_initialized_view(
+ self.bug.default_bugtask, name="+mute",
+ form={'field.actions.mute': 'Unmute bug mail'})
+ self.assertTrue(self.bug.isMuted(self.person))
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2011-05-18 08:17:35 +0000
+++ lib/lp/bugs/interfaces/bug.py 2011-05-20 14:30:32 +0000
@@ -509,7 +509,9 @@
@export_write_operation()
@operation_for_version('devel')
def unmute(person, unmuted_by):
- """Remove a muted subscription for `person`."""
+ """Remove a muted subscription for `person`.
+
+ Returns previously muted direct subscription, if any."""
def getDirectSubscriptions():
"""A sequence of IBugSubscriptions directly linked to this bug."""
@@ -779,9 +781,9 @@
schema=Interface, title=_('Target'), required=False),
nominations=List(
title=_("Nominations to search through."),
- value_type=Reference(schema=Interface), # IBugNomination
+ value_type=Reference(schema=Interface), # IBugNomination
required=False))
- @operation_returns_collection_of(Interface) # IBugNomination
+ @operation_returns_collection_of(Interface) # IBugNomination
@export_read_operation()
def getNominations(target=None, nominations=None):
"""Return a list of all IBugNominations for this bug.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-05-18 08:17:35 +0000
+++ lib/lp/bugs/model/bug.py 2011-05-20 14:30:32 +0000
@@ -54,6 +54,7 @@
Count,
Desc,
Exists,
+ In,
Join,
LeftJoin,
Max,
@@ -531,7 +532,7 @@
parent = message_by_id.get(parent.id, parent)
else:
message, bugmessage = row
- parent = None # parent attribute is not going to be accessed.
+ parent = None # parent attribute is not going to be accessed.
index = bugmessage.index
result = IndexedMessage(message, inside, index, parent)
if include_parents:
@@ -891,6 +892,7 @@
person = unmuted_by
mutes = self._getMutes(person)
store.remove(mutes.one())
+ return self.getSubscriptionForPerson(person)
@property
def subscriptions(self):
@@ -967,6 +969,12 @@
See the comment in getDirectSubscribers for a description of the
recipients argument.
"""
+ if self.private:
+ # We short-circuit for private bugs, since actually
+ # returning something non-empty here causes things to break
+ # in fun and interesting ways (see bug 780248).
+ return []
+
if level is None:
level = BugNotificationLevel.LIFECYCLE
info = self.getSubscriptionInfo(level)
@@ -2249,7 +2257,9 @@
return IStore(BugSubscription).find(
BugSubscription,
BugSubscription.bug_notification_level >= self.level,
- BugSubscription.bug == self.bug)
+ BugSubscription.bug == self.bug,
+ Not(In(BugSubscription.person_id,
+ Select(BugMute.person_id, BugMute.bug_id==self.bug.id))))
@cachedproperty
@freeze(BugSubscriptionSet)
@@ -2262,12 +2272,14 @@
BugSubscription,
BugSubscription.bug_notification_level >= self.level,
BugSubscription.bug_id == Bug.id,
- Bug.duplicateof == self.bug)
+ Bug.duplicateof == self.bug,
+ Not(In(BugSubscription.person_id,
+ Select(BugMute.person_id, BugMute.bug_id==Bug.id))))
@cachedproperty
@freeze(BugSubscriptionSet)
def duplicate_only_subscriptions(self):
- """Subscripitions to duplicates of the bug.
+ """Subscriptions to duplicates of the bug.
Excludes subscriptions for people who have a direct subscription or
are also notified for another reason.
@@ -2308,11 +2320,15 @@
if self.bug.private:
return BugSubscriberSet()
else:
+ muted = IStore(BugMute).find(
+ Person,
+ BugMute.person_id==Person.id,
+ BugMute.bug==self.bug)
return BugSubscriberSet().union(
self.structural_subscriptions.subscribers,
self.all_pillar_owners_without_bug_supervisors,
self.all_assignees).difference(
- self.direct_subscriptions.subscribers)
+ self.direct_subscriptions.subscribers).difference(muted)
@cachedproperty
def indirect_subscribers(self):
=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2011-05-16 16:57:55 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2011-05-20 14:30:32 +0000
@@ -132,21 +132,42 @@
return BugSubscriptionInfo(
self.bug, BugNotificationLevel.LIFECYCLE)
+ def _create_direct_subscriptions(self):
+ subscribers = (
+ self.factory.makePerson(),
+ self.factory.makePerson())
+ with person_logged_in(self.bug.owner):
+ subscriptions = tuple(
+ self.bug.subscribe(subscriber, subscriber)
+ for subscriber in subscribers)
+ return subscribers, subscriptions
+
def test_direct(self):
# The set of direct subscribers.
- subscribers = (
- self.factory.makePerson(),
- self.factory.makePerson())
- with person_logged_in(self.bug.owner):
- subscriptions = tuple(
- self.bug.subscribe(subscriber, subscriber)
- for subscriber in subscribers)
+ subscribers, subscriptions = self._create_direct_subscriptions()
found_subscriptions = self.getInfo().direct_subscriptions
self.assertEqual(set(subscriptions), found_subscriptions)
self.assertEqual(subscriptions, found_subscriptions.sorted)
self.assertEqual(set(subscribers), found_subscriptions.subscribers)
self.assertEqual(subscribers, found_subscriptions.subscribers.sorted)
+ def test_muted_direct(self):
+ # If a direct is muted, it is not listed.
+ subscribers, subscriptions = self._create_direct_subscriptions()
+ with person_logged_in(subscribers[0]):
+ self.bug.mute(subscribers[0], subscribers[0])
+ found_subscriptions = self.getInfo().direct_subscriptions
+ self.assertEqual(set([subscriptions[1]]), found_subscriptions)
+
+ def _create_duplicate_subscription(self):
+ duplicate_bug = self.factory.makeBug(product=self.target)
+ with person_logged_in(duplicate_bug.owner):
+ duplicate_bug.markAsDuplicate(self.bug)
+ duplicate_bug_subscription = (
+ duplicate_bug.getSubscriptionForPerson(
+ duplicate_bug.owner))
+ return duplicate_bug, duplicate_bug_subscription
+
def test_duplicate(self):
# The set of subscribers from duplicate bugs.
found_subscriptions = self.getInfo().duplicate_subscriptions
@@ -154,12 +175,8 @@
self.assertEqual((), found_subscriptions.sorted)
self.assertEqual(set(), found_subscriptions.subscribers)
self.assertEqual((), found_subscriptions.subscribers.sorted)
- duplicate_bug = self.factory.makeBug(product=self.target)
- with person_logged_in(duplicate_bug.owner):
- duplicate_bug.markAsDuplicate(self.bug)
- duplicate_bug_subscription = (
- duplicate_bug.getSubscriptionForPerson(
- duplicate_bug.owner))
+ duplicate_bug, duplicate_bug_subscription = (
+ self._create_duplicate_subscription())
found_subscriptions = self.getInfo().duplicate_subscriptions
self.assertEqual(
set([duplicate_bug_subscription]),
@@ -174,6 +191,15 @@
(duplicate_bug.owner,),
found_subscriptions.subscribers.sorted)
+ def test_muted_duplicate(self):
+ # If a duplicate is muted, it is not listed.
+ duplicate_bug, duplicate_bug_subscription = (
+ self._create_duplicate_subscription())
+ with person_logged_in(duplicate_bug.owner):
+ self.bug.mute(duplicate_bug.owner, duplicate_bug.owner)
+ found_subscriptions = self.getInfo().duplicate_subscriptions
+ self.assertEqual(set(), found_subscriptions)
+
def test_duplicate_for_private_bug(self):
# The set of subscribers from duplicate bugs is always empty when the
# master bug is private.
@@ -274,11 +300,7 @@
(bugtask.pillar.owner, bugtask2.pillar.owner),
found_owners.sorted)
- def test_also_notified_subscribers(self):
- # The set of also notified subscribers.
- found_subscribers = self.getInfo().also_notified_subscribers
- self.assertEqual(set(), found_subscribers)
- self.assertEqual((), found_subscribers.sorted)
+ def _create_also_notified_subscribers(self):
# Add an assignee, a bug supervisor and a structural subscriber.
bugtask = self.bug.default_bugtask
assignee = self.factory.makePerson()
@@ -291,6 +313,15 @@
with person_logged_in(structural_subscriber):
bugtask.target.addSubscription(
structural_subscriber, structural_subscriber)
+ return assignee, supervisor, structural_subscriber
+
+ def test_also_notified_subscribers(self):
+ # The set of also notified subscribers.
+ found_subscribers = self.getInfo().also_notified_subscribers
+ self.assertEqual(set(), found_subscribers)
+ self.assertEqual((), found_subscribers.sorted)
+ assignee, supervisor, structural_subscriber = (
+ self._create_also_notified_subscribers())
# Add a direct subscription.
direct_subscriber = self.factory.makePerson()
with person_logged_in(self.bug.owner):
@@ -305,6 +336,30 @@
(assignee, supervisor, structural_subscriber),
found_subscribers.sorted)
+ def test_muted_also_notified_subscribers(self):
+ # If someone is muted, they are not listed in the
+ # also_notified_subscribers.
+ assignee, supervisor, structural_subscriber = (
+ self._create_also_notified_subscribers())
+ # As a control, we first show that the
+ # the assignee, supervisor and structural subscriber do show up
+ # when they are not muted.
+ found_subscribers = self.getInfo().also_notified_subscribers
+ self.assertEqual(
+ set([assignee, supervisor, structural_subscriber]),
+ found_subscribers)
+ # Now we mute all of the subscribers.
+ with person_logged_in(assignee):
+ self.bug.mute(assignee, assignee)
+ with person_logged_in(supervisor):
+ self.bug.mute(supervisor, supervisor)
+ with person_logged_in(structural_subscriber):
+ self.bug.mute(structural_subscriber, structural_subscriber)
+ # Now we don't see them.
+ found_subscribers = self.getInfo().also_notified_subscribers
+ self.assertEqual(
+ set(), found_subscribers)
+
def test_also_notified_subscribers_for_private_bug(self):
# The set of also notified subscribers is always empty when the master
# bug is private.
@@ -472,7 +527,7 @@
self.info.all_pillar_owners_without_bug_supervisors
def test_also_notified_subscribers(self):
- with self.exactly_x_queries(5):
+ with self.exactly_x_queries(6):
self.info.also_notified_subscribers
def test_also_notified_subscribers_later(self):
@@ -482,11 +537,11 @@
self.info.all_pillar_owners_without_bug_supervisors
self.info.direct_subscriptions.subscribers
self.info.structural_subscriptions.subscribers
- with self.exactly_x_queries(0):
+ with self.exactly_x_queries(1):
self.info.also_notified_subscribers
def test_indirect_subscribers(self):
- with self.exactly_x_queries(6):
+ with self.exactly_x_queries(7):
self.info.indirect_subscribers
def test_indirect_subscribers_later(self):
=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py 2011-05-18 08:17:35 +0000
+++ lib/lp/bugs/tests/test_bug.py 2011-05-20 14:30:32 +0000
@@ -105,6 +105,22 @@
self.bug.unmute(self.person, self.person)
self.assertFalse(self.bug.isMuted(self.person))
+ def test_unmute_returns_direct_subscription(self):
+ # Bug.unmute() returns the previously muted direct subscription, if
+ # any.
+ with person_logged_in(self.person):
+ self.bug.mute(self.person, self.person)
+ self.assertEqual(True, self.bug.isMuted(self.person))
+ self.assertEqual(None, self.bug.unmute(self.person, self.person))
+ self.assertEqual(False, self.bug.isMuted(self.person))
+ subscription = self.bug.subscribe(
+ self.person, self.person,
+ level=BugNotificationLevel.METADATA)
+ self.bug.mute(self.person, self.person)
+ self.assertEqual(True, self.bug.isMuted(self.person))
+ self.assertEqual(
+ subscription, self.bug.unmute(self.person, self.person))
+
def test_unmute_mutes_unmuter(self):
# When exposed in the web API, the unmute method regards the
# first, `person` argument as optional, and the second
Follow ups
