launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27001
[Merge] ~pappacena/launchpad:comment-editing-api into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:comment-editing-api into launchpad:master with ~pappacena/launchpad:comment-editing-model as a prerequisite.
Commit message:
API to edit and delete comments for bug messages, answers and code review comments
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/402211
The API for the message revision history (listing and deleting old revisions) will be done in a future MP, in order to keep the review shorter.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:comment-editing-api into launchpad:master.
diff --git a/lib/lp/answers/browser/configure.zcml b/lib/lp/answers/browser/configure.zcml
index 46c7553..631e8b8 100644
--- a/lib/lp/answers/browser/configure.zcml
+++ b/lib/lp/answers/browser/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2021 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -278,6 +278,10 @@
module=".question"
classes="QuestionSetNavigation"
/>
+ <browser:navigation
+ module=".question"
+ classes="QuestionNavigation"
+ />
<browser:url
for="lp.answers.interfaces.questioncollection.IQuestionSet"
diff --git a/lib/lp/answers/browser/question.py b/lib/lp/answers/browser/question.py
index b039552..757da13 100644
--- a/lib/lp/answers/browser/question.py
+++ b/lib/lp/answers/browser/question.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Question views."""
@@ -109,6 +109,7 @@ from lp.services.webapp import (
Link,
Navigation,
NavigationMenu,
+ stepthrough,
)
from lp.services.webapp.authorization import check_permission
from lp.services.webapp.breadcrumb import Breadcrumb
@@ -257,6 +258,17 @@ class QuestionSetNavigation(Navigation):
canonical_url(question, self.request), status=301)
+class QuestionNavigation(Navigation):
+ """Navigation for the IQuestion."""
+
+ usedfor = IQuestion
+
+ @stepthrough('messages')
+ def traverse_comments(self, index):
+ index = int(index) - 1
+ return self.context.messages[index]
+
+
class QuestionBreadcrumb(Breadcrumb):
"""Builds a breadcrumb for an `IQuestion`."""
diff --git a/lib/lp/answers/configure.zcml b/lib/lp/answers/configure.zcml
index 9735be8..a7eefcd 100644
--- a/lib/lp/answers/configure.zcml
+++ b/lib/lp/answers/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2021 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -142,7 +142,9 @@
<class class=".model.questionmessage.QuestionMessage">
- <allow interface=".interfaces.questionmessage.IQuestionMessage"/>
+ <require permission="launchpad.Edit"
+ interface="lp.services.messages.interfaces.message.IMessageEdit" />
+ <allow interface=".interfaces.questionmessage.IQuestionMessageView"/>
</class>
<adapter
diff --git a/lib/lp/answers/interfaces/questionmessage.py b/lib/lp/answers/interfaces/questionmessage.py
index 176ddf8..dadd1d6 100644
--- a/lib/lp/answers/interfaces/questionmessage.py
+++ b/lib/lp/answers/interfaces/questionmessage.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Question message interface."""
@@ -26,15 +26,14 @@ from lp.answers.enums import (
QuestionAction,
QuestionStatus,
)
-from lp.services.messages.interfaces.message import IMessage
-
+from lp.services.messages.interfaces.message import (
+ IMessage,
+ IMessageView,
+ )
-@exported_as_webservice_entry(as_of='devel')
-class IQuestionMessage(IMessage):
- """A message part of a question.
- It adds attributes to the IMessage interface.
- """
+class IQuestionMessageView(IMessageView):
+ """Publicly visible attributes of a message part of a question."""
# This is really an Object field with schema=IQuestion, but that
# would create a circular dependency between IQuestion
# and IQuestionMessage
@@ -69,3 +68,8 @@ class IQuestionMessage(IMessage):
description=_("Whether or not the message is visible."),
readonly=True),
as_of="devel")
+
+
+@exported_as_webservice_entry(as_of='devel')
+class IQuestionMessage(IQuestionMessageView, IMessage):
+ """A message part of a question."""
diff --git a/lib/lp/answers/model/questionmessage.py b/lib/lp/answers/model/questionmessage.py
index 3acf257..3fe7c56 100644
--- a/lib/lp/answers/model/questionmessage.py
+++ b/lib/lp/answers/model/questionmessage.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""SQLBase implementation of IQuestionMessage."""
diff --git a/lib/lp/answers/stories/webservice.txt b/lib/lp/answers/stories/webservice.txt
index 852e915..eb01af6 100644
--- a/lib/lp/answers/stories/webservice.txt
+++ b/lib/lp/answers/stories/webservice.txt
@@ -234,6 +234,8 @@ that indicate how the message changed the question.
bug_attachments_collection_link: '...'
content: 'This is the answer'
date_created: '20...+00:00'
+ date_deleted: None
+ date_last_edit: None
index: 1
new_status: 'Answered'
owner_link: 'http://api.launchpad.test/devel/~contact'
diff --git a/lib/lp/answers/tests/test_question_workflow.py b/lib/lp/answers/tests/test_question_workflow.py
index 6450d61..66fa81b 100644
--- a/lib/lp/answers/tests/test_question_workflow.py
+++ b/lib/lp/answers/tests/test_question_workflow.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test the question workflow methods.
@@ -53,6 +53,7 @@ from lp.testing import (
ANONYMOUS,
login,
login_person,
+ person_logged_in,
TestCase,
)
from lp.testing.fixture import ZopeEventHandlerFixture
@@ -238,7 +239,8 @@ class BaseAnswerTrackerWorkflowTestCase(TestCase):
It also verifies that the question status, datelastquery (or
datelastresponse) were updated to reflect the time of the message.
"""
- self.assertTrue(verifyObject(IQuestionMessage, message))
+ with person_logged_in(message.owner):
+ self.assertTrue(verifyObject(IQuestionMessage, message))
self.assertEqual("Re: Help!", message.subject)
self.assertEqual(expected_owner, message.owner)
diff --git a/lib/lp/bugs/browser/bugcomment.py b/lib/lp/bugs/browser/bugcomment.py
index 2b61552..e40967f 100644
--- a/lib/lp/bugs/browser/bugcomment.py
+++ b/lib/lp/bugs/browser/bugcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2006-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2006-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Bug comment browser view classes."""
@@ -72,7 +72,10 @@ def build_comments_from_chunks(
cache = get_property_cache(message)
if getattr(cache, 'chunks', None) is None:
cache.chunks = []
- cache.chunks.append(removeSecurityProxy(chunk))
+ # Soft-deleted messages will have None chunk here. Skip cache
+ # filling in this case.
+ if chunk is not None:
+ cache.chunks.append(removeSecurityProxy(chunk))
bug_comment = comments.get(message.id)
if bug_comment is None:
if bugmessage.index == 0 and hide_first:
diff --git a/lib/lp/bugs/configure.zcml b/lib/lp/bugs/configure.zcml
index 73b99e8..b2a54ac 100644
--- a/lib/lp/bugs/configure.zcml
+++ b/lib/lp/bugs/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2021 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -773,7 +773,9 @@
<class
class="lp.bugs.model.bugmessage.BugMessage">
<allow
- interface="lp.bugs.interfaces.bugmessage.IBugMessage"/>
+ interface="lp.bugs.interfaces.bugmessage.IBugMessageView"/>
+ <require permission="launchpad.Edit"
+ interface="lp.services.messages.interfaces.message.IMessageEdit" />
<require
permission="launchpad.InternalScriptsOnly"
set_attributes="remote_comment_id bugwatch index"/>
diff --git a/lib/lp/bugs/interfaces/bugmessage.py b/lib/lp/bugs/interfaces/bugmessage.py
index 98c18f3..15b3373 100644
--- a/lib/lp/bugs/interfaces/bugmessage.py
+++ b/lib/lp/bugs/interfaces/bugmessage.py
@@ -1,4 +1,4 @@
-# Copyright 2004-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2004-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Bug message interfaces."""
@@ -31,11 +31,14 @@ from lp.bugs.interfaces.hasbug import IHasBug
from lp.registry.interfaces.person import IPerson
from lp.services.comments.interfaces.conversation import IComment
from lp.services.fields import Title
-from lp.services.messages.interfaces.message import IMessage
+from lp.services.messages.interfaces.message import (
+ IMessage,
+ IMessageView,
+ )
-class IBugMessage(IHasBug):
- """A link between a bug and a message."""
+class IBugMessageView(IMessageView, IHasBug):
+ """Public attributes for a link between a bug and a message."""
bug = Object(schema=IBug, title=u"The bug.")
# The index field is being populated in the DB; once complete it will be
@@ -57,6 +60,10 @@ class IBugMessage(IHasBug):
title=u"The Message owner mirrored from the message.", readonly=True)
+class IBugMessage(IBugMessageView, IMessage):
+ """A link between a bug and a message."""
+
+
class IBugMessageSet(Interface):
"""The set of all IBugMessages."""
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index 8f1480c..c1cc4c8 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -1577,8 +1577,12 @@ class Bug(SQLBase, InformationTypeMixin):
# We expect:
# 1 bugmessage -> 1 message -> small N chunks. For now, using a wide
# query seems fine as we have to join out from bugmessage anyway.
- result = Store.of(self).find((BugMessage, Message, MessageChunk),
- Message.id == MessageChunk.messageID,
+ # Since "soft-deleted" messages will have 0 chunks, we should use
+ # left join here.
+ message_join = LeftJoin(
+ Message, MessageChunk, Message.id == MessageChunk.messageID)
+ query = Store.of(self).using(BugMessage, message_join)
+ result = query.find((BugMessage, Message, MessageChunk),
BugMessage.message_id == Message.id,
BugMessage.bug == self.id, *ranges)
result.order_by(BugMessage.index, MessageChunk.sequence)
diff --git a/lib/lp/bugs/model/bugmessage.py b/lib/lp/bugs/model/bugmessage.py
index 97c3015..b8e7291 100644
--- a/lib/lp/bugs/model/bugmessage.py
+++ b/lib/lp/bugs/model/bugmessage.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -6,6 +6,7 @@ __all__ = ['BugMessage', 'BugMessageSet']
from email.utils import make_msgid
+from lazr.delegates import delegate_to
import six
from storm.properties import (
Int,
@@ -22,6 +23,7 @@ from lp.bugs.interfaces.bugmessage import (
from lp.registry.interfaces.person import validate_public_person
from lp.services.database.interfaces import IStore
from lp.services.database.stormbase import StormBase
+from lp.services.messages.interfaces.message import IMessage
from lp.services.messages.model.message import (
Message,
MessageChunk,
@@ -29,6 +31,7 @@ from lp.services.messages.model.message import (
@implementer(IBugMessage)
+@delegate_to(IMessage, context='message')
class BugMessage(StormBase):
"""A table linking bugs and messages."""
diff --git a/lib/lp/bugs/stories/webservice/xx-bug.txt b/lib/lp/bugs/stories/webservice/xx-bug.txt
index 6121737..0c43c88 100644
--- a/lib/lp/bugs/stories/webservice/xx-bug.txt
+++ b/lib/lp/bugs/stories/webservice/xx-bug.txt
@@ -234,6 +234,8 @@ Each bug has a collection of messages.
'http://.../firefox/+bug/5/comments/0/bug_attachments'
content: 'All ways of downloading firefox should provide...'
date_created: '2005-01-14T17:27:03.702622+00:00'
+ date_deleted: None
+ date_last_edit: None
owner_link: 'http://.../~name12'
parent_link: None
resource_type_link: 'http://.../#message'
@@ -1347,6 +1349,8 @@ attachment. This is where our comment is recorded.
bug_attachments_collection_link: 'http://.../firefox/+bug/1/comments/2/bug_attachments'
content: 'The numbers you asked for.'
date_created: '...'
+ date_deleted: None
+ date_last_edit: None
owner_link: 'http://.../~salgado'
parent_link: None
resource_type_link: 'http://.../#message'
diff --git a/lib/lp/code/browser/codereviewcomment.py b/lib/lp/code/browser/codereviewcomment.py
index 24af2f8..0d0496d 100644
--- a/lib/lp/code/browser/codereviewcomment.py
+++ b/lib/lp/code/browser/codereviewcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -23,7 +23,10 @@ from zope.interface import (
implementer,
Interface,
)
-from zope.schema import Text
+from zope.schema import (
+ Object,
+ Text,
+ )
from lp import _
from lp.app.browser.launchpadform import (
@@ -40,6 +43,10 @@ from lp.services.comments.browser.messagecomment import MessageComment
from lp.services.comments.interfaces.conversation import IComment
from lp.services.config import config
from lp.services.librarian.interfaces import ILibraryFileAlias
+from lp.services.messages.interfaces.message import (
+ IMessage,
+ IMessageEdit,
+ )
from lp.services.propertycache import (
cachedproperty,
get_property_cache,
@@ -55,10 +62,12 @@ from lp.services.webapp.interfaces import ILaunchBag
class ICodeReviewDisplayComment(IComment, ICodeReviewComment):
"""Marker interface for displaying code review comments."""
+ message = Object(schema=IMessage, title=_('The message.'))
@implementer(ICodeReviewDisplayComment)
@delegate_to(ICodeReviewComment, context='comment')
+@delegate_to(IMessageEdit, context='message')
class CodeReviewDisplayComment(MessageComment):
"""A code review comment or activity or both.
diff --git a/lib/lp/code/browser/tests/test_codereviewcomment.py b/lib/lp/code/browser/tests/test_codereviewcomment.py
index 799312b..e74e310 100644
--- a/lib/lp/code/browser/tests/test_codereviewcomment.py
+++ b/lib/lp/code/browser/tests/test_codereviewcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for CodeReviewComments."""
@@ -56,7 +56,8 @@ class TestCodeReviewComments(TestCaseWithFactory):
display_comment = CodeReviewDisplayComment(comment)
- verifyObject(ICodeReviewDisplayComment, display_comment)
+ with person_logged_in(comment.owner):
+ verifyObject(ICodeReviewDisplayComment, display_comment)
def test_extra_css_classes_visibility(self):
author = self.factory.makePerson()
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index 61c18c1..a373286 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2021 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -548,7 +548,9 @@
<!-- CodeReviewComment -->
<class class="lp.code.model.codereviewcomment.CodeReviewComment">
- <allow interface="lp.code.interfaces.codereviewcomment.ICodeReviewComment"/>
+ <require permission="launchpad.Edit"
+ interface="lp.services.messages.interfaces.message.IMessageEdit" />
+ <allow interface="lp.code.interfaces.codereviewcomment.ICodeReviewCommentView"/>
<allow interface="lp.code.interfaces.branchtarget.IHasBranchTarget"/>
</class>
<subscriber
diff --git a/lib/lp/code/interfaces/codereviewcomment.py b/lib/lp/code/interfaces/codereviewcomment.py
index f17e9a9..63fb51e 100644
--- a/lib/lp/code/interfaces/codereviewcomment.py
+++ b/lib/lp/code/interfaces/codereviewcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""CodeReviewComment interfaces."""
@@ -28,12 +28,15 @@ from lp import _
from lp.code.enums import CodeReviewVote
from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
from lp.registry.interfaces.person import IPerson
-from lp.services.messages.interfaces.message import IMessage
+from lp.services.messages.interfaces.message import (
+ IMessage,
+ IMessageCommon,
+ IMessageEdit,
+ )
-@exported_as_webservice_entry()
-class ICodeReviewComment(Interface):
- """A link between a merge proposal and a message."""
+class ICodeReviewCommentView(IMessageCommon):
+ """Globally visible attributes of ICodeReviewComment."""
id = exported(
Int(
@@ -99,6 +102,11 @@ class ICodeReviewComment(Interface):
"""
+@exported_as_webservice_entry()
+class ICodeReviewComment(ICodeReviewCommentView, IMessageEdit):
+ """A link between a merge proposal and a message."""
+
+
class ICodeReviewCommentDeletion(Interface):
"""This interface provides deletion of CodeReviewComments.
diff --git a/lib/lp/code/model/codereviewcomment.py b/lib/lp/code/model/codereviewcomment.py
index 68b9d78..53e8a84 100644
--- a/lib/lp/code/model/codereviewcomment.py
+++ b/lib/lp/code/model/codereviewcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The database implementation class for CodeReviewComment."""
@@ -10,6 +10,7 @@ __all__ = [
from textwrap import TextWrapper
+from lazr.delegates import delegate_to
from storm.locals import (
Int,
Reference,
@@ -27,6 +28,10 @@ from lp.code.interfaces.codereviewcomment import (
from lp.services.database.enumcol import DBEnum
from lp.services.database.stormbase import StormBase
from lp.services.mail.signedmessage import signed_message_from_bytes
+from lp.services.messages.interfaces.message import (
+ IMessageCommon,
+ IMessageEdit,
+ )
def quote_text_as_email(text, width=80):
@@ -61,7 +66,9 @@ def quote_text_as_email(text, width=80):
return '\n'.join(result)
-@implementer(ICodeReviewComment, ICodeReviewCommentDeletion, IHasBranchTarget)
+@implementer(ICodeReviewComment, ICodeReviewCommentDeletion, IHasBranchTarget,
+ IMessageCommon, IMessageEdit)
+@delegate_to(IMessageCommon, IMessageEdit, context='message')
class CodeReviewComment(StormBase):
"""A table linking branch merge proposals and messages."""
diff --git a/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
index 32f0373..8732de8 100644
--- a/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
+++ b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
@@ -198,9 +198,11 @@ The comments on a branch merge proposal are exposed through the API.
as_quoted_email: '> This is great work'
author_link: 'http://api.launchpad.test/devel/~...'
branch_merge_proposal_link: 'http://.../~source/fooix/fix-it/+merge/...'
+ content: 'This is great work'
date_created: '...'
id: ...
message_body: 'This is great work'
+ owner_link: 'http://...'
resource_type_link: 'http://.../#code_review_comment'
self_link: 'http://.../~source/fooix/fix-it/+merge/.../comments/...'
title: 'Comment on proposed merge of lp://dev/~source/fooix/fix-it into lp://dev/~target/fooix/trunk'
@@ -216,9 +218,11 @@ The comments on a branch merge proposal are exposed through the API.
as_quoted_email: '> This is mediocre work.'
author_link: 'http://api.launchpad.test/devel/~...'
branch_merge_proposal_link: 'http://.../~source/fooix/fix-it/+merge/...'
+ content: 'This is mediocre work.'
date_created: '...'
id: ...
message_body: 'This is mediocre work.'
+ owner_link: 'http://...'
resource_type_link: 'http://.../#code_review_comment'
self_link: 'http://.../~source/fooix/fix-it/+merge/.../comments/...'
title: ...
@@ -290,9 +294,11 @@ Now the code review should be made.
as_quoted_email: '> This is great work'
author_link: 'http://api.launchpad.test/devel/~...'
branch_merge_proposal_link: 'http://.../~source/fooix/fix-it/+merge/...'
+ content: 'This is great work'
date_created: '...'
id: ...
message_body: 'This is great work'
+ owner_link: 'http://...'
resource_type_link: 'http://.../#code_review_comment'
self_link: 'http://.../~source/fooix/fix-it/+merge/.../comments/...'
title: ...
diff --git a/lib/lp/services/messages/browser/message.py b/lib/lp/services/messages/browser/message.py
index 3b5a3c4..06800e6 100644
--- a/lib/lp/services/messages/browser/message.py
+++ b/lib/lp/services/messages/browser/message.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Message related view classes."""
@@ -18,7 +18,7 @@ class QuestionMessageCanonicalUrlData:
def __init__(self, question, message):
self.inside = question
- self.path = "messages/%d" % list(question.messages).index(message)
+ self.path = "messages/%d" % message.display_index
@implementer(ICanonicalUrlData)
@@ -45,14 +45,28 @@ class IndexedBugMessageCanonicalUrlData:
self.path = "comments/%d" % message.index
+@implementer(ICanonicalUrlData)
+class CodeReviewCommentCanonicalUrlData:
+ """An optimized bug message canonical_url implementation.
+ """
+ rootsite = 'code'
+
+ def __init__(self, message):
+ self.inside = message.branch_merge_proposal
+ self.path = "comments/%d" % message.id
+
+
def message_to_canonical_url_data(message):
"""This factory creates `ICanonicalUrlData` for Message."""
# Circular imports
from lp.answers.interfaces.questionmessage import IQuestionMessage
+ from lp.code.interfaces.codereviewcomment import ICodeReviewComment
if IIndexedMessage.providedBy(message):
return IndexedBugMessageCanonicalUrlData(message)
elif IQuestionMessage.providedBy(message):
return QuestionMessageCanonicalUrlData(message.question, message)
+ elif ICodeReviewComment.providedBy(message):
+ return CodeReviewCommentCanonicalUrlData(message)
else:
if message.bugs.count() == 0:
# Will result in a ComponentLookupError
diff --git a/lib/lp/services/messages/interfaces/message.py b/lib/lp/services/messages/interfaces/message.py
index 32d64a6..ab31077 100644
--- a/lib/lp/services/messages/interfaces/message.py
+++ b/lib/lp/services/messages/interfaces/message.py
@@ -8,7 +8,10 @@ __all__ = [
'IIndexedMessage',
'IMessage',
'IMessageChunk',
+ 'IMessageCommon',
+ 'IMessageEdit',
'IMessageSet',
+ 'IMessageView',
'IUserToUserEmail',
'IndexedMessage',
'InvalidEmailMessage',
@@ -21,9 +24,11 @@ from lazr.delegates import delegate_to
from lazr.restful.declarations import (
accessor_for,
export_read_operation,
+ export_write_operation,
exported,
exported_as_webservice_entry,
operation_for_version,
+ operation_parameters,
)
from lazr.restful.fields import (
CollectionField,
@@ -51,44 +56,61 @@ from lp.services.webservice.apihelpers import patch_reference_property
class IMessageEdit(Interface):
+ @export_write_operation()
+ @operation_parameters(
+ new_content=TextLine(
+ title=_("Message content"),
+ description=_("The new message content string"),
+ required=True))
+ @operation_for_version("devel")
def editContent(new_content):
"""Edit the content of this message, generating a new message
revision with the old content.
"""
+ @export_write_operation()
+ @operation_for_version("devel")
def deleteContent():
"""Deletes this message content."""
-class IMessageView(Interface):
- """Public attributes for message.
-
- This is like an email (RFC822) message, though it could be created through
- the web as well.
- """
+class IMessageCommon(Interface):
+ """Common public attributes for every IMessage implementation."""
id = Int(title=_('ID'), required=True, readonly=True)
+
+ chunks = Attribute(_('Message pieces'))
+ text_contents = exported(
+ Text(title=_('All the text/plain chunks joined together as a '
+ 'unicode string.')),
+ exported_as='content')
+ owner = exported(
+ Reference(title=_('Person'), schema=Interface,
+ required=False, readonly=True))
+
+ revisions = Attribute(_('Message revision history'))
datecreated = exported(
Datetime(title=_('Date Created'), required=True, readonly=True),
exported_as='date_created')
-
- date_last_edit = Datetime(
+ date_last_edit = exported(Datetime(
title=_('When this message was last edited'), required=False,
- readonly=True)
-
- date_deleted = Datetime(
+ readonly=True))
+ date_deleted = exported(Datetime(
title=_('When this message was deleted'), required=False,
- readonly=True)
+ readonly=True))
+
+
+class IMessageView(IMessageCommon):
+ """Public attributes for message.
+
+ This is like an email (RFC822) message, though it could be created through
+ the web as well.
+ """
subject = exported(
TextLine(title=_('Subject'), required=True, readonly=True))
- # XXX flacoste 2006-09-08: This attribute is only used for the
- # add form used by MessageAddView.
content = Text(title=_("Message"), required=True, readonly=True)
- owner = exported(
- Reference(title=_('Person'), schema=Interface,
- required=False, readonly=True))
# Schema is really IMessage, but this cannot be declared here. It's
# fixed below after the IMessage definition is complete.
@@ -104,15 +126,6 @@ class IMessageView(Interface):
title=_('Bug List'),
value_type=Reference(schema=Interface)) # Redefined in bug.py
- chunks = Attribute(_('Message pieces'))
-
- revisions = Attribute(_('Message revision history'))
-
- text_contents = exported(
- Text(title=_('All the text/plain chunks joined together as a '
- 'unicode string.')),
- exported_as='content')
-
title = TextLine(
title=_('The message title, usually just the subject.'),
readonly=True)
diff --git a/lib/lp/services/messages/model/message.py b/lib/lp/services/messages/model/message.py
index 0f3d884..db2ce4f 100644
--- a/lib/lp/services/messages/model/message.py
+++ b/lib/lp/services/messages/model/message.py
@@ -205,12 +205,13 @@ class Message(SQLBase):
del get_property_cache(self).text_contents
del get_property_cache(self).chunks
del get_property_cache(self).revisions
- return rev
def deleteContent(self):
"""See `IMessage`."""
for chunk in self._chunks:
chunk.destroySelf()
+ del get_property_cache(self).chunks
+ del get_property_cache(self).text_contents
self.date_deleted = utc_now()
diff --git a/lib/lp/services/messages/tests/test_message.py b/lib/lp/services/messages/tests/test_message.py
index 83384c8..ea92703 100644
--- a/lib/lp/services/messages/tests/test_message.py
+++ b/lib/lp/services/messages/tests/test_message.py
@@ -13,6 +13,7 @@ from email.utils import (
)
import six
+from testscenarios import WithScenarios
from testtools.matchers import (
Equals,
Is,
@@ -20,13 +21,23 @@ from testtools.matchers import (
)
import transaction
from zope.security.interfaces import Unauthorized
-from zope.security.proxy import ProxyFactory
+from zope.security.proxy import (
+ ProxyFactory,
+ removeSecurityProxy,
+ )
+from lp.bugs.interfaces.bugmessage import IBugMessage
+from lp.bugs.model.bugmessage import BugMessage
from lp.services.compat import message_as_bytes
+from lp.services.database.interfaces import IStore
from lp.services.messages.model.message import MessageSet
from lp.services.utils import utc_now
+from lp.services.webapp.interfaces import OAuthPermission
from lp.testing import (
+ admin_logged_in,
+ api_url,
login,
+ login_person,
person_logged_in,
TestCaseWithFactory,
)
@@ -34,6 +45,7 @@ from lp.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
)
+from lp.testing.pages import webservice_for_person
class TestMessageSet(TestCaseWithFactory):
@@ -183,16 +195,49 @@ class TestMessageSet(TestCaseWithFactory):
self.assertEqual(self.high_characters.decode('latin-1'), result)
-class TestMessageEditing(TestCaseWithFactory):
+class MessageTypeScenariosMixin(WithScenarios):
+
+ scenarios = [
+ ("bug", {"message_type": "bug"}),
+ ("question", {"message_type": "question"}),
+ ("MP comment", {"message_type": "mp"})
+ ]
+
+ def setUp(self):
+ super(MessageTypeScenariosMixin, self).setUp()
+ self.person = self.factory.makePerson()
+ login_person(self.person)
+
+ def makeMessage(self, content=None, **kwargs):
+ owner = kwargs.pop('owner', self.person)
+ if self.message_type == "bug":
+ msg = self.factory.makeBugComment(
+ owner=owner, body=content, **kwargs)
+ return ProxyFactory(IStore(BugMessage).find(
+ BugMessage, BugMessage.message == msg).one())
+ elif self.message_type == "question":
+ question = self.factory.makeQuestion()
+ return question.giveAnswer(owner, content)
+ elif self.message_type == "mp":
+ return self.factory.makeCodeReviewComment(
+ sender=owner, body=content)
+
+
+class TestMessageEditing(MessageTypeScenariosMixin, TestCaseWithFactory):
"""Test editing scenarios for Message objects."""
layer = DatabaseFunctionalLayer
- def makeMessage(self, owner=None, content=None):
- if owner is None:
- owner = self.factory.makePerson()
- msg = self.factory.makeMessage(owner=owner, content=content)
- return ProxyFactory(msg)
+ def assertIsMessageHistory(
+ self, msg_history, msg, created_at, content, deleted_at=None):
+ """Asserts that `msg_history` is a message a message history of
+ `msg` with the given extra info.
+ """
+ self.assertThat(msg_history, MatchesStructure(
+ content=Equals(content),
+ message=Equals(removeSecurityProxy(msg).message),
+ date_created=Equals(created_at),
+ date_deleted=Is(deleted_at)))
def test_non_owner_cannot_edit_message(self):
msg = self.makeMessage()
@@ -207,11 +252,9 @@ class TestMessageEditing(TestCaseWithFactory):
msg.editContent("This is the new content")
self.assertEqual("This is the new content", msg.text_contents)
self.assertEqual(1, len(msg.revisions))
- self.assertThat(msg.revisions[0], MatchesStructure(
- content=Equals("initial content"),
- message=Equals(msg),
- date_created=Equals(msg.datecreated),
- date_deleted=Is(None)))
+ self.assertIsMessageHistory(
+ msg.revisions[0], msg,
+ created_at=msg.datecreated, content="initial content")
def test_multiple_edits_revisions(self):
owner = self.factory.makePerson()
@@ -221,26 +264,20 @@ class TestMessageEditing(TestCaseWithFactory):
first_edit_date = msg.date_last_edit
self.assertEqual("first edit", msg.text_contents)
self.assertEqual(1, len(msg.revisions))
- self.assertThat(msg.revisions[0], MatchesStructure(
- content=Equals("initial content"),
- message=Equals(msg),
- date_created=Equals(msg.datecreated),
- date_deleted=Is(None)))
+ self.assertIsMessageHistory(
+ msg.revisions[0], msg,
+ content="initial content", created_at=msg.datecreated)
with person_logged_in(owner):
msg.editContent("final form")
self.assertEqual("final form", msg.text_contents)
self.assertEqual(2, len(msg.revisions))
- self.assertThat(msg.revisions[0], MatchesStructure(
- content=Equals("first edit"),
- message=Equals(msg),
- date_created=Equals(first_edit_date),
- date_deleted=Is(None)))
- self.assertThat(msg.revisions[1], MatchesStructure(
- content=Equals("initial content"),
- message=Equals(msg),
- date_created=Equals(msg.datecreated),
- date_deleted=Is(None)))
+ self.assertIsMessageHistory(
+ msg.revisions[0], msg,
+ content="first edit", created_at=first_edit_date)
+ self.assertIsMessageHistory(
+ msg.revisions[1], msg,
+ content="initial content", created_at=msg.datecreated)
def test_non_owner_cannot_delete_message(self):
owner = self.factory.makePerson()
@@ -260,3 +297,73 @@ class TestMessageEditing(TestCaseWithFactory):
self.assertEqual(0, len(msg.chunks))
self.assertIsNotNone(msg.date_deleted)
self.assertTrue(after_delete > msg.date_deleted > before_delete)
+
+
+class TestMessageEditingAPI(MessageTypeScenariosMixin, TestCaseWithFactory):
+ """Test editing scenarios for Message editing API."""
+
+ layer = DatabaseFunctionalLayer
+
+ def getWebservice(self, person):
+ return webservice_for_person(
+ person, permission=OAuthPermission.WRITE_PUBLIC,
+ default_api_version="devel")
+
+ def getMessageAPIURL(self, msg):
+ with admin_logged_in():
+ if IBugMessage.providedBy(msg):
+ # BugMessage has a special URL mapping that uses the
+ # IMessage object itself.
+ return api_url(msg.message)
+ else:
+ return api_url(msg)
+
+ def test_edit_message(self):
+ msg = self.makeMessage(content="initial content")
+ ws = self.getWebservice(self.person)
+ url = self.getMessageAPIURL(msg)
+ response = ws.named_post(
+ url, 'editContent', new_content="the new content")
+ self.assertEqual(200, response.status)
+
+ edited_obj = ws.get(url).jsonBody()
+ self.assertEqual("the new content", edited_obj['content'])
+ self.assertIsNone(edited_obj["date_deleted"])
+ self.assertIsNotNone(edited_obj["date_last_edit"])
+
+ def test_edit_message_permission_denied_for_non_owner(self):
+ msg = self.makeMessage(content="initial content")
+ ws = self.getWebservice(self.factory.makePerson())
+ url = self.getMessageAPIURL(msg)
+ response = ws.named_post(
+ url, 'editContent', new_content="the new content")
+ self.assertEqual(401, response.status)
+
+ edited_obj = ws.get(url).jsonBody()
+ self.assertEqual("initial content", edited_obj['content'])
+ self.assertIsNone(edited_obj["date_deleted"])
+ self.assertIsNone(edited_obj["date_last_edit"])
+
+ def test_delete_message(self):
+ msg = self.makeMessage(content="initial content")
+ ws = self.getWebservice(self.person)
+ url = self.getMessageAPIURL(msg)
+
+ response = ws.named_post(url, 'deleteContent')
+ self.assertEqual(200, response.status)
+
+ deleted_obj = ws.get(url).jsonBody()
+ self.assertEqual("", deleted_obj['content'])
+ self.assertIsNotNone(deleted_obj['date_deleted'])
+
+ def test_delete_message_permission_denied_for_non_owner(self):
+ msg = self.makeMessage(content="initial content")
+ ws = self.getWebservice(self.factory.makePerson())
+ url = self.getMessageAPIURL(msg)
+
+ response = ws.named_post(url, 'deleteContent')
+ self.assertEqual(401, response.status)
+
+ obj = ws.get(url).jsonBody()
+ self.assertEqual("initial content", obj['content'])
+ self.assertIsNone(obj['date_deleted'])
Follow ups
