On Tue, 2009-12-01 at 12:33 +0000, Graham Binns wrote:
>                              From: 
> Graham Binns <email address hidden>
>                          Reply-to: 
> <email address hidden>
>                                To: 
> Guilherme Salgado
> <email address hidden>
>                           Subject: 
> Request to review proposed merge of
> lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel
>                              Date: 
> 12/01/2009 10:33:48 AM (Tue, 01 Dec
> 2009 12:33:48 -0000)
> 
> 
> You have been requested to review the proposed merge of
> lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel.
> 
> This branch fixes bug 394097.
> 
> The bug was caused by messages in the offending bug's set of message
> having parent messages that weren't linked to the bug. This caused the
> webservice machinery to blow up.

When can a message have a parent which is linked to a different bug?
Might be worth noting that in a comment in the code.

> 
> To counter this, we've made it so that bug.indexed_messages ensures
> that IIndexedMessage.parent returns None if the parent of a message
> isn't linked to the same bug as the child message.
> 
> Gavin has added a regression test to cover the bug and I've added
> tests to cover the changes to Bug.indexed_messages.
> 

> === modified file 'lib/canonical/launchpad/interfaces/message.py'
> --- lib/canonical/launchpad/interfaces/message.py       2009-06-25
> 05:30:52 +0000
> +++ lib/canonical/launchpad/interfaces/message.py       2009-12-01
> 12:30:30 +0000
> @@ -189,15 +189,21 @@
>                  description=_("The index of this message in the list
> "
>                                "of messages in its context."))
>  
> +
>  class IndexedMessage:
>      """Adds the `inside` and `index` attributes to an IMessage."""

Do you think it's worth extending this docstring to mention that the
original message's .parent is overwritten here with whatever is given in
__init__?

>      delegates(IMessage)
>      implements(IIndexedMessage)
>  
> -    def __init__(self, context, inside, index):
> +    def __init__(self, context, inside, index, parent=None):
>          self.context = context
>          self.inside = inside
>          self.index = index
> +        self._parent = parent
> +
> +    @property
> +    def parent(self):
> +        return self._parent
>  
>  
>  class IMessageChunk(Interface):
> 
> === modified file 'lib/lp/bugs/model/bug.py'
> --- lib/lp/bugs/model/bug.py    2009-11-25 13:22:01 +0000
> +++ lib/lp/bugs/model/bug.py    2009-12-01 12:30:30 +0000
> @@ -279,9 +279,19 @@
>      def indexed_messages(self):
>          """See `IMessageTarget`."""
>          inside = self.default_bugtask
> -        return [
> -            IndexedMessage(message, inside, index)
> -            for index, message in enumerate(self.messages)]
> +        message_set = set(self.messages)
> +
> +        indexed_messages = []
> +        for index, message in enumerate(message_set):
> +            if message.parent not in message_set:
> +                parent = None
> +            else:
> +                parent = message.parent

I think the if/else block above should have a comment explaining why
it's needed; it's clear to me as I know the bug it's fixing, but won't
be clear to others, I'm pretty sure.

> +
> +            indexed_message = IndexedMessage(message, inside, index,
> parent)
> +            indexed_messages.append(indexed_message)
> +
> +        return indexed_messages
>  
>      @property
>      def displayname(self):
> 


-- 
Guilherme Salgado <email address hidden>