Bug links look innocently unprotected. I keep falling into this trap. This should be easy if we teach GetBugLink to use a Bugzilla::Bug object. Adding the padlock implies a move to CSS classes as well, so bug_status and resolution classes can be added at the same time.

Looks good in both FF and IE. Bug lists continue working.

Comment on attachment 204523 [details] [diff] [review] Patch GetBugLink() has moved from globals.pl to Template.pm, and it's code changed a bit.

See also bug 329459.

(In reply to comment #3) > See also bug 329459. > Marc, what's the relation with this bug??

Nothing special in particular. Both cope with bug links, so somebody who is interested in one bug might potentially be interested in the other.

Unrotted.

Comment on attachment 215314 [details] [diff] [review] Patch 1.0.1 >Index: Bugzilla/Template.pm >+ if (Bugzilla->dbh->selectrow_array('SELECT count(bug_id) FROM bugs >+ WHERE bug_id = ?', >+ undef, $bug_num)) { Please move this SQL call outside the IF block and write COUNT(*) instead. >+ my $bug = new Bugzilla::Bug($bug_num, Bugzilla->user->id); As you are now using a bug object, this SQL query: my ($bug_state, $bug_res, $bug_desc) = $dbh->selectrow_array('SELECT bugs.bug_status, resolution, short_desc FROM bugs WHERE bugs.bug_id = ?', undef, $bug_num); which is before the one above is now useless. Remove it. >- elsif (! &::IsOpenedState($bug_state)) { There is a small bitrot here. It's now "!is_open_state($bug_state)". Nit: with this patch applied, the padlock on buglist.cgi is crossed out too (even when the padlock is not displayed). From a UI point of view, that not nice. It would be fine if you could fix this problem.

(In reply to comment #7) > Please move this SQL call outside the IF block and write COUNT(*) instead. I'm using count(*) now, though I'd be thankful if you told me why you wanted that change. For readability reasons, I kep the SQL call inside the IF bracket -- it belongs to the IF and is unrelated to code following it. > which is before the one above is now useless. Remove it. Gone. > There is a small bitrot here. It's now "!is_open_state($bug_state)". Fixed by generating a new patch -- this was in a line going away. > Nit: with this patch applied, the padlock on buglist.cgi is crossed out too > (even when the padlock is not displayed). From a UI point of view, that not > nice. It would be fine if you could fix this problem. Fixed. Btw, it worked in IE all the time while FF displays it as you describe. No idea which browser is right about it :)

(In reply to comment #8) > I'm using count(*) now, though I'd be thankful if you told me why you wanted > that change. Because I heard (maybe it was bbaetz) that the SQL query was executed faster when the DB server was free to choose which column(s) to use for the count, depending on the indexes, for instance. As you don't want something special in this count, both will return the same result.

Comment on attachment 218334 [details] [diff] [review] Patch 1.1 >Index: Bugzilla/Template.pm >- $title = $bug_state; >+ my $bug = new Bugzilla::Bug($bug_num, Bugzilla->user->id); >+ my $title = $bug->bug_status; I missed that in my previous review: if a user is logged out or isn't allowed to see the bug whose ID is $bug_num, your patch doesn't display the bug status and resolution anymore, and the padlock isn't displayed as well as only $bug->{'bug_id'} and $bug->{'error'} are defined (all other fields/methods return ""). The actual behavior is to let the user know the bug status and resolution in *all cases*. And from what you said, you would also like to display the padlock even if the user isn't allowed to access the bug. > if (Bugzilla->user->can_see_bug($bug_num)) { >+ $title .= ' - ' . $bug->short_desc; > } Bugzilla->user->can_see_bug() executes a new SQL query instead of using known data from $bug. If the user isn't allowed to see the bug, then $bug->short_desc is undefined, so you could write: if ($bug->short_desc) { $title .= ' - ' . $bug->short_desc; } >- return qq{$pre<a href="$linkval" title="$title">$link_text</a>$post}; >+ return qq{<a href="$linkval" title="$title" class="$classes_as_string">$link_text</a>}; Nit: Konqueror displays an empty tooltip when title="". Maybe should we drop it completely if it's empty. Firefox and Opera do not display any tooltip at all in this case. >Index: skins/standard/buglist.css >Index: skins/standard/global.css I don't know where the problem is, but with your patch applied, there is in some cases (when the window is small) no space between the bug number and the second column in buglist.cgi using Firefox. This makes the UI especially ugly.

We are in "soft freeze" mode to prepare 3.0 RC1.

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 "blocking3.2" tp "?", 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.

(In reply to comment #10) > I missed that in my previous review: if a user is logged out or isn't allowed > to see the bug whose ID is $bug_num, your patch doesn't display the bug status > and resolution anymore, and the padlock isn't displayed as well as only [...] > Bugzilla->user->can_see_bug() executes a new SQL query instead of using known > data from $bug. If the user isn't allowed to see the bug, then $bug->short_desc > is undefined, so you could write: These seem not to be correct (any more? It's been a long time), so I kept it as is. > Nit: Konqueror displays an empty tooltip when title="". Maybe should we drop it > completely if it's empty. Firefox and Opera do not display any tooltip at all > in this case. Fixed. Although it shouldn't be empty at any time any more, since Status and Resolution are in the title at all times. > I don't know where the problem is, but with your patch applied, there is in > some cases (when the window is small) no space between the bug number and the > second column in buglist.cgi using Firefox. This makes the UI especially ugly. There seems to be an issue with Firefox, not taking left padding into account when calculating the minimum column width. I added an equal amount of right padding, working around this.

Comment on attachment 303234 [details] [diff] [review] Patch 1.2 >Index: Bugzilla/Template.pm >+ if (Bugzilla->dbh->selectrow_array('SELECT count(*) FROM bugs >+ WHERE bug_id = ?', >+ undef, $bug_num)) { >+ my $bug = new Bugzilla::Bug($bug_num, Bugzilla->user->id); You should call new() before the IF test and check $bug->error. Also, new() no longer takes two arguments. >+ my $linkval = "show_bug.cgi?id=$bug_num"; Why do you define $linkval here? It's already defined later in the method and you never use it before its second definition. >+ push(@classes, 'bz_' . $bug->bug_status); Just one problem: bug statuses may have whitespaces, and probably and dangerous HTML tag in them. You should filter them. >+ push(@classes, 'bz_' . $bug->resolution); Adding this class seems to be a separate RFE to me. >+ foreach (@{$bug->groups()}) { >+ if ($$_{'ison'}) { Better is to write: return unless $_->{ison}. This way, you avoid another level of identation (more readable as there is no else{} condition). >+ $secure = 1; This variable is useless. It's used only once in if ($secure) and could be replaced by if ($secure_mode_implied || $secure_mode_manual). >+ if ($secure_mode_manual) { >+ push(@classes, 'bz_secure_mode_manual'); >+ } >+ elsif ($secure_mode_implied) { >+ push(@classes, 'bz_secure_mode_implied'); >+ } If both variables are defined, only the manual one wins. Any reason for that? >- # Prevent code injection in the title. No reason to remove this comment. >+ $title = $title ? qq{ title="$title"} : ''; This is useless. $title is always defined as it always contains at least the bug status. >Index: skins/standard/global.css >-.bz_obsolete { >- text-decoration: line-through; >-} >-.bz_inactive { >- text-decoration: line-through; Any reason to remove them? Note that I didn't check the CSS changes in details. I will check while testing your updated patch.

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved. I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.