launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

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

Maybe `traverse_messages`?

> +        index = int(index) - 1
> +        return self.context.messages[index]

I believe that `stepto`/`stepthrough` traversal methods like this are normally meant to either (a) return an object, (b) return None, or (c) raise NotFoundError (which is treated as equivalent to (b)).  So this should probably be something like the following to avoid OOPSing on bad input:

    try:
        index = int(index) - 1
    except ValueError:
        return None
    try:
        return self.context.messages[index]
    except IndexError:
        return None

> +
> +
>  class QuestionBreadcrumb(Breadcrumb):
>      """Builds a breadcrumb for an `IQuestion`."""
>  
> 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
> @@ -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:

Shouldn't these be filtered out by `Bug.getMessagesForView` instead (e.g. adding a `MessageChunk.id != None` condition or something like that)?  This function is that method's only non-test caller, so that method might as well be more accurate for what we need here.

> +            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/code/stories/webservice/xx-branchmergeproposal.txt b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
> index 32f0373..58b86e7 100644
> --- a/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
> +++ b/lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
> @@ -198,9 +198,13 @@ 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'

It seems a bit unfortunate to effectively end up duplicating `message_body` onto `content`.  Is it more subtle than that, or can this duplication be avoided somehow (by more selective exports, or by explicitly deprecating the old name)?

>      date_created: '...'
> +    date_deleted: None
> +    date_last_edited: None
>      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'
> 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
> @@ -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

Won't this change the URL of every existing message (potentially problematic for things like spam-removal tools)?  Maybe `message.index` instead?

>  
>  
>  @implementer(ICanonicalUrlData)
> diff --git a/lib/lp/services/messages/interfaces/message.py b/lib/lp/services/messages/interfaces/message.py
> index 70d50b7..73edea1 100644
> --- a/lib/lp/services/messages/interfaces/message.py
> +++ b/lib/lp/services/messages/interfaces/message.py
> @@ -51,44 +56,62 @@ 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')

Can you make the `readonly=True` or `readonly=False` status of this attribute explicit here?  It seems to be writeable at the moment, although I assume that must be a mistake, and even in your branch stack I think it should only be edited using the `editContent` method?

> +    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_edited = exported(Datetime(
>  
> -    date_last_edited = 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.
> diff --git a/lib/lp/services/messages/tests/test_message.py b/lib/lp/services/messages/tests/test_message.py
> index b63e5aa..26461ef 100644
> --- a/lib/lp/services/messages/tests/test_message.py
> +++ b/lib/lp/services/messages/tests/test_message.py
> @@ -187,16 +198,50 @@ 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, rev, created_at, content, deleted_at=None):
> +        """Asserts that `msg_history` is a message a message history of

Duplicated "a message".

> +        `msg` with the given extra info.
> +        """
> +        self.assertThat(msg_history, MatchesStructure(
> +            content=Equals(content),
> +            revision=Equals(rev),
> +            message=Equals(removeSecurityProxy(msg).message),
> +            date_created=Equals(created_at),
> +            date_deleted=Is(deleted_at)))

This would be better with `Equals`.  It's true that we normally use `Is` for comparisons against None, but `deleted_at` may presumably be something other than None, and two `datetime` objects that compare equal may not necessarily be identical objects.

>  
>      def test_non_owner_cannot_edit_message(self):
>          msg = self.makeMessage()


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