839449 - bugmail_recipients hook does not include comments

Status: RESOLVED WONTFIX

(Bugzilla :: Extensions, enhancement)

Description

[Alex Schuilenburg]

The bugmail_recipients hook does not include the comments within the diffs as documented in the URL provided.  For my plugin, I need the comments to help with the decision making.

The solution is simple. Either provide the comments as an additional parameter (preferred, since they are not strictly diffs), or append the comments to the diffs.

Comment 1

[Alex Schuilenburg]

Attachment: Provide comments as an additional parameter

Comment 2

[Alex Schuilenburg]

Attachment: Alternative patch to provides comments in diffs as documented

This is an alternative to attachment 711771 [details] [diff] [review].  It just tacks the comments onto the end of the diffs argument.

Comment 3

[Frédéric Buclin]

Comment on attachment 711777 [details] [diff] [review]
Alternative patch to provides comments in diffs as documented

>-                              users => \%user_cache, diffs => \@diffs });
>+                              users => \%user_cache, diffs => [@diffs, @$comments] });

$comments and $diffs do not have the same format at all. It's not ok to mess data passed to the hook.

Comment 4

[Frédéric Buclin]

Comment on attachment 711771 [details] [diff] [review]
Provide comments as an additional parameter

>+                              users => \%user_cache, diffs => \@diffs,
>+                              comments => $comments });

I don't see why you need to pass comments to the hook. You already have this information as the $bug object is available. You simply have to call $bug->comments to get what you want. Also, the documentation in Hook.pm must be updated every time a hook is changed.

For you information, mkanat is no longer a reviewer as he left the Bugzilla project almost a year ago.

Comment 5

[Frédéric Buclin]

Closing this bug as wontfix as you already can access comments using $bug->comments. There is no need to duplicate this information.

Comment 6

[Alex Schuilenburg]

Surely $bug->comments will give the list of _all_ comments associated with the bug, not just the new comments added?

What is wanted is $bug->comments({ after => $start, to => $end }) which has already been fetched and filtered (lines 112 and 113 of Bug/BugMail.pm), which is why I was proposing passing them into the hook.  Yes, sure, I could refetch these after restablishing $start and $end and refiltering, but that is duplicating effort and unnecessarily loading the DB.

Also, while I agree with comment #3 which is why I proposed my initial patch, that is not what is actually documented in
http://www.bugzilla.org/docs/tip/en/html/api/Bugzill/Hook.html#bugmail_recipients

That implies that comments are passed in diffs, so at the very least this documentation needs to be fixed.  Do I need to file another bug for this?

Comment 7

[Alex Schuilenburg]

OK, I grokked this at last and agree with the resolution.  I was barking up the wrong tree and had misunderstood the format of diffs and what it is intended for.  Sorry for the inconvenience.