hide-comments.py is hiding the wrong bug

Right, if the comment number as shown on bug pages like
https://bugs.launchpad.net/ubuntu/+source/compiz-fusion-plugins-main/+bug/183685?comments=all
is used as the value of the parameter comment_number in
IBug.setCommentVisibility(comment_number, visible), the wrong
comment can be changed. The reason:

    def setCommentVisibility(self, user, comment_number, visible):
        """See `IBug`."""
        bug_message_set = getUtility(IBugMessageSet)
        bug_message = bug_message_set.getByBugAndMessage(
            self, self.messages[comment_number])
        bug_message.visible = visible

So, the method changes the bug_message having the index
comment_number in the sequence of all [bug_]messages.

But the comments displayed and indexed on
https://bugs.launchpad.net/ubuntu/+source/compiz-fusion-plugins-main/+bug/183685?comments=all
are slightly differently created: BugTaskView.comments gets the
sequence of comments to display from the function
get_comments_for_bugtask():

    def get_comments_for_bugtask(bugtask, truncate=False):
        """Return BugComments related to a bugtask.

        This code builds a sorted list of BugComments in one shot,
        requiring only two database queries. It removes the titles
        for those comments which do not have a "new" subject line
        """
        chunks = bugtask.bug.getMessageChunks()
        comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)

and getMessageChunks() retrieves, as the name suggests, message chunks
related to a given bug:

    def getMessageChunks(self):
        """See `IBug`."""
        query = """
            Message.id = MessageChunk.message AND
            BugMessage.message = Message.id AND
            BugMessage.bug = %s
            """ % sqlvalues(self)

        chunks = MessageChunk.select(query,
            clauseTables=["BugMessage", "Message"],

The problem: There exist messages without any associated chunks, so
these messages do not appear in the result of get_comments_for_bugtask().

To check:

    SELECT Message.datecreated, Message.id, BugMessage.id
      FROM BugMessage, Message
     WHERE BugMessage.bug = 183685 AND
           Message.id = BugMessage.message AND (1=1)
     ORDER BY Message.datecreated, Message.id;

returns all message IDs related to the given bug.

And

    select MessageChunk.id, Message.id, BugMessage.id
      from MessageChunk, Message, BugMessage
     where Message.id = MessageChunk.message AND
           BugMessage.message = Message.id AND
           BugMessage.bug = 183685;

returns the IDs of all messages having at least one MessageChunnk.

Comparing the results of these queries, it turns out that exactly
five Message IDs from the former query are missing the in the latter
one. And the former query is basically the same query as the one issued
when bug.messages is retrieved, i.e., when setCommentVisibility is
called.

See also bug 365092. Not exactly a duplicate but closely related

So a way to fix this is to use the same filtering logic in the script?

On 18.01.2011 15:43, Robert Collins wrote:
> So a way to fix this is to use the same filtering logic in the script?
>

Yes, but I don't like it very much: The filtering for the display of
comments on HTML pages is done in browser code, while the hide-comments
script directly accesses bug.messages[N], so we would have two places
where we would have to do the filtering, thus creating the typical risk
that code in one place might change but the code in the other place is
not updated accordingly.

Also, out of curiosity I played with the API:

lp-shell
Connected to LP service "edge" with API version "1.0":
Note: LP can be accessed through the "lp" object.
>>> b = lp.bugs[183685]
>>> len(list(msg for msg in b.messages if not msg.content and
len(list(msg.bug_attachments)) == 0))
14
>>> len(list(msg for msg in b.messages if msg.content is None and
len(list(msg.bug_attachments)) == 0))
0

So, we seem have 14 messages where len(msg.content) == 0, but only 5 of
them do actually not appear in the data returned by
get_comments_for_bugtask(), so it seems that we even have the case of
messages with an empty MessageChunk...

And we should not forget bug 365092 which is really closely related: As
I understand it, it is caused by a BugMessage referencing a BugWatch,
but the BugMessage is not included in the result of
get_comments_for_bugtask().

Better approaches might be:

  - clean up the database and ensure via constraints that we don't
    create empty messages any longer
  - add a method to class Bug, like getRealMesssages()

I'd prefer the former variant, because it would address bug 365092 as
well. (OK, having just a constraint would not prevent attempts to create
empty messages again, but as I wrote in a comment on bug 365092, I have
just a vague guess where empty messages are created. I'd prefer to sees
OOPSes when this happens so that we get a clue how the real fix should
look than to add some band-aid...)

We now permanently and persistently assign message #'s rather than recalculating at render time, removing the discrepancy.