Launchpad itself

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

  1. display-dupe-in-portlet-dupe-subscribers
  2. Merge into devel
Proposed by Brian Murray
Status: Merged
Approved by: Brian Murray
Approved revision: no longer in the source branch.
Merged at revision: 11531
Proposed branch: lp:~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers
Merge into: lp:launchpad
Diff against target: 146 lines (+46/-11)
5 files modified
lib/lp/bugs/doc/bugsubscription.txt (+4/-4)
lib/lp/bugs/interfaces/bugsubscription.py (+4/-0)
lib/lp/bugs/model/bugsubscription.py (+10/-1)
lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt (+4/-4)
lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt (+24/-2)
To merge this branch: bzr merge lp:~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Robert Collins (community) Needs Fixing
Matthew Revell text Pending
Review via email: mp+34501@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

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)

Revision history for this message
Brad Crittenden (bac) wrote :

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.

Revision history for this message
Brian Murray (brian-murray) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Robert Collins (lifeless) :
review: Needs Fixing
Revision history for this message
Matthew Revell (matthew.revell) wrote :

> 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

Revision history for this message
Brian Murray (brian-murray) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Brian Murray (brian-murray) wrote :

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?

Revision history for this message
Abel Deuring (adeuring) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
2--- lib/lp/bugs/doc/bugsubscription.txt 2010-08-23 09:18:51 +0000
3+++ lib/lp/bugs/doc/bugsubscription.txt 2010-09-09 21:04:00 +0000
4@@ -863,7 +863,7 @@
5 ... print '%s (%s)' % (
6 ... subscription.person.displayname,
7 ... subscription.display_subscribed_by)
8- Mark Shuttleworth (Subscribed themselves)
9+ Mark Shuttleworth (Self-subscribed)
10 Robert Collins (Subscribed by Mark Shuttleworth)
11 >>> params = CreateBugParams(
12 ... title="one more dupe test bug",
13@@ -877,6 +877,6 @@
14 >>> for subscription in ff_bug.getSubscriptionsFromDuplicates():
15 ... print '%s (%s)' % (
16 ... subscription.person.displayname,
17- ... subscription.display_subscribed_by)
18- Foo Bar (Subscribed by Robert Collins)
19- Scott James Remnant (Subscribed themselves)
20+ ... subscription.display_duplicate_subscribed_by)
21+ Foo Bar (Subscribed to bug 28 by Robert Collins)
22+ Scott James Remnant (Self-subscribed to bug 28)
23
24=== modified file 'lib/lp/bugs/interfaces/bugsubscription.py'
25--- lib/lp/bugs/interfaces/bugsubscription.py 2010-08-20 20:31:18 +0000
26+++ lib/lp/bugs/interfaces/bugsubscription.py 2010-09-09 21:04:00 +0000
27@@ -47,6 +47,7 @@
28 "e-mail address.")))
29 bug = exported(Reference(
30 IBug, title=_("Bug"), required=True, readonly=True))
31+ bugID = Int(title=u"The bug id.", readonly=True)
32 bug_notification_level = Choice(
33 title=_("Bug notification level"), required=True,
34 vocabulary=BugNotificationLevel,
35@@ -64,6 +65,9 @@
36 display_subscribed_by = Attribute(
37 "`subscribed_by` formatted for display.")
38
39+ display_duplicate_subscribed_by = Attribute(
40+ "duplicate bug `subscribed_by` formatted for display.")
41+
42 @call_with(user=REQUEST_USER)
43 @export_read_operation()
44 def canBeUnsubscribedByUser(user):
45
46=== modified file 'lib/lp/bugs/model/bugsubscription.py'
47--- lib/lp/bugs/model/bugsubscription.py 2010-08-20 20:31:18 +0000
48+++ lib/lp/bugs/model/bugsubscription.py 2010-09-09 21:04:00 +0000
49@@ -46,10 +46,19 @@
50 def display_subscribed_by(self):
51 """See `IBugSubscription`."""
52 if self.person == self.subscribed_by:
53- return u'Subscribed themselves'
54+ return u'Self-subscribed'
55 else:
56 return u'Subscribed by %s' % self.subscribed_by.displayname
57
58+ @property
59+ def display_duplicate_subscribed_by(self):
60+ """See `IBugSubscription`."""
61+ if self.person == self.subscribed_by:
62+ return u'Self-subscribed to bug %s' % (self.bugID)
63+ else:
64+ return u'Subscribed to bug %s by %s' % (self.bugID,
65+ self.subscribed_by.displayname)
66+
67 def canBeUnsubscribedByUser(self, user):
68 """See `IBugSubscription`."""
69 if user is None:
70
71=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
72--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2009-11-06 20:28:16 +0000
73+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt 2010-09-09 21:04:00 +0000
74@@ -41,7 +41,7 @@
75 >>> browser.open(
76 ... 'http://bugs.launchpad.dev/bugs/1/+bug-portlet-subscribers-content')
77 >>> print_direct_subscribers(browser.contents)
78- Foo Bar (Subscribed themselves) (Unsubscribe Foo Bar)
79+ Foo Bar (Self-subscribed) (Unsubscribe Foo Bar)
80 Sample Person (Subscribed by Launchpad Janitor)
81 Steve Alexander (Subscribed by Launchpad Janitor)
82
83@@ -208,7 +208,7 @@
84 ... "http://launchpad.dev/bugs/3/+bug-portlet-dupe-subscribers-content")
85 >>> print_subscribers_from_duplicates(stevea_browser.contents)
86 From duplicates:
87- Steve Alexander (Subscribed by Launchpad Janitor)
88+ Steve Alexander (Subscribed to bug 2 by Launchpad Janitor)
89 (Unsubscribe Steve Alexander)
90
91 >>> stevea_browser.getLink(id='unsubscribe-subscriber-11').mech_link.url
92@@ -267,7 +267,7 @@
93 From duplicates:
94 Sample Person (Subscribed ...)
95 Steve Alexander (Subscribed ...) (Unsubscribe Steve Alexander)
96- testing Spanish team (Subscribed by Foo Bar)
97+ testing Spanish team (Subscribed to bug 1 by Foo Bar)
98
99 >>> stevea_browser.open(
100 ... "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
101@@ -309,7 +309,7 @@
102
103 >>> print_subscribers_from_duplicates(foobar_browser.contents)
104 From duplicates:
105- Ubuntu Team (Subscribed by Foo Bar) (Unsubscribe Ubuntu Team)
106+ Ubuntu Team (Subscribed to bug 2 by Foo Bar) (Unsubscribe Ubuntu Team)
107
108 >>> foobar_browser.open(
109 ... "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
110
111=== modified file 'lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt'
112--- lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt 2009-11-05 19:01:12 +0000
113+++ lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt 2010-09-09 21:04:00 +0000
114@@ -20,8 +20,30 @@
115 id string:dupe-${subscription/css_name};
116 "
117 >
118- <metal:subscriber
119- metal:use-macro="bug/@@+bug-portlet-subscribers-content/subscriber-row" />
120+
121+ <a
122+ tal:condition="subscription/person/name|nothing"
123+ tal:attributes="
124+ href subscription/person/fmt:url;
125+ title subscription/display_duplicate_subscribed_by;
126+ name subscription/person/fmt:displayname
127+ "
128+ >
129+ <tal:block replace="structure subscription/person/fmt:icon" />
130+ <tal:block replace="subscription/person/fmt:displayname/fmt:shorten/20" />
131+ </a>
132+
133+ <a tal:condition="python: subscription.canBeUnsubscribedByUser(view.user)"
134+ href="+subscribe"
135+ tal:attributes="
136+ title string:Unsubscribe ${subscription/person/fmt:displayname};
137+ id string:unsubscribe-${subscription/css_name};
138+ class python: view.getSubscriptionClassForUser(subscription.person)
139+ "
140+ >
141+ <img class="unsub-icon" src="/@@/remove"
142+ tal:attributes="id string:unsubscribe-icon-${subscription/css_name}" />
143+ </a>
144 </div>
145 </div>
146 </div>