Code review comment for lp:~sinzui/launchpad/hold-message

On Wed, 2010-10-06 at 17:23 +0000, Edwin Grubbs wrote:
> Review: Approve code
> Hi Curtis,
>
> This is a nice improvement. I have some comments below.
>
> -Edwin
>
>
> >=== mo>+# GNU Affero General Public License version 3 (see the file LICENSE).
> >+"""Test the lpsize monekypatches"""
> >+
> >+from __future__ import with_statement
> >+
> >+__metaclass__ = type
> >+__all__ = []
> >+
> >+from email.mime.application import MIMEApplication
> >+
> >+from Mailman import Errors
> >+from Mailman.Handlers import LPSize
> >+
> >+from canonical.config import config
> >+from canonical.testing import (
> >+ DatabaseFunctionalLayer,
> >+ LaunchpadFunctionalLayer,
> >+ )
> >+from lp.services.mailman.testing import MailmanTestCase
> >+
> >+
> dified file 'lib/lp/services/mailman/monkeypatches/lpsize.py'
> >--- lib/lp/services/mailman/monkeypatches/lpsize.py 2010-08-20 20:31:18 +0000
> >+++ lib/lp/services/mailman/monkeypatches/lpsize.py 2010-10-06 15:36:24 +0000
> >...
> >+def truncated_message(original_message, limit=10000):
> >+ """Create a smaller version of the message for moderation."""
> >+ message = email.message_from_string(
> >+ original_message.as_string(), Message.Message)
> >+ for part in email.iterators.typed_subpart_iterator(message, 'multipart'):
> >+ subparts = part.get_payload()
> >+ removeable = []
> >+ for subpart in subparts:
> >+ if subpart.get_content_maintype() == 'multipart':
> >+ # This part is handled in the outer loop.
> >+ continue
> >+ elif subpart.get_content_type() == 'text/plain':
>
>
>
> Shouldn't it truncate text/html or any other "text/" content-type,
> since the user's mail client might create an empty text/plain section?

Launchpad does not support text/html messages. Messages without
text/plain parts are discarded in the hold process. There is an old bug
about empty messages in the moderation queue. It was discovered that all
cases are from spammers. We did not see a need to support text/html
since list subscribers are not using it and getting messages in the
moderation queue.

> >+ # Truncate the message at 10kb so that there is enough
> >+ # information for the moderator to make a decision.
> >+ content = subpart.get_payload().strip()
> >+ if len(content) > limit:
> >+ subpart.set_payload(
> >+ content[:limit] + ' [truncated for moderation]',
>
>
>
> It seems like [truncated for moderation] will be easier to see if
> it is on a new line.

Agreed

> >=== added file 'lib/lp/services/mailman/tests/test_lpsize.py'
> >--- lib/lp/services/mailman/tests/test_lpsize.py 1970-01-01 00:00:00 +0000
> >+++ lib/lp/services/mailman/tests/test_lpsize.py 2010-10-06 15:36:24 +0000
> >@@ -0,0 +1,129 @@
> >+# Copyright 20010 Canonical Ltd. This software is licensed under the
>
>
>
> I think the copyright will expire by the time Bender and Leela can use
> it.

:)

...

> >+class TestLPSizeTestCase(MailmanTestCase):
> >+ """Test LPSize.
> >+
> >+ Mailman process() methods quietly return. They may set msg_data key-values
> >+ or raise an error to end processing. This group of tests tests often check
>
>
>
> s/tests tests/tests/
> Either, use "These tests" instead of "This group of tests" or change
> "check" to "checks".

These tests often check

> >+ for errors, but that does not mean there is an error condition, it only
> >+ means message processing has reached a final decision. Messages that do
> >+ not cause a final decision pass-through and the process() methods ends
>
>
> "pass-through" with a hyphen looks like a noun instead of a verb.
> Add a comma before the "and".

I will update test_lpmoderate too.

>
> >+ without a return.
> >+ """
> >+
> >+ layer = LaunchpadFunctionalLayer
> >+
> >+ def setUp(self):
> >+ super(TestLPSizeTestCase, self).setUp()
> >+ self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
> >+ 'team-1', 'team-1-owner')
> >+ self.mm_list = self.makeMailmanList(self.mailing_list)
> >+ self.subscriber_email = self.team.teamowner.preferredemail.email
> >+
> >+ def tearDown(self):
> >+ super(TestLPSizeTestCase, self).tearDown()
> >+ self.cleanMailmanList(self.mm_list)
> >+
> >+ def test_process_size_under_soft_limit(self):
> >+ # Any message under 40kb is sent to the list.
> >+ attachment = MIMEApplication(
> >+ '\n'.join(['x' * 20] * 1000), 'octet-stream')
> >+ message = self.makeMailmanMessage(
> >+ self.mm_list, self.subscriber_email, 'subject', 'content',
> >+ attachment=attachment)
> >+ msg_data = {}
> >+ silence = LPSize.process(self.mm_list, message, msg_data)
> >+ self.assertEqual(None, silence)
> >+
> >+ def test_process_size_over_soft_limit_held(self):
> >+ # Messages over 40km held for moderation.
>
>
>
> Uh, based on how many millimeters per character?

/me checks his point/pica ruler (72pt/inch)
2,868,525 characters per km. maybe I should switch to kb.

--
__Curtis C. Hovey_________
http://launchpad.net/