launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



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

Are `MessageRevisionChunk` rows ever updated?  I thought they were only inserted or deleted.

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

I guess there's some kind of default permission that applies to `IMessageRevision`, but it seems as though it might be a good idea for it to explicitly delegate to its parent `IMessage`.

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

Maybe `required=True`, since the property can never return None?

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

What does "Logically deletes" mean?

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

Should be `required=True` to match the DB patch, I think.

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

This would normally be `UTC_NOW` (from `lp.services.database.constants`) instead, to use the transaction timestamp.

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

What happens if there's an existing non-text chunk with sequence 1?  It won't have been removed by the loop above, and this will clash with it.

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

Can this be turned into a single query rather than a loop?

> +        store.find(MessageRevision, MessageRevision.message == self).remove()

It seems a bit anomalous that this deletes both the `MessageRevisionChunk` and `MessageRevision` rows, while `MessageRevision.deleteContent` only deletes the `MessageRevisionChunk` rows.  What's the rationale for that?

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

`Message.chunks_text` joins on `'\n\n'` instead.  It feels as though the two should match.

> +
> +    def deleteContent(self):
> +        store = IStore(self)
> +        store.find(MessageRevisionChunk, message_revision=self).remove()
> +        self.date_deleted = utc_now()

As above, this should use `lp.services.database.constants.UTC_NOW` instead of the Python timestamp.

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

This could use `assertBetween`; but since there's no transaction boundary in this test, I think you could use `get_transaction_timestamp` and do an exact assertion instead.  (Though I've been wrong about this before, so feel free to fall back to `assertBetween` if an exact assertion doesn't work.)

> +        self.assertEqual(0, len(msg.revisions))

I'd like to see tests covering `MessageRevision.deleteContent` as well.



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