Closed Bug 904467 Opened 11 years ago Closed 10 years ago

error when %recipients is empty (e.g. after bugmail_recipients empties it)

Categories

(Bugzilla :: Email Notifications, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: dnozay, Assigned: dnozay)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

wrote an extension that clears the list of bugmail recipients for bulk updates; the gist is:

sub bugmail_recipients {
    my ($self, $args) = @_;
    my $recipients = @$args{qw(recipients)};
    # clear recipients
    %$recipients = ();
}



Actual results:

(note line numbers are from 4.2 not trunk)

Software error:

DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 2 [for Statement "SELECT watcher, watched FROM watch
                                  WHERE watched IN ()"] at Bugzilla/BugMail.pm line 200
	Bugzilla::BugMail::Send(1, 'HASH(0x3e45648)', 'HASH(0x4d004c8)') called at Bugzilla/Bug.pm line 1224
	Bugzilla::Bug::_send_bugmail('HASH(0x4d004c8)', 'HASH(0x488ec58)') called at Bugzilla/Bug.pm line 1163
	Bugzilla::Bug::send_changes('Bugzilla::Bug=HASH(0x4c923e8)', 'HASH(0x492add8)', 'HASH(0x488ec58)') called at /var/www/bugzilla.proximdata.com/process_bug.cgi line 385
For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error.

-----

code doesn't check for elements before using the empty %recipients in the db query.
please note that this code is fixed in BMO 4.2


Expected results:

"
email sent to:
    no one
"
Attached patch patch.diff (obsolete) — Splinter Review
trivial size check
Attachment #789419 - Flags: review?(justdave)
Blocks: 904482
Comment on attachment 789419 [details] [diff] [review]
patch.diff

>+    if (scalar keys %recipients) {

This is a hack. No developer will understand why this check is here when you know that %recipients always has at least two entries: the reporter and the bug assignee.

Moreoever, if your goal is to clear %recipients, then you will miss global watchers who will still get bugmails.
Severity: normal → minor
(In reply to Frédéric Buclin from comment #2)
> This is a hack. No developer will understand why this check is here when you
> know that %recipients always has at least two entries

Said differently: you must add a comment right above this line explaining why it's there.
Attached patch v2Splinter Review
Attachment #789600 - Flags: review?(LpSolit)
thanks for the review

(In reply to Frédéric Buclin from comment #2)
> Comment on attachment 789419 [details] [diff] [review]
> patch.diff
> 
> >+    if (scalar keys %recipients) {
> 
> This is a hack. No developer will understand why this check is here when you
> know that %recipients always has at least two entries: the reporter and the
> bug assignee.


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

any bugmail_recipients hook may do anything to the parameters passed by reference.

> 
> Moreoever, if your goal is to clear %recipients, then you will miss global
> watchers who will still get bugmails.

I fully understand that the global watchers are processed late. If "global watching" was a vanilla extension, it could inspect and augment %recipients in a bugmail_recipients hook. The only potential problem I see is the extension system which AFAIK doesn't let you specify an order to run hooks other than the one derived from the extension filenames lexical order.

Rather than addressing that, this is a "simple" patch to fix a broken assumption.
Attachment #789600 - Flags: review?(LpSolit) → review?(glob)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #789419 - Attachment is obsolete: true
Attachment #789419 - Flags: review?(justdave)
Assignee: email-notifications → damien.nozay
Comment on attachment 789600 [details] [diff] [review]
v2

r=glob

i'm happy with that comment; extension developers are expected to be able to understand both perl and bugzilla.
Attachment #789600 - Flags: review?(glob) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 5.0
Byron / Frédéric, would you mind committing on my behalf?
thank you.
Thanks for the patch!

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/BugMail.pm
Committed revision 8861.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: