Merge lp:~pcjc2/launchpad/allow-empty-comments into lp:launchpad

[Peter Clifton]

[r=gmb][ui=none][bug=692951] Avoid insersion of an "<empty comment>" placeholder for imported bug comments which contain an attachment.

Without this change, it is not possible for the import to give the same clean, uncluttered blank comments you see when adding files directly from within the launchpad web-interface.

[Graham Binns (gmb)]

Hi Peter,

Thanks for this branch. I'm marking it "Needs Fixing" because your
change is untested. We're very strict about adding tests for new
features in Launchpad, and there needs to be a unit test for this new
code before it can land.

The new test should go in lib/lp/bugs/scripts/tests/test_bugimport.py,
along with the other bug import tests. If you need any help writing the
test feel free to ping me (gmb) or Gavin (allenap) in #launchpad-dev on
Freenode.

Besides the test issue I've got one other comment:

37 self.parser.add_option('--dont-verify-users', dest='verify_users',
38 help="Don't verify newly created users",
39 action='store_false', default=True)
40 + self.parser.add_option('--allow-empty-comments',
41 + dest='allow_empty_comments',
42 + help="Allow empty bug comments",
43 + action='store_true', default=False)

It would be really great if you could format this, and the lines above
it (which are formatted incorrectly) according to our style guide, which
is thus:

        self.parser.add_option(
            '--allow-empty-comments', dest='allow_empty_comments',
            help="Allow empty bug comments", action='store_true',
            default=False)

review: Needs Fixing (code)

[Peter Clifton (pcjc2)]

Thanks for the review, I'll get onto that when I get a moment.

Is there any chance of getting it merged soon when that is fixed?

I've got our SourceForge bug data for two upstream projects I'm working on (and hoping to move to Launchpad), but they will not import properly without this patch. Is there a particular development / release cycle which will need to be synced up with before we can (for example), use this modification to do an import to Staging for testing?

[Peter Clifton (pcjc2)]

I have updated branch with the following changes:

1. Reformated option arguments as per previous code review
2. Removed --allow-empty-comments option previously added in the branch
3. Instead, added a test for attachments when generating the comment message.
   This then drops the place-holder "<empty comment>" for those with attachments.
4. Extended the bugimport unit test to check the new behaviour.

review: Needs Resubmitting

[Peter Clifton (pcjc2)]

review: Abstain

[Graham Binns (gmb)]

Hi Peter,

I'm really pleased with this branch - nice work! I've got a couple of stylistic nitpicks but it's nothing major. Once those are fixed I'll push it into our merge machinery for you.

9 + # If there is no attachment and no comment text, use a place-holder
10 + no_attachments = get_element (commentnode, 'attachment') == None

It took me a couple of seconds to parse this. I think it would be better as part of the conditional that follows it, thus:

    # If there is no attachment and no comment text, use a place-holder.
    if (get_element(commentnode, 'attachment') is None and
        (text is None or text == '')):
        ...

66 + self.assertEqual(message4.owner.preferredemail.email,
67 + '<email address hidden>')
68 + self.assertEqual(message4.datecreated.isoformat(),
69 + '2005-01-01T14:00:00+00:00')
70 + self.assertEqual(message4.subject, 'Re: A test bug')
71 + self.assertEqual(message4.text_contents, '<empty comment>')
72 + self.assertEqual(message4.bugattachments.count(), 0)
73 +
74 + # Message 5:
75 + self.assertEqual(message5.owner.preferredemail.email,
76 + '<email address hidden>')
77 + self.assertEqual(message5.datecreated.isoformat(),
78 + '2005-01-01T15:00:00+00:00')

I know that all the other asserts in the test are formatted like this, and I don't expect you to correct those too (I'll file a bug about cleaning it up), but these should be formatted as I said in my other review:

    self.assertEqual(
        message5.datecreated.isoformat(), ...)

And so on.

review: Needs Fixing (code)

[Graham Binns (gmb)]

review: Approve (code)

[Jelmer Vernooij (jelmer)]

Hi Graham, are you landing this branch ?

[Graham Binns (gmb)]

On Monday, 20 December 2010, Jelmer Vernooij <email address hidden> wrote:
> Hi Graham, are you landing this branch ?

Yes. I was waiting for confirmation of the contributors' agreement
dance having been concluded. I'll land this tomorrow.

--
Graham Binns
http://grahambinns.com

[Graham Binns (gmb)]

Hi Peter,

I've passed this on to Gavin Panella (allenap) and Deryck Hodge (deryck) to handle, as I'm taking the day off sick. Hopefully we can find out what's happened with your Contributor Agreement paperwork and get this landed ASAP.

Cheers,

Graham

=== modified file 'lib/lp/bugs/scripts/bugimport.py'
--- lib/lp/bugs/scripts/bugimport.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/scripts/bugimport.py	2010-12-16 17:06:41 +0000
@@ -408,7 +408,9 @@
         if date is None:
             raise BugXMLSyntaxError('No date for comment %r' % title)
         text = get_value(commentnode, 'text')
