launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/assertion-error-697294 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #697294 AssertionError editing a question 
  https://bugs.launchpad.net/bugs/697294

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/assertion-error-697294/+merge/47911

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
-- 
https://code.launchpad.net/~jcsackett/launchpad/assertion-error-697294/+merge/47911
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/assertion-error-697294 into lp:launchpad.

=== 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-01-29 17:25:14 +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-01-29 17:25:14 +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

=== 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-01-29 17:25:14 +0000
@@ -45,41 +45,24 @@
     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.")
-
     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))