launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26988
[Merge] ~pappacena/launchpad:comment-editing-model into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:comment-editing-model into launchpad:master.
Commit message:
Mapping database initial structure for message editing
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401894
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:comment-editing-model into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 1bbb845..c8b7d10 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -232,7 +232,8 @@ public.logintoken = SELECT, INSERT, UPDATE, DELETE
public.mailinglist = SELECT, INSERT, UPDATE, DELETE
public.mailinglistsubscription = SELECT, INSERT, UPDATE, DELETE
public.messageapproval = SELECT, INSERT, UPDATE, DELETE
-public.messagechunk = SELECT, INSERT
+public.messagechunk = SELECT, INSERT, DELETE
+public.messagerevision = SELECT, INSERT, UPDATE
public.milestone = SELECT, INSERT, UPDATE, DELETE
public.milestonetag = SELECT, INSERT, UPDATE, DELETE
public.mirrorcdimagedistroseries = SELECT, INSERT, DELETE
diff --git a/lib/lp/bugs/browser/tests/test_bugcomment.py b/lib/lp/bugs/browser/tests/test_bugcomment.py
index c1877a5..e5baac1 100644
--- a/lib/lp/bugs/browser/tests/test_bugcomment.py
+++ b/lib/lp/bugs/browser/tests/test_bugcomment.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the bugcomment module."""
@@ -35,6 +35,7 @@ from lp.testing import (
BrowserTestCase,
celebrity_logged_in,
login_person,
+ person_logged_in,
TestCase,
TestCaseWithFactory,
verifyObject,
@@ -300,7 +301,10 @@ class TestBugCommentImplementsInterface(TestCaseWithFactory):
bug_message = self.factory.makeBugComment()
bugtask = bug_message.bugs[0].bugtasks[0]
bug_comment = BugComment(1, bug_message, bugtask)
- verifyObject(IBugComment, bug_comment)
+ # Runs verifyObject logged in as the bug owner, so we don't fail on
+ # attributes that are not public to everyone.
+ with person_logged_in(bug_message.owner):
+ verifyObject(IBugComment, bug_comment)
def test_download_url(self):
"""download_url is provided and works as expected."""
diff --git a/lib/lp/security.py b/lib/lp/security.py
index 412f497..cd88d5c 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -3182,6 +3182,15 @@ class SetMessageVisibility(AuthorizationBase):
return (user.in_admin or user.in_registry_experts)
+class EditMessage(AuthorizationBase):
+ permission = 'launchpad.Edit'
+ usedfor = IMessage
+
+ def checkAuthenticated(self, user):
+ """Only message owner can edit it."""
+ return user.isOwner(self.obj)
+
+
class ViewPublisherConfig(AdminByAdminsTeam):
usedfor = IPublisherConfig
diff --git a/lib/lp/services/messages/configure.zcml b/lib/lp/services/messages/configure.zcml
index e989fe2..fcaffab 100644
--- a/lib/lp/services/messages/configure.zcml
+++ b/lib/lp/services/messages/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2011 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).
-->
@@ -10,9 +10,9 @@
i18n_domain="launchpad">
<!-- Message -->
- <class
- class="lp.services.messages.model.message.Message">
- <allow interface="lp.services.messages.interfaces.message.IMessage" />
+ <class class="lp.services.messages.model.message.Message">
+ <allow
+ interface="lp.services.messages.interfaces.message.IMessageView" />
<!-- setVisible is required to allow IBug.setCommentVisibility() to
change the visibility attribute whilst still ensuring restricted
access to the attribute via the API.-->
@@ -22,6 +22,19 @@
<require
permission="launchpad.Admin"
set_attributes="visible"/>
+ <require
+ permission="launchpad.Edit"
+ interface="lp.services.messages.interfaces.message.IMessageEdit" />
+ </class>
+
+ <!-- MessageRevision -->
+ <class
+ class="lp.services.messages.model.messagerevision.MessageRevision">
+ <allow
+ interface="lp.services.messages.interfaces.messagerevision.IMessageRevisionView"/>
+ <require
+ permission="launchpad.Edit"
+ interface="lp.services.messages.interfaces.messagerevision.IMessageRevisionEdit"/>
</class>
<class class="lp.services.messages.interfaces.message.IndexedMessage">
diff --git a/lib/lp/services/messages/interfaces/message.py b/lib/lp/services/messages/interfaces/message.py
index 7e18e7d..0c54c84 100644
--- a/lib/lp/services/messages/interfaces/message.py
+++ b/lib/lp/services/messages/interfaces/message.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
@@ -49,9 +49,19 @@ from lp.services.librarian.interfaces import ILibraryFileAlias
from lp.services.webservice.apihelpers import patch_reference_property
-@exported_as_webservice_entry('message')
-class IMessage(Interface):
- """A message.
+class IMessageEdit(Interface):
+
+ def edit_content(new_content):
+ """Edit the content of this message, generating a new message
+ revision with the old content.
+ """
+
+ def delete_content():
+ """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.
@@ -61,6 +71,15 @@ class IMessage(Interface):
datecreated = exported(
Datetime(title=_('Date Created'), required=True, readonly=True),
exported_as='date_created')
+
+ date_last_edit = Datetime(
+ title=_('When this message was last edited'), required=False,
+ readonly=True)
+
+ date_deleted = Datetime(
+ title=_('When this message was deleted'), required=False,
+ readonly=True)
+
subject = exported(
TextLine(title=_('Subject'), required=True, readonly=True))
@@ -87,6 +106,8 @@ class IMessage(Interface):
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.')),
@@ -115,6 +136,11 @@ class IMessage(Interface):
"""Return None because messages are not threaded over the API."""
+@exported_as_webservice_entry('message')
+class IMessage(IMessageEdit, IMessageView):
+ """A Message."""
+
+
# Fix for self-referential schema.
patch_reference_property(IMessage, 'parent', IMessage)
diff --git a/lib/lp/services/messages/interfaces/messagerevision.py b/lib/lp/services/messages/interfaces/messagerevision.py
new file mode 100644
index 0000000..96aeb4d
--- /dev/null
+++ b/lib/lp/services/messages/interfaces/messagerevision.py
@@ -0,0 +1,53 @@
+# Copyright 2019-2021 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Message revision history."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__all__ = [
+ 'IMessageRevision'
+ ]
+
+from lazr.restful.fields import Reference
+from zope.interface import Interface
+from zope.schema import (
+ Datetime,
+ Int,
+ Text,
+ )
+
+from lp import _
+from lp.services.messages.interfaces.message import IMessage
+
+
+class IMessageRevisionView(Interface):
+ """IMessageRevision readable attributes."""
+ id = Int(title=_("ID"), required=True, readonly=True)
+
+ content = Text(
+ title=_("The message at the given revision"),
+ required=False, readonly=True)
+
+ message = Reference(
+ title=_('The current message of this revision.'),
+ schema=IMessage, required=True, readonly=True)
+
+ date_created = Datetime(
+ title=_("The time when this message revision was created."),
+ required=True, readonly=True)
+
+ date_deleted = Datetime(
+ title=_("The time when this message revision was created."),
+ required=False, readonly=True)
+
+
+class IMessageRevisionEdit(Interface):
+ """IMessageRevision editable attributes."""
+
+ def destroySelf():
+ """Logically deletes this MessageRevision."""
+
+
+class IMessageRevision(IMessageRevisionView, IMessageRevisionEdit):
+ """A historical revision of a IMessage."""
diff --git a/lib/lp/services/messages/model/message.py b/lib/lp/services/messages/model/message.py
index e592daa..1966cc6 100644
--- a/lib/lp/services/messages/model/message.py
+++ b/lib/lp/services/messages/model/message.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
@@ -40,6 +40,7 @@ from sqlobject import (
from storm.locals import (
And,
DateTime,
+ Desc,
Int,
Reference,
Store,
@@ -71,7 +72,12 @@ from lp.services.messages.interfaces.message import (
IUserToUserEmail,
UnknownSender,
)
-from lp.services.propertycache import cachedproperty
+from lp.services.messages.model.messagerevision import MessageRevision
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
+from lp.services.utils import utc_now
def utcdatetime_from_field(field_value):
@@ -100,6 +106,8 @@ class Message(SQLBase):
_table = 'Message'
_defaultOrder = '-id'
datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
+ date_deleted = UtcDateTimeCol(notNull=False, default=None)
+ date_last_edit = UtcDateTimeCol(notNull=False, default=None)
subject = StringCol(notNull=False, default=None)
owner = ForeignKey(
dbName='owner', foreignKey='Person',
@@ -164,6 +172,47 @@ class Message(SQLBase):
"""See `IMessage`."""
return None
+ @cachedproperty
+ def revisions(self):
+ """See `IMessage`."""
+ return list(Store.of(self).find(
+ MessageRevision,
+ MessageRevision.message == self
+ ).order_by(Desc(MessageRevision.date_created)))
+
+ def edit_content(self, new_content):
+ """See `IMessage`."""
+ store = Store.of(self)
+ old_content = self.text_contents
+ # Remove the current content.
+ for chunk in self._chunks:
+ store.remove(chunk)
+
+ # Create the new content.
+ new_chunk = MessageChunk(message=self, sequence=1, content=new_content)
+ store.add(new_chunk)
+
+ # Move the old content to a new revision.
+ date_created = (self.date_last_edit if self.date_last_edit is not None
+ else self.datecreated)
+ rev = MessageRevision(message=self, content=old_content,
+ date_created=date_created)
+ self.date_last_edit = utc_now()
+ store.add(rev)
+ store.flush()
+
+ # Clean up caches.
+ del get_property_cache(self).text_contents
+ del get_property_cache(self).chunks
+ del get_property_cache(self).revisions
+ return rev
+
+ def delete_content(self):
+ """See `IMessage`."""
+ for chunk in self._chunks:
+ chunk.destroySelf()
+ self.date_deleted = utc_now()
+
def get_parent_msgids(parsed_message):
"""Returns a list of message ids the mail was a reply to.
diff --git a/lib/lp/services/messages/model/messagerevision.py b/lib/lp/services/messages/model/messagerevision.py
new file mode 100644
index 0000000..2ea188c
--- /dev/null
+++ b/lib/lp/services/messages/model/messagerevision.py
@@ -0,0 +1,52 @@
+# Copyright 2019-2021 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Message revision history."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+ 'MessageRevision'
+ ]
+
+import pytz
+from storm.locals import (
+ DateTime,
+ Int,
+ Reference,
+ Unicode,
+ )
+from zope.interface import implementer
+
+from lp.services.database.stormbase import StormBase
+from lp.services.messages.interfaces.messagerevision import IMessageRevision
+from lp.services.utils import utc_now
+
+
+@implementer(IMessageRevision)
+class MessageRevision(StormBase):
+ """A historical revision of a IMessage."""
+
+ __storm_table__ = 'MessageRevision'
+
+ id = Int(primary=True)
+
+ message_id = Int(name='message', allow_none=False)
+ message = Reference(message_id, 'Message.id')
+
+ content = Unicode(name="content", allow_none=False)
+
+ date_created = DateTime(
+ name="date_created", tzinfo=pytz.UTC, allow_none=False)
+ date_deleted = DateTime(
+ name="date_deleted", tzinfo=pytz.UTC, allow_none=True)
+
+ def __init__(self, message, content, date_created, date_deleted=None):
+ self.message = message
+ self.content = content
+ self.date_created = date_created
+ self.date_deleted = date_deleted
+
+ def destroySelf(self):
+ 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 a9e1042..49e15eb 100644
--- a/lib/lp/services/messages/tests/test_message.py
+++ b/lib/lp/services/messages/tests/test_message.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
@@ -13,15 +13,27 @@ from email.utils import (
)
import six
+from testtools.matchers import (
+ Equals,
+ Is,
+ MatchesStructure,
+ )
import transaction
+from zope.security.interfaces import Unauthorized
+from zope.security.proxy import ProxyFactory
from lp.services.compat import message_as_bytes
from lp.services.messages.model.message import MessageSet
+from lp.services.utils import utc_now
from lp.testing import (
login,
+ person_logged_in,
TestCaseWithFactory,
)
-from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.layers import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
class TestMessageSet(TestCaseWithFactory):
@@ -169,3 +181,82 @@ class TestMessageSet(TestCaseWithFactory):
'Treating unknown encoding "booga" as latin-1.'):
result = MessageSet.decode(self.high_characters, 'booga')
self.assertEqual(self.high_characters.decode('latin-1'), result)
+
+
+class TestMessageEditing(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 test_non_owner_cannot_edit_message(self):
+ msg = self.makeMessage()
+ someone_else = self.factory.makePerson()
+ with person_logged_in(someone_else):
+ self.assertRaises(Unauthorized, getattr, msg, "edit_content")
+
+ def test_msg_owner_can_edit(self):
+ owner = self.factory.makePerson()
+ msg = self.makeMessage(owner=owner, content="initial content")
+ with person_logged_in(owner):
+ msg.edit_content("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)))
+
+ def test_multiple_edits_revisions(self):
+ owner = self.factory.makePerson()
+ msg = self.makeMessage(owner=owner, content="initial content")
+ with person_logged_in(owner):
+ msg.edit_content("first edit")
+ 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)))
+
+ with person_logged_in(owner):
+ msg.edit_content("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)))
+
+ def test_non_owner_cannot_delete_message(self):
+ owner = self.factory.makePerson()
+ msg = self.makeMessage(owner=owner, content="initial content")
+ someone_else = self.factory.makePerson()
+ with person_logged_in(someone_else):
+ self.assertRaises(Unauthorized, getattr, msg, "delete_content")
+
+ def test_delete_message(self):
+ owner = self.factory.makePerson()
+ msg = self.makeMessage(owner=owner, content="initial content")
+ with person_logged_in(owner):
+ before_delete = utc_now()
+ msg.delete_content()
+ after_delete = utc_now()
+ self.assertEqual('', msg.text_contents)
+ self.assertEqual(0, len(msg.chunks))
+ self.assertIsNotNone(msg.date_deleted)
+ self.assertTrue(after_delete > msg.date_deleted > before_delete)
Follow ups
