Bug 486554

> I'd immediately, from that email, know what happened, so would need to hit
> bugzilla with two GETs.

... so would *not* need to hit bugzilla with two GETs.

Just to clarify do you mean show the aliases in the three column list of field changes? For example:

Brandon Perkins <bperkins> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bperkins
             Blocks|249459                      |SomeBugAlias
         AssignedTo|msuchy           |jpazdziora

Where the bug id is replaced by the bug's alias if one exists? Or do you mean the following

Brandon Perkins <bperkins> changed:

Summary: SomeBugSummary
Alias: SomeBugAlias

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bperkins
             Blocks|249459                      |456985
         AssignedTo|msuchy           |jpazdziora

Dave

I actually meant

Brandon Perkins <bperkins> changed:

Summary: SomeBugSummary
Alias: SomeBugAlias

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bperkins
             Blocks|249459 (SomeAlias)          |456985 (SomeOtherAlias)
         AssignedTo|msuchy           |jpazdziora

I don't think we want to remove the bugzilla numbers from those columns. I think it would be good to have both the number *and* the alias next to it.

Created attachment 333019 [details]
v1 for adding aliases to dependencies changes email notifications

hey Dave,

I think from Jan's first comment, he wanted to see the bug aliases next to bug ids of blocks/dependson in the removed/added columns of mail change notifications, as the example he put there. I have worked a patch to do this, applied to bz-web2 for testing please let me know what you think.

Noura

Comment on attachment 333019 [details]
v1 for adding aliases to dependencies changes email notifications

>Index: Bugzilla/BugMail.pm
>+        # REDHAT EXTENSIONS START 486554
>+        } elsif (exists($diff->{'fieldname'}) &&
>+            ($diff->{'fieldname'} eq 'dependson' || $diff->{'fieldname'} eq 'blocked')) {
>+             my $col_name;
>+             my @old_dependencies = split(/, ?/, $diff->{'old'});
>+             my @new_dependencies = split(/, ?/, $diff->{'new'});
>+             my @aliased_old = ();
>+             my @aliased_new = ();
>+             foreach my $dependency_bug (@new_dependencies) {
>+                 my $bug_obj = new Bugzilla::Bug($dependency_bug);
>+                 my $alias_string = $dependency_bug;
>+                 $alias_string .= "(" . join(", ", @{$bug_obj->alias}) . ")" if scalar @{$bug_obj->alias};
>+                 push @aliased_new, $alias_string;

Nit-pick: Please remove space in the @{$bug_obj->alias} join so that the text does not wrap there. 

$alias_string .= "(" . join(",", @{$bug_obj->alias}) . ")" if scalar @{$bug_obj->alias};

For example, one test email I viewed has the line break inside the alias list which looked awkward.

David Lawrence <dkl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |218983(TestAlias2,
                   |                            |TestAlias3)


Or do you think the line could be too long if we have many aliases for a single bug and maybe wrapping
at least will not allow the line to grow too much. 

>+             }
>+             foreach my $dependency_bug (@old_dependencies) {
>+                 my $bug_obj = new Bugzilla::Bug($dependency_bug);
>+                 my $alias_string = $dependency_bug;
>+                 $alias_string .= "(" . join(", ", @{$bug_obj->alias}) . ")" if scalar @{$bug_obj->alias};
>+                 push @aliased_old, $alias_string;
>+             }
>+             if (@aliased_old || @aliased_new) {
>+                 $col_name = "Depends on" if $diff->{'fieldname'} eq 'dependson';
>+                 $col_name = "Blocks" if $diff->{'fieldname'} eq 'blocked';
>+                 $diff->{'text'} = three_columns($col_name, join(", ", @aliased_old), join(", ", @aliased_new));

Leaving the extra space in the join here is ok though as it will put each bug id on it's own line with it's aliases.

For example:

David Lawrence <dkl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |218983(TestAlias2,TestAlias3),
                   |                            |435433(TestAlias4)


Otherwise looks good and works as expected. If you feel that leaving the extra space in the aliases themselves
so as to not let the line become too long then disregard my previous nit-pick.

This is ready to go in either case.
Dave

You guys rock.

I think that having the spaces there does not hurt -- if will wrap in the worst case. The information will be there and if someone is interested in that information, they should be able to parse wrapped lines. ;-)

Thanks,

Jan

Thanks for the review Dave, I think it is tidier not to let it wrap for single bug aliases, and make it wrap for different bugs. so i changed it as you suggested. all committed now to cvs.

Noura