278010 - checksetup.pl should fix empty group names

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Frédéric Buclin]

When viewing a bug when I'm logged out or using an account without privs, there
is no problem. Doing the same with an account having (admin) privs, show_bug.cgi
crashed with the error message I will attach.

No group is defined for the timetracking group. I just upgraded to the latest
version of 2.18 from CVS (after erik chechin).

This problem does not appear on the trunk. I haven't time to investigate...

Comment 1

[Frédéric Buclin]

Attachment: The error message I get

Comment 2

[Max Kanat-Alexander]

I can't reproduce this any way I try. I started with a blank directory, checked
out 2.18, and had checksetup create the database. I have one user (the admin),
and I can access bugs even when I:

+ File a new bug, without setting any params at all.
+ Edit the params, but leave timetrackinggroup blank.
+ Turn on timetracking group
+ Enter timetracking information and then turn off timetrackinggroup

I'm assuming that this is not actually a totally "fresh" installation of
Bugzilla, since you must have bugs in your DB in order to access show_bug.

Comment 3

[Frédéric Buclin]

OK, I have found the reason why I got this error:

As I said in IRC, this is related to the timetracking group which is empty in my
installations (both the 2.18rc3+ and 2.19.1+ ones). As I was testing my patch
for bug 277768, I had previously set a group with no name in my 2.18 branch
installation, where I did not apply my patch. [This is allowed per my comment in
bug 277768: no check is made when renaming a group so that you can delete it and
you get no error!]

So: when show_bug.cgi is called, you also call the Bug.pm and
default/bug/edit.html.tmpl files. And here is where the problem is:

From Bugzilla/Bug.pm, line 64:

    if (Param('timetrackinggroup')) {
        push @fields, qw(estimated_time remaining_time actual_time);
    }

As timetrackinggroup is empty (''), the test is wrong and you don't add these
three fields to the list (among which: remaining_time !)

    
From template/en/default/bug/edit.html.tmpl, line 94:

[% IF UserInGroup(Param('timetrackinggroup')) %]
  var fRemainingTime = [% bug.remaining_time %]; // holds the original value

Here, Param('timetrackinggroup') is still empty *but* a group with an empty
group name *exists* (the one with no name mentionned before)! Then, for all
users which are in this group without name (especially users in the admin group,
by inheritance), this test is *true* and we have to consider the
"bug.remaining_time" term which is not allowed by Bug.pm because remaining_time
has not been added to the field list, giving us the error message attached to
this bug.

Wow! :-p

Then, this bug is not a dupe of bug 277768 as it purpose is not to check group
names (among others), but is blocked by it.

Maybe should we add a patch here so that checksetup.pl checks if there is a
group with an empty name? This would be trivial to do.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> Then, this bug is not a dupe of bug 277768 as it purpose is not to check group
> names (among others), but is blocked by it.
> 
> Maybe should we add a patch here so that checksetup.pl checks if there is a
> group with an empty name? This would be trivial to do.
> 

Humm... to be more precise, bug 277768 is about editgroups.cgi and this one
would be about checksetup.pl. We could merge both in a single patch but I prefer
having two actually.

Comment 5

[Nick Barnes]

Attachment: checksetup.pl patch to fix empty group names

Patch for 2.18 tip checksetup.pl, to fix an empty group name.

Comment 6

[Dave Miller [:justdave]]

This should be fixed, but this is one of those situations that's not likely to
happen often, and the admin should know better ;)  I'll take it on the branch,
but this won't block release.

Comment 7

[Marc Schumann [:Wurblzap]]

Comment on attachment 171087 [details] [diff] [review]
checksetup.pl patch to fix empty group names

I can reproduce this on HEAD, too. The patch applies there.

Tested; works. I you don't wish to address my nits below, just request approval
and approval2.18.

>+        if ($trycount > 0) {
>+            $trygroupname .= "_$trycount";
>+        }
>+        $trygroupsth->execute($trygroupname);
>+        if ($trygroupsth->rows > 0) {
>+            $trycount ++;
>+        }

Nit: you could say if (++$trycount > 1) { $trygroupname .= "_$trycount"; } and
scrap the second if(){} block. (Numbering would start with 2 then, though.)

>+    $sth = $dbh->prepare("UPDATE groups SET name = ? WHERE id = $emptygroupid");
>+    $sth->execute($trygroupname);

Nit: these two can be merged into one $dbh->do().

Comment 8

[Nick Barnes]

Let's get this in.

Comment 9

[Max Kanat-Alexander]

This will have to be relnoted, so I'd rather put it in 2.18.1 as the relnotes
for 2.18 are already written and seem to have general approval.

Comment 10

[Shane H. W. Travis]

I'm not checking this in until the 2.18? is either approved or goes away (in 
case you're wondering what the holdup is).

Comment 11

[Shane H. W. Travis]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.330; previous revision: 1.329
done

Comment 12

[Max Kanat-Alexander]

This has been added to the release notes in bug 297590.