bugmail_recipients hook does not include comments

RESOLVED WONTFIX

Status

()

Bugzilla
Extensions
--
enhancement
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Alex Schuilenburg, Unassigned)

Tracking

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 711771 [details] [diff] [review]
Provide comments as an additional parameter
Attachment #711771 - Flags: review?(mkanat)
(Reporter)

Comment 2

5 years ago
Created attachment 711777 [details] [diff] [review]
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.
Attachment #711777 - Flags: review?(mkanat)

Comment 3

5 years ago
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.
Attachment #711777 - Flags: review?(mkanat) → review-

Comment 4

5 years ago
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.
Attachment #711771 - Flags: review?(mkanat) → review-

Comment 5

5 years ago
Closing this bug as wontfix as you already can access comments using $bug->comments. There is no need to duplicate this information.
Severity: normal → enhancement
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 6

5 years ago
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?
(Reporter)

Comment 7

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.