Code review comment for lp:~gmb/launchpad/indexed-message-parents

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>