97956 - Give summary and URL of bugs added or removed from dependencies in bugmail (notification email)

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P4)

Description

[Matthew Tuck [:CodeMachine]]

When you add or remove bugs from CC, it would be nice if their summaries
appeared on the bug's notification.

Comment 1

[Matthew Tuck [:CodeMachine]]

Don't know what I was smoking when I filed this, it must have been good.  I of
course meant the dependencies fields.

Comment 2

[Matthew Tuck [:CodeMachine]]

*** Bug 115648 has been marked as a duplicate of this bug. ***

Comment 3

[Matthew Tuck [:CodeMachine]]

And links to the bugs too.

Comment 4

[Matthew Tuck [:CodeMachine]]

*** Bug 109846 has been marked as a duplicate of this bug. ***

Comment 5

[Matthew Tuck [:CodeMachine]]

OK, I'm sick of this one, I'll take a look at it some time.

Comment 6

[Jonas Jørgensen]

See also bug 125888, Give summary/URL of bugs marked as duplicates.

Comment 7

[Jonas Jørgensen]

See also bug 125905, Give summary/URL of bugs mentioned in comment.

Comment 8

[Boris 'pi' Piwinger]

Also see bug 113688. Maybe there can be a fix for both in one.

pi

Comment 9

[Peter Lairo]

Please remove the "blocks bug 169476". It is an incorrect assertion. Thus bug is
highly useful and not "annoying" at all.

Comment 10

[Boris 'pi' Piwinger]

Peter, you missread that. This bug here is annoying. A solution would stop that.

pi

Comment 11

[Mike Fedyk]

Just think, once this is fixed there will be a much lower load on the
web/database servers.  I regularly have to fire up a bug just to see what is
being depended on... :-/

Comment 12

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 255625 has been marked as a duplicate of this bug. ***

Comment 14

[Jesper Hertel]

Could the Summary of this bug please be changed to "Give summary and URL of bugs
added or removed from dependencies in bugmail (notification mail)"?

This would bake this bug easier to find - I was looking for 10 minutes in
Bugzilla before I found it.

I am not allowed to change it myself.

Comment 15

[Michael Lefevre]

updating summary. (the bug mail part is indicated by the component though)

Comment 16

[Jesper Hertel]

> bug mail part is indicated by the component though

