853638 - Status and resolution values in dependency graphs not localizable

Status: RESOLVED FIXED

(Bugzilla :: Dependency Views, defect)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Status and resolution values in dependency graphs are not localizable. The attached patch fixes this.

Comment 1

[Frédéric Buclin]

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

>=== modified file 'showdependencygraph.cgi'

>+    my $stat_display       = display_value('bug_status', $stat);
>+    my $resolution_display = display_value('resolution', $resolution);

There is no need to define another variable for the bug status and resolution. $resolution is used nowhere else, and so you can simply reuse it to store the translated string. About $stat, you simply have to call display_value() after is_open_state() is called. This way, you can revert the changes made below.


>-        $resolution = $summary = '';
>+        $resolution_display = $summary = '';

I never saw this restriction before, but hidding the resolution doesn't make sense. When a restricted bug is mentioned in a comment, we always display the bug status and resolution. For consistency, I would suggest to remove this restriction here, and only hide the bug summary. This way, you could move both lines above after the call to is_open_state().


>-    $bugtitles{$k} = trim("$stat $resolution");
>+    $bugtitles{$k} = trim("$stat_display $resolution_display");

These changes are not needed, see above (and I don't like too long variable names anyway :)).


r=LpSolit with these comments fixed.

Comment 2

[Dave Miller [:justdave]]

Marc: do you plan to commit this or do you need someone to commit for you?

Comment 3

[Marc Schumann [:Wurblzap]]

I'd like to submit an updated patch for review. I'd like to fix the thing Frédéric mentioned. And I don't agree with his suggestion about variable names, so I'd like to keep this as-is in the new patch and ask for review again.

I'm clearing a+ for now.

Comment 4

[Marc Schumann [:Wurblzap]]

Attachment: 853638.diff

(In reply to Frédéric Buclin from comment #1)
> Comment on attachment 727898 [details] [diff] [review]
> Patch
> 
> >=== modified file 'showdependencygraph.cgi'
> 
> >+    my $stat_display       = display_value('bug_status', $stat);
> >+    my $resolution_display = display_value('resolution', $resolution);
> 
> There is no need to define another variable for the bug status and
> resolution. $resolution is used nowhere else, and so you can simply reuse it
> to store the translated string. About $stat, you simply have to call
> display_value() after is_open_state() is called. This way, you can revert
> the changes made below.

I'm convinced that variable names should reflect their content. If I'm re-using the variables here, then I'm violating this rule. After going through display_value, they're not status or resolution any more, but descriptive human-readable strings. See is_open_state() further down -- as you said, we'd need to wait for this to happen before we can re-use.
I'd rather keep it as it is, so the new patch still uses _display variables.

> >-        $resolution = $summary = '';
> >+        $resolution_display = $summary = '';
> 
> I never saw this restriction before, but hidding the resolution doesn't make
> sense. When a restricted bug is mentioned in a comment, we always display
> the bug status and resolution. For consistency, I would suggest to remove
> this restriction here, and only hide the bug summary. This way, you could
> move both lines above after the call to is_open_state().

Ok, I removed the restriction.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 788174 [details] [diff] [review]
853638.diff

Review of attachment 788174 [details] [diff] [review]:
-----------------------------------------------------------------

Little bitrot:

patching file Bugzilla/Util.pm
Hunk #1 FAILED at 21.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/Util.pm.rej

Make sure to fix on commit. r=dkl

::: showdependencygraph.cgi
@@ +176,5 @@
>      # Retrieve bug information from the database
>      my ($stat, $resolution, $summary) = $dbh->selectrow_array($sth, undef, $k);
>  
> +    my $stat_display       = display_value('bug_status', $stat);
> +    my $resolution_display = display_value('resolution', $resolution);

nit: I would move these two lines lower to right before they are needed.

@@ +214,4 @@
>      # Push the bug tooltip texts into a global hash so that 
>      # CreateImagemap sub (used with local dot installations) can
>      # use them later on.
> +    $bugtitles{$k} = trim("$stat_display $resolution_display");

I agree that variable names should not be needlessly long but I don't believe these fit into this category so am willing to let these go.

Comment 6

[Marc Schumann [:Wurblzap]]

Thanks for the review!
Nit addressed.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified showdependencygraph.cgi
modified Bugzilla/Util.pm
Committed revision 8701.