launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03424
[Merge] lp:~sinzui/launchpad/bugtask-create-question-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/bugtask-create-question-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #247685 in Launchpad itself: "Email for a question converted from a bug has the wrong sender"
https://bugs.launchpad.net/launchpad/+bug/247685
Bug #304960 in Launchpad itself: "Bug converted to Question lost subscribers"
https://bugs.launchpad.net/launchpad/+bug/304960
Bug #438116 in Launchpad itself: "Timeout when converting bug into question (BugTask:+create-question)"
https://bugs.launchpad.net/launchpad/+bug/438116
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/bugtask-create-question-1/+merge/58826
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
--
https://code.launchpad.net/~sinzui/launchpad/bugtask-create-question-1/+merge/58826
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/bugtask-create-question-1 into lp:launchpad.
=== 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 15:47:26 +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 recieve 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 15:47:26 +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' % (
@@ -318,7 +332,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 +366,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 +484,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 15:47:26 +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 15:47:26 +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 15:47:26 +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__))
