Merge lp:~bac/launchpad/bug-799901 into lp:launchpad

[Brad Crittenden]

[r=allenap][bug=799901] Add tooltip to bug subscribers list.

= Summary =

The new bug subscription work has a regression in that the tooltip showing who subscribed a person or team is missing. This branch adds that functionality back.

== Proposed fix ==

Add the tooltip for direct subscribers. The indirect subscribers (seen under "May be notified") don't have the required subscription information and were not previously decorated.

Note that I have chose to not include a tooltip when using "Subscribe someone else". Since the user just did the action it is forgiveable to not display the fact that he just did it. Adding that information would require another roundtrip to fetch the extra data.

== Pre-implementation notes ==

Chat with Gary.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt test_bugsubscription_views
firefox lib/lp/bugs/javascript/tests/test_subscribers_list.html

== Demo and Q/A ==

Go to https://launchpad.dev/bugs/1 and view the subscriber list.

= Launchpad lint =

Will fix this lint before landing.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bugsubscription.py
  lib/lp/bugs/doc/bugsubscription.txt
  lib/lp/bugs/browser/tests/test_bugsubscription_views.py
  lib/lp/bugs/browser/bugsubscription.py
  lib/lp/bugs/javascript/subscribers_list.js
  lib/lp/bugs/javascript/tests/test_subscribers_list.js

./lib/lp/bugs/javascript/subscribers_list.js
     725: Line exceeds 78 characters.
./lib/lp/bugs/javascript/tests/test_subscribers_list.js
     791: Line exceeds 78 characters.

[Robert Collins (lifeless)]

I haven't read the diff here in whole, but I'd like to note that this appears as though it will trigger late evaluation - the display_subscribed_by property isn't eager loaded, and a bug with a couple hundred subscriptions will cause eager loading of a couple hundred person objects *unless* they are all self-subscribed. It will also be a little slow because its comparing objects not keys - the property does:
        if self.person == self.subscribed_by:
            return u'Self-subscribed'
        else:
            return u'Subscribed by %s' % self.subscribed_by.displayname

This would be a little more efficient as

       if self.person_id == self.subscribed_by_id:
            return u'Self-subscribed'
        else:
            return u'Subscribed by %s' % self.subscribed_by.displayname

but the big thing is eager loading: we need to populate the subscribing person (but only for views that need it) en masse, rather than just-in-time.

I don't know if this will cause timeouts, but this sort of thing is one of the key drivers for timeouts.

[Данило Шеган (danilo)]

Indeed, Robert is right. For that sole reason I implemented a separate method which gets all the data with one query for this page, and that method needs extending: IBug.getDirectSubscribersWithDetails()

It returns a ResultSet of (Person, Subscription) tuples, and should be extended to return (Person, SubscribedBy, Subscription) tuples where SubscribedBy is a ClassAlias(Person) as appropriate.

Don't worry about changing the method on the model because this is the only place it is used in.

[Brad Crittenden (bac)]

Thanks for the suggestion Danilos. I've made the change you suggested. I did go ahead and update the model as Robert suggested just because I think it is better practice to do the comparisons the most efficient way even if the objects are cached so that someone else wouldn't pattern a new change off the inefficient approach.

I also added a test showing the query count for getting the direct subscribers is 1. This required a slight refactoring of the browser property.

[Gavin Panella (allenap)]

Tip top :)

[1]

+ if IBug.providedBy(self.context):
+ bug = self.context
+ elif IBugTask.providedBy(self.context):
+ bug = self.context.bug

I know you only moved this, but I thought I'd mention that there is an
adapter from IBugTask to IBug, so the above can be written as:

    bug = IBug(self.context)

[2]

+ harness = LaunchpadFormHarness(bug, BugPortletSubscribersWithDetails)

Cool, I've not see that before.

[3]

+ # A subscriber_data_js returns JSON string of a list

That should probably read:

        # subscriber_data_js returns a JSON string of a list

or something like that.

[4]

