395451 - Bugzilla::BugMail needs to use Bug objects internally instead of direct SQL

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Max Kanat-Alexander]

Right now Bugzilla::BugMail does all sorts of complicated things to populate %values in its Send() subroutine. All of this data could easily be gotten from a single Bugzilla::Bug object.

Once we have multi-select fields, this will be particularly important, since the values of multi-select fields aren't stored in the bug table, and thus won't show up in "New: " bugmails unless we do this.

Comment 1

[Max Kanat-Alexander]

Attachment: WIP

This is a work-in-progress. It compiles, but it almost certainly doesn't work yet.

Comment 2

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 3

[Reed Loden [:reed]]

Attachment: wip rev 2

This is blocking a bug I'm working on, so I'm going to take this.

Comment 4

[Reed Loden [:reed]]

Attachment: wip rev 3

runtests.pl passes fine with this. Still yet to actually test it, however. That's next.

https://landfill.bugzilla.org/bz395451/

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v1

This works based on my testing... I have a few more things to test, but going ahead and posting it for initial review.

Comment 6

[Reed Loden [:reed]]

Attachment: patch - v1.1

Noticed I wasn't updating $bug->{lastdiffed} after changing it in the DB, so fixed that.

One thing I did notice is that Bugzilla::BugMail::create() and Bugzilla::BugMail::update() modify `delta_ts` directly in the DB for other bugs (dependent / blocking). What happens to bug objects that might already be in memory somehow with a (now) obsolete `delta_ts` field? ... or would that never happen / not be possible? Figured I'd mention it, at least...

Comment 7

[Reed Loden [:reed]]

Comment on attachment 442652 [details] [diff] [review]
patch - v1.1

>+    my @fields = Bugzilla->get_fields({obsolete => 0});

One small optimization would be to add |in_new_bugmail => 1| to the get_fields() call, which would allow me to drop this check from a loop:

>+        foreach my $field (@fields) {
>+            # Skip if this field shouldn't be shown in new bugmail
>+            next unless $field->in_new_bugmail;

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> One thing I did notice is that Bugzilla::BugMail::create() and
> Bugzilla::BugMail::update() modify `delta_ts` directly in the DB for other bugs
> (dependent / blocking). What happens to bug objects that might already be in
> memory somehow with a (now) obsolete `delta_ts` field? ... or would that never
> happen / not be possible? Figured I'd mention it, at least...

  Those objects aren't re-used, so it's OK. There's no global cache of bug objects.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 442652 [details] [diff] [review]
patch - v1.1

This looks great. I'm really impressed with how awesome this patch is, actually. :-)

Comment 10

[Reed Loden [:reed]]

Attachment: what I landed

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified attachment.cgi
modified editusers.cgi
modified email_in.pl
modified importxml.pl
modified post_bug.cgi
modified process_bug.cgi
modified sanitycheck.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/BugMail.pm
modified Bugzilla/User.pm
modified Bugzilla/WebService/Bug.pm
modified contrib/sendbugmail.pl
modified extensions/Voting/Extension.pm
modified template/en/default/email/newchangedmail.txt.tmpl
Committed revision 7150.

Comment 11

[Tiago Mello [:timello]]

After this change I started getting the follow error:

"in_new_bugmail is not a valid parameter for the Bugzilla::Field::match function."

What should be the problem?

Comment 12

[Tiago Mello [:timello]]

(In reply to comment #11)
> After this change I started getting the follow error:
> 
> "in_new_bugmail is not a valid parameter for the Bugzilla::Field::match
> function."
> 
> What should be the problem?

When submitting a new bug, post_bug.cgi.

Comment 13

[Max Kanat-Alexander]

Added to the release notes in bug 604256.

Comment 14

[Max Kanat-Alexander]

Added to the release notes in bug 604256.