launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 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)

It looks like you changed `IMessageRevisionChunk.content` but forgot to change this one?

> +
> +    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."""
> +
> +
> +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)
> 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()
> +        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)

It's probably not vital, but I think ideally the new chunk should occupy the smallest unused sequence number, rather than the largest + 1.  (Consider [text comment, binary attachment], which if edited should probably turn into [new text comment, binary attachment] rather than [gap, binary attachment, new text comment].)

> +        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()
> +        store.find(MessageRevision, MessageRevision.message == self).remove()

Fair enough, thanks.

> +        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.


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401894
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:comment-editing-model.