-        if text is None or text == '':
+        # If there is no comment text and no attachment, use a place-holder
+        if ((text is None or text == '') and
+            get_element(commentnode, 'attachment') is None):
             text = '<empty comment>'
         return getUtility(IMessageSet).fromText(title, text, sender, date)
 

=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
--- lib/lp/bugs/scripts/tests/test_bugimport.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/scripts/tests/test_bugimport.py	2010-12-16 17:06:41 +0000
@@ -427,6 +427,25 @@
       <contents>QSBwYXRjaA==</contents>
     </attachment>
   </comment>
+  <comment>
+    <!-- empty comment -->
+    <sender name="nobody"/>
+    <date>2005-01-01T14:00:00Z</date>
+    <text></text>
+  </comment>
+  <comment>
+    <!-- empty comment with attachment -->
+    <sender name="nobody"/>
+    <date>2005-01-01T15:00:00Z</date>
+    <text></text>
+    <attachment>
+      <type>UNSPECIFIED</type>
+      <filename>hello.txt</filename>
+      <title>Hello</title>
+      <mimetype>text/plain</mimetype>
+      <contents>SGVsbG8gd29ybGQ=</contents>
+    </attachment>
+  </comment>
 </bug>'''
 
 duplicate_bug = '''\
@@ -491,50 +510,55 @@
         self.assertNotEqual(bug, None)
         # check bug attributes
         self.assertEqual(bug.owner.preferredemail.email, '[e-mail]')
-        self.assertEqual(bug.datecreated.isoformat(),
-                         '2004-10-12T12:00:00+00:00')
+        self.assertEqual(
+            bug.datecreated.isoformat(), '2004-10-12T12:00:00+00:00')
         self.assertEqual(bug.title, 'A test bug')
         self.assertEqual(bug.description, 'A modified bug description')
         self.assertEqual(bug.private, True)
         self.assertEqual(bug.security_related, True)
         self.assertEqual(bug.name, 'some-bug')
-        self.assertEqual(sorted(cve.sequence for cve in bug.cves),
-                         ['2005-2730', '2005-2736', '2005-2737'])
+        self.assertEqual(
+            sorted(cve.sequence for cve in bug.cves),
+            ['2005-2730', '2005-2736', '2005-2737'])
         self.assertEqual(bug.tags, ['bar', 'foo'])
         self.assertEqual(len(bug.getDirectSubscribers()), 2)
-        self.assertEqual(sorted(person.preferredemail.email
-                                for person in bug.getDirectSubscribers()),
-                         ['[e-mail]', '[e-mail]'])
+        self.assertEqual(
+            sorted(person.preferredemail.email for person in
+                bug.getDirectSubscribers()),
+            ['[e-mail]', '[e-mail]'])
         # There are two bug watches
         self.assertEqual(bug.watches.count(), 2)
-        self.assertEqual(sorted(watch.url for watch in bug.watches),
-                         ['http://bugzilla.gnome.org/show_bug.cgi?id=43',
-                          'https://bugzilla.mozilla.org/show_bug.cgi?id=42'])
+        self.assertEqual(
+            sorted(watch.url for watch in bug.watches),
+            ['http://bugzilla.gnome.org/show_bug.cgi?id=43',
+            'https://bugzilla.mozilla.org/show_bug.cgi?id=42'])
 
         # There should only be one bug task (on netapplet):
         self.assertEqual(len(bug.bugtasks), 1)
         bugtask = bug.bugtasks[0]
         self.assertEqual(bugtask.product, product)
-        self.assertEqual(bugtask.datecreated.isoformat(),
-                         '2004-10-12T12:00:00+00:00')
+        self.assertEqual(
+            bugtask.datecreated.isoformat(), '2004-10-12T12:00:00+00:00')
         self.assertEqual(bugtask.importance, BugTaskImportance.HIGH)
         self.assertEqual(bugtask.status, BugTaskStatus.CONFIRMED)
-        self.assertEqual(bugtask.assignee.preferredemail.email,
-                         '[e-mail]')
+        self.assertEqual(
+            bugtask.assignee.preferredemail.email, '[e-mail]')
         self.assertNotEqual(bugtask.milestone, None)
         self.assertEqual(bugtask.milestone.name, 'future')
 
-        # there are three comments:
-        self.assertEqual(bug.messages.count(), 3)
+        # there are five comments:
+        self.assertEqual(bug.messages.count(), 5)
         message1 = bug.messages[0]
         message2 = bug.messages[1]
         message3 = bug.messages[2]
+        message4 = bug.messages[3]
+        message5 = bug.messages[4]
 
         # Message 1:
