Merge lp:~jcsackett/launchpad/assertion-error-697294 into lp:launchpad

[j.c.sackett]

[r=henninge][ui=none][bug=697294] Refactors create_questionreopening to be called as a helper, rather than a listener to notifications; this avoids the need for a slew of guards on the method, and keeps it from doing work when we don't want it to.

Summary
=======

In bug 697294, an assertion error would popup from a question in a bad state hitting question reopen (see bug 709816 about the bad state error). While the bad state is of low importance, question reopen was erroneously being called and attempting to do work it shouldn't do.

To handle this, create_questionreopening is refactored into a helper method, rather than notification listener that is called on all status events.

Preimplementation
=================

Spoke with Curtis Hovey.

Proposed Fix
============

Refactor the reopening method into a helper rather than a notification listener.

Implementation
==============

lib/lp/answers/configure.zcml
-----------------------------

Removed create_questionreopening from the list of notification subscribers.

lib/lp/answers/model/question.py
--------------------------------

Altered reopen and setStatus to call create_questionreopening directly.

lib/lp/answers/model/questionreopening.py
-----------------------------------------

create_questionreopening had its various guards removed, as it will only be called when a reopening will actually be called.

Tests
=====

bin/test -t answers.*workflow

Demo & QA
=========

Run through a workflow on answers with any question you can reopen and declare solved. All transitions should work as expected.

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/configure.zcml
  lib/lp/answers/model/question.py
  lib/lp/answers/model/questionreopening.py

[Henning Eggers (henninge)]

Thanks for doing this! All I really miss is a test that triggers the assertion error (without the fix), all in the spirit of TDD. ;-) Can you please add that test or explain why it is not needed?

Also, I am not sure if formatting one parameter per line always increases readability or just length.

Cheers,
Henning

review: Needs Fixing

[j.c.sackett (jcsackett)]

Henninge--

There's not actually a way to simulate the events that caused the assertion error--it requires a mid-air collision between email processing and web ui components that can't really be simulated in our test infrastructure. See bug 709816 for details on the situation that would need to be simulated.

This refactor codes around the circumstances to add some robustness so the bad state doesn't enter the code the assertion error occured in (by removing that block of code outright).

[Henning Eggers (henninge)]

OIC

review: Approve (code)

=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml	2010-10-03 15:30:06 +0000
+++ lib/lp/answers/configure.zcml	2011-02-04 16:55:00 +0000
@@ -60,12 +60,6 @@
         handler=".karma.question_comment_added"
         />
 
-    <subscriber
-        for=".interfaces.question.IQuestion
-             lazr.lifecycle.interfaces.IObjectModifiedEvent"
-        handler=".model.questionreopening.create_questionreopening"
-        />
-
     <class class=".model.questionreopening.QuestionReopening">
         <allow
           interface=".interfaces.questionreopening.IQuestionReopening"

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2010-11-15 21:56:43 +0000
+++ lib/lp/answers/model/question.py	2011-02-04 16:55:00 +0000
@@ -87,6 +87,7 @@
 from lp.answers.interfaces.questiontarget import IQuestionTarget
 from lp.answers.model.answercontact import AnswerContact
 from lp.answers.model.questionmessage import QuestionMessage
+from lp.answers.model.questionreopening import create_questionreopening
 from lp.answers.model.questionsubscription import QuestionSubscription
 from lp.app.enums import ServiceUsage
 from lp.bugs.interfaces.buglink import IBugLinkTarget
@@ -278,16 +279,29 @@
             raise InvalidQuestionStateError(
                 "New status is same as the old one.")
 
+
         # If the previous state recorded an answer, clear those
-        # information as well.
+        # information as well, but copy it out for the reopening.
+        old_status = self.status
+        old_answerer = self.answerer
+        old_date_solved = self.date_solved
         self.answerer = None
         self.answer = None
         self.date_solved = None
 
-        return self._newMessage(
+        msg = self._newMessage(
             user, comment, datecreated=datecreated,
             action=QuestionAction.SETSTATUS, new_status=new_status)
 
+        if new_status == QuestionStatus.OPEN:
+            create_questionreopening(
+                self,
+                msg,
+                old_status,
+                old_answerer,
+                old_date_solved)
+        return msg
+
     @notify_question_modified()
     def addComment(self, user, comment, datecreated=None):
         """See `IQuestion`."""
@@ -474,12 +488,24 @@
     @notify_question_modified()
     def reopen(self, comment, datecreated=None):
         """See `IQuestion`."""
+        old_status = self.status
+        old_answerer = self.answerer
+        old_date_solved = self.date_solved
         if not self.can_reopen:
             raise InvalidQuestionStateError(
                 "Question status != ANSWERED, EXPIRED or SOLVED.")
         msg = self._newMessage(
-            self.owner, comment, datecreated=datecreated,
-            action=QuestionAction.REOPEN, new_status=QuestionStatus.OPEN)
+            self.owner,
+            comment,
+            datecreated=datecreated,
+            action=QuestionAction.REOPEN,
+            new_status=QuestionStatus.OPEN)
+        create_questionreopening(
+            self,
+            msg,
+            old_status,
+            old_answerer,
+            old_date_solved)
         self.answer = None
         self.answerer = None
         self.date_solved = None
@@ -705,7 +731,6 @@
                 raise AssertionError(
                     'product_id and distribution_id are NULL')
         return projects
