Merge lp:~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers into lp:launchpad

[Brian Murray]

Display the number of the duplicate bug to which a subscriber is subscribed in the subscriptions from duplicates portlet.

When a bug has a lot of duplicates and subscribers from duplicates it would be useful to know which duplicate you were subscribed to without checking every single duplicate bug to find out.

Instead you can mouse over your name in the subscription and find out to which one you are subscribed.

Test modified:

bin/test -cvvt bugsubscription.txt xx-bug-personal-subscriptions.txt

I'd be happy to continue using the macro"bug/@@+bug-portlet-subscribers-content/subscriber-row" if you've any ideas on how to replace the title in it. I couldn't figure out a way to do that.

[Robert Collins (lifeless)]

Two small points: have you checked the query impact of this? It looks
like it will do a lot of queries, to me.

Separately, have you considered just having the list of 'can
unsubscribe' things include separate actions for each bug.

e.g.
subscribers
teamfoo [-]
teamfoo (via 1234) [-]
teambar (via 3456) [-]

This would keep the unsubscribe actions in the same place (a good
thing), not make showing the list of dupes more expensive ( a good
thing) and leave the dup list cacheable if we choose to) (a good
thing)

[Brad Crittenden (bac)]

I'm adding Matthew Revell for a text review. Personally I think the phrase "Subscribed themselves" needs to be reworked. I hate using a the plural to represent a person of non-determined gender in general but it seem particularly clumsy here. Matthew may disagree.

[Brian Murray (brian-murray)]

Only one additional query, per duplicate subscriber, is ran with this change. Looking at the bug-portlet-dupe-subscribers-content page w/o the changes 27 queries are issued and with the change 28 queries are issued for a bug with one duplicate subscriber.

No, I did not consider reworking the subscribers portlet. Primarily, because I happened to learning about page templates and how they work and remembered a bug I'd reported and thought it'd be a quick and easy change. However, the subscriber name is actually shortened (tal:block replace="subscription/person/fmt:displayname/fmt:shorten/20"), to make it fit in the portlet, so that might not work so well.

[Robert Collins (lifeless)]

On Thu, Sep 9, 2010 at 8:57 AM, Brian Murray <email address hidden> wrote:
> Only one additional query, per duplicate subscriber, is ran with this change.

Thats *huge*. Alarm bells should be going off in your head right now.

Bugs commonly peak at 40 or more duplicates.
Every one of those duplicates will have *at least* one subscriber (the
filer) + structural subscribptions [perhaps not impacted].

>  Looking at the bug-portlet-dupe-subscribers-content page w/o the changes 27 queries are issued and with the change 28 queries are issued for a bug with one duplicate subscriber.

Adding 1 is a boundary case: when testing, please always add 2, or 10,
to be really sure.

> No, I did not consider reworking the subscribers portlet.  Primarily, because I happened to learning about page templates and how they work and remembered a bug I'd reported and thought it'd be a quick and easy change.  However, the subscriber name is actually shortened (tal:block replace="subscription/person/fmt:displayname/fmt:shorten/20"), to make it fit in the portlet, so that might not work so well.

It seems nicer to rework the portlet to me. Anyhow, with the scaling
factor you have this change will cause performance regressions if
landed, so I'm going to set it to WIP & needs fixing.

Thanks,
Rob

[Robert Collins (lifeless)]

review: Needs Fixing

[Matthew Revell (matthew.revell)]

> I'm adding Matthew Revell for a text review. Personally I think the phrase
> "Subscribed themselves" needs to be reworked. I hate using a the plural to
> represent a person of non-determined gender in general but it seem
> particularly clumsy here. Matthew may disagree.

"Themself" would be the more correct use of the unspecified sex pronoun. While I'm comfortable with the singular "they", I'm not sure it reads that well here. I don't have particularly good suggestions for a replacement, though.

Suggested alternative: Self-subscribed/Self-subscribed to bug xx

Interesting notes on usage of singular "they" -- http://www.oxforddictionaries.com/definition/themself and http://www.oxforddictionaries.com/view/entry/m_en_gb0858560#DWS-055164

[Brian Murray (brian-murray)]

I've modified the bugsubscription interface to include a bugID which reduces the quantity of database queries as shown below. I hope this is sufficient.

# of dup subs Devel Old Branch New Branch
0 24,25 24 24,25
1 26,27 27 26
2 28 29 28
3 29 31,32 29

I've also changed the wording from "Subscribed themselves" to "Self-subscribed" as suggested by Matthew.

[Robert Collins (lifeless)]

