783107 - New hook for whine.pl called whine_before_send to manipulate the query/bug data before mailing

Status: NEW

(Bugzilla :: Extensions, enhancement)

Description

[David Lawrence [:dkl]]

For BMO we needed to be able to sanitize bug data when being sent from whine.pl. So I created a hook called whine_before_send and added it in the beginning of the mail() function.

Patch coming

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add whine_before_send hook to whine.pl (v1)

Comment 2

[Frédéric Buclin]

Comment on attachment 652260 [details] [diff] [review]
Patch to add whine_before_send hook to whine.pl (v1)

>=== modified file 'extensions/Example/Extension.pm'

>+            $bug->{'is_open'} = $bug_obj->status->is_open ? 1 : 0;

I don't get it. How is that useful? This information is used nowhere. Shouldn't you also add a template hook in whine/mail.*.tmpl to benefit for it (e.g. to add new columns)?

Comment 3

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #2)
> Comment on attachment 652260 [details] [diff] [review]
> Patch to add whine_before_send hook to whine.pl (v1)
> 
> >=== modified file 'extensions/Example/Extension.pm'
> 
> >+            $bug->{'is_open'} = $bug_obj->status->is_open ? 1 : 0;
> 
> I don't get it. How is that useful? This information is used nowhere.
> Shouldn't you also add a template hook in whine/mail.*.tmpl to benefit for
> it (e.g. to add new columns)?

I was showing it was possible to update the data structure in a harmless way. For BMO we used it to simply filter bug attributes such as sumamry, etc. I will add some more to the templates as you suggest to show how adding extra data could be used to display in the email.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to add whine_before_send hook to whine.pl (v2)

New patch with better example of hook usage.

Comment 5

[Frédéric Buclin]

Comment on attachment 659385 [details] [diff] [review]
Patch to add whine_before_send hook to whine.pl (v2)

>=== modified file 'Bugzilla/Hook.pm'

>+on stored queries. This hook allows for an extension to manipulate the

s/for an extension//. All hooks are for extensions. :)


>+=item C<from>
>+
>+B<string> Bugzilla system email address

Remove it. All params are already available from templates using [% Param('foo') %] and so this should go away from whine.pl too (will do it in a separate bug).


>+=item C<title>
>+
>+B<string> Text title given to the current query in the whine event

s/Text//. You already said it's a string.


>+=item C<name>
>+
>+C<string> Text name of the current query

s/Text//. Same here.


>+=item C<schedule_id>
>+
>+C<int> Integer id of the schedule being run

s/Integer id/ID/. You already said it's an int(eger).



>=== modified file 'extensions/Example/Extension.pm'

>+            my $bug_obj = new Bugzilla::Bug($bug->{bug_id});

The extension now fails with:

Can't locate object method "new" via package "Bugzilla::Bug" at ./extensions/Example/Extension.pm line 907.

You must add |use Bugzilla::Bug| at the top of this module.



>=== added file 'extensions/Example/template/en/default/hook/whine/mail-query-bug-row.txt.tmpl'

>+Flags: [% bug.flags FILTER html %]

It's a plain text template. It must not have |FILTER html|. Also, the indentation is wrong with plain text emails:

  Priority: P5Severity: normal  : All
  Assignee: [e-mail]
    Status: NEW
   Summary: un autre test
Flags: approval-

I also note that the Severity row has no newline before it, which is a regression due to bug 451801 (Bugzilla 4.4 only). Do you want to fix it in this patch? Else I will reopen the other bug.


Otherwise looks good and works fine.

Comment 6

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #5)
> I also note that the Severity row has no newline before it, which is a
> regression due to bug 451801 (Bugzilla 4.4 only). Do you want to fix it in
> this patch? Else I will reopen the other bug.

FYI, this has been fixed as part of bug 245375, which also bitrots your patch.