Merge lp:~benji/launchpad/bug-777786 into lp:launchpad

[Benji York]

[r=gmb][bug=777786] instead of an ineffectual "stop email" link, give users the option to contact the subscribing team

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.

[Graham Binns (gmb)]

review: Approve (code)

=== 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-18 14:32:15 +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/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-05-18 14:32:15 +0000
@@ -255,9 +255,15 @@
 
     def isMuteAllowed(self, person):
         """See `IBugSubscriptionFilter`."""
+        subscriber = self.structural_subscription.subscriber
+        # The person can mute the Subscription if the subscription is via a
+        # team of which they are a member and the team doesn't have a contact
+        # address (because if the team does, then the mute would be
+        # ineffectual).
         return (
-            self.structural_subscription.subscriber.isTeam() and
-            person.inTeam(self.structural_subscription.subscriber))
+            subscriber.isTeam() and
+            person.inTeam(subscriber) and
+            subscriber.preferredemail is None)
 
     def muted(self, person):
         store = Store.of(self)

=== 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-18 14:32:15 +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-18 14:32:15 +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,11 +1304,11 @@
 /**
  * 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) {
+    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);
@@ -1335,23 +1335,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>&nbsp;</span>'));
@@ -1362,7 +1370,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 +1384,56 @@
 
     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) {
+        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 +1441,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 +1472,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 +1689,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-18 14:32:15 +0000
@@ -716,6 +716,160 @@
     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: false,
+                    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();
+            Assert.areEqual(
+                -1,
+                content.search(/Stop my emails/));
+        }
+    }));
+
+    suite.add(new Y.Test.Case({
         name: 'Structural Subscription: deleting failed filters',
 
         _should: {error: {}},