Merge ~cjwatson/launchpad:stormify-codereview into launchpad:master

[Colin Watson]

Convert CodeReviewComment and CodeReviewVoteReference to Storm

This requires a slight fix to the infrastructure for incoming email to decode message bodies to text at the appropriate point so that the code-review-by-email system keeps working.

[Ioana Lasc (ilasc)]

review: Approve

diff --git a/lib/lp/code/mail/tests/test_codehandler.py b/lib/lp/code/mail/tests/test_codehandler.py
index a02cc4d..97feda0 100644
--- a/lib/lp/code/mail/tests/test_codehandler.py
+++ b/lib/lp/code/mail/tests/test_codehandler.py
@@ -3,6 +3,8 @@
 
 """Testing the CodeHandler."""
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
 from textwrap import dedent
diff --git a/lib/lp/code/mail/tests/test_codereviewcomment.py b/lib/lp/code/mail/tests/test_codereviewcomment.py
index 930753e..a692a97 100644
--- a/lib/lp/code/mail/tests/test_codereviewcomment.py
+++ b/lib/lp/code/mail/tests/test_codereviewcomment.py
@@ -243,7 +243,7 @@ class TestCodeReviewComment(TestCaseWithFactory):
     def test_generateEmailWithVoteAndTag(self):
         """Ensure that vote tags are displayed."""
         mailer, subscriber = self.makeMailer(
-            vote=CodeReviewVote.APPROVE, vote_tag='DBTAG')
+            vote=CodeReviewVote.APPROVE, vote_tag=u'DBTAG')
         ctrl = mailer.generateEmail(
             subscriber.preferredemail.email, subscriber)
         self.assertEqual('Review: Approve dbtag', ctrl.body.splitlines()[0])
diff --git a/lib/lp/code/model/branchcollection.py b/lib/lp/code/model/branchcollection.py
index 7482778..bc3a026 100644
--- a/lib/lp/code/model/branchcollection.py
+++ b/lib/lp/code/model/branchcollection.py
@@ -484,10 +484,10 @@ class GenericBranchCollection:
         tables = [
             BranchMergeProposal,
             Join(CodeReviewVoteReference,
-                 CodeReviewVoteReference.branch_merge_proposalID == \
+                 CodeReviewVoteReference.branch_merge_proposal ==
                  BranchMergeProposal.id),
             LeftJoin(CodeReviewComment,
-                 CodeReviewVoteReference.commentID == CodeReviewComment.id)]
+                 CodeReviewVoteReference.comment == CodeReviewComment.id)]
 
         expressions = [
             CodeReviewVoteReference.reviewer == reviewer,
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 338cc29..2a346b4 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -46,6 +46,7 @@ from zope.interface import implementer
 from zope.security.interfaces import Unauthorized
 
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.bugs.interfaces.bugtaskfilter import filter_bugtasks_by_context
@@ -565,11 +566,14 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
     @property
     def all_comments(self):
         """See `IBranchMergeProposal`."""
-        return CodeReviewComment.selectBy(branch_merge_proposal=self.id)
+        return IStore(CodeReviewComment).find(
+            CodeReviewComment, branch_merge_proposal=self)
 
     def getComment(self, id):
         """See `IBranchMergeProposal`."""
-        comment = CodeReviewComment.get(id)
+        comment = IStore(CodeReviewComment).get(CodeReviewComment, id)
+        if comment is None:
+            raise NotFoundError(id)
         if comment.branch_merge_proposal != self:
             raise WrongBranchMergeProposal
         return comment
@@ -583,7 +587,10 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
 
     def setCommentVisibility(self, user, comment_number, visible):
         """See `IBranchMergeProposal`."""
-        comment = CodeReviewComment.get(comment_number)
+        comment = IStore(CodeReviewComment).get(
+            CodeReviewComment, comment_number)
+        if comment is None:
+            raise NotFoundError(comment_number)
         if comment.branch_merge_proposal != self:
             raise WrongBranchMergeProposal
         if not comment.userCanSetCommentVisibility(user):
@@ -596,7 +603,9 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
         """See `IBranchMergeProposal`.
 
         This function can raise WrongBranchMergeProposal."""
-        vote = CodeReviewVoteReference.get(id)
+        vote = IStore(CodeReviewVoteReference).get(CodeReviewVoteReference, id)
+        if vote is None:
+            raise NotFoundError(id)
         if vote.branch_merge_proposal != self:
             raise WrongBranchMergeProposal
         return vote
@@ -932,6 +941,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
                 date_created=_date_created)
             self._ensureAssociatedBranchesVisibleToReviewer(reviewer)
         vote_reference.review_type = review_type