-        
 
     @staticmethod
     def new(title=None, description=None, owner=None,

=== modified file 'lib/lp/answers/model/questionreopening.py'
--- lib/lp/answers/model/questionreopening.py	2010-10-03 15:30:06 +0000
+++ lib/lp/answers/model/questionreopening.py	2011-02-04 16:55:00 +0000
@@ -45,41 +45,27 @@
     priorstate = EnumCol(schema=QuestionStatus, notNull=True)
 
 
-def create_questionreopening(question, event):
-    """Event subscriber that creates a QuestionReopening event.
+def create_questionreopening(
+        question,
+        reopen_msg,
+        old_status,
+        old_answerer,
+        old_date_solved):
+    """Helper function to handle question reopening.
 
-    A QuestionReopening is created question with an answer changes back to the
-    OPEN state.
+    A QuestionReopening is created when question with an answer changes back
+    to the OPEN state.
     """
-    # XXX flacoste 2006-10-25 The QuestionReopening is probably not that
-    # useful anymore since the question history is nearly complete.
-    # If we decide to still keep that class, this subscriber should
-    # probably be moved outside of database code.
-    if question.status != QuestionStatus.OPEN:
-        return
-
-    # Only create a QuestionReopening if the question had previsouly an
-    # answer.
-    old_question = event.object_before_modification
-    if old_question.answerer is None:
-        return
-    assert question.answerer is None, (
-        "Open question shouldn't have an answerer.")
-
-    # The last added message is the cause of the reopening.
-    reopen_msg = question.messages[-1]
-
-    # Make sure that the last message is really the last added one.
-    assert [reopen_msg] == (
-        list(set(question.messages).difference(old_question.messages))), (
-            "Reopening message isn't the last one.")
-
+    # XXX jcsackett This guard has to be maintained because reopen can
+    # be called with the question in a bad state.
+    if old_answerer is None:
+        return
     reopening = QuestionReopening(
-            question=question, reopener=reopen_msg.owner,
+            question=question,
+            reopener=reopen_msg.owner,
             datecreated=reopen_msg.datecreated,
-            answerer=old_question.answerer,
-            date_solved=old_question.date_solved,
-            priorstate=old_question.status)
-
+            answerer=old_answerer,
+            date_solved=old_date_solved,
+            priorstate=old_status)
     reopening = ProxyFactory(reopening)
     notify(ObjectCreatedEvent(reopening, user=reopen_msg.owner))

=== modified file 'lib/lp/answers/tests/emailinterface.txt'
--- lib/lp/answers/tests/emailinterface.txt	2010-12-13 15:25:03 +0000
+++ lib/lp/answers/tests/emailinterface.txt	2011-02-04 16:55:00 +0000
@@ -1,4 +1,5 @@
-= Answer Tracker Email Interface =
+Answer Tracker Email Interface
+==============================
 
 The Answer Tracker has an email interface, although it's quite limited
 at the moment. The only thing you can do is post new messages on the
@@ -79,7 +80,8 @@
     >>> comment_msgid is None
     True
 
-== Incoming Email and Workflow ==
+Incoming Email and Workflow
+---------------------------
 
 With the way the Answer Tracker workflow is modelled (see
 answer-tracker-workflow.txt for the details), adding a message will
@@ -131,13 +133,15 @@
     ...                            datecreated=now.next())
     ...     login('[e-mail]')
 
-== Message From the Question Owner ==
+Message From the Question Owner
+-------------------------------
 
 When the owner sends a message on the question, the message is
 interpretated in three different manners based on the current question
 state.
 
-=== Open and Needs Information ===
+Open and Needs Information
+..........................
 
 In the Open and Needs Information states, we assume the message provides
 more information on the problem.
@@ -180,7 +184,8 @@
 correct our bad decision, since the question will stay on his
 list of open questions.
 
-=== Answered and Expired ===
+Answered and Expired
+....................
 
 When the question is in the Answered or Expired states, we assume that
 the email is reopening the question with more information.
@@ -223,7 +228,8 @@
 confusion, the outoing notification contain a footer explaining what
 will happen if one reply to the message.
 
-=== Solved and Invalid ===
+Solved and Invalid
+..................
 
 When the question is in the Solved or Invalid state, we interpret the
 message as a comment.
@@ -262,7 +268,8 @@
 state, there is no normal transition. The only possibility is that an
 admin comes to change the status of the question.
 
-=== Message From Another User ===
+Message From Another User
+.........................
 
 It is simpler when a user other than the owner sends an email. When
 the question is in the Open or Needs information state, there are only
@@ -315,7 +322,8 @@
     Answer
 
 
-=== Solved, Invalid and Expired ===
+Solved, Invalid and Expired
+...........................
 
 When another user than the owner sends a message to a question
 in the Solved, Invalid or Expired states, the only possible
@@ -357,7 +365,8 @@
     Comment
 
 
-=== Answers linked to FAQ questions ===
+Answers linked to FAQ questions
+...............................
 
 Answers may also be linked to FAQ questions.
 
@@ -387,7 +396,8 @@
     Answer
 
 
-== AnswerTrackerHandler Integration ==
+AnswerTrackerHandler Integration
+--------------------------------
 
 The general mail processor delegates all emails to the
 config.answertracker.email_domain to the AnswerTrackerHandler.