launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/mailman-email-addresses into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #924606 in Launchpad itself: "mailman xmlrpc is generating '-- MARK --' alerts"
  https://bugs.launchpad.net/launchpad/+bug/924606

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/mailman-email-addresses/+merge/124273

This is triggering nagios alerts, as the current MARK delta alert is set
to 40 minutes. Mailman is configured to write write MARKs to the log every
5 minutes.

Mailman's xmlrpc runner log shows it spends more than 99% of its time
acquiring locks to mailing lists to write email address cases changes to
disk. The email address are lower-cased though, so the change is no-op.

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

RULES

    Pre-implementation: no one
    * Lowercase the email addresses that are sent to mailman.


QA

    * Review the staging mailman/xmlrc log or run
      fgrep MARK /path/to/mailman/logs/xmlrpc | tail -100
    * Verify the log does not show mix-case changes.
    * After release and after logs mailman logs are sync to carob
      review the tail of /srv/launchpad.net-logs/mailman/polevik/xmlrpc
    * verify the log does not show wasted work saving mixed-case email
      addresses.
    * Is MARK appearing closer to every 5 minutes?


LINT

    lib/lp/registry/doc/message-holds.txt
    lib/lp/registry/model/mailinglist.py
    lib/lp/registry/tests/test_mailinglist.py


TEST

    ./bin/test -vv -t  TestMailinglistSet lp.registry.tests.test_mailinglist


LoC

    This branch adds two tests, but removes "Posting privileges" section
    from a doctest because the behaviour is test elsewhere in the doctest
    and explicitly in:
    TestMailinglistSetMessages.test_getSenderAddresses_approved_dict_values


IMPLEMENTATION

Lowercase the email address return by IMailingListSet getSenderAddresses()
and getSubscribedAddresses().
    lib/lp/registry/model/mailinglist.py
    lib/lp/registry/tests/test_mailinglist.py

Removed test that shows approving a held message add the use to
getSubscribedAddresses().
    lib/lp/registry/doc/message-holds.txt
-- 
https://code.launchpad.net/~sinzui/launchpad/mailman-email-addresses/+merge/124273
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/mailman-email-addresses into lp:launchpad.

=== modified file 'lib/lp/registry/doc/message-holds.txt'
--- lib/lp/registry/doc/message-holds.txt	2012-01-04 06:52:11 +0000
+++ lib/lp/registry/doc/message-holds.txt	2012-09-13 18:04:22 +0000
@@ -372,90 +372,6 @@
     The Launchpad team
     ----------------------------------------
 
-
-== Posting privileges ==
-Message approvals are also used to derived posting privileges for non-team
-members.  To start with, only the team owner can post to the list.
-
-    >>> login('no-priv@xxxxxxxxxxxxx')
-    >>> team_three, list_three = mailinglists_helper.new_team(
-    ...     'test-three', True)
-    >>> sorted(list_three.getSenderAddresses())
-    [u'no-priv@xxxxxxxxxxxxx']
-
-Salgado posts a message to the list, but because he is not a team member, it
-gets held for approval.
-
-    >>> message = message_set.fromEmail("""\
-    ... From: guilherme.salgado@xxxxxxxxxxxxx
-    ... To: test-one@xxxxxxxxxxxxxxxxxxx
-    ... Subject: My first test
-    ... Message-ID: <zebra>
-    ... Date: Fri, 01 Aug 2000 01:08:59 -0000
-    ...
-    ... This is my first post!
-    ... """)
-    >>> held_message = list_three.holdMessage(message)
-    >>> transaction.commit()
-
-If the message gets discarded (or rejected), Salgado is still not allowed to
-post to the mailing list.
-
-    >>> held_message.discard(owner)
-    >>> held_message.acknowledge()
-    >>> transaction.commit()
-
-    >>> sorted(list_three.getSenderAddresses())
-    [u'no-priv@xxxxxxxxxxxxx']
-
-    >>> from lp.registry.interfaces.mailinglist import IMailingListSet
-    >>> mailinglist_set = getUtility(IMailingListSet)
-    >>> bulk_addresses = mailinglist_set.getSenderAddresses(['test-three'])
-    >>> sorted(email for name, email in bulk_addresses['test-three'])
-    [u'no-priv@xxxxxxxxxxxxx']
-
-Salgado posts another message to the list, but this time it's approved.
-
-    >>> message = message_set.fromEmail("""\
-    ... From: guilherme.salgado@xxxxxxxxxxxxx
-    ... To: test-one@xxxxxxxxxxxxxxxxxxx
-    ... Subject: My first test
-    ... Message-ID: <yeti>
-    ... Date: Fri, 01 Aug 2000 01:08:59 -0000
-    ...
-    ... This is my second post!
-    ... """)
-    >>> held_message = list_three.holdMessage(message)
-    >>> transaction.commit()
-
-    >>> held_message.approve(owner)
-    >>> held_message.acknowledge()
-    >>> transaction.commit()
-
-Now, Salgado is on the list of approved posters.
-
-    >>> sorted(list_three.getSenderAddresses())
-    [u'guilherme.salgado@xxxxxxxxxxxxx', u'no-priv@xxxxxxxxxxxxx']
-
-    >>> bulk_addresses = mailinglist_set.getSenderAddresses(['test-three'])
-    >>> sorted(email for name, email in bulk_addresses['test-three'])
-    [u'guilherme.salgado@xxxxxxxxxxxxx', u'no-priv@xxxxxxxxxxxxx']
-
-But he will still not get messages delivered to his address, since he's not
-subscribed to the team mailing list.
-
-    >>> sorted(list_three.getSubscribedAddresses())
-    []
-
-Of course, Salgado still cannot post to a mailing list that he has not been
-approved for, because he has not had three approved moderations.
-
-    >>> team_four, list_four = mailinglists_helper.new_team(
-    ...     'test-four', True)
-    >>> sorted(list_four.getSenderAddresses())
-    [u'no-priv@xxxxxxxxxxxxx']
-
-
 == Duplicates ==
 
 When a person responds to a bug email, the message can end up in the database

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2012-01-04 22:30:45 +0000
+++ lib/lp/registry/model/mailinglist.py	2012-09-13 18:04:22 +0000
@@ -616,7 +616,7 @@
         for email, display_name, team_name in preferred:
             assert team_name in team_names, (
                 'Unexpected team name in results: %s' % team_name)