+        Store.of(vote_reference).flush()
         if _notify_listeners:
             notify(ReviewerNominatedEvent(vote_reference))
         return vote_reference
@@ -1098,11 +1108,13 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             if team_ref is not None:
                 return team_ref
         # Create a new reference.
-        return CodeReviewVoteReference(
+        vote_reference = CodeReviewVoteReference(
             branch_merge_proposal=self,
             registrant=user,
             reviewer=user,
             review_type=review_type)
+        Store.of(vote_reference).flush()
+        return vote_reference
 
     def createCommentFromMessage(self, message, vote, review_type,
                                  original_email, _notify_listeners=True,
@@ -1126,6 +1138,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             vote_reference.reviewer = message.owner
             vote_reference.review_type = review_type
             vote_reference.comment = code_review_message
+        Store.of(code_review_message).flush()
         if _notify_listeners:
             notify(ObjectCreatedEvent(code_review_message))
         return code_review_message
@@ -1389,15 +1402,15 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
         if include_votes:
             votes = load_referencing(
                 CodeReviewVoteReference, branch_merge_proposals,
-                ['branch_merge_proposalID'])
+                ['branch_merge_proposal_id'])
             votes_map = defaultdict(list)
             for vote in votes:
-                votes_map[vote.branch_merge_proposalID].append(vote)
+                votes_map[vote.branch_merge_proposal_id].append(vote)
             for mp in branch_merge_proposals:
                 get_property_cache(mp).votes = votes_map[mp.id]
-            comments = load_related(CodeReviewComment, votes, ['commentID'])
-            load_related(Message, comments, ['messageID'])
-            person_ids.update(vote.reviewerID for vote in votes)
+            comments = load_related(CodeReviewComment, votes, ['comment_id'])
+            load_related(Message, comments, ['message_id'])
+            person_ids.update(vote.reviewer_id for vote in votes)
 
             # we also provide a summary of diffs, so load them
             load_related(LibraryFileAlias, diffs, ['diff_textID'])
@@ -1439,8 +1452,8 @@ class BranchMergeProposalGetter:
             BranchMergeProposal.registrantID == participant.id)
 
         review_select = Select(
-                [CodeReviewVoteReference.branch_merge_proposalID],
-                [CodeReviewVoteReference.reviewerID == participant.id])
+                [CodeReviewVoteReference.branch_merge_proposal_id],
+                [CodeReviewVoteReference.reviewer == participant])
 
         query = Store.of(participant).find(
             BranchMergeProposal,
@@ -1463,13 +1476,13 @@ class BranchMergeProposalGetter:
         # the actual vote for that person.
         tables = [
             CodeReviewVoteReference,
-            Join(Person, CodeReviewVoteReference.reviewerID == Person.id),
+            Join(Person, CodeReviewVoteReference.reviewer == Person.id),
             LeftJoin(
                 CodeReviewComment,
-                CodeReviewVoteReference.commentID == CodeReviewComment.id)]
+                CodeReviewVoteReference.comment == CodeReviewComment.id)]
         results = store.using(*tables).find(
             (CodeReviewVoteReference, Person, CodeReviewComment),
-            CodeReviewVoteReference.branch_merge_proposalID.is_in(ids))
+            CodeReviewVoteReference.branch_merge_proposal_id.is_in(ids))
         for reference, person, comment in results:
             result[reference.branch_merge_proposal].append(reference)
         return result
