launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04314
[Merge] lp:~bac/launchpad/bug-799901 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-799901 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #799901 in Launchpad itself: ""subscribed by" omitted from the title of links in subscriber list"
https://bugs.launchpad.net/launchpad/+bug/799901
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-799901/+merge/68590
= 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.
--
https://code.launchpad.net/~bac/launchpad/bug-799901/+merge/68590
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-799901 into lp:launchpad.
=== 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-20 18:58:28 +0000
@@ -553,6 +553,7 @@
'self_link': absoluteURL(person, api_request),
'is_team': person.is_team,
'can_edit': can_edit,
+ 'subscribed_by': subscription.display_subscribed_by,
}
record = {
'subscriber': subscriber,
=== 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-20 18:58:28 +0000
@@ -487,6 +487,38 @@
'can_edit': False,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ '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):
+ # A subscriber_data_js returns JSON string of a list
+ # containing all subscriber information needed for
+ # subscribers_list.js subscribers loading.
+ 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),
+ 'subscribed_by': 'Subscribed by Someone (someone)',
},
'subscription_level': "Lifecycle",
}
@@ -497,8 +529,10 @@
# 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 +547,7 @@
'can_edit': False,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ 'subscribed_by': 'Subscribed by Team Owner (team-owner)',
},
'subscription_level': "Lifecycle",
}
@@ -523,8 +558,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 +577,7 @@
'can_edit': True,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ 'subscribed_by': 'Subscribed by Team Owner (team-owner)',
},
'subscription_level': "Lifecycle",
}
@@ -552,8 +590,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 +610,7 @@
'can_edit': True,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ 'subscribed_by': 'Subscribed by Team Owner (team-owner)',
},
'subscription_level': "Lifecycle",
}
@@ -598,6 +640,7 @@
'can_edit': True,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ 'subscribed_by': 'Self-subscribed',
},
'subscription_level': "Lifecycle",
}
@@ -615,7 +658,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 +673,7 @@
'can_edit': True,
'web_link': canonical_url(subscriber),
'self_link': absoluteURL(subscriber, api_request),
+ '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-20 18:58:28 +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-20 18:58:28 +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";,
+ * "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 `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.subscribed_by)) {
+ subscriber_link.set('title', subscriber.subscribed_by);
+ }
subscriber_link.appendChild(subscriber_text);
subscriber_node.appendChild(subscriber_link);
@@ -728,7 +731,7 @@
*
* @method addSubscriber
* @param subscriber {Object} Object containing `name`, `display_name`
- * `web_link` and `is_team` attributes describing the subscriber.
+ * `web_link`, `is_team`, and `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-20 18:58:28 +0000
@@ -780,18 +780,38 @@
var subscriber = {
name: 'user',
display_name: 'User Name',
- web_link: 'http://launchpad.net/~user'
+ web_link: 'http://launchpad.net/~user',
+ 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_subscribed_by: function() {
+ // When passed a subscriber object with no '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 +847,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/bugsubscription.py'
--- lib/lp/bugs/model/bugsubscription.py 2011-06-23 04:55:46 +0000
+++ lib/lp/bugs/model/bugsubscription.py 2011-07-20 18:58:28 +0000
@@ -65,7 +65,8 @@
if self.person == self.subscribed_by:
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`."""
Follow ups