Yes, but usually I just search for words in the subject, as it is not always
easy for me as an outsider to find the relevant component (in this case, I
could, but as I often fail in doing that, I don't do it anymore). This is why I
suggest adding "in bugmail (notification mail)" to the summary. Please tell me
the problem in adding that, as I can't see it.

Comment 17

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 18

[Wayne Mery (:wsmwk)]

mod summary

Comment 19

[Worcester12345]

(In reply to comment #17)
> Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. 

It's been a long freeze. (Cold winter?)

Comment 20

[Frédéric Buclin]

*** Bug 298886 has been marked as a duplicate of this bug. ***

Comment 21

[Max Kanat-Alexander]

2.20 is becoming a stable branch; it doesn't get any new features.

If this feature went in right this second, it would be targeted to 2.22.

Comment 22

[Worcester12345]

(In reply to comment #21)
> 2.20 is becoming a stable branch; it doesn't get any new features.
> 
> If this feature went in right this second, it would be targeted to 2.22.

I don't see that flag as one to target.

Comment 23

[Frédéric Buclin]

no activity + no patch from the assignee => default assignee

Comment 24

[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 29

[Josh Matthews [:jdm]]

Attachment: Patch

Here's my shot at improving things - bugs added or removed as dependencies or blockers now show up as "12345 - One bug summary per line".

Comment 30

[Josh Matthews [:jdm]]

Comment on attachment 451895 [details] [diff] [review]
Patch

I'm not a Perl hacker by any stretch of the imagination, so I'm expecting this to be a fabulous learning experience.

Comment 31

[Frédéric Buclin]

Comment on attachment 451895 [details] [diff] [review]
Patch

>+               "SELECT bugs.bug_id, bugs.short_desc
>+                FROM bugs
>+                WHERE bugs.bug_id IN (" .
>+                join(", ", @all_depbugs) . ")", "bug_id");

You bypass security checks. You could email the bug summary of bugs you are not allowed to access.

Comment 32

[Justin Dolske [:Dolske]]

How about a suggestion of where jsm might look to see how a security check here should be implemented?

Comment 33

[Max Kanat-Alexander]

$user->visible_bugs is usually the simplest way to do security checks, or $user->can_see_bug.

Comment 34

[Max Kanat-Alexander]

Also, it would probably be best to pass Bugzilla::Bug objects to the template. You can grab me on IRC if you have any questions. :-)

Comment 36

[Alex Faaborg [:faaborg] (Firefox UX)]

Copying my comment over from the recently duplicated bug:

I spend the bulk of my work day reading incoming bugmail, and the majority of
those messages are based on changes of dependency relationships, such as:


          What    |Removed                     |Added
----------------------------------------------------------------------------
        Depends on|                            |600196

People often set a large number of dependency relationships at the same time,
so I often encounter a sequence of 20 or 30 of these messages in a row.

Because the bug summary is not provided, I have two options:

1) Move on to the next piece of mail, assuming that if the dependent or
blocking bug was actually important someone will cc me. (However this often
isn't the case because the person changing the relationship believes this is
enough to put the new bug on my radar)

2) Actually look up the new bug to specifically see what it is about (which I
usually don't have time to do given the amount of incoming bugmail).

We really need to place the bug summary in these email notifications.  Ideally
we would also use a human readable sentence instead of an ASCII table, but
that's a secondary issue.

If we don't address this issue, we have to choose between accidentally ignoring
some bugs, or being considerably less efficient as we manage hundreds of
incoming bugmail messages.

Comment 37

[Guy Pyrzak]

So I'm not sure if this is the way max/lpsolit want this written but my suggestion for this bug is to use quoteUrls to add this capability, which makes this bug a dup of bug 113688.

thoughts?

Comment 38

[Frédéric Buclin]

Not a duplicate, no. Bug 113688 is about "bug NNN" in comments, while this bug is about "NNN" alone appearing in the diff table. Even if the backend code is the same, this remains two separate requests (which may or may not be fixed by the same patch).

I'm not sure it's that easy to do what mkanat suggests in bug 113688 comment 15, because I think it depends on the context (Perl script vs TT) quoteUrls() is called. But we will see when the patch is written.

Comment 40

[Mounir Lamouri (:mounir)]

Attachment: Patch (for text emails)

This patch is adding a summary of bugs after the REMOVED/ADDED table.
This is only working for text emails. I will try to do something for HTML emails later.

Comment 41

[Mounir Lamouri (:mounir)]

Attachment: Patch (for text emails)

Oups, it wasn't the right patch.

Comment 42

[Mounir Lamouri (:mounir)]

Attachment: Patch (for text emails)

Comment 43

[Ben Bucksch (:BenB)]

Mounir, Thanks!
This is one of bugzilla's most annoying bugs.

> I will try to do something for HTML emails later.

You could make a real HTML-table, and use the bug summary (or <bug #> + " - " + <summary>) instead of just the number, and then link it to the bug (number/ID). Several bugs added/removed would just be a list of linked summaries, separated by comma (not a link).

Comment 44

[Max Kanat-Alexander]

Nothing needs to be done for HTML emails, I believe; they should already have tooltips for each bug ID.

Comment 45

[Ben Bucksch (:BenB)]

tooltip is not sufficient (I need to manually mouse over each entry), and doesn't do justice to capabilities of HTML mail.

Comment 46

[Max Kanat-Alexander]

Okay. Well, this is a much lower priority now, though, since the data *is* in fact available in the standard email format.

Comment 47

[Ben Bucksch (:BenB)]

> this is a much lower priority now

I think you misunderstood. The patch for plaintext mails is just attached, not commited.

Comment 48

[Max Kanat-Alexander]

(In reply to comment #47)
> I think you misunderstood. The patch for plaintext mails is just attached, not
> commited.

  I am the Chief Architect of the Bugzilla Project, and I very much understood. :-) The primary emails of Bugzilla 4.2 are HTML, which have tooltips, which contain the data. So although it would possibly be nice to make that data more readily available, it's not as high priority as it was when this data could only be accessed by loading a webpage.

  If you want to talk about this more, feel free to grab me on IRC, and we can talk about it there. Don't change the priority again without discussing it with me, though.

Comment 49

[Mounir Lamouri (:mounir)]

(In reply to comment #44)
> Nothing needs to be done for HTML emails, I believe; they should already have
> tooltips for each bug ID.

AFAIK, they don't. They are shown as plain text.

Comment 51

[Frédéric Buclin]

(In reply to comment #49)
> AFAIK, they don't. They are shown as plain text.

Correct, they are shown as plain text. So HTML emails need to be fixed as well. And I agree that displaying bug summaries right below the table would make sense.

Comment 52

[Frédéric Buclin]

Comment on attachment 508139 [details] [diff] [review]
Patch (for text emails)

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

>+        if ($diff->{field_name} eq 'dependson' or $diff->{field_name} eq 'blocked') {
>+            $diff->{old} =~ s/\s+//;
>+            $diff->{old} = [split(',', $diff->{old})];
>+            $diff->{old} = Bugzilla::Bug->new_from_list($diff->{old});
>+
>+            $diff->{new} =~ s/\s+//;
>+            $diff->{new} = [split(',', $diff->{new})];
>+            $diff->{new} = Bugzilla::Bug->new_from_list($diff->{new});
>+        }
>     }

We shouldn't alter 'old' and 'new'. If we need extra data, then store them with another key, as I did for longdescs.isprivate.



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

>+    # When dependson or blocked fields are changed, an array of bugs is passed.
>+    if ($value and ($field eq 'dependson' or $field eq 'blocked')) {
>+         my @ids;
>+         foreach my $bug (@$value) {
>+             push(@ids, $bug->id);
>+         }
>+         return join(', ', @ids);
>+    }
>     return $value;
> }

This looks like a big hack. We shouldn't alter display_value() at all.



>=== modified file 'template/en/default/email/bugmail.txt.tmpl'

>+      [% FOREACH bug = change.old.merge(change.new) %]
>+        [% SET is_present = 0 %]
>+        [% FOREACH bug_in_list = bug_list %]

We are going to loop twice per user. This is far too much. Using hashes instead of arrays make things much easier.


>+    [% IF user.can_see_bug(bug.id) %]

Calling user.visible_bugs is more efficient as it queries the DB once for all bugs. Also, to avoid code duplication between the plain text and HTML templates, this check should be done in Bugzilla::BugMail::sendMail().

Comment 53

[Frédéric Buclin]

Attachment: patch, v2

I don't really plan to work on this bug, unless pyrzak and mkanat are fine with the design I propose. So I'm not taking this bug for now. :)

Comment 54

[Guy Pyrzak]

Comment on attachment 528726 [details] [diff] [review]
patch, v2

it is awkward to have the information about the deps/blocks in one place and have if they were added or removed in another place. Ideally they would be located in the same place, but that would probably require a good bit of reworking of the table, or not doing that and making the table very ugly, which would be unfortunate.

Comment 56

[Ben Bucksch (:BenB)]

Can we have this fixed finally, please? The bug is over 10 years old, and we had a patch a year ago. Please get it finished already.

Firefox dev uses many many dependency bugs recently to split up tasks, and this bug makes this pretty useless. I can't manually look up 30 sub-bugs (all added separately) for a single feature.

Sorry for the generalization, but the lack of action here is unbearable and completely symptomatic for bugzilla. Same arkwardness when making changes to bugs. I almost find myself wishing that mozilla.org would switch away from bugzilla. Other bugtrackers are so much more efficient, and it's small things like this.

Comment 57

[Ben Bucksch (:BenB)]

FWIW, the arkwardness of bugspam - and responses to bugspam - is so bad that it already starts to disrupt Firefox development, because nobody reads bugmail anymore.

Comment 58

[Frédéric Buclin]

(In reply to Ben Bucksch (:BenB) from comment #56)
> Sorry for the generalization, but the lack of action here is unbearable and
> completely symptomatic for bugzilla.

Why don't you jump in and help the Bugzilla community to fix this bug instead of complaining? Also, Mozilla has two developers working full time on b.m.o, why don't you ask MoCo to set this bug as high priority for their two devs?

Comment 59

[:glob ✱]

(In reply to Frédéric Buclin from comment #58)
> Why don't you jump in and help the Bugzilla community to fix this bug
> instead of complaining? Also, Mozilla has two developers working full time
> on b.m.o, why don't you ask MoCo to set this bug as high priority for their
> two devs?

bug 752400

Comment 60

[Justin Dolske [:Dolske]]

Bam! BMO bug filed, fixed, and deployed in 72 hours. That's some good work there, guys! <3

Comment 61

[Mounir Lamouri (:mounir)]

So what's the difference between bug 752400 and this one? Does that mean the feature has been implemented in bugzilla.mozilla.org only?

Comment 62

[Ben Bucksch (:BenB)]

Yes.

Comment 63

[Frédéric Buclin]

Too late for 4.4. We will retarget this bug once a patch is ready.

Comment 64

[Simon Green]

Attachment: v3 patch

This is not compatiable with BMO's version of the patch due to their extensions. It is semi compatible with 4.4 brc's code.

Comment 65

[Frédéric Buclin]

Comment on attachment 797597 [details] [diff] [review]
v3 patch

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

>+        if ($diff->{field_name} eq 'dependson' || $diff->{field_name} eq 'blocked') {
>+            push @referenced_bug_ids, grep { /^\d+$/ } split(/[\s,]+/, $diff->{old});
>+            push @referenced_bug_ids, grep { /^\d+$/ } split(/[\s,]+/, $diff->{new});
>+        }

Please run this code only once, before looping over each recipient, and pass the array to sendMail(). This code is user-independent and there is no need to recreate the array again and again for each recipient.

Comment 66

[Simon Green]

Attachment: v4 patch

Incorporates Frédéric's suggestion.

Comment 67

[Frédéric Buclin]

Comment on attachment 799284 [details] [diff] [review]
v4 patch

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

>+        if ($change->{field_name} eq 'dependson' || $change->{field_name} eq 'blocked') {
>+            push @referenced_bug_ids, grep { /^\d+$/ } split(/[\s,]+/, $change->{old});
>+            push @referenced_bug_ids, grep { /^\d+$/ } split(/[\s,]+/, $change->{new});
>+        }

Nit: they are all supposed to be bug IDs, so doing a grep to filter them is not required. Is it to exclude empty strings?


>+    my $referenced_bugs = Bugzilla::Bug->new_from_list([uniq @referenced_bug_ids]);

Nit: if @referenced_bug_ids is empty, there is no need to call new_from_list(). We already know the list will be empty.



>=== modified file 'template/en/default/email/bugmail.html.tmpl'

>+            [% FOREACH ref = referenced_bugs %]

Please write ref_bug instead of ref. That's easier to understand.


>+                [<a href="[% urlbase FILTER html %]show_bug.cgi?id=[% ref.id FILTER none %]">

Any reason why you don't call bug/link.html.tmpl instead of hardcoding the URL yourself?



>=== modified file 'template/en/default/email/bugmail.txt.tmpl'

>+[%+ "[" _ terms.Bug _ " " _ ref.id _ "] " _ ref.short_desc FILTER wrap_comment(76) %]

Any reason why you hardcode 76 instead of using the default COMMENT_COLS (80)? If the reason is to take the place taken by [Bug XXX] into account, then it should probably be shorter than that.


Your patch looks good and works fine. Just need some answers to my questions before granting review.

Comment 68

[Simon Green]

Attachment: v5 patch

Comment 69

[Simon Green]

This change incorporates the suggestions you made, and I've submitted a new patch. The code originally come from bug 752400 (the bmo version).

Comment 70

[Frédéric Buclin]

Comment on attachment 830664 [details] [diff] [review]
v5 patch

>=== modified file 'template/en/default/email/bugmail.html.tmpl'

>+                [[%+ "${terms.Bug} ${ref_bug.id}"

Nit: the + sign is useless as it follows a printable character.


r=LpSolit

Comment 71

[Simon Green]

(In reply to Matthew Tuck [:CodeMachine] from comment #5)
> OK, I'm sick of this one, I'll take a look at it some time.

11¾ years later, it is finally committed into trunk.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/BugMail.pm
modified template/en/default/email/bugmail.html.tmpl
modified template/en/default/email/bugmail.txt.tmpl
Committed revision 8813.

Comment 72

[Frédéric Buclin]

Added to relnotes for 5.0rc1.