launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

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

Ah, I see.  Fair enough then.

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

If this is the plan, then we should deprecate `ICodeReviewComment.message_body` in this MP, but not remove it: that is, just change its description to indicate that users of that attribute should use `content` instead.

>      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

Interesting - OK then.  And I checked lp-spam-utils and it doesn't in fact care about the message URL (it just does `message.question.setCommentVisibility(comment_number=message.index - 1, visible=False)` or similar), so I guess that won't be a problem.

>  
>  
>  @implementer(ICanonicalUrlData)


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