-        self.assertEqual(message1.owner.preferredemail.email,
-                         '[e-mail]')
-        self.assertEqual(message1.datecreated.isoformat(),
-                         '2004-10-12T12:00:00+00:00')
+        self.assertEqual(
+            message1.owner.preferredemail.email, '[e-mail]')
+        self.assertEqual(
+            message1.datecreated.isoformat(), '2004-10-12T12:00:00+00:00')
         self.assertEqual(message1.subject, 'A test bug')
         self.assertEqual(message1.text_contents, 'Original description')
         self.assertEqual(message1.bugattachments.count(), 1)
@@ -545,19 +569,19 @@
         self.assertEqual(attachment.libraryfile.mimetype, 'text/plain')
 
         # Message 2:
-        self.assertEqual(message2.owner.preferredemail.email,
-                         '[e-mail]')
-        self.assertEqual(message2.datecreated.isoformat(),
-                         '2005-01-01T11:00:00+00:00')
+        self.assertEqual(
+            message2.owner.preferredemail.email, '[e-mail]')
+        self.assertEqual(
+            message2.datecreated.isoformat(), '2005-01-01T11:00:00+00:00')
         self.assertEqual(message2.subject, 'Re: A test bug')
-        self.assertEqual(message2.text_contents,
-                         'A comment from an anonymous user')
+        self.assertEqual(
+            message2.text_contents, 'A comment from an anonymous user')
 
         # Message 3:
-        self.assertEqual(message3.owner.preferredemail.email,
-                         '[e-mail]')
-        self.assertEqual(message3.datecreated.isoformat(),
-                         '2005-01-01T13:00:00+00:00')
+        self.assertEqual(
+            message3.owner.preferredemail.email, '[e-mail]')
+        self.assertEqual(
+            message3.datecreated.isoformat(), '2005-01-01T13:00:00+00:00')
         self.assertEqual(message3.subject, 'Re: A test bug')
         self.assertEqual(
             message3.text_contents,
@@ -581,6 +605,29 @@
         # mime type forced to text/plain because we have a patch
         self.assertEqual(attachment2.libraryfile.mimetype, 'text/plain')
 
+        # Message 4:
+        self.assertEqual(
+            message4.owner.preferredemail.email, '[e-mail]')
+        self.assertEqual(
+            message4.datecreated.isoformat(), '2005-01-01T14:00:00+00:00')
+        self.assertEqual(message4.subject, 'Re: A test bug')
+        self.assertEqual(message4.text_contents, '<empty comment>')
+        self.assertEqual(message4.bugattachments.count(), 0)
+
+        # Message 5:
+        self.assertEqual(
+            message5.owner.preferredemail.email, '[e-mail]')
+        self.assertEqual(
+            message5.datecreated.isoformat(), '2005-01-01T15:00:00+00:00')
+        self.assertEqual(message5.subject, 'Re: A test bug')
+        self.assertEqual(message5.text_contents, '')
+        self.assertEqual(message5.bugattachments.count(), 1)
+        attachment = message5.bugattachments[0]
+        self.assertEqual(attachment.type, BugAttachmentType.UNSPECIFIED)
+        self.assertEqual(attachment.title, 'Hello')
+        self.assertEqual(attachment.libraryfile.filename, 'hello.txt')
+        self.assertEqual(attachment.libraryfile.mimetype, 'text/plain')
+
         self.assertNoPendingNotifications(bug)
 
     def test_duplicate_bug(self):

=== modified file 'scripts/bug-import.py'
--- scripts/bug-import.py	2010-11-08 12:52:43 +0000
+++ scripts/bug-import.py	2010-12-16 17:06:41 +0000
@@ -22,21 +22,22 @@
     loglevel = logging.INFO
 
     def add_my_options(self):
-        self.parser.add_option('-p', '--product', metavar='PRODUCT',
-                               action='store',
-                               help='Which product to export',
-                               type='string', dest='product', default=None)
-        self.parser.add_option('--cache', metavar='FILE', action='store',
-                               help='Cache for bug ID mapping',
-                               type='string', dest='cache_filename',
-                               default='bug-map.pickle')
+        self.parser.add_option(
+            '-p', '--product', metavar='PRODUCT', action='store',
+            help='Which product to export', type='string', dest='product',
+            default=None)
+        self.parser.add_option(
+            '--cache', metavar='FILE', action='store',
+            help='Cache for bug ID mapping', type='string',
+            dest='cache_filename', default='bug-map.pickle')
         # XXX: jamesh 2007-04-11 bugs=86352
         # Not verifying users created by a bug import can result in
         # problems with mail notification, so should not be used for
         # imports.
-        self.parser.add_option('--dont-verify-users', dest='verify_users',
-                               help="Don't verify newly created users",
-                               action='store_false', default=True)
+        self.parser.add_option(
+            '--dont-verify-users', dest='verify_users',
+            help="Don't verify newly created users", action='store_false',
+            default=True)
 
     def main(self):
         if self.options.product is None: