launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:questions-about-storm-messages into launchpad:master.

Commit message:
Convert QuestionMessage to Storm

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/386166
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:questions-about-storm-messages into launchpad:master.

diff --git a/lib/lp/answers/browser/tests/views.txt b/lib/lp/answers/browser/tests/views.txt
index f552ca4..3a87546 100644
--- a/lib/lp/answers/browser/tests/views.txt
+++ b/lib/lp/answers/browser/tests/views.txt
@@ -214,7 +214,7 @@ Let's say they confirm the previous answer, in this case, the question
 will move to the 'SOLVED' state. Note that the UI doesn't enable the
 user to enter a confirmation message at that stage.
 
-    >>> answer_message_number = firefox_question.messages.count() - 1
+    >>> answer_message_number = len(firefox_question.messages) - 1
     >>> workflow_harness.submit(
     ...     'confirm', {'answer_id': answer_message_number,
     ...                 'field.message': ''})
diff --git a/lib/lp/answers/doc/expiration.txt b/lib/lp/answers/doc/expiration.txt
index bacb58c..5027480 100644
--- a/lib/lp/answers/doc/expiration.txt
+++ b/lib/lp/answers/doc/expiration.txt
@@ -65,13 +65,13 @@ somebody are subject to expiration.
     >>> recent_open_question = questionset.get(2)
     >>> recent_open_question.giveInfo(
     ...     'SVG works better now, but is still broken')
-    <QuestionMessage...>
+    <lp.answers.model.questionmessage.QuestionMessage...>
 
     # This one was put in the NEEDSINFO state recently.
     >>> recent_needsinfo_question = questionset.get(4)
     >>> recent_needsinfo_question.requestInfo(
     ...     no_priv, 'What URL were you visiting?')
-    <QuestionMessage...>
+    <lp.answers.model.questionmessage.QuestionMessage...>
 
     # Old open questions.
     >>> old_open_question = questionset.get(5)
diff --git a/lib/lp/answers/doc/notifications.txt b/lib/lp/answers/doc/notifications.txt
index 0faaa18..1aa97dd 100644
--- a/lib/lp/answers/doc/notifications.txt
+++ b/lib/lp/answers/doc/notifications.txt
@@ -707,7 +707,7 @@ the notifications.
 
     >>> pt_BR_question.giveInfo(
     ...     "Veja o screenshot: http://tinyurl.com/y8jq8z";)
-    <QuestionMessage...>
+    <lp.answers.model.questionmessage.QuestionMessage...>
 
     >>> ignore = pop_questionemailjobs()
 
diff --git a/lib/lp/answers/doc/person.txt b/lib/lp/answers/doc/person.txt
index 2a33486..41333cf 100644
--- a/lib/lp/answers/doc/person.txt
+++ b/lib/lp/answers/doc/person.txt
@@ -237,7 +237,7 @@ subscribed to...
 
     >>> es_question = getUtility(IQuestionSet).get(12)
     >>> es_question.reject(foo_bar_raw, 'Reject question.')
-    <QuestionMessage...>
+    <lp.answers.model.questionmessage.QuestionMessage...>
 
     >>> print(', '.join(
     ...     sorted(language.code
@@ -257,7 +257,7 @@ subscribed to...
     >>> en_question = getUtility(IQuestionSet).get(1)
     >>> login('carlos@xxxxxxxxxxxxx')
     >>> en_question.addComment(carlos_raw, 'A simple comment.')
-    <QuestionMessage...>
+    <lp.answers.model.questionmessage.QuestionMessage...>
 
     >>> print(', '.join(
     ...     sorted(language.code
diff --git a/lib/lp/answers/doc/questionsets.txt b/lib/lp/answers/doc/questionsets.txt
index 9655993..8afcd0f 100644
--- a/lib/lp/answers/doc/questionsets.txt
+++ b/lib/lp/answers/doc/questionsets.txt
@@ -275,7 +275,7 @@ It returns the number of open questions for each given package.
     ...     ubuntu_evolution, 2)
     >>> closed_question.setStatus(
     ...     closed_question.owner, QuestionStatus.SOLVED, 'no comment')
-    <QuestionMessage at ...>
+    <lp.answers.model.questionmessage.QuestionMessage ...>
 
     >>> packages = (
     ...     ubuntu_evolution, ubuntu_pmount, debian_evolution, debian_pmount)
diff --git a/lib/lp/answers/model/question.py b/lib/lp/answers/model/question.py
index 921a0c6..de38575 100644
--- a/lib/lp/answers/model/question.py
+++ b/lib/lp/answers/model/question.py
@@ -34,6 +34,10 @@ from sqlobject import (
     StringCol,
     )
 from storm.expr import LeftJoin
+from storm.locals import (
+    Int,
+    Reference,
+    )
 from storm.references import ReferenceSet
 from storm.store import Store
 from zope.component import getUtility
@@ -186,8 +190,8 @@ class Question(SQLBase, BugLinkTargetMixin):
     answerer = ForeignKey(
         dbName='answerer', notNull=False, foreignKey='Person',
         storm_validator=validate_public_person, default=None)
-    answer = ForeignKey(dbName='answer', notNull=False,
-        foreignKey='QuestionMessage', default=None)
+    answer_id = Int(name='answer', allow_none=True, default=None)
+    answer = Reference(answer_id, 'QuestionMessage.id')
     datecreated = UtcDateTimeCol(notNull=True, default=DEFAULT)
     datedue = UtcDateTimeCol(notNull=False, default=None)
     datelastquery = UtcDateTimeCol(notNull=True, default=DEFAULT)
@@ -213,11 +217,18 @@ class Question(SQLBase, BugLinkTargetMixin):
     subscribers = ReferenceSet(
         'id', 'QuestionSubscription.question_id',
         'QuestionSubscription.person_id', 'Person.id', order_by='Person.name')
-    messages = SQLMultipleJoin('QuestionMessage', joinColumn='question',
-        prejoins=['message'], orderBy=['QuestionMessage.id'])
+    # This should be `messages`, but we use it in a SnapShot during the
+    # notification cycle, which don't appear to support saving the state of
+    # ReferenceSets, so use a list() property instead.
+    _messages = ReferenceSet(
+        'id', 'QuestionMessage.question_id', order_by='QuestionMessage.id')
     reopenings = SQLMultipleJoin('QuestionReopening', orderBy='datecreated',
         joinColumn='question')
 
+    @property
+    def messages(self):
+        return list(self._messages)
+
     # attributes
     def target(self):
         """See `IQuestion`."""
@@ -651,7 +662,7 @@ class Question(SQLBase, BugLinkTargetMixin):
             MessageChunk(message=msg, content=content, sequence=1)
 
         tktmsg = QuestionMessage(
-            question=self, message=msg, action=action, new_status=new_status)
+            self, msg, action, new_status, owner)
         notify(ObjectCreatedEvent(tktmsg, user=tktmsg.owner))
         # Make sure we update the relevant date of response or query.
         if update_question_dates:
diff --git a/lib/lp/answers/model/questionmessage.py b/lib/lp/answers/model/questionmessage.py
index 075a622..3acf257 100644
--- a/lib/lp/answers/model/questionmessage.py
+++ b/lib/lp/answers/model/questionmessage.py
@@ -10,7 +10,10 @@ __all__ = [
     ]
 
 from lazr.delegates import delegate_to
-from sqlobject import ForeignKey
+from storm.locals import (
+    Int,
+    Reference,
+    )
 from zope.interface import implementer
 
 from lp.answers.enums import (
@@ -19,40 +22,44 @@ from lp.answers.enums import (
     )
 from lp.answers.interfaces.questionmessage import IQuestionMessage
 from lp.registry.interfaces.person import validate_public_person
-from lp.services.database.enumcol import EnumCol
-from lp.services.database.sqlbase import SQLBase
+from lp.services.database.enumcol import DBEnum
+from lp.services.database.stormbase import StormBase
 from lp.services.messages.interfaces.message import IMessage
 from lp.services.propertycache import cachedproperty
 
 
 @implementer(IQuestionMessage)
 @delegate_to(IMessage, context='message')
-class QuestionMessage(SQLBase):
+class QuestionMessage(StormBase):
     """A table linking questions and messages."""
 
-    _table = 'QuestionMessage'
+    __storm_table__ = 'QuestionMessage'
+
+    id = Int(primary=True)
+    question_id = Int(name="question", allow_none=False)
+    question = Reference(question_id, 'Question.id')
 
-    question = ForeignKey(
-        dbName='question', foreignKey='Question', notNull=True)
-    message = ForeignKey(dbName='message', foreignKey='Message', notNull=True)
+    message_id = Int(name="message", allow_none=False)
+    message = Reference(message_id, 'Message.id')
 
-    action = EnumCol(
-        schema=QuestionAction, notNull=True, default=QuestionAction.COMMENT)
+    action = DBEnum(
+        name='action', enum=QuestionAction, default=QuestionAction.COMMENT,
+        allow_none=False)
 
-    new_status = EnumCol(
-        schema=QuestionStatus, notNull=True, default=QuestionStatus.OPEN)
+    new_status = DBEnum(
+        name='new_status', enum=QuestionStatus, default=QuestionStatus.OPEN,
+        allow_none=False)
 
-    owner = ForeignKey(dbName='owner', foreignKey='Person',
-        storm_validator=validate_public_person, notNull=True)
+    owner_id = Int(
+        name="owner", allow_none=False, validator=validate_public_person)
+    owner = Reference(owner_id, 'Person.id')
 
-    def __init__(self, **kwargs):
-        if 'owner' not in kwargs:
-            # Although a trigger will set the owner after the SQL
-            # INSERT has been executed, we must specify the parameter
-            # explicitly to fulfill the DB constraint OWNER NOT NULL,
-            # otherweise we'll get an error from the DB server.
-            kwargs['owner'] = kwargs['message'].owner
-        super(QuestionMessage, self).__init__(**kwargs)
+    def __init__(self, question, message, action, new_status, owner):
+        self.question = question
+        self.message = message
+        self.action = action
+        self.new_status = new_status
+        self.owner = owner if owner else self.message.owner
 
     def __iter__(self):
         """See IMessage."""