Merge lp:~sinzui/launchpad/contact-team into lp:launchpad

[Curtis Hovey]

[r=bac][bug=246022] Allow team members to contact all the team members, non-members contact the team admins.

Allow team members to contact all the team members, non-members contact the
team admins.

    Launchpad bug: https://bugs.launchpad.net/bugs/246022
    Pre-implementation: lifeless

A team administrator cannot contact all members at once if, for example,
the contact address for the team is a mailing list, and not all members
are subscribed to it.

This issues has mutated over the years. When bug mail was out of control,
we decided to honour a team's choice to set a contact address to send
all email to a black hole. Bug mail is no longer an issue. When a member
choose the contact-this-team form, Launchpad will send email to all members
so that important messages can be sent. contact-this-team is still not
substitute for mailing lists because we limit its use to 3 time a day.

While discussing the rules recently, we discovered that we did not update
contact-this-team to contact team admins instead of the owner. We allow
the owner to leave the team, delegating all responsibility to the admins.

--------------------------------------------------------------------

RULES

    * Delete the rules for when the team has a contact address, allowing
      the team member rules to be the only rule in play.
    * Change the contact owner rule to contact admins.
    * ADDENDUM: I cannot 'hit' the cancel link. I do not think the cancel
      is a verb and suffices.

QA

    * Visit https://qastaging.launchpad.net/~registry
    * Set the contact address to the mailing list.
    * Choose contact this team's members.
    * Verify the page says
      You are contacting nn members of the Registry Administrators (registry)
      team directly.
    * Verify the page says
       If you do not want Registry Administrators to know your email address,
       _cancel_ now.

    * Visit https://qastaging.launchpad.net/~bzr (or a
      team you are not a member of)
    * Choose contact this team's admins.
    * Verify the page says
      You are contacting the Bazaar Developers (bzr) team admins.

LINT

    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/person-views.txt
    lib/lp/registry/browser/tests/team-views.txt
    lib/lp/registry/browser/tests/user-to-user-views.txt
    lib/lp/registry/stories/team/xx-team-home.txt
    lib/lp/registry/templates/contact-user.pt

TEST

    ./bin/test -vcc -t user-to-user-views -t person-views -t team-views \
        -t xx-team-home lp.registry

IMPLEMENTATION

Removed the TO_TEAM state of ContactViaWebNotificationRecipientSet, deleted
all callsites that accessed it, deleted tests that check that the team
email address was contacted.

Renamed TO_OWNER to TO_ADMINS and revised the descriptive test. Updated
tests to verify team admins are contacted. Replaced
_getPrimaryRecipient() with a block to get the team admins (make the
tests pass). While there was no method that returned person and email
address, my implementation uses the get_recipients helper that caches
the user/team email addresses.

[Brad Crittenden (bac)]

This code looks good Curtis and is a welcome improvement.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-02-03 22:03:43 +0000
+++ lib/lp/registry/browser/person.py	2012-02-10 21:51:21 +0000
@@ -217,6 +217,7 @@
     Milestone,
     milestone_sort_key,
     )
+from lp.registry.model.person import get_recipients
 from lp.services.config import config
 from lp.services.database.sqlbase import flush_database_updates
 from lp.services.feeds.browser import FeedsMixin
@@ -2581,8 +2582,7 @@
         :return: the recipients of the message.
         :rtype: `ContactViaWebNotificationRecipientSet` constant:
                 TO_USER
