launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03654
[Merge] lp:~benji/launchpad/bug-777786 into lp:launchpad
Benji York has proposed merging lp:~benji/launchpad/bug-777786 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-777786/+merge/61274
Bug 777786 notes that it's somewhat rude to show users a "Stop my emails
from this subscription" for subscriptions via teams that have a contact
address because it won't actually stop the email. This branch replaces
that link with a link to contact the team with a pre-populated message
that the user can edit.
I also fixed a bit of lint in lib/lp/registry/interfaces/mailinglist.py.
The make lint report is clean.
The tests are in lib/lp/registry/javascript/tests/test_structural_subscription.js.
--
https://code.launchpad.net/~benji/launchpad/bug-777786/+merge/61274
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-777786 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py 2011-05-10 15:16:05 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py 2011-05-17 15:59:31 +0000
@@ -14,7 +14,10 @@
'StructuralSubscribersPortletView',
]
-from operator import attrgetter
+from operator import (
+ attrgetter,
+ itemgetter,
+ )
from lazr.restful.interfaces import (
IJSONRequestCache,
@@ -491,20 +494,33 @@
subscriber = subscription.subscriber
for filter in subscription.bug_filters:
is_team = subscriber.isTeam()
- user_is_team_admin = (is_team and
- subscriber in administered_teams)
+ user_is_team_admin = (
+ is_team and subscriber in administered_teams)
+ team_has_contact_address = (
+ is_team and subscriber.preferredemail is not None)
+ mailing_list = subscriber.mailing_list
+ user_is_on_team_mailing_list = (
+ team_has_contact_address and
+ mailing_list is not None and
+ mailing_list.is_usable and
+ mailing_list.getSubscription(subscriber) is not None)
record['filters'].append(dict(
filter=filter,
subscriber_link=absoluteURL(subscriber, api_request),
- subscriber_url = canonical_url(
+ subscriber_url=canonical_url(
subscriber, rootsite='mainsite'),
+ target_bugs_url=canonical_url(
+ target, rootsite='bugs'),
subscriber_title=subscriber.title,
subscriber_is_team=is_team,
user_is_team_admin=user_is_team_admin,
+ team_has_contact_address=team_has_contact_address,
+ user_is_on_team_mailing_list=user_is_on_team_mailing_list,
can_mute=filter.isMuteAllowed(user),
- is_muted=filter.muted(user) is not None))
+ is_muted=filter.muted(user) is not None,
+ target_title=target.title))
info = info.values()
- info.sort(key=lambda item: item['target_url'])
+ info.sort(key=itemgetter('target_url'))
IJSONRequestCache(request).objects['subscription_info'] = info
=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py 2011-05-12 21:33:10 +0000
+++ lib/lp/registry/interfaces/mailinglist.py 2011-05-17 15:59:31 +0000
@@ -110,7 +110,7 @@
INACTIVE = DBItem(7, """
Inactive
- A previously active mailing lit has been made inactive by its team
+ A previously active mailing list has been made inactive by its team
owner.
""")
@@ -132,7 +132,7 @@
The mailing list has been flagged for deactivation by the team owner.
Mailman will be informed of this and will take the necessary actions
- to deactive the list.
+ to deactivate the list.
""")
MOD_FAILED = DBItem(11, """
@@ -246,8 +246,7 @@
title=_('Review date'),
description=_('The date on which this mailing list registration was '
'reviewed, or None if the registration has not yet '
- 'been reviewed.')
- )
+ 'been reviewed.'))
date_activated = Datetime(
title=_('Activation date'),
@@ -255,8 +254,7 @@
'meaning that the Mailman process has successfully '
'created it. This may be None if the mailing list '
'has not yet been activated, or that its activation '
- 'has failed.')
- )
+ 'has failed.'))
status = Choice(
title=_('Status'),
@@ -434,8 +432,8 @@
def getReviewableMessages(message_id_filter=None):
"""Return the set of all held messages for this list requiring review.
- :param message_id_filter: If supplied only messages with message ids in
- the filter are returned.
+ :param message_id_filter: If supplied only messages with message ids
+ in the filter are returned.
:return: A sequence of `IMessageApproval`s for this mailing list,
where the status is `PostedMessageStatus.NEW`. The returned set
is ordered first by the date the message was posted, then by
=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js 2011-05-11 21:01:58 +0000
+++ lib/lp/registry/javascript/structural-subscription.js 2011-05-17 15:59:31 +0000
@@ -1239,7 +1239,7 @@
} else {
filter_info.is_muted = false;
}
- handle_mute(node, filter_info.is_muted);
+ handle_mute(node, filter_info);
},
failure: error_handler.getFailureHandler()
}
@@ -1304,20 +1304,27 @@
/**
* For a given filter node, set it up properly based on mute state.
*/
-function handle_mute(node, muted) {
+function handle_mute(node, filter_info) {
var control = node.one('a.mute-subscription');
var label = node.one('em.mute-label');
var description = node.one('.filter-description');
- if (muted) {
- control.set('text', 'Send my emails for this subscription');
- control.replaceClass(MUTE_ICON_CLASS, UNMUTE_ICON_CLASS);
- label.setStyle('display', null);
- description.setStyle('color', '#bbb');
+ if (!filter_info.team_has_contact_address) {
+ // If the subscriber is a team that has a contact address, then the
+ // user muting the subscription won't do anything, so don't show them
+ // an option that won't do them any good.
+ if (filter_info.is_muted) {
+ control.set('text', 'Send my emails for this subscription');
+ control.replaceClass(MUTE_ICON_CLASS, UNMUTE_ICON_CLASS);
+ label.setStyle('display', null);
+ description.setStyle('color', '#bbb');
+ } else {
+ control.set('text', 'Stop my emails from this subscription');
+ control.replaceClass(UNMUTE_ICON_CLASS, MUTE_ICON_CLASS);
+ label.setStyle('display', 'none');
+ description.setStyle('color', null);
+ }
} else {
- control.set('text', 'Stop my emails from this subscription');
- control.replaceClass(UNMUTE_ICON_CLASS, MUTE_ICON_CLASS);
label.setStyle('display', 'none');
- description.setStyle('color', null);
}
}
@@ -1335,23 +1342,31 @@
'<strong class="filter-name"></strong>'));
if (filter_info.can_mute) {
- filter_node.appendChild(Y.Node.create(
- '<em class="mute-label" style="padding-left: 1em;">You '+
- 'do not receive emails from this subscription.</em>'));
+ filter_node.append(Y.Node.create('<em/>')
+ .set('text', 'You do not receive emails from this subscription.')
+ .addClass('mute-label')
+ .setStyle('paddingLeft', '1em'));
}
var control = filter_node.appendChild(
Y.Node.create('<span style="float: right"></span>'));
if (filter_info.can_mute) {
- var link = control.appendChild(Y.Node.create(
- '<a href="#" class="sprite js-action mute-subscription"></a>'));
- var help = control.appendChild(Y.Node.create(
- '<a target="help" class="sprite maybe mute-help"'+
- ' style="visibility: hidden;"'+
- ' href="/+help/structural-subscription-mute.html">'+
- ' <span class="invisible-link">Delivery help</span>'+
- '</a>'));
+ var link = control.appendChild(Y.Node.create('<a/>')
+ .set('href', '#')
+ .addClass('sprite')
+ .addClass('js-action')
+ .addClass('mute-subscription'));
+ var help = control.appendChild(Y.Node.create('<a/>')
+ .set('href', '/+help/structural-subscription-mute.html')
+ .set('target', 'help')
+ .addClass('sprite')
+ .addClass('maybe')
+ .addClass('mute-help')
+ .setStyle('visibility', 'hidden')
+ .append(Y.Node.create('<span/>')
+ .addClass('invisible-link')
+ .set('text', 'Delivery help')));
// For some reason the help link will not appear in Chrome unless
// there is a non-empty element immediately after the help node.
control.append(Y.Node.create('<span> </span>'));
@@ -1362,7 +1377,7 @@
help.setStyle('visibility', 'visible');
// Every time we trigger the display of the help link we need to
// cancel any pending hiding of the help link so it doesn't
- // dissapear on us. If there isn't one pending, this is a NOP.
+ // disappear on us. If there isn't one pending, this is a NOP.
clearTimeout(hide_help_timeout);
};
var hide_help = function () {
@@ -1376,20 +1391,57 @@
if (can_edit(filter_info)) {
// User can edit the subscription.
- control.append(Y.Node.create(
- '<a href="#" style="margin-right: 2em;"'+
- ' class="sprite modify edit js-action edit-subscription">'+
- ' Edit this subscription</a>'+
- '<a href="#" class="sprite modify remove js-action '+
- ' delete-subscription">Unsubscribe</a>'));
- }
-
- filter_node.appendChild(
- Y.Node.create('<div style="padding-left: 1em" '+
- 'class="filter-description"></div>'));
+ control.append(Y.Node.create('<a/>')
+ .set('href', '#')
+ .set('text', 'Edit this subscription')
+ .setStyle('marginRight', '2em')
+ .addClass('sprite')
+ .addClass('modify')
+ .addClass('edit')
+ .addClass('js-action')
+ .addClass('edit-subscription'));
+ control.append(Y.Node.create('<a/>')
+ .set('href', '#')
+ .set('text', 'Unsubscribe')
+ .addClass('sprite')
+ .addClass('modify')
+ .addClass('remove')
+ .addClass('js-action')
+ .addClass('delete-subscription'));
+ }
+
+ if (filter_info.team_has_contact_address
+ && !filter_info.user_is_team_admin) {
+ console.log(filter_info);
+ var subject = urlEncode('Team contact address and subscriptions');
+ var user_participation;
+ if (filter_info.user_is_on_team_mailing_list) {
+ user_participation = 'subscribe to the team\'s mailing list';
+ } else {
+ user_participation = 'be a part of the team';
+ }
+ var message = urlEncode(
+ 'Hello. I receive email notifications about bugs in '+
+ filter_info.target_title+' because of a team subscription for '+
+ filter_info.subscriber_title+'. I would like to continue to '+
+ user_participation+', but I would like to receive less email '+
+ 'from this subscription. Could you remove the team contact '+
+ 'email address so that the team members can manage their own '+
+ 'subscriptions (see '+filter_info.subscriber_url+'), or delete '+
+ 'or reduce level of the subscription itself (see '+
+ filter_info.target_bugs_url+'/+subscriptions)?\n\nThank you.');
+ control.append(Y.Node.create('<a/>')
+ .set('href', filter_info.subscriber_url+'/+contactuser'+
+ '?field.message='+message+'&field.subject='+subject)
+ .set('text', 'Request team administrators change'));
+ }
+
+ filter_node.append(Y.Node.create('<div/>')
+ .setStyle('paddingLeft', '1em')
+ .addClass('filter-description'));
if (filter_info.can_mute) {
- handle_mute(filter_node, filter_info.is_muted);
+ handle_mute(filter_node, filter_info);
}
fill_filter_description(filter_node, filter_info, filter);
@@ -1397,6 +1449,10 @@
return filter_node;
}
+// Expose in the namespace for testing purposes.
+namespace._create_filter_node = create_filter_node;
+
+
/**
* Create a node with subscription description.
*/
@@ -1424,7 +1480,7 @@
// and see the information you expect to see.
LP.cache['structural-subscription-filter-'+filter_id.toString()] =
filter;
- node.appendChild(create_filter_node(filter_id, filter_info, filter));
+ node.append(create_filter_node(filter_id, filter_info, filter));
filter_id += 1;
}
return node;
@@ -1641,7 +1697,7 @@
var team_info = get_team_info(form_data);
var filter_info = {
- filter : filter,
+ filter: filter,
subscriber_is_team: team_info.is_team,
user_is_team_admin: team_info.is_team,
can_mute: team_info.is_team,
=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-05-11 21:05:17 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-05-17 15:59:31 +0000
@@ -716,6 +716,159 @@
suite.add(test_case);
suite.add(new Y.Test.Case({
+ name: 'Structural Subscription team contact',
+
+ _should: {error: {}},
+
+ setUp: function() {
+ // Monkeypatch LP to avoid network traffic and to allow
+ // insertion of test data.
+ this.original_lp = monkeypatch_LP();
+
+ LP.cache.subscription_info = [{
+ target_url: 'http://example.com',
+ target_title:'Example project',
+ filters: [{
+ filter: {
+ description: 'DESCRIPTION',
+ statuses: [],
+ importances: [],
+ tags: [],
+ find_all_tags: true,
+ bug_notification_level: 'Discussion',
+ self_link: 'http://example.com/a_filter'
+ },
+ can_mute: true,
+ is_muted: false,
+ team_has_contact_address: true,
+ user_is_team_admin: false,
+ subscriber_is_team: true,
+ subscriber_url: 'http://example.com/subscriber',
+ subscriber_title: 'Thidwick'
+ }]
+ }];
+ this.configuration = {
+ content_box: content_box_id,
+ lp_client: make_lp_client_stub()
+ };
+
+ this.content_node = create_test_node();
+ Y.one('body').appendChild(this.content_node);
+ },
+
+ tearDown: function() {
+ window.LP = this.original_lp;
+ remove_test_node();
+ delete this.content_node;
+ },
+
+ test_administrative_change_link: function() {
+ var filter_info = {
+ filter: {
+ description: 'DESCRIPTION',
+ statuses: [],
+ importances: [],
+ tags: [],
+ find_all_tags: true,
+ bug_notification_level: 'Discussion',
+ self_link: 'http://example.com/a_filter'
+ },
+ can_mute: true,
+ is_muted: false,
+ team_has_contact_address: true,
+ user_is_team_admin: false,
+ user_is_on_team_mailing_list: true,
+ subscriber_is_team: true,
+ subscriber_url: 'http://example.com/subscriber',
+ subscriber_title: 'Thidwick'
+ };
+ var node = module._create_filter_node(
+ 'ID', filter_info, filter_info.filter);
+ var content = node.getContent();
+ // If a subscription is via a team and the user isn't a team
+ // admin and the team has a contact address, the user gets a link
+ // to request the administrators change the subscription or drop
+ // the contact address.
+ Assert.areNotEqual(
+ -1,
+ content.search(/Request team administrators change/));
+ // If the team's contact address is to a (launchpad-managed)
+ // mailing list, then the pre-filled in email message is phrased
+ // accordingly.
+ Assert.areNotEqual(
+ -1,
+ content.search(/subscribe%20to%20the%20team/));
+ },
+
+ test_administrative_change_link_no_mailing_list: function() {
+ var filter_info = {
+ filter: {
+ description: 'DESCRIPTION',
+ statuses: [],
+ importances: [],
+ tags: [],
+ find_all_tags: true,
+ bug_notification_level: 'Discussion',
+ self_link: 'http://example.com/a_filter'
+ },
+ can_mute: true,
+ is_muted: false,
+ team_has_contact_address: true,
+ user_is_team_admin: false,
+ user_is_on_team_mailing_list: false,
+ subscriber_is_team: true,
+ subscriber_url: 'http://example.com/subscriber',
+ subscriber_title: 'Thidwick'
+ };
+ var node = module._create_filter_node(
+ 'ID', filter_info, filter_info.filter);
+ var content = node.getContent();
+ // If a subscription is via a team and the user isn't a team
+ // admin and the team has a contact address, the user gets a link
+ // to request the administrators change the subscription or drop
+ // the contact address.
+ Assert.areNotEqual(
+ -1,
+ content.search(/Request team administrators change/));
+ // If the team's contact address is not a (launchpad-managed)
+ // mailing list, then the pre-filled in email message is phrased
+ // accordingly.
+ Assert.areNotEqual(
+ -1,
+ content.search(/be%20a%20part%20of%20the%20team/));
+ },
+
+ test_mute_not_shown_when_ineffectual: function() {
+ // If muting the subscription in question won't have an effect,
+ // then the mute link isn't shown.
+ var filter_info = {
+ filter: {
+ description: 'DESCRIPTION',
+ statuses: [],
+ importances: [],
+ tags: [],
+ find_all_tags: true,
+ bug_notification_level: 'Discussion',
+ self_link: 'http://example.com/a_filter'
+ },
+ can_mute: true,
+ is_muted: false,
+ team_has_contact_address: true,
+ user_is_team_admin: false,
+ user_is_on_team_mailing_list: true,
+ subscriber_is_team: true,
+ subscriber_url: 'http://example.com/subscriber',
+ subscriber_title: 'Thidwick'
+ };
+ var node = module._create_filter_node(
+ 'ID', filter_info, filter_info.filter);
+ Assert.areEqual(
+ -1,
+ content.search(/Stop my emails/));
+ }
+ }));
+
+ suite.add(new Y.Test.Case({
name: 'Structural Subscription: deleting failed filters',
_should: {error: {}},
