launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25273
[Merge] ~cjwatson/launchpad:stormify-codereview into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:stormify-codereview into launchpad:master.
Commit message:
Convert CodeReviewComment and CodeReviewVoteReference to Storm
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/390667
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:stormify-codereview into launchpad:master.
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):