-                TO_TEAM (Send to team's preferredemail)
-                TO_OWNER
+                TO_ADMINS
                 TO_MEMBERS
         """
         return ContactViaWebNotificationRecipientSet(
@@ -2597,13 +2597,10 @@
                 return 'Send an email to yourself through Launchpad'
             else:
                 return 'Send an email to this user through Launchpad'
-        elif self.group_to_contact == ContactViaWeb.TO_TEAM:
-            return ("Send an email to your team's contact email address "
-                    "through Launchpad")
         elif self.group_to_contact == ContactViaWeb.TO_MEMBERS:
             return "Send an email to your team's members through Launchpad"
-        elif self.group_to_contact == ContactViaWeb.TO_OWNER:
-            return "Send an email to this team's owner through Launchpad"
+        elif self.group_to_contact == ContactViaWeb.TO_ADMINS:
+            return "Send an email to this team's admins through Launchpad"
         else:
             raise AssertionError('Unknown group to contact.')
 
@@ -2615,12 +2612,10 @@
             # Note that we explicitly do not change the text to "Contact
             # yourself" when viewing your own page.
             return 'Contact this user'
-        elif self.group_to_contact == ContactViaWeb.TO_TEAM:
-            return "Contact this team's email address"
         elif self.group_to_contact == ContactViaWeb.TO_MEMBERS:
             return "Contact this team's members"
-        elif self.group_to_contact == ContactViaWeb.TO_OWNER:
-            return "Contact this team's owner"
+        elif self.group_to_contact == ContactViaWeb.TO_ADMINS:
+            return "Contact this team's admins"
         else:
             raise AssertionError('Unknown group to contact.')
 
@@ -4823,9 +4818,8 @@
 
     # Primary reason enumerations.
     TO_USER = object()
-    TO_TEAM = object()
     TO_MEMBERS = object()
-    TO_OWNER = object()
+    TO_ADMINS = object()
 
     def __init__(self, user, person_or_team):
         """Initialize the state based on the context and the user.
@@ -4861,36 +4855,16 @@
         """
         if person_or_team.is_team:
             if self.user.inTeam(person_or_team):
-                if removeSecurityProxy(person_or_team).preferredemail is None:
-                    # Send to each team member.
-                    return self.TO_MEMBERS
-                else:
-                    # Send to the team's contact address.
-                    return self.TO_TEAM
+                return self.TO_MEMBERS
             else:
                 # A non-member can only send emails to a single person to
                 # hinder spam and to prevent leaking membership
                 # information for private teams when the members reply.
-                return self.TO_OWNER
+                return self.TO_ADMINS
         else:
             # Send to the user
             return self.TO_USER
 
-    def _getPrimaryRecipient(self, person_or_team):
-        """Return the primary recipient.
-
-        The primary recipient is the ``person_or_team`` in all cases
-        except for when the email is restricted to a team owner.
-
-        :param person_or_team: The party that is the context of the email.
-        :type person_or_team: `IPerson`.
-        """
-        if self._primary_reason is self.TO_OWNER:
-            person_or_team = person_or_team.teamowner
-            while person_or_team.is_team:
-                person_or_team = person_or_team.teamowner
-        return person_or_team
-
     def _getReasonAndHeader(self, person_or_team):
         """Return the reason and header why the email was received.
 
@@ -4902,20 +4876,13 @@
                 'using the "Contact this user" link on your profile page\n'
                 '(%s)' % canonical_url(person_or_team))
             header = 'ContactViaWeb user'
-        elif self._primary_reason is self.TO_OWNER:
+        elif self._primary_reason is self.TO_ADMINS:
             reason = (
-                'using the "Contact this team\'s owner" link on the '
+                'using the "Contact this team\'s admins" link on the '
                 '%s team page\n(%s)' % (
                     person_or_team.displayname,
                     canonical_url(person_or_team)))
             header = 'ContactViaWeb owner (%s team)' % person_or_team.name
