Merge lp:~sinzui/launchpad/bugtask-create-question-1 into lp:launchpad

[Curtis Hovey]

Send the first and last messages only when converting a bug to a question.

    Launchpad bugs:
        https://bugs.launchpad.net/bugs/438116
        https://bugs.launchpad.net/bugs/247685
        https://bugs.launchpad.net/bugs/304960
    Pre-implementation: jcsackett, lifeless

BugTask:+create-question times out:
There are two principle issues described in this bug: sending multiple emails
to users, and sending them in proc. The later issue is also the cause of bug
608037, bug 618390, and bug 618385. Fixing this bug will only be about
addressing the unpredictable number of messages.

While reading the CreateQuestionFromBug() I saw two places that are creating
more emails than we want. I also saw that there are two other bugs that
can be fixed while tuning this method.

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

RULES

bugs #438116 "Timeout when converting bug into question "
    * Convert only the last bug message to a question message.
    * Do not send emails about the implicit bug link (create it directly?)

bug #247685 "Email for a question converted from a bug has the wrong sender"
    * Ensure the sender of the first message is the user who actually sent
      the message. The sender comes from the user in the event...this
      needs to be corrected.

bug #304960 "Bug converted to Question lost subscribers"
    * Ensure direct subscribers to the bug are added as direct subscribers
      to the question.

QA

    * Convert a bug with more than two comments and at least two subscribers
      to a question.
    * Verify each contact gets two messages, the first from the user
      who reported the bug, and a second from the user who converted the
      bug to a question.
    * Verify the question has the same direct subscribers as the bug.

LINT

    lib/lp/answers/notification.py
    lib/lp/answers/model/question.py
    lib/lp/answers/tests/test_question_notifications.py
    lib/lp/answers/tests/test_questiontarget.py
    lib/lp/bugs/tests/test_bugtarget.py

TEST

    ./bin/test -vv -t TestQuestionTargetCreateQuestionFromBug \
        -t QuestionCreatedTestCase -t bugtarget-questiontarget

IMPLEMENTATION

The bug link is directly created instead of using the helper method. The
method sends an duplicate email about the bug to the answer contacts, and
it is subscribing the question owner to a bug hie is already subscribed too.
The message loop is replaced with a single message pulled from the end of
the bug messages. The direct bug subscribers are subscribed to the question
so that they can learn the answer.
    lib/lp/answers/model/question.py
    lib/lp/answers/tests/test_questiontarget.py