The table formatted awkwardly.
Can you confirm:
(dup subs, devel, old proposal, current proposal)
(0, 24, 24, 25)
(1, 26, 27, 26)
(2, 28, 29, 28)
(3, 29, 32, 29)

This is certainly better. It appears to still scale with duplication
subscriptions which is a problem. (And the query count is still quite
large: for a portlet like this, 10-15 queries is what I'd expect).

I haven't reviewed the code yet, but on the sheer performance size,
these query counts (which are only a surrogate) are at least no-worse.

If you want to grab an OCR to review this your friday, please do :
don't block on me looking at the actual change - whomever reviews it
can advise on the tastefulness of your approach to the scaling issue.

[Brian Murray (brian-murray)]

The table data is correct. I had to convert the portlet to a full page to get the query count so perhaps that has something to do with the quantity of queries?

[Abel Deuring (adeuring)]

Brian,

this is a _very_ useful improvement, I think!

> When a bug has a lot of duplicates and subscribers from duplicates
> it would be useful to know which duplicate you were subscribed to
> without checking every single duplicate bug to find out.
>
> Instead you can mouse over your name in the subscription and find
> out to which one you are subscribed.
>
> Test modified:
>
> bin/test -cvvt bugsubscription.txt xx-bug-personal-subscriptions.txt
>
> I'd be happy to continue using the macro
> "bug/@@+bug-portlet-subscribers-content/subscriber-row" if you've any
> ideas on how to replace the title in it. I couldn't figure out a way
> to do that.

I think you replace the model class properties display_subscribed_by and
display_duplicate_subscribed_by by methods displayDuplicateSubscribedBy()
of the browser classes
lp.bugs.browser.bugsubscription.BugPortletDuplicateSubcribersContents
and lp.bugs.browser.bugsubscription.BugPortletSubcribersContents

But the current variant is fine for me too.

> [Robert:]
>
> The table formatted awkwardly.
> Can you confirm:
> (dup subs, devel, old proposal, current proposal)
> (0, 24, 24, 25)
> (1, 26, 27, 26)
> (2, 28, 29, 28)
> (3, 29, 32, 29)
>
> This is certainly better. It appears to still scale with duplication
> subscriptions which is a problem. (And the query count is still quite
> large: for a portlet like this, 10-15 queries is what I'd expect).
>
> I haven't reviewed the code yet, but on the sheer performance size,
> these query counts (which are only a surrogate) are at least no-worse.
>
> If you want to grab an OCR to review this your friday, please do :
> don't block on me looking at the actual change - whomever reviews it
> can advise on the tastefulness of your approach to the scaling issue.

So we have one more query for zero duplicate subscribers, and the same
number of queries for one or more subscribers. I think this is fine.

Reducing the number of queries is something for another branch.
(pre-loading the subscriber and subscribed_by objects in the query
that retrieves the subscriptions comes to mind)

review: Approve (code)

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2010-08-23 09:18:51 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2010-09-09 21:04:00 +0000
@@ -863,7 +863,7 @@
     ...     print '%s (%s)' % (
     ...         subscription.person.displayname,
     ...         subscription.display_subscribed_by)
-    Mark Shuttleworth (Subscribed themselves)
+    Mark Shuttleworth (Self-subscribed)
     Robert Collins (Subscribed by Mark Shuttleworth)
     >>> params = CreateBugParams(
     ...     title="one more dupe test bug",
@@ -877,6 +877,6 @@
     >>> for subscription in ff_bug.getSubscriptionsFromDuplicates():
     ...     print '%s (%s)' % (
     ...         subscription.person.displayname,
-    ...         subscription.display_subscribed_by)
-    Foo Bar (Subscribed by Robert Collins)
-    Scott James Remnant (Subscribed themselves)
+    ...         subscription.display_duplicate_subscribed_by)
+    Foo Bar (Subscribed to bug 28 by Robert Collins)
+    Scott James Remnant (Self-subscribed to bug 28)

=== modified file 'lib/lp/bugs/interfaces/bugsubscription.py'
--- lib/lp/bugs/interfaces/bugsubscription.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bugsubscription.py	2010-09-09 21:04:00 +0000
@@ -47,6 +47,7 @@
         "e-mail address.")))
     bug = exported(Reference(
         IBug, title=_("Bug"), required=True, readonly=True))
+    bugID = Int(title=u"The bug id.", readonly=True)
     bug_notification_level = Choice(
         title=_("Bug notification level"), required=True,
         vocabulary=BugNotificationLevel,
@@ -64,6 +65,9 @@
     display_subscribed_by = Attribute(
         "`subscribed_by` formatted for display.")
 
+    display_duplicate_subscribed_by = Attribute(
+        "duplicate bug `subscribed_by` formatted for display.")
+
     @call_with(user=REQUEST_USER)
     @export_read_operation()
     def canBeUnsubscribedByUser(user):

=== modified file 'lib/lp/bugs/model/bugsubscription.py'
--- lib/lp/bugs/model/bugsubscription.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugsubscription.py	2010-09-09 21:04:00 +0000
@@ -46,10 +46,19 @@
     def display_subscribed_by(self):
         """See `IBugSubscription`."""
         if self.person == self.subscribed_by:
-            return u'Subscribed themselves'
+            return u'Self-subscribed'
         else:
             return u'Subscribed by %s' % self.subscribed_by.displayname
 
+    @property
+    def display_duplicate_subscribed_by(self):
+        """See `IBugSubscription`."""
+        if self.person == self.subscribed_by:
+            return u'Self-subscribed to bug %s' % (self.bugID)
+        else:
+            return u'Subscribed to bug %s by %s' % (self.bugID,
+                self.subscribed_by.displayname)
+
     def canBeUnsubscribedByUser(self, user):
         """See `IBugSubscription`."""
         if user is None:

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2009-11-06 20:28:16 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2010-09-09 21:04:00 +0000
@@ -41,7 +41,7 @@
   >>> browser.open(
   ...     'http://bugs.launchpad.dev/bugs/1/+bug-portlet-subscribers-content')
   >>> print_direct_subscribers(browser.contents)
-  Foo Bar (Subscribed themselves) (Unsubscribe Foo Bar)
+  Foo Bar (Self-subscribed) (Unsubscribe Foo Bar)
   Sample Person (Subscribed by Launchpad Janitor)
   Steve Alexander (Subscribed by Launchpad Janitor)
 
@@ -208,7 +208,7 @@
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-dupe-subscribers-content")
   >>> print_subscribers_from_duplicates(stevea_browser.contents)
   From duplicates:
-  Steve Alexander (Subscribed by Launchpad Janitor)
+  Steve Alexander (Subscribed to bug 2 by Launchpad Janitor)
                   (Unsubscribe Steve Alexander)
 
   >>> stevea_browser.getLink(id='unsubscribe-subscriber-11').mech_link.url
@@ -267,7 +267,7 @@
   From duplicates:
   Sample Person (Subscribed ...)
   Steve Alexander (Subscribed ...) (Unsubscribe Steve Alexander)
-  testing Spanish team (Subscribed by Foo Bar)
+  testing Spanish team (Subscribed to bug 1 by Foo Bar)
 
   >>> stevea_browser.open(
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
@@ -309,7 +309,7 @@
 
   >>> print_subscribers_from_duplicates(foobar_browser.contents)
   From duplicates:
-  Ubuntu Team (Subscribed by Foo Bar) (Unsubscribe Ubuntu Team)
+  Ubuntu Team (Subscribed to bug 2 by Foo Bar) (Unsubscribe Ubuntu Team)
 
   >>> foobar_browser.open(
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")

=== modified file 'lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt'
--- lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt	2009-11-05 19:01:12 +0000
+++ lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt	2010-09-09 21:04:00 +0000
@@ -20,8 +20,30 @@
         id string:dupe-${subscription/css_name};
       "
     >
-      <metal:subscriber
-          metal:use-macro="bug/@@+bug-portlet-subscribers-content/subscriber-row" />
+
+            <a
+               tal:condition="subscription/person/name|nothing"
+               tal:attributes="
+                 href subscription/person/fmt:url;
+                 title subscription/display_duplicate_subscribed_by;
+                 name subscription/person/fmt:displayname
+               "
+            >
+              <tal:block replace="structure subscription/person/fmt:icon" />
+              <tal:block replace="subscription/person/fmt:displayname/fmt:shorten/20" />
+            </a>
+
+            <a tal:condition="python: subscription.canBeUnsubscribedByUser(view.user)"
+               href="+subscribe"
+               tal:attributes="
+                 title string:Unsubscribe ${subscription/person/fmt:displayname};
+                 id string:unsubscribe-${subscription/css_name};
+                 class python: view.getSubscriptionClassForUser(subscription.person)
+               "
+            >
+              <img class="unsub-icon" src="/@@/remove"
+                tal:attributes="id string:unsubscribe-icon-${subscription/css_name}" />
+            </a>
     </div>
   </div>
 </div>