In the subscriber dict, assembled in direct_subscriber_data,
subscribed_by /feels/ like it should be the person, rather than a
description. Consider doing instead something like:

            subscriber = {
                'name': person.name,
                [...]
                'display_subscribed_by': subscription.display_subscribed_by,
                'subscribed_by_link': absoluteURL(
                    subscription.subscribed_by, self.api_request),
                }

That makes the data more generally useful, and it more closely
resembles the BugSubscription API.

Ach, it's not important, just an idea.

review: Approve

[Данило Шеган (danilo)]

Another minor note from me (I've had the tab loaded, sorry :)): perhaps you can also move the display_subscribed_by into the view and not rely on storm caching since you've already have both subscriber and subscribed_by objects which you can directly compare?

Otherwise, looks great!

[Данило Шеган (danilo)]

Of course, it's too late for it now (I see it's merged already, and I haven't refreshed the tab recently). So, never mind it.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2011-06-29 13:41:36 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2011-07-21 18:09:56 +0000
@@ -49,7 +49,6 @@
 from lp.bugs.enum import BugNotificationLevel
 from lp.bugs.interfaces.bug import IBug
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
-from lp.bugs.interfaces.bugtask import IBugTask
 from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions
 from lp.bugs.model.structuralsubscription import (
     get_structural_subscriptions_for_bug,
@@ -529,17 +528,19 @@
 class BugPortletSubscribersWithDetails(LaunchpadView):
     """A view that returns a JSON dump of the subscriber details for a bug."""
 
-    @property
-    def subscriber_data_js(self):
-        """Return subscriber_ids in a form suitable for JavaScript use."""
+    @cachedproperty
+    def api_request(self):
+        return IWebServiceClientRequest(self.request)
+
+    def direct_subscriber_data(self, bug):
+        """Get the direct subscriber data.
+
+        This method is isolated from the subscriber_data_js so that query
+        count testing can be done accurately and robustly.
+        """
         data = []
-        if IBug.providedBy(self.context):
-            bug = self.context
-        elif IBugTask.providedBy(self.context):
-            bug = self.context.bug
         details = list(bug.getDirectSubscribersWithDetails())
-        api_request = IWebServiceClientRequest(self.request)
-        for person, subscription in details:
+        for person, subscribed_by, subscription in details:
             can_edit = subscription.canBeUnsubscribedByUser(self.user)
             if person == self.user or (person.private and not can_edit):
                 # Skip the current user viewing the page,
@@ -550,9 +551,10 @@
                 'name': person.name,
                 'display_name': person.displayname,
                 'web_link': canonical_url(person, rootsite='mainsite'),
-                'self_link': absoluteURL(person, api_request),
+                'self_link': absoluteURL(person, self.api_request),
                 'is_team': person.is_team,
                 'can_edit': can_edit,
+                'display_subscribed_by': subscription.display_subscribed_by,
                 }
             record = {
                 'subscriber': subscriber,
@@ -560,6 +562,13 @@
                     removeSecurityProxy(subscription.bug_notification_level)),
                 }
             data.append(record)
+        return data
+
+    @property
+    def subscriber_data_js(self):
+        """Return subscriber_ids in a form suitable for JavaScript use."""
+        bug = IBug(self.context)
+        data = self.direct_subscriber_data(bug)
 
         others = list(bug.getIndirectSubscribers())
         for person in others:
@@ -570,7 +579,7 @@
                 'name': person.name,
                 'display_name': person.displayname,
                 'web_link': canonical_url(person, rootsite='mainsite'),
-                'self_link': absoluteURL(person, api_request),
+                'self_link': absoluteURL(person, self.api_request),
                 'is_team': person.is_team,
                 'can_edit': False,
                 }

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-06-30 22:41:02 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-07-21 18:09:56 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BugSubscription views."""
@@ -6,7 +6,8 @@
 __metaclass__ = type
 
 from simplejson import dumps
-
+from storm.store import Store
+from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.traversing.browser import absoluteURL
 
@@ -23,8 +24,10 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing import (
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.sampledata import ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
@@ -467,7 +470,7 @@
         self.assertEqual(dumps([]), harness.view.subscriber_data_js)
 
     def test_data_person_subscription(self):
-        # A subscriber_data_js returns JSON string of a list
+        # subscriber_data_js returns JSON string of a list
         # containing all subscriber information needed for
         # subscribers_list.js subscribers loading.
         bug = self._makeBugWithNoSubscribers()
@@ -487,18 +490,69 @@
                 'can_edit': False,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
-                },
-            'subscription_level': "Lifecycle",
-            }
-        self.assertEqual(
-            dumps([expected_result]), harness.view.subscriber_data_js)
+                'display_subscribed_by': 'Self-subscribed',
+                },
+            'subscription_level': "Lifecycle",
+            }
+        self.assertEqual(
+            dumps([expected_result]), harness.view.subscriber_data_js)
+
+    def test_data_person_subscription_other_subscriber(self):
+        # Ensure subscriber_data_js does the correct thing when the person who
+        # did the subscribing is someone else.
+        bug = self._makeBugWithNoSubscribers()
+        subscribed_by = self.factory.makePerson(
+            name="someone", displayname='Someone')
+        subscriber = self.factory.makePerson(
+            name='user', displayname='Subscriber Name')
+        with person_logged_in(subscriber):
+            bug.subscribe(person=subscriber,
+                          subscribed_by=subscribed_by,
+                          level=BugNotificationLevel.LIFECYCLE)
+        harness = LaunchpadFormHarness(bug, BugPortletSubscribersWithDetails)
+        api_request = IWebServiceClientRequest(harness.request)
+
+        expected_result = {
+            'subscriber': {
+                'name': 'user',
+                'display_name': 'Subscriber Name',
+                'is_team': False,
+                'can_edit': False,
+                'web_link': canonical_url(subscriber),
+                'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': 'Subscribed by Someone (someone)',
+                },
+            'subscription_level': "Lifecycle",
+            }
+        self.assertEqual(
+            dumps([expected_result]), harness.view.subscriber_data_js)
+
+    def test_data_person_subscription_other_subscriber_query_count(self):
+        # All subscriber data should be retrieved with a single query.
+        bug = self._makeBugWithNoSubscribers()
+        subscribed_by = self.factory.makePerson(
+            name="someone", displayname='Someone')
+        subscriber = self.factory.makePerson(
+            name='user', displayname='Subscriber Name')
+        with person_logged_in(subscriber):
+            bug.subscribe(person=subscriber,
+                          subscribed_by=subscribed_by,
+                          level=BugNotificationLevel.LIFECYCLE)
+        harness = LaunchpadFormHarness(bug, BugPortletSubscribersWithDetails)
+        # Invoke the view method, ignoring the results.
+        Store.of(bug).invalidate()
+        with StormStatementRecorder() as recorder:
+            harness.view.direct_subscriber_data(bug)
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
 
     def test_data_team_subscription(self):
         # For a team subscription, subscriber_data_js has is_team set
         # to true.
         bug = self._makeBugWithNoSubscribers()
+        teamowner = self.factory.makePerson(
+            name="team-owner", displayname="Team Owner")
         subscriber = self.factory.makeTeam(
-            name='team', displayname='Team Name')
+            name='team', displayname='Team Name', owner=teamowner)
         with person_logged_in(subscriber.teamowner):
             bug.subscribe(subscriber, subscriber.teamowner,
                           level=BugNotificationLevel.LIFECYCLE)
@@ -513,6 +567,8 @@
                 'can_edit': False,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': \
+                    'Subscribed by Team Owner (team-owner)',
                 },
             'subscription_level': "Lifecycle",
             }
@@ -523,8 +579,10 @@
         # For a team subscription, subscriber_data_js has can_edit
         # set to true for team owner.
         bug = self._makeBugWithNoSubscribers()
+        teamowner = self.factory.makePerson(
+            name="team-owner", displayname="Team Owner")
         subscriber = self.factory.makeTeam(
-            name='team', displayname='Team Name')
+            name='team', displayname='Team Name', owner=teamowner)
         with person_logged_in(subscriber.teamowner):
             bug.subscribe(subscriber, subscriber.teamowner,
                           level=BugNotificationLevel.LIFECYCLE)
@@ -540,6 +598,8 @@
                 'can_edit': True,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': \
+                    'Subscribed by Team Owner (team-owner)',
                 },
             'subscription_level': "Lifecycle",
             }
@@ -552,8 +612,11 @@
         # set to true for team member.
         bug = self._makeBugWithNoSubscribers()
         member = self.factory.makePerson()
+        teamowner = self.factory.makePerson(
+            name="team-owner", displayname="Team Owner")
         subscriber = self.factory.makeTeam(
-            name='team', displayname='Team Name', members=[member])
+            name='team', displayname='Team Name', owner=teamowner,
+            members=[member])
         with person_logged_in(subscriber.teamowner):
             bug.subscribe(subscriber, subscriber.teamowner,
                           level=BugNotificationLevel.LIFECYCLE)
@@ -569,6 +632,8 @@
                 'can_edit': True,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': \
+                    'Subscribed by Team Owner (team-owner)',
                 },
             'subscription_level': "Lifecycle",
             }
@@ -598,6 +663,7 @@
                 'can_edit': True,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': 'Self-subscribed',
                 },
             'subscription_level': "Lifecycle",
             }
@@ -615,7 +681,7 @@
         subscriber = self.factory.makePerson(
             name='user', displayname='Subscriber Name')
         subscribed_by = self.factory.makePerson(
-            name='someone', displayname='Subscribed By Name')
+            name='someone', displayname='Someone')
         with person_logged_in(subscriber):
             bug.subscribe(subscriber, subscribed_by,
                           level=BugNotificationLevel.LIFECYCLE)
@@ -630,6 +696,7 @@
                 'can_edit': True,
                 'web_link': canonical_url(subscriber),
                 'self_link': absoluteURL(subscriber, api_request),
+                'display_subscribed_by': 'Subscribed by Someone (someone)',
                 },
             'subscription_level': "Lifecycle",
             }

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2011-06-23 21:46:48 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2011-07-21 18:09:56 +0000
@@ -888,14 +888,14 @@
     >>> ff_bug.subscribe(lifeless, mark)
     <lp.bugs.model.bugsubscription.BugSubscription ...>
     >>> subscriptions = [
-    ...     "%s (%s)" % (
+    ...     "%s: %s" % (
     ...         subscription.person.displayname,
     ...         subscription.display_subscribed_by)
     ...     for subscription in ff_bug.getDirectSubscriptions()]
     >>> for subscription in sorted(subscriptions):
     ...     print subscription
-    Mark Shuttleworth (Self-subscribed)
-    Robert Collins (Subscribed by Mark Shuttleworth)
+    Mark Shuttleworth: Self-subscribed
+    Robert Collins: Subscribed by Mark Shuttleworth (mark)
     >>> params = CreateBugParams(
     ...     title="one more dupe test bug",
     ...     comment="one more dupe test description",
@@ -906,8 +906,8 @@
     >>> dupe_ff_bug.subscribe(foobar, lifeless)
     <lp.bugs.model.bugsubscription.BugSubscription ...>
     >>> for subscription in ff_bug.getSubscriptionsFromDuplicates():
-    ...     print '%s (%s)' % (
+    ...     print '%s: %s' % (
     ...         subscription.person.displayname,
     ...         subscription.display_duplicate_subscribed_by)
-    Scott James Remnant (Self-subscribed to bug ...)
-    Foo Bar (Subscribed to bug ... by Robert Collins)
+    Scott James Remnant: Self-subscribed to bug ...
+    Foo Bar: Subscribed to bug ... by Robert Collins (lifeless)

=== modified file 'lib/lp/bugs/javascript/subscribers_list.js'
--- lib/lp/bugs/javascript/subscribers_list.js	2011-07-20 13:35:24 +0000
+++ lib/lp/bugs/javascript/subscribers_list.js	2011-07-21 18:09:56 +0000
@@ -192,8 +192,9 @@
  *           "name": "foobar",
  *           "display_name": "Foo Bar",
  *           "can_edit": true/false,
- *           "is_team": false/false,
- *           "web_link": "https://launchpad.dev/~foobar"
+ *           "is_team": true/false,
+ *           "web_link": "https://launchpad.dev/~foobar",
+ *           "display_subscribed_by": "Matt Zimmerman (mdz)"
  *           },
  *         "subscription_level": "Details"},
  *       { "subscriber": ... }
@@ -322,7 +323,6 @@
     var subscriber = person.getAttrs();
     this.subscribers_list.addSubscriber(subscriber, 'Discussion');
     this.subscribers_list.indicateSubscriberActivity(subscriber);
-
     var loader = this;
 
     function on_success() {
@@ -687,7 +687,7 @@
  *
  * @method _createSubscriberNode
  * @param subscriber {Object} Object containing `name`, `display_name`
- *    `web_link` and `is_team` attributes.
+ *    `web_link`, `is_team` and `display_subscribed_by` attributes.
  * @return {Object} Node containing a subscriber link.
  */
 SubscribersList.prototype._createSubscriberNode = function(subscriber) {
@@ -712,6 +712,9 @@
     } else {
         subscriber_text.addClass('person');
     }
+    if (Y.Lang.isString(subscriber.display_subscribed_by)) {
+        subscriber_link.set('title', subscriber.display_subscribed_by);
+    }
     subscriber_link.appendChild(subscriber_text);
 
     subscriber_node.appendChild(subscriber_link);
@@ -728,7 +731,8 @@
  *
  * @method addSubscriber
  * @param subscriber {Object} Object containing `name`, `display_name`
- *    `web_link` and `is_team` attributes describing the subscriber.
+ *    `web_link`, `is_team`, and `display_subscribed_by` attributes describing
+ *    the subscriber.
  * @param level {String} Level of the subscription.
  * @param config {Object} Object containing potential 'unsubscribe' callback
  *     in the `unsubscribe_callback` attribute.

=== modified file 'lib/lp/bugs/javascript/tests/test_subscribers_list.js'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.js	2011-07-20 13:35:24 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.js	2011-07-21 18:09:56 +0000
@@ -780,18 +780,39 @@
         var subscriber = {
             name: 'user',
             display_name: 'User Name',
-            web_link: 'http://launchpad.net/~user'
+            web_link: 'http://launchpad.net/~user',
+            display_subscribed_by: 'Subscribed by Someone (someone)'
         };
         var node = subscribers_list._createSubscriberNode(subscriber);
         Y.Assert.isTrue(node.hasClass('subscriber'));
 
         var link = node.one('a');
         Y.Assert.areEqual('http://launchpad.net/~user', link.get('href'));
-
+        Y.Assert.areEqual(
+            'Subscribed by Someone (someone)', link.get('title'));
         var text = link.one('span');
         Y.Assert.areEqual('User Name', text.get('text'));
         Y.Assert.isTrue(text.hasClass('sprite'));
         Y.Assert.isTrue(text.hasClass('person'));
+
+    },
+
+    test_createSubscriberNode_missing_display_subscribed_by: function() {
+        // When passed a subscriber object with no 'display_subscribed_by'
+        // attribute then the title is simply not set but shows up
+        // as a null string.
+        var subscribers_list = setUpSubscribersList(this.root);
+        var subscriber = {
+            name: 'user',
+            display_name: 'User Name',
+            web_link: 'http://launchpad.net/~user'
+        };
+        var node = subscribers_list._createSubscriberNode(subscriber);
+        Y.Assert.isTrue(node.hasClass('subscriber'));
+
+        var link = node.one('a');
+        Y.Assert.areEqual('http://launchpad.net/~user', link.get('href'));
+        Y.Assert.areEqual('', link.get('title'));
     },
 
     test_createSubscriberNode_display_name_truncated: function() {
@@ -827,7 +848,7 @@
 
     test_addSubscriber: function() {
         // When there is no subscriber in the subscriber list,
-        // new node is constructed and appropriate section is added.
+        // a new node is constructed and the appropriate section is added.
         var subscribers_list = setUpSubscribersList(this.root);
         var node = subscribers_list.addSubscriber(
             { name: 'user' }, 'Details');

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-07-19 20:34:27 +0000
+++ lib/lp/bugs/model/bug.py	2011-07-21 18:09:56 +0000
@@ -939,10 +939,12 @@
 
     def getDirectSubscribersWithDetails(self):
         """See `IBug`."""
+        SubscribedBy = ClassAlias(Person, name="subscribed_by")
         results = Store.of(self).find(
-            (Person, BugSubscription),
+            (Person, SubscribedBy, BugSubscription),
             BugSubscription.person_id == Person.id,
             BugSubscription.bug_id == self.id,
+            BugSubscription.subscribed_by_id == SubscribedBy.id,
             Not(In(BugSubscription.person_id,
                    Select(BugMute.person_id, BugMute.bug_id == self.id)))
             ).order_by(Person.displayname)

=== modified file 'lib/lp/bugs/model/bugsubscription.py'
--- lib/lp/bugs/model/bugsubscription.py	2011-06-23 04:55:46 +0000
+++ lib/lp/bugs/model/bugsubscription.py	2011-07-21 18:09:56 +0000
@@ -62,10 +62,11 @@
     @property
     def display_subscribed_by(self):
         """See `IBugSubscription`."""
-        if self.person == self.subscribed_by:
+        if self.person_id == self.subscribed_by_id:
             return u'Self-subscribed'
         else:
-            return u'Subscribed by %s' % self.subscribed_by.displayname
+            return u'Subscribed by %s (%s)' % (
+                self.subscribed_by.displayname, self.subscribed_by.name)
 
     @property
     def display_duplicate_subscribed_by(self):
@@ -73,8 +74,9 @@
         if self.person == self.subscribed_by:
             return u'Self-subscribed to bug %s' % (self.bug_id)
         else:
-            return u'Subscribed to bug %s by %s' % (self.bug_id,
-                self.subscribed_by.displayname)
+            return u'Subscribed to bug %s by %s (%s)' % (
+                self.bug_id, self.subscribed_by.displayname,
+                self.subscribed_by.name)
 
     def canBeUnsubscribedByUser(self, user):
         """See `IBugSubscription`."""

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2011-07-19 14:42:52 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2011-07-21 18:09:56 +0000
@@ -205,9 +205,27 @@
             set(subscribers), set(direct_subscribers),
             "Subscribers did not match expected value.")
 
-    def test_get_direct_subscribers_with_details(self):
-        # getDirectSubscribersWithDetails() returns both
-        # Person and BugSubscription records in one go.
+    def test_get_direct_subscribers_with_details_other_subscriber(self):
+        # getDirectSubscribersWithDetails() returns
+        # Person and BugSubscription records in one go as well as the
+        # BugSubscription.subscribed_by person.
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            # Unsubscribe bug owner so it doesn't taint the result.
+            bug.unsubscribe(bug.owner, bug.owner)
+        subscriber = self.factory.makePerson()
+        subscribee = self.factory.makePerson()
+        with person_logged_in(subscriber):
+            subscription = bug.subscribe(
+                subscribee, subscriber, level=BugNotificationLevel.LIFECYCLE)
+        self.assertContentEqual(
+            [(subscribee, subscriber, subscription)],
+            bug.getDirectSubscribersWithDetails())
+
+    def test_get_direct_subscribers_with_details_self_subscribed(self):
+        # getDirectSubscribersWithDetails() returns
+        # Person and BugSubscription records in one go as well as the
+        # BugSubscription.subscribed_by person.
         bug = self.factory.makeBug()
         with person_logged_in(bug.owner):
             # Unsubscribe bug owner so it doesn't taint the result.
@@ -216,9 +234,8 @@
         with person_logged_in(subscriber):
             subscription = bug.subscribe(
                 subscriber, subscriber, level=BugNotificationLevel.LIFECYCLE)
-
         self.assertContentEqual(
-            [(subscriber, subscription)],
+            [(subscriber, subscriber, subscription)],
             bug.getDirectSubscribersWithDetails())
 
     def test_get_direct_subscribers_with_details_mute_excludes(self):