Added a property to select the correct user when sending emails. Since
Questions can be created for users, the question owner is the real sender
of the email (bug #247685). The test is somewhat arcane. The use of fakes
allows the tests to run on the lowest layer and does not send emails.
    lib/lp/answers/notification.py
    lib/lp/answers/tests/test_question_notifications.py

Run the bugtarget doctests on the correct layer.
    lib/lp/bugs/tests/test_bugtarget.py

[Brad Crittenden (bac)]

Hi Curtis,

Thanks for tackling this really old problem. Our users will be glad to get this fixed.

* s/recieve/receive

The tests look good and the code makes sense.

review: Approve (code)

=== modified file 'lib/lp/answers/doc/notifications.txt'
--- lib/lp/answers/doc/notifications.txt	2011-03-23 16:28:51 +0000
+++ lib/lp/answers/doc/notifications.txt	2011-04-22 21:17:28 +0000
@@ -852,15 +852,19 @@
     >>> bug_question = ubuntu.createQuestionFromBug(bug)
     >>> notifications = pop_notifications()
     >>> len(notifications)
-    2
+    4
 
     >>> [email_msg['To'] for email_msg in notifications]
-    ['[e-mail]', '[e-mail]']
+    ['[e-mail]', '[e-mail]',
+     '[e-mail]', '[e-mail]']
+
+    >>> notifications[0]['Subject']
+    '[Question #...]: Installer fails on a Mac PPC'
 
     >>> notifications[1]['Subject']
-    '[Question #...]: Installer fails on a Mac PPC'
+    'Re: [Question #...]: Installer fails on a Mac PPC'
 
-    >>> notification_body = notifications[1].get_payload(decode=True)
+    >>> notification_body = notifications[0].get_payload(decode=True)
     >>> print notification_body
     New question #... on Ubuntu:
     http://answers.launchpad.dev/ubuntu/+question/...

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2011-04-21 21:14:50 +0000
+++ lib/lp/answers/model/question.py	2011-04-22 21:17:28 +0000
@@ -1190,13 +1190,18 @@
         # Give the datelastresponse a current datetime, otherwise the
         # Launchpad Janitor would quickly expire questions made from old bugs.
         question.datelastresponse = datetime.now(pytz.timezone('UTC'))
-        question.linkBug(bug)
-        for message in bug.messages[1:]:
-            # Bug.message[0] is the original message, and probably a duplicate
-            # of Bug.description.
-            question.addComment(
-                message.owner, message.text_contents,
-                datecreated=message.datecreated)
+        # Directly create the BugLink so that users do not receive duplicate
+        # messages about the bug.
+        question.createBugLink(bug)
+        # Copy the last message that explains why the bug is a question.
+        message = bug.messages[-1]
+        question.addComment(
+            message.owner, message.text_contents,
+            datecreated=message.datecreated)
+        # Direct subscribers to the bug want to know the question answer.
+        for subscriber in bug.getDirectSubscribers():
+            if subscriber != question.owner:
+                question.subscribe(subscriber)
         return question
 
     def getQuestion(self, question_id):

=== modified file 'lib/lp/answers/notification.py'
--- lib/lp/answers/notification.py	2010-08-24 10:45:57 +0000
+++ lib/lp/answers/notification.py	2011-04-22 21:17:28 +0000
@@ -49,11 +49,16 @@
         """
         self.question = question
         self.event = event
-        self.user = IPerson(self.event.user)
+        self._user = IPerson(self.event.user)
         self.initialize()
         if self.shouldNotify():
             self.send()
 
+    @property
+    def user(self):
+        """Return the user from the event. """
+        return self._user
+
     def getFromAddress(self):
         """Return a formatted email address suitable for user in the From
         header of the question notification.
@@ -180,6 +185,15 @@
 class QuestionAddedNotification(QuestionNotification):
     """Notification sent when a question is added."""
 
+    @property
+    def user(self):
+        """Return the question owner.
+
+        Questions can be created by other users for the owner; the
+        question is from the owner.
+        """
+        return self.question.owner
+
     def getBody(self):
         """See QuestionNotification."""
         question = self.question
@@ -222,7 +236,7 @@
         """Textual representation of the changes to the question metadata."""
         question = self.question
         old_question = self.old_question
-        indent = 4*' '
+        indent = 4 * ' '
         info_fields = []
         if question.status != old_question.status:
             info_fields.append(indent + 'Status: %s => %s' % (
@@ -306,10 +320,6 @@
             # The first message cannot contain a References
             # because we don't create a Message instance for the
             # question description, so we don't have a Message-ID.
-
-            # XXX sinzui 2007-02-01 bug=164435:
-            # Added an assert to gather better Opps information about
-            # the state of the messages.
             messages = list(self.question.messages)
             assert self.new_message in messages, (
                 "Question %s: message id %s not in %s." % (
@@ -318,7 +328,7 @@
             index = messages.index(self.new_message)
             if index > 0:
                 headers['References'] = (
-                    self.question.messages[index-1].rfc822msgid)
+                    self.question.messages[index - 1].rfc822msgid)
         return headers
 
     def shouldNotify(self):
@@ -352,7 +362,6 @@
         """
         original_recipients = QuestionNotification.getRecipients(self)
         recipients = NotificationRecipientSet()
-        owner = self.question.owner
         for person in original_recipients:
             if person != self.question.owner:
                 rationale, header = original_recipients.getReason(person)
@@ -471,5 +480,3 @@
             'question_url': canonical_url(question),
             'question_language': question.language.englishname,
             'comment': question.description}
-
-

=== modified file 'lib/lp/answers/tests/test_question_notifications.py'
--- lib/lp/answers/tests/test_question_notifications.py	2010-07-18 00:21:11 +0000
+++ lib/lp/answers/tests/test_question_notifications.py	2011-04-22 21:17:28 +0000
@@ -9,7 +9,10 @@
 
 from zope.interface import implements
 
-from lp.answers.notification import QuestionModifiedDefaultNotification
+from lp.answers.notification import (
+    QuestionAddedNotification,
+    QuestionModifiedDefaultNotification,
+    )
 from lp.registry.interfaces.person import IPerson
 
 
@@ -35,6 +38,7 @@
     def __init__(self, id=1, title="Question title"):
         self.id = id
         self.title = title
+        self.owner = FakeUser()
 
 
 class StubQuestionMessage:
@@ -109,3 +113,30 @@
         self.assertEquals(
             'Re: [Question #1]: Message subject',
             self.notification.getSubject())
+
+    def test_user_is_event_user(self):
+        """The notification user is always the event user."""
+        question = StubQuestion()
+        event = FakeEvent()
+        notification = TestQuestionModifiedNotification(question, event)
+        self.assertEqual(event.user, notification.user)
+        self.assertNotEqual(question.owner, notification.user)
+
+
+class TestQuestionAddedNotification(QuestionAddedNotification):
+    """A subclass that does not send emails."""
+
+    def shouldNotify(self):
+        return False
+
+
+class QuestionCreatedTestCase(TestCase):
+    """Test cases for mail notifications about created questions."""
+
+    def test_user_is_question_owner(self):
+        """The notification user is always the question owner."""
+        question = StubQuestion()
+        event = FakeEvent()
+        notification = TestQuestionAddedNotification(question, event)
+        self.assertEqual(question.owner, notification.user)
+        self.assertNotEqual(event.user, notification.user)

=== modified file 'lib/lp/answers/tests/test_questiontarget.py'
--- lib/lp/answers/tests/test_questiontarget.py	2011-04-21 21:22:16 +0000
+++ lib/lp/answers/tests/test_questiontarget.py	2011-04-22 21:17:28 +0000
@@ -14,7 +14,10 @@
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 
 
 class TestQuestionTarget_answer_contacts_with_languages(TestCaseWithFactory):
@@ -74,3 +77,41 @@
             lang.englishname for lang in answer_contact.getLanguagesCache()]
         # The languages cache has been filled in the correct order.
         self.failUnlessEqual(langs, [u'English', u'Portuguese (Brazil)'])
+
+
+class TestQuestionTargetCreateQuestionFromBug(TestCaseWithFactory):
+    """Test the createQuestionFromBug from bug behavior."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestQuestionTargetCreateQuestionFromBug, self).setUp()
+        self.bug = self.factory.makeBug(description="first comment")
+        self.target = self.bug.bugtasks[0].target
+        self.contributor = self.target.owner
+        self.reporter = self.bug.owner
+
+    def test_first_and_last_messages_copied_to_question(self):
+        # The question is created with the bug's description and the last
+        # message which presumably is about why the bug was converted.
+        with person_logged_in(self.reporter):
+            self.bug.newMessage(owner=self.reporter, content='second comment')
+        with person_logged_in(self.contributor):
+            last_message = self.bug.newMessage(
+                owner=self.contributor, content='third comment')
+            question = self.target.createQuestionFromBug(self.bug)
+        question_messages = list(question.messages)
+        self.assertEqual(1, len(question_messages))
+        self.assertEqual(last_message.content, question_messages[0].content)
+        self.assertEqual(self.bug.description, question.description)
+
+    def test_bug_subscribers_copied_to_question(self):
+        # Users who subscribe to the bug are also interested in the answer.
+        subscriber = self.factory.makePerson()
+        with person_logged_in(subscriber):
+            self.bug.subscribe(subscriber, subscriber)
+        with person_logged_in(self.contributor):
+            self.bug.newMessage(owner=self.contributor, content='comment')
+            question = self.target.createQuestionFromBug(self.bug)
+        self.assertTrue(question.isSubscribed(subscriber))
+        self.assertTrue(question.isSubscribed(question.owner))

=== modified file 'lib/lp/bugs/tests/test_bugtarget.py'
--- lib/lp/bugs/tests/test_bugtarget.py	2010-11-18 14:00:49 +0000
+++ lib/lp/bugs/tests/test_bugtarget.py	2011-04-22 21:17:28 +0000
@@ -28,7 +28,7 @@
     tearDown,
     )
 from canonical.launchpad.webapp.interfaces import ILaunchBag
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.interfaces.bug import CreateBugParams
 from lp.bugs.interfaces.bugtask import (
     BugTaskSearchParams,
@@ -191,7 +191,7 @@
 class TestBugTargetSearchTasks(TestCaseWithFactory):
     """Tests of IHasBugs.searchTasks()."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestBugTargetSearchTasks, self).setUp()
@@ -299,7 +299,7 @@
     for setUpMethod in setUpMethods:
         test = LayeredDocFileSuite('bugtarget-questiontarget.txt',
             setUp=setUpMethod, tearDown=tearDown,
-            layer=LaunchpadFunctionalLayer)
+            layer=DatabaseFunctionalLayer)
         suite.addTest(test)
 
     setUpMethods.remove(sourcePackageForQuestionSetUp)
@@ -309,7 +309,7 @@
     for setUpMethod in setUpMethods:
         test = LayeredDocFileSuite('bugtarget-bugcount.txt',
             setUp=setUpMethod, tearDown=tearDown,
-            layer=LaunchpadFunctionalLayer)
+            layer=DatabaseFunctionalLayer)
         suite.addTest(test)
 
     suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))