-        elif self._primary_reason is self.TO_TEAM:
-            reason = (
-                'using the "Contact this team" link on the '
-                '%s team page\n(%s)' % (
-                    person_or_team.displayname,
-                    canonical_url(person_or_team)))
-            header = 'ContactViaWeb member (%s team)' % person_or_team.name
         else:
             # self._primary_reason is self.TO_MEMBERS.
             reason = (
@@ -4937,15 +4904,9 @@
             return (
                 'You are contacting %s (%s).' %
                 (person_or_team.displayname, person_or_team.name))
-        elif self._primary_reason is self.TO_OWNER:
-            return (
-                'You are contacting the %s (%s) team owner, %s (%s).' %
-                (person_or_team.displayname, person_or_team.name,
-                 self._primary_recipient.displayname,
-                 self._primary_recipient.name))
-        elif self._primary_reason is self.TO_TEAM:
-            return (
-                'You are contacting the %s (%s) team.' %
+        elif self._primary_reason is self.TO_ADMINS:
+            return (
+                'You are contacting the %s (%s) team admins.' %
                 (person_or_team.displayname, person_or_team.name))
         else:
             # This is a team without a contact address (self.TO_MEMBERS).
@@ -4968,6 +4929,16 @@
             for recipient in team.getMembersWithPreferredEmails():
                 email = removeSecurityProxy(recipient).preferredemail.email
                 all_recipients[email] = recipient
+        elif self._primary_reason is self.TO_ADMINS:
+            team = self._primary_recipient
+            for admin in team.adminmembers:
+                # This method is similar to getTeamAdminsEmailAddresses, but
+                # this case needs to know the user. Since both methods
+                # ultimately iterate over get_recipients, this case is not
+                # in a different performance class.
+                for recipient in get_recipients(admin):
+                    email = removeSecurityProxy(recipient).preferredemail.email
+                    all_recipients[email] = recipient
         elif self._primary_recipient.is_valid_person_or_team:
             email = removeSecurityProxy(
                 self._primary_recipient).preferredemail.email
@@ -5040,7 +5011,7 @@
         """
         self._reset_state()
         self._primary_reason = self._getPrimaryReason(person)
-        self._primary_recipient = self._getPrimaryRecipient(person)
+        self._primary_recipient = person
         if reason is None:
             reason, header = self._getReasonAndHeader(person)
         self._reason = reason

=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
--- lib/lp/registry/browser/tests/person-views.txt	2012-01-15 13:32:27 +0000
+++ lib/lp/registry/browser/tests/person-views.txt	2012-02-10 21:51:21 +0000
@@ -525,14 +525,6 @@
 Non-member contacting a Team
 ----------------------------
 
-Users can contact teams, but the behaviour depends upon whether the user
-is a member of the team. No Privileges Person is not a member of the
-Landscape Developers team.
-
-    >>> view = create_initialized_view(landscape_developers, '+index')
-    >>> print view.contact_link_title
-    Send an email to this team's owner through Launchpad
-
 The EmailToPersonView can be used by non-members to contact the team
 owner.
 
@@ -545,7 +537,7 @@
 
     >>> print view.recipients.description
     You are contacting the Landscape Developers (landscape-developers) team
-    owner, Sample Person (name12).
+    admins.
 
     >>> [recipient.name for recipient in view.recipients]
     [u'name12']
@@ -608,19 +600,6 @@
     >>> [recipient.name for recipient in view.recipients]
     [u'name12']
 
-EmailToPersonView will use the contact address when the team has one.
-
-    >>> email_address = factory.makeEmail(
-    ...     '[e-mail]', landscape_developers)
-    >>> landscape_developers.setContactAddress(email_address)
-
-    >>> view = create_initialized_view(landscape_developers, '+contactuser')
-    >>> print view.recipients.description
-    You are contacting the Landscape Developers (landscape-developers) team.
-
-    >>> [recipient.name for recipient in view.recipients]
-    [u'landscape-developers']
-
 
 Contact this user/team valid addresses and quotas
 -------------------------------------------------

=== modified file 'lib/lp/registry/browser/tests/team-views.txt'
--- lib/lp/registry/browser/tests/team-views.txt	2012-01-09 12:39:11 +0000
+++ lib/lp/registry/browser/tests/team-views.txt	2012-02-10 21:51:21 +0000
@@ -213,9 +213,9 @@
 
     >>> view = create_initialized_view(guadamen, '+index')
     >>> print view.contact_link_title
-    Send an email to this team's owner through Launchpad
+    Send an email to this team's admins through Launchpad
     >>> print view.specific_contact_text
-    Contact this team's owner
+    Contact this team's admins
 
 
 Mugshots

=== modified file 'lib/lp/registry/browser/tests/user-to-user-views.txt'
--- lib/lp/registry/browser/tests/user-to-user-views.txt	2011-12-29 05:29:36 +0000
+++ lib/lp/registry/browser/tests/user-to-user-views.txt	2012-02-10 21:51:21 +0000
@@ -305,13 +305,14 @@
     --
     This message was sent from Launchpad by
     Bart (http://launchpad.dev/~bart)
-    using the "Contact this team's owner" link on the GuadaMen team page
+    using the "Contact this team's admins" link on the GuadaMen team page
     (http://launchpad.dev/~guadamen).
     For more information see
     https://help.launchpad.net/YourAccount/ContactingPeople
-    # of Messages: 1
+    # of Messages: 2
     Recipients:
         Foo Bar <[e-mail]>
+        Ubuntu Team <[e-mail]>
 
 
 Member to team
@@ -363,110 +364,7 @@
         Steve Alexander <[e-mail]>
         Ubuntu Team <[e-mail]>
 
-The Guadamen team creates a mailing list but does not set it to be the
-contact address. The mailing list will not be used.
-
-    >>> team, mailing_list = factory.makeTeamAndMailingList(
-    ...     guadamen.name, guadamen.teamowner.name)
-
-    # Ignore the 'new mailing list message'
-
-    >>> transaction.commit()
-    >>> del stub.test_emails[:]
-
-Foo Bar now contacts them again, which he can do because his quota is
-still not met. This message includes a "%s" combination; it is not a
-interpolation instruction.
-
-    >>> view = create_view(
-    ...     foo_bar, guadamen, {
-    ...         'field.field.from_': '[e-mail]',
-    ...         'field.subject': 'My last question for Guadamen',
-    ...         'field.message': 'Can one of you help me with "%s" usage!',
-    ...         'field.actions.send': 'Send',
-    ...         })
-    >>> print_notifications(view)
-    Message sent to GuadaMen
-
-Foo Bar's message gets sent to each individual member of he team.
-
-    >>> transaction.commit()
-    >>> print_messages()
-    Senders: set(['Foo Bar <[e-mail]>'])
-    Subjects: set(['My last question for Guadamen'])
-    Bodies:
-    Can one of you help me with "%s" usage!
-    --
-    This message was sent from Launchpad by
-    Foo Bar (http://launchpad.dev/~name16)
-    to each member of the GuadaMen team
-    using the "Contact this team" link on the GuadaMen
-    team page (http://launchpad.dev/~guadamen).
-    For more information see
-    https://help.launchpad.net/YourAccount/ContactingPeople
-    # of Messages: 10
-    Recipients:
-        Alexander Limi <[e-mail]>
-        Celso Providelo <[e-mail]>
-        Colin Watson <[e-mail]>
-        Daniel Silverstone <[e-mail]>
-        Edgar Bursic <[e-mail]>
-        Foo Bar <[e-mail]>
-        Jeff Waugh <[e-mail]>
-        Mark Shuttleworth <[e-mail]>
-        Steve Alexander <[e-mail]>
-        Ubuntu Team <[e-mail]>
-
-The Guadamen team now registers an external contact address (they could
-have also used their Launchpad mailing list address).
-
-    >>> from lp.services.identity.interfaces.emailaddress import (
-    ...     IEmailAddressSet)
-    >>> email_set = getUtility(IEmailAddressSet)
-    >>> address = email_set.new('[e-mail]', guadamen)
-    >>> guadamen.setContactAddress(address)
-
-Foo Bar contacts the Guadamen team again, which is allowed because his
-quota was not met by his first message.  This time only one message is
-sent, and that to the new contact address.
-
-    >>> view = create_view(
-    ...     foo_bar, guadamen, {
-    ...         'field.field.from_': '[e-mail]',
-    ...         'field.subject': 'Hello again Guadamen',
-    ...         'field.message': 'Can one of you help me?',
-    ...         'field.actions.send': 'Send',
-    ...         })
-    >>> print_notifications(view)
-    Message sent to GuadaMen
-
-    >>> transaction.commit()
-    >>> len(stub.test_emails)
-    1
-
-    >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-    >>> print from_addr, to_addrs
-    [e-mail] [u'GuadaMen <[e-mail]>']
-
-    >>> print raw_msg
-    Content-Type: text/plain; charset="us-ascii"
-    ...
-    From: Foo Bar <[e-mail]>
-    To: GuadaMen <[e-mail]>
-    Subject: Hello again Guadamen
-    ...
-    X-Launchpad-Message-Rationale: ContactViaWeb member (guadamen team)
-    ...
-    <BLANKLINE>
-    Can one of you help me?
-    --
-    This message was sent from Launchpad by
-    Foo Bar (http://launchpad.dev/~name16)
-    using the "Contact this team" link on the GuadaMen team page
-    (http://launchpad.dev/~guadamen).
-    For more information see
-    https://help.launchpad.net/YourAccount/ContactingPeople
-
+    >>> transaction.commit()
 
 Message quota
 -------------
@@ -477,6 +375,22 @@
 emails again. The is_possible property will be False if is_allowed is
 False.
 
+    >>> view = create_view(
+    ...     foo_bar, guadamen, {
+    ...         'field.field.from_': '[e-mail]',
+    ...         'field.subject': 'Hello Guadamen',
+    ...         'field.message': 'Can one of you help me?',
+    ...         'field.actions.send': 'Send',
+    ...         })
+    >>> view.contact_is_allowed
+    True
+
+    >>> view.contact_is_possible
+    True
+
+    >>> view.initialize()
+    >>> transaction.commit()
+
 Foo Bar has now reached his quota and can send no more contact messages
 today.
 
@@ -500,16 +414,6 @@
     Your message was not sent because you have exceeded your daily quota of
     3 messages to contact users. Try again in ...
 
-Bart has sent 1 message, he may send more messages.
-
-    >>> login_person(bart)
-    >>> view = create_view(bart, guadamen)
-    >>> view.contact_is_allowed
-    True
-
-    >>> view.contact_is_possible
-    True
-
 
 Identifying information
 -----------------------

=== modified file 'lib/lp/registry/stories/team/xx-team-home.txt'
--- lib/lp/registry/stories/team/xx-team-home.txt	2011-01-04 16:08:57 +0000
+++ lib/lp/registry/stories/team/xx-team-home.txt	2012-02-10 21:51:21 +0000
@@ -1,4 +1,5 @@
-= A team's home page =
+A team's home page
+==================
 
 The home page of a public team is visible to everyone.
 
@@ -133,18 +134,6 @@
     ...     find_tag_by_id(browser.contents, 'your-involvement'))
     You are a member of this team...
 
-He can also see the contact email address and a link to contact the team.
-
-    >>> browser.open('http://launchpad.dev/~ubuntu-team')
-    >>> print extract_text(
-    ...     find_tag_by_id(browser.contents, 'contact-email'))
-    Email:
-    [e-mail]
-    Set contact address
-    >>> print extract_text(
-    ...     find_tag_by_id(browser.contents, 'contact-user'))
-    Contact this team's email address
-
 Member can contact their team even if the team does not have a contact
 address:
 
@@ -190,7 +179,8 @@
     ...
 
 
-== Team admins ==
+Team admins
+-----------
 
 Team owners and admins can see a link to approve and decline applicants.
 
@@ -207,7 +197,8 @@
     <Link text='Approve or decline members' url='.../+editproposedmembers'>
 
 
-== Non members ==
+Non members
+-----------
 
 No Privileges Person is not a member of the Ubuntu team.
 
@@ -220,13 +211,14 @@
 He can see the contact address, and the link explains the email
 will actually go to the team's administrators.
 
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'contact-email'))
+    >>> print extract_text(find_tag_by_id(
+    ...     user_browser.contents, 'contact-email'))
     Email:
     [e-mail]
     >>> content = find_tag_by_id(user_browser.contents, 'contact-user')
     >>> print extract_text(content)
-    Contact this team's owner
+    Contact this team's admins
 
     >>> content.a
     <a href="+contactuser"...
-      title="Send an email to this team's owner through Launchpad">...
+      title="Send an email to this team's admins through Launchpad">...

=== modified file 'lib/lp/registry/templates/contact-user.pt'
--- lib/lp/registry/templates/contact-user.pt	2011-10-13 05:42:45 +0000
+++ lib/lp/registry/templates/contact-user.pt	2012-02-10 21:51:21 +0000
@@ -17,7 +17,7 @@
             specified below.  If you do not want
             <tal:person
              tal:content="context/displayname">Anne Person</tal:person>
-            to know your email address, hit
+            to know your email address,
             <a href="" tal:attributes="href view/cancel_url">Cancel</a> now.
             </p>
           </div>