launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Addressed all the comments. I would like your opinion on the comment on xx-branchmergeproposal.txt, about deprecating an API attribute.

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

Right!

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

Right! Fixing it.

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

This method is used by `get_comments_for_bugtask`, which is the one used to traverse bugtask to its comments. If we filter out completely (with an `INNER JOIN`, `MessageChunk.id != None`) the Message objects that doesn't have content, we wouldn't be able to fetch "deleted" bug messages in the API, for example.

And the initial idea was keeping both in the web UI and in the API the Message objects that had its contents deleted.

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

Ideally, deprecating `message_body` (keeping all the messages compatible with the `content` interface) would be preferred, but I'm a bit afraid of breaking backward compatibility with whatever currently depends on this API.

Do you think we should do it in this MP?

>      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

Actually, the current traverse for a specific question message doesn't seem to work today:

For example:
- https://answers.launchpad.net/api/devel/launchpad/+question/697126/messages returns a list of messages
- https://answers.launchpad.net/api/devel/launchpad/+question/697126/messages/{0,1} both returns error

Also, the answers/browser/configure.zcml declares `path_expression="string:messages/${display_index}"`, so I guess the initial intention was always to use "display_index" (starting from 1), as for code review comments and bug comments.

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

Sure! You're right: the idea is making it readonly=True, allowing editing only using `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

Oops

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

You're right. My mistake when refactoring it. Fixing right now.

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