diff --git a/lib/lp/code/model/codereviewcomment.py b/lib/lp/code/model/codereviewcomment.py
index 47f640c..1029b4e 100644
--- a/lib/lp/code/model/codereviewcomment.py
+++ b/lib/lp/code/model/codereviewcomment.py
@@ -10,9 +10,11 @@ __all__ = [
 
 from textwrap import TextWrapper
 
-from sqlobject import (
-    ForeignKey,
-    StringCol,
+from storm.locals import (
+    Int,
+    Reference,
+    Store,
+    Unicode,
     )
 from zope.interface import implementer
 
@@ -22,8 +24,8 @@ from lp.code.interfaces.codereviewcomment import (
     ICodeReviewComment,
     ICodeReviewCommentDeletion,
     )
-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.mail.signedmessage import signed_message_from_string
 
 
@@ -60,17 +62,27 @@ def quote_text_as_email(text, width=80):
 
 
 @implementer(ICodeReviewComment, ICodeReviewCommentDeletion, IHasBranchTarget)
-class CodeReviewComment(SQLBase):
+class CodeReviewComment(StormBase):
     """A table linking branch merge proposals and messages."""
 
-    _table = 'CodeReviewMessage'
-
-    branch_merge_proposal = ForeignKey(
-        dbName='branch_merge_proposal', foreignKey='BranchMergeProposal',
-        notNull=True)
-    message = ForeignKey(dbName='message', foreignKey='Message', notNull=True)
-    vote = EnumCol(dbName='vote', notNull=False, schema=CodeReviewVote)
-    vote_tag = StringCol(default=None)
+    __storm_table__ = 'CodeReviewMessage'
+
+    id = Int(primary=True)
+    branch_merge_proposal_id = Int(
+        name='branch_merge_proposal', allow_none=False)
+    branch_merge_proposal = Reference(
+        branch_merge_proposal_id, 'BranchMergeProposal.id')
+    message_id = Int(name='message', allow_none=False)
+    message = Reference(message_id, 'Message.id')
+    vote = DBEnum(name='vote', allow_none=True, enum=CodeReviewVote)
+    vote_tag = Unicode(default=None)
+
+    def __init__(self, branch_merge_proposal, message, vote=None,
+                 vote_tag=None):
+        self.branch_merge_proposal = branch_merge_proposal
+        self.message = message
+        self.vote = vote
+        self.vote_tag = vote_tag
 
     @property
     def author(self):
@@ -134,3 +146,7 @@ class CodeReviewComment(SQLBase):
         return (
             self.branch_merge_proposal.userCanSetCommentVisibility(user) or
             (user is not None and user.inTeam(self.author)))
+
+    def destroySelf(self):
+        """Delete this comment."""
+        Store.of(self).remove(self)
diff --git a/lib/lp/code/model/codereviewvote.py b/lib/lp/code/model/codereviewvote.py
index d2e5c53..b695ebe 100644
--- a/lib/lp/code/model/codereviewvote.py
+++ b/lib/lp/code/model/codereviewvote.py
@@ -8,12 +8,15 @@ __all__ = [
     'CodeReviewVoteReference',
     ]
 
-from sqlobject import (
-    ForeignKey,
-    StringCol,
+import pytz
+from storm.locals import (
+    DateTime,
+    Int,
+    Reference,
+    Store,
+    Unicode,
     )
 from zope.interface import implementer
-from zope.schema import Int
 
 from lp.code.errors import (
     ClaimReviewFailed,
@@ -22,27 +25,36 @@ from lp.code.errors import (
     )
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.services.database.constants import DEFAULT
-from lp.services.database.datetimecol import UtcDateTimeCol
-from lp.services.database.sqlbase import SQLBase
+from lp.services.database.stormbase import StormBase
 
 
 @implementer(ICodeReviewVoteReference)
-class CodeReviewVoteReference(SQLBase):
+class CodeReviewVoteReference(StormBase):
     """See `ICodeReviewVote`"""
 
-    _table = 'CodeReviewVote'
-    id = Int()
-    branch_merge_proposal = ForeignKey(
-        dbName='branch_merge_proposal', foreignKey='BranchMergeProposal',
-        notNull=True)
-    date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
-    registrant = ForeignKey(
-        dbName='registrant', foreignKey='Person', notNull=True)
-    reviewer = ForeignKey(
-        dbName='reviewer', foreignKey='Person', notNull=True)
-    review_type = StringCol(default=None)
-    comment = ForeignKey(
-        dbName='vote_message', foreignKey='CodeReviewComment', default=None)
+    __storm_table__ = 'CodeReviewVote'
+
+    id = Int(primary=True)
+    branch_merge_proposal_id = Int(
+        name='branch_merge_proposal', allow_none=False)
+    branch_merge_proposal = Reference(
+        branch_merge_proposal_id, 'BranchMergeProposal.id')
+    date_created = DateTime(tzinfo=pytz.UTC, allow_none=False, default=DEFAULT)
+    registrant_id = Int(name='registrant', allow_none=False)
+    registrant = Reference(registrant_id, 'Person.id')
+    reviewer_id = Int(name='reviewer', allow_none=False)
+    reviewer = Reference(reviewer_id, 'Person.id')
+    review_type = Unicode(default=None)
+    comment_id = Int(name='vote_message', default=None)
+    comment = Reference(comment_id, 'CodeReviewComment.id')
+
+    def __init__(self, branch_merge_proposal, registrant, reviewer,
+                 review_type=None, date_created=DEFAULT):
+        self.branch_merge_proposal = branch_merge_proposal
+        self.registrant = registrant
+        self.reviewer = reviewer
+        self.review_type = review_type
+        self.date_created = date_created
 
     @property
     def is_pending(self):
@@ -96,6 +108,10 @@ class CodeReviewVoteReference(SQLBase):
         self.validateReasignReview(reviewer)
         self.reviewer = reviewer
 
+    def destroySelf(self):
+        """Delete this vote."""
+        Store.of(self).remove(self)
+
     def delete(self):
         """See `ICodeReviewVote`"""
         if not self.is_pending:
diff --git a/lib/lp/code/model/gitcollection.py b/lib/lp/code/model/gitcollection.py
index 4095d5e..6447d0b 100644
--- a/lib/lp/code/model/gitcollection.py
+++ b/lib/lp/code/model/gitcollection.py
@@ -412,10 +412,10 @@ class GenericGitCollection:
         tables = [
             BranchMergeProposal,
             Join(CodeReviewVoteReference,
-                 CodeReviewVoteReference.branch_merge_proposalID == \
+                 CodeReviewVoteReference.branch_merge_proposal ==
                  BranchMergeProposal.id),
             LeftJoin(CodeReviewComment,
-                 CodeReviewVoteReference.commentID == CodeReviewComment.id)]
+                 CodeReviewVoteReference.comment == CodeReviewComment.id)]
 
         expressions = [
             CodeReviewVoteReference.reviewer == reviewer,
diff --git a/lib/lp/code/model/tests/test_branch.py b/lib/lp/code/model/tests/test_branch.py
index 1978e88..e2421ba 100644
--- a/lib/lp/code/model/tests/test_branch.py
+++ b/lib/lp/code/model/tests/test_branch.py
@@ -1566,8 +1566,8 @@ class TestBranchDeletionConsequences(TestCase):
         comment_id = comment.id
         branch = comment.branch_merge_proposal.source_branch
         branch.destroySelf(break_references=True)
-        self.assertRaises(
-            SQLObjectNotFound, CodeReviewComment.get, comment_id)
+        self.assertIsNone(
+            IStore(CodeReviewComment).get(CodeReviewComment, comment_id))
 
     def test_deleteTargetCodeReviewComment(self):
         """Deletion of branches that have CodeReviewComments works."""
@@ -1575,8 +1575,8 @@ class TestBranchDeletionConsequences(TestCase):
         comment_id = comment.id
         branch = comment.branch_merge_proposal.target_branch
         branch.destroySelf(break_references=True)
-        self.assertRaises(
-            SQLObjectNotFound, CodeReviewComment.get, comment_id)
+        self.assertIsNone(
+            IStore(CodeReviewComment).get(CodeReviewComment, comment_id))
 
     def test_branchWithBugRequirements(self):
         """Deletion requirements for a branch with a bug are right."""
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index a8b9c35..62bf865 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -1086,8 +1086,8 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
         comment_id = comment.id
         repository = comment.branch_merge_proposal.source_git_repository
         repository.destroySelf(break_references=True)
-        self.assertRaises(
-            SQLObjectNotFound, CodeReviewComment.get, comment_id)
+        self.assertIsNone(
+            IStore(CodeReviewComment).get(CodeReviewComment, comment_id))
 
     def test_delete_target_CodeReviewComment(self):
         # Deletion of target repositories that have CodeReviewComments works.
@@ -1095,8 +1095,8 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
         comment_id = comment.id
         repository = comment.branch_merge_proposal.target_git_repository
         repository.destroySelf(break_references=True)
-        self.assertRaises(
-            SQLObjectNotFound, CodeReviewComment.get, comment_id)
+        self.assertIsNone(
+            IStore(CodeReviewComment).get(CodeReviewComment, comment_id))
 
     def test_sourceBranchWithCodeReviewVoteReference(self):
         # break_references handles CodeReviewVoteReference source repository.
diff --git a/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
index 1580ea4..98483a4 100644
--- a/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
+++ b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
@@ -463,7 +463,7 @@ which is the one we want the method to return.
     ...     product=blob, set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
     ...     registrant=branch_owner, source_branch=source_branch)
     >>> proposal.nominateReviewer(target_owner, branch_owner)
-    <CodeReviewVoteReference at ...>
+    <lp.code.model.codereviewvote.CodeReviewVoteReference object at ...>
 
 And then we propose a merge the other way, so that the owner is target,
 but they have not been asked to review, meaning that the method shouldn't
@@ -474,7 +474,7 @@ return this review.
     ...     product=blob, set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
     ...     registrant=target_owner, source_branch=target_branch)
     >>> proposal.nominateReviewer(branch_owner, target_owner)
-    <CodeReviewVoteReference at ...>
+    <lp.code.model.codereviewvote.CodeReviewVoteReference object at ...>
     >>> logout()
 
     >>> proposals = webservice.named_get('/~target', 'getRequestedReviews'
diff --git a/lib/lp/services/mail/helpers.py b/lib/lp/services/mail/helpers.py
index 82640aa..80f68d3 100644
--- a/lib/lp/services/mail/helpers.py
+++ b/lib/lp/services/mail/helpers.py
@@ -35,7 +35,13 @@ class IncomingEmailError(Exception):
 
 
 def get_main_body(signed_msg):
-    """Returns the first text part of the email."""
+    """Returns the first text part of the email.
+
+    This always returns text (or None if the email has no text parts at
+    all).  It decodes using the character set in the text part's
+    Content-Type, or ISO-8859-1 if unspecified (in order to minimise the
+    chances of `UnicodeDecodeError`s).
+    """
     msg = getattr(signed_msg, 'signedMessage', None)
     if msg is None:
         # The email wasn't signed.
@@ -43,9 +49,11 @@ def get_main_body(signed_msg):
     if msg.is_multipart():
         for part in msg.walk():
             if part.get_content_type() == 'text/plain':
-                return part.get_payload(decode=True)
+                charset = part.get_content_charset('ISO-8859-1')
+                return part.get_payload(decode=True).decode(charset)
     else:
-        return msg.get_payload(decode=True)
+        charset = msg.get_content_charset('ISO-8859-1')
+        return msg.get_payload(decode=True).decode(charset)
 
 
 def guess_bugtask(bug, person):