622813 - Provide user objects to email sending decision hook (bugmail_recipients)

Status: RESOLVED FIXED

(Bugzilla :: Extensions, enhancement)

Description

[Gervase Markham [:gerv]]

The problem: I want (or rather, b.m.o. wants) to, notwithstanding any other settings, block all email to addresses which end in ".tld" or ".bugs". We would like to do this using hooks/extensibility mechanisms.

I don't want to do this in the bugmail_recipients hook because that would require going through the recipients hash and creating user objects for each ID, so we could look at the login names. As Bugzilla is about to do that, in a loop, a bit further down the code, that seems very wasteful.

I guess I could also hand-roll some SQL to search for user ids in <set> which match <regexp> and then walk the data structure to remove them, but that seems icky too.

Two possible nicer solutions (I'm sure there are more):

1) Allow hooking Bugzilla::User::wants_bug_mail
2) Change mailer_before_send hook in some way which allows the hook to cancel 
   sending cleanly (at the moment it can only change the message)

Max? What solution would you prefer?

Gerv

Comment 1

[Frédéric Buclin]

Why don't you simply turn on the disable_mail bit for all these user accounts?

Comment 2

[Gervase Markham [:gerv]]

Could do, but there's a lot of them, and people create new ones all the time, and it's a hassle to have to impersonate the user to disable the mail (and not everyone has that ability anyway) and currently we just solve the whole problem with a two-line patch to BugMail.pm. I'd just like to not have to do the two line patch.

Gerv

Comment 3

[Max Kanat-Alexander]

The bugmail_recipients hook is the time to decide who to send email to.

Perhaps a nicer solution would be to create user objects before the hook.

You could also have your extension override Bugzilla::User::disabledtext, although there might be times we search the DB directly for it too, in which case that wouldn't help.

Comment 4

[Gervase Markham [:gerv]]

We can create the User objects first, although we'd have to store them in a separate structure - which you might think is a little inelegant.

Like this:

my %users;

foreach my $id (keys %recipients) {
    $users{$id} = new Bugzilla::User($id);
}

Bugzilla::Hook::process('bugmail_recipients',
                        { bug => $bug, recipients => \%recipients,
                          diffs => $diffs, users => \%users });

...

foreach my $user_id (keys %recipients) {
    my $user = $users{$id} || new Bugzilla::User($user_id);

Would that be acceptable?

Gerv

Comment 5

[Max Kanat-Alexander]

  Hmm. Yes, as long as you use new_from_list instead of calling "new" in a loop, that should be fine. Also note that we already have %user_cache.

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Using new_from_list actually makes the code more complicated, because you have to extract a list of uncached users, call new_from_list, and then put the results back in the cache.

Gerv

Comment 7

[Max Kanat-Alexander]

Comment on attachment 501293 [details] [diff] [review]
Patch v.1

Oh, you're right. How about we just use "new"--after all, will there even ever be people in %recipients who aren't already in %user_cache?

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Yes, there are certainly circumstances where that is true.

Here is a patch which adds a simple loop to make sure the cache is complete. It's not doing extra work because we need a user object for all these people eventually.

Gerv

Comment 9

[Max Kanat-Alexander]

Comment on attachment 501331 [details] [diff] [review]
Patch v.2

Looks good to me. This needs a Bugzilla::Hook docs update now, also.

And why are you also adding "diffs" in this patch, isn't that a separate bug? (I have qualms about adding diffs that should be discussed elsewhere.)

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Docs added, diffs removed. Bug 616422 is the bug about adding a reference to the diffs; I would love to hear your objections :-) BMO is certainly finding it necessary to know that information for its implementation of this hook.

Gerv

Comment 11

[Max Kanat-Alexander]

Comment on attachment 501344 [details] [diff] [review]
Patch v.3

>+(But if you add additional recipients, you are not required to create and
>+add to this hash objects representing them.)

  Fix on checkin:

  (If you add additional recipients to the recipients hash, you are B<not> required to add them to this hash.)

Comment 12

[Max Kanat-Alexander]

Unfortunately, %user_cache doesn't exist on 4.0, so I can only approve this for trunk.

Comment 13

[Max Kanat-Alexander]

Oh, and make that "to the C<recipients> hash"

Comment 14

[Gervase Markham [:gerv]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/BugMail.pm
modified Bugzilla/Hook.pm
Committed revision 7648.

Gerv