Addressed all the comments.
Diff comments:
> diff --git a/database/schema/security.cfg b/database/schema/security.cfg
> index c3aba65..b712521 100644
> --- a/database/schema/security.cfg
> +++ b/database/schema/security.cfg
> @@ -232,7 +232,9 @@ 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, DELETE
> +public.messagerevisionchunk = SELECT, INSERT, UPDATE, DELETE
You are right. Fixing it.
> public.milestone = SELECT, INSERT, UPDATE, DELETE
> public.milestonetag = SELECT, INSERT, UPDATE, DELETE
> public.mirrorcdimagedistroseries = SELECT, INSERT, DELETE
> 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)
Right. Adding it.
> +
> +
> class ViewPublisherConfig(AdminByAdminsTeam):
> usedfor = IPublisherConfig
>
> diff --git a/lib/lp/services/messages/interfaces/messagerevision.py
> b/lib/lp/services/messages/interfaces/messagerevision.py
> new file mode 100644
> index 0000000..3b72673
> --- /dev/null
> +++ b/lib/lp/services/messages/interfaces/messagerevision.py
> @@ -0,0 +1,69 @@
> +# 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',
> + 'IMessageRevisionChunk',
> + ]
> +
> +from lazr.restful.fields import Reference
> +from zope.interface import (
> + Attribute,
> + 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)
> +
> + revision = Int(title=_("Revision number"), required=True, readonly=True)
> +
> + content = Text(
> + title=_("The message at the given revision"),
> + required=False, readonly=True)
Ok!
> +
> + 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)
> +
> + chunks = Attribute(_('Message pieces'))
> +
> +
> +class IMessageRevisionEdit(Interface):
> + """IMessageRevision editable attributes."""
> +
> + def deleteContent():
> + """Logically deletes this MessageRevision."""
Fixing this description.
> +
> +
> +class IMessageRevision(IMessageRevisionView, IMessageRevisionEdit):
> + """A historical revision of a IMessage."""
> +
> +
> +class IMessageRevisionChunk(Interface):
> + id = Int(title=_('ID'), required=True, readonly=True)
> + messagerevision = Int(
> + title=_('MessageRevision'), required=True, readonly=True)
> + sequence = Int(title=_('Sequence order'), required=True, readonly=True)
> + content = Text(title=_('Text content'), required=False, readonly=True)
Ok!
> diff --git a/lib/lp/services/messages/model/message.py
> b/lib/lp/services/messages/model/message.py
> index e592daa..b73e212 100644
> --- a/lib/lp/services/messages/model/message.py
> +++ b/lib/lp/services/messages/model/message.py
> @@ -164,6 +176,63 @@ 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.revision)))
> +
> + def editContent(self, new_content):
> + """See `IMessage`."""
> + store = Store.of(self)
> +
> + # Move the old content to a new revision.
> + date_created = (
> + self.date_last_edited if self.date_last_edited is not None
> + else self.datecreated)
> + current_rev_num = store.find(
> + Max(MessageRevision.revision),
> + MessageRevision.message == self).one()
> + rev_num = (current_rev_num or 0) + 1
> + rev = MessageRevision(
> + message=self, revision=rev_num, date_created=date_created)
> + self.date_last_edited = utc_now()
Ok!
> + store.add(rev)
> +
> + # Move the current text content to the recently created revision.
> + for chunk in self._chunks:
> + if chunk.blob is None:
> + revision_chunk = MessageRevisionChunk(
> + rev, chunk.sequence, chunk.content)
> + store.add(revision_chunk)
> + store.remove(chunk)
> +
> + # Create the new content.
> + new_chunk = MessageChunk(message=self, sequence=1,
> content=new_content)
You're right. I'll set the new chunk sequence to the bigger sequence + 1.
> + store.add(new_chunk)
> +
> + 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 deleteContent(self):
> + """See `IMessage`."""
> + store = Store.of(self)
> + store.find(MessageChunk, MessageChunk.message == self).remove()
> + for rev in self.revisions:
> + store.find(MessageRevisionChunk, message_revision=rev).remove()
Sure!
> + store.find(MessageRevision, MessageRevision.message == self).remove()
The idea is that deleting a message itself will remove everything about that
message (content and history), keeping only the Message object to mark that a
message existed there before.
On the other hand, deleting a message revision content will clear only its
content, marking that something was edited at that time, but the content is no
longer available (useful, for example, if you edited a message because you've
mistakenly typed a password there and you don't want that password hanging
around on the message history).
> + del get_property_cache(self).text_contents
> + del get_property_cache(self).chunks
> + del get_property_cache(self).revisions
> + 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..c8b6142
> --- /dev/null
> +++ b/lib/lp/services/messages/model/messagerevision.py
> @@ -0,0 +1,92 @@
> +# 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',
> + 'MessageRevisionChunk',
> + ]
> +
> +import pytz
> +from storm.locals import (
> + DateTime,
> + Int,
> + Reference,
> + Unicode,
> + )
> +from zope.interface import implementer
> +
> +from lp.services.database.interfaces import IStore
> +from lp.services.database.stormbase import StormBase
> +from lp.services.messages.interfaces.messagerevision import (
> + IMessageRevision,
> + IMessageRevisionChunk,
> + )
> +from lp.services.propertycache import (
> + cachedproperty,
> + get_property_cache,
> + )
> +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')
> +
> + revision = Int(name='revision', 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, revision, date_created, date_deleted=None):
> + self.message = message
> + self.revision = revision
> + self.date_created = date_created
> + self.date_deleted = date_deleted
> +
> + @cachedproperty
> + def chunks(self):
> + return list(IStore(self).find(
> + MessageRevisionChunk, message_revision=self))
> +
> + @property
> + def content(self):
> + return ''.join(i.content for i in self.chunks)
Ok!
> +
> + def deleteContent(self):
> + store = IStore(self)
> + store.find(MessageRevisionChunk, message_revision=self).remove()
> + self.date_deleted = utc_now()
Fixing it.
> + del get_property_cache(self).chunks
> +
> +
> +@implementer(IMessageRevisionChunk)
> +class MessageRevisionChunk(StormBase):
> + __storm_table__ = 'MessageRevisionChunk'
> +
> + id = Int(primary=True)
> +
> + message_revision_id = Int(name='messagerevision', allow_none=False)
> + message_revision = Reference(message_revision_id, 'MessageRevision.id')
> +
> + sequence = Int(name='sequence', allow_none=False)
> +
> + content = Unicode(name="content", allow_none=False)
> +
> + def __init__(self, message_revision, sequence, content):
> + self.message_revision = message_revision
> + self.sequence = sequence
> + self.content = content
> diff --git a/lib/lp/services/messages/tests/test_message.py
> b/lib/lp/services/messages/tests/test_message.py
> index a9e1042..d015f87 100644
> --- a/lib/lp/services/messages/tests/test_message.py
> +++ b/lib/lp/services/messages/tests/test_message.py
> @@ -169,3 +185,122 @@ 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, "editContent")
> +
> + 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.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"),
> + revision=Equals(1),
> + 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.editContent("first edit")
> + first_edit_date = msg.date_last_edited
> + self.assertEqual("first edit", msg.text_contents)
> + self.assertEqual(1, len(msg.revisions))
> + self.assertThat(msg.revisions[0], MatchesStructure(
> + content=Equals("initial content"),
> + revision=Equals(1),
> + message=Equals(msg),
> + date_created=Equals(msg.datecreated),
> + date_deleted=Is(None)))
> +
> + 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"),
> + revision=Equals(2),
> + message=Equals(msg),
> + date_created=Equals(first_edit_date),
> + date_deleted=Is(None)))
> + self.assertThat(msg.revisions[1], MatchesStructure(
> + content=Equals("initial content"),
> + revision=Equals(1),
> + message=Equals(msg),
> + date_created=Equals(msg.datecreated),
> + date_deleted=Is(None)))
> +
> + def test_edit_message_with_blobs(self):
> + # Messages with blobs should keep the blobs untouched when the
> + # content is edited.
> + owner = self.factory.makePerson()
> + msg = self.makeMessage(owner=owner, content="initial content")
> + files = [self.factory.makeLibraryFileAlias(db_only=True)
> + for _ in range(2)]
> + store = IStore(msg)
> + for seq, blob in enumerate(files):
> + store.add(MessageChunk(message=msg, sequence=seq + 2, blob=blob))
> +
> + with person_logged_in(owner):
> + msg.editContent("final form")
> + self.assertThat(msg.revisions[0], MatchesStructure(
> + content=Equals("initial content"),
> + revision=Equals(1),
> + message=Equals(msg),
> + date_created=Equals(msg.datecreated),
> + date_deleted=Is(None)))
> +
> + # Check that current message chunks are 3: the 2 old blobs, and the
> + # new text message.
> + self.assertEqual(3, len(msg.chunks))
> + self.assertEqual("final form", msg.chunks[0].content)
> + self.assertEqual(files, [i.blob for i in msg.chunks[1:]])
> +
> + # Check revision chunks. It should be the old text message.
> + rev_chunks = msg.revisions[0].chunks
> + self.assertEqual(1, len(rev_chunks))
> + self.assertThat(rev_chunks[0], MatchesStructure(
> + sequence=Equals(1),
> + content=Equals("initial content")))
> +
> + 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, "deleteContent")
> +
> + def test_delete_message(self):
> + owner = self.factory.makePerson()
> + msg = self.makeMessage(owner=owner, content="initial content")
> + with person_logged_in(owner):
> + msg.editContent("new content")
> + with person_logged_in(owner):
> + before_delete = utc_now()
> + msg.deleteContent()
> + 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)
The exact assertion works fine. Thanks! Changing it.
> + self.assertEqual(0, len(msg.revisions))
I totally missed that. Adding it now!
--
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401894
Your team Launchpad code reviewers is subscribed to branch
~pappacena/launchpad:comment-editing-model.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