-            value = (display_name, email)
+            value = (display_name, email.lower())
             by_team[team_name].add(value)
         explicit = store.using(*tables).find(
             (EmailAddress.email, Person.displayname, Team.name),
@@ -628,7 +628,7 @@
         for email, display_name, team_name in explicit:
             assert team_name in team_names, (
                 'Unexpected team name in results: %s' % team_name)
-            value = (display_name, email)
+            value = (display_name, email.lower())
             by_team[team_name].add(value)
         # Turn the results into a mapping of lists.
         results = {}
@@ -688,7 +688,7 @@
         for team_name, person_displayname, email in all_posters:
             assert team_name in team_names, (
                 'Unexpected team name in results: %s' % team_name)
-            value = (person_displayname, email)
+            value = (person_displayname, email.lower())
             by_team[team_name].add(value)
         # Turn the results into a mapping of lists.
         results = {}

=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py	2012-08-13 19:51:16 +0000
+++ lib/lp/registry/tests/test_mailinglist.py	2012-09-13 18:04:22 +0000
@@ -108,6 +108,20 @@
             for m in team1.allmembers])
         self.assertEqual(list_senders, sorted(result[team1.name]))
 
+    def test_getSenderAddresses_multiple_and_lowercase_email(self):
+        # getSenderAddresses() contains multiple email addresses for
+        # users and they are lowercased for mailman.
+        # {team_name: [(member_displayname, member_email) ...]}
+        team1, member1 = self.factory.makeTeamWithMailingListSubscribers(
+            'team1', auto_subscribe=False)
+        email = self.factory.makeEmail('me@xxxxxx', member1)
+        result = self.mailing_list_set.getSenderAddresses([team1.name])
+        list_senders = sorted([
+            (m.displayname, m.preferredemail.email)
+            for m in team1.allmembers])
+        list_senders.append((member1.displayname, email.email.lower()))
+        self.assertContentEqual(list_senders, result[team1.name])
+
     def test_getSenderAddresses_participation_dict_values(self):
         # getSenderAddresses() dict values includes indirect participants.
         team1, member1 = self.factory.makeTeamWithMailingListSubscribers(
@@ -140,6 +154,25 @@
             (member1.displayname, member1.preferredemail.email)]
         self.assertEqual(list_subscribers, result[team1.name])
 
+    def test_getSubscribedAddresses_multiple_lowercase_email(self):
+        # getSubscribedAddresses() contains email addresses for
+        # users and they are lowercased for mailman. The email maybe
+        # explicitly set instead of the preferred email.
+        # {team_name: [(member_displayname, member_email) ...]}
+        team1, member1 = self.factory.makeTeamWithMailingListSubscribers(
+            'team1')
+        with person_logged_in(member1):
+            email1 = self.factory.makeEmail('me@xxxxxx', member1)
+            member1.setPreferredEmail(email1)
+        with person_logged_in(team1.teamowner):
+            email2 = self.factory.makeEmail('you@xxxxxx', team1.teamowner)
+            team1.mailing_list.subscribe(team1.teamowner, email2)
+        result = self.mailing_list_set.getSubscribedAddresses([team1.name])
+        list_subscribers = [
+            (member1.displayname, email1.email.lower()),
+            (team1.teamowner.displayname, email2.email.lower())]
+        self.assertContentEqual(list_subscribers, result[team1.name])
+
     def test_getSubscribedAddresses_participation_dict_values(self):
         # getSubscribedAddresses() dict values includes indirect participants.
         team1, member1 = self.factory.makeTeamWithMailingListSubscribers(